feat(macos+web): branded About window — React route loaded by Electron (LUM-1971)#32622
Merged
Conversation
Replace Electron's default `aboutPanel` (just shows the bundle name)
with the same info surface Swift Vellum exposes today: app name,
version, commit SHA, copyright, link to the website.
## Implementation
`src/main/about.ts`:
- `installAbout()` (idempotent) — registers IPC handlers for
`vellum:app:versionInfo` and `vellum:app:openWebsite`, and seeds
the native About panel via `app.setAboutPanelOptions` so AppleScript
/ accessibility queries still see correct metadata.
- `openAboutWindow()` — opens (or focuses) a 360x360 frameless
BrowserWindow with `titleBarStyle: "hiddenInset"` (Swift's
`titlebarAppearsTransparent` equivalent).
- `getVersionInfo()` — pure, returns the bundle metadata. Exported
for both the IPC handler and tests.
The About content lives in `about.html` and is imported via Vite's
`?raw` suffix as a string, then loaded into the BrowserWindow as a
`data:` URL. That keeps the asset story trivial (no `extraResources`
copy step, no file-path resolution that drifts between dev and
packaged builds) at the cost of a small `'unsafe-inline'` CSP for the
single inline `<script>` that pulls version info from `window.vellum.app.*`.
Window is sandboxed with `contextIsolation: true` and
`nodeIntegration: false` — same security posture as the main
BrowserWindow.
## SHA injection
`electron.vite.config.ts` resolves the SHA at config-evaluation time
and inlines it via Vite's `define`:
- CI: 7-char prefix of `process.env.GITHUB_SHA`.
- Dev: `git rev-parse --short HEAD`.
- Tarball checkout: literal "unknown".
Verified the build replaces `__VELLUM_BUILD_SHA__` with a string
literal in `out/main/index.js`.
## Menu integration
`src/main/menu.ts` swaps `{ role: "about" }` (which would open the
native panel) for `{ label: "About Vellum", click: openAboutWindow }`.
The native panel metadata is still populated so external invocations
work correctly.
## Bridge
`src/preload/index.ts` adds `window.vellum.app.versionInfo()` and
`window.vellum.app.openWebsite()`, paralleling the existing
`commands`/`settings`/`dock` namespaces. The ambient declaration in
`apps/web/src/runtime/is-electron.ts` mirrors the surface so the
renderer's view of the bridge stays honest, even though the About
window itself loads its content from a `data:` URL outside the
`apps/web` bundle.
## Tests
`src/main/about.test.ts` covers `getVersionInfo`, `installAbout`
(IPC + panel options + idempotency), and `openAboutWindow` (singleton
behavior, reconstruct-after-close).
## Deviation from the ticket
Ticket proposed a `/about` React route in `apps/web`. `apps/web`'s
Vite config sets `base: "/assistant/"`, which makes wiring a
standalone top-level `/about` route more friction than it's worth
for a ~50-line metadata page. The inline-HTML-via-data-URL approach
keeps the About surface entirely main-process-owned and avoids any
apps/web routing changes; the bridge surface stays identical so a
future React-rendered version can drop in if/when there's UX reason
to do so.
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: 1525c9dbdd
ℹ️ 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".
…main Addresses two findings from Codex review on #32622. P1 (about.ts): the About window has the full `window.vellum` preload bridge but had no navigation guard. If the document ever navigated away — bare-`<a>` href fallback when the async script handler failed to attach, dropped URLs/files onto the window, `window.location` writes from a future script — the destination would load with the privileged bridge still exposed. The About window has no legitimate top-level navigations (its only outbound link routes through `window.vellum.app.openWebsite()` → main → `shell.openExternal`), so the fix is to block every navigation path: webContents.on("will-navigate", (event) => event.preventDefault()); webContents.setWindowOpenHandler(() => ({ action: "deny" })); Added a test that registers a stub `preventDefault`-spying event and verifies the `will-navigate` handler calls it. P2 (index.ts): the `activate` handler checked `BrowserWindow.getAllWindows().length === 0`, which counted the About window. After the user closed the main window with About still open, clicking the Dock icon wouldn't recreate the main window. Switched the check to `!mainWindow || mainWindow.isDestroyed()` — `mainWindow` is the existing module-scope nullable that `createWindow` already sets and clears on `closed`, so the activate path now only cares about the main window and ignores auxiliary ones (About, future thread pop-outs). https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Per audit, the inline-HTML-via-data-URL approach was the wrong pattern to copy. Every future auxiliary window (thread pop-outs, command palette, branded settings, share-feedback) is going to want React + design library + Tailwind tokens, and repeating the inline approach would drift four UI surfaces away from the rest of the app. This rewrite lifts the About UI into apps/web as the convention every future auxiliary window will follow: - apps/web/src/components/about-page.tsx — the React component, styled with the design system. Reads version info from the host via the runtime wrapper and falls back to "—" off Electron. - apps/web/src/runtime/app-info.ts — per-capability wrapper for `window.vellum.app.*`, mirroring the `dock.ts` shape ELECTRON.md documents. - apps/web/src/utils/routes.ts + routes.tsx — adds /assistant/about as a sibling of /assistant (not a child) so React Router's most-specific matcher picks it BEFORE the auth-protected app tree. URL sits under /assistant/* so Vite's SPA fallback (scoped to base: "/assistant/") serves it in dev. - apps/macos/src/main/about.ts shrinks: drops the inline HTML, the ?raw import, the unsafe-inline CSP, and the data-URL composition. Now just builds the BrowserWindow and calls loadURL(<dev-or-prod-base>/about). IPC handlers, panel options, will-navigate guard, singleton behavior all preserved. - apps/macos/src/main/about.html and vite-env.d.ts deleted (no longer needed). The IPC bridge surface and main-process structure are unchanged from the prior commit — only the UI's location moves. https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
ashleeradka
changed the title
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
APPROVE
The headline isn't the About window — it's the convention this PR locks in. The data: URL prototype (1525c9 → 4c75b3) sidestepped apps/web's base: "/assistant/" routing question by inlining HTML, which would have forced every future auxiliary window (thread pop-outs LUM-1870, command palette LUM-1867, branded settings, share-feedback LUM-1980) to either copy the ?raw + 'unsafe-inline' workaround or re-solve the routing problem from scratch. The fa313d rewrite pays the integration tax once: declare /assistant/about as a sibling of /assistant (not a child) so React Router's most-specific matcher picks it before falling into the auth-protected tree, keep the URL under /assistant/* so Vite's SPA fallback (scoped to base) serves it in dev. Every future auxiliary window now follows: React route in apps/web, opened by loadURL from a BrowserWindow factory in apps/macos/src/main/. The main-process file ends up ~140 lines of pure window+IPC+panel — no UI in it.
Integrates cleanly with #32614's resolveAppProtocolPath extraction. Packaged builds load app://vellum.ai/assistant/about, which flows through the same protocol.handle registered in index.ts — including the path-traversal guard #32614 just made unit-testable. No new attack surface in the renderer-load path.
Codex / Devin flags — all addressed at HEAD fa313dcb:
- Codex P1 (will-navigate guard before preload): closed by
webContents.on("will-navigate", e => e.preventDefault())+setWindowOpenHandler(() => ({ action: "deny" }))on the About BrowserWindow. The only outbound link routes throughwindow.vellum.app.openWebsite()→shell.openExternalin main; everything else is denied, so the preload bridge can't be carried into a destination we don't control (bare<a>href fallback, dropped URL/file, futurewindow.locationwrite). The test covers it. - Codex P2 + Devin (activate handler counting auxiliary windows): closed by switching from
BrowserWindow.getAllWindows().length === 0to!mainWindow || mainWindow.isDestroyed()inindex.ts. This was a pre-existing latent bug — landing the About window made it observable. Good catch by the bots, clean fix.
CI: 11/11 green at HEAD fa313dcb.
Walked the diff (11 files) — observations, all non-blocking
Sandbox + isolation posture (about.ts:88–98): sandbox: true, contextIsolation: true, nodeIntegration: false, preload from path.join(__dirname, "../preload/index.js"). Reuses the main window's preload to expose window.vellum.app.*. Two-line will-navigate + window-open block above is the right pair for a sandboxed auxiliary that shares the bridge. Idempotent installAbout() via module-scope installed flag — ipcMain.handle would throw on duplicate registration otherwise. Test exercises 3 calls → asserts 2 IPC channel registrations total.
Route placement (routes.tsx): declared before the /assistant auth-protected tree, so the most-specific matcher wins. Comment in utils/routes.ts is doing real work — it explains why this lives at /assistant/about rather than e.g. /about (SPA fallback scoping under Vite's base). The constraint is easy to lose; the comment carries it.
SHA injection (electron.vite.config.ts): execSync at config-evaluation time with a tarball-checkout fallback to "unknown". The try/catch around git rev-parse is correct — execSync throws on non-zero exit. CI path uses GITHUB_SHA.slice(0, 7) which matches what the native panel + Swift Vellum's About show. Verified the build replaces __VELLUM_BUILD_SHA__ with a string literal per the PR body; no runtime lookup remains.
Renderer wrapper (runtime/app-info.ts): per-capability shape mirroring dock.ts. Returns Promise<AppVersionInfo | null> so the About page can render its FALLBACK constant without crashing if someone hits /assistant/about on the web build. Bridge declaration in preload/index.ts returns the non-nullable shape; wrapper coerces via ?? null. Right asymmetry — the bridge is strict because it knows it's in Electron, the wrapper is permissive because it doesn't.
ABOUT_PATH duplication (about.ts:62 vs utils/routes.ts:31): docstring acknowledges the literal is duplicated rather than imported because apps/macos and apps/web are separate TS projects. Drift surface (About window loading the NotFound page) is loud and obvious — accepted trade-off, no action.
Tests (about.test.ts): 4 behaviors covered — version-info shape, install idempotency (3 calls → 2 IPC registrations + 1 panel-options call), window factory + reuse + reconstruct-after-closed, and the will-navigate guard. The Bun-module-scope installed-flag note is a useful breadcrumb for anyone adding a second installAbout test.
Minor things I noticed, not flags:
about-page.tsx—useEffectcallsgetAppVersionInfo().then(setInfo)with no unmount abort. In a window that's destroyed-not-hidden, this is fine; React's mount-time warning would only fire on a flow that today doesn't exist. If the About content ever becomes part of a panel that hides instead of destroys, revisit.app-info.ts—window.vellum?.app.versionInfo()is defensively chained even thoughisElectron()already gated. The chain is harmless; if Boss prefers consistency with the strictbridge.app.*shape in preload, drop the?.. Either way.aboutWindowUrl()—process.env.VELLUM_DEV_URL ?? "http://localhost:5173/assistant"then concatenates${ABOUT_PATH}which starts with/. If someone setsVELLUM_DEV_URL=http://localhost:5173/assistant/(trailing slash), the resulting URL becomes/assistant//about. React Router will redirect, but worth a one-linereplace(/\/+$/, "")if you want to be defensive. Truly minor.
Convention is the headline; everything else is plumbing in service of it. APPROVE.
… about URL Picks up a non-blocking observation from review. If someone sets `VELLUM_DEV_URL=http://localhost:5173/assistant/` (trailing slash), the previous concat produced `/assistant//about`. React Router collapses the double-slash on redirect, but the URL is ugly and the foot-gun is easy to avoid with a `replace(/\/+$/, "")` at the seam. https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
Contributor
Author
|
Picked up #3 in 0201751 — trailing-slash strip on
Generated by Claude Code |
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 020175138e
ℹ️ 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".
Comment on lines
+50
to
+51
| event.preventDefault(); | ||
| void openAppWebsite(); |
There was a problem hiding this comment.
When this standalone route is loaded in the web build (which the fallback copy explicitly supports), openAppWebsite() returns immediately because isElectron() is false, but the click handler has already prevented the anchor's default navigation. In that context the visible vellum.ai link does nothing; only prevent the default and use IPC inside Electron, or let the browser follow the href off Electron.
Useful? React with 👍 / 👎.
The previous click handler unconditionally `preventDefault()`'d the website link, then called `openAppWebsite()` which is a no-op off Electron. Result: in the web build the visible vellum.ai link did literally nothing. Off Electron, let the browser navigate via the bare href. In Electron the renderer is sandboxed, so we still prevent default and route through the IPC → `shell.openExternal` path that keeps the About BrowserWindow from navigating away from its own route. https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe
Contributor
Author
|
Codex P3 (web-build fallback link did nothing): applied in f6a5d68. The click handler now early-returns off Electron so the browser navigates via the bare Generated by Claude Code |
Contributor
There was a problem hiding this comment.
✦ APPROVE — reviewed at f6a5d68e (2nd APPROVE; supersedes my dismissed review at fa313dcb)
Two new commits since fa313dcb, both substantive — one picks up a non-blocking observation from the prior review, one closes a new Codex P3 that I missed on the first pass.
020175138e — VELLUM_DEV_URL trailing-slash strip. Boss applied the replace(/\/+$/, "") defense at the seam in aboutWindowUrl(). Commit message names the failure mode (/assistant//about if anyone configures the env var with a trailing slash). Clean.
f6a5d68e — web-build href fallback. This was a real bug Codex caught and I didn't.
What was happening: the click handler unconditionally preventDefault()'d the vellum.ai link, then called openAppWebsite(). Off Electron, openAppWebsite() resolves through window.vellum?.app.openWebsite() → ?. short-circuits to undefined → silent no-op. Result: in the standalone web build of /assistant/about (which the FALLBACK copy and the comments on app-info.ts explicitly support — "off-Electron, falls back to a stable default") the visible link did literally nothing.
The fix is the right asymmetry:
if (!isElectron()) return; // let the bare href navigate
event.preventDefault();
void openAppWebsite();- Off Electron → no preventDefault, browser follows
href={display.website}naturally. The web-build About page is no longer a dead-link surface. - In Electron → preventDefault stays, IPC →
shell.openExternalin main still keeps the sandboxed renderer from navigating away from its own route. That's the load-bearing constraint, and it's preserved.
The inline JSDoc above the early-return names both sides of the asymmetry — why off-Electron lets it through, and why in-Electron must not. Good "constraint-source" framing, matches the rewrite pattern Boss has been carrying through #32614.
My self-correction: my prior review's #1/#2 non-blocking observations were correctly skipped by Boss with reasoning I now agree with (BrowserWindow destroyed-not-hidden makes the unmount-abort moot; window.vellum?. chain is the right asymmetry — strict at bridge type, permissive at consumer). And I missed the actual P3 that Codex caught. Carrying forward: when reviewing a route that has both an in-Electron path and a web fallback, trace the fallback path through every handler — not just the rendering branches.
Behavior preservation — traced f6a5d68:
- Off Electron:
event.preventDefaultnot called → anchor's native behavior fires → browser navigates todisplay.website(either realinfo?.websitefrom bridge, orFALLBACK.website = "https://vellum.ai"). ✅ - In Electron: behavior byte-identical to fa313dc — preventDefault +
openAppWebsite()→ IPC → main'sshell.openExternal. ✅ isElectron()import is the same module the rest of the file already coordinates with (runtime/is-electron); no new dependency surface.
CI: 11/11 green at HEAD f6a5d68e.
Merge gate — re-review status
- Codex's substantive P3 + the boilerplate review header are at
020175138eand earlier; the f6a5d68 fix landed after Codex's last pass. Worth one more@codex reviewat HEADf6a5d68efor a clean 👍 on the resolved P3. - Devin's prior reviews are at
1525c9dbdd/4c75b39435— both predate the rewrite into a React route. A fresh@devin-ai review this PRat HEAD would close the second-approval gate per the merge criteria.
The convention this PR locks in (auxiliary windows = React routes in apps/web, opened by Electron via loadURL, packaged through app://vellum.ai/assistant/* flowing through #32614's resolveAppProtocolPath guard) is still the headline — but the f6a5d68 fix makes the standalone-web-build path actually work, which is what makes the React-route choice load-bearing instead of nominal.
Vellum Constitution — Aware: my last review approved a working Electron path but a broken web fallback. The fix proves the React-route choice is the right one — because the surface is now usable from both runtimes, the same way every future auxiliary window will need to be.
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
Merged
5 tasks
ashleeradka
pushed a commit
that referenced
this pull request
May 29, 2026
… 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
ashleeradka
pushed a commit
that referenced
this pull request
May 29, 2026
… 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
ashleeradka
added a commit
that referenced
this pull request
May 30, 2026
…mode (LUM-1965, PR 1) (#32630) * feat(macos): menu-bar (Tray) status item with click + context menu (LUM-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 * refactor(macos): extract main-window.ts, fix focus-before-dispatch, flip 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 * fix(macos): wire dock visibility filter, tray ready-await, app-config 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 * fix(macos): gate main-window readiness on both did-finish-load AND ready-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 * fix(macos): drop /index.html suffix on prod main-window load target 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 * fix(macos): per-window readiness state + dispatch by reference (proactive 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 * fix(macos): existing-window ensureVisible returns the in-flight readiness, 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 * fix(macos): strip /assistant mount prefix in app:// protocol handler 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 * fix(macos): dock policy is a function of main-window visibility, not 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces Electron's default
aboutPanel(which only shows the bundle name) with the same info surface Swift Vellum exposes today: app name, version, commit SHA, copyright, link to vellum.ai. Vellum > About Vellum… opens a 360x360 frameless macOS window matching the Swift app's transparent-title-bar treatment. Native panel metadata (app.setAboutPanelOptions) is still seeded so AppleScript / accessibility queries see the right values when they bypass our menu.The PR also lands the convention every future auxiliary window will follow: the UI is a React route in
apps/web, loaded by an Electron BrowserWindow vialoadURL. See the audit-driven rewrite section below for why this shape was chosen.Architecture
apps/web/— the UIapps/web/src/components/about-page.tsx— React component, styled with the design system (Tailwind + design-library tokens). Reads version info from the host via the runtime wrapper, falls back to"—"off Electron.apps/web/src/runtime/app-info.ts— per-capability wrapper forwindow.vellum.app.*(getAppVersionInfo,openAppWebsite), mirroring thedock.tsshapeELECTRON.mddocuments.apps/web/src/utils/routes.ts— addsroutes.about = "/assistant/about".apps/web/src/routes.tsx— registers/assistant/aboutas a sibling of/assistant(not a child), so React Router's most-specific matcher picks it for that path BEFORE falling into the auth-protected app tree. The URL sits under/assistant/*so Vite's SPA fallback (scoped tobase: "/assistant/") serves it in dev.apps/macos/— the hostapps/macos/src/main/about.ts—installAbout()(idempotent) registersvellum:app:versionInfo+vellum:app:openWebsiteIPC handlers and seeds the native About panel.openAboutWindow()opens (or focuses) a 360x360 BrowserWindow withtitleBarStyle: "hiddenInset",sandbox: true,contextIsolation: true, loading<dev-or-prod-base>/about. Module-scope window handle so reopening focuses an existing window;closedresets.apps/macos/src/main/menu.ts— swaps{ role: "about" }(would open the native panel) for{ label: "About Vellum", click: openAboutWindow }.apps/macos/src/main/index.ts— wiresinstallAbout()inwhenReady. Also fixes a pre-existing latent bug in theactivatehandler: it checkedBrowserWindow.getAllWindows().length === 0, which would have included the About window once it landed — so closing main with About still open then clicking the Dock icon wouldn't recreate the main window. Switched to!mainWindow || mainWindow.isDestroyed().Bridge
apps/macos/src/preload/index.ts— addswindow.vellum.app.versionInfo()+window.vellum.app.openWebsite()paralleling the existingcommands/settings/docknamespaces.apps/web/src/runtime/is-electron.ts— mirrors the new bridge field on the renderer-side ambient declaration.SHA injection
electron.vite.config.tsresolves the SHA at config-evaluation time and inlines it via Vite'sdefine:process.env.GITHUB_SHA.git rev-parse --short HEAD."unknown".Verified the build replaces
__VELLUM_BUILD_SHA__with a string literal inout/main/index.js.Audit-driven rewrite
The first version of this PR (commits 1525c9 → 4c75b3) shipped the About content as inline HTML imported via Vite's
?rawand loaded into the BrowserWindow as adata:URL. That sidesteppedapps/web'sbase: "/assistant/"routing question but made the About surface architecturally divergent — brand styling lived inapps/macos/, not the design system;'unsafe-inline'CSP was required; every future auxiliary window (thread pop-outs LUM-1870, command palette LUM-1867, branded settings, share-feedback LUM-1980) would have had to either copy the workaround or solve the routing question separately.Rewrite (commit fa313d) moves the UI into
apps/webas a React route and solves the loading story once: declare the route as a sibling of/assistantso React Router opts it out of the auth/layout tree, keep the URL under/assistant/*so Vite's SPA fallback serves it. Every future auxiliary window follows the same pattern.The main process file (
about.ts) is now ~140 lines of pure window factory + IPC + panel options, with no UI in it.Reviewer feedback addressed inline
webContents.on("will-navigate", e => e.preventDefault())+setWindowOpenHandler(() => ({ action: "deny" })). The About window's only outbound link routes throughwindow.vellum.app.openWebsite()→shell.openExternalin main; blocking every other path keeps the preload bridge from being carried into a destination we don't control.activatehandler scope): switched fromBrowserWindow.getAllWindows().length === 0to!mainWindow || mainWindow.isDestroyed()so auxiliary windows don't block main-window recreation.Tests
apps/macos/src/main/about.test.tscovers:getVersionInfo— shape + falls-back-to-"unknown" SHA off the build pipeline.installAbout— registers both IPC channels, seeds the native panel, idempotent on repeated calls.openAboutWindow— constructs on first call (with a URL ending in/about), focuses an existing window on repeat, reconstructs afterclosed, and registers awill-navigatehandler that callspreventDefault.Test plan
bun --cwd apps/macos run typecheck— green.bun --cwd apps/macos run test:ci— 5 test files, all green.bun --cwd apps/macos run build— main + preload bundles build cleanly; SHA substitution verified.bun --cwd apps/web run typecheckagainst the new files — clean (only pre-existing assistant-api migration errors surface, none touching these changes).https://claude.ai/code/session_011sZ4p8AkqDQMSavHvWfioe