feat(web+macos): wire deep-link consumers — composer pre-fill, thread nav, window activation (LUM-2068)#32658
Conversation
… nav, window activation (LUM-2068)
Closes the consumer loop on the deep-link bridge from LUM-1872. The
bus events fired by `use-event-bus-init` now have a chat-domain
subscriber that takes user-visible action, plus a new
`runtime/main-window.ts` bridge so any consumer can bring the
window forward.
## What
### Main-window bridge (new imperative surface)
- `apps/macos/src/main/main-window.ts` — `installMainWindow` now
registers `ipcMain.handle("vellum:mainWindow:ensureVisible", ...)`
that routes through the existing `ensureVisible()` (recreate if
destroyed, restore from minimize, show, focus, await renderer-
ready). Test seam `__resetForTesting` added so tests can
re-exercise the install path without re-mocking the entire module.
- `apps/macos/src/main/preload/index.ts` — `window.vellum.mainWindow.ensureVisible()`.
- `apps/web/src/runtime/is-electron.ts` — ambient mirror.
- `apps/web/src/runtime/main-window.ts` — no-op-off-Electron
wrapper; web has its own foregrounding (it IS the foreground tab)
and Capacitor handles app activation natively.
This is the third imperative wrapper after `dock.ts` and
`app-info.ts`; same shape, same convention.
### Consumer hook in the chat domain
- `apps/web/src/domains/chat/hooks/use-deep-link-consumer.ts` —
subscribes to `deeplink.send`, `deeplink.openThread`,
`deeplink.unknown` and:
* `deeplink.send` → `ensureMainWindowVisible()`; if composer is
empty (`.trim().length === 0`) set the input to the message;
if non-empty, Sentry breadcrumb + drop. The conservative
drop-don't-overwrite call is documented inline — we can
revisit with a "queue or non-destructive prompt" UX once we
know how often it happens in practice.
* `deeplink.openThread` → `ensureMainWindowVisible()` +
`navigate(routes.conversation(threadId))`.
* `deeplink.unknown` → Sentry breadcrumb with the URL, no
other action.
Uses refs to mirror the dynamic props (composerInput,
setComposerInput, navigate) so the bus subscription is mounted
once — without this, every keystroke would tear down + resubscribe
the handlers, opening a race window where a deep link landing
between unsubscribe and resubscribe would be dropped.
### Mounted from ChatPage
- `apps/web/src/domains/chat/chat-page.tsx` — calls
`useDeepLinkConsumer({ composerInput: input, setComposerInput: setInput })`
next to the existing `useDraftInput`.
## Tests
- `apps/macos/src/main/main-window.test.ts` (+2 cases) — IPC handler
registered, IPC handler routes through `ensureVisible`.
- `apps/web/src/domains/chat/hooks/use-deep-link-consumer.test.tsx`
(7 cases) — `deeplink.send` empty-composer pre-fill,
in-progress-typing preserved + Sentry breadcrumb, whitespace-only
counts as empty; `deeplink.openThread` navigates + ensures window;
`deeplink.unknown` Sentry breadcrumb only;
re-render-doesn't-resubscribe (the dep-array-via-refs choice);
unmount unsubscribes.
26/26 macOS tests + 26/26 chat+bus tests green.
## Out of scope
- The "queue or non-destructive prompt" UX for in-progress typing.
Sentry breadcrumb now; revisit with data.
- Accessory-mode → regular transition before showing the window.
The main-window helper's `ensureVisible` already handles the
show/focus sequence; the dock policy refresh fires off the
window's `show` event via `onMainWindowVisibilityChange`, so
the transition happens implicitly. Worth a follow-up audit if
the dock policy lags the show.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbcb122956
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // and ensure the main window is visible first. The hook gates the | ||
| // composer pre-fill on `input` being empty so it doesn't clobber | ||
| // in-progress typing. Off Electron the bus events never fire. | ||
| useDeepLinkConsumer({ composerInput: input, setComposerInput: setInput }); |
There was a problem hiding this comment.
When the current route is anything other than ChatPage (for example /assistant/settings, /assistant/logs, /assistant/home, or the inspector routes in routes.tsx), this is the only subscription to deeplink.send / deeplink.openThread, so no handler is mounted. useEventBusInit still drains/publishes those deep-link events at RootLayout scope, and the event bus drops publishes with no subscribers, so clicking vellum://thread/... or vellum://send?... while the user is on one of those routes consumes the link without navigating or pre-filling the composer. Mount the consumer (or at least the navigation/pending-send part) at a route level that is always present for authenticated assistant pages.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| * All three handlers fire `ensureMainWindowVisible()` first so | ||
| * the action lands on a user-visible window. Off Electron the | ||
| * wrapper no-ops. |
There was a problem hiding this comment.
🟡 Docstring incorrectly states all three handlers call ensureMainWindowVisible
The docstring at lines 24-26 states "All three handlers fire ensureMainWindowVisible() first" but the deeplink.unknown handler (lines 87-94) does NOT call ensureMainWindowVisible(). The test at apps/web/src/domains/chat/hooks/use-deep-link-consumer.test.tsx:131 explicitly asserts expect(ensureMainWindowVisibleMock).not.toHaveBeenCalled(), confirming this is the intended behavior. The code and tests are internally consistent — only the docstring is wrong, which could mislead future maintainers into thinking deeplink.unknown should activate the window.
| * All three handlers fire `ensureMainWindowVisible()` first so | |
| * the action lands on a user-visible window. Off Electron the | |
| * wrapper no-ops. | |
| * The `send` and `openThread` handlers fire `ensureMainWindowVisible()` | |
| * first so the action lands on a user-visible window. Off Electron the | |
| * wrapper no-ops. `unknown` only logs — no window activation. | |
Was this helpful? React with 👍 or 👎 to provide feedback.
…bound links
Codex P1 — the prior consumer mounted only on ChatPage, so deep links
arriving while the user was on /assistant/settings, /logs, /home,
/inspect, etc. dropped silently. useEventBusInit drained pending links
at RootLayout scope and published them on the bus, but no subscriber
existed → bus drops them. Real user impact: vellum://thread/<id>
clicks did nothing on every non-chat route.
## Architectural split
Two hooks, two stores, one narrow-waist hand-off.
`apps/web/src/hooks/use-global-deep-link-consumer.ts` (new) —
mounted at RootLayout, alive on every authenticated assistant route.
Subscribes to the bus, handles navigation + window + unknown:
- deeplink.openThread → ensureMainWindowVisible() +
navigate(routes.conversation(threadId))
- deeplink.send → ensureMainWindowVisible() + navigate(routes.assistant)
+ park the message in usePendingDeepLinkStore
- deeplink.unknown → Sentry breadcrumb
`apps/web/src/stores/pending-deep-link-store.ts` (new) — Zustand
one-shot inbox. The global consumer writes; the chat-domain consumer
reads-and-clears on mount. Latest message wins if a second link
arrives before consumption. Not persisted — deep links are transient
signals.
`apps/web/src/domains/chat/hooks/use-deep-link-consumer.ts`
(refactored) — no longer subscribes to the bus directly. Reads
pendingComposerMessage via the atomic selector, consumes on the
effect, applies empty-composer-only policy. Refs gone since the
deps are simple now.
## Why this shape
The composer pre-fill is chat-domain-specific (owns setInput); it can
ONLY mount when ChatPage mounts. The navigation, window, and unknown
handlers are route-independent and MUST be always-on. The Zustand
store is the narrow-waist hand-off: global writes, chat reads-once.
Alternative considered: mount the whole consumer at RootLayout and
push setInput up via outlet context / Zustand. Rejected because
the composer state is intentionally local to useDraftInput (its
draft persistence + conversation-switch lifecycle). The hand-off
store is a smaller surface than lifting the composer state up.
## Tests
`use-global-deep-link-consumer.test.tsx` (new, 4 cases): send
navigates + parks the message + ensures window; openThread navigates
+ ensures window; unknown is breadcrumb-only; unmount unsubscribes.
`use-deep-link-consumer.test.tsx` (rewritten, 5 cases): empty-composer
pre-fill when a message is pending; non-empty composer preserves
typing + breadcrumb + clears the store anyway (so it doesn't resurface
on next render); whitespace-only counts as empty; no-op when nothing
pending; LATE arrival fires on the Zustand re-render.
All 9 tests green.
## Related to Devin BUG_0001
Devin flagged a docstring inconsistency on the previous commit
(dbcb122) — the old `useDeepLinkConsumer` claimed "all three
handlers fire ensureMainWindowVisible" but `deeplink.unknown` did
not. Moot post-refactor: the chat-domain hook no longer handles bus
events at all, and the global consumer's docstring lists each
handler's behavior precisely.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
Both findings addressed in
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f549e899
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const unsubSend = bus.subscribe("deeplink.send", ({ message }) => { | ||
| void ensureMainWindowVisible(); | ||
| usePendingDeepLinkStore.getState().setPendingComposerMessage(message); | ||
| navigateRef.current(routes.assistant); |
There was a problem hiding this comment.
When the macOS app is still running after the main window was closed, this renderer-side subscriber never exists, so ensureMainWindowVisible() is never called for a vellum://send or vellum://thread click. I checked the macOS paths: window-all-closed keeps the app alive on Darwin, open-url only calls handleDeepLink, and handleDeepLink just buffers when there are zero subscribers, so the buffered link is never drained because no window is recreated. The activation needs to happen in the main-process deep-link path (or another path that runs without a renderer), not only from this bus consumer.
Useful? React with 👍 / 👎.
Codex P2 — the renderer-side `ensureMainWindowVisible()` doesn't fire
when no renderer exists, which is exactly the macOS no-window state
(`window-all-closed` doesn't quit on Darwin; user closes the main
window, app keeps running in the tray / accessory mode).
Trace of the bug:
1. User closes main window. App alive, no renderer.
2. User clicks vellum://thread/abc.
3. `open-url` → `handleDeepLink` → parses, buffers (subscriberCount
=== 0), broadcasts (no windows to receive).
4. Window never recreates. Link sits in buffer indefinitely.
5. Renderer-side activation depended on a subscriber that doesn't
exist.
Fix: `handleDeepLink` calls `ensureVisible()` from main-window.ts
for actionable kinds — independent of whether a renderer is alive.
The renderer then mounts, drains the buffered link, and acts on it.
The renderer-side `ensureMainWindowVisible()` call stays as
defense-in-depth (no-op when already visible).
`unknown` kinds explicitly skip activation — we don't want a
foreign-scheme URL (`javascript:`, `data:`, etc.) routed by the OS
to give an attacker a UI side effect.
Tests added (4 new cases):
- send activates the window
- openThread activates the window
- unknown / foreign schemes / malformed URLs do NOT activate
- buffer-then-activate sequence: link is parked AND window
recreates so the renderer-on-mount drain delivers it (the
bug's primary fix path)
30/30 deep-links tests green.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
|
Codex P2 (renderer-only activation doesn't help when no renderer exists) addressed in d84b5ef. The macOS path Codex traced is exact: app alive after main window closes ( Fix:
4 new tests pin the behavior:
30/30 deep-links tests green. Devin BUG_0001 docstring finding remains moot post- Generated by Claude Code |
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: deep-link consumers — Vex APPROVE
Reviewed at HEAD d84b5ef9aba71aa99407e5b3adf63e94fa8b49ab. CI 11/11 green. Both Codex passes substantively closed across the 3 commits.
Architecture verification
The split into useGlobalDeepLinkConsumer (RootLayout) + useDeepLinkConsumer (chat domain) + pending-deep-link-store (one-shot Zustand inbox) is the right shape — Codex P1's "mount the consumer somewhere always-present" gets the structural fix rather than the easy patch. The store is the narrow-waist hand-off so the global consumer doesn't have to know about setInput and the chat-domain consumer doesn't have to know about routing or window activation.
Findings closed by construction in the same architecture move:
- Codex P1 (initial,
dbcb1229) — consumer only mounted onChatPage, deep links on/assistant/settings//logs/etc dropped. Closed at83f549e8by lifting nav + window activation toRootLayout. - Devin nit (initial,
dbcb1229) — docstring claimed all three handlers fireensureMainWindowVisible. Closed implicitly because the new chat-domain consumer no longer subscribes to the bus directly — its docstring is accurate to its smaller scope. - Codex P2 (
83f549e8) — renderer-side activation can't help when no renderer exists (Darwin keeps the app alive afterwindow-all-closed). Closed atd84b5ef9by moving the activation intohandleDeepLinkin main, gated onlink.kind !== "unknown"so foreign schemes can't produce a UI side effect.
Verified at HEAD
handleDeepLinkactually activates the window for actionable kinds ✓ —deep-links.ts:140-148. 4 new tests indeep-links.test.ts:357-400pin:sendactivates,openThreadactivates,unknown/javascript:/file:/malformed do NOT activate, and the buffer-then-activate sequence (the actual fix path: link parked AND window recreated).- Subscribe-then-drain ordering preserved ✓ —
useEventBusInit's drain is async (IPCinvoke), so React's synchronous effect ordering guaranteesuseGlobalDeepLinkConsumer's subscribe runs before any drain payload publishes to the bus. ThesubscriberCountmodel from #32656 still gates correctly: zero → buffer, ≥1 → broadcast-only. useDeepLinkConsumerdoesn't race withuseDraftInput✓ —useDraftInputinitializesinputsynchronously to""and only restores onisSwitch(prev key ≠ current key). On initial mountprevKey === null→ no restore, so an empty composer is the honest state when the pre-fill check runs. On a switch landing simultaneously with a pending message, the second render (input committed to "") re-firesuseDeepLinkConsumer's effect via thecomposerInputdep and the pre-fill lands.ensureVisibleis a no-op on already-visible window ✓ —main-window.ts:240-250short-circuits when not destroyed and not minimized. The renderer-sideensureMainWindowVisible()duplicating main's call when main triggered creation is harmless.
Non-blocking observations
1. pending-deep-link-store docstring vs code mismatch on overwrite
The docstring at pending-deep-link-store.ts:13-15 says:
If a second deep link arrives before consumption, the latest message wins (older drops with a Sentry breadcrumb).
But setPendingComposerMessage is a plain set({ pendingComposerMessage: message }) with no breadcrumb on overwrite. Either:
- Drop the "with a Sentry breadcrumb" clause from the docstring (the silent-overwrite policy is fine — "latest wins" is honest as-is), or
- Add a breadcrumb inside the setter when overwriting non-null, mirroring the empty-composer-drop breadcrumb in
useDeepLinkConsumer.
I'd take option 1 — the breadcrumb claim adds an obligation the code doesn't carry, and two-link-overwrite is probably below the noise floor.
2. deeplink.send mid-conversation navigates user away from current thread
useGlobalDeepLinkConsumer always navigate(routes.assistant) on a send. If the user is on /assistant/conversations/<id> actively typing a reply to that conversation, an external vellum://send?message=hi click yanks them to the root chat (their xyz draft is preserved by useDraftInput per-conversation, but they lose context).
Per the PR body this is intentionally deferred until telemetry — right call. Worth a note for the eventual UX revisit: "open in current conversation" could land the parked message in the active conversation's composer instead of forcing nav. Not blocking — current behavior matches the spec.
3. Main + renderer double-call to ensureMainWindowVisible on the cold path
When main triggers window creation (subscriberCount === 0, link buffered, ensureMainWindowVisible() from handleDeepLink), the new renderer mounts, drains the buffered link, publishes it, and useGlobalDeepLinkConsumer fires another ensureMainWindowVisible() from the renderer. The second call is a no-op, but the doubling is implicit.
A short comment at either site noting "main owns the cold path activation (window-all-closed on Darwin), renderer owns the hot path (minimized / behind another window); the duplicate is intentional defense in depth" would save the next reader the trace. Could go on deep-links.ts:148 or use-global-deep-link-consumer.ts:42.
4. Theoretical subscribe-race in preload (already self-flagged)
Already called out by the comment in deep-links.ts:118-122 — a link arriving between renderer's ipcRenderer.on("vellum:deepLinks:event", handler) and main processing vellum:deepLinks:subscribe could in principle be both buffered and broadcast (delivered twice on next drain). Sub-microsecond window, not realistically triggerable. Renderer-side dedup is the right fix if it ever surfaces; today the comment is the right level of acknowledgment.
Merge gate
- Codex's last pass is at
83f549e899(pre thed84b5ef9main-process activation fix). Devin's only review is atdbcb1229(very stale). - The 4 new activation tests pin the Codex P2 closure structurally and CI is 11/11 green at HEAD, so a fresh
@codex review+@devin-ai review this PRat HEAD would formalize closure rather than uncover anything new. Worth firing both before merge per second-approval criterion.
There was a problem hiding this comment.
Review: deep-link consumers — Vex APPROVE
Reviewed at HEAD d84b5ef9aba71aa99407e5b3adf63e94fa8b49ab. CI 11/11 green. Both Codex passes substantively closed across the 3 commits.
Architecture verification
The split into useGlobalDeepLinkConsumer (RootLayout) + useDeepLinkConsumer (chat domain) + pending-deep-link-store (one-shot Zustand inbox) is the right shape — Codex P1's "mount the consumer somewhere always-present" gets the structural fix rather than the easy patch. The store is the narrow-waist hand-off so the global consumer doesn't have to know about setInput and the chat-domain consumer doesn't have to know about routing or window activation.
Findings closed by construction in the same architecture move:
- Codex P1 (initial,
dbcb1229) — consumer only mounted onChatPage, deep links on/assistant/settings//logs/etc dropped. Closed at83f549e8by lifting nav + window activation toRootLayout. - Devin nit (initial,
dbcb1229) — docstring claimed all three handlers fireensureMainWindowVisible. Closed implicitly because the new chat-domain consumer no longer subscribes to the bus directly — its docstring is accurate to its smaller scope. - Codex P2 (
83f549e8) — renderer-side activation can't help when no renderer exists (Darwin keeps the app alive afterwindow-all-closed). Closed atd84b5ef9by moving the activation intohandleDeepLinkin main, gated onlink.kind !== "unknown"so foreign schemes can't produce a UI side effect.
Verified at HEAD
handleDeepLinkactually activates the window for actionable kinds ✓ —deep-links.ts:140-148. 4 new tests indeep-links.test.ts:357-400pin:sendactivates,openThreadactivates,unknown/javascript:/file:/malformed do NOT activate, and the buffer-then-activate sequence (the actual fix path: link parked AND window recreated).- Subscribe-then-drain ordering preserved ✓ —
useEventBusInit's drain is async (IPCinvoke), so React's synchronous effect ordering guaranteesuseGlobalDeepLinkConsumer's subscribe runs before any drain payload publishes to the bus. ThesubscriberCountmodel from #32656 still gates correctly: zero → buffer, ≥1 → broadcast-only. useDeepLinkConsumerdoesn't race withuseDraftInput✓ —useDraftInputinitializesinputsynchronously to""and only restores onisSwitch(prev key ≠ current key). On initial mountprevKey === null→ no restore, so an empty composer is the honest state when the pre-fill check runs. On a switch landing simultaneously with a pending message, the second render (input committed to "") re-firesuseDeepLinkConsumer's effect via thecomposerInputdep and the pre-fill lands.ensureVisibleis a no-op on already-visible window ✓ —main-window.ts:240-250short-circuits when not destroyed and not minimized. The renderer-sideensureMainWindowVisible()duplicating main's call when main triggered creation is harmless.
Non-blocking observations
1. pending-deep-link-store docstring vs code mismatch on overwrite
The docstring at pending-deep-link-store.ts:13-15 says:
If a second deep link arrives before consumption, the latest message wins (older drops with a Sentry breadcrumb).
But setPendingComposerMessage is a plain set({ pendingComposerMessage: message }) with no breadcrumb on overwrite. Either:
- Drop the "with a Sentry breadcrumb" clause from the docstring (the silent-overwrite policy is fine — "latest wins" is honest as-is), or
- Add a breadcrumb inside the setter when overwriting non-null, mirroring the empty-composer-drop breadcrumb in
useDeepLinkConsumer.
I'd take option 1 — the breadcrumb claim adds an obligation the code doesn't carry, and two-link-overwrite is probably below the noise floor.
2. deeplink.send mid-conversation navigates user away from current thread
useGlobalDeepLinkConsumer always navigate(routes.assistant) on a send. If the user is on /assistant/conversations/<id> actively typing a reply to that conversation, an external vellum://send?message=hi click yanks them to the root chat (their xyz draft is preserved by useDraftInput per-conversation, but they lose context).
Per the PR body this is intentionally deferred until telemetry — right call. Worth a note for the eventual UX revisit: "open in current conversation" could land the parked message in the active conversation's composer instead of forcing nav. Not blocking — current behavior matches the spec.
3. Main + renderer double-call to ensureMainWindowVisible on the cold path
When main triggers window creation (subscriberCount === 0, link buffered, ensureMainWindowVisible() from handleDeepLink), the new renderer mounts, drains the buffered link, publishes it, and useGlobalDeepLinkConsumer fires another ensureMainWindowVisible() from the renderer. The second call is a no-op, but the doubling is implicit.
A short comment at either site noting "main owns the cold path activation (window-all-closed on Darwin), renderer owns the hot path (minimized / behind another window); the duplicate is intentional defense in depth" would save the next reader the trace. Could go on deep-links.ts:148 or use-global-deep-link-consumer.ts:42.
4. Theoretical subscribe-race in preload (already self-flagged)
Already called out by the comment in deep-links.ts:118-122 — a link arriving between renderer's ipcRenderer.on("vellum:deepLinks:event", handler) and main processing vellum:deepLinks:subscribe could in principle be both buffered and broadcast (delivered twice on next drain). Sub-microsecond window, not realistically triggerable. Renderer-side dedup is the right fix if it ever surfaces; today the comment is the right level of acknowledgment.
Merge gate
- Codex's last pass is at
83f549e899(pre thed84b5ef9main-process activation fix). Devin's only review is atdbcb1229(very stale). - The 4 new activation tests pin the Codex P2 closure structurally and CI is 11/11 green at HEAD, so a fresh
@codex review+@devin-ai review this PRat HEAD would formalize closure rather than uncover anything new. Worth firing both before merge per second-approval criterion.
…h activation
Two non-blocking observations from review, both docs-only:
1. `pending-deep-link-store.ts` docstring claimed "older drops with a
Sentry breadcrumb" on overwrite — but `setPendingComposerMessage`
is a plain set with no breadcrumb. Drop the claim: silent overwrite
("latest wins") is honest and two-link-overwrite is below the noise
floor.
2. `deep-links.ts` `handleDeepLink` now has a paragraph explaining the
intentional duplicate activation between main (cold path —
no-renderer activation) and renderer (`useGlobalDeepLinkConsumer`'s
hot path — minimized / behind another window). The second call is
a no-op via `ensureVisible`'s short-circuit; the comment saves the
next reader the trace.
No behavior changes.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d84b5ef9ab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (link.kind !== "unknown") { | ||
| void ensureMainWindowVisible(); |
There was a problem hiding this comment.
When the app is launched from a vellum:// URL, this handler can run from the will-finish-launching/open-url path before app.whenReady() (the top-level install comment in index.ts explicitly relies on that timing). Calling ensureMainWindowVisible() here reaches createWindow()/new BrowserWindow() before Electron is ready, so cold-start deep links can fail instead of being buffered for the first renderer. Buffer the link immediately, but schedule/await activation after app.whenReady().
Useful? React with 👍 / 👎.
| if (link.kind !== "unknown") { | ||
| void ensureMainWindowVisible(); |
There was a problem hiding this comment.
In the common macOS path where the renderer subscribed and the user then closes the main window, React effect cleanup is not guaranteed to send vellum:deepLinks:unsubscribe, so subscriberCount can remain positive with zero BrowserWindows. A later vellum://send reaches this activation path, but the link was not buffered because subscriberCount !== 0 and the broadcast had no live window; the recreated renderer then drains nothing and the deep link is lost. Track subscribers by webContents/clear them on destroy, or base buffering on live windows rather than only renderer-sent cleanup.
Useful? React with 👍 / 👎.
…ers by WebContents
Two new Codex P1s, both real, both closed by the same restructuring.
## Defer activation until Electron is ready
Cold-launch trace: `will-finish-launching` → `open-url` fires BEFORE
`app.whenReady()`. My previous `ensureMainWindowVisible()` call from
`handleDeepLink` reaches `new BrowserWindow()` pre-ready, which races
Electron's init.
Fix: gate `ensureMainWindowVisible()` on `app.isReady()`. Pre-ready
the link is already buffered above; the initial `installMainWindow`
in the `whenReady` chain creates the first window which drains the
link on mount. Post-ready the activation fires for the recreate
case.
## Track subscribers by WebContents
Previous counter-based model leaked: React effect cleanup is not
guaranteed to run when a webContents is destroyed (window-close
kills the JS context before useEffect cleanups flush), so the
counter could stay positive with zero live subscribers → next link
sees count > 0 → not buffered → broadcast to no listener → lost.
Fix: `Set<WebContents>` keyed by `event.sender`, with a per-sender
`once("destroyed", ...)` listener that self-cleans regardless of
React's behavior. Common subscribe/unsubscribe path unchanged from
the renderer's perspective; the destroyed listener is defense-in-
depth.
Closes LUM-2074 (subscriber-set + destroyed listener) by
construction — was filed as a follow-up, now landing in this PR
since the same Codex finding requires it.
## Tests
- `defers activation when app is not yet ready (cold-launch
via vellum://)`.
- `activates after app becomes ready (warm path)`.
- `destroyed webContents auto-clears its subscription (no leak
when React cleanup misses)`.
- Existing subscribe / unsubscribe / drain tests refactored to
pass a synthetic `event.sender` with a `once("destroyed", ...)`
capture helper (`makeSender()`).
- `subscribe/unsubscribe IPC accounting is reference-counted and
never goes negative` renamed to `unsubscribe with no matching
subscriber is a no-op (idempotent delete)` — set-based semantics
make the assertion clearer.
33/33 deep-links tests green; 9/9 macOS test files green.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
|
Two new Codex P1s both addressed in 4f063cb: Defer activation until Track subscribers by This work also closes LUM-2074 by construction — that follow-up was for exactly this fix; marking it Done. The remaining Devin BUG_0001 docstring finding stays moot post- 33/33 deep-links tests + 9/9 macOS test files green. Generated by Claude Code |
There was a problem hiding this comment.
✦ APPROVE — reviewed at 4f063cb1
Value: vellum:// deep links now actually work end-to-end on macOS: cold-start launch opens the window and routes to the right place; hot-path links navigate while the app is running; a click on /assistant/settings won't silently drop an inbound vellum://send.
Full analysis
Architecture
Split-hook design is exactly right:
useGlobalDeepLinkConsumer→ mounted atRootLayout(confirmed inroot-layout.tsx). Route-stable. Handles navigation + window activation for every authenticated route.useDeepLinkConsumer→ mounted atChatPage. Reads fromusePendingDeepLinkStore(the narrow-waist hand-off) and pre-fills the composer. Only the chat domain knows aboutsetInput; store is how the two halves coordinate across mount timing.
Codex P1 ("Keep the deep-link consumer mounted outside ChatPage") is a false positive. Codex saw useDeepLinkConsumer in ChatPage and missed that useGlobalDeepLinkConsumer is already at RootLayout doing exactly that job. The split is the architecture.
deep-links.ts (macOS main)
subscriberCount → Set<WebContents>: correct fix. A React effect cleanup that fires before JS context teardown was the only thing preventing subscriberCount from leaking — under window-close the renderer process dies before useEffect cleanups run, so the counter could stay positive with zero real recipients. Tracking by WebContents + auto-deregistering on destroyed closes this properly.
app.isReady() gate: needed for the cold-start path where open-url fires before the app is fully up. Commit title + docs are clear about the timing. Addresses the prior Codex P1.
ensureVisible as ensureMainWindowVisible imported from ./main-window in the main process — correct. The main process handles window activation directly on cold-start without going through the renderer.
pending-deep-link-store.ts
One-shot semantics, last-write-wins on double-link. Transient (not persisted). All three choices are documented and reasonable. The "two links arriving before consumption" scenario is acknowledged and low-priority.
runtime/main-window.ts
Clean wrapper: no-ops off Electron, delegates through window.vellum?.mainWindow.ensureVisible(). Same shape as dock.ts / app-info.ts — consistent.
preload/index.ts
mainWindow.ensureVisible added to the bridge. Types updated in is-electron.ts. Wired through ipcMain.handle("vellum:mainWindow:ensureVisible") in main-window.ts. Full chain is solid.
Tests
deep-links.test.ts: WebContents stub withfireDestroyed()— tests the auto-cleanup path.appIsReadyflag tests the readiness gate. Good coverage.use-deep-link-consumer.test.tsx: Covers pending-consume, non-empty-composer-drops, whitespace-counts-as-empty.use-global-deep-link-consumer.test.tsx: Covers send/openThread/unknown event types, window activation call, store parking.main-window.test.ts: IPC handler registration + routing verified.
Codex P2 ("Move deep-link window activation out of renderer")
Partially addressed — main process now calls ensureMainWindowVisible directly via the ./main-window import for the cold-start path. The P2 concern about a backgrounded app with closed window is handled: main-process handleDeepLink can activate the window without needing a renderer subscriber. The renderer path is defense-in-depth for the hot case. Not blocking.
Vellum Constitution — Yours: the assistant responds when you call it — a deep link that silently drops because the user happened to be on settings is the assistant not showing up.
2 participants
Summary
Closes the consumer loop on the deep-link bridge from #32656. The typed
deeplink.send/deeplink.openThread/deeplink.unknownbus events now have a chat-domain subscriber that takes user-visible action, plus a newruntime/main-window.tsbridge so any consumer can bring the window forward when reacting to an inbound signal.Architecture
Main-window bridge (new imperative surface)
Third imperative wrapper after
dock.tsandapp-info.ts; same shape, same convention.apps/macos/src/main/main-window.ts—installMainWindownow registersipcMain.handle("vellum:mainWindow:ensureVisible", ...)that routes through the existingensureVisible()(recreate if destroyed, restore from minimize, show, focus, await renderer-ready). Test seam__resetForTestingadded so tests can re-exercise the install path.apps/macos/src/preload/index.ts—window.vellum.mainWindow.ensureVisible().apps/web/src/runtime/is-electron.ts— ambient mirror.apps/web/src/runtime/main-window.ts— no-op-off-Electron wrapper. Web has its own foregrounding (it IS the foreground tab); Capacitor handles app activation natively.Consumer hook in the chat domain
Per
ELECTRON.md's "hooks that bridge feature state live in the domain":apps/web/src/domains/chat/hooks/use-deep-link-consumer.ts— subscribes to the three bus events:deeplink.send→ensureMainWindowVisible(); if composer is empty (.trim().length === 0) set the input to the message; if non-empty, Sentry breadcrumb + drop.deeplink.openThread→ensureMainWindowVisible()+navigate(routes.conversation(threadId)).deeplink.unknown→ Sentry breadcrumb with the URL, no other action.composerInput,setComposerInput,navigate) so the bus subscription mounts ONCE — without this, every keystroke would tear down + resubscribe the handlers, opening a race window where a deep link landing between unsubscribe and resubscribe would be dropped.Mounted from ChatPage
apps/web/src/domains/chat/chat-page.tsxcallsuseDeepLinkConsumer({ composerInput: input, setComposerInput: setInput })next to the existinguseDraftInput.Conservative drop-don't-overwrite design choice
When a deep link's
messagearrives but the composer has in-progress text, the link is dropped with a Sentry breadcrumb (andensureMainWindowVisiblestill fires so the user sees Vellum come forward). The "queue or non-destructive prompt" UX from LUM-2068's spec is deliberately deferred — Sentry telemetry first, UX investment once we know how often it actually happens.Tests
apps/macos/src/main/main-window.test.ts(+2 cases): IPC handler registered, IPC handler routes throughensureVisible.apps/web/src/domains/chat/hooks/use-deep-link-consumer.test.tsx(7 cases):deeplink.sendempty-composer pre-fill.deeplink.openThreadnavigates + ensures window visible.deeplink.unknownSentry breadcrumb only.26/26 macOS tests + 26/26 chat+bus tests green.
Out of scope (called out for follow-up)
showevent viaonMainWindowVisibilityChange(from LUM-1965), so the regular-mode transition happens implicitly whenensureVisiblefires the show. Worth a follow-up audit if the dock policy lags the show in practice.Test plan
bun --cwd apps/macos run typecheck— green.bun --cwd apps/macos run test:ci— 9 test files, all green.bun --cwd apps/macos run build— main + preload bundles build cleanly.bun --cwd apps/web test src/domains/chat/hooks/use-deep-link-consumer.test.tsx src/stores/event-bus-store.test.ts— 26 cases green.open vellum://send?message=hellofrom another app while Vellum is backgrounded → main window comes forward, composer pre-fills.open vellum://thread/<existing-conversation-id>→ main window comes forward, router navigates.open vellum://send?message=different→ composer text preserved, Sentry breadcrumb fires.https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
Generated by Claude Code