Feat/play a completion sound - #1217
oskarwojciski wants to merge 5 commits into
Conversation
|
Documentation Changes Added
Actions
If neither actions are selected, on PR close/merge the docs branch in ReadMe will remain open. |
Kimchi Code Review
Summary📊 Review Score: 78/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Good unit coverage for both new modules: the 📝 Found 7 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 78/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Good unit coverage for both new modules: the shouldPlaySound decision matrix, getSoundSettings parsing (missing/invalid JSON, unknown mode, blank soundFile), platform-specific playSound behavior with injected spawn/existsSync fakes, and thorough FocusEventFilter chunking tests including sequences split across chunk boundaries. Gaps: no test exercising a ~-prefixed soundFile (which silently fails in production), and the top-level vi.mock("node:child_process")/spawnMock in done-sound.test.ts is dead code never asserted on since all playback tests use the injected spawnImpl.
📝 Found 7 issue(s). See inline comments for details.
| * The audio file to play: the user's `soundFile` when set, otherwise a | ||
| * platform default (none on platforms without a default). | ||
| */ | ||
| export function resolveSoundFile( |
There was a problem hiding this comment.
A soundFile of "~/ding.wav" is never tilde-expanded — neither resolveSoundFile nor spawn expands ~ (that is shell behavior), so existsSync("~/ding.wav") returns false and the user silently gets the terminal bell instead of their custom sound. The test suite even stores "~/work/ding.wav" in a fixture as if it were a valid path, which makes the gap easy to ship unnoticed.
💡 Suggestion: Expand a leading ~/ (or bare ~) to homedir() in getSoundSettings or resolveSoundFile, e.g. file = file.startsWith("~/") ? join(homedir(), file.slice(2)) : file, and add a test asserting the expansion.
|
|
||
| function tryNextPlayer(spawnImpl: SpawnFn, players: Player[], file: string, index: number): void { | ||
| if (index >= players.length) { | ||
| bell() |
There was a problem hiding this comment.
Spawned player processes are never unref'd. Without unref(), Node keeps the child-process handle referenced, so the CLI cannot exit until the audio finishes playing — a long or hung custom sound file (or a stuck mpv) will delay process exit after the user has otherwise finished. The same applies to the afplay and PowerShell branches in playSound.
💡 Suggestion: Call child.unref() after each spawnImpl(...) in tryNextPlayer and in the darwin/win32 branches of playSound (guard with typeof child.unref === "function" so the injected test fakes keep working).
| if (i === n - 1 && data[i] === ESC) break | ||
| if (i === n - 2 && data[i] === ESC && data[i + 1] === "[") break | ||
| out += data[i] | ||
| i += 1 |
There was a problem hiding this comment.
A trailing bare ESC is held in pending indefinitely until the next input chunk arrives. When the user presses the Escape key alone — the standard cancel/interrupt key in TUIs — the byte is swallowed by extractFocusEvents and the TUI never sees it until some other key is pressed. This makes Escape appear dead; combined with the eager install in done-sound.ts, it affects every user on a focus-capable terminal even though the sound feature defaults to off.
💡 Suggestion: Flush pending after a short escape-timeout (e.g. 25–50 ms via setTimeout(...).unref() armed when a trailing ESC/ESC[ is captured, emitting the held bytes if no continuation arrives), matching how TUIs themselves disambiguate a lone Escape from a CSI prefix.
| export default function doneSoundExtension(pi: ExtensionAPI): void { | ||
| // Arm focus tracking eagerly so a mode flip via settings.json works | ||
| // without restart. No-op when the terminal can't report focus. | ||
| installFocusTracking() |
There was a problem hiding this comment.
installFocusTracking() is called eagerly at extension registration even though the default mode is "off", meaning every user gets stdin.emit monkey-patched and focus-reporting escape sequences enabled — with all the associated input-handling edge cases — for a feature they did not enable. A live settings.json edit could still be honored by arming lazily on the next agent_end.
💡 Suggestion: Defer installFocusTracking() until the first agent_end where getSoundSettings().mode === "agent-end-without-focus" (cache the result so the install happens once); "off" and "agent-end" modes never need focus state at all.
| const chunk = args[0] | ||
| const text = typeof chunk === "string" ? chunk : Buffer.isBuffer(chunk) ? chunk.toString("utf8") : null | ||
| if (text !== null && filter) { | ||
| const cleaned = filter.feed(text) |
There was a problem hiding this comment.
The wrapped stdin.emit changes the chunk type mid-stream: when focus bytes are stripped it emits a string (cleaned) while all other chunks remain Buffers — consumers doing Buffer.isBuffer checks or byte-length accounting will see inconsistent types. Additionally, chunk.toString("utf8") mangles a multi-byte UTF-8 character split across reads if a focus event happens to share that chunk (its tail becomes U+FFFD and the next chunk's continuation bytes are also corrupted).
💡 Suggestion: Re-encode the cleaned output to a Buffer when the original chunk was a Buffer (Buffer.from(cleaned, "utf8")), and use string_decoder to hold incomplete multi-byte tails instead of Buffer.toString("utf8") so split characters survive intact.
|
|
||
| /** Disarms focus reporting. Safe to call when tracking was never enabled. */ | ||
| export function disableFocusTracking( | ||
| write: (s: string) => void = (s) => { |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
disableFocusTracking writes the disable sequence but leaves stdin.emit wrapped and installAttempted/trackingEnabled set — the module cannot be cleanly re-armed or fully uninstalled, which limits reuse outside the single process-exit path it is currently wired to.
💡 Suggestion: Restore the original stdin.emit, reset trackingEnabled/installAttempted/filter, and null out the captured references when disabling, or document explicitly that disarm is process-lifetime final.
| import type { SoundSettings } from "./done-sound.js" | ||
| import { getSoundSettings, playSound, resolveSoundFile, shouldPlaySound } from "./done-sound.js" | ||
|
|
||
| vi.mock("node:child_process", () => ({ |
There was a problem hiding this comment.
ℹ️🧪 Testing
The top-level vi.mock("node:child_process") and the spawnMock fetched in the afplay test are never asserted on — every playback test passes its own injected spawnImpl, so the module mock is dead code that suggests coverage it doesn't provide.
💡 Suggestion: Delete the vi.mock block and the unused spawn import/mockClear in the first playSound test, or drop the dependency injection and assert on the mocked spawn directly so the mock earns its place.
1 participant
What
An opt-in notification sound when the agent finishes responding — useful
when the terminal is in the background. Configured in
~/.config/kimchi/harness/settings.json:soundvalue"off"(default)"agent-end""agent-end-without-focus"Plus optional
"soundFile"for a custom audio file; both re-read on everyresponse (live edits, no restart).
Notes
off).agent-end-without-focusrelies on focus reporting; Terminal.app andtmux (without
set -g focus-events on) can't report, so there it playsrather than silently miss.