Symphony: Forced Parallel Execution: bypass message queue with modifier shortcut (#702)#711
Merged
pedramamini merged 7 commits intorcfrom Apr 4, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds a configurable "Forced Parallel Execution" feature: new settings and persistence, a one-time warning modal component and modal priority, Settings UI toggle with shortcut badge and acknowledgement flow, a configurable keyboard shortcut to trigger per-message forced sends, processInput API extension to support forceParallel and queue-bypass, and accompanying tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as GeneralTab UI
participant Modal as ForcedParallelWarningModal
participant Store as settingsStore
participant KeyHandler as useInputKeyDown
participant Processor as useInputProcessing
participant Persist as window.maestro.settings
participant ProcessAPI as window.maestro.process
User->>UI: Toggle Forced Parallel (first time)
alt not acknowledged
UI->>Modal: open modal
Modal->>User: show warning
User->>Modal: confirm
Modal->>Store: setForcedParallelAcknowledged(true)
Store->>Persist: persist acknowledgment
Modal->>Store: setForcedParallelExecution(true)
Store->>Persist: persist execution enabled
else already acknowledged
UI->>Store: setForcedParallelExecution(true)
Store->>Persist: persist execution enabled
end
User->>KeyHandler: Press configured shortcut (AI mode)
KeyHandler->>Processor: processInput(undefined, {forceParallel: true})
Processor->>Store: read forcedParallelExecution
alt forceParallel enabled
Processor->>Processor: shouldQueue = false
Processor->>ProcessAPI: spawn (targetSessionId + -fp-{timestamp})
else
Processor->>Processor: normal queue logic
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Collaborator
|
Thanks for the contribution, @chr1syy! 🙌 It looks like this PR is still in progress — the title is marked Once your Auto Run completes and the implementation is pushed, feel free to drop a comment here and we'll take a look. No merge conflicts at the moment, so you're in good shape there. |
Contributor
Author
Symphony Contribution SummaryThis pull request was created using Maestro Symphony - connecting AI-powered contributors with open source projects. Contribution Stats
Powered by Maestro • Learn about Symphony |
Greptile SummaryThis PR adds a "Forced Parallel Execution" feature that lets users bypass the message queue and send input immediately while the agent is busy, triggered by a configurable modifier shortcut ( Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant useInputKeyDown
participant useInputProcessing
participant settingsStore
participant GeneralTab
participant ForcedParallelWarningModal
User->>GeneralTab: Toggle Forced Parallel Execution
alt First time (not acknowledged)
GeneralTab->>ForcedParallelWarningModal: isOpen=true
User->>ForcedParallelWarningModal: Click "I understand, enable it"
ForcedParallelWarningModal->>settingsStore: setForcedParallelAcknowledged(true)
ForcedParallelWarningModal->>settingsStore: setForcedParallelExecution(true)
else Already acknowledged or toggling off
GeneralTab->>settingsStore: setForcedParallelExecution(!current)
end
User->>useInputKeyDown: Press ⌘+⇧+Enter (forcedParallelSend shortcut)
useInputKeyDown->>settingsStore: getState().shortcuts.forcedParallelSend
useInputKeyDown->>useInputKeyDown: Match modifier keys
useInputKeyDown->>useInputProcessing: processInput(undefined, { forceParallel: true })
useInputProcessing->>settingsStore: getState().forcedParallelExecution
alt forcedParallelExecution === true
useInputProcessing->>useInputProcessing: forceParallel = true → shouldQueue = false
useInputProcessing->>useInputProcessing: Spawn immediately (bypass queue)
else forcedParallelExecution === false
useInputProcessing->>useInputProcessing: Normal queue logic applies
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/__tests__/renderer/hooks/useInputProcessing.test.ts (1)
881-927: Strengthen forced-bypass tests by asserting queue state, not only spawn calls.Right now these cases can pass even if a queued item is still appended. Add an explicit
executionQueueassertion to lock behavior.🧪 Suggested assertion pattern
await act(async () => { await result.current.processInput(undefined, { forceParallel: true }); }); - // Should NOT queue — should process immediately (spawn called) + // Should NOT queue — should process immediately expect(window.maestro.process.spawn).toHaveBeenCalled(); + const setSessionsCall = mockSetSessions.mock.calls[0]?.[0]; + if (typeof setSessionsCall === 'function') { + const updatedSessions = setSessionsCall([busySession]); + expect(updatedSessions[0].executionQueue.length).toBe(0); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useInputProcessing.test.ts` around lines 881 - 927, The tests currently only assert window.maestro.process.spawn was called; also assert the queue state to ensure no queued item was appended when bypassing: after calling result.current.processInput(undefined, { forceParallel: true }) add an assertion that executionQueue (or the module's queue exposed by useInputProcessing) did not grow — e.g. expect(executionQueue.length).toBe(0) or the prior length — and/or assert there is no new entry for the inputValue; update both tests that use createDeps/result.current.processInput and window.maestro.process.spawn to include this executionQueue assertion so the bypass behavior is fully verified.src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx (1)
202-215: Strengthen layer-stack integration test with an Escape-path assertion.Unmount-only verification is weak; add an Escape key interaction check so stack registration/priority regressions are caught.
Based on learnings: "For modal escape key handling, register the modal with the layer stack and do not handle Escape locally. Ensure the modal priority is set correctly in the modal priorities configuration."🧪 Suggested enhancement
describe('layer stack integration', () => { - it('registers and unregisters without errors', () => { + it('registers with layer stack and handles Escape via stack close', () => { + const onCancel = vi.fn(); const { unmount } = renderWithLayerStack( <ForcedParallelWarningModal isOpen={true} onConfirm={vi.fn()} - onCancel={vi.fn()} + onCancel={onCancel} theme={testTheme} /> ); expect(screen.getByRole('dialog')).toBeInTheDocument(); + fireEvent.keyDown(document, { key: 'Escape' }); + expect(onCancel).toHaveBeenCalledTimes(1); expect(() => unmount()).not.toThrow(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx` around lines 202 - 215, The test for layer-stack integration in ForcedParallelWarningModal currently only verifies mount/unmount; update the test that uses renderWithLayerStack and unmount to also simulate an Escape key press and assert the modal is closed or that the dialog is no longer in the document (use screen.getByRole('dialog') before and after). Specifically, after rendering ForcedParallelWarningModal (isOpen=true) and asserting the dialog exists, fire an Escape keyboard event (via your test utils) and then assert that the dialog is removed (or that the onCancel handler was invoked) to ensure the modal relies on the layer stack Escape-path handling and correct priority registration.src/__tests__/renderer/stores/settingsStore.test.ts (1)
633-659: AddloadAllSettingshydration coverage for the new forced-parallel flags.Defaults and setters are covered, but persisted-read behavior for these two keys isn’t validated yet.
Based on learnings: "When adding settings, ensure the wrapper function calls `window.maestro.settings.set()` and the loading code in `useSettings.ts` useEffect properly retrieves and persists settings."🧪 Suggested test addition
+ it('loads forced parallel flags from persisted settings', async () => { + vi.mocked(window.maestro.settings.getAll).mockResolvedValue({ + forcedParallelExecution: true, + forcedParallelAcknowledged: true, + }); + + await loadAllSettings(); + + expect(useSettingsStore.getState().forcedParallelExecution).toBe(true); + expect(useSettingsStore.getState().forcedParallelAcknowledged).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/stores/settingsStore.test.ts` around lines 633 - 659, Add tests that verify loadAllSettings hydrates the new flags from persisted storage: mock window.maestro.settings.get to return values for 'forcedParallelExecution' and 'forcedParallelAcknowledged', call useSettingsStore.getState().loadAllSettings(), then assert that useSettingsStore.getState().forcedParallelExecution and forcedParallelAcknowledged match the mocked values and that window.maestro.settings.set was invoked for those keys (to ensure the wrapper persistence path is exercised); reference the loadAllSettings function, useSettingsStore, and window.maestro.settings.get/set in the new assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx`:
- Around line 1-217: This file has Prettier formatting issues; run Prettier and
commit the formatted output. Run the project's Prettier config (e.g., npx
prettier --write) against
src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx,
reformatting the entire test (including imports, mock setup, test blocks like
renderWithLayerStack and the describe/it blocks around
ForcedParallelWarningModal) and then stage/commit the changes so CI's prettier
--check passes.
In `@src/__tests__/renderer/hooks/useInputKeyDown.test.ts`:
- Around line 948-970: The test's assertion is too narrow—replace the final
expect that uses not.toHaveBeenCalledWith on deps.processInput with a stricter
assertion that the mock was not called at all (e.g.,
expect(deps.processInput).not.toHaveBeenCalled() or
expect(deps.processInput).toHaveBeenCalledTimes(0)). Update the test around
useInputKeyDown, createMockDeps, result.current.handleInputKeyDown and
deps.processInput so it verifies no invocation occurred in terminal mode rather
than just not being called with the specific forceParallel argument.
In `@src/__tests__/renderer/stores/settingsStore.test.ts`:
- Around line 633-659: Prettier formatting drift detected in the tests around
the "Forced Parallel Execution" block; reformat this test file so it matches
repository Prettier rules (run Prettier and commit the changes). Specifically
reformat the test cases that call
useSettingsStore.getState().setForcedParallelExecution,
useSettingsStore.getState().setForcedParallelAcknowledged, and the expectations
for forcedParallelExecution/forcedParallelAcknowledged so spacing/line breaks
match project style, then run npx prettier --write . (or your repo format
command) and commit the modified file.
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx`:
- Around line 752-754: The code dereferences shortcuts.forcedParallelSend
directly which prevents the fallback from ever running and crashes when
shortcuts is undefined; update the rendering to guard access to shortcuts (e.g.,
check shortcuts && shortcuts.forcedParallelSend or use optional chaining like
shortcuts?.forcedParallelSend) before passing keys into formatShortcutKeys, and
apply the same defensive check for the similar occurrences around lines
referencing formatShortcutKeys(shortcuts.forcedParallelSend.keys) and the block
at the other occurrence (the chunk covering the other 787-789 usage) so the
fallback strings are used when shortcuts or forcedParallelSend are missing.
In `@src/renderer/hooks/input/useInputHandlers.ts`:
- Around line 103-106: The updated TypeScript typing block for the input
handlers (symbols processInput and processInputRef) has formatting drift; run
the project's Prettier formatter (e.g., prettier --write) on
src/renderer/hooks/input/useInputHandlers.ts (including the region around the
processInput/processInputRef declarations) to fix whitespace/linebreaks and then
commit the formatted file so CI passes.
In `@src/renderer/hooks/input/useInputKeyDown.ts`:
- Around line 240-263: The forced-parallel send shortcut handler (inside the
useInputKeyDown branch that checks activeSession?.inputMode !== 'terminal')
currently triggers unconditionally when the shortcut keys match; update it to
first check the user setting that enables the feature
(settings.forcedParallelExecution or similar) before processing the shortcut. In
practice, guard the block that reads settings.shortcuts.forcedParallelSend and
calls processInput(undefined, { forceParallel: true }) with a condition that the
forcedParallelExecution flag is truthy, so the shortcut is ignored until the
user enables it; keep the existing key-matching logic and call site
(processInput) unchanged.
In `@src/renderer/hooks/input/useInputProcessing.ts`:
- Around line 405-419: When forceParallel can bypass the queue (forceParallel in
this hook), ensure each send gets a unique per-send process key so concurrent
forced sends from the same AI tab don't reuse the same session key; when
constructing the sessionId (currently `${activeSession.id}-ai-${tabId}` used
later in spawn/bookkeeping), append a per-send unique token (e.g., timestamp,
incrementing counter, or generated id) only for forced-parallel sends and
propagate that altered sessionId into the process spawn/bookkeeping paths
(functions that consume sessionId like the batch spawn/process-entry points) so
each forced send is distinguishable.
---
Nitpick comments:
In `@src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx`:
- Around line 202-215: The test for layer-stack integration in
ForcedParallelWarningModal currently only verifies mount/unmount; update the
test that uses renderWithLayerStack and unmount to also simulate an Escape key
press and assert the modal is closed or that the dialog is no longer in the
document (use screen.getByRole('dialog') before and after). Specifically, after
rendering ForcedParallelWarningModal (isOpen=true) and asserting the dialog
exists, fire an Escape keyboard event (via your test utils) and then assert that
the dialog is removed (or that the onCancel handler was invoked) to ensure the
modal relies on the layer stack Escape-path handling and correct priority
registration.
In `@src/__tests__/renderer/hooks/useInputProcessing.test.ts`:
- Around line 881-927: The tests currently only assert
window.maestro.process.spawn was called; also assert the queue state to ensure
no queued item was appended when bypassing: after calling
result.current.processInput(undefined, { forceParallel: true }) add an assertion
that executionQueue (or the module's queue exposed by useInputProcessing) did
not grow — e.g. expect(executionQueue.length).toBe(0) or the prior length —
and/or assert there is no new entry for the inputValue; update both tests that
use createDeps/result.current.processInput and window.maestro.process.spawn to
include this executionQueue assertion so the bypass behavior is fully verified.
In `@src/__tests__/renderer/stores/settingsStore.test.ts`:
- Around line 633-659: Add tests that verify loadAllSettings hydrates the new
flags from persisted storage: mock window.maestro.settings.get to return values
for 'forcedParallelExecution' and 'forcedParallelAcknowledged', call
useSettingsStore.getState().loadAllSettings(), then assert that
useSettingsStore.getState().forcedParallelExecution and
forcedParallelAcknowledged match the mocked values and that
window.maestro.settings.set was invoked for those keys (to ensure the wrapper
persistence path is exercised); reference the loadAllSettings function,
useSettingsStore, and window.maestro.settings.get/set in the new assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30f68095-c97d-4adb-aef6-7b408b86f045
📒 Files selected for processing (13)
src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsxsrc/__tests__/renderer/hooks/useInputKeyDown.test.tssrc/__tests__/renderer/hooks/useInputProcessing.test.tssrc/__tests__/renderer/stores/settingsStore.test.tssrc/renderer/components/ForcedParallelWarningModal.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/constants/modalPriorities.tssrc/renderer/constants/shortcuts.tssrc/renderer/hooks/input/useInputHandlers.tssrc/renderer/hooks/input/useInputKeyDown.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/hooks/settings/useSettings.tssrc/renderer/stores/settingsStore.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/__tests__/renderer/stores/settingsStore.test.ts (1)
125-125: Field count should stay accurate as the store evolves.Consider whether maintaining a hard-coded field count ("68 fields") in the test name adds value. If future settings are added and this number becomes stale, it could cause confusion. A generic description like "has correct default values for all fields" might be more maintainable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/stores/settingsStore.test.ts` at line 125, Rename the test description that currently reads 'has correct default values for all 68 fields' to a generic, maintainable string like 'has correct default values for all fields' in the test defined in src/__tests__/renderer/stores/settingsStore.test.ts so the test name does not contain a hard-coded field count; update the test title in the it(...) call to remove the "68" and preserve the test body and any assertions (e.g., the test function referencing settingsStore/defaults) unchanged.src/renderer/components/Settings/tabs/GeneralTab.tsx (1)
734-799: Forced Parallel Execution UI section is well-implemented.The new settings card includes:
- Clear labeling and shortcut badge display
- Accessible toggle switch with proper
role="switch"andaria-checked- Warning message with AlertTriangle icon styled appropriately
- Opacity reduction when disabled for visual feedback
One minor observation: the section's opacity is set to 0.7 when disabled (line 740), which may make the toggle itself harder to see. Consider keeping the toggle at full opacity while dimming only the description text for better usability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Settings/tabs/GeneralTab.tsx` around lines 734 - 799, The container div currently dims the entire card using opacity tied to forcedParallelExecution, making the toggle hard to see; instead, remove or set full opacity on the outer div and only reduce opacity for descriptive elements (e.g., the description div using class "flex items-start..." and the shortcut badge span) when forcedParallelExecution is false; update the JSX around the outer container that reads opacity: forcedParallelExecution ? 1 : 0.7 to keep the toggle button (used by handleForcedParallelToggle) fully opaque while applying the reduced opacity to the descriptive text and shortcut display (the spans that render shortcuts via formatShortcutKeys and the warning text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/renderer/stores/settingsStore.test.ts`:
- Line 125: Rename the test description that currently reads 'has correct
default values for all 68 fields' to a generic, maintainable string like 'has
correct default values for all fields' in the test defined in
src/__tests__/renderer/stores/settingsStore.test.ts so the test name does not
contain a hard-coded field count; update the test title in the it(...) call to
remove the "68" and preserve the test body and any assertions (e.g., the test
function referencing settingsStore/defaults) unchanged.
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx`:
- Around line 734-799: The container div currently dims the entire card using
opacity tied to forcedParallelExecution, making the toggle hard to see; instead,
remove or set full opacity on the outer div and only reduce opacity for
descriptive elements (e.g., the description div using class "flex
items-start..." and the shortcut badge span) when forcedParallelExecution is
false; update the JSX around the outer container that reads opacity:
forcedParallelExecution ? 1 : 0.7 to keep the toggle button (used by
handleForcedParallelToggle) fully opaque while applying the reduced opacity to
the descriptive text and shortcut display (the spans that render shortcuts via
formatShortcutKeys and the warning text).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd183f3e-7216-49af-8d17-afbad293ed66
📒 Files selected for processing (8)
src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsxsrc/__tests__/renderer/hooks/useInputKeyDown.test.tssrc/__tests__/renderer/stores/settingsStore.test.tssrc/renderer/components/ForcedParallelWarningModal.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/hooks/input/useInputHandlers.tssrc/renderer/hooks/input/useInputKeyDown.tssrc/renderer/hooks/input/useInputProcessing.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/renderer/hooks/useInputKeyDown.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/hooks/input/useInputKeyDown.ts
- src/renderer/hooks/input/useInputProcessing.ts
chr1syy
changed the title
…priority Phase 1 of forced parallel execution feature: adds forcedParallelSend shortcut definition, FORCED_PARALLEL_WARNING modal priority, and forcedParallelExecution/forcedParallelAcknowledged settings with persistence to the Zustand settings store. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Creates the warning modal shown when users first enable Forced Parallel Execution. Uses the existing Modal/ModalFooter pattern with layer stack registration at FORCED_PARALLEL_WARNING priority. Includes AlertTriangle icon with warning accent, explanatory copy about parallel write risks, and Cancel / "I understand, enable it" action buttons. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add comprehensive test coverage for the forced parallel execution feature: - useInputProcessing: 4 tests verifying forceParallel queue bypass logic (enabled+busy, enabled+autorun, disabled+busy, absent+busy) - useInputKeyDown: 4 tests for Cmd/Ctrl+Shift+Enter shortcut detection (AI mode, terminal mode exclusion, custom shortcut support) - settingsStore: 4 tests for forcedParallelExecution/Acknowledged defaults and setter persistence. Updated resetStore() and field count (66→68) - ForcedParallelWarningModal: 10 tests covering rendering, open/closed state, button handlers, focus management, and layer stack integration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lel execution - Gate forced parallel shortcut behind forcedParallelExecution setting - Add optional chaining for shortcuts in GeneralTab to prevent TypeError - Append unique suffix to targetSessionId for forced-parallel sends to prevent process key collisions - Remove redundant useCallback wrapper in ForcedParallelWarningModal - Document Enter-only constraint for forced parallel shortcut detection - Tighten terminal-mode test assertion and add test for disabled feature state - Apply Prettier formatting to all affected files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chr1syy
force-pushed
the
symphony/issue-702-mnhxkuso
branch
from
b12f2c5 to
6299d4a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/__tests__/renderer/hooks/useInputKeyDown.test.ts (1)
996-998:⚠️ Potential issue | 🟡 MinorStrengthen negative assertions to prevent false positives.
At Line 997 and Line 1021,
not.toHaveBeenCalledWith(...)still passes ifprocessInputis called with different arguments. For these checks, assert zero calls at that checkpoint.Suggested assertion hardening
@@ - // Should NOT call processInput with forceParallel when feature is disabled - expect(deps.processInput).not.toHaveBeenCalledWith(undefined, { forceParallel: true }); + // Should NOT call processInput at all when feature is disabled + expect(deps.processInput).not.toHaveBeenCalled(); @@ act(() => { result.current.handleInputKeyDown(e1); }); - expect(deps.processInput).not.toHaveBeenCalledWith(undefined, { forceParallel: true }); + expect(deps.processInput).toHaveBeenCalledTimes(0); @@ act(() => { result.current.handleInputKeyDown(e2); }); + expect(deps.processInput).toHaveBeenCalledTimes(1); expect(deps.processInput).toHaveBeenCalledWith(undefined, { forceParallel: true });#!/bin/bash # Verify no narrow negative matcher remains for forced-parallel assertions rg -n "not\.toHaveBeenCalledWith\(undefined, \{ forceParallel: true \}\)" src/__tests__/renderer/hooks/useInputKeyDown.test.tsExpected result after fix: no matches.
Also applies to: 1016-1022
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useInputKeyDown.test.ts` around lines 996 - 998, The negative assertions using not.toHaveBeenCalledWith(undefined, { forceParallel: true }) are weak and can yield false positives; update the two checkpoints in the useInputKeyDown.test (the tests that verify the feature-disabled and related scenarios) to assert that deps.processInput was not called at all by replacing those not.toHaveBeenCalledWith(...) checks with expect(deps.processInput).not.toHaveBeenCalled() so the test fails if processInput was invoked with any arguments (references: deps.processInput and the failing test cases in useInputKeyDown.test).
🧹 Nitpick comments (2)
src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx (2)
174-178: Add defensive assertion before clicking close button.The non-null assertion
closeButton!could mask a test failure if the DOM structure changes. Assert the button exists first for clearer failure messages.🔧 Suggested improvement
- const closeButton = screen.getByTestId('x-icon').closest('button'); - fireEvent.click(closeButton!); + const closeButton = screen.getByTestId('x-icon').closest('button'); + expect(closeButton).toBeInTheDocument(); + fireEvent.click(closeButton!);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx` around lines 174 - 178, Add a defensive assertion that the resolved closeButton exists before calling fireEvent.click to avoid masking DOM regressions; after computing closeButton via screen.getByTestId('x-icon').closest('button') (the closeButton variable in the ForcedParallelWarningModal.test), assert it is present (e.g., expect(closeButton).not.toBeNull() or toBeInTheDocument()) and then call fireEvent.click(closeButton) and the existing expect(onCancel).toHaveBeenCalledTimes(1).
181-198: Consider adding Escape key dismissal test.The focus management test is good. However, modals should typically support Escape key to close. Based on learnings, this modal should register with the layer stack for Escape handling. Consider adding a test to verify pressing Escape calls
onCancel.🔧 Example test for Escape key
it('calls onCancel when Escape key is pressed', () => { const onCancel = vi.fn(); renderWithLayerStack( <ForcedParallelWarningModal isOpen={true} onConfirm={vi.fn()} onCancel={onCancel} theme={testTheme} /> ); fireEvent.keyDown(screen.getByRole('dialog'), { key: 'Escape' }); expect(onCancel).toHaveBeenCalledTimes(1); });Based on learnings: "For modal escape not working, register with layer stack (don't handle Escape locally) and check priority is set correctly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx` around lines 181 - 198, Add a test that verifies the ForcedParallelWarningModal calls its onCancel prop when Escape is pressed: render the modal using renderWithLayerStack with isOpen={true}, pass a vi.fn() for onCancel, then simulate pressing Escape (e.g., fireEvent.keyDown on the dialog element returned by screen.getByRole('dialog')) and assert onCancel was called once; ensure the test uses the same helpers (renderWithLayerStack, testTheme) and targets the modal component ForcedParallelWarningModal so it also verifies the modal is registered with the layer stack for Escape handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/__tests__/renderer/hooks/useInputKeyDown.test.ts`:
- Around line 996-998: The negative assertions using
not.toHaveBeenCalledWith(undefined, { forceParallel: true }) are weak and can
yield false positives; update the two checkpoints in the useInputKeyDown.test
(the tests that verify the feature-disabled and related scenarios) to assert
that deps.processInput was not called at all by replacing those
not.toHaveBeenCalledWith(...) checks with
expect(deps.processInput).not.toHaveBeenCalled() so the test fails if
processInput was invoked with any arguments (references: deps.processInput and
the failing test cases in useInputKeyDown.test).
---
Nitpick comments:
In `@src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsx`:
- Around line 174-178: Add a defensive assertion that the resolved closeButton
exists before calling fireEvent.click to avoid masking DOM regressions; after
computing closeButton via screen.getByTestId('x-icon').closest('button') (the
closeButton variable in the ForcedParallelWarningModal.test), assert it is
present (e.g., expect(closeButton).not.toBeNull() or toBeInTheDocument()) and
then call fireEvent.click(closeButton) and the existing
expect(onCancel).toHaveBeenCalledTimes(1).
- Around line 181-198: Add a test that verifies the ForcedParallelWarningModal
calls its onCancel prop when Escape is pressed: render the modal using
renderWithLayerStack with isOpen={true}, pass a vi.fn() for onCancel, then
simulate pressing Escape (e.g., fireEvent.keyDown on the dialog element returned
by screen.getByRole('dialog')) and assert onCancel was called once; ensure the
test uses the same helpers (renderWithLayerStack, testTheme) and targets the
modal component ForcedParallelWarningModal so it also verifies the modal is
registered with the layer stack for Escape handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4f78fff-ce7c-481f-b84d-98250955be35
📒 Files selected for processing (2)
src/__tests__/renderer/components/ForcedParallelWarningModal.test.tsxsrc/__tests__/renderer/hooks/useInputKeyDown.test.ts
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.
Maestro Symphony Contribution
Closes #702
Contributed via Maestro Symphony.
Status: In Progress
Started: 2026-04-02T20:32:11.363Z
This PR will be updated automatically when the Auto Run completes.
Summary by CodeRabbit
New Features
Tests