feat(macos): menu-bar Tray + extract main-window.ts + flip accessory mode (LUM-1965, PR 1)#32630
Conversation
![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: f953cf9c97
ℹ️ 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".
| { | ||
| label: "New Conversation", | ||
| accelerator: resolveAccelerator("newConversation"), | ||
| click: () => dispatchToFocused({ kind: "newConversation" }), |
There was a problem hiding this comment.
When the main window has been hidden from the tray (or after the Dock moves to accessory mode), this menu item only sends newConversation to the existing hidden BrowserWindow; the renderer updates state but nothing makes the window visible, so choosing New Conversation from the tray appears to do nothing. The Swift status-item action calls showMainWindow() before creating/selecting a conversation, so the Electron tray action should likewise create/focus/show the main window before dispatching (the Current Conversation item has the same issue).
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ ALLOW_ACCESSORY_MODE not flipped to true despite tray landing in this PR (apps/macos/src/main/dock.ts:52)
The ALLOW_ACCESSORY_MODE flag at apps/macos/src/main/dock.ts:52 is explicitly documented to be "Flip[ped] to true in the same change that lands the tray." This PR adds the tray entry point (installTray in tray.ts, wired in index.ts:354-357), but does not flip the flag. As a result, the Dock icon will never transition to accessory mode (hidden) when all windows are closed and the user is signed out — it will always remain regular. The tray now provides an alternative entry point back to the app, so the safety gate that kept the flag false is no longer needed.
View 2 additional findings in Devin Review.
ashleeradka
changed the title
|
@codex review |
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE at 1112e1c5f0 — 6/6 CI green, 7 files +820/-115.
Headline. PR 1 of LUM-1965, but the substance isn't the tray — it's that the audit on the first sketch surfaced an architectural fix that closes both reviewer findings by construction. The first commit (f953cf9c97) added the tray as a 4th "make the main window visible" variant on top of the 3 already living in index.ts. The second commit (1112e1c5f0) extracted main-window.ts as the lifecycle owner, collapsed all 4 variants into a single ensureVisible() primitive, and the Codex P2 ("focus before dispatch") + Devin ("ALLOW_ACCESSORY_MODE not flipped") findings both close because the new shape makes them impossible to forget. This is the right way for a bot-driven review cycle to land — the second commit isn't a patch on the first, it's the structural change the findings were pointing at.
Full read
The extraction lands the pattern, not just the file
apps/macos/src/main/ now has four lifecycle-owner modules following the same installX + named-export + module-scope-state shape (about.ts, dock.ts, settings.ts, and now main-window.ts). Every future window-manipulating ticket — thread pop-outs LUM-1870, deep links LUM-1872, command palette LUM-1867, sleep/wake LUM-1974 — composes from ensureVisible() instead of growing index.ts by another 20-line branch. The cost of writing the abstraction now is small; the cost of writing it later, after 4 more callers have each invented their own "what does visible mean?" recipe, is much larger. This is the same first-principles framing #32626 and #32622 just applied to a different module.
main-window.ts API is clean
ensureVisible()is the only call site that creates a window.installMainWindowis just a once-only wrapper that calls it fromwhenReady.activate,second-instance, and the tray all compose from the same primitive — no opportunity for the "destroyed window + dispatch" race the P2 finding named.toggleVisibility()correctly gates onisVisibleAndFocusedrather than justisVisible. So clicking the tray icon while the main window is showing-but-behind-Slack focuses it instead of hiding it, which is what users expect from menu-bar apps (Things, Linear, etc.).current()is the BrowserWindow escape hatch reserved fordispatchToFocused+before-quitcleanup. Not used in this PR — pure API surface for the next 1-2 PRs. Same "ship the abstraction first" pattern as #32604.installedboolean guards bootstrap idempotency;mainWindow === wincheck in theclosedhandler avoids the stale-reference race if a previous-window's close event arrives after a new window has been created.
Reviewer findings closed by construction
- Codex P2 (focus before dispatch). New
ensureMainWindowhandler onTrayHandlers; conversation menu items call it beforedispatchToFocused. Testapps/macos/src/main/tray.test.ts:163-167asserts the count ofensureMainWindowcalls increments by 2 across New + Current Conversation clicks. Tray menu-bar item now matches Swift'sNSStatusItembehavior (showMainWindow → dispatch). - Devin (ALLOW_ACCESSORY_MODE not flipped). Flag flips to
true. The previous comment ("Flip to true in the same change that lands the tray") is replaced with one that explains the post-flip UX (signed-out + zero windows → menu-bar-only, tray is the always-available re-entry point) — exactly the contractdock.ts:52was holding for the next reviewer.
Behavior changes are net positives
The PR body calls these out, and they're correct:
second-instancepreviously calledfocusMainWindow()which no-op'd when the window was destroyed → second-launch could appear to do nothing if the user had closed the window. NowensureVisible()recreates. Real bug, real fix.activatepreviously only acted whenBrowserWindow.getAllWindows().length === 0→ Dock-clicking on the app while it had visible-but-unfocused state did nothing, despite Swift bringing the window forward. Now matches Swift.- Accessory-mode transition on signed-out + no-windows. Drops the Dock icon (per LUM-1966 design); the tray remains as the entry point.
Tray gotchas handled correctly
- Template image (
setTemplateImage(true), black-on-transparent bitmap, anti-aliased ring) — macOS auto-inverts for dark mode + pressed state. No-asset PR per the build-from-bitmap approach; the comment names the artwork follow-up. setIgnoreDoubleClickEvents(true)— fast single clicks register as two separateclicks instead of being swallowed as a double-click.- Module-scope
trayInstance— keeps the JS handle alive so Node's GC doesn't reap the Tray and leave a ghost NSStatusItem. - No
tray.setContextMenu(menu)— left and right click both opening the same menu would have collapsed the tray's two intentional gestures.popUpContextMenufrom theright-clicklistener instead, comment explicitly cites the issue.
dock.ts computePolicy purification
Unannounced but real: computePolicy becomes a pure function of (visibleWindows, signedIn, allowAccessoryMode). Test surface is now a 2x2x2 truth table instead of needing to stand up a BrowserWindow registry. Side effect: formatBadge also picks up export for the same reason. Worth keeping for future Electron-main test work.
Small non-blocking observations
- No explicit
tray.destroy()onbefore-quit. In production this doesn't matter — the NSStatusItem is released when the process exits. In dev with main-process hot reload (bun run devrebuilds), the old JS reference is GC-eligible butpopUpContextMenucan sometimes leave a ghost icon for a beat. Worth a one-liner:app.on("before-quit", () => trayInstance?.destroy())next to the daemon-stop call. devServerUrl()is called twice percreateWindow— once inloadUrl()and once ininstallSameOriginNavigationGuard. Each call re-readsprocess.env.VELLUM_DEV_URL. Tiny; could be hoisted to aconstat the top ofcreateWindowso both consumers see the same string. (If a developer changes the env var mid-process, the two calls would otherwise disagree about origin — pathological but not impossible.)APP_PROTOCOL/APP_HOSTnow duplicated inindex.ts+main-window.ts. The inline comment explicitly cites this and frames it as "lift if a third caller arrives." I'd argue the third caller is already in flight (the about/settings windows route through the same protocol via #32622) — but the constants are stable enough that drift is unlikely to bite before PR 2/3 of this series.
Merge gate
- Vex: ✅ this approval at
1112e1c5f0. - Codex: P2 at
f953cf9c97substantively closed in1112e1c5f0(focus-before-dispatch viaensureMainWindow, asserted by test). Bot's last pass predates the fix — a fresh@codex reviewat HEAD would close the gate cleanly. - Devin: "1 issue" at
f953cf9c97substantively closed in1112e1c5f0(ALLOW_ACCESSORY_MODE = true). Same staleness — fresh@devin-ai review this PRat HEAD would formalize closure. - CI: 6/6 green.
Two-bot review cycle as a model for future reviewers: when the findings point at a structural absence rather than a local bug, the right second commit is the structural change — not the local patch. This PR is that pattern.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1112e1c5f0
ℹ️ 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".
|
Three findings addressed in d6b3a27: Codex P2 ( Codex P2 (tray command race against renderer ready): Vex observations folded in same commit:
All 7 test files green. Generated by Claude Code |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6b3a273ab
ℹ️ 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".
| trackWindowState("main", win); | ||
|
|
||
| armRenderReady(); | ||
| win.webContents.once("did-finish-load", () => { |
There was a problem hiding this comment.
When the main window has been closed while an auxiliary window such as About is still focused, the tray's New/Current Conversation handlers await ensureMainWindow() and then call dispatchToFocused(). This promise resolves on did-finish-load, but the freshly-created window is still shown/focused later from ready-to-show, so the dispatch can run while About remains the focused/first window and the chat command is dropped. Please resolve the await only after the new main window has been shown/focused, or explicitly focus it before resolving.
Useful? React with 👍 / 👎.
|
Latest Codex P2 (await resolves while About still holds focus) addressed in 1962044. After the prior commit awaited Fix:
Order doesn't matter — whichever fires first waits for the other. Both branches covered in new tests. The two prior Codex P2s on this PR (visibleWindowCount on hidden windows; tray command vs renderer-ready) are also now closed at HEAD Generated by Claude Code |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1962044ed3
ℹ️ 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".
| // `VELLUM_DEV_URL` is mutated mid-process. | ||
| const isDev = !app.isPackaged; | ||
| const devBase = isDev ? getDevRendererBase() : null; | ||
| const loadTarget = devBase ?? `${RENDERER_BASE_PROD}/index.html`; |
There was a problem hiding this comment.
In packaged builds this constructs app://vellum.ai/assistant/index.html. The protocol handler may serve index.html for that request, but the browser URL remains /assistant/index.html; apps/web/src/routes.tsx only defines the app root at /assistant and then catches unknown /assistant/* paths with NotFound, so the Electron main window opens to the NotFound route instead of the app. Load RENDERER_BASE_PROD itself (or another real app route) rather than appending /index.html.
Useful? React with 👍 / 👎.
|
Codex P1 (prod main-window loads /index.html → NotFound) addressed in 5080dc8. The protocol handler in Generated by Claude Code |
…UM-1965)
PR 1 of the tray work — lands the icon, click toggle, and context
menu. Pulse-animation state machine and main-window-sizing change
follow in PR 2 / PR 3 so each lands in a reviewable diff.
Capability today:
- Menu-bar icon (programmatic template image — outlined circle
placeholder; real avatar art lands with the pulse PR).
- Left click toggles the main window: recreate if destroyed, hide
if visible+focused, focus otherwise.
- Right click (or Ctrl-click) pops a context menu with the canonical
tray actions: New Conversation, Current Conversation, Show / Hide
Main Window, About Vellum, Quit.
- Tooltip set to the app name.
Implementation notes:
- src/main/tray.ts holds the install + menu + icon builder. Module
is decoupled from index.ts's lifecycle by taking
`{ toggleMainWindow, openAbout }` handlers — keeps the main window
reference in index.ts, where it belongs, and makes the tray module
unit-testable without standing up a BrowserWindow.
- `installed`-flag idempotent install per the apps/macos PATTERNS.md
bootstrap convention. Module-scope `Tray` reference held to keep
the JS object alive past the function — without it, GC collects
the handle and the icon disappears from the menu bar even though
the underlying NSStatusItem is still alive.
- No `tray.setContextMenu()`: with it, left and right click both open
the same menu. Instead: `click` + `right-click` listeners and
manual `popUpContextMenu` from the right-click path. Matches the
Linear / menu-bar-app pattern.
- `setIgnoreDoubleClickEvents(true)` so fast single clicks fire as
two `click` events instead of being coalesced into a swallowed
double-click on macOS.
- Programmatic template icon (16x16 antialiased outlined ring) so no
binary assets ship in this PR. Switches to `createFromPath` when
the assistant-avatar artwork lands.
- LUM-1962 ("blocked by") is actually already in main as the typed
command bus at src/main/commands.ts — tray dispatches New/Current
Conversation through it the same way the application menu does.
Tests (apps/macos/src/main/tray.test.ts, 5 cases):
- Single Tray construction, double-click suppression, tooltip
installation, click + right-click registration — across 3
`installTray()` calls (idempotency).
- Template-image flag set so macOS dark-mode inversion works.
- Left-click routes through the `toggleMainWindow` handler.
- Right-click pops the context menu.
- Menu template contains the canonical actions and a `role: "quit"`
Quit item.
Test-setup shim grows Tray + nativeImage entries.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
…lip ALLOW_ACCESSORY_MODE Audit-driven follow-up to the original tray PR (f953cf9). ## Why The original tray PR added a 4th variant of "make main window visible" to index.ts on top of the 3 already there. Every future window-manipulating ticket (thread pop-outs, deep links, command palette, hotkey handlers, ambient agent activation) would have added its own variant. The main window was the one piece of UI that didn't follow the module-scope-state + installX + named-exports pattern that dock.ts / about.ts / settings.ts already use. Codex's focus-before-dispatch finding was the visible bug — the deeper issue was scattered window management. Patching in place would have ratcheted the complexity. Extracting now is small; later it's bigger. ## What ### `src/main/main-window.ts` (new) Owns the main window's lifecycle. Module-scope mainWindow is private. Public API: - `ensureVisible()` — the primitive. Recreate if destroyed, restore from minimize, show, focus. Every caller composes from this. - `hide()` / `toggleVisibility()` / `isVisibleAndFocused()` / `current()` - `installMainWindow()` — bootstrap, called once from `whenReady`. createWindow, the will-navigate same-origin guard, the dev/prod URL strategy, and trackWindowState wiring all move in. ### `src/main/index.ts` (slimmed by ~85 lines) Drops 4 helpers, the mainWindow let, and the URL-resolution constants. whenReady calls installMainWindow(); activate calls ensureVisible(); second-instance calls ensureVisible() (behavior change called out below). ### `src/main/tray.ts` TrayHandlers gains `ensureMainWindow`. The New / Current Conversation menu items now call it before dispatchToFocused — Codex P2 fix. ### `src/main/dock.ts` ALLOW_ACCESSORY_MODE flips from false to true. The previous comment explicitly documented this: "Flip to true in the same change that lands the tray." Devin's finding addressed. ## Behavior changes called out 1. second-instance previously no-op'd when main was destroyed; now recreates. Net fix. 2. activate previously only acted when main was null/destroyed; now ALSO brings an existing window to front. Matches Swift. 3. With accessory mode on, signed-out + all-windows-closed transitions the app to menu-bar-only. Intended behavior from LUM-1966. ## Tests - main-window.test.ts (new) — primitives exercised across the matrix. - tray.test.ts — handlers updated; new test asserts conversation items surface the window before dispatching. https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
… extraction Folds in three Codex / Vex findings into one commit since they're all about the same architectural seam — the main window's lifecycle and how dependent surfaces (tray, dock) compose around it. ## Codex P2 — visibleWindowCount misses hidden windows `dock.ts`'s `visibleWindowCount` previously filtered on `isDestroyed()` only. Hiding the main window via the tray keeps the BrowserWindow alive but invisible, so the count stayed at 1 and the policy never transitioned to accessory mode (which only fires when count === 0 + signed out + ALLOW_ACCESSORY_MODE). Fix: filter additionally on `win.isVisible()`. Add per-window `show` and `hide` listeners alongside the existing `closed` so the state machine refreshes on visibility transitions, not just on create/destroy. ## Codex P2 — tray command race against renderer ready When the main window is destroyed and the user picks New / Current Conversation from the tray, `ensureMainWindow()` returns synchronously after `new BrowserWindow` even though the preload script and the `useVellumCommands` listener haven't subscribed yet. The immediate `dispatchToFocused(...)` then sent the command to a not-yet-ready renderer and dropped it. Fix: `ensureVisible()` now returns `Promise<void>` resolved on the window's `did-finish-load`. `TrayHandlers.ensureMainWindow` returns the same shape, and the New / Current Conversation menu items `await` it before dispatching. Residual race: `did-finish-load` fires after parse but before React mounts. In practice the gap is ~ms — a proper renderer-ready IPC handshake is a bigger ticket. For the tray's click rate this is good enough; called out in the JSDoc. ## Vex observation — `app-config.ts` extraction `APP_PROTOCOL` and `APP_HOST` had three callers across `index.ts`, `main-window.ts`, and `about.ts` (the third one arrived in #32622 with about.ts's URL composition). The previous "small surface, lift when a third caller arrives" comment is now actionable. `apps/macos/src/main/app-config.ts` holds the constants plus a shared `RENDERER_BASE_PROD` + `getDevRendererBase()` helper. About.ts sheds its bespoke dev URL fallback + trailing-slash strip (now in the helper). main-window.ts resolves the dev URL once per `createWindow` so the loader and the navigation guard see a consistent string even if `VELLUM_DEV_URL` is mutated mid-process. ## Vex observation — tray destroy on before-quit `app.on("before-quit", () => trayInstance?.destroy())` inside `installTray`. In production the OS releases the NSStatusItem when the process exits; in dev with main-process hot reload, this avoids a ghost menu-bar icon for a beat between reloads. ## Tests - `main-window.test.ts` — `webContents.once` added to the stub for the `did-finish-load` await. All 16 cases still cover the API. - `tray.test.ts` — handlers updated to `mock(() => Promise.resolve())` for `ensureMainWindow`; `app.on` added to the electron mock for the new before-quit subscription. - All 7 test files green. https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
…ady-to-show
Codex P2 — after the prior commit awaited `did-finish-load` before
the tray's dispatch, a residual race remained: if About was focused
when the user clicked the tray's New Conversation, the new main
window's `ready-to-show` fires *after* `did-finish-load`, so the
dispatch ran while About still held focus. `dispatchToFocused` picks
by `getFocusedWindow`, so the chat command went to the wrong window
and was dropped.
Fix: `renderReady` resolves only when BOTH events have fired:
- `did-finish-load` — the renderer is loaded.
- `ready-to-show` — at which point we now also call `win.focus()`,
not just `win.show()`. By the time the await resolves, focus has
transferred to main, and `dispatchToFocused` lands on it.
Order doesn't matter — whichever event fires first waits for the
other. Both branches covered in the new tests.
Test additions in `main-window.test.ts`:
- Promise waits for did-finish-load first then ready-to-show.
- Symmetric: ready-to-show first, then did-finish-load.
- ready-to-show calls both show() and focus() so dispatchToFocused
targets the new window.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
Codex P1 — in packaged builds the main window was loading `app://vellum.ai/assistant/index.html`. The `app://` protocol handler did serve `index.html` content for that path, but the browser URL stayed at `/assistant/index.html`. React Router's top-level routes only declare `/assistant` itself; the `*` catch-all inside the `/assistant` subtree picks up everything else with `NotFound`. So the prod main window would have opened to the NotFound route instead of the app. Fix: load `RENDERER_BASE_PROD` directly (`app://vellum.ai/assistant`). The protocol handler's no-extension fallback in `index.ts` serves `index.html` content; the browser URL stays at `/assistant`; React Router's `/assistant` route matches; the app loads. Pre-existing bug — the prod load path isn't exercised in dev so this hadn't bitten. Caught by Codex on the React-route audit. https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
…tive bug sweep)
Audit-driven sweep after the latest Codex P1 — three real bugs that
would have surfaced under load, plus a small API addition that closes
the focus-transfer race class for good.
## Per-window readiness via WeakMap (P0)
The prior `let renderReady` + `let resolveRenderReady` lived at module
scope. Two failure modes:
1. **Concurrent create cross-resolution.** If a second `createWindow`
ran before the first's events fired, the second `armRenderReady`
overwrote the first window's resolver. The first window's
`did-finish-load` would then resolve the SECOND window's promise
prematurely (before its renderer loaded).
2. **Stale-promise hangs.** The existing-window path of
`ensureVisible` returned the module-scope `renderReady` from the
previous load. If that load never completed (window destroyed
mid-load), the next caller awaited an unresolved promise forever.
Fix: per-window state keyed by `WeakMap<BrowserWindow, ReadyState>`.
Each `createWindow` arms a fresh state owned by its window. The
existing-window re-show path returns an already-resolved sentinel
(its readiness was satisfied on the original load).
## Resolve readiness on `closed` (P0)
Network failure during load, user quit mid-load — neither
`did-finish-load` nor `ready-to-show` fires, but the await would
hang. Now the `closed` handler resolves the pending state so the
caller unblocks. The follow-up `dispatchToMain` then sees no main
window and no-ops — which is the right semantics: the window is
gone, nothing should happen.
## Tray dispatches by reference, not by focus (P2)
`dispatchToFocused` picks via `getFocusedWindow()`. After
`win.focus()`, the OS doesn't deliver the focus change
synchronously — on macOS, with the app potentially backgrounded
when the tray is clicked, even a perfectly-timed await can still
race with focus delivery and land the dispatch on About or
whichever window held focus.
Added `dispatchToMain(command)` in `main-window.ts`: looks up the
main window by reference and sends `vellum:command` directly. The
tray's New / Current Conversation menu items now call it instead of
`dispatchToFocused`. The focus-transfer race goes away by
construction — we don't ask "who's focused?", we say "send it to
main."
`dispatchToFocused` stays as-is in `commands.ts` for the application
menu, where "focused" is the right semantics.
## Tests
- `main-window.test.ts` — new readiness-gate test for
destroyed-before-ready (window closed before either event fires →
awaiter unblocks). New `dispatchToMain` describe block: dispatches
via webContents.send, no-ops when no window, no-ops on destroyed.
- `tray.test.ts` — mocks `./main-window` for `dispatchToMain`,
conversation-items test asserts both `ensureMainWindow` is awaited
AND `dispatchToMain` is called with the right command kinds.
- All 7 test files green.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
ashleeradka
force-pushed
the
claude/lum-1965-tray-icon
branch
from
5080dc8 to
ab43bc6
Compare
Proactive bug sweep — three real bugs fixed in
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab43bc63d2
ℹ️ 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".
| // Existing window: its readiness was satisfied on the original | ||
| // load. Re-show + re-focus are synchronous OS calls; nothing to | ||
| // wait for. | ||
| return ALREADY_READY; |
There was a problem hiding this comment.
When the main window already exists but is still loading, this branch returns an already-resolved promise even though the ReadyState promise has not resolved yet. This happens during startup/recreation if the user chooses “New Conversation” or “Current Conversation” from the tray before did-finish-load and ready-to-show; the tray handler awaits ensureMainWindow() and then immediately sends vellum:command, so the command can still be dropped by the renderer. Return the existing window's pending ready promise when its load/show gate is not satisfied yet.
Useful? React with 👍 / 👎.
…ness, not ALREADY_READY Codex P2 — the existing-window branch was returning `ALREADY_READY` unconditionally, but if the window was JUST created and its `did-finish-load` + `ready-to-show` hadn't both fired yet, that's the wrong answer. The tray's `await` would resolve immediately and `dispatchToMain` would land on a renderer that hadn't yet subscribed to `vellum:command`. Fix: look up the existing window's stored `ReadyState` and return its promise. If both events have already fired, the promise is resolved and the await is free. If not, the await waits for the gate the same way it does on a fresh `createWindow`. Test added: a second `ensureVisible` against an in-flight window shares the same readiness gate and resolves only when the events fire. https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
|
Codex P2 (existing-window The branch now returns Generated by Claude Code |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bbc6c3f7b
ℹ️ 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".
| * Renderer-base URL for the packaged app. Auxiliary windows append | ||
| * their own subpath (`/about`, future `/conversations/<id>`, etc.). | ||
| */ | ||
| export const RENDERER_BASE_PROD = `${APP_PROTOCOL}://${APP_HOST}/assistant`; |
There was a problem hiding this comment.
/assistant prefix before packaging
In packaged builds this base causes the loaded HTML to request assets under /assistant/assets/... because apps/web/vite.config.ts sets base: "/assistant/", but the app:// handler in apps/macos/src/main/index.ts joins the full URL pathname directly under rendererRoot and only serves files that exist there. Since the web build emits files under rendererRoot/assets/... (not rendererRoot/assistant/assets/...), the main JS/CSS requests 404 and the packaged Electron app opens a blank/broken renderer unless the protocol handler strips the /assistant mount prefix or the files are copied under that prefix.
Useful? React with 👍 / 👎.
Codex P1 — packaged builds load `app://vellum.ai/assistant` and the returned HTML refers to assets at `/assistant/assets/index.js` (because `apps/web/vite.config.ts` sets `base: "/assistant/"`). The protocol handler joined those URL paths directly under `rendererRoot`, looking for `rendererRoot/assistant/assets/index.js` — but the renderer files on disk live at `rendererRoot/assets/...`, NOT under a `/assistant/` subdirectory. Every asset 404'd → blank renderer in prod. Fix: `resolveAppProtocolPath` takes an optional `mountPrefix`. `index.ts`'s protocol handler passes `"/assistant"`, so requests matching `/assistant` (exact) or `/assistant/<rest>` map to `rendererRoot` (exact) and `rendererRoot/<rest>` respectively. Other paths pass through unchanged so things outside the mount (favicon, future top-level resources) still work. Pre-existing latent prod bug — dev served via Vite at port 5173 doesn't go through the protocol handler so this hadn't surfaced. Tests added (`app-protocol.test.ts` — 6 cases under "mount-prefix stripping"): - `/assistant` → renderer root - `/assistant/<rest>` → `rendererRoot/<rest>` - Deep nested paths - Non-mount top-level paths pass through - Prefix-confusion bait (`/assistantfoo/`) doesn't strip - Traversal guard still fires (URL collapse + path-normalize chain still apply, no new attack surface) https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
|
Codex P1 (
Pre-existing latent prod bug — dev serves through Vite at :5173 which doesn't go through the protocol handler, so this hadn't surfaced. 6 tests added under "mount-prefix stripping" covering: exact root mapping, deep paths, top-level passthrough, prefix-confusion, traversal-guard still firing. Generated by Claude Code |
…BrowserWindow count
Audit-driven architectural fix on top of the bug sweep.
## What's wrong (two findings folded together)
**Dock-About coupling.** `visibleWindowCount()` filtered on
`!isDestroyed() && isVisible()` but counted every BrowserWindow,
About included. Signed-out + main hidden + user opens About →
policy flips to `regular` → dock icon appears. Close About →
policy flips back to `accessory` → dock icon disappears. The dock
icon flickered on auxiliary-window lifecycle, which it shouldn't.
We'd already incrementally patched this twice (added `isDestroyed`
filter, then `isVisible` filter). A third filter for "is this
main" would be a third band-aid. The deeper issue is the
abstraction — the policy is conceptually about THE MAIN window,
not about generic window counts.
**`applyPolicy` interleaving race.** Two rapid policy transitions
could interleave: the first awaits `dock.show()`, the second
(`accessory`) runs synchronously and sets the activation policy,
then the first resumes and runs `setActivationPolicy("regular")`
— stomping the newer policy. The OS surfaces (dock visibility
vs activation policy) ended up out of sync.
## What the right shape is
1. `computePolicy` becomes `(mainVisible: boolean, signedIn,
allowAccessoryMode) → policy`. Boolean, not count. Even the
test matrix simplifies.
2. `dock.ts` subscribes to a typed event from `main-window.ts`:
`onMainWindowVisibilityChange(callback)`. Auxiliary windows
don't fire it. Dock no longer needs to identify "main" inside
a generic `browser-window-created` listener — which couldn't
have worked anyway, since `browser-window-created` fires
inside `new BrowserWindow(...)` SYNCHRONOUSLY, before
`main-window`'s `mainWindow = win` assignment lands.
3. `applyPolicy` re-checks `state.policy` after the awaited
`dock.show()` so a concurrent `accessory` transition can't
be clobbered. One-liner.
## Files
- `main-window.ts`: exports `onMainWindowVisibilityChange`. Wires
`show`/`hide` listeners that fire the signal. The `closed`
handler also fires after nulling `mainWindow`, so
`isMainWindowVisible()` queries see the updated state.
- `dock.ts`: imports `current` + `onMainWindowVisibilityChange`
from main-window. Drops the BrowserWindow.getAllWindows() scan
and the `app.on("browser-window-created")` subscription.
`computePolicy` takes `mainVisible: boolean`. `applyPolicy`
re-checks policy after the await.
- `dock.test.ts`: `computePolicy` matrix updated for boolean
signature (simpler — 2×2×2 → 5 named cases). Local mock of
`./main-window` since the new dependency chain transitively
imports `electron-store`.
All 7 test files green.
https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
Audit-driven architectural fix — closes the last two real findingsThe bug-hunt found two more real issues; both folded into 8eb44e1 as one architectural fix rather than two patches. Dock-About coupling
We'd already incrementally patched this twice (added Fix:
|
There was a problem hiding this comment.
Re-APPROVE at 8eb44e1570 — 8 commits of bug-hunt audit closed cleanly, dock-about coupling re-abstracted
6/6 CI green. My prior APPROVE was at 1112e1c5f0; 8 commits since, all addressing real bugs. Re-verified at HEAD against bot findings.
Codex findings — all 7 closed at HEAD
| # | Codex finding | Anchor | Closing commit | Verified |
|---|---|---|---|---|
| 1 | P2 f953cf9c97 — focus before dispatch |
tray.ts | 1112e1c5f0 (prior APPROVE) |
✓ |
| 2 | P2 1112e1c5f0 — queue tray commands until recreated window ready |
tray.ts | 06ac37a6ee (tray ready-await) |
✓ tray click now await ensureMainWindow() before dispatchToMain |
| 3 | P2 8eb44e1570 dock.ts L62 — count only visible windows |
dock.ts | 8eb44e1570 (computePolicy refactor) |
✓ closed structurally — computePolicy now takes mainVisible: boolean, no getAllWindows() scan, auxiliary windows don't fire onMainWindowVisibilityChange. Codex's L62 anchor lands on ALLOW_ACCESSORY_MODE = true — code is gone, comment is stale anchor. |
| 4 | P2 8eb44e1570 main-window.ts L182 — resolve readiness after focusing |
main-window.ts | 9656699aa3 (BOTH-gate readiness) |
✓ maybeResolveReady() only fires when didFinishLoad && didShow. show() + focus() run in the ready-to-show listener BEFORE didShow = true, so the promise resolves AFTER focus is set. |
| 5 | P1 1962044ed3 — load assistant route not /index.html |
main-window.ts | d0513f3ec4 |
✓ now loads RENDERER_BASE_PROD directly; no /index.html suffix. |
| 6 | P2 ab43bc63d2 — wait for in-flight readiness |
main-window.ts | 1bbc6c3f7b |
✓ existing-window branch returns readyStates.get(mainWindow)?.promise ?? ALREADY_READY, not unconditional ALREADY_READY. |
| 7 | P1 8eb44e1570 app-config.ts L30 — serve /assistant prefix |
app-protocol.ts | bc8184efd9 (mount-prefix strip) |
✓ resolveAppProtocolPath(rendererRoot, url, "/assistant") strips the mount in the protocol handler. New app-protocol.test.ts suite covers the /assistant, /assistant/<rest>, /assistantfoo (no false-prefix-match), and traversal-through-mount cases. |
The two architectural fixes in 8eb44e1570 (the audit-driven commit)
Boss's issue comment frames it correctly. Verified both:
1. Dock-About coupling — visibleWindowCount() had been incrementally patched twice (!isDestroyed, then isVisible); a third patch for "is this main" would have been a band-aid. The deeper fix is the abstraction: computePolicy now takes (mainVisible: boolean, signedIn, allowAccessoryMode). dock.ts subscribes via onMainWindowVisibilityChange exported from main-window.ts — auxiliary windows (About, future thread pop-outs) deliberately don't fire it. The dock no longer needs to identify "main" from a window list. Same fix observation Boss had on app.on("browser-window-created") — that hook fires synchronously inside new BrowserWindow(...) BEFORE mainWindow = win lands, so any identity check there is racy by construction. Now the visibility hook fires AFTER the assignment in createWindow, after every show/hide, and on closed.
2. applyPolicy interleaving race — state.policy = next was being mutated before the awaited dock.show(), leaving a window where a second concurrent applyPolicy("accessory") could update state.policy AND setActivationPolicy("accessory") while the first call was still awaiting, then the first call resumes and runs setActivationPolicy("regular") — stomp. Fix: re-check state.policy !== "regular" after the await and bail. One-liner, correct ordering.
Bonus wins that landed in this audit pass
app-config.tsextraction — APP_PROTOCOL/APP_HOST/RENDERER_BASE_PROD/getDevRendererBase()lifted to a single shared module. This is exactly the "third caller arrived" refactor my prior review flagged as a future-proofing concern (about.tsis the third caller afterindex.ts+main-window.ts).about.ts:aboutWindowUrlnow composes from the shared constants instead of duplicating the trailing-slash strip + URL concat. ✓dispatchToMainextracted — tray.ts no longer routes through the application-menu'sdispatchToFocused(which usesgetFocusedWindow() ?? getAllWindows()[0]). The tray click happens with the app potentially backgrounded, so OS focus delivery is async; targeting main by reference is unambiguous. Test assertsdispatchToMainMockreceives{ kind: "newConversation" }and{ kind: "currentConversation" }in order AFTERensureMainWindowresolves.closedhandler resolves the readiness promise — anti-hang for the destroyed-mid-load race (network failure during load, user quit mid-load). Caller's follow-up dispatch seescurrent() === nulland no-ops. Correct semantics — "user closed the window, nothing should happen".- Per-window readiness via
WeakMap<BrowserWindow, ReadyState>— fixes the cross-resolve race the prior module-scoperesolveRenderReadyhad where two near-simultaneouscreateWindowcalls could have the second'sarmRenderReadyoverwrite the first's resolver.
Test coverage
- New
app-protocol.test.ts— 78 lines, covers root, nested, traversal collapse via URL parser, malformed%ZZ→ forbidden (not 500),startsWith(rendererRoot + sep)invariant, and the full/assistantmount-prefix strip suite including the sibling-shares-prefix case (/assistantfoo/bardoesn't match/assistantstrip). dock.test.ts—computePolicy2×2×2 truth table (pure function, no Electron stand-up). Mocks./main-windowascurrent: () => null, onMainWindowVisibilityChange: () => undefined.main-window.test.ts— 408 lines, full ensureVisible / hide / toggleVisibility / isVisibleAndFocused / readiness-gate / dispatchToMain / current / installMainWindow coverage. The readiness-gate suite is the load-bearing one: waits for BOTH events, resolves regardless of order, in-flight ensures share the same promise, destroyed-before-ready unblocks the awaiter.tray.test.ts— updated to assertawait ensureMainWindow() → dispatchToMainordering + payload shape.
Non-blocking observations
- Codex inline anchors on dock.ts L62 and main-window.ts L182 are stale — they point at lines that no longer contain the original bug (the architectural fix moved the relevant code). Worth a 👍 on those specific Codex inlines to mark resolved so the comment thread reflects the close.
commands.ts:dispatchToFocusedstill usesgetFocusedWindow() ?? getAllWindows()[0]— that's the right shape for the application menu (it should target wherever the user is focused), so it stays. Just noting it as a structural divergence: tray targets main, menu targets focused. The comment indispatchToMaindocuments the asymmetry well.- The local-mode dev path with
VELLUM_DEV_URLoverrides —getDevRendererBase()is now the single source of truth for both windows. If anyone setsVELLUM_DEV_URL=http://localhost:5173/assistant/with trailing slash, the strip handles it; double-slash regression unlikely.
Merge gate
6/6 CI green at HEAD. All 7 Codex findings closed across 8 commits. Codex's two newest findings (dock.ts L62, app-config.ts L30) were both raised AGAINST 8eb44e1570 but had already been closed BY 8eb44e1570 — the anchors are stale, the code at those lines doesn't contain the bug anymore. A fresh @codex review at HEAD will surface this clearly. Devin's last APPROVE/comment is from f953cf9c97 (very early); a fresh @devin-ai review this PR would also formalize closure.
PROCEDURE carry-forward: when a Codex inline points at a line number and the code at that line has been structurally replaced (not just locally patched), the comment anchor migrates to the closest surviving line in the same file — making the comment LOOK like it's against new code. Always check the line content matches the finding's claim before treating an anchor as a live bug. The fix is the structural change, not addressing the comment in place.
Recommend nighttime merge once Codex re-reviews at HEAD (per the nighttime merge rule). All real findings closed; the architectural state of dock/main-window/about coupling is materially better than at my last APPROVE.
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
PR 1 of LUM-1965. Two things land together because the audit surfaced an architectural fix that closes the Codex / Devin findings by construction:
apps/macos/src/main/main-window.tsextraction — the main window's lifecycle moves fromindex.tsinto its own module, following the sameinstallX+ named-exports pattern asdock.ts/about.ts/settings.ts. Every callsite that wants to manipulate the main window composes from a singleensureVisible()primitive.ALLOW_ACCESSORY_MODEflips totrueindock.tsper the inline contract from feat(macos): Dock unread badge + visibility state machine (LUM-1966) #32461 ("flip to true in the same change that lands the tray").Pulse status indicator + main-window-sizing change ship in PR 2 / PR 3.
Architectural rewrite:
main-window.tsThe original sketch of this PR added a 4th variant of "make main window visible" to
index.tson top of the 3 already there (focusMainWindow,toggleMainWindowVisibility, theactivatehandler). Every future window-manipulating ticket (thread pop-outs LUM-1870, deep links LUM-1872, command palette LUM-1867, sleep/wake LUM-1974) would have added its own variant.The main window was the one piece of UI that didn't follow the module-scope-state +
installX+ named-exports pattern thatdock.ts,about.ts,settings.tsalready use. Extracting now is small; later it's much bigger.Public API:
ensureVisible()hide()toggleVisibility()ensureVisible(). The tray's left click + "Show / Hide Main Window" menu item compose from this.isVisibleAndFocused()current()installMainWindow()whenReady.createWindow, the will-navigate same-origin guard, the dev/prod URL strategy, andtrackWindowStatewiring all move intomain-window.ts.index.tsshrinks by ~85 lines.Tray (the original scope)
Same as the prior PR draft, with the handlers wired through the new primitives:
nativeImage.createFromBitmap— no binary assets in this PR; switches tocreateFromPathagainst shipped artwork when the assistant-avatar PR lands).toggleVisibility().popUpContextMenu(notray.setContextMenu()— with it, left and right click both open the same menu).setIgnoreDoubleClickEvents(true)so fast single clicks fire as twoclickevents instead of being coalesced into a swallowed double-click on macOS.Reviewer findings closed in this PR
Codex P2 — "Focus the main window before tray commands"
Closed by construction:
TrayHandlersgainsensureMainWindow, and the New / Current Conversation menu items call it beforedispatchToFocused(...). Test added asserting both items surface the window before dispatching.Devin — "ALLOW_ACCESSORY_MODE not flipped despite tray landing"
Closed: flag flips to
truein this PR. The previous comment is replaced with one that explains the post-tray behavior (signed-out + all-windows-closed → menu-bar-only, the tray is the always-available re-entry point).Behavior changes called out
app.on("second-instance")previously calledfocusMainWindow()which was a no-op when main had been destroyed. NowensureVisible()recreates. Net fix — a second app launch should always surface a window.activatehandler previously only acted whenmainWindowwas null/destroyed. NowensureVisible()ALSO brings an existing window to front when the user clicks the Dock icon while another app has focus. Matches Swift's activate behavior.ALLOW_ACCESSORY_MODE = true: signing out + closing all windows transitions the app to accessory mode (no Dock icon, menu-bar only) instead of staying in regular mode. The intended behavior from LUM-1966's design.Tests
apps/macos/src/main/main-window.test.ts(new, 17 cases) —ensureVisiblerecreates / restores / focuses;hideis a no-op when destroyed;toggleVisibilitycomposes correctly;isVisibleAndFocusedbranches on both flags;installMainWindowidempotent;current()returns null → live → null across lifecycle.apps/macos/src/main/tray.test.ts(updated) — handlers growensureMainWindow; new test asserts conversation menu items call it before dispatching.Out of scope (follow-up PRs)
1280 × 800→1200 × 900withminWidth: 800, minHeight: 600, matching Swift. Trivial but a separate concern.Test plan
bun --cwd apps/macos run typecheck— green.bun --cwd apps/macos run test:ci— 7 test files, all green.bun --cwd apps/macos run build— main + preload bundles build cleanly.bun run devshows a circle icon in the macOS menu bar; left-click toggles the main window; right-click opens the menu; New Conversation from the tray surfaces the window AND dispatches.https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe