Repository navigation
Conversation
Gentleman-Programming#1896 slice 1. No schema change: both timing keys already exist in `gentle-shell.notifications/v1`. The card's Enter cycle was the fixed constant `[null, ...builtins]`. A `file:` reference is never a member, so `indexOf`/`findIndex` returned -1, `(-1 + 1) % 4` selected silence, and the user's own sound was unreachable afterwards. The toast still said "Audio notification preferences saved." - Make the cycle per row: `cycleFor()` appends the local reference the row owns and `ownedFile()` keeps it remembered while the row is silenced, so Enter reads `file -> silence -> success tone -> error tone -> attention tone -> file` and returns to the assignment. The row also re-reads its own file from the current value, so a reference replaced with `f` cannot come back from the memory. - Expose the two global timing keys (`minimumIntervalMs`, `coalesceWindowMs`) as card rows. Enter opens an inline field prefilled with the current value; only a whole number inside the schema range is saved, anything else is refused with a notice and writes nothing. `0` still disables that timing window. The issue sketched the cycle as `[null, ...BUILTIN_NOTIFICATION_IDS, ...(current?.startsWith("file:") ? [current] : [])]`, which recomputes the cycle from the current value alone: entering from a file it still selects silence, and the file is unreachable on every later press. That is the same outcome as the bug. The remembered per-row reference is what makes the stated goal ("an assignment is never discarded") hold, so the cycle is per row as the requirement says rather than as the sketch spells it. The remembered reference lives in the open card; reopening `/gentle:customize` rebuilds the rows, so a row silenced before closing reads as silence again. Persisting a set of own sounds is the library that stays in slice 2, together with the `/v2` decision and the mixed-group question. Verified on Windows, node 22.23.0: - tests/notification-customize.test.ts: 41/42; the only failure is the pre-existing `f opens an inline field ...` case, which writes a POSIX-style path the native win32 path flavor refuses and fails identically without this change. - notification + customize-view group: 24 pre-existing failures / 150 pass / 174 tests, against 24 / 146 / 170 for the same command without this change. Same failure set, 4 new tests all green, no new failure. - `pnpm run typecheck`: 186 recorded diagnostics, no regressions. Refs Gentleman-Programming#1896
Gentleman-Programming#1896 slice 2 reduced to what the user asked for: a persisted library of own sounds, saved with `Ctrl+S` from the path field, walked by each row's `Enter` cycle. Specs S1-S8 quote the request verbatim; the persistence decision (a separate file, so `notifications.json` stays `/v1`) is recorded in the log with its evidence. Refs Gentleman-Programming#1896
Saved own sounds need somewhere to live before `Enter` can rotate among them.
`notifications.json` validates its keys exactly, so a library key inside
`audio` would make every build that does not know it classify the audio
configuration as malformed and offer "Replace invalid audio configuration?".
`lib/notification-sounds.ts` stores the library beside the audio configuration
under its own schema (`gentle-shell.notification-sounds/v1`), so the existing
file and its `/v1` contract do not change and older builds simply ignore it.
- Strict shape: `{ schema, sounds: [{ path }] }`, one key per entry, and every
path re-validated with the same absolute-path policy the sound assignments
use. An unknown schema, an extra key or an invalid path invalidates the whole
file instead of being partially accepted.
- One entry per file: the comparison key folds Windows case and separators and
stays case-sensitive on POSIX, while the stored entry keeps the user's own
spelling.
- Bounded at eight entries: a full library refuses a new sound and reports it
rather than dropping one the user saved. A missing file is an empty library
that is never created by a read.
- The write is atomic (same-directory exclusive `0600` temporary plus rename)
and validates the whole list before any filesystem call.
`notification-policy.ts` now exports its `DEFAULT_IO` and `nativeFlavor`: the
writer and the path policy are security-relevant seams that must have exactly
one definition rather than a copy.
Verified on Windows, node 22.23.0:
- tests/notification-sounds.test.ts 9/9.
- `pnpm run typecheck`: 186 recorded diagnostics, no regressions.
Refs Gentleman-Programming#1896
The inline bridge resolved a field to its text or to cancellation, so a row
could not tell "the user pressed Enter" from "the user asked to save this
value". `CustomizeInline.input` now resolves `{ value, save }`, the view
intercepts `ctrl+s` (`\x13` and the terminal's CSI-u sequence, and the shortcut
never becomes text), and the field footer advertises `Ctrl+S save` only when
the request asks for it with `allowSave`. A field that is not fully visible
cannot submit through the shortcut, exactly like Enter.
No row uses the flag yet: the notifications card only adapts to the new result
shape, so its behavior is unchanged in this commit.
Verified on Windows, node 22.23.0:
- tests/visual-customize-view.test.ts 42/42, including the new shortcut, hint
and invisible-field cases.
- tests/notification-customize.test.ts 41/42: the only failure is the
pre-existing `f opens an inline field ...` case, which writes a POSIX-style
path the native win32 path flavor refuses.
- `pnpm run typecheck`: 186 recorded diagnostics, no regressions.
Refs Gentleman-Programming#1896
The saved-sounds file had no writer and no reader. `Ctrl+S` in the path field now adds the already-validated sound to it and assigns it, `Enter` keeps assigning without saving, and every row's `Enter` cycle walks `silence -> included tones -> saved sounds in saved order -> the row's own unsaved file`. - The cycle takes the saved list first and appends the row's own reference only when the library does not already carry it. Ordering it the other way let a saved sound jump the queue as soon as a row walked over it; the new cycle test caught exactly that. - A save reports what happened: saved, already saved, refused because the list is unreadable (the card names the file and never overwrites it), or refused because the list is full. A failed save still assigns the sound in the same action, so the user is never left without the sound they picked. - The library is read once per card and refreshed in memory after a successful save, so rendering a row never touches the filesystem. Docs: `docs/sound-notifications.md` gains a "Saved sounds" section (path, schema, strict shape, one entry per file, the eight-sound bound, the atomic write, and why `notifications.json` stays `/v1`); the per-row memory note now says what survives closing the card. README and the technical reference carry one clause each. Verified on Windows, node 22.23.0: - notification group: 24 pre-existing failures / 170 pass / 194 tests, against 24 / 146 / 170 before this feature. The 24 are the 23 failures in files this change does not touch plus the known `f opens an inline field ...` case, and all 20 new tests pass. - `pnpm run typecheck`: 186 recorded diagnostics, no regressions. Refs Gentleman-Programming#1896
T1-T3 landed as `ebb9faea0`, `c1e74ec6a` and `7fe52d6d7`; the log records the RED observed per unit, the cycle-order bug the new test caught, the platform baseline this change does not move, and what stays out of scope (per-sound volume, the `/v2` move and the mixed-group question). Refs Gentleman-Programming#1896
The library tests used Windows paths against the default path flavor, which is `posix` off Windows, so `writeSavedSounds` and `resolveSavedSounds` rejected them and four tests failed on Linux. The flavor is now an explicit input: `win32` for the documents that carry Windows paths and a new `posix` round-trip case for the other branch. The full-library card test seeds platform-native paths for the same reason, and the card keeps using the native flavor, which is what production does. Verified with node 24 on both platforms: - Linux (the platform CI runs the full suite on): notification group 195/195 and tests/gentle-shell.test.ts 263/263. - Windows: 102 tests with the single pre-existing `f opens an inline field ...` failure, an environment artifact of this machine (the case writes a POSIX path the native win32 flavor refuses) that CI's Windows job does not run. Refs Gentleman-Programming#1896
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughPriority: ➖ Normal Change: Feature Merge Risk: ⚪ Minimal · up to No actionable issue identified here prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Direct issue Resolution Implement per-sound volume as an integer percentage with validation and persistence. Apply attenuation through the specified player routes, including PCM gain where the route has no volume parameter, and add automated coverage. Resolve and document mixed-group Enter behavior, with automated coverage, before treating the saved-sounds library as complete. Full details: Docstring CoverageExplanation Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 7 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ation The branch is pushed and the PR is open against `main` with the design question about the library file called out. The log also records what running the tests on Linux changed: the saved-sounds tests were pinning the wrong path flavor, which would have failed CI. Refs Gentleman-Programming#1896
The log recorded a commit count that the very commit recording it changes, so it now names the surface instead. Refs Gentleman-Programming#1896
Contributor
|
Thorough work on this slice @Fivoryu. Regarding your design question on schema bump vs separate file: Keeping
A few quick observations on the implementation:
Code looks clean, robust, and well-covered by tests. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Closes #1896
Triage note: #1896 is still untriaged (no labels), so it does not carry
status:approved, and my account hasREADon this repository, so I cannot label it. This PR is therefore opened for triage rather than after approval; the issue body already describes both parts.PR type
type:feature)The first commit is the bug fix the issue asks for in slice 1 and the rest is slice 2 without per-sound volume, so please apply whichever single
type:*label fits your triage.Summary
Entercycle per row: a row that owns a local file keeps it as the last step of its own cycle, so the firstEnterno longer discards the assignment for good.minimumIntervalMs,coalesceWindowMs) as card rows:Enteropens an inline field and only a whole number inside the schema range is saved instead of being clamped.Ctrl+Sinside the path field and walked by every row's cycle:silence → included tones → saved sounds in saved order → the row's own unsaved file.Changes
lib/notification-customize.tsCtrl+Ssaving through the already-validated path.lib/visual-customize-view.tsCustomizeInline.inputresolves{ value, save }; the field interceptsctrl+sand advertisesCtrl+S saveonly when the request asks for it.lib/notification-sounds.tsnotifications-sounds.json.lib/notification-policy.tsDEFAULT_IOandnativeFlavorso the new writer reuses one definition of the path policy and of the filesystem seam.tests/notification-sounds.test.tstests/notification-customize.test.tsCtrl+Ssave/duplicate/full/unreadable cases, and the eight-control card.tests/visual-customize-view.test.tsdocs/sound-notifications.md,README.md,docs/readme-reference.mdodd/tasks/notifications-saved-sounds.mdDesign decision that needs your call
#1896 proposes the library inside the audio schema (
audio.sounds) with a/v1 → /v2bump.lib/notification-policy.tsvalidates the keys ofaudioexactly, so such a key makes any build that does not know it classify the configuration as malformed and offer "Replace invalid audio configuration?" — which for a shared or downgraded configuration can discard assignments. This PR keepsnotifications.jsonat/v1and writes the library to a second file under its own schema instead: older builds ignore it, and the assigned sound stays afile:reference, so audio keeps working even if that file is removed. If you prefer the schema route, the store is one module and migrating from the file is bounded.Test plan
pnpm run typecheck: 186 recorded diagnostics, no regressions.notification-sounds,notification-customize,notification-audio,notification-policy,notification-scheduler,notification-service,gentle-notifications,notification-ui,visual-customize-view): 195 passed, 0 failed.tests/gentle-shell.test.tson Linux: 263 passed, 0 failed, including the customize overlay flow.node scripts/verify-package-files.mjs: 196 files, 69 byte-pinned artifacts.pnpm run check:runtime-modules: matches the TypeScript sources.git diff --check: clean.pnpm testin full andpnpm run test:packed-packagelocally: not run. The first needs a Linux toolchain this machine does not have and the second would shell out to a Windowsnpmunder a Linux node, so both are left to CI.Entercycle walks back to the assigned file (the reporter of the issue verified it beforeCtrl+Sexisted). Physical listening stays unverified, as the current documentation already records.Contributor checklist
status:approved. feat(notifications): per-sound volume, saved-sounds library, and a cycle that keeps an assigned file #1896 is untriaged and this account cannot label it.Co-Authored-Bytrailer.Scope and review
The branch touches eleven files, all inside the notification surface. Nothing else moves: no change to the
notifications.jsonschema, no new dependency, no player, backend or scheduler change, and no shell script (shellcheck is not applicable). Out of scope on purpose: per-sound volume, the/v2schema move, and the mixed-group question the issue records —Enteron a mixed type still applies that type's recommended tone.Summary by CodeRabbit