feat: add maestro:// deep link protocol and clickable OS notifications#509
feat: add maestro:// deep link protocol and clickable OS notifications#509pedramamini wants to merge 5 commits intomainfrom
Conversation
Introduce maestro:// URL scheme support for navigating to agents, tabs, and groups from external apps and OS notification clicks. - Add deep-links module with URL parsing, protocol registration, single-instance locking, and cross-platform event handling - Wire notification click handlers to navigate to the originating agent/tab via deep link dispatch - Thread sessionId/tabId context through notification preload bridge - Add onDeepLink listener in renderer with routing to existing navigation handlers - Register maestro:// protocol in electron-builder config - Add 18 tests covering URL parsing and notification click wiring
|
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:
📝 WalkthroughWalkthroughAdds maestro:// deep link support across the app: builders, parser, main-process protocol registration and single-instance handling, buffered dispatch to renderer via IPC, renderer listener and navigation, notification click wiring, preload API exposure, template variables, UI deep-link copy, tests, and documentation. Changes
Sequence DiagramsequenceDiagram
participant OS as User / OS
participant Main as Electron Main
participant Parser as DeepLink Parser
participant IPC as App IPC
participant Renderer as Renderer
participant Nav as Navigation Handler
OS->>Main: Open maestro://... (protocol/notification click)
Main->>Parser: parseDeepLink(url)
Parser-->>Main: ParsedDeepLink { action, ids? }
Main->>Main: Foreground window or buffer pending URL
Main->>IPC: Emit "app:deepLink" with parsed payload
IPC->>Renderer: Receive deepLink event
Renderer->>Renderer: onDeepLink callback invoked
alt action == session
Renderer->>Nav: handleToastSessionClick(sessionId, tabId?)
Nav-->>Renderer: Navigate to session
else action == group
Renderer->>Nav: Find group's first session, expand group
Nav-->>Renderer: Navigate & update UI
else action == focus
Renderer-->>Renderer: No-op (foreground handled by main)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces a Key changes and issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant OS as OS / CLI
participant Main as Electron Main
participant DeepLinks as deep-links.ts
participant Notif as notifications.ts
participant Renderer as Renderer (App.tsx)
Note over Main,DeepLinks: App startup (before app.whenReady)
Main->>DeepLinks: setupDeepLinkHandling(() => mainWindow)
DeepLinks->>Main: requestSingleInstanceLock()
alt got lock
DeepLinks-->>Main: true (proceed)
Main->>Main: app.whenReady() → createWindow()
Main->>DeepLinks: flushPendingDeepLink(() => mainWindow)
else lock taken
DeepLinks-->>Main: false
Main->>Main: app.quit()
end
Note over OS,Renderer: Deep link via OS protocol (macOS open-url)
OS->>Main: open-url event (maestro://session/xyz/tab/abc)
Main->>DeepLinks: processDeepLink(url)
DeepLinks->>DeepLinks: parseDeepLink(url) → ParsedDeepLink
DeepLinks->>Renderer: webContents.send('app:deepLink', parsed)
Renderer->>Renderer: handleToastSessionClick(sessionId, tabId)
Note over OS,Renderer: Deep link via Windows/Linux second-instance
OS->>Main: second-instance event (argv with maestro:// URL)
Main->>DeepLinks: processDeepLink(deepLinkUrl)
DeepLinks->>Renderer: webContents.send('app:deepLink', parsed)
Note over Notif,Renderer: Notification click-to-navigate
Renderer->>Notif: notification:show(title, body, sessionId, tabId)
Notif->>Notif: new Notification() + .on('click', handler)
OS->>Notif: User clicks OS notification
Notif->>DeepLinks: dispatchDeepLink(parsed, getMainWindow)
DeepLinks->>Renderer: webContents.send('app:deepLink', parsed)
Renderer->>Renderer: handleToastSessionClick(sessionId, tabId)
Last reviewed commit: ffbdb2f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const deepLinkUrl = tabId | ||
| ? `maestro://session/${sessionId}/tab/${tabId}` | ||
| : `maestro://session/${sessionId}`; | ||
|
|
There was a problem hiding this comment.
Session/tab IDs not URI-encoded in constructed deep link URL
sessionId and tabId are inserted into the URL string without calling encodeURIComponent, but parseDeepLink calls decodeURIComponent on the extracted segments. If a session or tab ID contains / (or ?, #, %, etc.), the URL will be split incorrectly and the wrong ID will be decoded.
For example, if sessionId = "abc/def", the constructed URL maestro://session/abc/def would parse as resource='session', id='abc', sub='def' — returning sessionId: 'abc' instead of 'abc/def'. The PR's own test for tab/slash documents this exact risk (it expects the slash to survive a round-trip), but the encoder side was not wired up in the notification handler.
| const deepLinkUrl = tabId | |
| ? `maestro://session/${sessionId}/tab/${tabId}` | |
| : `maestro://session/${sessionId}`; | |
| const deepLinkUrl = tabId | |
| ? `maestro://session/${encodeURIComponent(sessionId)}/tab/${encodeURIComponent(tabId)}` | |
| : `maestro://session/${encodeURIComponent(sessionId)}`; |
| const gotSingleInstanceLock = setupDeepLinkHandling(() => mainWindow); | ||
| if (!gotSingleInstanceLock) { | ||
| app.quit(); | ||
| } |
There was a problem hiding this comment.
No guard after app.quit() — module-level code continues executing
When setupDeepLinkHandling returns false (another instance already holds the lock), app.quit() is called but execution continues. The entire app.whenReady().then(...) block below is still registered, IPC handlers are set up, and store initialisation runs — all for an instance that is actively quitting. While Electron's app.quit() does prevent whenReady() from resolving, this creates unnecessary work and makes the intent less clear.
Consider adding a process.exit(0) or an early return equivalent (e.g., wrapping the rest of module-level setup in an if (gotSingleInstanceLock) guard) so the secondary-instance fast-exit path is explicit and side-effect-free.
| const gotSingleInstanceLock = setupDeepLinkHandling(() => mainWindow); | |
| if (!gotSingleInstanceLock) { | |
| app.quit(); | |
| } | |
| const gotSingleInstanceLock = setupDeepLinkHandling(() => mainWindow); | |
| if (!gotSingleInstanceLock) { | |
| app.quit(); | |
| process.exit(0); | |
| } |
src/renderer/App.tsx
Outdated
| useEffect(() => { | ||
| const unsubscribe = window.maestro.app.onDeepLink((deepLink) => { | ||
| if (deepLink.action === 'focus') { | ||
| // Window already brought to foreground by main process | ||
| return; | ||
| } | ||
| if (deepLink.action === 'session' && deepLink.sessionId) { | ||
| handleToastSessionClick(deepLink.sessionId, deepLink.tabId); | ||
| return; | ||
| } | ||
| if (deepLink.action === 'group' && deepLink.groupId) { | ||
| // Find first session in group and navigate to it | ||
| const groupSession = sessions.find((s) => s.groupId === deepLink.groupId); | ||
| if (groupSession) { | ||
| handleToastSessionClick(groupSession.id); | ||
| } | ||
| // Expand the group if it's collapsed | ||
| setGroups((prev) => | ||
| prev.map((g) => | ||
| g.id === deepLink.groupId ? { ...g, collapsed: false } : g | ||
| ) | ||
| ); | ||
| } | ||
| }); | ||
| return unsubscribe; | ||
| }, [handleToastSessionClick, sessions, setGroups]); |
There was a problem hiding this comment.
sessions in dependency array causes excessive listener churn
sessions is included in the useEffect dependency array, so the app:deepLink IPC listener is torn down and re-registered on every render that changes the sessions list (e.g., each time an agent updates). The cleanup and re-registration are cheap, but the callback also captures a stale sessions snapshot at registration time — meaning a deep link fired between a render and the re-subscription would see the previous snapshot.
A common fix is to store the latest value in a useRef so the listener is registered only once but always reads fresh data:
const sessionsRef = useRef(sessions);
useEffect(() => { sessionsRef.current = sessions; }, [sessions]);
useEffect(() => {
const unsubscribe = window.maestro.app.onDeepLink((deepLink) => {
// use sessionsRef.current instead of sessions
...
});
return unsubscribe;
}, [handleToastSessionClick, setGroups]); // sessions removed
src/renderer/global.d.ts
Outdated
| confirmQuit: () => void; | ||
| cancelQuit: () => void; | ||
| onSystemResume: (callback: () => void) => () => void; | ||
| onDeepLink: (callback: (deepLink: { action: 'focus' | 'session' | 'group'; sessionId?: string; tabId?: string; groupId?: string }) => void) => () => void; |
There was a problem hiding this comment.
Inline type duplicates ParsedDeepLink instead of reusing the shared type
The callback parameter type is written out as a literal object { action: 'focus' | 'session' | 'group'; sessionId?: string; tabId?: string; groupId?: string } which is structurally identical to ParsedDeepLink in src/shared/types.ts. The preload module already re-exports ParsedDeepLink from src/main/preload/index.ts, and the system preload (src/main/preload/system.ts) imports and uses it directly. Duplicating the shape here means that if the interface evolves (e.g., a new action is added), this declaration can drift out of sync silently.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/__tests__/main/deep-links.test.ts (1)
131-139: Add a malformed URI-encoding test for parser hardening.Deep links are external input; a bad
%-encoding should be handled gracefully (returnnull, not throw).🧪 Suggested test addition
describe('error handling', () => { it('should return null for unrecognized resource', () => { expect(parseDeepLink('maestro://unknown/abc')).toBeNull(); }); it('should return null for completely malformed URLs', () => { // parseDeepLink is tolerant of most inputs, but unrecognized resources return null expect(parseDeepLink('maestro://settings')).toBeNull(); }); + + it('should return null for invalid URI encoding', () => { + expect(parseDeepLink('maestro://session/%E0%A4%A')).toBeNull(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/deep-links.test.ts` around lines 131 - 139, Add a new test in the same "error handling" describe block that calls parseDeepLink with a malformed percent-encoded URL (e.g. a stray '%' or invalid hex like '%ZZ') and asserts the result is null; reference parseDeepLink so the test ensures the parser returns null (not throws) for bad URI-encoding inputs and hardens handling of external deep-link input.src/shared/types.ts (1)
376-385: TightenParsedDeepLinkto a discriminated union for stronger type safety.Current optional fields allow invalid states at compile time. A union keeps action-specific payload requirements explicit.
♻️ Proposed type refinement
-export interface ParsedDeepLink { - /** The type of navigation action */ - action: 'focus' | 'session' | 'group'; - /** Maestro session ID (for action: 'session') */ - sessionId?: string; - /** Tab ID within the session (for action: 'session') */ - tabId?: string; - /** Group ID (for action: 'group') */ - groupId?: string; -} +export type ParsedDeepLink = + | { + /** Bring app to foreground */ + action: 'focus'; + } + | { + /** Navigate to a session (optionally a tab) */ + action: 'session'; + sessionId: string; + tabId?: string; + } + | { + /** Navigate to a group */ + action: 'group'; + groupId: string; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types.ts` around lines 376 - 385, ParsedDeepLink currently allows invalid combos because optional fields coexist; replace the interface with a discriminated union of three specific types: one for action: 'focus' (no extra properties), one for action: 'session' (require sessionId: string and optional tabId?: string), and one for action: 'group' (require groupId: string). Update the type declaration named ParsedDeepLink to be this union so TypeScript enforces action-specific payloads and adjust any code using ParsedDeepLink to narrow on the action before accessing sessionId/tabId/groupId.src/__tests__/main/ipc/handlers/notifications.test.ts (1)
205-233: Good test coverage for click handler registration.The tests verify:
- Click handler is registered when
sessionIdis provided- Click handler is registered when both
sessionIdandtabIdare provided- Click handler is NOT registered when
sessionIdis absent or undefinedConsider adding a test that invokes the registered click handler to verify it calls
dispatchDeepLinkwith the correct arguments. This would provide end-to-end coverage of the navigation flow.🧪 Optional: Add test for click handler invocation
it('should dispatch deep link when notification is clicked', async () => { const { dispatchDeepLink } = await import('../../../../main/deep-links'); const handler = handlers.get('notification:show')!; await handler({}, 'Title', 'Body', 'session-123', 'tab-456'); // Get the registered click handler const clickCall = mocks.mockNotificationOn.mock.calls.find( ([event]) => event === 'click' ); expect(clickCall).toBeDefined(); // Simulate click const clickHandler = clickCall![1]; clickHandler(); expect(dispatchDeepLink).toHaveBeenCalledWith( expect.objectContaining({ action: 'session', sessionId: 'session-123' }), expect.any(Function) ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/notifications.test.ts` around lines 205 - 233, Add a test that after calling handlers.get('notification:show') (the notification handler) you locate the registered click handler on mocks.mockNotificationOn, invoke that handler, and assert that the imported dispatchDeepLink (from main/deep-links) was called with the expected deep-link payload (e.g., action: 'session', sessionId: 'session-123') and a callback; use the same approach as other tests to call handler({}, 'Title','Body','session-123','tab-456'), find the mockNotificationOn call where event === 'click', invoke its second argument, and assert dispatchDeepLink was called with expect.objectContaining({ action: 'session', sessionId: 'session-123' }) and expect.any(Function).src/main/ipc/handlers/notifications.ts (1)
356-368: Consider centralizing deep link URL construction.The URL format (
maestro://session/{id}/tab/{id}) is hardcoded here and must stay in sync withparseDeepLinkindeep-links.ts. If the format changes, both locations need updating.Since
dispatchDeepLinkaccepts a pre-parsedParsedDeepLinkobject, you can construct the object directly instead of building a URL string and re-parsing it:♻️ Construct ParsedDeepLink directly instead of URL string
// Wire click handler for navigation if session context is provided if (sessionId && deps?.getMainWindow) { - const deepLinkUrl = tabId - ? `maestro://session/${sessionId}/tab/${tabId}` - : `maestro://session/${sessionId}`; - notification.on('click', () => { - const parsed = parseDeepLink(deepLinkUrl); - if (parsed) { - dispatchDeepLink(parsed, deps.getMainWindow); - } + const parsed: ParsedDeepLink = tabId + ? { action: 'session', sessionId, tabId } + : { action: 'session', sessionId }; + dispatchDeepLink(parsed, deps.getMainWindow); }); }This also requires adding the import:
-import { parseDeepLink, dispatchDeepLink } from '../../deep-links'; +import { dispatchDeepLink } from '../../deep-links'; +import type { ParsedDeepLink } from '../../../shared/types';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/notifications.ts` around lines 356 - 368, The click handler builds a hardcoded deep-link URL and reparses it, which duplicates format logic; instead, construct a ParsedDeepLink object directly and pass it to dispatchDeepLink. Modify the notification.on('click') handler in notifications.ts to import the ParsedDeepLink type from deep-links.ts, create an object like { type: 'session', sessionId: sessionId, tabId: tabId } (matching ParsedDeepLink shape used by parseDeepLink), and call dispatchDeepLink(parsedObject, deps.getMainWindow) — remove/deprecate the deepLinkUrl string and the parseDeepLink call so URL format lives only in deep-links.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/App.tsx`:
- Around line 1515-1517: The session deep-link handler calls
handleToastSessionClick(deepLink.sessionId, deepLink.tabId) without verifying
the session exists, which can leave the UI in an invalid "no matching active
session" state; update the deep-link handling in App.tsx to first look up the
target session (e.g., via the existing sessions array, getSessionById, or
sessionMap used elsewhere), and only call handleToastSessionClick when a
matching session is found; if not found, bail out or show a user-facing
toast/error instead of mutating active session state.
---
Nitpick comments:
In `@src/__tests__/main/deep-links.test.ts`:
- Around line 131-139: Add a new test in the same "error handling" describe
block that calls parseDeepLink with a malformed percent-encoded URL (e.g. a
stray '%' or invalid hex like '%ZZ') and asserts the result is null; reference
parseDeepLink so the test ensures the parser returns null (not throws) for bad
URI-encoding inputs and hardens handling of external deep-link input.
In `@src/__tests__/main/ipc/handlers/notifications.test.ts`:
- Around line 205-233: Add a test that after calling
handlers.get('notification:show') (the notification handler) you locate the
registered click handler on mocks.mockNotificationOn, invoke that handler, and
assert that the imported dispatchDeepLink (from main/deep-links) was called with
the expected deep-link payload (e.g., action: 'session', sessionId:
'session-123') and a callback; use the same approach as other tests to call
handler({}, 'Title','Body','session-123','tab-456'), find the mockNotificationOn
call where event === 'click', invoke its second argument, and assert
dispatchDeepLink was called with expect.objectContaining({ action: 'session',
sessionId: 'session-123' }) and expect.any(Function).
In `@src/main/ipc/handlers/notifications.ts`:
- Around line 356-368: The click handler builds a hardcoded deep-link URL and
reparses it, which duplicates format logic; instead, construct a ParsedDeepLink
object directly and pass it to dispatchDeepLink. Modify the
notification.on('click') handler in notifications.ts to import the
ParsedDeepLink type from deep-links.ts, create an object like { type: 'session',
sessionId: sessionId, tabId: tabId } (matching ParsedDeepLink shape used by
parseDeepLink), and call dispatchDeepLink(parsedObject, deps.getMainWindow) —
remove/deprecate the deepLinkUrl string and the parseDeepLink call so URL format
lives only in deep-links.ts.
In `@src/shared/types.ts`:
- Around line 376-385: ParsedDeepLink currently allows invalid combos because
optional fields coexist; replace the interface with a discriminated union of
three specific types: one for action: 'focus' (no extra properties), one for
action: 'session' (require sessionId: string and optional tabId?: string), and
one for action: 'group' (require groupId: string). Update the type declaration
named ParsedDeepLink to be this union so TypeScript enforces action-specific
payloads and adjust any code using ParsedDeepLink to narrow on the action before
accessing sessionId/tabId/groupId.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
package.jsonsrc/__tests__/main/deep-links.test.tssrc/__tests__/main/ipc/handlers/notifications.test.tssrc/main/deep-links.tssrc/main/index.tssrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/notifications.tssrc/main/preload/index.tssrc/main/preload/notifications.tssrc/main/preload/system.tssrc/renderer/App.tsxsrc/renderer/global.d.tssrc/renderer/stores/notificationStore.tssrc/shared/types.ts
- URI-encode sessionId/tabId when constructing deep link URLs in notification click handler to prevent malformed URLs with special chars - Add process.exit(0) after app.quit() so secondary instances exit immediately without running further module-level setup - Use useRef for sessions in deep link effect to avoid tearing down and re-registering the IPC listener on every sessions change - Guard against navigating to non-existent session IDs in deep link handler to prevent invalid UI state - Add cross-reference comment in global.d.ts linking to canonical ParsedDeepLink type (can't import in ambient declaration file) - Add test for URI-encoding round-trip in notification click handler
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/index.ts`:
- Around line 405-406: flushPendingDeepLink is invoked right after
createWindow() and may call webContents.send on mainWindow before the renderer's
onDeepLink listener in App.tsx is registered, causing missed cold-start links;
change the flush behavior to wait until the window has finished loading (listen
for mainWindow.webContents.once('did-finish-load') and then call
flushPendingDeepLink(() => mainWindow)), or alternatively implement a
renderer-initiated request (have App.tsx call an IPC like
'request-pending-deeplink' after registering onDeepLink and then respond from
the main process with the pending link), updating the code paths that reference
flushPendingDeepLink, mainWindow, createWindow, and the renderer onDeepLink
registration accordingly.
In `@src/renderer/App.tsx`:
- Around line 1507-1511: Remove the redundant local declaration and updater for
sessionsRef: delete the const sessionsRef = useRef(sessions) and the
accompanying useEffect that sets sessionsRef.current, and instead rely on the
existing sessionsRef defined earlier in the same component (the store-backed
getter from useSessionStore.getState().sessions). Ensure all later code still
reads from sessionsRef.current (no renaming) and that there are no other local
declarations shadowing the existing sessionsRef in the App component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ad3bfb0-1475-4596-913b-f53e8c4edf12
📒 Files selected for processing (5)
src/__tests__/main/ipc/handlers/notifications.test.tssrc/main/index.tssrc/main/ipc/handlers/notifications.tssrc/renderer/App.tsxsrc/renderer/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/main/ipc/handlers/notifications.test.ts
- src/renderer/global.d.ts
| // Flush any deep link URL that arrived before the window was ready (cold start) | ||
| flushPendingDeepLink(() => mainWindow); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the context around lines 405-406 in src/main/index.ts
cat -n src/main/index.ts | sed -n '395,415p'Repository: RunMaestro/Maestro
Length of output: 975
🏁 Script executed:
# Find and examine the flushPendingDeepLink and processDeepLink implementations
rg -n "flushPendingDeepLink|processDeepLink" src/main/ -A 8Repository: RunMaestro/Maestro
Length of output: 3802
🏁 Script executed:
# Check how the renderer registers the deep link listener
rg -n "onDeepLink" src/renderer/ -A 3 -B 2Repository: RunMaestro/Maestro
Length of output: 865
🏁 Script executed:
# Check the window-manager.ts for createWindow and loading timing
rg -n "createWindow|loadURL|loadFile|did-finish-load" src/main/ -A 3 -B 1Repository: RunMaestro/Maestro
Length of output: 9112
🏁 Script executed:
# Check for any IPC handler registration related to deep links
rg -n "ipcMain|handle.*[Dd]eep|on.*[Dd]eep" src/main/index.ts -A 5Repository: RunMaestro/Maestro
Length of output: 220
🏁 Script executed:
# Check if there's any did-finish-load handling or other sync mechanisms
rg -n "did-finish-load|webContents.on|once.*load" src/main/ -A 3Repository: RunMaestro/Maestro
Length of output: 2337
🏁 Script executed:
# Check the complete processDeepLink implementation to see if there's any queuing/retry
cat -n src/main/deep-links.ts | sed -n '84,110p'Repository: RunMaestro/Maestro
Length of output: 997
🏁 Script executed:
# Check if renderer has any fallback to request pending deep links
rg -n "getPendingDeepLink|request.*[Dd]eep" src/renderer/ -A 3Repository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Look for any await or promise handling around createWindow or flushPendingDeepLink
cat -n src/main/index.ts | sed -n '400,425p'Repository: RunMaestro/Maestro
Length of output: 1117
🏁 Script executed:
# Check if there's any 'ready-to-show' or similar event being used
rg -n "ready-to-show|did-finish-load|dom-ready" src/main/Repository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Verify what isWebContentsAvailable actually checks
rg -n "function isWebContentsAvailable|const isWebContentsAvailable" src/main/ -A 5Repository: RunMaestro/Maestro
Length of output: 415
🏁 Script executed:
# Check when the App.tsx useEffect with onDeepLink listener runs relative to page load
cat -n src/renderer/App.tsx | sed -n '1510,1530p'Repository: RunMaestro/Maestro
Length of output: 1070
🏁 Script executed:
# Check if there are any other mechanisms to handle the IPC before the listener is registered
rg -n "window.maestro.app" src/preload/ src/renderer/ | head -20Repository: RunMaestro/Maestro
Length of output: 712
🏁 Script executed:
# Find the preload script location and check how onDeepLink is set up
find src -name "preload*" -type fRepository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Check how the IPC listener bridge is set up between main and renderer
rg -n "onDeepLink|ipcRenderer.on\|ipcRenderer.handle" src/ -A 2 | head -40Repository: RunMaestro/Maestro
Length of output: 784
🏁 Script executed:
# Check the full preload setup to confirm when listeners are actually registered
cat -n src/main/preload/system.ts | sed -n '205,220p'Repository: RunMaestro/Maestro
Length of output: 775
🏁 Script executed:
# Verify that the IPC channel name matches between send and listen
rg -n "IPC_CHANNEL|app:deepLink|deep-link" src/main/deep-links.tsRepository: RunMaestro/Maestro
Length of output: 198
🏁 Script executed:
# Verify the complete preload initialization to understand when system.ts is loaded
cat -n src/main/preload/system.ts | head -30Repository: RunMaestro/Maestro
Length of output: 846
Potential race condition: deep link may be dispatched before renderer listener is registered.
flushPendingDeepLink is called immediately after createWindow(), but loadURL/loadFile is asynchronous. The renderer's onDeepLink listener is registered in App.tsx's useEffect (which runs after React hydration), not during preload. If the IPC message is sent via webContents.send() before the listener is attached, the cold-start deep link will be silently dropped.
Consider waiting for the window's did-finish-load event before flushing, or have the renderer request any pending deep link after its listener is registered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/index.ts` around lines 405 - 406, flushPendingDeepLink is invoked
right after createWindow() and may call webContents.send on mainWindow before
the renderer's onDeepLink listener in App.tsx is registered, causing missed
cold-start links; change the flush behavior to wait until the window has
finished loading (listen for mainWindow.webContents.once('did-finish-load') and
then call flushPendingDeepLink(() => mainWindow)), or alternatively implement a
renderer-initiated request (have App.tsx call an IPC like
'request-pending-deeplink' after registering onDeepLink and then respond from
the main process with the pending link), updating the code paths that reference
flushPendingDeepLink, mainWindow, createWindow, and the renderer onDeepLink
registration accordingly.
| // Keep a ref to sessions so the deep link listener doesn't churn on every sessions change | ||
| const sessionsRef = useRef(sessions); | ||
| useEffect(() => { | ||
| sessionsRef.current = sessions; | ||
| }, [sessions]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine both declarations of sessionsRef
rg -n 'const\s+sessionsRef' src/renderer/App.tsx -A 2 -B 2Repository: RunMaestro/Maestro
Length of output: 513
🏁 Script executed:
# Get line count to ensure we're looking at valid ranges
wc -l src/renderer/App.tsxRepository: RunMaestro/Maestro
Length of output: 87
🏁 Script executed:
# Read the context around Line 456
sed -n '450,465p' src/renderer/App.tsxRepository: RunMaestro/Maestro
Length of output: 462
🏁 Script executed:
# Read the context around Line 1508 and the surrounding deep-link code
sed -n '1500,1545p' src/renderer/App.tsxRepository: RunMaestro/Maestro
Length of output: 1619
Blocker: sessionsRef is redeclared in the same scope.
At Line 1508, const sessionsRef is redeclared even though sessionsRef already exists at Line 456 in the same component function. This is a TypeScript/JavaScript compile-time error (Cannot redeclare block-scoped variable 'sessionsRef').
The existing sessionsRef at Line 456 is a store-backed getter that always returns current sessions from useSessionStore.getState().sessions. The new useRef declaration at Line 1508-1511 is redundant; remove it and rely on the existing ref.
🛠️ Proposed fix
- // Keep a ref to sessions so the deep link listener doesn't churn on every sessions change
- const sessionsRef = useRef(sessions);
- useEffect(() => {
- sessionsRef.current = sessions;
- }, [sessions]);📝 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.
| // Keep a ref to sessions so the deep link listener doesn't churn on every sessions change | |
| const sessionsRef = useRef(sessions); | |
| useEffect(() => { | |
| sessionsRef.current = sessions; | |
| }, [sessions]); |
🧰 Tools
🪛 Biome (2.4.4)
[error] 1508-1508: Shouldn't redeclare 'sessionsRef'. Consider to delete it or rename it.
(lint/suspicious/noRedeclare)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/App.tsx` around lines 1507 - 1511, Remove the redundant local
declaration and updater for sessionsRef: delete the const sessionsRef =
useRef(sessions) and the accompanying useEffect that sets sessionsRef.current,
and instead rely on the existing sessionsRef defined earlier in the same
component (the store-backed getter from useSessionStore.getState().sessions).
Ensure all later code still reads from sessionsRef.current (no renaming) and
that there are no other local declarations shadowing the existing sessionsRef in
the App component.
- Add shared deep-link-urls.ts with buildSessionDeepLink(),
buildGroupDeepLink(), and buildFocusDeepLink() utilities
- Add {{AGENT_DEEP_LINK}}, {{TAB_DEEP_LINK}}, {{GROUP_DEEP_LINK}}
template variables available in system prompts, custom AI commands,
and Auto Run documents
- Wire activeTabId and groupId into TemplateContext at all call sites
(agentStore, useInputProcessing, useRemoteHandlers,
useDocumentProcessor, useMergeTransferHandlers, batch-processor)
- Refactor notifications.ts to use shared buildSessionDeepLink()
- Add sessionId/tabId to notifyToast callers where context is available
(merge, transfer, summarize, PR creation)
- Add docs/deep-links.md documentation page with URL format, usage
examples, template variables, and platform behavior
- Add 8 tests for URL builders, 6 tests for template variable
substitution including URI encoding
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/hooks/agent/useSummarizeAndContinue.ts (1)
346-351:⚠️ Potential issue | 🔴 CriticalUndefined
sessionIdat Line 350 breaks this path.
sessionIdis not declared in this scope, so this branch won’t type-check/build. Usesession.id(or hoistsourceSessionIdbefore this block).Suggested fix
if (!targetTab || !canSummarize(session.contextUsage, targetTab.logs)) { notifyToast({ type: 'warning', title: 'Cannot Compact', message: `Context too small. Need at least ${contextSummarizationService.getMinContextUsagePercent()}% usage, ~2k tokens, or 8+ messages to compact.`, - sessionId, + sessionId: session.id, tabId: targetTabId, }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useSummarizeAndContinue.ts` around lines 346 - 351, The notifyToast call in useSummarizeAndContinue.ts passes an undefined sessionId variable causing a build error; update that call to use the correct session identifier (e.g., session.id) or hoist and use the existing sourceSessionId before this branch so notifyToast receives a defined session id; specifically change the sessionId argument in the notifyToast invocation (near notifyToast(...) and contextSummarizationService.getMinContextUsagePercent()) to session.id or sourceSessionId as appropriate.
♻️ Duplicate comments (1)
src/renderer/App.tsx (1)
1507-1511:⚠️ Potential issue | 🔴 CriticalBlocker: Remove the redundant
sessionsRefdeclaration.At Line 1508,
sessionsRefis redeclared even though it already exists at Line 456 as a store-backed getter. This causes a TypeScript compile-time error:Cannot redeclare block-scoped variable 'sessionsRef'.The existing
sessionsRefat Line 456 already provides current sessions viauseSessionStore.getState().sessions, so the newuseRefand accompanyinguseEffectare unnecessary.🛠️ Proposed fix
- // Keep a ref to sessions so the deep link listener doesn't churn on every sessions change - const sessionsRef = useRef(sessions); - useEffect(() => { - sessionsRef.current = sessions; - }, [sessions]); - // Deep link navigation handler — processes maestro:// URLs from OS notifications,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 1507 - 1511, Remove the redundant local sessionsRef declaration and its accompanying useEffect: delete the useRef(sessions) and the effect that sets sessionsRef.current, and instead rely on the existing sessionsRef provided earlier (the store-backed getter via useSessionStore.getState().sessions); update any local references to sessionsRef to use that existing sessionsRef (or directly use useSessionStore.getState().sessions) so there is no redeclaration of the block-scoped variable sessionsRef.
🧹 Nitpick comments (2)
docs/deep-links.md (1)
13-15: Add a language specifier to the fenced code block.The URL format code block lacks a language identifier. While this is a generic format example, adding a specifier like
textorplaintextsatisfies linters and improves accessibility for screen readers.-``` +```text maestro://[action]/[parameters]<details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/deep-links.mdaround lines 13 - 15, Update the fenced code block in the
deep-links.md snippet to include a language specifier (e.g., change the opening
totext or ```plaintext) so linters and screen readers recognize it;
locate the code block showing "maestro://[action]/[parameters]" and add the
chosen specifier to the opening fence.</details> </blockquote></details> <details> <summary>src/main/ipc/handlers/notifications.ts (1)</summary><blockquote> `344-345`: **Make handler deps required to avoid silent click-navigation regressions.** `registerNotificationsHandlers(deps?)` allows accidental registration without `getMainWindow`, which silently disables notification click routing. Since both current call sites already pass deps, making this required would harden the API. <details> <summary>♻️ Suggested API hardening diff</summary> ```diff export interface NotificationsHandlerDependencies { getMainWindow: () => BrowserWindow | null; } -export function registerNotificationsHandlers(deps?: NotificationsHandlerDependencies): void { +export function registerNotificationsHandlers(deps: NotificationsHandlerDependencies): void { // Show OS notification (with optional click-to-navigate support) ipcMain.handle( 'notification:show', @@ - if (sessionId && deps?.getMainWindow) { + if (sessionId) { const deepLinkUrl = buildSessionDeepLink(sessionId, tabId); notification.on('click', () => { const parsed = parseDeepLink(deepLinkUrl); if (parsed) { dispatchDeepLink(parsed, deps.getMainWindow); } }); } ``` </details> Based on learnings: Verify IPC handlers are registered in src/main/index.ts before modifying caller code when debugging IPC issues. Also applies to: 364-365 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/notifications.ts` around lines 344 - 345, Change the optional dependency parameter on registerNotificationsHandlers from optional to required to prevent accidental omission of getMainWindow: update the function signature to registerNotificationsHandlers(deps: NotificationsHandlerDependencies): void (and any related overloads/types) and adjust callers to pass the existing deps objects (ensure they provide getMainWindow). Locate usages of registerNotificationsHandlers and confirm each callsite supplies a valid NotificationsHandlerDependencies instance so click-to-navigate behavior cannot be silently disabled. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In@src/renderer/hooks/agent/useSummarizeAndContinue.ts:
- Around line 346-351: The notifyToast call in useSummarizeAndContinue.ts passes
an undefined sessionId variable causing a build error; update that call to use
the correct session identifier (e.g., session.id) or hoist and use the existing
sourceSessionId before this branch so notifyToast receives a defined session id;
specifically change the sessionId argument in the notifyToast invocation (near
notifyToast(...) and contextSummarizationService.getMinContextUsagePercent()) to
session.id or sourceSessionId as appropriate.
Duplicate comments:
In@src/renderer/App.tsx:
- Around line 1507-1511: Remove the redundant local sessionsRef declaration and
its accompanying useEffect: delete the useRef(sessions) and the effect that sets
sessionsRef.current, and instead rely on the existing sessionsRef provided
earlier (the store-backed getter via useSessionStore.getState().sessions);
update any local references to sessionsRef to use that existing sessionsRef (or
directly use useSessionStore.getState().sessions) so there is no redeclaration
of the block-scoped variable sessionsRef.
Nitpick comments:
In@docs/deep-links.md:
- Around line 13-15: Update the fenced code block in the deep-links.md snippet
to include a language specifier (e.g., change the openingtotext orshowing "maestro://[action]/[parameters]" and add the chosen specifier to the opening fence. In `@src/main/ipc/handlers/notifications.ts`: - Around line 344-345: Change the optional dependency parameter on registerNotificationsHandlers from optional to required to prevent accidental omission of getMainWindow: update the function signature to registerNotificationsHandlers(deps: NotificationsHandlerDependencies): void (and any related overloads/types) and adjust callers to pass the existing deps objects (ensure they provide getMainWindow). Locate usages of registerNotificationsHandlers and confirm each callsite supplies a valid NotificationsHandlerDependencies instance so click-to-navigate behavior cannot be silently disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID:
1c1348f2-0c75-40b3-88be-c44f42923217📒 Files selected for processing (15)
docs/deep-links.mddocs/docs.jsonsrc/__tests__/shared/deep-link-urls.test.tssrc/__tests__/shared/templateVariables.test.tssrc/cli/services/batch-processor.tssrc/main/ipc/handlers/notifications.tssrc/renderer/App.tsxsrc/renderer/hooks/agent/useMergeTransferHandlers.tssrc/renderer/hooks/agent/useSummarizeAndContinue.tssrc/renderer/hooks/batch/useDocumentProcessor.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/hooks/remote/useRemoteHandlers.tssrc/renderer/stores/agentStore.tssrc/shared/deep-link-urls.tssrc/shared/templateVariables.ts
Adds a "Copy Deep Link" button to the tab context menu that copies
a maestro://session/{id}/tab/{tabId} URL to the clipboard. Reuses
the existing buildSessionDeepLink shared utility.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/TabBar.tsx`:
- Around line 718-727: The deep-link "Copy Deep Link" lives only in a hover
overlay so keyboard users cannot access it; update the TabBar tab trigger
element (the element that checks sessionId and renders the overlay) to include
tabIndex={0} and add onFocus and onBlur handlers that mirror the existing mouse
enter/leave behavior to show/hide the overlay, so the button that calls
handleCopyDeepLink and uses buildSessionDeepLink and showCopied becomes
reachable via keyboard; ensure the focus handlers call the same state toggles
used by the hover logic and preserve ARIA attributes for the overlay for
accessibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 712f90f0-0994-4d7e-bb2f-53089f68d148
📒 Files selected for processing (3)
src/__tests__/renderer/components/TabBar.test.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/TabBar.tsx
| {sessionId && ( | ||
| <button | ||
| onClick={handleCopyDeepLink} | ||
| className="w-full flex items-center gap-2 px-2 py-1.5 rounded text-xs hover:bg-white/10 transition-colors" | ||
| style={{ color: theme.colors.textMain }} | ||
| title={buildSessionDeepLink(sessionId, tabId)} | ||
| > | ||
| <Link className="w-3.5 h-3.5" style={{ color: theme.colors.textDim }} /> | ||
| {showCopied === 'deepLink' ? 'Copied!' : 'Copy Deep Link'} | ||
| </button> |
There was a problem hiding this comment.
Make the new deep-link action reachable without a mouse.
Copy Deep Link only exists inside the hover overlay, but the tab trigger still has no tabIndex or focus/blur handling. Keyboard users cannot open this menu or use the new copy flow.
As per coding guidelines, "src/renderer/components/**/*.{ts,tsx}: Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus".
🤖 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 718 - 727, The deep-link
"Copy Deep Link" lives only in a hover overlay so keyboard users cannot access
it; update the TabBar tab trigger element (the element that checks sessionId and
renders the overlay) to include tabIndex={0} and add onFocus and onBlur handlers
that mirror the existing mouse enter/leave behavior to show/hide the overlay, so
the button that calls handleCopyDeepLink and uses buildSessionDeepLink and
showCopied becomes reachable via keyboard; ensure the focus handlers call the
same state toggles used by the hover logic and preserve ARIA attributes for the
overlay for accessibility.
In dev mode, setAsDefaultProtocolClient without extra args registers the stock Electron binary, causing the Electron splash screen to open instead of Maestro. Pass process.execPath and the resolved app entry point so macOS launches the correct application.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/deep-links.ts`:
- Around line 95-114: flushPendingDeepLink currently sends pendingDeepLinkUrl as
soon as a BrowserWindow exists and clears it, which can drop cold-start links
before the renderer's onDeepLink handler is installed; change the logic so
flushPendingDeepLink (and any code that sets/clears pendingDeepLinkUrl) does not
clear the pendingDeepLinkUrl until the renderer explicitly acknowledges receipt.
Implement a small handshake using the existing IPC channel (IPC_CHANNEL) — have
the renderer emit a readiness/ack message (e.g., "deep-link-ready" or
"deep-link-ack" from the onDeepLink installer in App.tsx) once it registers its
handler, and have the main process (where flushPendingDeepLink, getMainWindow
and pendingDeepLinkUrl live) wait for that ack before sending parsed via
win.webContents.send and then clear pendingDeepLinkUrl; also ensure the main
still restores/shows/focuses the window but retains the pending link across
retries until ack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d340847f-74e2-4d45-8625-c43437f2ed3c
📒 Files selected for processing (1)
src/main/deep-links.ts
| const win = getMainWindow(); | ||
| if (!win) { | ||
| // Window not ready yet — buffer for later | ||
| pendingDeepLinkUrl = url; | ||
| logger.debug('Window not ready, buffering deep link', 'DeepLink'); | ||
| return; | ||
| } | ||
|
|
||
| // Bring window to foreground | ||
| if (win.isMinimized()) win.restore(); | ||
| win.show(); | ||
| win.focus(); | ||
|
|
||
| // For 'focus' action, bringing window to front is all we need | ||
| if (parsed.action === 'focus') return; | ||
|
|
||
| // Send parsed payload to renderer for navigation | ||
| if (isWebContentsAvailable(win)) { | ||
| win.webContents.send(IPC_CHANNEL, parsed); | ||
| } |
There was a problem hiding this comment.
Cold-start deep links can be dropped before the renderer subscribes.
flushPendingDeepLink() is called right after createWindow(), but the renderer only installs onDeepLink later in src/renderer/App.tsx:1515-1545. Here, the pending URL is cleared and sent as soon as a BrowserWindow exists, with no retry if the renderer/preload side is not ready yet. That makes startup maestro://... opens foreground the app and then do nothing. Please keep the link queued until the renderer explicitly signals readiness (or acknowledges receipt) before clearing it.
Also applies to: 202-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/deep-links.ts` around lines 95 - 114, flushPendingDeepLink currently
sends pendingDeepLinkUrl as soon as a BrowserWindow exists and clears it, which
can drop cold-start links before the renderer's onDeepLink handler is installed;
change the logic so flushPendingDeepLink (and any code that sets/clears
pendingDeepLinkUrl) does not clear the pendingDeepLinkUrl until the renderer
explicitly acknowledges receipt. Implement a small handshake using the existing
IPC channel (IPC_CHANNEL) — have the renderer emit a readiness/ack message
(e.g., "deep-link-ready" or "deep-link-ack" from the onDeepLink installer in
App.tsx) once it registers its handler, and have the main process (where
flushPendingDeepLink, getMainWindow and pendingDeepLinkUrl live) wait for that
ack before sending parsed via win.webContents.send and then clear
pendingDeepLinkUrl; also ensure the main still restores/shows/focuses the window
but retains the pending link across retries until ack.
1 participant
Summary
maestro://deep link URL scheme for navigating to agents, tabs, and groups from external apps and CLI (open maestro://session/{id}/tab/{id},maestro://group/{id},maestro://focus)open-url, Windows/Linuxsecond-instance, cold-startprocess.argv)Changes
src/main/deep-links.ts— core module with URL parsing, protocol registration, lifecycle hooks, and dispatchsrc/__tests__/main/deep-links.test.ts— 14 unit tests for URL parsingsrc/main/index.ts— wire lifecycle hooks before/afterapp.whenReady()src/main/ipc/handlers/notifications.ts— notification click → deep link dispatchsrc/main/preload/system.ts—onDeepLinklistener APIsrc/main/preload/notifications.ts— threadsessionId/tabIdparamssrc/renderer/App.tsx— deep link navigation effect routing to existing handlerssrc/renderer/stores/notificationStore.ts— pass session context to OS notificationssrc/shared/types.ts—ParsedDeepLinkinterfacepackage.json— electron-builderprotocolsconfigTest plan
npm run devwithREGISTER_DEEP_LINKS_IN_DEV=1, verifyopen maestro://focusbrings app to foregroundopen maestro://session/{id}andopen maestro://group/{id}from terminalnpm run test -- deep-linksandnpm run test -- notifications— all tests passSummary by CodeRabbit
New Features
Documentation
Tests