refactor: decompose MainPanel + NewInstanceModal into directory modules#710
Merged
reachrazamair merged 11 commits intorcfrom Apr 4, 2026
Merged
refactor: decompose MainPanel + NewInstanceModal into directory modules#710reachrazamair merged 11 commits intorcfrom
reachrazamair merged 11 commits intorcfrom
Conversation
…d hooks MainPanel.tsx (2,000 lines) decomposed into 7-file directory module (616-line shell) plus 6 domain hooks (431 lines) and 94 new tests (1,512 lines). Components: - MainPanelHeader: header bar (session name, SSH, git, auto mode, cost, context) - MainPanelContent: content routing (file preview, wizard, output, input, terminal) - AgentErrorBanner: error banner sub-component - CopyNotificationToast: flash notification Hooks: - useTerminalMounting: terminal mount/evict lifecycle - useContextWindow: context window loading + calculation - useFilePreviewHandlers: 7 file preview callbacks + memos - useGitInfo: git branch/status/detail consolidation - useSshRemoteName: SSH remote name resolution - useCopyToClipboard: clipboard + notification All 134 existing integration tests pass unchanged.
|
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:
📝 WalkthroughWalkthroughReplaces the old monolithic MainPanel with a composed implementation (new MainPanel forwardRef, MainPanelHeader, MainPanelContent), extracts many main-panel hooks, splits NewInstanceModal into smaller components, adds UI primitives and validation hooks, updates CueEngine subscription behavior, and adds many tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,220,255,0.5)
participant UI as MainPanel (UI)
participant Header as MainPanelHeader
participant Content as MainPanelContent
participant Hooks as MainPanel Hooks
participant Maestro as window.maestro
participant FS as FS (maestro.fs)
end
UI->>Hooks: request derived state (contextWindow, ssh name, git info, mounted terminals)
Hooks-->>UI: state (contextWindow, sshName, gitInfo, mounted ids)
UI->>Header: render header with git/ssh/context data
UI->>Content: render content (file preview | AI view | terminal) based on activeSession/inputMode
Content->>Maestro: async calls (agents.getConfig, sshRemote.getConfigs, gitService.getDiff)
Content->>FS: writeFile (save)
FS-->>Content: write result
Content->>Hooks: copyToClipboard -> set copyNotification
Hooks-->>UI: copyNotification update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 SummaryThis PR decomposes the 2,000-line monolithic The architecture is solid and the decomposition is well-executed, but three correctness issues were found in the new code:
Two style-level issues were also noted:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
MP["MainPanel (shell)\nforwardRef + React.memo"]
subgraph Hooks["Extracted hooks"]
UTM["useTerminalMounting\nhooks/terminal/"]
UCW["useContextWindow\nhooks/mainPanel/"]
UFPH["useFilePreviewHandlers\nhooks/mainPanel/"]
UGI["useGitInfo\nhooks/mainPanel/"]
USSH["useSshRemoteName\nhooks/mainPanel/"]
UCTC["useCopyToClipboard\nhooks/mainPanel/"]
end
subgraph Children["Child components"]
MPH["MainPanelHeader\n(React.memo)"]
MPC["MainPanelContent\n(React.memo)"]
AEB["AgentErrorBanner\n(React.memo)"]
CNT["CopyNotificationToast\n(React.memo)"]
TB["TabBar"]
end
subgraph Stores["Zustand stores"]
SS["settingsStore"]
UIS["uiStore"]
SES["sessionStore"]
end
MP --> UTM
MP --> UCW
MP --> UFPH
MP --> UGI
MP --> USSH
MP --> UCTC
UTM --> SES
MPH --> SS
MPH --> UIS
MPC --> SS
MPC --> UIS
MP --> MPH
MP --> MPC
MP --> AEB
MP --> CNT
MP --> TB
Reviews (1): Last reviewed commit: "refactor: decompose MainPanel.tsx into d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+213
to
+240
| useImperativeHandle( | ||
| ref, | ||
| () => ({ | ||
| refreshGitInfo: refreshGitStatus, | ||
| focusFilePreview: () => { | ||
| // Use the FilePreview's focus method if available, otherwise fallback to container | ||
| if (filePreviewRef.current) { | ||
| filePreviewRef.current.focus(); | ||
| } else { | ||
| filePreviewContainerRef.current?.focus(); | ||
| } | ||
| }, | ||
| clearActiveTerminal: () => { | ||
| if (activeSession) { | ||
| terminalViewRefs.current.get(activeSession.id)?.clearActiveTerminal(); | ||
| } | ||
| }, | ||
| focusActiveTerminal: () => { | ||
| if (activeSession) { | ||
| terminalViewRefs.current.get(activeSession.id)?.focusActiveTerminal(); | ||
| } | ||
| }, | ||
| openTerminalSearch: () => { | ||
| setTerminalSearchOpen(true); | ||
| }, | ||
| }), | ||
| [refreshGitStatus] | ||
| ); |
There was a problem hiding this comment.
useImperativeHandle — wrong session targeted after switch
clearActiveTerminal and focusActiveTerminal both close over activeSession to retrieve the session ID, but activeSession is absent from the dependency array. Once a user switches to a different session, the handle is not recreated, so both methods will keep looking up the original session's ID in terminalViewRefs. Any caller (e.g., a global keyboard shortcut) will silently operate on the wrong terminal.
Suggested change
| useImperativeHandle( | |
| ref, | |
| () => ({ | |
| refreshGitInfo: refreshGitStatus, | |
| focusFilePreview: () => { | |
| // Use the FilePreview's focus method if available, otherwise fallback to container | |
| if (filePreviewRef.current) { | |
| filePreviewRef.current.focus(); | |
| } else { | |
| filePreviewContainerRef.current?.focus(); | |
| } | |
| }, | |
| clearActiveTerminal: () => { | |
| if (activeSession) { | |
| terminalViewRefs.current.get(activeSession.id)?.clearActiveTerminal(); | |
| } | |
| }, | |
| focusActiveTerminal: () => { | |
| if (activeSession) { | |
| terminalViewRefs.current.get(activeSession.id)?.focusActiveTerminal(); | |
| } | |
| }, | |
| openTerminalSearch: () => { | |
| setTerminalSearchOpen(true); | |
| }, | |
| }), | |
| [refreshGitStatus] | |
| ); | |
| useImperativeHandle( | |
| ref, | |
| () => ({ | |
| refreshGitInfo: refreshGitStatus, | |
| focusFilePreview: () => { | |
| // Use the FilePreview's focus method if available, otherwise fallback to container | |
| if (filePreviewRef.current) { | |
| filePreviewRef.current.focus(); | |
| } else { | |
| filePreviewContainerRef.current?.focus(); | |
| } | |
| }, | |
| clearActiveTerminal: () => { | |
| if (activeSession) { | |
| terminalViewRefs.current.get(activeSession.id)?.clearActiveTerminal(); | |
| } | |
| }, | |
| focusActiveTerminal: () => { | |
| if (activeSession) { | |
| terminalViewRefs.current.get(activeSession.id)?.focusActiveTerminal(); | |
| } | |
| }, | |
| openTerminalSearch: () => { | |
| setTerminalSearchOpen(true); | |
| }, | |
| }), | |
| [refreshGitStatus, activeSession, setTerminalSearchOpen] | |
| ); |
Comment on lines
+12
to
+21
|
|
||
| const copyToClipboard = useCallback(async (text: string, message?: string) => { | ||
| const ok = await safeClipboardWrite(text); | ||
| if (ok) { | ||
| // Show centered flash notification | ||
| setCopyNotification(message || 'Copied to Clipboard'); | ||
| setTimeout(() => setCopyNotification(null), 2000); | ||
| } | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
Each call to copyToClipboard starts a new 2-second timer without cancelling any previously running timer. If the user copies two items within 2 seconds (e.g., branch name then remote URL), the first timer fires ~2 s after the first copy and calls setCopyNotification(null), wiping out the message that was set by the second copy and making the second notification disappear far too early.
Fix by tracking the timer in a ref and clearing it before scheduling a new one:
import { useState, useCallback, useRef } from 'react';
import { safeClipboardWrite } from '../../utils/clipboard';
export function useCopyToClipboard() {
const [copyNotification, setCopyNotification] = useState<string | null>(null);
const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const copyToClipboard = useCallback(async (text: string, message?: string) => {
const ok = await safeClipboardWrite(text);
if (ok) {
if (timerRef.current !== null) clearTimeout(timerRef.current);
setCopyNotification(message || 'Copied to Clipboard');
timerRef.current = setTimeout(() => {
setCopyNotification(null);
timerRef.current = null;
}, 2000);
}
}, []);
return { copyNotification, copyToClipboard };
}
Comment on lines
+288
to
+300
| const handleViewGitDiff = async () => { | ||
| if (!activeSession || !activeSession.isGitRepo) return; | ||
|
|
||
| const cwd = | ||
| activeSession.inputMode === 'terminal' | ||
| ? activeSession.shellCwd || activeSession.cwd | ||
| : activeSession.cwd; | ||
| const diff = await gitService.getDiff(cwd, undefined, filePreviewSshRemoteId); | ||
|
|
||
| if (diff.diff) { | ||
| setGitDiffPreview(diff.diff); | ||
| } | ||
| }; |
There was a problem hiding this comment.
React.memo on MainPanelHeader
handleViewGitDiff is a plain async function that is recreated on every render of MainPanel and then passed to the React.memo-wrapped MainPanelHeader as handleViewGitDiff. Because the reference changes every render, React.memo's shallow-comparison will always see a new value and re-render the header unnecessarily.
Suggested change
| const handleViewGitDiff = async () => { | |
| if (!activeSession || !activeSession.isGitRepo) return; | |
| const cwd = | |
| activeSession.inputMode === 'terminal' | |
| ? activeSession.shellCwd || activeSession.cwd | |
| : activeSession.cwd; | |
| const diff = await gitService.getDiff(cwd, undefined, filePreviewSshRemoteId); | |
| if (diff.diff) { | |
| setGitDiffPreview(diff.diff); | |
| } | |
| }; | |
| const handleViewGitDiff = useCallback(async () => { | |
| if (!activeSession || !activeSession.isGitRepo) return; | |
| const cwd = | |
| activeSession.inputMode === 'terminal' | |
| ? activeSession.shellCwd || activeSession.cwd | |
| : activeSession.cwd; | |
| const diff = await gitService.getDiff(cwd, undefined, filePreviewSshRemoteId); | |
| if (diff.diff) { | |
| setGitDiffPreview(diff.diff); | |
| } | |
| }, [activeSession, filePreviewSshRemoteId, setGitDiffPreview]); |
Comment on lines
+6
to
+14
| usageStats?: { | ||
| inputTokens?: number; | ||
| outputTokens?: number; | ||
| cacheCreationInputTokens?: number; | ||
| cacheReadInputTokens?: number; | ||
| contextWindow?: number; | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
AITab interface duplicates the canonical type
A narrow local AITab interface is defined here instead of importing the existing one from ../../types. If the shared AITab type gains required fields or renames existing ones, this local definition will silently diverge — the hook will still compile but may operate on a structurally incompatible object.
Suggested change
| usageStats?: { | |
| inputTokens?: number; | |
| outputTokens?: number; | |
| cacheCreationInputTokens?: number; | |
| cacheReadInputTokens?: number; | |
| contextWindow?: number; | |
| }; | |
| } | |
| import type { Session, AITab } from '../../types'; |
Then remove the local interface AITab { ... } block and update the function signature to accept AITab | null directly.
Comment on lines
+47
to
+81
| colorBlindMode: boolean; | ||
| contextWarningsEnabled: boolean; | ||
| contextWarningYellowThreshold: number; | ||
| contextWarningRedThreshold: number; | ||
| refreshGitStatus: () => Promise<void>; | ||
| handleViewGitDiff: () => Promise<void>; | ||
| copyToClipboard: (text: string, message?: string) => Promise<void>; | ||
| getContextColor: (usage: number, theme: Theme) => string; | ||
| setGitLogOpen?: (open: boolean) => void; | ||
| setAgentSessionsOpen: (open: boolean) => void; | ||
| setActiveAgentSessionId: (id: string | null) => void; | ||
| onStopBatchRun?: (sessionId?: string) => void; | ||
| onOpenWorktreeConfig?: () => void; | ||
| onOpenCreatePR?: () => void; | ||
| hasCapability: (cap: keyof AgentCapabilities) => boolean; | ||
| } | ||
|
|
||
| export const MainPanelHeader = React.memo(function MainPanelHeader({ | ||
| activeSession, | ||
| activeTab, | ||
| theme, | ||
| gitInfo, | ||
| sshRemoteName, | ||
| activeTabContextWindow, | ||
| activeTabContextTokens, | ||
| activeTabContextUsage, | ||
| isCurrentSessionAutoMode, | ||
| isCurrentSessionStopping, | ||
| currentSessionBatchState, | ||
| isWorktreeChild, | ||
| activeFileTabId, | ||
| colorBlindMode: _colorBlindMode, | ||
| contextWarningsEnabled: _contextWarningsEnabled, | ||
| contextWarningYellowThreshold: _contextWarningYellowThreshold, | ||
| contextWarningRedThreshold: _contextWarningRedThreshold, |
There was a problem hiding this comment.
colorBlindMode, contextWarningsEnabled, contextWarningYellowThreshold, and contextWarningRedThreshold are declared in MainPanelHeaderProps and passed by MainPanel, yet in the component body they are immediately discarded with _-prefixed destructuring aliases and never referenced again. Because MainPanelHeader is wrapped in React.memo, any time these store values change (e.g., the user tweaks context thresholds in settings) the memo's shallow comparison will detect a changed prop and schedule a re-render even though the output is unaffected.
Consider either:
- Removing these four props from the interface and the call-site in
MainPanel.tsx, sincegetContextColoralready encapsulates the colour logic, or - Genuinely using them inside the component if they are needed for future logic.
Collaborator
|
Hey @reachraza — first, thank you for this contribution! The decomposition is really well-structured and the test coverage is impressive. The overall architecture follows the established patterns nicely. However, I'd like to see a few issues addressed before we merge. The Greptile review flagged some real bugs that I've verified by reading the source on the branch: Must-fix (P1)
Should-fix (P2)
The P1 items are runtime correctness/perf bugs — please address those. The P2 items are cleanup that can be in this PR or a follow-up, your call. Also, CI tests are still pending — once those go green and the fixes above are in, we should be good to go. Thanks again for the solid work here! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/__tests__/renderer/hooks/useContextWindow.test.ts (1)
130-141: Make the unmount cleanup test deterministic with fake timers.Using real
setTimeoutdelays here can cause flaky timing. Prefer fake timers and explicit timer advancement for a stable cleanup test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useContextWindow.test.ts` around lines 130 - 141, The test "cleans up on unmount (isActive flag)" uses real async timeouts and should be made deterministic by switching to fake timers: in the test for useContextWindow, call jest.useFakeTimers() before rendering, replace the real setTimeout waits with explicit jest.advanceTimersByTime(...) calls to simulate the 100ms and 150ms delays, and call jest.useRealTimers() (or restoreTimers) at the end; update references to mockGetConfig, makeSession, and the renderHook/unmount flow so the hook's async work is driven by advancing timers rather than real time.src/__tests__/renderer/components/MainPanel/CopyNotificationToast.test.tsx (1)
30-35: Strengthen style assertions to verify actual theme mapping.
toBeTruthy()won’t catch incorrect colors. Assert exact expected values (or computed rgb equivalents) forbackgroundColorandcolor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/MainPanel/CopyNotificationToast.test.tsx` around lines 30 - 35, The test for CopyNotificationToast uses toBeTruthy() which doesn't validate the actual mapped colors; update the assertions to check exact expected values by deriving them from mockTheme (e.g., mockTheme.accent or the known mapped hex) and comparing against the computed styles of the rendered element: use window.getComputedStyle(toast).backgroundColor and .color and assert they equal the exact expected string (or the computed rgb equivalent) instead of toBeTruthy(); reference the CopyNotificationToast component and mockTheme in the test to obtain the correct expected color values.src/__tests__/renderer/hooks/useSshRemoteName.test.ts (1)
32-97: Add a regression test for out-of-ordergetConfigsresolution.Given async fetching on prop changes, add a test where request A resolves after request B and assert final state reflects B. This protects against stale-update regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSshRemoteName.test.ts` around lines 32 - 97, Add a regression test to ensure useSshRemoteName handles out-of-order getConfigs responses: mimic two sequential calls to mockGetConfigs that return controllable Promises (deferred) so you can resolve the second (B) before the first (A), renderHook with enabled/id A then rerender with id B, resolve B then A, and assert result.current equals B; implement by replacing mockGetConfigs with an implementation that returns the two deferred promises in order and use rerender + waitFor to check final state remains the latest request.src/renderer/hooks/terminal/useTerminalMounting.ts (1)
20-21: Update stale inline comment about terminal visibility strategy.Line 20 says terminals are kept alive with
display:none, but current rendering usesvisibility:hidden; this comment now conflicts with actual behavior and can mislead future edits.Suggested wording update
- // Once a session's terminals are mounted we keep them alive (display:none) so that + // Once a session's terminals are mounted we keep them alive (hidden, not unmounted) so that🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/terminal/useTerminalMounting.ts` around lines 20 - 21, The inline comment in useTerminalMounting.ts is stale: it says terminals are kept alive with `display:none` but the code uses `visibility:hidden`; update the comment (near useTerminalMounting / terminal mounting logic) to reflect the current strategy (e.g., "we keep mounted terminal elements hidden via `visibility:hidden` so their xterm.js buffers are preserved when switching sessions") or generalize the wording to avoid mentioning a specific CSS property.src/renderer/components/MainPanel/MainPanelHeader.tsx (1)
47-50: Trim the dead header props from the extracted surface.
colorBlindMode,contextWarningsEnabled,contextWarningYellowThreshold, andcontextWarningRedThresholdare still part ofMainPanelHeaderPropsand are still threaded fromMainPanel, but the header never reads them. Should I remove these now-unused elements:colorBlindMode,contextWarningsEnabled,contextWarningYellowThreshold,contextWarningRedThreshold?
As per coding guidelines, "After refactoring, identify now-unreachable code, list it explicitly, ask 'Should I remove these now-unused elements: [list]?' Don't leave corpses and don't delete without approval."Also applies to: 78-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MainPanel/MainPanelHeader.tsx` around lines 47 - 50, MainPanelHeaderProps still declares unused props (colorBlindMode, contextWarningsEnabled, contextWarningYellowThreshold, contextWarningRedThreshold) that are passed from MainPanel but never read by MainPanelHeader; remove these properties from the MainPanelHeaderProps type/interface and from the MainPanelHeader component signature, and also remove the corresponding prop threading in the MainPanel where it forwards those props to MainPanelHeader so no unused props are passed; ensure no other references to those four symbols remain in MainPanelHeader or MainPanel (search for colorBlindMode, contextWarningsEnabled, contextWarningYellowThreshold, contextWarningRedThreshold) before committing.
🤖 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/renderer/components/MainPanel/CopyNotificationToast.tsx`:
- Around line 16-24: The toast div in CopyNotificationToast should include
live-region semantics so screen readers announce copy feedback; update the
rendered <div> (inside the CopyNotificationToast component) to add
role="status", aria-live="polite", and aria-atomic="true" (optionally) to the
element with the className currently used for the toast so assistive tech will
announce the message when {message} changes.
In `@src/renderer/components/MainPanel/MainPanelHeader.tsx`:
- Around line 123-176: The git/SSH pill elements (see activeSession.isGitRepo,
sshRemoteName, gitTooltip.triggerHandlers, refreshGitStatus, setGitLogOpen) are
currently non-focusable spans/divs and must be keyboard accessible: replace
these interactive spans/divs with <button> (or add tabIndex={0} if button is not
possible), add outline-none class, wire onFocus to
gitTooltip.triggerHandlers.onMouseEnter and onBlur to
gitTooltip.triggerHandlers.onMouseLeave (and similarly for the SSH pill), ensure
tooltip state stays open while focus moves inside the tooltip by keeping the
tooltip open on focus events, and use a ref callback (ref={el => el?.focus()})
for any auto-focus behavior required by the coding guideline.
In `@src/renderer/hooks/mainPanel/useContextWindow.ts`:
- Around line 46-48: Replace the console-only catch in useContextWindow (the
catch block that currently logs "Failed to load agent context window setting")
with a Sentry report: call Sentry.captureException (or your app's Sentry
wrapper) passing the caught error and add contextual data (e.g., a message like
"Failed to load agent context window setting" and relevant locals such as
isActive) so the exception is visible in production, and keep the existing
fallback call to setConfiguredContextWindow(0).
In `@src/renderer/hooks/mainPanel/useCopyToClipboard.ts`:
- Around line 17-20: The hook useCopyToClipboard schedules a setTimeout for
dismissing notifications but doesn't cancel previous timers, so rapid copies can
be cleared prematurely; fix by adding a ref (e.g., timeoutRef) to store the
current timer id, call clearTimeout(timeoutRef.current) before creating a new
setTimeout, assign the new id to timeoutRef.current, and in the useEffect
cleanup clearTimeout(timeoutRef.current) to avoid leaks; keep using
setCopyNotification to set/clear the message.
In `@src/renderer/hooks/mainPanel/useFilePreviewHandlers.ts`:
- Around line 74-80: The current filePreviewCwd computation uses
startsWith(activeSession.fullPath) which can falsely match sibling paths; update
the guard to check directory boundary by comparing activeFileTab.path ===
activeSession.fullPath OR
activeFileTab.path.startsWith(activeSession.fullPathWithSep) where you derive
activeSession.fullPathWithSep by ensuring activeSession.fullPath ends with a
slash (e.g., append '/' if missing) and then slice only when the path has that
rooted prefix; update references in useMemo that compute relativePath/lastSlash
accordingly.
- Around line 63-70: The save handler handleFilePreviewSave currently always
calls window.maestro.fs.writeFile(path, content) treating previews as local;
change it to pass the derived filePreviewSshRemoteId so SSH-backed previews save
to the remote host — e.g., call window.maestro.fs.writeFile(path, content,
filePreviewSshRemoteId) or the equivalent API overload when
filePreviewSshRemoteId is set, and keep calling the existing two-arg form when
it's undefined; preserve the onFileTabEditContentChange update using
activeFileTabId after the save.
In `@src/renderer/hooks/mainPanel/useSshRemoteName.ts`:
- Line 28: The .catch currently swallows the IPC error; update the catch to both
setSshRemoteName(null) and report the exception to Sentry (e.g., call
Sentry.captureException or your project's reporting utility) including
contextual tags/message (mentioning useSshRemoteName and the IPC channel/action)
so errors are visible in telemetry while preserving the null fallback; modify
the promise chain in useSshRemoteName (the .catch that calls
setSshRemoteName(null)) to accept the error parameter and call the
Sentry/reporting function with the error and context before calling
setSshRemoteName(null).
- Around line 18-29: The async callback in useSshRemoteName can race and
overwrite newer selections; add an effect-local sequence guard: inside the
effect (where window.maestro.sshRemote.getConfigs() is called) create a local
token/seq counter (e.g., let current = ++localSeq or const callId = Symbol())
and capture it in the promise chain, then only call setSshRemoteName(...) if the
token still matches (and skip on mismatch); also mark the token as invalid in
the effect cleanup so any late resolution or catch does nothing. Reference
useSshRemoteName, setSshRemoteName, and window.maestro.sshRemote.getConfigs when
applying the guard.
---
Nitpick comments:
In `@src/__tests__/renderer/components/MainPanel/CopyNotificationToast.test.tsx`:
- Around line 30-35: The test for CopyNotificationToast uses toBeTruthy() which
doesn't validate the actual mapped colors; update the assertions to check exact
expected values by deriving them from mockTheme (e.g., mockTheme.accent or the
known mapped hex) and comparing against the computed styles of the rendered
element: use window.getComputedStyle(toast).backgroundColor and .color and
assert they equal the exact expected string (or the computed rgb equivalent)
instead of toBeTruthy(); reference the CopyNotificationToast component and
mockTheme in the test to obtain the correct expected color values.
In `@src/__tests__/renderer/hooks/useContextWindow.test.ts`:
- Around line 130-141: The test "cleans up on unmount (isActive flag)" uses real
async timeouts and should be made deterministic by switching to fake timers: in
the test for useContextWindow, call jest.useFakeTimers() before rendering,
replace the real setTimeout waits with explicit jest.advanceTimersByTime(...)
calls to simulate the 100ms and 150ms delays, and call jest.useRealTimers() (or
restoreTimers) at the end; update references to mockGetConfig, makeSession, and
the renderHook/unmount flow so the hook's async work is driven by advancing
timers rather than real time.
In `@src/__tests__/renderer/hooks/useSshRemoteName.test.ts`:
- Around line 32-97: Add a regression test to ensure useSshRemoteName handles
out-of-order getConfigs responses: mimic two sequential calls to mockGetConfigs
that return controllable Promises (deferred) so you can resolve the second (B)
before the first (A), renderHook with enabled/id A then rerender with id B,
resolve B then A, and assert result.current equals B; implement by replacing
mockGetConfigs with an implementation that returns the two deferred promises in
order and use rerender + waitFor to check final state remains the latest
request.
In `@src/renderer/components/MainPanel/MainPanelHeader.tsx`:
- Around line 47-50: MainPanelHeaderProps still declares unused props
(colorBlindMode, contextWarningsEnabled, contextWarningYellowThreshold,
contextWarningRedThreshold) that are passed from MainPanel but never read by
MainPanelHeader; remove these properties from the MainPanelHeaderProps
type/interface and from the MainPanelHeader component signature, and also remove
the corresponding prop threading in the MainPanel where it forwards those props
to MainPanelHeader so no unused props are passed; ensure no other references to
those four symbols remain in MainPanelHeader or MainPanel (search for
colorBlindMode, contextWarningsEnabled, contextWarningYellowThreshold,
contextWarningRedThreshold) before committing.
In `@src/renderer/hooks/terminal/useTerminalMounting.ts`:
- Around line 20-21: The inline comment in useTerminalMounting.ts is stale: it
says terminals are kept alive with `display:none` but the code uses
`visibility:hidden`; update the comment (near useTerminalMounting / terminal
mounting logic) to reflect the current strategy (e.g., "we keep mounted terminal
elements hidden via `visibility:hidden` so their xterm.js buffers are preserved
when switching sessions") or generalize the wording to avoid mentioning a
specific CSS property.
🪄 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: f9ace35a-9c3a-4ecd-8546-84b7b598c3bf
📒 Files selected for processing (24)
src/__tests__/renderer/components/MainPanel/AgentErrorBanner.test.tsxsrc/__tests__/renderer/components/MainPanel/CopyNotificationToast.test.tsxsrc/__tests__/renderer/components/MainPanel/MainPanelContent.test.tsxsrc/__tests__/renderer/components/MainPanel/MainPanelHeader.test.tsxsrc/__tests__/renderer/hooks/useContextWindow.test.tssrc/__tests__/renderer/hooks/useCopyToClipboard.test.tssrc/__tests__/renderer/hooks/useFilePreviewHandlers.test.tssrc/__tests__/renderer/hooks/useGitInfo.test.tssrc/__tests__/renderer/hooks/useSshRemoteName.test.tssrc/__tests__/renderer/hooks/useTerminalMounting.test.tssrc/renderer/components/MainPanel.tsxsrc/renderer/components/MainPanel/AgentErrorBanner.tsxsrc/renderer/components/MainPanel/CopyNotificationToast.tsxsrc/renderer/components/MainPanel/MainPanel.tsxsrc/renderer/components/MainPanel/MainPanelContent.tsxsrc/renderer/components/MainPanel/MainPanelHeader.tsxsrc/renderer/components/MainPanel/index.tssrc/renderer/components/MainPanel/types.tssrc/renderer/hooks/mainPanel/useContextWindow.tssrc/renderer/hooks/mainPanel/useCopyToClipboard.tssrc/renderer/hooks/mainPanel/useFilePreviewHandlers.tssrc/renderer/hooks/mainPanel/useGitInfo.tssrc/renderer/hooks/mainPanel/useSshRemoteName.tssrc/renderer/hooks/terminal/useTerminalMounting.ts
💤 Files with no reviewable changes (1)
- src/renderer/components/MainPanel.tsx
- CopyNotificationToast: add role="status", aria-live="polite", aria-atomic - MainPanelHeader: replace interactive spans with buttons for git/SSH pills, add onFocus/onBlur for keyboard tooltip access, remove unused props (colorBlindMode, contextWarnings*) - useContextWindow: replace console.error with Sentry captureException - useCopyToClipboard: add timeoutRef to cancel previous timers on rapid copies, clean up on unmount - useFilePreviewHandlers: fix directory boundary check (fullPath + '/'), pass sshRemoteId to writeFile for SSH-backed previews - useSshRemoteName: add Sentry reporting in catch, add sequence guard to prevent stale out-of-order responses from overwriting newer state - useTerminalMounting: fix stale comment (display:none → visibility:hidden) - Tests: exact color assertions, fake timers for deterministic cleanup test, SSH save test, out-of-order race regression test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/renderer/hooks/mainPanel/useContextWindow.ts (1)
6-14: Reuse the sharedAITabtype here.Line 6 redefines the tab shape this hook consumes. If
usageStatschanges insrc/renderer/types, this copy can drift silently from the realMainPaneldata flow. Prefer importing the canonical type here, orPick<>the fields you actually need from it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/mainPanel/useContextWindow.ts` around lines 6 - 14, The local interface AITab defined in useContextWindow.ts duplicates the canonical AITab/usageStats shape and may drift; replace this local definition by importing the shared AITab type (or use Pick<SharedAITab, "usageStats"> or Pick<SharedAITab["usageStats"], "contextWindow" | "inputTokens" | ...> to only select the fields you need) from the central types module and update any references in this file to use that imported type (e.g., change the local AITab to the imported AITab or a Pick-based alias) so the hook stays in sync with src/renderer/types.src/__tests__/renderer/hooks/useFilePreviewHandlers.test.ts (2)
55-76: Strengthen the memoization regression by rerendering with a new tab object.Line 74 rerenders with the same
tabreference, so this still passes if the hook regresses back to depending onactiveFileTabidentity. Re-render with an equivalent cloned object to prove the memo stays stable across value-equal tabs.🧪 Suggested change
- rerender({ tab }); // Same tab + rerender({ tab: { ...tab } }); // Same values, new reference expect(result.current.memoizedFilePreviewFile).toBe(first); // Same reference🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useFilePreviewHandlers.test.ts` around lines 55 - 76, The test currently re-renders with the same tab reference which won't catch regressions keyed on object identity; update the rerender call in the test that uses useFilePreviewHandlers so it supplies a new but value-equal tab object (e.g., a shallow clone of makeFileTab() or calling makeFileTab() again) instead of the original `tab` variable, so memoizedFilePreviewFile (from useFilePreviewHandlers) must remain referentially stable across value-equal tabs; locate the test around the assertions for memoizedFilePreviewFile and change the rerender({ tab }) to pass a cloned/equivalent tab.
78-98: Add the sibling-prefix regression case forfilePreviewCwd.Lines 78-98 verify happy paths, but they do not cover the exact
/test/projectvs/test/project2/...boundary bug this hook just fixed. Please add that case so a future fallback to plainstartsWith(fullPath)is caught immediately.🧪 Suggested test
+ it('returns empty cwd for sibling paths outside the project root', () => { + const { result } = renderHook(() => + useFilePreviewHandlers({ + activeSession: makeSession({ fullPath: '/test/project' }), + activeFileTabId: 'file-1', + activeFileTab: makeFileTab({ path: '/test/project2/test.ts' }), + }) + ); + expect(result.current.filePreviewCwd).toBe(''); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useFilePreviewHandlers.test.ts` around lines 78 - 98, Add a test to catch the sibling-prefix regression for useFilePreviewHandlers by asserting that a file path like '/test/project2/src/foo.ts' does not get treated as inside an activeSession with fullPath '/test/project'; specifically, in the test file useFilePreviewHandlers.test.ts, render the hook with activeSession: makeSession({ fullPath: '/test/project' }) and activeFileTab: makeFileTab({ path: '/test/project2/src/foo.ts' }) and assert result.current.filePreviewCwd is '' to ensure the hook performs a proper path-boundary check (not a plain startsWith).
🤖 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/renderer/components/MainPanel/MainPanel.tsx`:
- Around line 213-240: The imperative handle created in useImperativeHandle
(providing refreshGitInfo, focusFilePreview, clearActiveTerminal,
focusActiveTerminal, openTerminalSearch) closes over activeSession so
clearActiveTerminal() and focusActiveTerminal() can stale; update the dependency
array to include activeSession?.id (in addition to refreshGitStatus) so the
handle is rebound when the active terminal session changes (this ensures
mainPanelRef used by useMainKeyboardHandler and
terminalViewRefs.current.get(activeSession.id) target the current session).
- Around line 287-300: The handler handleViewGitDiff is recreated each render
causing MainPanelHeader (memoized with React.memo) to re-render; wrap
handleViewGitDiff in useCallback to memoize it before passing to
MainPanelHeader. Ensure the dependency array includes the reactive values used
inside the handler (e.g., activeSession or its relevant fields like
activeSession?.isGitRepo, activeSession?.inputMode, activeSession?.shellCwd,
activeSession?.cwd), filePreviewSshRemoteId, and any external services like
gitService (and include setGitDiffPreview if not guaranteed stable) so the
callback updates only when its inputs change.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useFilePreviewHandlers.test.ts`:
- Around line 55-76: The test currently re-renders with the same tab reference
which won't catch regressions keyed on object identity; update the rerender call
in the test that uses useFilePreviewHandlers so it supplies a new but
value-equal tab object (e.g., a shallow clone of makeFileTab() or calling
makeFileTab() again) instead of the original `tab` variable, so
memoizedFilePreviewFile (from useFilePreviewHandlers) must remain referentially
stable across value-equal tabs; locate the test around the assertions for
memoizedFilePreviewFile and change the rerender({ tab }) to pass a
cloned/equivalent tab.
- Around line 78-98: Add a test to catch the sibling-prefix regression for
useFilePreviewHandlers by asserting that a file path like
'/test/project2/src/foo.ts' does not get treated as inside an activeSession with
fullPath '/test/project'; specifically, in the test file
useFilePreviewHandlers.test.ts, render the hook with activeSession:
makeSession({ fullPath: '/test/project' }) and activeFileTab: makeFileTab({
path: '/test/project2/src/foo.ts' }) and assert result.current.filePreviewCwd is
'' to ensure the hook performs a proper path-boundary check (not a plain
startsWith).
In `@src/renderer/hooks/mainPanel/useContextWindow.ts`:
- Around line 6-14: The local interface AITab defined in useContextWindow.ts
duplicates the canonical AITab/usageStats shape and may drift; replace this
local definition by importing the shared AITab type (or use Pick<SharedAITab,
"usageStats"> or Pick<SharedAITab["usageStats"], "contextWindow" | "inputTokens"
| ...> to only select the fields you need) from the central types module and
update any references in this file to use that imported type (e.g., change the
local AITab to the imported AITab or a Pick-based alias) so the hook stays in
sync with src/renderer/types.
🪄 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: ef11874d-c122-41c2-9161-2da89641465e
📒 Files selected for processing (12)
src/__tests__/renderer/components/MainPanel/CopyNotificationToast.test.tsxsrc/__tests__/renderer/hooks/useContextWindow.test.tssrc/__tests__/renderer/hooks/useFilePreviewHandlers.test.tssrc/__tests__/renderer/hooks/useSshRemoteName.test.tssrc/renderer/components/MainPanel/CopyNotificationToast.tsxsrc/renderer/components/MainPanel/MainPanel.tsxsrc/renderer/components/MainPanel/MainPanelHeader.tsxsrc/renderer/hooks/mainPanel/useContextWindow.tssrc/renderer/hooks/mainPanel/useCopyToClipboard.tssrc/renderer/hooks/mainPanel/useFilePreviewHandlers.tssrc/renderer/hooks/mainPanel/useSshRemoteName.tssrc/renderer/hooks/terminal/useTerminalMounting.ts
✅ Files skipped from review due to trivial changes (4)
- src/tests/renderer/components/MainPanel/CopyNotificationToast.test.tsx
- src/renderer/hooks/mainPanel/useCopyToClipboard.ts
- src/renderer/components/MainPanel/CopyNotificationToast.tsx
- src/tests/renderer/hooks/useContextWindow.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/hooks/mainPanel/useSshRemoteName.ts
- src/tests/renderer/hooks/useSshRemoteName.test.ts
- src/renderer/hooks/terminal/useTerminalMounting.ts
- src/renderer/components/MainPanel/MainPanelHeader.tsx
- useImperativeHandle: add activeSession?.id to deps so terminal ref methods rebind when the active session changes - handleViewGitDiff: wrap in useCallback to prevent unnecessary MainPanelHeader re-renders (React.memo break) - useContextWindow: replace local AITab interface with canonical import from ../../types to prevent type drift - useFilePreviewHandlers test: rerender with value-equal clone to verify memo stability on object identity change; add sibling-prefix regression test for filePreviewCwd boundary check
getGraphData() filtered subscriptions by agent_id, excluding those targeting inactive sessions. When the pipeline was reconstructed from filtered data and saved, those subscriptions were permanently lost. Remove the filter — downstream dedup in graphSessionsToPipelines() already handles duplicates across sessions sharing the same cue.yaml.
The prompts container did not flex to fill available space in collapsed mode, and a maxHeight constraint caused multiple edge prompt rows to overlap. Use flex: 1 unconditionally and add overflow: auto so the collapsed panel scrolls instead of cramming content.
Increase collapsed panel heights (240→320, 280→360, 340→420), textarea min-heights, and container gap so input fields have room to breathe instead of feeling cramped.
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Split NewInstanceModal.tsx (1,845 lines) into 8 focused files following established MainPanel/SessionList/Settings decomposition patterns. Structure: - NewInstanceModal/NewInstanceModal.tsx (816) — create mode shell - NewInstanceModal/EditAgentModal.tsx (547) — edit mode, own file - NewInstanceModal/AgentPickerGrid.tsx (325) — agent selection grid (memo'd) - NewInstanceModal/NudgeMessageField.tsx (37) — shared nudge textarea (memo'd) - NewInstanceModal/RemotePathStatus.tsx (48) — remote path validation display (memo'd) - NewInstanceModal/types.ts (122) — interfaces and constants - NewInstanceModal/index.ts (2) — barrel preserving import paths - hooks/agent/useRemotePathValidation.ts (92) — debounced SSH path validation 58 new tests across 5 test files (130 total), 24,309 full suite passing.
reachrazamair
changed the title
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
src/renderer/components/NewInstanceModal/NewInstanceModal.tsx (2)
732-735: PassremoteHosttoRemotePathStatusfor more informative messaging.The
sshRemoteHostis computed at lines 102-110 but not passed toRemotePathStatus. Without it, users will see generic messages like "Checking remote path..." instead of host-specific ones like "Checking path on my-server.com...".♻️ Proposed fix
{/* Remote path validation status (only shown when SSH is enabled) */} {isSshEnabled && workingDir.trim() && ( - <RemotePathStatus theme={theme} validation={remotePathValidation} /> + <RemotePathStatus theme={theme} validation={remotePathValidation} remoteHost={sshRemoteHost} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/NewInstanceModal.tsx` around lines 732 - 735, Pass the computed sshRemoteHost into the RemotePathStatus component so its messages are host-specific: in NewInstanceModal, where RemotePathStatus is rendered (inside the isSshEnabled && workingDir.trim() conditional), add a prop remoteHost={sshRemoteHost} (sshRemoteHost is computed around lines where sshRemoteHost is defined in NewInstanceModal) and update the RemotePathStatus prop handling accordingly so it can include the host in its UI messages.
61-66: Consider memoizingexpandTildeto stabilizehandleCreatereferences.The
expandTildefunction is recreated on every render and is listed inhandleCreate's dependency array (line 385). This causeshandleCreateto be recreated on every render, which may cause unnecessary re-renders ofModalFooter. SinceexpandTildeonly depends onhomeDir, wrapping it inuseCallbackwould stabilize its reference.♻️ Proposed fix
- // Expand tilde in path - const expandTilde = (path: string): string => { - if (!homeDir) return path; - if (path === '~') return homeDir; - if (path.startsWith('~/')) return homeDir + path.slice(1); - return path; - }; + // Expand tilde in path + const expandTilde = useCallback( + (path: string): string => { + if (!homeDir) return path; + if (path === '~') return homeDir; + if (path.startsWith('~/')) return homeDir + path.slice(1); + return path; + }, + [homeDir] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/NewInstanceModal.tsx` around lines 61 - 66, The expandTilde function is recreated each render causing handleCreate (which lists expandTilde in its deps) to be unstable and trigger extra renders (e.g., ModalFooter); wrap expandTilde in React.useCallback (depend on homeDir) so its reference is stable, update its declaration to useCallback(() => { ... }, [homeDir]) and ensure React/useCallback is imported so handleCreate's dependency can stay unchanged.src/__tests__/renderer/components/NewInstanceModal/EditAgentModal.test.tsx (3)
420-451: Test assertion doesn't match test name.The test is titled "should render SSH remote selector when remotes exist" but only asserts that
getConfigswas called. It doesn't verify that theSshRemoteSelectorcomponent actually renders in the DOM when remotes are present.Suggested improvement
// SSH selector should appear after SSH configs load await waitFor(() => { - // The SshRemoteSelector renders when remotes exist - // We can verify it loaded by checking the remotes were fetched - expect(window.maestro.sshRemote.getConfigs).toHaveBeenCalled(); + // Verify the SSH remote selector UI is rendered + expect(screen.getByText(/SSH Remote Execution/i)).toBeInTheDocument(); + // Or check for the specific remote name + expect(screen.getByText('Dev Server')).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/NewInstanceModal/EditAgentModal.test.tsx` around lines 420 - 451, The test currently only asserts that window.maestro.sshRemote.getConfigs was called but the title says "should render SSH remote selector when remotes exist"; update the assertion to confirm the SshRemoteSelector actually appears in the DOM after configs load. After mocking getConfigs and rendering EditAgentModal, use the existing waitFor (or findBy* helpers) to wait for and assert presence of the SshRemoteSelector (e.g., by test id/label/text unique to SshRemoteSelector or by checking rendered remote name "Dev Server") instead of only asserting getConfigs was called.
50-68:as anycast bypasses type safety.The
as anytype assertion masks potential type mismatches between the test fixture and the actualSessiontype. IfSessiongains required fields, these tests will continue passing with incomplete data.Consider using
Partial<Session>combined with a type assertion toSession, or creating a more complete factory that satisfies theSessioninterface withoutany.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/NewInstanceModal/EditAgentModal.test.tsx` around lines 50 - 68, The test factory createSession currently ends with an unsafe "as any" cast; remove that and ensure the factory satisfies the Session type explicitly by returning a properly-typed Session. Change the signature of createSession to accept overrides: Partial<Session> and return a Session, and replace the trailing "as any" with a safe type assertion like "as Session" (or build the fixture so it fully matches Session), so the returned object from createSession (and any overrides) are checked against the Session interface.
19-25: Incomplete LayerStackContext mock may cause false positives.The mock only provides
registerLayer,unregisterLayer, andupdateLayerHandler, but the actualLayerStackAPIinterface (fromsrc/renderer/hooks/ui/useLayerStack.ts) includes additional methods:getTopLayer,closeTopLayer,getLayers,hasOpenLayers,hasOpenModal, andlayerCount. If the component under test accesses any of these missing methods, tests will pass with undefined returns rather than failing explicitly.Suggested improvement
vi.mock('../../../../renderer/contexts/LayerStackContext', () => ({ useLayerStack: () => ({ registerLayer: mockRegisterLayer, unregisterLayer: mockUnregisterLayer, updateLayerHandler: mockUpdateLayerHandler, + getTopLayer: vi.fn(() => undefined), + closeTopLayer: vi.fn(() => Promise.resolve(true)), + getLayers: vi.fn(() => []), + hasOpenLayers: vi.fn(() => false), + hasOpenModal: vi.fn(() => false), + layerCount: 0, }), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/NewInstanceModal/EditAgentModal.test.tsx` around lines 19 - 25, The LayerStackContext mock in the test only supplies registerLayer/unregisterLayer/updateLayerHandler causing potential false positives; update the vi.mock for useLayerStack to implement the full LayerStackAPI by adding getTopLayer (returning null or a mock layer), closeTopLayer (jest/vi.fn), getLayers (returning an empty array), hasOpenLayers (returning false), hasOpenModal (returning false), and layerCount (returning 0) alongside the existing mockRegisterLayer, mockUnregisterLayer, and mockUpdateLayerHandler so any component access to those methods fails loudly or behaves predictably in tests.src/renderer/components/NewInstanceModal/EditAgentModal.tsx (3)
88-92: Consider Sentry reporting for API failures.Multiple
catchblocks only log to console (lines 91, 130, 252, 266, 514-516). While these appear to be recoverable errors where the UI continues functioning, capturing them in Sentry would aid monitoring and debugging production issues.As per coding guidelines: "Use Sentry utilities for explicit reporting: import
captureException... to report exceptions with context."Also applies to: 128-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx` around lines 88 - 92, The catch blocks that currently only console.error (e.g., the promise on window.maestro.agents.getModels used with setAvailableModels and setLoadingModels) should also report exceptions to Sentry: import captureException from the project's Sentry utilities and call captureException(err, { contexts: { component: 'EditAgentModal', action: 'getModels' , toolType: activeToolType } }) (or similar contextual metadata) inside each catch before or after the console.error; apply the same pattern to the other catches referenced (around lines handling model loading and related API calls) so failures are captured with context while preserving existing UI behavior.
148-156: Potential double API calls on modal open.When the modal opens, the second
useEffect(lines 149-155) setsselectedToolTypefromsession.toolType, which triggers the firstuseEffect(lines 75-146) to run again sinceselectedToolTypeis in its dependency array. This could cause redundant API calls toagents.detect(),agents.getConfig(), etc.Consider consolidating these effects or guarding against the initial toolType being the same as
session.toolType.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx` around lines 148 - 156, The modal population effect is causing a second useEffect to re-run and trigger redundant API calls (agents.detect, agents.getConfig) because setSelectedToolType(session.toolType) changes selectedToolType in the dependency array; to fix, change the population logic in the useEffect that runs on isOpen/session to only call setSelectedToolType if session.toolType !== selectedToolType (or consolidate the two effects into one initialization effect that sets instanceName, nudgeMessage and selectedToolType together and then performs detection/config only once), referencing the useEffect that currently sets instanceName/nudgeMessage/selectedToolType and the effect that depends on selectedToolType (which calls agents.detect and agents.getConfig) so you can add the equality guard or merge the logic to avoid duplicate API calls.
47-47: Unused state variable_customModel.The
_customModelstate is set viasetCustomModel(lines 138, 143) but never read. The actual model value is derived fromagentConfig.modelat line 197. This is dead state that adds confusion.Suggested removal
- const [_customModel, setCustomModel] = useState('');And update lines 138 and 143 to remove the
setCustomModelcalls, or if the intent was to track the original session model, integrate it properly withagentConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx` at line 47, Remove the dead state _customModel and its updater setCustomModel from EditAgentModal: delete the useState declaration for _customModel and remove or replace the two setCustomModel(...) calls currently invoked (they are the only places updating that state), and if tracking an original/custom model was intended instead integrate that value into agentConfig (e.g., set agentConfig.model) or persist it elsewhere; ensure all UI logic reads from agentConfig.model (the existing source of truth) so no unused state remains.src/renderer/components/NewInstanceModal/types.ts (2)
41-41:theme: anyloses type safety.Lines 41 and 65 use
theme: anyfor modal props, butThemeis imported at line 1 and correctly used inNudgeMessageFieldProps(line 78) and other interfaces. This inconsistency reduces type safety for the modal components.Suggested fix
export interface NewInstanceModalProps { // ... - theme: any; + theme: Theme; // ... } export interface EditAgentModalProps { // ... - theme: any; + theme: Theme; // ... }Also applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/types.ts` at line 41, The modal props currently declare theme: any which loses type safety; update those props to use the imported Theme type instead of any (e.g., change theme: any to theme: Theme) for the modal interfaces where it appears (for example NewInstanceModalProps and the other modal props at the second occurrence), and ensure the existing Theme import at the top is used and not duplicated.
35-40: Consider extracting repeatedsessionSshRemoteConfigtype.The same inline type structure is defined in both
NewInstanceModalProps(lines 35-39) andEditAgentModalProps(lines 59-63). Extracting to a named type would reduce duplication and ensure consistency.Suggested extraction
+export interface SessionSshRemoteConfigInput { + enabled: boolean; + remoteId: string | null; + workingDirOverride?: string; +} export interface NewInstanceModalProps { // ... - sessionSshRemoteConfig?: { - enabled: boolean; - remoteId: string | null; - workingDirOverride?: string; - } + sessionSshRemoteConfig?: SessionSshRemoteConfigInput; // ... }Also applies to: 59-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/types.ts` around lines 35 - 40, Extract the repeated inline object type used for sessionSshRemoteConfig into a single exported named type (e.g., SessionSshRemoteConfig) that declares enabled: boolean, remoteId: string | null, and optional workingDirOverride?: string; then replace the inline declarations in NewInstanceModalProps and EditAgentModalProps with that new named type and export it so both interfaces reference the same type for consistency.
🤖 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/renderer/components/NewInstanceModal/AgentPickerGrid.tsx`:
- Around line 73-74: The parent container in AgentPickerGrid that renders
sortedAgents via sortedAgents.map currently lacks role="listbox" while its
children (the agent cards rendered inside the map, referenced as role="option")
are ARIA options; update the parent div with className "space-y-1" to include
role="listbox" (and ensure it remains the container for the mapped items) so the
options are correctly contained within a listbox for accessibility compliance.
- Around line 303-307: The PATH display mapping in AgentPickerGrid uses a
non-unique key `${debugInfo.platform}-${p}` which can collide when the same path
segment repeats; update the map over debugInfo.envPath.split(':') to use a
stable unique key per element (e.g., include the map index or another unique
discriminator) so each rendered <div> gets a distinct key; locate the mapping
inside the AgentPickerGrid component where debugInfo.envPath is split and change
the key construction to incorporate the index (or a generated unique id) to
avoid React reconciliation issues.
In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx`:
- Around line 62-69: handleCopySessionId can suffer a race where multiple rapid
clicks leave an earlier setTimeout clearing a newer copied state; fix by storing
the timeout id in a ref (e.g., copyTimeoutRef) inside the component, clear any
existing timeout before creating a new one, assign the new timeout id to the ref
when setting setCopiedId(true) and scheduling setCopiedId(false), and add a
cleanup effect (useEffect cleanup) to clear the timeout on unmount; update
handleCopySessionId to clear copyTimeoutRef.current before calling setTimeout to
prevent premature clearing.
In `@src/renderer/hooks/agent/useRemotePathValidation.ts`:
- Around line 50-86: The timeout's async callback can call setValidation after
unmount; add an "aborted"/"cancelled" flag in useEffect scope, set it true in
the cleanup alongside clearTimeout(timeoutId), and check the flag before every
setValidation inside the setTimeout async function (both after the awaited
window.maestro.fs.stat and inside the catch) so no state updates run if
cancelled; reference the timeoutId, debounceMs, window.maestro.fs.stat, and
setValidation in your change.
---
Nitpick comments:
In `@src/__tests__/renderer/components/NewInstanceModal/EditAgentModal.test.tsx`:
- Around line 420-451: The test currently only asserts that
window.maestro.sshRemote.getConfigs was called but the title says "should render
SSH remote selector when remotes exist"; update the assertion to confirm the
SshRemoteSelector actually appears in the DOM after configs load. After mocking
getConfigs and rendering EditAgentModal, use the existing waitFor (or findBy*
helpers) to wait for and assert presence of the SshRemoteSelector (e.g., by test
id/label/text unique to SshRemoteSelector or by checking rendered remote name
"Dev Server") instead of only asserting getConfigs was called.
- Around line 50-68: The test factory createSession currently ends with an
unsafe "as any" cast; remove that and ensure the factory satisfies the Session
type explicitly by returning a properly-typed Session. Change the signature of
createSession to accept overrides: Partial<Session> and return a Session, and
replace the trailing "as any" with a safe type assertion like "as Session" (or
build the fixture so it fully matches Session), so the returned object from
createSession (and any overrides) are checked against the Session interface.
- Around line 19-25: The LayerStackContext mock in the test only supplies
registerLayer/unregisterLayer/updateLayerHandler causing potential false
positives; update the vi.mock for useLayerStack to implement the full
LayerStackAPI by adding getTopLayer (returning null or a mock layer),
closeTopLayer (jest/vi.fn), getLayers (returning an empty array), hasOpenLayers
(returning false), hasOpenModal (returning false), and layerCount (returning 0)
alongside the existing mockRegisterLayer, mockUnregisterLayer, and
mockUpdateLayerHandler so any component access to those methods fails loudly or
behaves predictably in tests.
In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx`:
- Around line 88-92: The catch blocks that currently only console.error (e.g.,
the promise on window.maestro.agents.getModels used with setAvailableModels and
setLoadingModels) should also report exceptions to Sentry: import
captureException from the project's Sentry utilities and call
captureException(err, { contexts: { component: 'EditAgentModal', action:
'getModels' , toolType: activeToolType } }) (or similar contextual metadata)
inside each catch before or after the console.error; apply the same pattern to
the other catches referenced (around lines handling model loading and related
API calls) so failures are captured with context while preserving existing UI
behavior.
- Around line 148-156: The modal population effect is causing a second useEffect
to re-run and trigger redundant API calls (agents.detect, agents.getConfig)
because setSelectedToolType(session.toolType) changes selectedToolType in the
dependency array; to fix, change the population logic in the useEffect that runs
on isOpen/session to only call setSelectedToolType if session.toolType !==
selectedToolType (or consolidate the two effects into one initialization effect
that sets instanceName, nudgeMessage and selectedToolType together and then
performs detection/config only once), referencing the useEffect that currently
sets instanceName/nudgeMessage/selectedToolType and the effect that depends on
selectedToolType (which calls agents.detect and agents.getConfig) so you can add
the equality guard or merge the logic to avoid duplicate API calls.
- Line 47: Remove the dead state _customModel and its updater setCustomModel
from EditAgentModal: delete the useState declaration for _customModel and remove
or replace the two setCustomModel(...) calls currently invoked (they are the
only places updating that state), and if tracking an original/custom model was
intended instead integrate that value into agentConfig (e.g., set
agentConfig.model) or persist it elsewhere; ensure all UI logic reads from
agentConfig.model (the existing source of truth) so no unused state remains.
In `@src/renderer/components/NewInstanceModal/NewInstanceModal.tsx`:
- Around line 732-735: Pass the computed sshRemoteHost into the RemotePathStatus
component so its messages are host-specific: in NewInstanceModal, where
RemotePathStatus is rendered (inside the isSshEnabled && workingDir.trim()
conditional), add a prop remoteHost={sshRemoteHost} (sshRemoteHost is computed
around lines where sshRemoteHost is defined in NewInstanceModal) and update the
RemotePathStatus prop handling accordingly so it can include the host in its UI
messages.
- Around line 61-66: The expandTilde function is recreated each render causing
handleCreate (which lists expandTilde in its deps) to be unstable and trigger
extra renders (e.g., ModalFooter); wrap expandTilde in React.useCallback (depend
on homeDir) so its reference is stable, update its declaration to useCallback(()
=> { ... }, [homeDir]) and ensure React/useCallback is imported so
handleCreate's dependency can stay unchanged.
In `@src/renderer/components/NewInstanceModal/types.ts`:
- Line 41: The modal props currently declare theme: any which loses type safety;
update those props to use the imported Theme type instead of any (e.g., change
theme: any to theme: Theme) for the modal interfaces where it appears (for
example NewInstanceModalProps and the other modal props at the second
occurrence), and ensure the existing Theme import at the top is used and not
duplicated.
- Around line 35-40: Extract the repeated inline object type used for
sessionSshRemoteConfig into a single exported named type (e.g.,
SessionSshRemoteConfig) that declares enabled: boolean, remoteId: string | null,
and optional workingDirOverride?: string; then replace the inline declarations
in NewInstanceModalProps and EditAgentModalProps with that new named type and
export it so both interfaces reference the same type for consistency.
🪄 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: 5aeaa1ec-103f-4a6f-8418-474e452215b0
📒 Files selected for processing (15)
src/__tests__/renderer/components/NewInstanceModal/AgentPickerGrid.test.tsxsrc/__tests__/renderer/components/NewInstanceModal/EditAgentModal.test.tsxsrc/__tests__/renderer/components/NewInstanceModal/NudgeMessageField.test.tsxsrc/__tests__/renderer/components/NewInstanceModal/RemotePathStatus.test.tsxsrc/__tests__/renderer/hooks/useRemotePathValidation.test.tssrc/renderer/components/NewInstanceModal.tsxsrc/renderer/components/NewInstanceModal/AgentPickerGrid.tsxsrc/renderer/components/NewInstanceModal/EditAgentModal.tsxsrc/renderer/components/NewInstanceModal/NewInstanceModal.tsxsrc/renderer/components/NewInstanceModal/NudgeMessageField.tsxsrc/renderer/components/NewInstanceModal/RemotePathStatus.tsxsrc/renderer/components/NewInstanceModal/index.tssrc/renderer/components/NewInstanceModal/types.tssrc/renderer/hooks/agent/index.tssrc/renderer/hooks/agent/useRemotePathValidation.ts
💤 Files with no reviewable changes (1)
- src/renderer/components/NewInstanceModal.tsx
✅ Files skipped from review due to trivial changes (1)
- src/renderer/components/NewInstanceModal/index.ts
Inline fixes: - Add role="listbox" to AgentPickerGrid agent list container for ARIA compliance - Fix PATH debug display key collision by including index in map key - Fix handleCopySessionId timeout race with ref-based cleanup - Add cancelled flag in useRemotePathValidation to prevent post-unmount setState Nitpick fixes: - SSH selector test now asserts DOM presence instead of just IPC call - Replace unsafe `as any` cast with `as Session` in test factory - Expand LayerStack mock with full API surface (getTopLayer, getLayers, etc.) - Remove dead `_customModel` state and its setCustomModel calls - Add equality guard in population effect to prevent redundant API calls - Pass remoteHost to RemotePathStatus in create modal for host-specific messages - Wrap expandTilde in useCallback for stable reference - Replace theme:any with Theme type, extract SessionSshRemoteConfig named type
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/renderer/components/NewInstanceModal/AgentPickerGrid.tsx`:
- Around line 108-113: The header row's onKeyDown handler is firing for nested
controls and buttons default to submit; update the onKeyDown where
handleAgentHeaderActivate is invoked to ignore events that originate from child
elements (e.g., only call handleAgentHeaderActivate when e.target ===
e.currentTarget or when the row itself has focus) so nested buttons don't bubble
Enter/Space to the row, and set explicit type="button" on the refresh and other
inline Button components (the buttons rendered near the refresh control and the
agent action) to prevent unintended form submissions; reference the onKeyDown
that calls handleAgentHeaderActivate and the Button elements near the refresh
and agent action to locate changes.
- Around line 169-177: The two button elements in AgentPickerGrid.tsx (the one
with title "Refresh detection" that calls onRefreshAgent(agent.id) and the other
button block around lines 309-313) are missing an explicit type attribute; add
type="button" to both button elements to make them semantically correct and
consistent with other buttons (e.g., the Modal close button) so they cannot be
treated as form submit buttons.
In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx`:
- Around line 96-100: The promise handlers that now only call console.error
(e.g., the window.maestro.agents.getModels(activeToolType) chain that calls
setAvailableModels and setLoadingModels, plus the similar handlers around SSH
loading, refresh actions, and the config write path in the save flow) must
surface failures to Sentry and prevent the UI from proceeding as if successful:
import captureException (or captureMessage) from the renderer sentry util and
replace console.error calls with captureException(err, { extra: { context:
'EditAgentModal.getModels' } }) (use distinct context strings like
'EditAgentModal.loadSSH', 'EditAgentModal.refresh',
'EditAgentModal.saveConfig'), then either rethrow the error or set an explicit
error state so the save/refresh UI path aborts instead of continuing; for the
write/save block (the block that currently proceeds on lines around the config
write), ensure the catch reports via captureException and blocks the success
flow (do not call the success handlers or close the modal when captureException
is invoked).
- Around line 61-77: The copied badge state and timeout are only cleared on
unmount, so copiedId can persist when the modal is closed or when session
changes; update the component to clear copyTimeoutRef and reset copiedId
whenever isOpen becomes false or session?.id changes by adding a useEffect that
watches [isOpen, session?.id] and, on change to closed or new id, clears any
existing timeout (copyTimeoutRef.current) and sets copiedId to false; reference
copyTimeoutRef, copiedId, handleCopySessionId, isOpen and session?.id to locate
where to add this reset logic.
- Around line 188-192: The remote-path validator is using session?.projectRoot
instead of the effective working dir, so the status banner can show a different
path than what you persist; update the useRemotePathValidation call in
EditAgentModal to pass the path as sshRemoteConfig?.workingDirOverride ??
session?.projectRoot ?? '' (i.e., prefer sshRemoteConfig.workingDirOverride and
fall back to session.projectRoot) while keeping the existing isSshEnabled and
sshRemoteId arguments unchanged so the banner reflects the actual persisted
working directory.
- Around line 83-162: The first useEffect that loads agent data (the effect
referencing selectedToolType, session and calling
window.maestro.agents.detect/getModels/getConfig and setting state via setAgent,
setAvailableModels, setAgentConfig, etc.) can start using a stale
selectedToolType before the second effect updates it; modify this effect to
first check that selectedToolType === session.toolType (or wait until they
match) before triggering detect/getModels/getConfig and the per-session
hydrations (setCustomPath/setCustomArgs/setCustomEnvVars); additionally add a
local request identifier or "stale" flag in the effect closure and clear it in
the cleanup to ignore any late promise results (or abort requests) so that
out-of-order responses do not overwrite newer state.
🪄 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: b202ff30-6e9f-4a9a-b6f6-52e1f1b44c94
📒 Files selected for processing (7)
src/__tests__/renderer/components/NewInstanceModal.test.tsxsrc/__tests__/renderer/components/NewInstanceModal/EditAgentModal.test.tsxsrc/renderer/components/NewInstanceModal/AgentPickerGrid.tsxsrc/renderer/components/NewInstanceModal/EditAgentModal.tsxsrc/renderer/components/NewInstanceModal/NewInstanceModal.tsxsrc/renderer/components/NewInstanceModal/types.tssrc/renderer/hooks/agent/useRemotePathValidation.ts
✅ Files skipped from review due to trivial changes (2)
- src/tests/renderer/components/NewInstanceModal.test.tsx
- src/renderer/components/NewInstanceModal/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tests/renderer/components/NewInstanceModal/EditAgentModal.test.tsx
- src/renderer/hooks/agent/useRemotePathValidation.ts
- src/renderer/components/NewInstanceModal/NewInstanceModal.tsx
Comment on lines
+96
to
+100
| window.maestro.agents | ||
| .getModels(activeToolType) | ||
| .then((models) => setAvailableModels(models)) | ||
| .catch((err) => console.error('Failed to load models:', err)) | ||
| .finally(() => setLoadingModels(false)); |
There was a problem hiding this comment.
Don't swallow these async failures with console.error only.
These branches log and continue, so model loading, SSH loading, refresh actions, and config persistence can fail without Sentry visibility or any explicit recovery path. The write path on Lines 519-523 is the riskiest one here because the UI proceeds as if the config saved. Please either handle these as expected failures or report/rethrow them via the renderer Sentry utilities.
As per coding guidelines "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production." and "Use Sentry utilities for explicit reporting: import captureException and captureMessage from src/main/utils/sentry.ts or src/renderer/utils/sentry.ts to report exceptions with context and notable events."
Also applies to: 131-138, 258-260, 272-274, 519-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx` around lines 96
- 100, The promise handlers that now only call console.error (e.g., the
window.maestro.agents.getModels(activeToolType) chain that calls
setAvailableModels and setLoadingModels, plus the similar handlers around SSH
loading, refresh actions, and the config write path in the save flow) must
surface failures to Sentry and prevent the UI from proceeding as if successful:
import captureException (or captureMessage) from the renderer sentry util and
replace console.error calls with captureException(err, { extra: { context:
'EditAgentModal.getModels' } }) (use distinct context strings like
'EditAgentModal.loadSSH', 'EditAgentModal.refresh',
'EditAgentModal.saveConfig'), then either rethrow the error or set an explicit
error state so the save/refresh UI path aborts instead of continuing; for the
write/save block (the block that currently proceeds on lines around the config
write), ensure the catch reports via captureException and blocks the success
flow (do not call the success handlers or close the modal when captureException
is invoked).
- Add e.target === e.currentTarget guard to AgentPickerGrid onKeyDown to prevent nested button Enter/Space from bubbling to the header row - Add type="button" to refresh and dismiss buttons in AgentPickerGrid - Reset copiedId state and clear timeout when modal closes or session changes - Use sshRemoteConfig.workingDirOverride in remote path validation fallback so the banner reflects the actual persisted working directory - Add stale flag to EditAgentModal loading effect to prevent out-of-order promise results from overwriting newer state
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/renderer/components/NewInstanceModal/AgentPickerGrid.tsx (1)
73-73:⚠️ Potential issue | 🟡 MinorAdd an accessible name to the listbox container.
The listbox currently has no explicit accessible name, so assistive tech may announce it ambiguously.
Suggested fix
- <div className="space-y-1" role="listbox"> + <div className="space-y-1" role="listbox" aria-label="Agent Provider">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/AgentPickerGrid.tsx` at line 73, The div with role="listbox" in AgentPickerGrid.tsx lacks an accessible name; add an explicit label by either adding aria-label="..." (e.g., aria-label="Agent picker") or aria-labelledby referencing a visible heading element (create or reuse an element id and set aria-labelledby="that-id") on the same container; ensure the label text is descriptive for screen readers and update any nearby heading element if using aria-labelledby to match the chosen label.src/renderer/components/NewInstanceModal/EditAgentModal.tsx (1)
94-129:⚠️ Potential issue | 🟠 MajorMissing error handlers on
detect()andgetConfig()promises.Both
window.maestro.agents.detect()(line 95) andwindow.maestro.agents.getConfig()(line 118) lack.catch()handlers entirely. If these IPC calls fail, the promise rejections go unhandled, which can cause silent failures with no user feedback or Sentry visibility.As per coding guidelines: "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production."
Suggested fix
// Load agent definition to get configOptions window.maestro.agents.detect().then((agents: AgentConfig[]) => { if (stale) return; const foundAgent = agents.find((a) => a.id === activeToolType); setAgent(foundAgent || null); // Load models if agent supports model selection if (foundAgent?.capabilities?.supportsModelSelection) { setLoadingModels(true); window.maestro.agents .getModels(activeToolType) .then((models) => { if (!stale) setAvailableModels(models); }) .catch((err) => console.error('Failed to load models:', err)) .finally(() => { if (!stale) setLoadingModels(false); }); } else { setAvailableModels([]); } - }); + }).catch((err) => { + if (!stale) console.error('Failed to detect agents:', err); + }); // Load agent config for defaults, but use session-level overrides when available // Both model and contextWindow are now per-session window.maestro.agents.getConfig(activeToolType).then((globalConfig) => { if (stale) return; if (isProviderSwitch) { // When provider changed, use global defaults for the new provider setAgentConfig(globalConfig); } else { // Use session-level values if set, otherwise use global defaults const modelValue = session.customModel ?? globalConfig.model ?? ''; const contextWindowValue = session.customContextWindow ?? globalConfig.contextWindow; setAgentConfig({ ...globalConfig, model: modelValue, contextWindow: contextWindowValue }); } - }); + }).catch((err) => { + if (!stale) console.error('Failed to load agent config:', err); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx` around lines 94 - 129, Add .catch handlers to the window.maestro.agents.detect() and window.maestro.agents.getConfig(activeToolType) promise chains so failures are reported (e.g., via window.Sentry.captureException or your app logging) and not silently swallowed; for detect() also ensure you clean up UI state (e.g., setAvailableModels([]) and setLoadingModels(false) or setAgent(null)) on error, and for getConfig() report the error and re-throw or propagate it instead of swallowing so Sentry/upper layers can observe it; refer to detect(), getModels(), setAvailableModels, setLoadingModels, getConfig(), and setAgentConfig to place the catch logic and state cleanup.
🤖 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/renderer/components/NewInstanceModal/AgentPickerGrid.tsx`:
- Around line 305-307: The debug output currently splits debugInfo.envPath with
a hard-coded ':' which breaks Windows paths; update the split to be
platform-aware (use Node's path.delimiter or use debugInfo.platform === 'win32'
? ';' : ':' ) when mapping debugInfo.envPath in AgentPickerGrid so drive-letter
entries and Windows PATH entries are not mis-parsed; change the split call on
debugInfo.envPath (the mapping that also uses debugInfo.platform in the key) to
use the platform-aware delimiter.
---
Duplicate comments:
In `@src/renderer/components/NewInstanceModal/AgentPickerGrid.tsx`:
- Line 73: The div with role="listbox" in AgentPickerGrid.tsx lacks an
accessible name; add an explicit label by either adding aria-label="..." (e.g.,
aria-label="Agent picker") or aria-labelledby referencing a visible heading
element (create or reuse an element id and set aria-labelledby="that-id") on the
same container; ensure the label text is descriptive for screen readers and
update any nearby heading element if using aria-labelledby to match the chosen
label.
In `@src/renderer/components/NewInstanceModal/EditAgentModal.tsx`:
- Around line 94-129: Add .catch handlers to the window.maestro.agents.detect()
and window.maestro.agents.getConfig(activeToolType) promise chains so failures
are reported (e.g., via window.Sentry.captureException or your app logging) and
not silently swallowed; for detect() also ensure you clean up UI state (e.g.,
setAvailableModels([]) and setLoadingModels(false) or setAgent(null)) on error,
and for getConfig() report the error and re-throw or propagate it instead of
swallowing so Sentry/upper layers can observe it; refer to detect(),
getModels(), setAvailableModels, setLoadingModels, getConfig(), and
setAgentConfig to place the catch logic and state cleanup.
🪄 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: 44fce6b8-b4e4-4148-8a8d-f19cdd62792a
📒 Files selected for processing (2)
src/renderer/components/NewInstanceModal/AgentPickerGrid.tsxsrc/renderer/components/NewInstanceModal/EditAgentModal.tsx
- Use platform-aware PATH delimiter (';' on win32, ':' elsewhere) in
AgentPickerGrid debug display
- Add aria-label to role="listbox" container for screen reader accessibility
- Add .catch handlers to detect() and getConfig() promise chains in
EditAgentModal to prevent silent failures and clean up UI state on error
5 tasks
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
Decomposes two monolithic components —
MainPanel.tsx(2,000 lines) andNewInstanceModal.tsx(1,845 lines) — into clean directory modules following the established SOC/SRP patterns used by SettingsModal, SessionList, TabBar, FilePreview, and AutoRun decompositions.tsc --noEmitandeslintclean, zero regressionsWhat changed
MainPanel directory module (
src/renderer/components/MainPanel/)MainPanel.tsxMainPanelHeader.tsxMainPanelContent.tsxAgentErrorBanner.tsxCopyNotificationToast.tsxtypes.tsindex.tsMainPanel extracted hooks
useTerminalMountinghooks/terminal/useContextWindowhooks/mainPanel/useFilePreviewHandlershooks/mainPanel/useGitInfohooks/mainPanel/useSshRemoteNamehooks/mainPanel/useCopyToClipboardhooks/mainPanel/NewInstanceModal directory module (
src/renderer/components/NewInstanceModal/)NewInstanceModal.tsx_pending_SSH state machineEditAgentModal.tsxAgentPickerGrid.tsxNudgeMessageField.tsxRemotePathStatus.tsxtypes.tsindex.tsNewInstanceModal extracted hook
useRemotePathValidationhooks/agent/fs.stat, shared by both modalsSelf-sourcing pattern
MainPanelHeader and MainPanelContent self-source settings and UI state from Zustand stores (shortcuts, font settings, scroll behavior, etc.) instead of receiving them as props — following the Tier 1 component self-sourcing pattern established in earlier phases.
Test plan
npx tsc --noEmit— clean (0 errors)npm run lint— cleannpm run test— 24,309+ tests passSummary by CodeRabbit
New Features
Refactor
Tests
Bug Fixes
UI