Conversation
- Subscribe to OpenTUI paste events via renderer.keyInput - Filter out keypresses with modifiers (ctrl/meta/alt/super/hyper) - Use proper KeyEvent type from @opentui/core Fixes paste not working in TUI chat input and prevents system shortcuts (like voice dictation) from inserting stray characters.
- `ralph-tui` with no args now behaves like `ralph-tui run` - Removed unused launchTui() function and App import - Updated help text to reflect new default behavior Previously, running `ralph-tui` without arguments launched a bare TUI shell that wasn't useful. Now it starts the actual task orchestration, making the CLI more intuitive.
Add cross-platform desktop notification support using node-notifier ^10.x. Create src/notifications.ts with sendNotification function that accepts title, body, and optional icon. Errors are handled gracefully via warning logs to prevent crashes from notification failures.
- Add notifications.enabled boolean to config schema (default: true) - Add --notify CLI flag to force enable notifications - Add --no-notify CLI flag to force disable notifications - CLI flags override config file setting - Create resolveNotificationsEnabled() helper to resolve final state
Add a boolean toggle for desktop notifications in the SettingsView. The toggle reads from config.notifications.enabled (defaulting to true when absent) and persists changes to config.toml when saved.
- Add sound mode config: 'off', 'system', or 'ralph' (Wiggum quotes) - Create cross-platform sound player (afplay/paplay/PowerShell) - Bundle 8 iconic Ralph Wiggum WAV clips (~480KB) - Add sound option to Settings TUI - Fix config merge missing autoCommit, notifications, and other fields - Fix session version warning for pre-versioning files
WalkthroughAdds desktop notifications and sound playback, extends config/schema/types for notifications, threads notification options through run flows, makes CLI default to run when no subcommand is given, updates TUI settings and input handling, and improves session persistence version handling. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (cli.tsx)
participant Config as Config Resolver
participant Engine as Run Engine
participant Notifier as Notifications
participant Sound as Sound Driver
User->>CLI: run [--notify / --no-notify]
CLI->>Config: resolve notifications (stored config + CLI)
Config-->>CLI: NotificationRunOptions
CLI->>Engine: executeRunCommand(notificationOptions)
Engine->>Engine: start run (track start time)
loop processing
Engine->>Engine: process tasks/iterations
end
alt Completion
Engine->>Notifier: sendCompletionNotification(duration, taskCount)
else Max iterations reached
Engine->>Notifier: sendMaxIterationsNotification(iterations, remaining, duration)
else Error
Engine->>Notifier: sendErrorNotification(errorSummary)
end
Notifier->>Config: determine enabled & soundMode
alt Notifications enabled
Notifier->>Sound: playNotificationSound(soundMode)
Sound->>Sound: select backend & play file
end
Notifier->>User: show desktop notification (node-notifier)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @package.json:
- Line 56: The package.json lists node-notifier@10.0.1 while
@types/node-notifier is at v8, causing a major-version type mismatch; inspect
the runtime package node-notifier to see if it ships its own TypeScript
declarations for v10 or if v8 @types adequately covers v10 APIs, and then either
(a) upgrade/downgrade the package(s) so majors align (e.g., update
@types/node-notifier to a matching major if available or pin node-notifier to a
version that matches v8 types), or (b) remove the @types entry if node-notifier
includes its own types and adjust import/use sites if necessary; update
package.json accordingly and run TypeScript compilation/tests to confirm type
compatibility for the symbols using node-notifier.
In @src/sound.ts:
- Around line 239-243: The default branch of the switch declares const
_exhaustive: never = mode; which can leak between cases; wrap the default case
body in its own block (e.g., default: { const _exhaustive: never = mode; return
_exhaustive; }) so the _exhaustive identifier is block-scoped and cannot be
accessed by other cases.
- Around line 94-109: The Windows branch in the case 'win32' block interpolates
filePath directly into the PowerShell command (spawn, proc, filePath), creating
a command injection risk; fix it by removing direct interpolation and instead
either (a) pass the path as a separate argument to PowerShell and reference it
as a parameter inside the -Command/scriptblock, or (b) reliably escape the path
(e.g., double single quotes) before inserting it; update the spawn call to use
argument separation rather than embedding filePath into the command string so
that untrusted characters cannot break out of the command.
🧹 Nitpick comments (5)
package.json (1)
18-18: Cross-platform build script may fail on Windows.The
cp -rcommand is Unix-specific and will fail on Windows systems. Since this is a Bun project and targets Bun runtime, consider using a cross-platform alternative.💡 Suggested fix using Bun's shell or cross-platform tooling
If Windows build support is needed, you could use a cross-platform copy utility or a small Node.js script:
- "build": "bun build ./src/cli.tsx --outdir ./dist --target bun --sourcemap=external && bun build ./src/index.ts --outdir ./dist --target bun --sourcemap=external && cp -r assets dist/", + "build": "bun build ./src/cli.tsx --outdir ./dist --target bun --sourcemap=external && bun build ./src/index.ts --outdir ./dist --target bun --sourcemap=external && node -e \"require('fs').cpSync('assets', 'dist/assets', {recursive: true})\"",src/sound.ts (2)
18-31: Path detection logic could match unintended directories.The check
currentDir.includes('/dist/')might incorrectly match paths like/home/dist/myproject/src/. Consider using a more specific check.♻️ Suggested improvement
function getSoundsDir(): string { // Handle both development and bundled scenarios const currentFile = fileURLToPath(import.meta.url); const currentDir = dirname(currentFile); // In dev: src/sound.ts -> ../assets/sounds // In dist: dist/cli.js -> assets/sounds (copied during build) // Check if we're in dist by looking at the path - if (currentDir.endsWith('dist') || currentDir.includes('/dist/')) { + if (currentDir.endsWith('dist') || currentDir.endsWith('/dist') || currentDir.includes('/dist/src')) { return join(currentDir, 'assets', 'sounds'); } // Development: src directory return join(currentDir, '..', 'assets', 'sounds'); }Alternatively, consider checking for the existence of the assets directory at each candidate path to determine the correct location.
173-188: Apply the same path escaping for Windows system sound.For consistency and safety, the Windows
playSystemSoundimplementation should also escape paths, although in this case the path is hardcoded.The current implementation is safe since the command doesn't use user-controlled paths, but consider this for future maintainability.
src/commands/run.tsx (1)
1076-1111: Consider extracting notification logic to reduce duplication.The notification handling in headless mode (lines 1076-1111) duplicates the logic from TUI mode (lines 773-814). While acceptable for this PR, consider extracting a shared helper function in future refactoring.
src/notifications.ts (1)
111-116: Consider handling negative durations.The
formatDurationfunction doesn't handle edge cases like negative values or very large numbers. While unlikely in practice, defensive handling could prevent unexpected output.♻️ Optional defensive improvement
export function formatDuration(durationMs: number): string { + if (durationMs < 0) { + return '0m 0s'; + } const totalSeconds = Math.floor(durationMs / 1000); const minutes = Math.floor(totalSeconds / 60); const seconds = totalSeconds % 60; return `${minutes}m ${seconds}s`; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
assets/sounds/choc.wavis excluded by!**/*.wavassets/sounds/daddy.wavis excluded by!**/*.wavassets/sounds/feel.wavis excluded by!**/*.wavassets/sounds/funny.wavis excluded by!**/*.wavassets/sounds/icecream.wavis excluded by!**/*.wavassets/sounds/idunno.wavis excluded by!**/*.wavassets/sounds/iwon.wavis excluded by!**/*.wavassets/sounds/specialr.wavis excluded by!**/*.wavbun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
package.jsonsrc/cli.tsxsrc/commands/run.tsxsrc/config/index.tssrc/config/schema.tssrc/config/types.tssrc/index.tssrc/notifications.tssrc/session/persistence.tssrc/sound.tssrc/tui/components/PrdChatApp.tsxsrc/tui/components/SettingsView.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
src/config/schema.ts (1)
src/config/index.ts (1)
NotificationSoundModeSchema(638-638)
src/sound.ts (2)
src/config/index.ts (1)
NotificationSoundMode(626-626)src/config/types.ts (1)
NotificationSoundMode(53-53)
src/notifications.ts (2)
src/config/types.ts (2)
NotificationSoundMode(53-53)NotificationsConfig(58-63)src/sound.ts (2)
NotificationSoundMode(287-287)playNotificationSound(227-244)
src/config/types.ts (2)
src/config/index.ts (1)
NotificationSoundMode(626-626)src/sound.ts (1)
NotificationSoundMode(287-287)
src/tui/components/SettingsView.tsx (3)
src/config/index.ts (1)
NotificationSoundMode(626-626)src/config/types.ts (1)
NotificationSoundMode(53-53)src/sound.ts (1)
NotificationSoundMode(287-287)
src/cli.tsx (2)
src/commands/run.tsx (1)
executeRunCommand(1203-1498)src/commands/index.ts (1)
executeRunCommand(15-15)
🪛 Biome (2.1.2)
src/sound.ts
[error] 241-241: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (30)
src/session/persistence.ts (1)
187-198: LGTM! Correct backward compatibility handling for pre-versioning session files.The implementation correctly:
- Treats
undefinedversion as1to avoid spurious warnings on legacy files.- Warns only when an explicitly different version is encountered.
- Normalises the version field to
1for future saves.This aligns well with the PR objective of avoiding warnings for pre-versioning files whilst still alerting users to genuinely incompatible future versions.
src/tui/components/PrdChatApp.tsx (6)
9-10: LGTM!The new imports for
useRenderer,KeyEvent, andPasteEventare correctly added and properly utilised in the component.
408-409: LGTM!The explicit
KeyEventtype annotation improves type safety for the keyboard handler.
466-470: LGTM!The modifier key check correctly prevents keyboard shortcuts (Ctrl+C, Cmd+V, etc.) from being interpreted as text input. This is a sensible guard before processing printable characters.
490-509: Paste event handling is correctly implemented.The effect properly subscribes to and cleans up the paste event listener. The guards for
isLoadingandshowQuitConfirmensure paste is only processed when appropriate.One minor consideration: including
isLoadingandshowQuitConfirmin the dependency array means the listener is re-subscribed on every state change. This is functionally correct since the cleanup runs synchronously before the new effect, but if you find performance concerns, you could move these checks to refs. The current approach is perfectly acceptable for this use case.
411-418: LGTM!The expanded quit confirmation handling correctly supports multiple cancel paths (
n,N,Escape) alongside the existingy/Yconfirmation. The state reset is appropriate for the cancel action.
485-488: LGTM!The dependency array for
handleKeyboardcorrectly includes all reactive values used within the callback. TheuseKeyboardhook is properly invoked with the memoised handler.src/index.ts (1)
11-11: LGTM!The new export follows the established pattern and correctly uses the
.jsextension for ES module compatibility.src/config/schema.ts (2)
43-56: LGTM!The notification schemas follow established patterns in the file and use correct Zod 4 syntax. The JSDoc comments clearly document the default values.
131-133: LGTM!The
notificationsfield is correctly integrated as an optional nested object, consistent with other nested configuration objects likeerrorHandlingandrateLimitHandling.src/cli.tsx (2)
33-33: LGTM!The help text clearly documents the new default behaviour and the notification control flags. The documentation is consistent with the implementation.
Also applies to: 60-61
231-232: LGTM!The default behaviour change to execute
runwhen no subcommand is provided is a sensible UX improvement. Passingargsdirectly ensures that flags like--notifyare correctly forwarded to the run command handler.src/config/index.ts (2)
165-181: LGTM!The extended
mergeConfigsfunction correctly handles the new fields:
- Scalar fields (
progressFile,autoCommit,subagentTracingDetail) use the appropriate override pattern.fallbackAgentsarray replacement is consistent with other arrays (lines 146-147).- Nested object merging for
rateLimitHandlingandnotificationsfollows the established pattern used forerrorHandling.This addresses the config merge bug mentioned in the PR objectives.
626-626: LGTM!The new type and schema re-exports correctly expose
NotificationSoundModeandNotificationSoundModeSchemafor downstream consumers.Also applies to: 638-638
src/tui/components/SettingsView.tsx (3)
11-11: LGTM!The import of
NotificationSoundModeis correctly sourced from the config types module, aligning with the type re-exports shown in the relevant code snippets.
143-157: LGTM!The notifications toggle setting is well implemented:
- Correctly defaults to
truewhenconfig.notifications?.enabledis undefined.- Uses spread operator to preserve other notification settings (e.g.,
sound) when updatingenabled.- No restart required, which is appropriate for a runtime toggle.
158-173: LGTM!The notification sound setting is properly implemented:
- Options array matches the
NotificationSoundModetype exactly.- Defaults to
'off'which aligns with the type definition insrc/config/types.ts.- Correctly preserves the
enabledfield when updatingsound.src/config/types.ts (3)
47-63: LGTM!The notification-related types are well-designed:
NotificationSoundModeprovides a clear union type for the three supported modes.NotificationsConfiguses optional fields appropriately, allowing partial configuration.- JSDoc comments clearly document the default behaviour.
116-118: LGTM!The
notifyfield inRuntimeOptionscorrectly supports the CLI override pattern (--notify/--no-notify), withundefinedrepresenting "not specified" to allow config file values to take precedence.
181-183: LGTM!The
notificationsfield inStoredConfigappropriately references theNotificationsConfiginterface, enabling persistence of notification settings.src/sound.ts (2)
55-127: LGTM!The
playFilefunction is well-implemented:
- Verifies file accessibility before attempting playback.
- Uses detached processes with
unref()to avoid blocking.- Gracefully handles errors by resolving rather than rejecting.
- Linux fallback from
paplaytoaplayis handled appropriately.
252-284: LGTM!The
checkSoundAvailablefunction provides a sensible availability check for each platform. The assumption that PowerShell is always available on modern Windows is reasonable.src/commands/run.tsx (5)
55-56: LGTM!Imports for notification utilities are correctly sourced from the new notifications module.
176-183: LGTM!The CLI argument parsing for
--notifyand--no-notifyflags follows the established pattern and correctly setsoptions.notifytotrueorfalserespectively.
703-711: LGTM!The
NotificationRunOptionsinterface cleanly encapsulates notification configuration for the run command, with clear field names and appropriate types.
773-814: LGTM!The notification event handling in TUI mode is well-implemented:
- Correctly guards on
notificationOptions?.notificationsEnabled && engineStartTime.- Calculates duration from tracked start time.
- Maps event types to appropriate notification functions.
- Captures
lastErrorfromiteration:failedwithabortaction for use in error notifications.
1437-1446: LGTM!The notification settings resolution correctly:
- Uses
resolveNotificationsEnabledto apply the priority chain (CLI > config > default).- Falls back to
'off'for sound mode if not configured.- Bundles both settings into
NotificationRunOptionsfor clean parameter passing.src/notifications.ts (3)
39-68: LGTM!The
sendNotificationfunction is robustly implemented:
- Disables node-notifier's built-in sound for cross-platform consistency.
- Handles sound playback separately with proper error catching.
- Outer try-catch ensures notification failures don't crash the application.
- Callback-based error handling from node-notifier logs warnings appropriately.
82-98: LGTM!The
resolveNotificationsEnabledfunction implements a clear priority chain:
- CLI flag (
--notify/--no-notify)- Config file (
notifications.enabled)- Default (
true)This aligns with the documented behaviour in the JSDoc and the type definitions.
139-148: LGTM!The notification helper functions are well-designed:
sendCompletionNotification: Clear success message with task count and duration.sendMaxIterationsNotification: Informative message about iteration limit with remaining work.sendErrorNotification: Truncates long error messages to 100 characters for readability.All helpers correctly handle singular/plural forms for task counts.
Also applies to: 175-187, 212-229
- Downgrade node-notifier to v8 to match @types/node-notifier@8 - Fix Windows PowerShell command injection by passing path as argument - Add block scoping to exhaustive switch check
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/sound.ts:
- Around line 268-271: checkSoundAvailable currently only checks for 'paplay' on
Linux while playFile falls back to 'aplay'; update checkSoundAvailable to detect
either player. In the linux branch of checkSoundAvailable (and any use of
checkCmd/checkArgs), try resolving 'paplay' first and if not found try 'aplay'
(e.g., use command -v or run which twice), and return available if either is
present; ensure the same preference/order is reflected in playFile so the
fallback behavior matches the availability check.
🧹 Nitpick comments (2)
package.json (1)
18-18: Build script uses platform-specificcpcommand.The
cp -r assets dist/command won't work on Windows. If cross-platform development is a requirement, consider using a Node-based copy utility or a build tool that handles this portably.However, since the project uses Bun and the
enginesfield specifies Bun, this may be acceptable if Windows development isn't a priority.🔧 Cross-platform alternative (if needed)
You could use a small script or a package like
copyfiles:- "build": "bun build ./src/cli.tsx --outdir ./dist --target bun --sourcemap=external && bun build ./src/index.ts --outdir ./dist --target bun --sourcemap=external && cp -r assets dist/", + "build": "bun build ./src/cli.tsx --outdir ./dist --target bun --sourcemap=external && bun build ./src/index.ts --outdir ./dist --target bun --sourcemap=external && bun run scripts/copy-assets.ts",Or inline with Bun's shell:
- "build": "bun build ./src/cli.tsx --outdir ./dist --target bun --sourcemap=external && bun build ./src/index.ts --outdir ./dist --target bun --sourcemap=external && cp -r assets dist/", + "build": "bun build ./src/cli.tsx --outdir ./dist --target bun --sourcemap=external && bun build ./src/index.ts --outdir ./dist --target bun --sourcemap=external && bun -e \"await Bun.$\\`cp -r assets dist/\\`\"",src/sound.ts (1)
78-92: Linux fallback works but has a subtle race.When
paplayfails to spawn, theerrorhandler spawnsaplay. However, the main Promise'serrorhandler (line 124-127) will also fire and callresolve()with a warning message, even though the fallbackaplaymight succeed.This is functionally harmless (Promise ignores duplicate resolves), but the warning message "Failed to play sound" could be misleading when the fallback actually works.
💡 Consider restructuring the fallback
case 'linux': - // Linux: try paplay (PulseAudio) first, fall back to aplay (ALSA) - proc = spawn('paplay', [filePath], { - stdio: 'ignore', - detached: true, - }); - proc.on('error', () => { - // paplay not available, try aplay - const alsaProc = spawn('aplay', ['-q', filePath], { - stdio: 'ignore', - detached: true, - }); - alsaProc.unref(); - }); + // Linux: try paplay (PulseAudio) first, fall back to aplay (ALSA) + proc = spawn('paplay', [filePath], { + stdio: 'ignore', + detached: true, + }); + proc.on('error', () => { + // paplay not available, try aplay + const alsaProc = spawn('aplay', ['-q', filePath], { + stdio: 'ignore', + detached: true, + }); + alsaProc.unref(); + alsaProc.on('spawn', () => resolve()); + alsaProc.on('error', (err) => { + console.warn(`[sound] Failed to play sound: ${err.message}`); + resolve(); + }); + }); + // Return early to avoid attaching duplicate handlers + proc.unref(); + proc.on('spawn', () => resolve()); + return; break;Alternatively, abstract the fallback logic into a separate helper.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
package.jsonsrc/sound.ts
🔇 Additional comments (4)
src/sound.ts (4)
18-31: Path resolution logic is reasonable.The approach handles both development and bundled scenarios. Minor note: the comment on line 24 mentions "dist/cli.js" but this file is
sound.ts/sound.js—consider updating for accuracy.
150-173: Linux system sound paths may not exist on all distributions.The freedesktop sound paths (
/usr/share/sounds/freedesktop/stereo/complete.oga) are common but not universal. Some distributions or minimal installations may lack these files. The fallback tomessage.ogahelps, but both might be missing.This is acceptable for a best-effort notification sound, but users on minimal Linux setups may see warnings.
211-222: LGTM!Random sound selection is correctly implemented with appropriate defensive checks.
229-247: LGTM!The exhaustive switch with the
nevertype check ensures compile-time safety if new sound modes are added. Clean implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Line 32: The npm publish step is missing authentication and flags: add the
NODE_AUTH_TOKEN from GitHub Secrets to the publish step's environment (set env:
NODE_AUTH_TOKEN: ${{ secrets.NODE_AUTH_TOKEN }}), update the run command to
include --provenance and, if this is a scoped package, --access public (e.g.,
npm publish --provenance [--access public]), and ensure the workflow permissions
include id-token: write and packages: write so the publish action can
authenticate and attach provenance.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml
🔇 Additional comments (3)
.github/workflows/release.yml (3)
8-10: Permissions block looks correct for npm provenance.The
id-token: writepermission is required for OIDC-based npm provenance publishing, andcontents: readis sufficient for checkout. This is the recommended setup for npm provenance.
28-31: Build pipeline steps are clean and appropriate.The streamlined
buncommands for install, typecheck, lint, and build are clear and follow standard CI practices.
18-21: No action needed — Node.js 24 is already an LTS release.Node.js 24 entered long-term support in October 2025 and is the current active LTS version as of January 2026, with security support guaranteed through April 2028. Using version 24 in CI pipelines is appropriate and stable.
Likely an incorrect or invalid review comment.
feat: notification sounds and config fixes
Summary
off,system, andralph(Ralph Wiggum quotes)autoCommitand other fields set during setup weren't loading in TUIChanges
src/sound.tswith platform-specific playback (afplay/paplay/PowerShell)assets/sounds/notifications.soundoption, fixedmergeConfigsto include missing fieldsTest plan
ralph-tui run, open Settings (S), verify "Notif Sound" option appearsautoCommitset during setup now shows correctly in SettingsSummary by CodeRabbit
New Features
Changes
Bug Fixes
Other
✏️ Tip: You can customize this high-level summary in your review settings.