Open
Conversation
Collaborator
|
NICE! |
Wire per-window usage tracking in stats DB with migration, add window-scoped notification routing, update test setup and test suites across IPC handlers, preload bindings, renderer components, and hooks to cover multi-window context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove unused React namespace imports (HistoryEntryItem, HistoryFilterToggle, HistoryDetailModal, WindowContext) - Remove unused imports: selectActiveSession, GroupChatMessage, estimateContextUsage, estimateAccumulatedGrowth, DEFAULT_CONTEXT_WINDOWS, WindowInfo - Remove unused variable isCurrentSessionStopping from MainPanel - Fix incomplete mock Session aiTab in windowSessionOrdering test - Remove stale UseHandsOnTimeTrackerReturn type export (hook returns void) - Add .ai-audit/ to .gitignore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openasocket
force-pushed
the
symphony/issue-133-mlg7l02p
branch
from
86423c6 to
ee122f3
Compare
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds end-to-end multi-window support: WindowRegistry/WindowManager, persisted multi-window state and migrations, renderer WindowContext and hooks, preload/windows API and IPC handlers, notification/window-focused behavior, window-usage analytics, group-chat initiatorWindowId, many tests/docs, and Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (UI)
participant Preload as Preload / IPC
participant Registry as WindowRegistry (Main)
participant Store as WindowStateStore
participant OtherR as Other Renderer
Renderer->>Preload: windows.moveSession(sessionId, toWindowId)
Preload->>Registry: request moveSession(sessionId, toWindowId)
Registry->>Registry: update in-memory mapping
Registry->>Store: set('multiWindowState', updatedState)
Registry->>Preload: emit 'windows:sessionMoved'(sessionId, fromWindowId, toWindowId)
Preload->>Renderer: deliver 'windows:sessionMoved'
Preload->>OtherR: deliver 'windows:sessionMoved'
sequenceDiagram
participant App as App Startup
participant Store as WindowStateStore
participant Registry as WindowRegistry
participant Manager as WindowManager
App->>Store: get('multiWindowState')
Store-->>App: persisted multiWindowState
App->>Registry: initialize registry from persisted state
App->>Manager: restore windows using registry
Manager->>Registry: register created BrowserWindow(s)
Registry->>Store: persist synced snapshots
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/renderer/components/TabBar.tsx (1)
598-618:⚠️ Potential issue | 🟠 MajorMake interactive tab containers keyboard-focusable.
The clickable/draggable tab containers at Line 598 and Line 1422 are not keyboard-focusable and have no focus handlers.
♿ Proposed fix
<div ref={setTabRef} data-tab-id={tab.id} + tabIndex={0} + onFocus={handleMouseEnter} + onBlur={handleMouseLeave} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleTabSelect(); + } + }} className={`<div ref={setTabRef} data-tab-id={tab.id} + tabIndex={0} + onFocus={handleMouseEnter} + onBlur={handleMouseLeave} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleTabSelect(); + } + }} className={`As per coding guidelines "Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus".
Also applies to: 1422-1442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TabBar.tsx` around lines 598 - 618, The tab container JSX that uses setTabRef and handlers like handleTabSelect, handleMouseDown, handleMouseEnter/Leave and drag handlers must be made keyboard-focusable: add tabIndex={0} to the root div and wire focus handlers (onFocus and onBlur) to the existing focus-related helpers (or implement new handlers such as handleTabFocus and handleTabBlur alongside setTabRef) and ensure keyboard activation is supported via onKeyDown (e.g., call handleTabSelect when Enter/Space is pressed). Apply the same changes for the other tab instance (the block around lines 1422–1442) so both draggable tabs are accessible via keyboard and have focus state handling.src/main/preload/groupChat.ts (1)
15-19:⚠️ Potential issue | 🔴 CriticalInclude
customModelin the preloadModeratorConfiginterface.The renderer's
NewGroupChatModalcomponent constructs amoderatorConfigwithcustomModel(line 175 of the component), but the preload interface excludes this field. This causescustomModelto be silently dropped at the IPC boundary, even though the main process expects and uses it for agent execution (seesrc/main/group-chat/group-chat-router.tslines 467, 503, 532, etc.).Update the preload interface to match:
export interface ModeratorConfig { customPath?: string; customArgs?: string; customEnvVars?: Record<string, string>; customModel?: string; }Note:
sshRemoteConfigis also missing from the preload type but is not currently constructed by the renderer, so it may be intentionally excluded for security reasons. If remote SSH execution will be exposed through the group chat API in future, addsshRemoteConfigas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/groupChat.ts` around lines 15 - 19, The preload ModeratorConfig interface is missing the customModel property so moderatorConfig built by NewGroupChatModal gets dropped at the IPC boundary; update the export interface ModeratorConfig to include customModel?: string (i.e., add the optional customModel field alongside customPath, customArgs, customEnvVars) so the value is preserved for the main process handlers in group-chat-router (where customModel is consumed).src/main/index.ts (1)
839-839:⚠️ Potential issue | 🟠 MajorPass
windowRegistryinto notification handler registration.At Line 839,
registerNotificationsHandlers()is called without dependencies, so metadata-based focus resolution innotifications.tscannot useWindowRegistryand falls back to source-window behavior.Based on learnings: Verify IPC handlers are registered in src/main/index.ts before modifying caller code when debugging IPC issues.Suggested fix
- registerNotificationsHandlers(); + registerNotificationsHandlers({ + getWindowRegistry: () => windowRegistry, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` at line 839, registerNotificationsHandlers() is being invoked without the required WindowRegistry, causing metadata-based focus resolution in notifications.ts to fall back to source-window behavior; update the call in src/main/index.ts to pass the existing WindowRegistry instance (e.g., windowRegistry) into registerNotificationsHandlers and update the registerNotificationsHandlers function signature in notifications.ts to accept and store/use that WindowRegistry for focus resolution, ensuring IPC handlers remain registered in the correct order before making this change.src/renderer/App.tsx (1)
8371-8374:⚠️ Potential issue | 🟠 MajorWizard-created sessions are not assigned to the current window.
This callback now tracks
assignSessionsToWindowin deps, but the newly created wizard session is never actually assigned, which can break per-window ownership/routing.🔧 Proposed fix
// Add session and make it active setSessions((prev) => [...prev, newSession]); setActiveSessionId(newId); + await assignSessionsToWindow([newId]); updateGlobalStats({ totalSessions: 1 });Also applies to: 8447-8447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 8371 - 8374, The new wizard-created session is added via setSessions and activated with setActiveSessionId but never assigned to the current window, so update the callback that references assignSessionsToWindow to call it for the newly created session ID (newId) after adding the session; specifically, after setSessions/setActiveSessionId/updateGlobalStats, invoke assignSessionsToWindow([newId]) (or the appropriate signature) so the session is bound to the window—apply the same change at the other occurrence around the code that uses setSessions/setActiveSessionId/updateGlobalStats (the second block referenced).
🧹 Nitpick comments (12)
src/main/process-listeners/data-listener.ts (1)
109-109: Avoid all-window fan-out forprocess:dataif session ownership is known.
process:datais a hot path; broadcasting every chunk to every renderer scales as O(windows × output). Prefer routing to the owning window (or a narrowed subscriber set) to reduce IPC volume and renderer work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-listeners/data-listener.ts` at line 109, The current hot-path call broadcastToAllWindows('process:data', sessionId, data) fans out every chunk to all renderers; instead, resolve the owning renderer for sessionId (e.g., via a sessionManager/getWindowForSession or similar ownership lookup) and send the IPC only to that window (use the per-window send method rather than broadcast); if ownership is unknown or multiple subscribers exist, fall back to a narrowed subscriber list or to broadcast as last resort — update the logic around broadcastToAllWindows to prefer sendToWindow/getWindowForSession(sessionId) with a safe fallback.src/main/stats/schema.ts (1)
133-139: Add non-negative CHECK constraints for counter columns.
window_countandsession_countshould reject invalid negative values at the DB layer to protect analytics integrity.Suggested schema hardening
export const CREATE_WINDOW_USAGE_EVENTS_SQL = ` CREATE TABLE IF NOT EXISTS window_usage_events ( id TEXT PRIMARY KEY, recorded_at INTEGER NOT NULL, - window_count INTEGER NOT NULL, - session_count INTEGER NOT NULL, + window_count INTEGER NOT NULL CHECK(window_count >= 0), + session_count INTEGER NOT NULL CHECK(session_count >= 0), is_multi_window INTEGER NOT NULL CHECK(is_multi_window IN (0, 1)) ) `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/stats/schema.ts` around lines 133 - 139, The window_usage_events table definition must enforce non-negative counters: modify the CREATE TABLE for window_usage_events to add CHECK constraints (or named CONSTRAINTs) on the window_count and session_count columns so they require value >= 0; reference the table name window_usage_events and the column names window_count and session_count when making the change and ensure the updated schema is used wherever the table is created/migrated in schema.ts.src/main/process-listeners/forwarding-listeners.ts (1)
15-44: Consider session-targeted dispatch for high-frequency forwarded events.This migration is correct functionally, but broadcasting chunks/errors/tools to every window can create avoidable renderer churn as window count grows. If you already have session→window ownership available, routing these channels to the owning window would scale better.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-listeners/forwarding-listeners.ts` around lines 15 - 44, The current listeners (processManager.on handlers for 'thinking-chunk', 'stderr', 'tool-execution', 'slash-commands', 'command-exit') call broadcastToAllWindows which causes unnecessary renderer churn; instead route events to the specific owning window for the session by looking up the session→window mapping and sending only to that window. Modify the function signature to accept or use an existing session-to-window helper (e.g. getWindowForSession or broadcastToWindow) and replace broadcastToAllWindows('process:...', sessionId, ...) in the handlers with a targeted call that resolves the window for sessionId and sends the same payload only to that window (fall back to broadcastToAllWindows if no owner found). Ensure you update all handlers listed above to use the targeted dispatch.src/__tests__/main/preload/notifications.test.ts (1)
39-44: Test correctly updated for new signature.The test now expects
undefinedas the third argument when metadata is not provided, which validates backward compatibility.Consider adding a test case that verifies the metadata is correctly forwarded when provided:
it('should invoke notification:show with title, body, and metadata', async () => { mockInvoke.mockResolvedValue({ success: true }); const metadata = { sessionId: 'session-1', windowId: 'window-1' }; await api.show('Test Title', 'Test Body', metadata); expect(mockInvoke).toHaveBeenCalledWith( 'notification:show', 'Test Title', 'Test Body', metadata ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/preload/notifications.test.ts` around lines 39 - 44, Add a new unit test that verifies metadata is forwarded by calling api.show with a metadata object and asserting mockInvoke was called with 'notification:show', the title, the body, and the same metadata; specifically, in src/__tests__/main/preload/notifications.test.ts add an it block that sets mockInvoke.mockResolvedValue({ success: true }), constructs a metadata object (e.g., { sessionId: 'session-1', windowId: 'window-1' }), calls await api.show('Test Title', 'Test Body', metadata), and expects mockInvoke.toHaveBeenCalledWith('notification:show', 'Test Title', 'Test Body', metadata) so the test covers the new signature path in api.show and mockInvoke usage.src/__tests__/renderer/components/QuickActionsModal.test.tsx (1)
814-816: Indentation inconsistency.Lines 814-816 appear to have lost their indentation, breaking the visual structure of the test block.
🔧 Proposed fix
- expect(buttons[0]).toHaveStyle({ backgroundColor: mockTheme.colors.accent }); - }); - }); + expect(buttons[0]).toHaveStyle({ backgroundColor: mockTheme.colors.accent }); + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/QuickActionsModal.test.tsx` around lines 814 - 816, The final three lines containing expect(buttons[0]).toHaveStyle({ backgroundColor: mockTheme.colors.accent }); }); }); have lost their indentation; restore their indentation to match the enclosing test block so the assertion and the closing braces line up with the surrounding describe/it structure (ensure the expect line is indented inside the it block and the two closing "});" align with their corresponding it/describe openings referencing the QuickActionsModal test and the buttons/mocKTheme usage).src/main/app-lifecycle/quit-handler.ts (1)
328-331: Consider simplifying the fallback chain.The pattern
windowStateStore.get('multiWindowState') ?? windowStateStore.store.multiWindowStateappears redundant sinceget()should return the stored value. However, if this handles an edge case with electron-store's behavior, consider adding a brief comment explaining why both accesses are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/app-lifecycle/quit-handler.ts` around lines 328 - 331, The fallback chain assigning persisted is redundant: replace the dual access with a single reliable read using windowStateStore.get('multiWindowState') and fall back to fallback (i.e., remove windowStateStore.store.multiWindowState), or if you intentionally need the direct store access for an electron-store edge case, keep both but add a brief comment above the line explaining the edge case and why windowStateStore.store.multiWindowState is required; refer to windowStateStore.get, windowStateStore.store.multiWindowState, and the persisted variable when making the change.src/renderer/components/MainPanel.tsx (2)
1245-1249: ReusehandleStopWindowAutoRunfor the center AUTO button.The click handler duplicates stop-target resolution logic that already exists in
handleStopWindowAutoRun.♻️ Proposed change
- <button - onClick={() => { - if (isWindowAutoModeStopping) return; - const targetSessionId = windowAutoRunSessionId ?? activeSession.id; - onStopBatchRun?.(targetSessionId); - }} + <button + onClick={() => { + if (isWindowAutoModeStopping) return; + handleStopWindowAutoRun(); + }}Also applies to: 761-774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MainPanel.tsx` around lines 1245 - 1249, The click handler duplicates stop-target resolution—replace the inline logic with a call to the existing handleStopWindowAutoRun function: remove the anonymous onClick handler that checks isWindowAutoModeStopping, computes targetSessionId from windowAutoRunSessionId or activeSession.id, and calls onStopBatchRun; instead invoke handleStopWindowAutoRun so the single implementation handles stopping (ensure handleStopWindowAutoRun is in scope for MainPanel and wired to the same onStopBatchRun behavior and state such as isWindowAutoModeStopping and windowAutoRunSessionId).
1884-1885: GateonStopAutoRunby active/stopping state, not object presence.Using a truthy
windowAutoRunStatecheck is looser than necessary if state objects can persist briefly after completion.♻️ Proposed change
- onStopAutoRun={windowAutoRunState ? handleStopWindowAutoRun : undefined} + onStopAutoRun={ + isWindowAutoModeActive || isWindowAutoModeStopping + ? handleStopWindowAutoRun + : undefined + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MainPanel.tsx` around lines 1884 - 1885, The current conditional passes onStopAutoRun whenever windowAutoRunState is truthy; instead gate it on the run being active or stopping. Change the prop expression so onStopAutoRun is set only when windowAutoRunState indicates an active/stopping state (e.g. windowAutoRunState?.status === 'active' || windowAutoRunState?.status === 'stopping') while still passing autoRunState={windowAutoRunState || undefined}; update the JSX where windowAutoRunState and handleStopWindowAutoRun are used to enforce this state-based check.src/main/window-registry.ts (1)
457-462: Shallow clone may be insufficient ifPersistedWindowStategains nested objects.Currently
cloneWindowStatespreads the top level and clonessessionIds. IfPersistedWindowStatelater includes nested objects beyond arrays, this would need updating. For now this is adequate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/window-registry.ts` around lines 457 - 462, cloneWindowState currently does a shallow spread plus copying sessionIds, which will miss nested objects if PersistedWindowState later includes them; update the implementation of cloneWindowState to perform a true deep clone of the entire PersistedWindowState (e.g., use structuredClone(windowState) where available, with a safe fallback like JSON.parse(JSON.stringify(windowState)) or lodash.cloneDeep) so all nested objects and arrays are duplicated rather than referenced, keeping the function name cloneWindowState and return type PersistedWindowState unchanged.src/main/app-lifecycle/window-manager.ts (1)
587-587: Minor: Extra blank line.There's an extra blank line at 587, though this is a very minor formatting nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/app-lifecycle/window-manager.ts` at line 587, Remove the extraneous blank line inside the WindowManager class in window-manager.ts (the blank within the method/block around where the reviewer noted line 587); edit the WindowManager source to delete that single empty line so formatting matches the surrounding code and linter/style checks pass.src/main/ipc/handlers/windows.ts (1)
554-576: OnlycloneWindowStateis truly duplicated; the other functions have context-specific differences.
cloneWindowStateis identical in both files and could be extracted to a shared utility. However,getMultiWindowStateSnapshotandcreateDefaultWindowStateSnapshothave different implementations:
getMultiWindowStateSnapshotuses different fallback logic and has different return type annotationscreateDefaultWindowStateSnapshotdiffers in null-safety ([0]vs?.[0]), x/y coordinate handling, andwindow-registry.tsincludes an additionaldisplayIdfieldBefore extracting, clarify whether these differences are intentional for their respective contexts or represent a genuine maintenance issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/windows.ts` around lines 554 - 576, cloneWindowState is duplicated and should be extracted to a shared utility to avoid duplication; move the implementation of cloneWindowState into a common module (e.g., a new util file), export it, update call sites to import the shared cloneWindowState, and ensure its signature and shallow-copy behavior (spreading sessionIds into a new array) remain identical. Do not change or consolidate getMultiWindowStateSnapshot or createDefaultWindowStateSnapshot here — keep their context-specific fallbacks, null-safety (?.[0] vs [0]), x/y handling, return types and any extra fields like displayId intact; when updating imports, run type checks to ensure no type mismatch at call sites.src/main/ipc/handlers/index.ts (1)
294-303: Consider broadcasting logger events to all windows for multi-window DevTools visibility.The logger event forwarding currently sends to the primary window only. If logs should be visible in DevTools across all application windows, consider implementing a broadcast pattern similar to
marketplace:manifestChangedornotification:commandCompleted. However, this would require passing additional dependencies (e.g.,getWindowRegistryor abroadcastToAllWindowsutility) tosetupLoggerEventForwarding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/index.ts` around lines 294 - 303, The logger forwarding currently targets only getMainWindow; modify setupLoggerEventForwarding to accept a window-broadcast capability (e.g., getWindowRegistry or a broadcastToAllWindows utility) and change its implementation to broadcast logger events to every window returned by that registry (similar to how marketplace:manifestChanged or notification:commandCompleted broadcast). Update the call site in this file to pass deps.getWindowRegistry (or the broadcast helper) instead of just deps.getMainWindow, and ensure any related registration logic in registerWindowsHandlers remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/general-usage.md`:
- Around line 331-342: The "Multi-Window Workflow" heading is currently at level
"##" and incorrectly elevates it out of the "Tab Management" section; change the
heading "Multi-Window Workflow" to a lower level (e.g., "### Multi-Window
Workflow") so it remains nested under the existing "Tab Management" heading and
adjust any sibling headings in this block if needed to preserve correct
hierarchy for subsequent topics like "Opening a New Window" and "Tracking Where
Tabs Live".
In `@src/main/app-lifecycle/window-manager.ts`:
- Around line 353-364: initAutoUpdater(browserWindow) is being invoked for every
new window in production which causes the auto-updater's single stored
mainWindow to be overwritten; guard initialization so it runs only once (or only
for the designated main window). Update the window creation path to check a
module/app-level flag (e.g., autoUpdaterInitialized) or only call
initAutoUpdater from the main window creation logic, set that flag to true after
calling initAutoUpdater(browserWindow), and keep registerDevAutoUpdaterStubs()
behavior in development; ensure the stored mainWindow inside the auto-updater is
only set on the first initialization to avoid later windows overwriting it.
In `@src/main/index.ts`:
- Around line 601-604: The current window-all-closed handler uses
windowRegistry?.getPrimary() and quits when process.platform !== 'darwin' ||
!primaryWindow, which causes the app to quit on macOS; change the handler to
only call app.quit() when process.platform !== 'darwin' (remove the
!primaryWindow check and any dependence on windowRegistry.getPrimary()), so
macOS keeps the app running and follows the normal activate lifecycle; update
the app.on('window-all-closed', ...) callback accordingly.
- Around line 329-333: The current close handler only nulls the global
mainWindow when the closed BrowserWindow equals browserWindow, which leaves
mainWindow null even if other windows exist; update the handler attached to
browserWindow (the browserWindow.on('closed', ...) block) to: when a window
closes, if mainWindow === browserWindow then attempt to reassign mainWindow to
another open window returned by BrowserWindow.getAllWindows() (pick the first
remaining window) and only set mainWindow = null if no other windows exist;
ensure you reference the mainWindow variable and BrowserWindow.getAllWindows()
in the implementation.
In `@src/main/ipc/handlers/windows.ts`:
- Around line 245-251: The broadcast loop uses targetWindowId as fromWindowId
when a session was previously unassigned, producing misleading events; update
the loop in the assignments handling (the for ... of assignments block that
calls broadcastSessionMoved) to detect when fromWindowId is null/undefined and
either skip calling broadcastSessionMoved for newly assigned sessions or pass a
clear sentinel (e.g., null/undefined or a dedicated string) as fromWindowId
instead of targetWindowId; ensure references to sessionId, fromWindowId,
targetWindowId and windowRegistry remain intact and adjust any downstream
consumers to handle the sentinel if you choose that route.
In `@src/main/process-listeners/__tests__/data-listener.test.ts`:
- Line 12: Remove the extra leading tab before the describe block for the test
suite "describe('Data Listener', () => {" so the line starts at column zero;
locate the describe('Data Listener', () => { statement in the test file (the
Data Listener test) and delete the leading whitespace character to restore
consistent file formatting.
In `@src/main/window-registry.ts`:
- Around line 165-179: In remove(windowId: string) you set primaryWindowId to
null when removing the primary but never promote another open window; update
remove to, after deleting the window (this.windows.delete(windowId)), check if
this.primaryWindowId === windowId and if so reassign this.primaryWindowId to the
first remaining key from this.windows (e.g. Array.from(this.windows.keys())[0]
or this.windows.keys().next().value) or null if none remain; keep using the same
null sentinel and ensure emitWindowCountMetrics() still runs as before.
In `@src/renderer/App.tsx`:
- Around line 589-591: The current logic sets windowSessionIdSet to null when
windowSessionIds is empty, which causes filtering to be disabled (fails open);
change the useMemo so it always returns a Set (new Set(windowSessionIds)) even
for an empty array so an empty assignment becomes an explicit empty scope that
filters out all sessions; update the same pattern wherever used (the other
occurrences referenced: the block around active-session resolution and the
thinkingSessions computation) to stop treating null as "no scope" and instead
treat an empty Set as an explicit empty scope.
- Around line 8202-8203: The code calls assignSessionsToWindow([newId]) twice in
succession causing duplicate side-effects; remove the redundant call so
assignSessionsToWindow is invoked only once after creating newId (locate the
duplicate calls around where newId is created/assigned in App.tsx and keep a
single await assignSessionsToWindow([newId]) invocation).
In `@src/renderer/components/QuickActionsModal.tsx`:
- Around line 227-228: The current logic calls setActiveSessionId based on
sessionWindowAssignments which is populated asynchronously and can momentarily
activate a session that belongs to another window; change the guard to first
check membership in windowSessionIds (from useWindowContext) before calling
setActiveSessionId, and only fall back to sessionWindowAssignments once it is
loaded. Locate usages in QuickActionsModal where setActiveSessionId is invoked
(references: useWindowContext, windowSessionIds, useSessionWindowAssignments,
setActiveSessionId, sessionWindowAssignments) and update each to: if
(!windowSessionIds.includes(targetSessionId)) return; then proceed to
setActiveSessionId (or consult sessionWindowAssignments if needed). Apply the
same change for the other occurrences mentioned around the 231-246 and 340-343
sections.
In `@src/renderer/components/SessionList.tsx`:
- Around line 1552-1559: The click handler currently calls
window.maestro.windows.focusWindow(assignment.windowId) and returns early on any
failure, so when focusWindow rejects the user click becomes a no-op; change this
by handling focusWindow's promise so that on rejection you log the error but
continue with the normal selection flow (i.e., do not return early). Locate the
block referencing currentWindowId, sessionWindowAssignments.get(sessionId),
assignment.windowId and window.maestro.windows.focusWindow and modify it so the
promise rejection is caught and the handler proceeds to the same selection logic
as the success path (or calls the session selection function), ensuring you
avoid duplicate selection side-effects if focus succeeds. Ensure the catch still
logs the error detail (error) for debugging.
In `@src/renderer/components/ShortcutsHelpModal.tsx`:
- Around line 16-19: The user-facing shortcut help text in
ShortcutsHelpModal.tsx uses "session/session tab" which violates the repo
guideline to use "agent" in UI copy; update the string values in
SCOPE_DESCRIPTIONS (and any other user-facing strings in this file around the
other shortcut/help text blocks referenced) to replace "session" or "session
tab" with "agent" or "agent tab" while leaving internal types like
ShortcutScope/Session interfaces unchanged so only the displayed copy changes.
In `@src/renderer/components/TabBar.tsx`:
- Around line 1714-1723: The early return using sessionAllowed is placed before
hooks (e.g., useState calls for draggingTabId, dragOverTabId,
isDraggingOutsideWindow, isDropZoneHighlightActive) which breaks React hook
ordering; move the `if (!sessionAllowed) return null;` check down so all hooks
in the TabBarInner component (the useState declarations named draggingTabId,
dragOverTabId, isDraggingOutsideWindow, isDropZoneHighlightActive) execute
unconditionally at render start, and perform the sessionAllowed early return
only immediately before the component's final return JSX.
In `@src/renderer/contexts/WindowContext.tsx`:
- Around line 240-266: The state update performed via setWindowState (the block
that merges sessionIds from sessionIdsToAssign into prev.sessionIds) runs before
checking feature availability; move or gate this optimistic UI update so it only
runs if window.maestro?.windows?.assignSessions is present. Specifically, ensure
the window.maestro?.windows?.assignSessions check happens before invoking
setWindowState (or wrap the setWindowState logic with that condition) so you
don't mutate local state when the backend API is unavailable; reference the
sessionIdsToAssign variable and the setWindowState callback to locate and adjust
the code.
In `@src/renderer/hooks/agent/useMergeSession.ts`:
- Around line 688-694: The merge path in executeMerge awaits the external
callback assignSessionsToWindow([newSession.id]) with no timeout, so a hung
promise can block completion; update the code in useMergeSession's executeMerge
flow to wrap the assignSessionsToWindow call in a bounded promise (e.g.,
Promise.race with a short timeout like 3–5s) so that if assignSessionsToWindow
hangs or rejects the merge still completes, log or console.error on
timeout/rejection and continue; reference assignSessionsToWindow, executeMerge,
and newSession.id when locating where to add the timeout wrapper and error
handling.
---
Outside diff comments:
In `@src/main/index.ts`:
- Line 839: registerNotificationsHandlers() is being invoked without the
required WindowRegistry, causing metadata-based focus resolution in
notifications.ts to fall back to source-window behavior; update the call in
src/main/index.ts to pass the existing WindowRegistry instance (e.g.,
windowRegistry) into registerNotificationsHandlers and update the
registerNotificationsHandlers function signature in notifications.ts to accept
and store/use that WindowRegistry for focus resolution, ensuring IPC handlers
remain registered in the correct order before making this change.
In `@src/main/preload/groupChat.ts`:
- Around line 15-19: The preload ModeratorConfig interface is missing the
customModel property so moderatorConfig built by NewGroupChatModal gets dropped
at the IPC boundary; update the export interface ModeratorConfig to include
customModel?: string (i.e., add the optional customModel field alongside
customPath, customArgs, customEnvVars) so the value is preserved for the main
process handlers in group-chat-router (where customModel is consumed).
In `@src/renderer/App.tsx`:
- Around line 8371-8374: The new wizard-created session is added via setSessions
and activated with setActiveSessionId but never assigned to the current window,
so update the callback that references assignSessionsToWindow to call it for the
newly created session ID (newId) after adding the session; specifically, after
setSessions/setActiveSessionId/updateGlobalStats, invoke
assignSessionsToWindow([newId]) (or the appropriate signature) so the session is
bound to the window—apply the same change at the other occurrence around the
code that uses setSessions/setActiveSessionId/updateGlobalStats (the second
block referenced).
In `@src/renderer/components/TabBar.tsx`:
- Around line 598-618: The tab container JSX that uses setTabRef and handlers
like handleTabSelect, handleMouseDown, handleMouseEnter/Leave and drag handlers
must be made keyboard-focusable: add tabIndex={0} to the root div and wire focus
handlers (onFocus and onBlur) to the existing focus-related helpers (or
implement new handlers such as handleTabFocus and handleTabBlur alongside
setTabRef) and ensure keyboard activation is supported via onKeyDown (e.g., call
handleTabSelect when Enter/Space is pressed). Apply the same changes for the
other tab instance (the block around lines 1422–1442) so both draggable tabs are
accessible via keyboard and have focus state handling.
---
Nitpick comments:
In `@src/__tests__/main/preload/notifications.test.ts`:
- Around line 39-44: Add a new unit test that verifies metadata is forwarded by
calling api.show with a metadata object and asserting mockInvoke was called with
'notification:show', the title, the body, and the same metadata; specifically,
in src/__tests__/main/preload/notifications.test.ts add an it block that sets
mockInvoke.mockResolvedValue({ success: true }), constructs a metadata object
(e.g., { sessionId: 'session-1', windowId: 'window-1' }), calls await
api.show('Test Title', 'Test Body', metadata), and expects
mockInvoke.toHaveBeenCalledWith('notification:show', 'Test Title', 'Test Body',
metadata) so the test covers the new signature path in api.show and mockInvoke
usage.
In `@src/__tests__/renderer/components/QuickActionsModal.test.tsx`:
- Around line 814-816: The final three lines containing
expect(buttons[0]).toHaveStyle({ backgroundColor: mockTheme.colors.accent });
}); }); have lost their indentation; restore their indentation to match the
enclosing test block so the assertion and the closing braces line up with the
surrounding describe/it structure (ensure the expect line is indented inside the
it block and the two closing "});" align with their corresponding it/describe
openings referencing the QuickActionsModal test and the buttons/mocKTheme
usage).
In `@src/main/app-lifecycle/quit-handler.ts`:
- Around line 328-331: The fallback chain assigning persisted is redundant:
replace the dual access with a single reliable read using
windowStateStore.get('multiWindowState') and fall back to fallback (i.e., remove
windowStateStore.store.multiWindowState), or if you intentionally need the
direct store access for an electron-store edge case, keep both but add a brief
comment above the line explaining the edge case and why
windowStateStore.store.multiWindowState is required; refer to
windowStateStore.get, windowStateStore.store.multiWindowState, and the persisted
variable when making the change.
In `@src/main/app-lifecycle/window-manager.ts`:
- Line 587: Remove the extraneous blank line inside the WindowManager class in
window-manager.ts (the blank within the method/block around where the reviewer
noted line 587); edit the WindowManager source to delete that single empty line
so formatting matches the surrounding code and linter/style checks pass.
In `@src/main/ipc/handlers/index.ts`:
- Around line 294-303: The logger forwarding currently targets only
getMainWindow; modify setupLoggerEventForwarding to accept a window-broadcast
capability (e.g., getWindowRegistry or a broadcastToAllWindows utility) and
change its implementation to broadcast logger events to every window returned by
that registry (similar to how marketplace:manifestChanged or
notification:commandCompleted broadcast). Update the call site in this file to
pass deps.getWindowRegistry (or the broadcast helper) instead of just
deps.getMainWindow, and ensure any related registration logic in
registerWindowsHandlers remains unchanged.
In `@src/main/ipc/handlers/windows.ts`:
- Around line 554-576: cloneWindowState is duplicated and should be extracted to
a shared utility to avoid duplication; move the implementation of
cloneWindowState into a common module (e.g., a new util file), export it, update
call sites to import the shared cloneWindowState, and ensure its signature and
shallow-copy behavior (spreading sessionIds into a new array) remain identical.
Do not change or consolidate getMultiWindowStateSnapshot or
createDefaultWindowStateSnapshot here — keep their context-specific fallbacks,
null-safety (?.[0] vs [0]), x/y handling, return types and any extra fields like
displayId intact; when updating imports, run type checks to ensure no type
mismatch at call sites.
In `@src/main/process-listeners/data-listener.ts`:
- Line 109: The current hot-path call broadcastToAllWindows('process:data',
sessionId, data) fans out every chunk to all renderers; instead, resolve the
owning renderer for sessionId (e.g., via a sessionManager/getWindowForSession or
similar ownership lookup) and send the IPC only to that window (use the
per-window send method rather than broadcast); if ownership is unknown or
multiple subscribers exist, fall back to a narrowed subscriber list or to
broadcast as last resort — update the logic around broadcastToAllWindows to
prefer sendToWindow/getWindowForSession(sessionId) with a safe fallback.
In `@src/main/process-listeners/forwarding-listeners.ts`:
- Around line 15-44: The current listeners (processManager.on handlers for
'thinking-chunk', 'stderr', 'tool-execution', 'slash-commands', 'command-exit')
call broadcastToAllWindows which causes unnecessary renderer churn; instead
route events to the specific owning window for the session by looking up the
session→window mapping and sending only to that window. Modify the function
signature to accept or use an existing session-to-window helper (e.g.
getWindowForSession or broadcastToWindow) and replace
broadcastToAllWindows('process:...', sessionId, ...) in the handlers with a
targeted call that resolves the window for sessionId and sends the same payload
only to that window (fall back to broadcastToAllWindows if no owner found).
Ensure you update all handlers listed above to use the targeted dispatch.
In `@src/main/stats/schema.ts`:
- Around line 133-139: The window_usage_events table definition must enforce
non-negative counters: modify the CREATE TABLE for window_usage_events to add
CHECK constraints (or named CONSTRAINTs) on the window_count and session_count
columns so they require value >= 0; reference the table name window_usage_events
and the column names window_count and session_count when making the change and
ensure the updated schema is used wherever the table is created/migrated in
schema.ts.
In `@src/main/window-registry.ts`:
- Around line 457-462: cloneWindowState currently does a shallow spread plus
copying sessionIds, which will miss nested objects if PersistedWindowState later
includes them; update the implementation of cloneWindowState to perform a true
deep clone of the entire PersistedWindowState (e.g., use
structuredClone(windowState) where available, with a safe fallback like
JSON.parse(JSON.stringify(windowState)) or lodash.cloneDeep) so all nested
objects and arrays are duplicated rather than referenced, keeping the function
name cloneWindowState and return type PersistedWindowState unchanged.
In `@src/renderer/components/MainPanel.tsx`:
- Around line 1245-1249: The click handler duplicates stop-target
resolution—replace the inline logic with a call to the existing
handleStopWindowAutoRun function: remove the anonymous onClick handler that
checks isWindowAutoModeStopping, computes targetSessionId from
windowAutoRunSessionId or activeSession.id, and calls onStopBatchRun; instead
invoke handleStopWindowAutoRun so the single implementation handles stopping
(ensure handleStopWindowAutoRun is in scope for MainPanel and wired to the same
onStopBatchRun behavior and state such as isWindowAutoModeStopping and
windowAutoRunSessionId).
- Around line 1884-1885: The current conditional passes onStopAutoRun whenever
windowAutoRunState is truthy; instead gate it on the run being active or
stopping. Change the prop expression so onStopAutoRun is set only when
windowAutoRunState indicates an active/stopping state (e.g.
windowAutoRunState?.status === 'active' || windowAutoRunState?.status ===
'stopping') while still passing autoRunState={windowAutoRunState || undefined};
update the JSX where windowAutoRunState and handleStopWindowAutoRun are used to
enforce this state-based check.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (94)
.gitignoredocs/general-usage.mdsrc/__tests__/main/app-lifecycle/quit-handler.test.tssrc/__tests__/main/app-lifecycle/window-manager.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/stats.test.tssrc/__tests__/main/stats/data-management.test.tssrc/__tests__/main/stores/defaults.test.tssrc/__tests__/main/stores/instances.test.tssrc/__tests__/main/stores/types.test.tssrc/__tests__/main/window-registry.test.tssrc/__tests__/renderer/components/MainPanel.test.tsxsrc/__tests__/renderer/components/QuickActionsModal.test.tsxsrc/__tests__/renderer/components/ShortcutsHelpModal.test.tsxsrc/__tests__/renderer/components/TabBar.test.tsxsrc/__tests__/renderer/hooks/useSendToAgent.test.tssrc/__tests__/renderer/hooks/useWindowState.test.tsxsrc/__tests__/renderer/utils/windowOrdering.test.tssrc/__tests__/setup.tssrc/main/app-lifecycle/quit-handler.tssrc/main/app-lifecycle/window-manager.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/process-listeners/__tests__/data-listener.test.tssrc/main/process-listeners/__tests__/error-listener.test.tssrc/main/process-listeners/__tests__/exit-listener.test.tssrc/main/process-listeners/__tests__/forwarding-listeners.test.tssrc/main/process-listeners/__tests__/session-id-listener.test.tssrc/main/process-listeners/__tests__/usage-listener.test.tssrc/main/process-listeners/data-listener.tssrc/main/process-listeners/error-listener.tssrc/main/process-listeners/exit-listener.tssrc/main/process-listeners/forwarding-listeners.tssrc/main/process-listeners/session-id-listener.tssrc/main/process-listeners/types.tssrc/main/process-listeners/usage-listener.tssrc/main/stats/data-management.tssrc/main/stats/migrations.tssrc/main/stats/row-mappers.tssrc/main/stats/schema.tssrc/main/stats/stats-db.tssrc/main/stats/window-usage.tssrc/main/stores/defaults.tssrc/main/stores/instances.tssrc/main/stores/types.tssrc/main/window-registry.tssrc/renderer/App.tsxsrc/renderer/components/History/HistoryEntryItem.tsxsrc/renderer/components/History/HistoryFilterToggle.tsxsrc/renderer/components/HistoryDetailModal.tsxsrc/renderer/components/InputArea.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/SessionItem.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/ShortcutsHelpModal.tsxsrc/renderer/components/TabBar.tsxsrc/renderer/constants/shortcuts.tssrc/renderer/contexts/ToastContext.tsxsrc/renderer/contexts/WindowContext.tsxsrc/renderer/global.d.tssrc/renderer/hooks/agent/useMergeSession.tssrc/renderer/hooks/agent/useSendToAgent.tssrc/renderer/hooks/index.tssrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/hooks/props/useMainPanelProps.tssrc/renderer/hooks/session/index.tssrc/renderer/hooks/session/useSessionWindowAssignments.tssrc/renderer/hooks/useWindowState.tssrc/renderer/types/index.tssrc/renderer/utils/__tests__/windowSessionOrdering.test.tssrc/renderer/utils/windowOrdering.tssrc/renderer/utils/windowSessionOrdering.tssrc/shared/group-chat-types.tssrc/shared/stats-types.tssrc/shared/types/notification.tssrc/shared/types/window.ts
Comment on lines
+331
to
+342
| ## Multi-Window Workflow | ||
|
|
||
| Maestro windows share the same pool of sessions, and every window is labeled `Maestro [N]` (both in the draggable title bar and your OS window switcher) so you always know where a tab lives. | ||
|
|
||
| ### Opening a New Window | ||
|
|
||
| - **Drag any session tab off the tab bar**. Once your cursor leaves the window frame, release to spawn a new window anchored near your cursor with that tab already active. | ||
| - **Right-click → Move to New Window**. Every session tab context menu includes a "Move to New Window" action if you prefer a precise command instead of drag-and-drop. | ||
|
|
||
| Tabs keep their entire conversation history, SSH settings, and toggle states when moved, so you can lay out long-running sessions across monitors without losing context. | ||
|
|
||
| ### Tracking Where Tabs Live |
There was a problem hiding this comment.
Fix heading hierarchy in Tab Management docs.
Line 331 promotes Multi-Window to ##, which incorrectly nests later tab topics under it. Keep it under Tab Management instead.
📚 Proposed heading-level fix
-## Multi-Window Workflow
+### Multi-Window Workflow
@@
-### Opening a New Window
+#### Opening a New Window
@@
-### Tracking Where Tabs Live
+#### Tracking Where Tabs Live📝 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.
Suggested change
| ## Multi-Window Workflow | |
| Maestro windows share the same pool of sessions, and every window is labeled `Maestro [N]` (both in the draggable title bar and your OS window switcher) so you always know where a tab lives. | |
| ### Opening a New Window | |
| - **Drag any session tab off the tab bar**. Once your cursor leaves the window frame, release to spawn a new window anchored near your cursor with that tab already active. | |
| - **Right-click → Move to New Window**. Every session tab context menu includes a "Move to New Window" action if you prefer a precise command instead of drag-and-drop. | |
| Tabs keep their entire conversation history, SSH settings, and toggle states when moved, so you can lay out long-running sessions across monitors without losing context. | |
| ### Tracking Where Tabs Live | |
| ### Multi-Window Workflow | |
| Maestro windows share the same pool of sessions, and every window is labeled `Maestro [N]` (both in the draggable title bar and your OS window switcher) so you always know where a tab lives. | |
| #### Opening a New Window | |
| - **Drag any session tab off the tab bar**. Once your cursor leaves the window frame, release to spawn a new window anchored near your cursor with that tab already active. | |
| - **Right-click → Move to New Window**. Every session tab context menu includes a "Move to New Window" action if you prefer a precise command instead of drag-and-drop. | |
| Tabs keep their entire conversation history, SSH settings, and toggle states when moved, so you can lay out long-running sessions across monitors without losing context. | |
| #### Tracking Where Tabs Live |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/general-usage.md` around lines 331 - 342, The "Multi-Window Workflow"
heading is currently at level "##" and incorrectly elevates it out of the "Tab
Management" section; change the heading "Multi-Window Workflow" to a lower level
(e.g., "### Multi-Window Workflow") so it remains nested under the existing "Tab
Management" heading and adjust any sibling headings in this block if needed to
preserve correct hierarchy for subsequent topics like "Opening a New Window" and
"Tracking Where Tabs Live".
Comment on lines
+353
to
+364
| // Initialize auto-updater (only in production) | ||
| if (!isDevelopment) { | ||
| initAutoUpdater(browserWindow); | ||
| logger.info('Auto-updater initialized', 'Window'); | ||
| } else { | ||
| // Register stub handlers in development mode so users get a helpful error | ||
| registerDevAutoUpdaterStubs(); | ||
| logger.info( | ||
| 'Auto-updater disabled in development mode (stub handlers registered)', | ||
| 'Window' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Auto-updater initialized for every window in production.
initAutoUpdater(browserWindow) is called for each window created. Looking at the auto-updater snippet from context, it stores a single mainWindow reference. Multiple windows calling this will overwrite that reference, and update events will only be sent to the last window created.
🔧 Proposed fix to initialize auto-updater only once
+// Track if auto-updater has been initialized (module-level)
+let autoUpdaterInitialized = false;
+
// Initialize auto-updater (only in production)
if (!isDevelopment) {
- initAutoUpdater(browserWindow);
- logger.info('Auto-updater initialized', 'Window');
+ if (!autoUpdaterInitialized) {
+ initAutoUpdater(browserWindow);
+ autoUpdaterInitialized = true;
+ logger.info('Auto-updater initialized', 'Window');
+ }
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/app-lifecycle/window-manager.ts` around lines 353 - 364,
initAutoUpdater(browserWindow) is being invoked for every new window in
production which causes the auto-updater's single stored mainWindow to be
overwritten; guard initialization so it runs only once (or only for the
designated main window). Update the window creation path to check a
module/app-level flag (e.g., autoUpdaterInitialized) or only call
initAutoUpdater from the main window creation logic, set that flag to true after
calling initAutoUpdater(browserWindow), and keep registerDevAutoUpdaterStubs()
behavior in development; ensure the stored mainWindow inside the auto-updater is
only set on the first initialization to avoid later windows overwriting it.
Comment on lines
+329
to
333
| browserWindow.on('closed', () => { | ||
| if (mainWindow === browserWindow) { | ||
| mainWindow = null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Reassign mainWindow when the current one closes.
At Line 331, mainWindow is set to null when that window closes, even if other windows remain. Callers that still rely on getMainWindow() can break until a new window is created.
Suggested fix
browserWindow.on('closed', () => {
if (mainWindow === browserWindow) {
- mainWindow = null;
+ const nextPrimary = windowRegistry?.getPrimary();
+ mainWindow = nextPrimary?.browserWindow ?? null;
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/index.ts` around lines 329 - 333, The current close handler only
nulls the global mainWindow when the closed BrowserWindow equals browserWindow,
which leaves mainWindow null even if other windows exist; update the handler
attached to browserWindow (the browserWindow.on('closed', ...) block) to: when a
window closes, if mainWindow === browserWindow then attempt to reassign
mainWindow to another open window returned by BrowserWindow.getAllWindows()
(pick the first remaining window) and only set mainWindow = null if no other
windows exist; ensure you reference the mainWindow variable and
BrowserWindow.getAllWindows() in the implementation.
Comment on lines
601
to
604
| app.on('window-all-closed', () => { | ||
| if (process.platform !== 'darwin') { | ||
| const primaryWindow = windowRegistry?.getPrimary(); | ||
| if (process.platform !== 'darwin' || !primaryWindow) { | ||
| app.quit(); |
There was a problem hiding this comment.
window-all-closed currently forces quit on macOS.
At Line 603, !primaryWindow makes the condition true after all windows close, so the app quits on macOS too. That bypasses the usual macOS lifecycle where the app stays alive and reopens on activate.
Suggested fix
app.on('window-all-closed', () => {
- const primaryWindow = windowRegistry?.getPrimary();
- if (process.platform !== 'darwin' || !primaryWindow) {
+ if (process.platform !== 'darwin') {
app.quit();
}
});📝 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.
Suggested change
| app.on('window-all-closed', () => { | |
| if (process.platform !== 'darwin') { | |
| const primaryWindow = windowRegistry?.getPrimary(); | |
| if (process.platform !== 'darwin' || !primaryWindow) { | |
| app.quit(); | |
| app.on('window-all-closed', () => { | |
| if (process.platform !== 'darwin') { | |
| app.quit(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/index.ts` around lines 601 - 604, The current window-all-closed
handler uses windowRegistry?.getPrimary() and quits when process.platform !==
'darwin' || !primaryWindow, which causes the app to quit on macOS; change the
handler to only call app.quit() when process.platform !== 'darwin' (remove the
!primaryWindow check and any dependence on windowRegistry.getPrimary()), so
macOS keeps the app running and follows the normal activate lifecycle; update
the app.on('window-all-closed', ...) callback accordingly.
Comment on lines
+245
to
+251
| for (const { sessionId, fromWindowId } of assignments) { | ||
| broadcastSessionMoved(windowRegistry, { | ||
| sessionId, | ||
| fromWindowId: fromWindowId ?? targetWindowId, | ||
| toWindowId: targetWindowId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Incorrect fromWindowId when session was previously unassigned.
When fromWindowId is null (session wasn't previously assigned to any window), the broadcast uses targetWindowId as fromWindowId, which is misleading. Consider omitting the broadcast for newly assigned sessions or using a sentinel value.
🔧 Proposed fix
for (const { sessionId, fromWindowId } of assignments) {
+ if (!fromWindowId) {
+ // Skip broadcast for sessions that were not previously assigned
+ continue;
+ }
broadcastSessionMoved(windowRegistry, {
sessionId,
- fromWindowId: fromWindowId ?? targetWindowId,
+ fromWindowId,
toWindowId: targetWindowId,
});
}📝 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.
Suggested change
| for (const { sessionId, fromWindowId } of assignments) { | |
| broadcastSessionMoved(windowRegistry, { | |
| sessionId, | |
| fromWindowId: fromWindowId ?? targetWindowId, | |
| toWindowId: targetWindowId, | |
| }); | |
| } | |
| for (const { sessionId, fromWindowId } of assignments) { | |
| if (!fromWindowId) { | |
| // Skip broadcast for sessions that were not previously assigned | |
| continue; | |
| } | |
| broadcastSessionMoved(windowRegistry, { | |
| sessionId, | |
| fromWindowId, | |
| toWindowId: targetWindowId, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/windows.ts` around lines 245 - 251, The broadcast loop
uses targetWindowId as fromWindowId when a session was previously unassigned,
producing misleading events; update the loop in the assignments handling (the
for ... of assignments block that calls broadcastSessionMoved) to detect when
fromWindowId is null/undefined and either skip calling broadcastSessionMoved for
newly assigned sessions or pass a clear sentinel (e.g., null/undefined or a
dedicated string) as fromWindowId instead of targetWindowId; ensure references
to sessionId, fromWindowId, targetWindowId and windowRegistry remain intact and
adjust any downstream consumers to handle the sentinel if you choose that route.
Comment on lines
+1552
to
+1559
| if (currentWindowId) { | ||
| const assignment = sessionWindowAssignments.get(sessionId); | ||
| if (assignment && assignment.windowId !== currentWindowId) { | ||
| window.maestro.windows | ||
| .focusWindow(assignment.windowId) | ||
| .catch((error) => console.error('Failed to focus window for session', error)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Handle remote-focus failure so selection clicks don’t become no-ops.
If focusWindow fails (stale assignment/window closed race), the early return drops the user action with only a console error.
🛠️ Suggested fallback
(sessionId: string) => {
if (currentWindowId) {
const assignment = sessionWindowAssignments.get(sessionId);
if (assignment && assignment.windowId !== currentWindowId) {
window.maestro.windows
.focusWindow(assignment.windowId)
- .catch((error) => console.error('Failed to focus window for session', error));
+ .catch((error) => {
+ console.error('Failed to focus window for session', error);
+ // Fallback for stale assignment races
+ setActiveSessionId(sessionId);
+ });
return;
}
}
setActiveSessionId(sessionId);
},📝 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.
Suggested change
| if (currentWindowId) { | |
| const assignment = sessionWindowAssignments.get(sessionId); | |
| if (assignment && assignment.windowId !== currentWindowId) { | |
| window.maestro.windows | |
| .focusWindow(assignment.windowId) | |
| .catch((error) => console.error('Failed to focus window for session', error)); | |
| return; | |
| } | |
| if (currentWindowId) { | |
| const assignment = sessionWindowAssignments.get(sessionId); | |
| if (assignment && assignment.windowId !== currentWindowId) { | |
| window.maestro.windows | |
| .focusWindow(assignment.windowId) | |
| .catch((error) => { | |
| console.error('Failed to focus window for session', error); | |
| // Fallback for stale assignment races | |
| setActiveSessionId(sessionId); | |
| }); | |
| return; | |
| } | |
| } | |
| setActiveSessionId(sessionId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SessionList.tsx` around lines 1552 - 1559, The click
handler currently calls window.maestro.windows.focusWindow(assignment.windowId)
and returns early on any failure, so when focusWindow rejects the user click
becomes a no-op; change this by handling focusWindow's promise so that on
rejection you log the error but continue with the normal selection flow (i.e.,
do not return early). Locate the block referencing currentWindowId,
sessionWindowAssignments.get(sessionId), assignment.windowId and
window.maestro.windows.focusWindow and modify it so the promise rejection is
caught and the handler proceeds to the same selection logic as the success path
(or calls the session selection function), ensuring you avoid duplicate
selection side-effects if focus succeeds. Ensure the catch still logs the error
detail (error) for debugging.
Comment on lines
+16
to
+19
| const SCOPE_DESCRIPTIONS: Record<ShortcutScope, string> = { | ||
| window: 'Only impacts the window where you trigger it (panels, focus, tab layout).', | ||
| global: 'Syncs across every Maestro window (sessions, bookmarks, shared dashboards).', | ||
| }; |
There was a problem hiding this comment.
Use “agent” terminology in the new user-facing shortcut help text.
The newly added helper copy uses “session/session tab,” which conflicts with the repo terminology rule for user-facing language.
✏️ Suggested wording update
const SCOPE_DESCRIPTIONS: Record<ShortcutScope, string> = {
window: 'Only impacts the window where you trigger it (panels, focus, tab layout).',
- global: 'Syncs across every Maestro window (sessions, bookmarks, shared dashboards).',
+ global: 'Syncs across every Maestro window (agents, bookmarks, shared dashboards).',
};
@@
<span
className="px-2 py-1 rounded border"
style={{ borderColor: theme.colors.border, color: theme.colors.textDim }}
>
- All windows — shared sessions, bookmarks, dashboards
+ All windows — shared agents, bookmarks, dashboards
</span>
@@
- <strong>Move to New Window:</strong> Right-click any session tab and choose "Move to New Window" to pop it out. The window registry keeps that session assigned until you drag it elsewhere.
+ <strong>Move to New Window:</strong> Right-click any agent tab and choose "Move to New Window" to pop it out. The window registry keeps that agent assigned until you drag it elsewhere.
</div>As per coding guidelines src/**/*.{ts,tsx}: "Use the Session interface in code to represent agents (due to historical naming), but use 'agent' in user-facing language and 'session' for provider-level conversation contexts".
Also applies to: 113-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/ShortcutsHelpModal.tsx` around lines 16 - 19, The
user-facing shortcut help text in ShortcutsHelpModal.tsx uses "session/session
tab" which violates the repo guideline to use "agent" in UI copy; update the
string values in SCOPE_DESCRIPTIONS (and any other user-facing strings in this
file around the other shortcut/help text blocks referenced) to replace "session"
or "session tab" with "agent" or "agent tab" while leaving internal types like
ShortcutScope/Session interfaces unchanged so only the displayed copy changes.
Comment on lines
+1714
to
+1723
| const sessionAllowed = | ||
| !sessionId || windowSessionIds.length === 0 || windowSessionIds.includes(sessionId); | ||
|
|
||
| if (!sessionAllowed) { | ||
| return null; | ||
| } | ||
| const [draggingTabId, setDraggingTabId] = useState<string | null>(null); | ||
| const [dragOverTabId, setDragOverTabId] = useState<string | null>(null); | ||
| const [isDraggingOutsideWindow, setIsDraggingOutsideWindow] = useState(false); | ||
| const [isDropZoneHighlightActive, setIsDropZoneHighlightActive] = useState(false); |
There was a problem hiding this comment.
Fix conditional hook execution in TabBarInner.
The guard at Line 1717 returns before subsequent hooks (e.g., Line 1720), which violates React’s hook ordering guarantees.
🐛 Proposed fix
const sessionAllowed =
!sessionId || windowSessionIds.length === 0 || windowSessionIds.includes(sessionId);
-
- if (!sessionAllowed) {
- return null;
- }// Place this immediately before the component's final `return (...)`,
// after all hook declarations:
if (!sessionAllowed) {
return null;
}Run this read-only check after patching (expected: early_return_before_state_hook = False):
#!/bin/bash
python - <<'PY'
from pathlib import Path
text = Path("src/renderer/components/TabBar.tsx").read_text()
early = text.find("if (!sessionAllowed)")
state_hook = text.find("const [draggingTabId")
print("early_return_before_state_hook =", early != -1 and state_hook != -1 and early < state_hook)
PY🧰 Tools
🪛 Biome (2.4.4)
[error] 1720-1720: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
(lint/correctness/useHookAtTopLevel)
[error] 1721-1721: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
(lint/correctness/useHookAtTopLevel)
[error] 1722-1722: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
(lint/correctness/useHookAtTopLevel)
[error] 1723-1723: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/TabBar.tsx` around lines 1714 - 1723, The early
return using sessionAllowed is placed before hooks (e.g., useState calls for
draggingTabId, dragOverTabId, isDraggingOutsideWindow,
isDropZoneHighlightActive) which breaks React hook ordering; move the `if
(!sessionAllowed) return null;` check down so all hooks in the TabBarInner
component (the useState declarations named draggingTabId, dragOverTabId,
isDraggingOutsideWindow, isDropZoneHighlightActive) execute unconditionally at
render start, and perform the sessionAllowed early return only immediately
before the component's final return JSX.
Comment on lines
+240
to
+266
| setWindowState((prev) => { | ||
| if (!prev) { | ||
| return prev; | ||
| } | ||
|
|
||
| const existing = new Set(prev.sessionIds); | ||
| let changed = false; | ||
| for (const sessionId of sessionIdsToAssign) { | ||
| if (!existing.has(sessionId)) { | ||
| existing.add(sessionId); | ||
| changed = true; | ||
| } | ||
| } | ||
|
|
||
| if (!changed) { | ||
| return prev; | ||
| } | ||
|
|
||
| return { | ||
| ...prev, | ||
| sessionIds: Array.from(existing), | ||
| }; | ||
| }); | ||
|
|
||
| if (!window.maestro?.windows?.assignSessions) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid mutating local state before feature-availability check.
At Line 240, local windowState is updated before verifying window.maestro.windows.assignSessions exists. If that API is unavailable, Line 265 returns early and leaves UI state inconsistent with backend state.
Suggested fix
const assignSessionsToWindow = useCallback(
async (sessionIdsToAssign: string[]) => {
if (!windowId || !sessionIdsToAssign.length) {
return;
}
+ if (!window.maestro?.windows?.assignSessions) {
+ return;
+ }
+
setWindowState((prev) => {
if (!prev) {
return prev;
}
@@
- if (!window.maestro?.windows?.assignSessions) {
- return;
- }
-
try {
await window.maestro.windows.assignSessions({
sessionIds: sessionIdsToAssign,
windowId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/contexts/WindowContext.tsx` around lines 240 - 266, The state
update performed via setWindowState (the block that merges sessionIds from
sessionIdsToAssign into prev.sessionIds) runs before checking feature
availability; move or gate this optimistic UI update so it only runs if
window.maestro?.windows?.assignSessions is present. Specifically, ensure the
window.maestro?.windows?.assignSessions check happens before invoking
setWindowState (or wrap the setWindowState logic with that condition) so you
don't mutate local state when the backend API is unavailable; reference the
sessionIdsToAssign variable and the setWindowState callback to locate and adjust
the code.
Comment on lines
+688
to
+694
| if (assignSessionsToWindow) { | ||
| try { | ||
| await assignSessionsToWindow([newSession.id]); | ||
| } catch (error) { | ||
| console.error('Failed to assign merged session to window', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bound the window-assignment await to prevent hung merge completion.
executeMerge currently waits on an external async callback with no timeout. If that promise hangs, this flow never resolves even after session creation/state update.
Proposed fix
setSessions((prev) => [...prev, newSession]);
if (assignSessionsToWindow) {
+ const ASSIGN_TIMEOUT_MS = 3000;
try {
- await assignSessionsToWindow([newSession.id]);
+ await Promise.race([
+ Promise.resolve(assignSessionsToWindow([newSession.id])),
+ new Promise((_, reject) =>
+ setTimeout(
+ () => reject(new Error('assignSessionsToWindow timed out')),
+ ASSIGN_TIMEOUT_MS
+ )
+ ),
+ ]);
} catch (error) {
console.error('Failed to assign merged session to window', error);
}
}📝 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.
Suggested change
| if (assignSessionsToWindow) { | |
| try { | |
| await assignSessionsToWindow([newSession.id]); | |
| } catch (error) { | |
| console.error('Failed to assign merged session to window', error); | |
| } | |
| } | |
| if (assignSessionsToWindow) { | |
| const ASSIGN_TIMEOUT_MS = 3000; | |
| try { | |
| await Promise.race([ | |
| Promise.resolve(assignSessionsToWindow([newSession.id])), | |
| new Promise((_, reject) => | |
| setTimeout( | |
| () => reject(new Error('assignSessionsToWindow timed out')), | |
| ASSIGN_TIMEOUT_MS | |
| ) | |
| ), | |
| ]); | |
| } catch (error) { | |
| console.error('Failed to assign merged session to window', error); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/agent/useMergeSession.ts` around lines 688 - 694, The
merge path in executeMerge awaits the external callback
assignSessionsToWindow([newSession.id]) with no timeout, so a hung promise can
block completion; update the code in useMergeSession's executeMerge flow to wrap
the assignSessionsToWindow call in a bounded promise (e.g., Promise.race with a
short timeout like 3–5s) so that if assignSessionsToWindow hangs or rejects the
merge still completes, log or console.error on timeout/rejection and continue;
reference assignSessionsToWindow, executeMerge, and newSession.id when locating
where to add the timeout wrapper and error handling.
…llision with PR-391 PR-391 (Virtuosos) claims migration v4 for account usage tables. Since PR-339 merges last, bump window_usage_events migration to v5. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/main/stats/stats-db.test.ts (1)
287-289: Optional: avoid repeating hard-coded schema version literals.These assertions/mocks are correct, but centralizing the expected version (e.g.,
STATS_DB_VERSION) will reduce maintenance churn on the next migration bump.♻️ Suggested cleanup
import type { QueryEvent, AutoRunSession, AutoRunTask, SessionLifecycleEvent, StatsTimeRange, StatsFilters, StatsAggregation, + STATS_DB_VERSION, } from '../../../shared/stats-types'; @@ - expect(db.getTargetVersion()).toBe(5); + expect(db.getTargetVersion()).toBe(STATS_DB_VERSION); @@ - if (sql === 'user_version') return [{ user_version: 5 }]; + if (sql === 'user_version') return [{ user_version: STATS_DB_VERSION }]; @@ - let currentVersion = 5; + let currentVersion = STATS_DB_VERSION; @@ - expect(db.getCurrentVersion()).toBe(5); - expect(db.getTargetVersion()).toBe(5); + expect(db.getCurrentVersion()).toBe(STATS_DB_VERSION); + expect(db.getTargetVersion()).toBe(STATS_DB_VERSION);Also applies to: 293-293, 308-309, 323-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 287 - 289, Replace hard-coded schema version literals in the tests with a centralized constant: import or define STATS_DB_VERSION and use expect(db.getTargetVersion()).toBe(STATS_DB_VERSION) (and similarly replace other numeric assertions/mocks referencing 5 around the same test: the occurrences near getTargetVersion() and other expectations at lines ~293, ~308-309, ~323-325). Ensure the test file references the same STATS_DB_VERSION used by the migrations code (or export it from the module that defines the schema version) so the tests stay in sync with future migration bumps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/stats/migrations.ts`:
- Around line 59-63: The migrations list has a gap (version 4 is missing) which
will prevent future v4 migrations from applying; fix by adding a proper version
4 migration entry before the existing version 5 item or renumbering the current
migrateV5 entry to version 4. Concretely, either insert a new migration object
with version: 4 and a no-op or compatible up(db) implementation (so existing DBs
at v5 remain correct), or rename the current { version: 5, description: 'Add
window_usage_events...', up: (db) => migrateV5(db) } entry to version: 4 and
increment subsequent versions accordingly; ensure the unique symbol migrateV5 is
updated/retained to match the changed version.
---
Nitpick comments:
In `@src/__tests__/main/stats/stats-db.test.ts`:
- Around line 287-289: Replace hard-coded schema version literals in the tests
with a centralized constant: import or define STATS_DB_VERSION and use
expect(db.getTargetVersion()).toBe(STATS_DB_VERSION) (and similarly replace
other numeric assertions/mocks referencing 5 around the same test: the
occurrences near getTargetVersion() and other expectations at lines ~293,
~308-309, ~323-325). Ensure the test file references the same STATS_DB_VERSION
used by the migrations code (or export it from the module that defines the
schema version) so the tests stay in sync with future migration bumps.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/__tests__/main/stats/stats-db.test.tssrc/main/stats/migrations.tssrc/main/stats/schema.tssrc/shared/stats-types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/stats/schema.ts
Comment on lines
+59
to
+63
| { | ||
| version: 5, | ||
| description: 'Add window_usage_events table for multi-window analytics', | ||
| up: (db) => migrateV5(db), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Declared migration versions:"
rg -nP 'version:\s*\d+' src/main/stats/migrations.ts
python - <<'PY'
import re
from pathlib import Path
text = Path("src/main/stats/migrations.ts").read_text()
versions = [int(v) for v in re.findall(r'version:\s*(\d+)', text)]
if not versions:
print("No migrations found.")
raise SystemExit(0)
mn, mx = min(versions), max(versions)
expected = list(range(mn, mx + 1))
missing = sorted(set(expected) - set(versions))
print("Parsed versions:", versions)
print("Expected contiguous range:", expected)
print("Missing versions:", missing)
PYRepository: RunMaestro/Maestro
Length of output: 257
Fill the migration version gap between v3 and v5.
Version 5 directly follows version 3, skipping version 4 entirely. If a v4 migration is introduced later, databases already at v5 will never apply it (since m.version > currentVersion would be false), leaving the schema potentially incomplete. Ensure all shipped migrations form a strictly monotonic sequence with no gaps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/stats/migrations.ts` around lines 59 - 63, The migrations list has a
gap (version 4 is missing) which will prevent future v4 migrations from
applying; fix by adding a proper version 4 migration entry before the existing
version 5 item or renumbering the current migrateV5 entry to version 4.
Concretely, either insert a new migration object with version: 4 and a no-op or
compatible up(db) implementation (so existing DBs at v5 remain correct), or
rename the current { version: 5, description: 'Add window_usage_events...', up:
(db) => migrateV5(db) } entry to version: 4 and increment subsequent versions
accordingly; ensure the unique symbol migrateV5 is updated/retained to match the
changed version.
Rebase artifact caused assignSessionsToWindow([newId]) to be called twice when creating a new session. Removed the duplicate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/App.tsx (1)
8370-8374:⚠️ Potential issue | 🟠 MajorWizard-created sessions are not assigned to the current window.
In
handleWizardLaunchSession, Line 8371-Line 8373 creates and activates a new session but never callsassignSessionsToWindow([newId])(unlike Line 8202 and Line 12497). This can break per-window scoping for wizard-created sessions.🔧 Proposed fix
// Add session and make it active setSessions((prev) => [...prev, newSession]); setActiveSessionId(newId); + await assignSessionsToWindow([newId]); updateGlobalStats({ totalSessions: 1 });Also applies to: 8446-8447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 8370 - 8374, handleWizardLaunchSession creates and activates a new session (it calls setSessions, setActiveSessionId, updateGlobalStats) but never assigns the new session to the current window, breaking per-window scoping; fix by calling assignSessionsToWindow([newId]) after the session is created and activated (and likewise add that call in the other wizard-created branch around lines noted), ensuring you reference the newId produced in handleWizardLaunchSession so the newly created session is bound to the current window.
♻️ Duplicate comments (1)
src/renderer/App.tsx (1)
589-591:⚠️ Potential issue | 🟠 MajorWindow scope still fails open when
windowSessionIdsis empty.At Line 590,
windowSessionIdSetbecomesnullfor an empty assignment list, which disables filtering at Line 608, Line 3562, and Line 5499. In multi-window mode, an empty assignment should be an explicit empty scope, not global access.🔧 Proposed fix
- const windowSessionIdSet = useMemo(() => { - return windowSessionIds.length ? new Set(windowSessionIds) : null; - }, [windowSessionIds]); + const windowSessionIdSet = useMemo(() => new Set(windowSessionIds), [windowSessionIds]); const activeSession = useMemo(() => { if (!sessions.length) { return null; } const restrictedSessionIds = windowSessionIdSet; const resolveSession = (sessionId?: string | null) => { if (!sessionId) { return null; } const match = sessions.find((session) => session.id === sessionId); if (!match) { return null; } - if (restrictedSessionIds && !restrictedSessionIds.has(match.id)) { + if (!restrictedSessionIds.has(match.id)) { return null; } return match; }; - if (restrictedSessionIds) { - return ( - resolveSession(windowActiveSessionId) ?? - resolveSession(activeSessionId) ?? - sessions.find((session) => restrictedSessionIds.has(session.id)) ?? - null - ); - } - return ( resolveSession(windowActiveSessionId) ?? resolveSession(activeSessionId) ?? - sessions[0] ?? + sessions.find((session) => restrictedSessionIds.has(session.id)) ?? null ); }, [sessions, windowSessionIdSet, windowActiveSessionId, activeSessionId]); const thinkingSessions = useMemo(() => { const busySessions = sessions.filter((s) => s.state === 'busy' && s.busySource === 'ai'); - if (!windowSessionIdSet) { - return busySessions; - } return busySessions.filter((session) => windowSessionIdSet.has(session.id)); }, [sessions, windowSessionIdSet]);Also applies to: 608-621, 3560-3566, 5499-5514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 589 - 591, The current useMemo for windowSessionIdSet returns null when windowSessionIds is an empty array, which causes the code paths that check for falsy to treat an explicit empty assignment as "no scope"; update the memo so it returns an actual empty Set for an empty array and only returns null when windowSessionIds is truly undefined/null. Concretely, modify the windowSessionIdSet useMemo (the useMemo creating windowSessionIdSet) to something like: if windowSessionIds is strictly null/undefined return null, otherwise return new Set(windowSessionIds) so downstream checks (the spots referencing windowSessionIdSet around the filtering logic at the lines you noted) will treat an empty Set as an explicit empty scope rather than global access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/App.tsx`:
- Around line 8370-8374: handleWizardLaunchSession creates and activates a new
session (it calls setSessions, setActiveSessionId, updateGlobalStats) but never
assigns the new session to the current window, breaking per-window scoping; fix
by calling assignSessionsToWindow([newId]) after the session is created and
activated (and likewise add that call in the other wizard-created branch around
lines noted), ensuring you reference the newId produced in
handleWizardLaunchSession so the newly created session is bound to the current
window.
---
Duplicate comments:
In `@src/renderer/App.tsx`:
- Around line 589-591: The current useMemo for windowSessionIdSet returns null
when windowSessionIds is an empty array, which causes the code paths that check
for falsy to treat an explicit empty assignment as "no scope"; update the memo
so it returns an actual empty Set for an empty array and only returns null when
windowSessionIds is truly undefined/null. Concretely, modify the
windowSessionIdSet useMemo (the useMemo creating windowSessionIdSet) to
something like: if windowSessionIds is strictly null/undefined return null,
otherwise return new Set(windowSessionIds) so downstream checks (the spots
referencing windowSessionIdSet around the filtering logic at the lines you
noted) will treat an empty Set as an explicit empty scope rather than global
access.
…I mocks The PR's multi-window additions (useWindowContext, useSessionWindowAssignments) caused all 129 SessionList tests to crash with "useWindowContext must be used within a WindowProvider". Added vi.mock for WindowContext and window.maestro.windows API matching the pattern used in TabBar.test.tsx. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ead of safeSend The multi-window PR renamed safeSend to broadcastToAllWindows in ProcessListenerDependencies but 6 test files still passed the old safeSend key, causing 61 test failures. Updated mock deps and assertions in data-listener, error-listener, exit-listener, forwarding-listeners, session-id-listener, and usage-listener tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
WindowRegistrymanager tracks multiple ElectronBrowserWindowinstances with per-window session assignments, panel collapse state, and display placementWindowContextprovides window identity throughout the React tree;TabBar,SessionList, keyboard shortcuts (Cmd+[,Cmd+K), and panel collapse are all scoped per windowTest plan
npm run testpasses for all new and updated test suites🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores