refactor: consolidate modal layer and group chat spawn (Phase 10)#824
Merged
reachrazamair merged 5 commits intoRunMaestro:rcfrom Apr 16, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralized modal layer management by replacing manual Changes
Sequence Diagram(s)sequenceDiagram
participant Router as GroupChat Router
participant Helper as spawnGroupChatAgent
participant SSH as SSH Wrapper (sshStore)
participant WinCfg as Windows Spawn Config
participant PM as ProcessManager
Router->>Helper: await spawnGroupChatAgent(config)
alt ssh enabled
Helper->>SSH: wrapSpawnWithSsh(command,args,cwd,prompt,env)
SSH-->>Helper: { command, args, cwd, sshStdinScript }
end
Helper->>WinCfg: getWindowsSpawnConfig(agentId, sshRemoteConfig?)
WinCfg-->>Helper: windows spawn options
Helper->>PM: processManager.spawn({ sessionId, command, args, cwd, env, stdinScript, readOnlyMode, ... })
PM-->>Helper: { pid, success }
Helper-->>Router: return { pid, success }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
jSydorowicz21
force-pushed
the
dedup/phase-10-modal-spawn-consolidation
branch
from
f361226 to
4af7a74
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/FirstRunCelebration.tsx (1)
198-209:⚠️ Potential issue | 🟠 MajorRemove local
Escapehandling now that the modal is layer-registered.After adopting
useModalLayer, handlingEscapeagain in the window listener (Line 201) creates competing close paths and can break top-layer Escape routing.Suggested fix
useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Enter' || e.key === 'Escape') { + if (e.key === 'Enter') { e.preventDefault(); handleClose(); } }; window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); }, [handleClose]);As per coding guidelines "For modal escape not working, register with layer stack (don't handle Escape locally) and check priority is set correctly in
src/renderer/constants/modalPriorities.ts."Also applies to: 212-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FirstRunCelebration.tsx` around lines 198 - 209, The window keydown effect currently intercepts Escape and conflicts with the modal layer; remove local Escape handling by either deleting the keydown listener entirely or changing the handleKeyDown callback to only handle Enter (leave Escape handling to useModalLayer), and remove any duplicate listeners elsewhere (the other effect at the 212-216 region). Update the useEffect that defines handleKeyDown (and any matching cleanup) so it no longer calls handleClose on Escape; ensure the modal is registered via useModalLayer and that its priority in modalPriorities.ts is correct.
🧹 Nitpick comments (2)
src/renderer/components/KeyboardMasteryCelebration.tsx (1)
168-183: Redundant Escape handling between window listener and modal layer.The component handles Escape in two places:
- The
useModalLayerhook (line 181-183)- The window
keydownlistener (lines 168-178)While the
isClosingRefguard prevents double-execution, the Escape handling in the window listener is now redundant. Consider removinge.key === 'Escape'from the window listener sinceuseModalLayeralready handles it through the layer stack.♻️ Suggested simplification
// Handle keyboard events - use ref to avoid stale closure useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Enter' || e.key === 'Escape') { + if (e.key === 'Enter') { e.preventDefault(); handleCloseRef.current(); } }; window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); }, []); // Empty deps - handler reads from ref🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/KeyboardMasteryCelebration.tsx` around lines 168 - 183, The window-level keydown handler in useEffect redundantly checks for 'Escape' even though useModalLayer already handles Escape via the layer stack; update the handleKeyDown function (inside the useEffect) to only handle 'Enter' (remove the e.key === 'Escape' branch), keep e.preventDefault() and handleCloseRef.current() for Enter, and leave useModalLayer(...) and its Escape handling untouched so Escape is only handled by useModalLayer.src/renderer/components/FirstRunCelebration.tsx (1)
218-221: Consider dropping manual mount-focus to avoid focus ownership conflicts.
useModalLayeralready registers this as a focus-capturing modal; this extra mount focus can be redundant and occasionally race with stack-managed focus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FirstRunCelebration.tsx` around lines 218 - 221, Remove the manual mount-focus useEffect in FirstRunCelebration: delete the block that calls containerRef.current?.focus() inside useEffect and rely on useModalLayer's focus management instead; ensure containerRef remains attached to the modal container (and not removed) so useModalLayer can manage focus without the redundant focus call.
🤖 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/main/group-chat/spawnGroupChatAgent.ts`:
- Around line 97-130: When sshRemoteConfig?.enabled is true but sshStore is null
you must not silently fall back to a local spawn; detect that condition and fail
fast instead of continuing. In spawnGroupChatAgent.ts, add a check after
evaluating sshRemoteConfig: if (sshRemoteConfig?.enabled && !sshStore) throw a
descriptive error (include debugLabel/agentId) so the error bubbles to Sentry;
otherwise continue to call wrapSpawnWithSsh(...) when sshStore exists and call
getWindowsSpawnConfig(agentId, undefined) only when SSH is not enabled or has
been wrapped. This ensures wrapSpawnWithSsh, sshRemoteConfig, sshStore,
getWindowsSpawnConfig and the spawn* variables behave correctly.
In `@src/renderer/components/AutoRun/AutoRunLightbox.tsx`:
- Around line 63-70: The modal layer is being enabled even when the lightbox
cannot render (preview URL missing), which blocks shortcuts/focus; update the
useModalLayer call in the AutoRunLightbox component to only enable when the
component will actually render (e.g. change the enabled prop from isVisible /
lightboxFilename check to include a truthy preview URL/previewReady flag), keep
the same priority (MODAL_PRIORITIES.AUTORUN_LIGHTBOX) and continue to rely on
the layer stack for Escape handling (do not handle Escape locally).
In `@src/renderer/components/HistoryDetailModal.tsx`:
- Around line 100-102: The parent HistoryDetailModal registers an Escape layer
with useModalLayer(MODAL_PRIORITIES.CONFIRM, ..., onCloseRef) which closes the
whole modal even when the nested delete confirmation (showDeleteConfirm) is
open; update the logic so the parent only reacts to Escape when the nested
confirm is not visible (e.g., guard the onCloseRef call with !showDeleteConfirm)
and ensure the nested DeleteConfirmModal itself registers its own layer using
useModalLayer(MODAL_PRIORITIES.CONFIRM, undefined, onNestedClose) so the
confirmation becomes the top layer and handles Escape instead of the parent.
In `@src/renderer/components/SymphonyModal.tsx`:
- Around line 1415-1428: The modal-layer Escape handler registered via
useModalLayer(MODAL_PRIORITIES.SYMPHONY, ...) is closing the modal even when the
project-search input is focused; change the handler to ignore Escape if the
focused element is an input/textarea or the project-search element (e.g., check
document.activeElement and return early when it's an input, textarea, or matches
the project search's selector or has a distinguishing attribute/class), so that
Escape only triggers the layer close/back (showHelpRef, showDetailViewRef,
handleBackRef, onCloseRef) when focus is not inside a focused text input;
alternatively, consume the Escape in the project-search key handler before it
bubbles to useModalLayer.
In `@src/renderer/components/Wizard/ExistingDocsModal.tsx`:
- Around line 53-55: The root onKeyDown handler in ExistingDocsModal is
preventing Escape from reaching the modal layer stack—change the handler so it
does not stop propagation for the Escape key (allow it to bubble) and let
useModalLayer(MODAL_PRIORITIES.EXISTING_AUTORUN_DOCS, 'Existing Playbook
Documents Found', () => onCancelRef.current()) handle close-on-Escape;
specifically, update the component's onKeyDown logic to only stop propagation
for keys that must be blocked (e.g., Tab if needed) and never call
stopPropagation for 'Escape', ensuring the modal priority constant remains the
correct MODAL_PRIORITIES.EXISTING_AUTORUN_DOCS.
In `@src/renderer/components/Wizard/tour/TourOverlay.tsx`:
- Around line 245-248: The window keydown handler still calls skipTour() and
Escape is also wired via useModalLayer(MODAL_PRIORITIES.TOUR...), which can
double-fire skipTour; remove the Escape handling from the local window keydown
listener (the handler that invokes skipTour) so Escape is handled only by the
layer stack, or alternatively have the local handler call
event.stopImmediatePropagation()/preventDefault() before calling skipTour to
prevent the layer from also receiving it; ensure useModalLayer is used with the
correct MODAL_PRIORITIES.TOUR priority and that only one path invokes
skipTour/onClose to avoid duplicate analytics and races.
---
Outside diff comments:
In `@src/renderer/components/FirstRunCelebration.tsx`:
- Around line 198-209: The window keydown effect currently intercepts Escape and
conflicts with the modal layer; remove local Escape handling by either deleting
the keydown listener entirely or changing the handleKeyDown callback to only
handle Enter (leave Escape handling to useModalLayer), and remove any duplicate
listeners elsewhere (the other effect at the 212-216 region). Update the
useEffect that defines handleKeyDown (and any matching cleanup) so it no longer
calls handleClose on Escape; ensure the modal is registered via useModalLayer
and that its priority in modalPriorities.ts is correct.
---
Nitpick comments:
In `@src/renderer/components/FirstRunCelebration.tsx`:
- Around line 218-221: Remove the manual mount-focus useEffect in
FirstRunCelebration: delete the block that calls containerRef.current?.focus()
inside useEffect and rely on useModalLayer's focus management instead; ensure
containerRef remains attached to the modal container (and not removed) so
useModalLayer can manage focus without the redundant focus call.
In `@src/renderer/components/KeyboardMasteryCelebration.tsx`:
- Around line 168-183: The window-level keydown handler in useEffect redundantly
checks for 'Escape' even though useModalLayer already handles Escape via the
layer stack; update the handleKeyDown function (inside the useEffect) to only
handle 'Enter' (remove the e.key === 'Escape' branch), keep e.preventDefault()
and handleCloseRef.current() for Enter, and leave useModalLayer(...) and its
Escape handling untouched so Escape is only handled by useModalLayer.
🪄 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: 9d320c11-8b0a-41cc-bd10-9e0217839265
📒 Files selected for processing (58)
src/__tests__/renderer/components/CueModal.test.tsxsrc/__tests__/renderer/components/DocumentGraph/DocumentGraphView.test.tsxsrc/__tests__/renderer/components/ExecutionQueueBrowser.test.tsxsrc/__tests__/renderer/components/LeaderboardRegistrationModal.test.tsxsrc/__tests__/renderer/components/NewInstanceModal.test.tsxsrc/__tests__/renderer/components/SettingsModal.test.tsxsrc/__tests__/renderer/components/UsageDashboard/responsive-layout.test.tsxsrc/__tests__/renderer/components/UsageDashboard/state-transition-animations.test.tsxsrc/__tests__/renderer/components/UsageDashboardModal.test.tsxsrc/main/group-chat/group-chat-router.tssrc/main/group-chat/spawnGroupChatAgent.tssrc/renderer/components/AgentCreationDialog.tsxsrc/renderer/components/AgentPromptComposerModal.tsxsrc/renderer/components/AgentSessionsBrowser.tsxsrc/renderer/components/AgentSessionsModal.tsxsrc/renderer/components/AutoRun/AutoRunExpandedModal.tsxsrc/renderer/components/AutoRun/AutoRunLightbox.tsxsrc/renderer/components/AutoRun/AutoRunSearchBar.tsxsrc/renderer/components/BatchRunnerModal.tsxsrc/renderer/components/CreatePRModal.tsxsrc/renderer/components/CreateWorktreeModal.tsxsrc/renderer/components/CueModal/CueModal.tsxsrc/renderer/components/DirectorNotes/DirectorNotesModal.tsxsrc/renderer/components/DocumentGraph/DocumentGraphView.tsxsrc/renderer/components/DocumentsPanel.tsxsrc/renderer/components/ExecutionQueueBrowser.tsxsrc/renderer/components/FileSearchModal.tsxsrc/renderer/components/FirstRunCelebration.tsxsrc/renderer/components/GitDiffViewer.tsxsrc/renderer/components/GitLogViewer.tsxsrc/renderer/components/HistoryDetailModal.tsxsrc/renderer/components/InlineWizard/WizardExitConfirmDialog.tsxsrc/renderer/components/KeyboardMasteryCelebration.tsxsrc/renderer/components/LeaderboardRegistrationModal.tsxsrc/renderer/components/LogViewer.tsxsrc/renderer/components/MarketplaceModal.tsxsrc/renderer/components/MergeProgressModal.tsxsrc/renderer/components/MergeSessionModal.tsxsrc/renderer/components/PlaygroundPanel.tsxsrc/renderer/components/ProcessMonitor.tsxsrc/renderer/components/PromptComposerModal.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/QuitConfirmModal.tsxsrc/renderer/components/SendToAgentModal.tsxsrc/renderer/components/Settings/SettingsModal.tsxsrc/renderer/components/StandingOvationOverlay.tsxsrc/renderer/components/SummarizeProgressModal.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/TabSwitcherModal.tsxsrc/renderer/components/TransferProgressModal.tsxsrc/renderer/components/UsageDashboard/UsageDashboardModal.tsxsrc/renderer/components/Wizard/ExistingAutoRunDocsModal.tsxsrc/renderer/components/Wizard/ExistingDocsModal.tsxsrc/renderer/components/Wizard/MaestroWizard.tsxsrc/renderer/components/Wizard/WizardExitConfirmModal.tsxsrc/renderer/components/Wizard/WizardResumeModal.tsxsrc/renderer/components/Wizard/tour/TourOverlay.tsxsrc/renderer/hooks/ui/useModalLayer.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/components/AutoRun/AutoRunLightbox.tsx (1)
58-63: Consider reusing one derived image URL to avoid divergence.
lightboxImageUrlis computed at Line 59-61, then similar logic is recomputed at Line 200-202. Reusing the same value for the render guard and<img src>reduces drift risk.♻️ Proposed refactor
- // Don't render if no image is selected - const imageUrl = - lightboxExternalUrl || - (lightboxFilename ? attachmentPreviews.get(lightboxFilename) : undefined); - if (!lightboxFilename || !imageUrl) { + // Don't render if no image is selected + if (!isVisible || !lightboxImageUrl) { return null; } @@ - <img - src={imageUrl} + <img + src={lightboxImageUrl} alt={lightboxFilename} className="max-w-[90%] max-h-[90%] rounded shadow-2xl" onClick={(e) => e.stopPropagation()} />Also applies to: 200-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun/AutoRunLightbox.tsx` around lines 58 - 63, The code computes lightboxImageUrl in the top block but recomputes the same logic later; consolidate by deriving lightboxImageUrl once (using lightboxExternalUrl || (lightboxFilename ? attachmentPreviews.get(lightboxFilename) : undefined)) and reuse that single variable for both the visibility guard (isVisible) and the <img> src so the render guard and image source cannot diverge; update uses of isVisible, lightboxImageUrl, and any later inline recomputation to reference the single derived variable instead.
🤖 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/main/group-chat/spawnGroupChatAgent.ts`:
- Around line 105-133: The Windows spawn config is currently determined from
sshRemoteConfig even when wrapSpawnWithSsh fell back to local execution; change
the gate to use the actual wrapSpawnWithSsh result (sshWrapped.sshRemoteUsed)
instead of just sshRemoteConfig. Specifically, after calling
wrapSpawnWithSsh(...), use the sshWrapped.sshRemoteUsed truthiness to decide
what to pass into getWindowsSpawnConfig (e.g. getWindowsSpawnConfig(agentId,
sshWrapped.sshRemoteUsed ? sshRemoteConfig : undefined)), so Windows shell/stdin
behavior is enabled for true local execution and skipped only when an SSH remote
is actually used.
---
Nitpick comments:
In `@src/renderer/components/AutoRun/AutoRunLightbox.tsx`:
- Around line 58-63: The code computes lightboxImageUrl in the top block but
recomputes the same logic later; consolidate by deriving lightboxImageUrl once
(using lightboxExternalUrl || (lightboxFilename ?
attachmentPreviews.get(lightboxFilename) : undefined)) and reuse that single
variable for both the visibility guard (isVisible) and the <img> src so the
render guard and image source cannot diverge; update uses of isVisible,
lightboxImageUrl, and any later inline recomputation to reference the single
derived variable instead.
🪄 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: 3c8dced7-0d00-4389-b288-c2b5f2b00d53
📒 Files selected for processing (6)
src/main/group-chat/spawnGroupChatAgent.tssrc/renderer/components/AutoRun/AutoRunLightbox.tsxsrc/renderer/components/HistoryDetailModal.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/Wizard/ExistingDocsModal.tsxsrc/renderer/components/Wizard/tour/TourOverlay.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/components/Wizard/ExistingDocsModal.tsx
- src/renderer/components/HistoryDetailModal.tsx
- src/renderer/components/SymphonyModal.tsx
Comment on lines
+105
to
+133
| const sshWrapped = await wrapSpawnWithSsh( | ||
| { | ||
| command: baseCommand, | ||
| args, | ||
| cwd, | ||
| prompt, | ||
| customEnvVars, | ||
| promptArgs: agent.promptArgs, | ||
| noPromptSeparator: agent.noPromptSeparator, | ||
| agentBinaryName: agent.binaryName, | ||
| }, | ||
| sshRemoteConfig, | ||
| sshStore | ||
| ); | ||
| spawnCommand = sshWrapped.command; | ||
| spawnArgs = sshWrapped.args; | ||
| spawnCwd = sshWrapped.cwd; | ||
| spawnPrompt = sshWrapped.prompt; | ||
| spawnEnvVars = sshWrapped.customEnvVars; | ||
| spawnSshStdinScript = sshWrapped.sshStdinScript; | ||
| if (sshWrapped.sshRemoteUsed && debugLabel) { | ||
| console.log( | ||
| `[GroupChat:Debug] SSH remote used for ${debugLabel}: ${sshWrapped.sshRemoteUsed.name}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Get Windows-specific spawn config (shell, stdin mode) - skipped for SSH | ||
| const winConfig = getWindowsSpawnConfig(agentId, sshRemoteConfig ?? undefined); |
There was a problem hiding this comment.
Use actual SSH usage result (not only config) to gate Windows spawn behavior.
At Line 133, getWindowsSpawnConfig(...) is keyed off sshRemoteConfig.enabled, but wrapSpawnWithSsh(...) can still fall back to local execution (sshRemoteUsed: null). In that fallback case, Windows shell/stdin config stays disabled even though execution is local.
Proposed fix
let spawnSshStdinScript: string | undefined;
+let sshRemoteUsed = false;
// Apply SSH wrapping if configured
if (sshRemoteConfig?.enabled && !sshStore) {
throw new Error(`SSH remote is enabled but sshStore is not available for ${debugLabel ?? sessionId}`);
}
if (sshStore && sshRemoteConfig?.enabled) {
@@
spawnEnvVars = sshWrapped.customEnvVars;
spawnSshStdinScript = sshWrapped.sshStdinScript;
+ sshRemoteUsed = !!sshWrapped.sshRemoteUsed;
if (sshWrapped.sshRemoteUsed && debugLabel) {
@@
-const winConfig = getWindowsSpawnConfig(agentId, sshRemoteConfig ?? undefined);
+const winConfig = getWindowsSpawnConfig(
+ agentId,
+ sshRemoteUsed ? (sshRemoteConfig ?? undefined) : undefined
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/group-chat/spawnGroupChatAgent.ts` around lines 105 - 133, The
Windows spawn config is currently determined from sshRemoteConfig even when
wrapSpawnWithSsh fell back to local execution; change the gate to use the actual
wrapSpawnWithSsh result (sshWrapped.sshRemoteUsed) instead of just
sshRemoteConfig. Specifically, after calling wrapSpawnWithSsh(...), use the
sshWrapped.sshRemoteUsed truthiness to decide what to pass into
getWindowsSpawnConfig (e.g. getWindowsSpawnConfig(agentId,
sshWrapped.sshRemoteUsed ? sshRemoteConfig : undefined)), so Windows shell/stdin
behavior is enabled for true local execution and skipped only when an SSH remote
is actually used.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/group-chat/spawnGroupChatAgent.ts (1)
134-135:⚠️ Potential issue | 🟠 MajorGate Windows spawn config by actual SSH usage, not SSH enablement flag.
At Line 135,
getWindowsSpawnConfig(...)is keyed offsshRemoteConfig, butwrapSpawnWithSsh(...)can fall back to local (sshRemoteUsed: null). That leaves local fallback executions without required Windows shell/stdin behavior.As per coding guidelines, "When implementing features that spawn agent processes ... support SSH remote execution by checking for `sshRemoteConfig` and using `wrapSpawnWithSsh()`..."Proposed fix
let spawnPrompt: string | undefined = prompt; let spawnEnvVars = customEnvVars; let spawnSshStdinScript: string | undefined; + let sshRemoteUsed = false; @@ spawnPrompt = sshWrapped.prompt; spawnEnvVars = sshWrapped.customEnvVars; spawnSshStdinScript = sshWrapped.sshStdinScript; + sshRemoteUsed = !!sshWrapped.sshRemoteUsed; if (sshWrapped.sshRemoteUsed && debugLabel) { @@ - const winConfig = getWindowsSpawnConfig(agentId, sshRemoteConfig ?? undefined); + const winConfig = getWindowsSpawnConfig( + agentId, + sshRemoteUsed ? (sshRemoteConfig ?? undefined) : undefined + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/spawnGroupChatAgent.ts` around lines 134 - 135, The code calls getWindowsSpawnConfig(agentId, sshRemoteConfig ?? undefined) based only on sshRemoteConfig, but wrapSpawnWithSsh(...) can decide sshRemoteUsed is null (local fallback); change the logic to gate Windows-specific config on the actual sshRemoteUsed returned by wrapSpawnWithSsh (or obtain sshRemoteUsed first) so you only apply getWindowsSpawnConfig when sshRemoteUsed is non-null; update references to getWindowsSpawnConfig, wrapSpawnWithSsh, sshRemoteConfig, and sshRemoteUsed in spawnGroupChatAgent accordingly.
🤖 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/main/group-chat/spawnGroupChatAgent.ts`:
- Around line 134-135: The code calls getWindowsSpawnConfig(agentId,
sshRemoteConfig ?? undefined) based only on sshRemoteConfig, but
wrapSpawnWithSsh(...) can decide sshRemoteUsed is null (local fallback); change
the logic to gate Windows-specific config on the actual sshRemoteUsed returned
by wrapSpawnWithSsh (or obtain sshRemoteUsed first) so you only apply
getWindowsSpawnConfig when sshRemoteUsed is non-null; update references to
getWindowsSpawnConfig, wrapSpawnWithSsh, sshRemoteConfig, and sshRemoteUsed in
spawnGroupChatAgent accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 761dcc3e-3eb5-4467-9c08-372626ad2906
📒 Files selected for processing (1)
src/main/group-chat/spawnGroupChatAgent.ts
jSydorowicz21
force-pushed
the
dedup/phase-10-modal-spawn-consolidation
branch
from
662362e to
28cbb0b
Compare
Sub-phase 10A: Modal layer migration - Extend `useModalLayer` hook with optional `enabled` flag for conditional registration; switch the underlying registration to a stable ref-based escape closure so the layer no longer needs to be re-registered or have its handler manually re-pushed when the parent's onEscape callback identity changes. - Migrate 47 modal call sites across 46 files from manual `useLayerStack().registerLayer/unregisterLayer/updateLayerHandler` boilerplate to `useModalLayer(priority, ariaLabel, onEscape, options?)`. - Update 9 tests whose `useLayerStack` mocks needed `updateLayerHandler` (the migrated hook always uses it). Update one assertion in `NewInstanceModal.test.tsx` to verify behavior (escape routes to latest onClose) rather than implementation detail (literal handler identity). Sub-phase 10B: Group chat spawn helper - Extract `spawnGroupChatAgent()` helper at `src/main/group-chat/spawnGroupChatAgent.ts` that encapsulates SSH wrapping + Windows shell config + processManager.spawn into a single call. - Replace 4 near-identical inline spawn sequences in `group-chat-router.ts` (moderator, participant, synthesis, recovery) with calls to the helper. Each site collapses ~60 lines of imperative spawn/wrap/Windows code to a single configuration object. Sites skipped (10A): - 9 `type: 'overlay'` / `allowClickOutside` registrations in DocumentGraphView, FileExplorerPanel, FilePreview, LightboxModal, TerminalOutput, TerminalSearchBar (the hook hardcodes `type: 'modal'`). - These keep their existing `useLayerStack()` calls. Net change: 57 files, +445 / -1554 (~1109 lines removed).
- Fail fast when SSH enabled but sshStore unavailable (spawnGroupChatAgent) - Remove duplicate Escape handler from TourOverlay window listener - Route Escape to delete confirmation before closing HistoryDetailModal - Only enable AutoRunLightbox layer when preview URL is available - Let Escape propagate through ExistingDocsModal to layer stack - Stop Escape propagation in SymphonyModal search to prevent modal close
Wraps long error message line in spawnGroupChatAgent.ts to satisfy prettier's print-width rule. CI lint-and-format was failing on this.
Retriggers CI after flaky prettier failure on identical content.
jSydorowicz21
force-pushed
the
dedup/phase-10-modal-spawn-consolidation
branch
from
28cbb0b to
954a9f4
Compare
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.
Summary
Two structural consolidations:
useModalLayerhook, replacing manualregisterLayer/unregisterLayerboilerplate and hand-rolled Escape handlersspawnGroupChatAgenthelper that encapsulates SSH wrap + Windows shell/stdin handling + process spawn, consolidating 4 near-identical spawn sitesNet: 58 files, -954 lines (exceeds the ~328-line playbook target)
10A - Modal layer migration
Hook enhancement (
src/renderer/hooks/ui/useModalLayer.ts):enabledflagonEscapecallback identity changesMigrated 47 call sites across 46 files to
useModalLayer(priority, ariaLabel, onEscape, options?):AgentCreationDialog, AgentPromptComposerModal, AgentSessionsBrowser, AgentSessionsModal, AutoRunExpandedModal, AutoRunLightbox, AutoRunSearchBar, BatchRunnerModal, CreatePRModal, CreateWorktreeModal, CueModal, DirectorNotesModal, DocumentGraphView, DocumentsPanel, ExecutionQueueBrowser, FileSearchModal, FirstRunCelebration, GitDiffViewer, GitLogViewer, HistoryDetailModal, WizardExitConfirmDialog, KeyboardMasteryCelebration, LeaderboardRegistrationModal, LogViewer, MarketplaceModal, MergeProgressModal, MergeSessionModal, PlaygroundPanel, ProcessMonitor, PromptComposerModal, QuickActionsModal, QuitConfirmModal, SendToAgentModal, SettingsModal, StandingOvationOverlay, SummarizeProgressModal, SymphonyModal, TabSwitcherModal, TransferProgressModal, UsageDashboardModal, ExistingAutoRunDocsModal, ExistingDocsModal, MaestroWizard, WizardExitConfirmModal, WizardResumeModal, TourOverlay.
Sites skipped (9): overlay-type registrations (
type: 'overlay'/allowClickOutside) in DocumentGraph dropdowns, FileExplorerPanel, FilePreview, LightboxModal, TerminalOutput, TerminalSearchBar. The canonical hook hardcodestype: 'modal'; these need a separate variant.Test mock updates: 9 test files had their
useLayerStackmock extended withupdateLayerHandler; one NewInstanceModal assertion was rewritten to verify behavior (escape routes to latestonClose) instead of handler identity.10B - Group chat spawn helper
New:
src/main/group-chat/spawnGroupChatAgent.ts(~155 lines) encapsulates:processManager.spawninvocationConsolidated 4 call sites in
group-chat-router.ts: moderator spawn, participant spawn, synthesis spawn, recovery spawn. Each collapsed ~60 lines of imperative code to a single helper call.Store
resolve()consolidation: not applicable - no widespread duplication exists.Test plan
npm run lintpasses (all 3 tsconfigs)npx prettier --check .passesRisk
Medium. Modal layer touches keyboard routing across the app. 540+ tests cover the migrated paths and all are green.
Summary by CodeRabbit
Tests
Refactor
Chores