[WIP] Symphony: Feature Request: multiple windows (#133)#1028
Conversation
📝 WalkthroughWalkthroughAdds full multi-window support across main, preload, and renderer with a WindowRegistry, IPC handlers/APIs, renderer WindowContext, state migration/restore, notifications metadata, quit/restore flows, telemetry, and comprehensive tests and docs updates. ChangesMulti-window Feature and Telemetry
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Renderer as Renderer (TabBar/WindowContext)
participant Preload as Preload (Windows API)
participant Main as Main (Windows Handlers)
participant Registry as WindowRegistry
participant Store as WindowState Store
User->>Renderer: Drag tab outside window
Renderer->>Preload: findWindowAtPoint(x,y)
Preload->>Main: windows:findWindowAtPoint
Main->>Registry: enumerate windows
Registry-->>Main: window entries
Main-->>Preload: target WindowInfo|null
Preload-->>Renderer: target WindowInfo|null
Renderer->>Preload: moveSession(sessionId, fromId, toId?) or create(bounds)
Preload->>Main: windows:moveSession / windows:create
Main->>Registry: move/create, update ownership
Main->>Store: upsert window state (bounds, activeSessionId)
Main-->>Renderer: windows:sessionMoved broadcast
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Symphony Contribution SummaryThis pull request was created using Maestro Symphony - connecting AI-powered contributors with open source projects. Contribution Stats
Powered by Maestro • Learn about Symphony |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/ipc/handlers/process.ts (1)
629-661:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove window-session assignment after a successful spawn.
At Line 629, window/session ownership is updated before
processManager.spawn(...). If spawn fails, the session is still persisted and broadcast as moved, which leaves stale UI/store ownership.Proposed fix
- assignSpawnSessionToSenderWindow(event, config.sessionId, windowManager, windowStateStore); - const result = processManager.spawn({ ...config, command: commandToSpawn, @@ sshStdinScript, }); + + if (result.success) { + assignSpawnSessionToSenderWindow( + event, + config.sessionId, + windowManager, + windowStateStore + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/handlers/process.ts` around lines 629 - 661, The code currently calls assignSpawnSessionToSenderWindow(event, config.sessionId, windowManager, windowStateStore) before invoking processManager.spawn(...), which can persist/broadcast a moved session even if spawn fails; move the call so it runs only after a successful spawn (i.e., after processManager.spawn resolves without error), and wrap the spawn call in a try/catch so failures do not trigger the window/session assignment or leave stale ownership; reference assignSpawnSessionToSenderWindow, processManager.spawn, config.sessionId, windowManager, and windowStateStore when making the change.src/renderer/components/TabBar.tsx (1)
1901-2041:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep file-tab drags out of the session-move path.
FileTabnow uses the same drag lifecycle as AI tabs, but the outside-window branch always acts onwindowContext?.activeSessionId. Dragging a file preview outside the window will therefore create/move the active session instead of the file tab the user actually grabbed. Carry the dragged item type through the drag payload, or skip the cross-window branch for file tabs.Also applies to: 2455-2465
🧹 Nitpick comments (1)
src/main/app-lifecycle/window-manager.ts (1)
376-383: 💤 Low valueAuto-reload after crash could cause reload loop if crash is deterministic.
If the renderer crashes due to a deterministic bug (e.g., bad initialization code), the 1-second delay auto-reload will create an infinite loop. Consider adding a crash counter with a max reload threshold (e.g., 3 crashes within 30 seconds).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/app-lifecycle/window-manager.ts` around lines 376 - 383, The auto-reload after renderer crash (inside the handler that checks details.reason and calls mainWindow.webContents.reload()) can cause a reload loop for deterministic crashes; add a crash tracking mechanism (e.g., a crash counter and timestamp list) scoped near the window manager globals that records each crash for mainWindow and only allows reload if crashes in the last 30 seconds are below a threshold (suggested max 3). Update the existing block that currently calls setTimeout(... mainWindow.webContents.reload()) to check the crash counter before scheduling a reload, increment and prune timestamps on each crash, and when the threshold is exceeded stop reloading and log/warn or show a recovery UI instead; reference symbols: mainWindow, details.reason, and the reload call (mainWindow.webContents.reload()) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/stats/multi-window-recorder.ts`:
- Around line 50-53: The catch block that currently logs and returns null in
multi-window-recorder.ts (the catch handling around the function that records
multi-window telemetry) must differentiate recoverable DB errors from unexpected
exceptions: import and use captureException/captureMessage from
src/utils/sentry.ts, keep returning null for known/recoverable DB errors
(identify via the DB error class/type checks used elsewhere), but for any
other/unexpected error call captureException with contextual data (include
LOG_CONTEXT and any relevant payload), then re-throw the error instead of
returning null so Sentry and upper layers can handle it; update the catch that
currently calls logger.warn(`Failed to record multi-window telemetry: ${error}`,
LOG_CONTEXT) to implement this behavior.
In `@src/renderer/components/SessionList/SessionList.tsx`:
- Around line 140-148: The current filter for visibleGroupChats wrongly excludes
chats that have an initiatorWindowId when no windowId is provided; update the
filter in the SessionList component so that if windowId is null/undefined it
does not filter out initiator-bound chats. Concretely, change the predicate used
to derive visibleGroupChats (the useMemo over groupChats) to allow all chats
when windowId is falsy, otherwise apply the existing check (e.g.
!chat.initiatorWindowId || chat.initiatorWindowId === windowId). Keep references
to visibleGroupChats, groupChats, windowId, and initiatorWindowId intact so
visibleActiveGroupChatId logic continues to work.
In `@src/renderer/components/TabBar.tsx`:
- Around line 1792-1805: The fire-and-forget IPC calls (e.g.,
window.maestro?.windows?.highlightDropZone in setHighlightedDropZone and
clearHighlightedDropZone, plus
highlightDropZone/findWindowAtPoint/getWindowBounds/create/moveSession usages
referenced) must not silently swallow rejections; update each call to properly
await the returned Promise (or attach a .catch), handle expected/recoverable
errors explicitly, and for unexpected failures call the Sentry helper
(captureException or captureMessage from src/utils/sentry.ts) with contextual
info (function name, windowId/point/session id) and then re-throw so Sentry can
capture it; ensure you reference the existing functions setHighlightedDropZone,
clearHighlightedDropZone, and any callbacks that call window.maestro to locate
and fix each site.
In `@src/renderer/contexts/WindowContext.tsx`:
- Around line 195-210: The moveSessionToNewWindow function uses the stale
closed-over sessionIds when computing the fallback active session; change the
logic so the next active session is computed from the up-to-date state inside
the setter callback instead of the outer sessionIds variable—specifically, after
creating newWindow, call setSessionIds with a functional updater that removes
sessionId and then compute getNextActiveSessionId using that currentSessionIds
result (or invoke setActiveSessionId from within that same updater by deriving
the new fallback) so setActiveSessionId never uses the outdated sessionIds
captured earlier; reference moveSessionToNewWindow, setSessionIds,
setActiveSessionId, and getNextActiveSessionId when making the change.
In `@src/renderer/hooks/agent/useAgentListeners.ts`:
- Around line 210-220: The helper is incorrectly returning true for any
sessionId starting with 'group-chat-', bypassing per-window scoping and causing
group-chat updates to reach all windows; update isSessionInCurrentWindow so it
does not special-case 'group-chat-*' — always derive the window-scoped id via
getWindowScopedSessionId(sessionId) and check membership against
windowSessionIdsRef.current (and still return true only if
windowSessionIdsRef.current or windowIdRef.current are null), so onAgentError
and useGroupChatStore only update the owning window. Reference:
isSessionInCurrentWindow, windowSessionIdsRef, windowIdRef,
getWindowScopedSessionId, onAgentError, useGroupChatStore.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 124-129: The current isGroupChatVisibleInWindow function hides
chats when windowId is null but chat.initiatorWindowId is set; change the logic
to treat a missing windowId as permissive: if windowId === null return true
(allow visibility), otherwise keep the existing behavior (return true when
chat.initiatorWindowId is falsy or equals windowId). Update the function
isGroupChatVisibleInWindow to check windowId first and then fall back to the
current initiatorWindowId equality check.
- Around line 417-426: The catch in the start-moderator block currently swallows
all errors; change it to import captureException and captureMessage from
src/utils/sentry.ts, then in the catch distinguish expected failures (e.g.,
inspect error.code or error.message for known conditions like deleted chat) and
handle those with a console.warn and early return, but for any other/unexpected
error call captureException/captureMessage with context about the group chat id
and re-throw the error so it surfaces; keep existing setGroupChats logic
unchanged and reference window.maestro.groupChat.startModerator and
setGroupChats when applying the changes.
---
Outside diff comments:
In `@src/main/ipc/handlers/process.ts`:
- Around line 629-661: The code currently calls
assignSpawnSessionToSenderWindow(event, config.sessionId, windowManager,
windowStateStore) before invoking processManager.spawn(...), which can
persist/broadcast a moved session even if spawn fails; move the call so it runs
only after a successful spawn (i.e., after processManager.spawn resolves without
error), and wrap the spawn call in a try/catch so failures do not trigger the
window/session assignment or leave stale ownership; reference
assignSpawnSessionToSenderWindow, processManager.spawn, config.sessionId,
windowManager, and windowStateStore when making the change.
---
Nitpick comments:
In `@src/main/app-lifecycle/window-manager.ts`:
- Around line 376-383: The auto-reload after renderer crash (inside the handler
that checks details.reason and calls mainWindow.webContents.reload()) can cause
a reload loop for deterministic crashes; add a crash tracking mechanism (e.g., a
crash counter and timestamp list) scoped near the window manager globals that
records each crash for mainWindow and only allows reload if crashes in the last
30 seconds are below a threshold (suggested max 3). Update the existing block
that currently calls setTimeout(... mainWindow.webContents.reload()) to check
the crash counter before scheduling a reload, increment and prune timestamps on
each crash, and when the threshold is exceeded stop reloading and log/warn or
show a recovery UI instead; reference symbols: mainWindow, details.reason, and
the reload call (mainWindow.webContents.reload()) when making the change.
🪄 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: be6cfcb0-5b5b-4fa4-bd04-08366f73288d
📒 Files selected for processing (97)
docs/context-management.mddocs/docs.jsondocs/features.mddocs/multi-window.mdsrc/__tests__/main/app-lifecycle/quit-handler.test.tssrc/__tests__/main/app-lifecycle/window-close-policy.test.tssrc/__tests__/main/app-lifecycle/window-manager.test.tssrc/__tests__/main/app-lifecycle/window-state-restore.test.tssrc/__tests__/main/group-chat/group-chat-storage.test.tssrc/__tests__/main/ipc/handlers/groupChat.test.tssrc/__tests__/main/ipc/handlers/notifications.test.tssrc/__tests__/main/ipc/handlers/process.test.tssrc/__tests__/main/ipc/handlers/stats.test.tssrc/__tests__/main/ipc/handlers/windows.test.tssrc/__tests__/main/preload/groupChat.test.tssrc/__tests__/main/preload/notifications.test.tssrc/__tests__/main/preload/windows.test.tssrc/__tests__/main/stats/stats-db.test.tssrc/__tests__/main/stores/defaults.test.tssrc/__tests__/main/stores/instances.test.tssrc/__tests__/main/stores/types.test.tssrc/__tests__/main/utils/safe-send.test.tssrc/__tests__/main/window-registry.test.tssrc/__tests__/renderer/components/QuickActionsModal.test.tsxsrc/__tests__/renderer/components/RightPanel.test.tsxsrc/__tests__/renderer/components/ShortcutsHelpModal.test.tsxsrc/__tests__/renderer/components/TabBar.test.tsxsrc/__tests__/renderer/contexts/WindowContext.test.tsxsrc/__tests__/renderer/hooks/ui/useWindowState.test.tsxsrc/__tests__/renderer/hooks/useAgentListeners.test.tssrc/__tests__/renderer/hooks/useCycleSession.test.tssrc/__tests__/renderer/hooks/useGroupChatHandlers.test.tssrc/__tests__/renderer/stores/notificationStore.test.tssrc/__tests__/renderer/utils/windowSessionOwnership.test.tssrc/__tests__/renderer/utils/windowSessionScope.test.tssrc/__tests__/shared/group-chat-types.test.tssrc/__tests__/shared/window-types.test.tssrc/__tests__/web/hooks/useWebSocket.test.tssrc/main/app-lifecycle/index.tssrc/main/app-lifecycle/quit-handler.tssrc/main/app-lifecycle/window-close-policy.tssrc/main/app-lifecycle/window-manager.tssrc/main/app-lifecycle/window-state-restore.tssrc/main/group-chat/group-chat-storage.tssrc/main/index.tssrc/main/ipc/handlers/groupChat.tssrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/notifications.tssrc/main/ipc/handlers/process.tssrc/main/ipc/handlers/windows.tssrc/main/preload/groupChat.tssrc/main/preload/index.tssrc/main/preload/notifications.tssrc/main/preload/stats.tssrc/main/preload/windows.tssrc/main/stats/aggregations.tssrc/main/stats/data-management.tssrc/main/stats/migrations.tssrc/main/stats/multi-window-recorder.tssrc/main/stats/multi-window.tssrc/main/stats/schema.tssrc/main/stats/stats-db.tssrc/main/stores/defaults.tssrc/main/stores/getters.tssrc/main/stores/instances.tssrc/main/stores/types.tssrc/main/utils/safe-send.tssrc/main/window-registry.tssrc/renderer/App.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/RightPanel.tsxsrc/renderer/components/SessionItem.tsxsrc/renderer/components/SessionList/SessionList.tsxsrc/renderer/components/SessionList/SkinnySidebar.tsxsrc/renderer/components/ShortcutsHelpModal.tsxsrc/renderer/components/TabBar.tsxsrc/renderer/components/UsageDashboard/UsageDashboardModal.tsxsrc/renderer/contexts/WindowContext.tsxsrc/renderer/global.d.tssrc/renderer/hooks/agent/useAgentListeners.tssrc/renderer/hooks/groupChat/useGroupChatHandlers.tssrc/renderer/hooks/props/useMainPanelProps.tssrc/renderer/hooks/props/useRightPanelProps.tssrc/renderer/hooks/session/useCycleSession.tssrc/renderer/hooks/stats/useStats.tssrc/renderer/hooks/ui/index.tssrc/renderer/hooks/ui/useWindowState.tssrc/renderer/stores/notificationStore.tssrc/renderer/utils/windowSessionOwnership.tssrc/renderer/utils/windowSessionScope.tssrc/shared/group-chat-types.tssrc/shared/index.tssrc/shared/stats-types.tssrc/shared/types/index.tssrc/shared/types/window.tssrc/web/hooks/useWebSocket.ts
| const visibleGroupChats = useMemo( | ||
| () => | ||
| groupChats.filter((chat) => !chat.initiatorWindowId || chat.initiatorWindowId === windowId), | ||
| [groupChats, windowId] | ||
| ); | ||
| const activeGroupChatId = useGroupChatStore((s) => s.activeGroupChatId); | ||
| const visibleActiveGroupChatId = visibleGroupChats.some((chat) => chat.id === activeGroupChatId) | ||
| ? activeGroupChatId | ||
| : null; |
There was a problem hiding this comment.
Preserve group chat visibility when window context is unavailable.
The current filter hides chats with an initiatorWindowId when no windowId is present, which breaks the optional-context fallback path.
Suggested fix
- const visibleGroupChats = useMemo(
- () =>
- groupChats.filter((chat) => !chat.initiatorWindowId || chat.initiatorWindowId === windowId),
- [groupChats, windowId]
- );
+ const visibleGroupChats = useMemo(
+ () =>
+ !windowId
+ ? groupChats
+ : groupChats.filter(
+ (chat) => !chat.initiatorWindowId || chat.initiatorWindowId === windowId
+ ),
+ [groupChats, windowId]
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const visibleGroupChats = useMemo( | |
| () => | |
| groupChats.filter((chat) => !chat.initiatorWindowId || chat.initiatorWindowId === windowId), | |
| [groupChats, windowId] | |
| ); | |
| const activeGroupChatId = useGroupChatStore((s) => s.activeGroupChatId); | |
| const visibleActiveGroupChatId = visibleGroupChats.some((chat) => chat.id === activeGroupChatId) | |
| ? activeGroupChatId | |
| : null; | |
| const visibleGroupChats = useMemo( | |
| () => | |
| !windowId | |
| ? groupChats | |
| : groupChats.filter( | |
| (chat) => !chat.initiatorWindowId || chat.initiatorWindowId === windowId | |
| ), | |
| [groupChats, windowId] | |
| ); | |
| const activeGroupChatId = useGroupChatStore((s) => s.activeGroupChatId); | |
| const visibleActiveGroupChatId = visibleGroupChats.some((chat) => chat.id === activeGroupChatId) | |
| ? activeGroupChatId | |
| : null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/components/SessionList/SessionList.tsx` around lines 140 - 148,
The current filter for visibleGroupChats wrongly excludes chats that have an
initiatorWindowId when no windowId is provided; update the filter in the
SessionList component so that if windowId is null/undefined it does not filter
out initiator-bound chats. Concretely, change the predicate used to derive
visibleGroupChats (the useMemo over groupChats) to allow all chats when windowId
is falsy, otherwise apply the existing check (e.g. !chat.initiatorWindowId ||
chat.initiatorWindowId === windowId). Keep references to visibleGroupChats,
groupChats, windowId, and initiatorWindowId intact so visibleActiveGroupChatId
logic continues to work.
| void window.maestro?.windows?.highlightDropZone?.(highlightedWindowId, false); | ||
| }, []); | ||
|
|
||
| const setHighlightedDropZone = useCallback( | ||
| (windowId: string | null) => { | ||
| if (highlightedWindowIdRef.current === windowId) { | ||
| return; | ||
| } | ||
|
|
||
| clearHighlightedDropZone(); | ||
| highlightedWindowIdRef.current = windowId; | ||
| if (windowId) { | ||
| void window.maestro?.windows?.highlightDropZone?.(windowId, true); | ||
| } |
There was a problem hiding this comment.
Handle rejected window IPC calls instead of fire-and-forget.
The new highlightDropZone(), findWindowAtPoint(), getWindowBounds(), create(), and moveSession() calls are all launched without any rejection handling. A failed IPC call here will silently leave drag state/highlights wrong and give Sentry nothing useful. As per coding guidelines src/renderer/**/*.{ts,tsx}: Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly (e.g., NETWORK_ERROR). For unexpected errors, re-throw them to allow Sentry to capture them. Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context.
Also applies to: 1859-1870, 1935-1943, 1986-2021
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/components/TabBar.tsx` around lines 1792 - 1805, The
fire-and-forget IPC calls (e.g., window.maestro?.windows?.highlightDropZone in
setHighlightedDropZone and clearHighlightedDropZone, plus
highlightDropZone/findWindowAtPoint/getWindowBounds/create/moveSession usages
referenced) must not silently swallow rejections; update each call to properly
await the returned Promise (or attach a .catch), handle expected/recoverable
errors explicitly, and for unexpected failures call the Sentry helper
(captureException or captureMessage from src/utils/sentry.ts) with contextual
info (function name, windowId/point/session id) and then re-throw so Sentry can
capture it; ensure you reference the existing functions setHighlightedDropZone,
clearHighlightedDropZone, and any callbacks that call window.maestro to locate
and fix each site.
| const moveSessionToNewWindow = useCallback( | ||
| async (sessionId: string): Promise<WindowInfo> => { | ||
| if (!windowId) { | ||
| throw new Error('Cannot move session before window state is initialized'); | ||
| } | ||
|
|
||
| const newWindow = await window.maestro.windows.create([sessionId]); | ||
| setSessionIds((currentSessionIds) => | ||
| currentSessionIds.filter((currentSessionId) => currentSessionId !== sessionId) | ||
| ); | ||
| setActiveSessionId((currentActiveSessionId) => | ||
| getNextActiveSessionId(sessionIds, sessionId, currentActiveSessionId) | ||
| ); | ||
| return newWindow; | ||
| }, | ||
| [sessionIds, windowId] |
There was a problem hiding this comment.
Compute the fallback active session from current state, not the closed-over array.
moveSessionToNewWindow() awaits IPC and then calls getNextActiveSessionId(sessionIds, ...) with the stale sessionIds captured before the move started. If another window move lands before this promise resolves, activeSessionId can be set to a session that's already gone from this window.
Suggested fix
const moveSessionToNewWindow = useCallback(
async (sessionId: string): Promise<WindowInfo> => {
if (!windowId) {
throw new Error('Cannot move session before window state is initialized');
}
const newWindow = await window.maestro.windows.create([sessionId]);
- setSessionIds((currentSessionIds) =>
- currentSessionIds.filter((currentSessionId) => currentSessionId !== sessionId)
- );
- setActiveSessionId((currentActiveSessionId) =>
- getNextActiveSessionId(sessionIds, sessionId, currentActiveSessionId)
- );
+ setSessionIds((currentSessionIds) => {
+ setActiveSessionId((currentActiveSessionId) =>
+ getNextActiveSessionId(currentSessionIds, sessionId, currentActiveSessionId)
+ );
+ return currentSessionIds.filter((currentSessionId) => currentSessionId !== sessionId);
+ });
return newWindow;
},
- [sessionIds, windowId]
+ [windowId]
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/contexts/WindowContext.tsx` around lines 195 - 210, The
moveSessionToNewWindow function uses the stale closed-over sessionIds when
computing the fallback active session; change the logic so the next active
session is computed from the up-to-date state inside the setter callback instead
of the outer sessionIds variable—specifically, after creating newWindow, call
setSessionIds with a functional updater that removes sessionId and then compute
getNextActiveSessionId using that currentSessionIds result (or invoke
setActiveSessionId from within that same updater by deriving the new fallback)
so setActiveSessionId never uses the outdated sessionIds captured earlier;
reference moveSessionToNewWindow, setSessionIds, setActiveSessionId, and
getNextActiveSessionId when making the change.
| const isSessionInCurrentWindow = (sessionId: string) => { | ||
| const scopedSessionIds = windowSessionIdsRef.current; | ||
| if (!scopedSessionIds || windowIdRef.current == null) { | ||
| return true; | ||
| } | ||
| if (sessionId.startsWith('group-chat-')) { | ||
| return true; | ||
| } | ||
|
|
||
| return scopedSessionIds.includes(getWindowScopedSessionId(sessionId)); | ||
| }; |
There was a problem hiding this comment.
Don't bypass window scoping for group-chat-* sessions.
This helper returns true for every group-chat session, so the group-chat branch in onAgentError will still update useGroupChatStore in every open window. That undoes the new per-window ownership model and will surface moderator/participant errors in the wrong renderer. Scope group-chat session IDs to their owning window here instead of blanket-allowing them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/agent/useAgentListeners.ts` around lines 210 - 220, The
helper is incorrectly returning true for any sessionId starting with
'group-chat-', bypassing per-window scoping and causing group-chat updates to
reach all windows; update isSessionInCurrentWindow so it does not special-case
'group-chat-*' — always derive the window-scoped id via
getWindowScopedSessionId(sessionId) and check membership against
windowSessionIdsRef.current (and still return true only if
windowSessionIdsRef.current or windowIdRef.current are null), so onAgentError
and useGroupChatStore only update the owning window. Reference:
isSessionInCurrentWindow, windowSessionIdsRef, windowIdRef,
getWindowScopedSessionId, onAgentError, useGroupChatStore.
| function isGroupChatVisibleInWindow( | ||
| chat: { initiatorWindowId?: string | null }, | ||
| windowId: string | null | ||
| ): boolean { | ||
| return !chat.initiatorWindowId || chat.initiatorWindowId === windowId; | ||
| } |
There was a problem hiding this comment.
Allow fallback visibility when no window context is available.
With windowId === null, this currently hides any chat that has initiatorWindowId, which can block opening existing chats in optional/non-window contexts. Consider treating missing windowId as permissive.
💡 Proposed fix
function isGroupChatVisibleInWindow(
chat: { initiatorWindowId?: string | null },
windowId: string | null
): boolean {
+ if (!windowId) return true;
return !chat.initiatorWindowId || chat.initiatorWindowId === windowId;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isGroupChatVisibleInWindow( | |
| chat: { initiatorWindowId?: string | null }, | |
| windowId: string | null | |
| ): boolean { | |
| return !chat.initiatorWindowId || chat.initiatorWindowId === windowId; | |
| } | |
| function isGroupChatVisibleInWindow( | |
| chat: { initiatorWindowId?: string | null }, | |
| windowId: string | null | |
| ): boolean { | |
| if (!windowId) return true; | |
| return !chat.initiatorWindowId || chat.initiatorWindowId === windowId; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts` around lines 124 - 129,
The current isGroupChatVisibleInWindow function hides chats when windowId is
null but chat.initiatorWindowId is set; change the logic to treat a missing
windowId as permissive: if windowId === null return true (allow visibility),
otherwise keep the existing behavior (return true when chat.initiatorWindowId is
falsy or equals windowId). Update the function isGroupChatVisibleInWindow to
check windowId first and then fall back to the current initiatorWindowId
equality check.
| try { | ||
| const moderatorSessionId = await window.maestro.groupChat.startModerator(id); | ||
| if (moderatorSessionId) { | ||
| setGroupChats((prev) => | ||
| prev.map((c) => (c.id === id ? { ...c, moderatorSessionId } : c)) | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to start moderator for group chat ${id}:`, error); | ||
| } |
There was a problem hiding this comment.
Don’t swallow unexpected moderator-start failures.
This catch handles every failure with console.warn and continues. Expected failures (e.g., deleted chat) should be handled explicitly, but unexpected ones should be reported and re-thrown.
💡 Proposed fix
+import { captureException } from '../../utils/sentry';
...
try {
const moderatorSessionId = await window.maestro.groupChat.startModerator(id);
if (moderatorSessionId) {
setGroupChats((prev) =>
prev.map((c) => (c.id === id ? { ...c, moderatorSessionId } : c))
);
}
} catch (error) {
- console.warn(`Failed to start moderator for group chat ${id}:`, error);
+ const message = error instanceof Error ? error.message : '';
+ const isRecoverableDeleteRace =
+ message.includes('deleted') || message.includes('not found');
+ if (isRecoverableDeleteRace) return;
+ captureException(error, {
+ tags: { feature: 'group-chat', operation: 'startModerator' },
+ extra: { groupChatId: id, windowId },
+ });
+ throw error;
}As per coding guidelines src/**/*.{ts,tsx}: Do not silently swallow errors... For unexpected errors, re-throw them... Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts` around lines 417 - 426,
The catch in the start-moderator block currently swallows all errors; change it
to import captureException and captureMessage from src/utils/sentry.ts, then in
the catch distinguish expected failures (e.g., inspect error.code or
error.message for known conditions like deleted chat) and handle those with a
console.warn and early return, but for any other/unexpected error call
captureException/captureMessage with context about the group chat id and
re-throw the error so it surfaces; keep existing setGroupChats logic unchanged
and reference window.maestro.groupChat.startModerator and setGroupChats when
applying the changes.
Greptile SummaryThis PR implements multiple-window support for Maestro, introducing a
Confidence Score: 3/5The multi-window wiring is broadly correct, but one shutdown-path error handler silently discards all exceptions without Sentry capture, directly contravening a project-wide convention that exists specifically to prevent blind spots in production telemetry. The window registry, state migration, session-movement serialisation, and display-position restore are well-designed. The main concern is that saveAllWindowStates in quit-handler.ts (and the empty catch in window-manager.ts saveWindowState wrapper) swallow every exception with only logger.warn, so any unexpected store or I/O error on shutdown disappears without a Sentry report. There is also a stale-closure edge case in moveSessionToNewWindow and a missing broadcast from windows:create, though those are lower-impact. src/main/app-lifecycle/quit-handler.ts (saveAllWindowStates error handling), src/main/app-lifecycle/window-manager.ts (saveWindowState catch block), src/main/ipc/handlers/windows.ts (windows:create broadcast gap) Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer (Window A)
participant P as Preload (windows API)
participant M as Main (windows IPC)
participant WR as WindowRegistry
participant WS as windowStateStore
participant RB as Renderer (Window B/new)
Note over R,RB: moveSessionToNewWindow(sessionId)
R->>P: windows.create([sessionId])
P->>M: IPC invoke windows:create
M->>WR: createSecondaryWindow(sessionIds, bounds)
WR->>WR: setSessionsForWindow(newId, sessionIds)
Note over WR: removes sessionId from Window A entry
M->>WS: upsertStoredWindowState(newWindowId)
M->>WS: upsertStoredWindowState(sourceWindowId)
M-->>R: return WindowInfo
R->>R: setSessionIds / setActiveSessionId
Note over RB: New window loads
RB->>M: windows:getState
M->>WR: get(windowId).sessionIds
M-->>RB: "WindowState {sessionIds:[sessionId]}"
RB->>RB: WindowContext initialises
Note over R,RB: windows:moveSession (drag-drop)
R->>M: windows:moveSession (queued)
M->>WR: moveSession(id, from, to)
M->>WS: upsertStoredWindowState x2
M->>R: windows:sessionMoved
M->>RB: windows:sessionMoved
Note over R,RB: Secondary window closes
RB->>M: close event
M->>WR: moveSessionsToPrimary
M->>R: windows:sessionsMovedToPrimary
R->>R: merge sessions + toast
Reviews (1): Last reviewed commit: "MAESTRO: Document multi-window support" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| function createWindowState( | ||
| windowId: string, | ||
| entry: WindowRegistryEntry, | ||
| currentState: MultiWindowState | ||
| ): WindowState { |
There was a problem hiding this comment.
The catch block logs via logger.warn but never reports to Sentry, violating the project's error-handling convention from CLAUDE.md. Any unexpected error here (e.g., a corrupt store, disk I/O error unrelated to ENFILE/ENOSPC) is silently discarded and will never surface in production telemetry. Per the project guideline, unknown errors should be captured: call captureException(err, { operation: 'saveWindowStates' }) before returning rather than swallowing silently. This same pattern recurs in the saveWindowState helper inside window-manager.ts (the empty catch {} block at line ~234).
Context Used: CLAUDE.md (source)
| windowStateStore, | ||
| sourceWindowId, | ||
| sourceWindow, | ||
| sourceEntry.sessionIds, | ||
| { | ||
| activeSessionId: sourceMovedActiveSession | ||
| ? getNextActiveSessionId(sourceSessionIds, sourceActiveSessionId) | ||
| : (sourceActiveSessionId ?? null), | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| recordMultiWindowUsage(settingsStore, windowRegistry, 'window_created'); | ||
|
|
||
| return toWindowInfo(windowId, windowManager, activeSessionId); | ||
| } | ||
| ); | ||
|
|
||
| ipcMain.handle('windows:close', async (_event, windowId: string): Promise<WindowCloseResult> => { | ||
| const entry = windowRegistry.get(windowId); | ||
| if (!entry) { | ||
| return { closed: false, reason: 'not-found' }; | ||
| } | ||
| if (entry.isMain) { | ||
| return { closed: false, reason: 'primary-window' }; | ||
| } | ||
|
|
||
| entry.browserWindow.close(); | ||
| return { closed: true }; | ||
| }); | ||
|
|
||
| ipcMain.handle('windows:list', async (): Promise<WindowInfo[]> => { | ||
| return getWindowList(windowManager, windowStateStore); | ||
| }); | ||
|
|
||
| ipcMain.handle( |
There was a problem hiding this comment.
windows:create does not broadcast session-list changes to other open windows
windows:moveSession calls broadcastSessionMoved so every window's onSessionMoved listener fires. windows:create skips this broadcast entirely: the source window's renderer updates its own state manually (via moveSessionToNewWindow), but any other open window that has subscribed to onSessionMoved will not learn that the source window's session list changed. In a 3+-window setup this leaves the windows' cached session lists inconsistent until the next explicit windows:list call. Consider calling broadcastSessionMoved for each session moved out of the source window, mirroring what windows:moveSession does.
| const moveSessionToNewWindow = useCallback( | ||
| async (sessionId: string): Promise<WindowInfo> => { | ||
| if (!windowId) { | ||
| throw new Error('Cannot move session before window state is initialized'); | ||
| } | ||
|
|
||
| const newWindow = await window.maestro.windows.create([sessionId]); | ||
| setSessionIds((currentSessionIds) => | ||
| currentSessionIds.filter((currentSessionId) => currentSessionId !== sessionId) | ||
| ); | ||
| setActiveSessionId((currentActiveSessionId) => | ||
| getNextActiveSessionId(sessionIds, sessionId, currentActiveSessionId) | ||
| ); | ||
| return newWindow; | ||
| }, | ||
| [sessionIds, windowId] | ||
| ); |
There was a problem hiding this comment.
sessionIds closure in active-session calculation after async call
getNextActiveSessionId(sessionIds, sessionId, ...) captures sessionIds from the closure, not the live state at the point the await window.maestro.windows.create resolves. If an onSessionMoved event arrives while waiting for the IPC round-trip, the index look-up in getNextActiveSessionId will use the outdated list and may pick the wrong replacement active session. Reading the live list inside the setSessionIds functional updater (and calling setActiveSessionId there) would ensure both updates share the same consistent snapshot.
| continue; | ||
| } | ||
| entry.sessionIds = entry.sessionIds.filter((id) => id !== sessionId); | ||
| } | ||
|
|
||
| if (!toWindow.sessionIds.includes(sessionId)) { | ||
| toWindow.sessionIds.push(sessionId); | ||
| } | ||
| } | ||
|
|
||
| saveWindowState(windowId: string): WindowState | null { | ||
| if (!this.windowStateStore) { | ||
| return null; | ||
| } | ||
|
|
||
| const entry = this.windows.get(windowId); |
There was a problem hiding this comment.
x/y/width/height for the non-maximised path
nextWindowState is first populated with existingWindowState?.x ?? bounds.x (etc.), but for every non-maximised/non-fullscreen window the block below immediately overwrites those same fields with bounds.x (etc.). The existingWindowState?.x fallback only matters when the window IS maximised, so the initial assignment for the non-maximised case is dead code. Consider initialising directly from bounds.* and only falling back to existingWindowState.* inside the maximised branch, which makes the intent explicit.
|
@DRAZY thanks for the contribution — this is a substantial multi-window feature with thorough tests and docs. Greptile and CodeRabbit have both posted reviews; flagging the highest-impact items below so they can land before this comes out of WIP: Correctness
Error handling (per CLAUDE.md)
Reliability
Minor
No merge conflicts on |
2 participants
Maestro Symphony Contribution
Closes #133
Contributed via Maestro Symphony.
Status: In Progress
Started: 2026-05-20T21:34:53.665Z
This PR will be updated automatically when the Auto Run completes.
Summary by CodeRabbit
Release Notes
New Features
Documentation