diff --git a/apps/macos/src/main/about.ts b/apps/macos/src/main/about.ts index ef2fc68b515..c5351683917 100644 --- a/apps/macos/src/main/about.ts +++ b/apps/macos/src/main/about.ts @@ -1,6 +1,8 @@ import { BrowserWindow, app, ipcMain, shell } from "electron"; import path from "node:path"; +import { RENDERER_BASE_PROD, getDevRendererBase } from "./app-config"; + /** * Branded About window — replaces Electron's default `aboutPanel` * (which only shows the bundle name) with the same information surface @@ -60,14 +62,8 @@ export const getVersionInfo = (): AppVersionInfo => ({ const ABOUT_PATH = "/about"; const aboutWindowUrl = (): string => { - if (app.isPackaged) return `app://vellum.ai/assistant${ABOUT_PATH}`; - // Dev: build atop the same env-var the main window honors. Strip any - // trailing slash so a `VELLUM_DEV_URL=http://localhost:5173/assistant/` - // override doesn't produce `/assistant//about`. - const devBase = ( - process.env.VELLUM_DEV_URL ?? "http://localhost:5173/assistant" - ).replace(/\/+$/, ""); - return `${devBase}${ABOUT_PATH}`; + const base = app.isPackaged ? RENDERER_BASE_PROD : getDevRendererBase(); + return `${base}${ABOUT_PATH}`; }; // Module-scope handle so reopening the menu item focuses the existing diff --git a/apps/macos/src/main/app-config.ts b/apps/macos/src/main/app-config.ts new file mode 100644 index 00000000000..94f33b763a9 --- /dev/null +++ b/apps/macos/src/main/app-config.ts @@ -0,0 +1,39 @@ +/** + * Shared application identity constants for the main process. + * + * `app-protocol` and `app-host` define the custom scheme the packaged + * renderer is served from. They're referenced from at least three + * places today (`index.ts` for `protocol.registerSchemesAsPrivileged` + * + the `protocol.handle` registration, `main-window.ts` for the + * BrowserWindow load URL and the same-origin navigation guard, + * `about.ts` for the About window's prod URL), so they live here as a + * single source of truth. Drift between callers would have shown up + * as a broken renderer load rather than a build error, which is + * exactly the kind of thing a small shared module prevents. + * + * The renderer-base URLs are derived: `RENDERER_BASE_PROD` is the + * packaged path the `app://` protocol handler resolves to; the dev + * path is honored from `VELLUM_DEV_URL` (vel's edge proxy, or the + * local Vite default at port 5173). Both end at the `/assistant` + * suffix that `apps/web/vite.config.ts`'s `base` setting requires. + */ + +export const APP_PROTOCOL = "app"; +export const APP_HOST = "vellum.ai"; + +const DEV_SERVER_FALLBACK_URL = "http://localhost:5173/assistant"; + +/** + * Renderer-base URL for the packaged app. Auxiliary windows append + * their own subpath (`/about`, future `/conversations/`, etc.). + */ +export const RENDERER_BASE_PROD = `${APP_PROTOCOL}://${APP_HOST}/assistant`; + +/** + * Renderer-base URL in dev. Honors `VELLUM_DEV_URL` so the launcher + * can point at whichever Vite-or-equivalent is up (standalone Vite + * at :5173, or vel's edge proxy at :3000). Strips any trailing slash + * so callers can append `/` without producing `//`. + */ +export const getDevRendererBase = (): string => + (process.env.VELLUM_DEV_URL ?? DEV_SERVER_FALLBACK_URL).replace(/\/+$/, ""); diff --git a/apps/macos/src/main/app-protocol.test.ts b/apps/macos/src/main/app-protocol.test.ts index 5cc194cb187..2e97c2b8708 100644 --- a/apps/macos/src/main/app-protocol.test.ts +++ b/apps/macos/src/main/app-protocol.test.ts @@ -122,3 +122,81 @@ describe("resolveRelativePath — startsWith guard", () => { }); }); }); + +// `apps/web/vite.config.ts` sets `base: "/assistant/"`, so the +// built HTML emits asset URLs like `/assistant/assets/index.js`. +// The renderer files on disk live directly under `rendererRoot`, +// NOT under a `/assistant/` subdirectory. The protocol handler +// passes `mountPrefix: "/assistant"` so these tests pin the +// stripping behavior. +describe("resolveAppProtocolPath — mount-prefix stripping", () => { + test("maps `/assistant` to the root itself", () => { + expect( + resolveAppProtocolPath(ROOT, "app://vellum.ai/assistant", "/assistant"), + ).toEqual({ kind: "ok", resolved: ROOT }); + }); + + test("maps `/assistant/` to `rendererRoot/`", () => { + expect( + resolveAppProtocolPath( + ROOT, + "app://vellum.ai/assistant/assets/index.js", + "/assistant", + ), + ).toEqual({ kind: "ok", resolved: path.join(ROOT, "assets/index.js") }); + }); + + test("a nested deep path under the mount maps cleanly", () => { + expect( + resolveAppProtocolPath( + ROOT, + "app://vellum.ai/assistant/assets/css/app.css", + "/assistant", + ), + ).toEqual({ + kind: "ok", + resolved: path.join(ROOT, "assets/css/app.css"), + }); + }); + + test("a path that doesn't start with the mount is passed through (and 404s as usual)", () => { + // `/favicon.ico` lives at the bare protocol root. Stays under + // `rendererRoot/favicon.ico` (where Vite emits favicon). + expect( + resolveAppProtocolPath( + ROOT, + "app://vellum.ai/favicon.ico", + "/assistant", + ), + ).toEqual({ kind: "ok", resolved: path.join(ROOT, "favicon.ico") }); + }); + + test("a sibling that shares the mount prefix as a string is NOT stripped", () => { + // `/assistantfoo` shares `/assistant` as a string prefix but + // isn't under the mount. The strip only fires for exact match or + // `/assistant/` prefix. + expect( + resolveAppProtocolPath( + ROOT, + "app://vellum.ai/assistantfoo/bar", + "/assistant", + ), + ).toEqual({ + kind: "ok", + resolved: path.join(ROOT, "assistantfoo/bar"), + }); + }); + + test("path-traversal guard still fires for stripped paths", () => { + // `/assistant/../etc/passwd` — URL normalization collapses to + // `/etc/passwd`, which doesn't start with the mount, so no strip. + // The result lands at `rendererRoot/etc/passwd` (in-root, ok). + expect( + resolveAppProtocolPath( + ROOT, + "app://vellum.ai/assistant/../etc/passwd", + "/assistant", + ), + ).toEqual({ kind: "ok", resolved: path.join(ROOT, "etc/passwd") }); + }); +}); diff --git a/apps/macos/src/main/app-protocol.ts b/apps/macos/src/main/app-protocol.ts index edac3eba72f..6f8a512860b 100644 --- a/apps/macos/src/main/app-protocol.ts +++ b/apps/macos/src/main/app-protocol.ts @@ -56,11 +56,29 @@ export const resolveRelativePath = ( export const resolveAppProtocolPath = ( rendererRoot: string, requestUrl: string, + mountPrefix?: string, ): ResolveResult => { const url = new URL(requestUrl); + // `apps/web/vite.config.ts` sets `base: "/assistant/"`, so the + // built HTML references assets under `/assistant/assets/...`. The + // renderer files on disk live directly under `rendererRoot` — they + // are NOT nested in a `/assistant/` subdirectory. Stripping the + // mount prefix here maps `/assistant/` requests to + // `rendererRoot/`. A `mountPrefix` of `"/assistant"` matches + // both the bare `/assistant` (mapped to the root) and + // `/assistant/` paths; other top-level requests pass + // through untouched (and 404 or land in `forbidden`). + let pathname = url.pathname; + if (mountPrefix) { + if (pathname === mountPrefix) { + pathname = "/"; + } else if (pathname.startsWith(`${mountPrefix}/`)) { + pathname = pathname.slice(mountPrefix.length); + } + } let relativePath: string; try { - relativePath = decodeURIComponent(url.pathname).replace(/^\/+/, ""); + relativePath = decodeURIComponent(pathname).replace(/^\/+/, ""); } catch { // Malformed percent-encoding (e.g. `%ZZ`) throws `URIError`. // Convert to `forbidden` so the protocol handler returns a clean diff --git a/apps/macos/src/main/dock.test.ts b/apps/macos/src/main/dock.test.ts index 476963c92c7..5e9b4b7b42f 100644 --- a/apps/macos/src/main/dock.test.ts +++ b/apps/macos/src/main/dock.test.ts @@ -1,6 +1,17 @@ -import { describe, expect, test } from "bun:test"; +import { describe, expect, mock, test } from "bun:test"; -import { computePolicy, formatBadge } from "./dock"; +// `./main-window` (which `./dock` imports `current` / +// `onMainWindowVisibilityChange` from) transitively pulls in +// `./window-state`, which depends on the `electron-store` module — +// stub both so the pure-function tests below don't need a real +// store. The mocks are no-ops; the `computePolicy` matrix tests +// only exercise the pure path. +mock.module("./main-window", () => ({ + current: () => null, + onMainWindowVisibilityChange: () => undefined, +})); + +const { computePolicy, formatBadge } = await import("./dock"); describe("formatBadge", () => { test("returns empty string for zero", () => { @@ -44,26 +55,25 @@ describe("formatBadge", () => { }); describe("computePolicy", () => { - test("regular while any window is visible (signed out, gate off)", () => { - expect(computePolicy(1, false, false)).toBe("regular"); - expect(computePolicy(3, false, false)).toBe("regular"); + test("regular while the main window is visible (signed out, gate off)", () => { + expect(computePolicy(true, false, false)).toBe("regular"); }); - test("regular while signed in even with no visible windows", () => { - expect(computePolicy(0, true, false)).toBe("regular"); - expect(computePolicy(0, true, true)).toBe("regular"); + test("regular while signed in even when the main window is hidden", () => { + expect(computePolicy(false, true, false)).toBe("regular"); + expect(computePolicy(false, true, true)).toBe("regular"); }); - test("regular when signed out + no windows AND accessory gate off", () => { - expect(computePolicy(0, false, false)).toBe("regular"); + test("regular when signed out + main hidden AND accessory gate off", () => { + expect(computePolicy(false, false, false)).toBe("regular"); }); - test("accessory only when signed out + no windows + gate on", () => { - expect(computePolicy(0, false, true)).toBe("accessory"); + test("accessory only when signed out + main hidden + gate on", () => { + expect(computePolicy(false, false, true)).toBe("accessory"); }); - test("visible windows override every other signal", () => { - expect(computePolicy(2, false, true)).toBe("regular"); - expect(computePolicy(1, true, true)).toBe("regular"); + test("main-window visibility overrides every other signal", () => { + expect(computePolicy(true, false, true)).toBe("regular"); + expect(computePolicy(true, true, true)).toBe("regular"); }); }); diff --git a/apps/macos/src/main/dock.ts b/apps/macos/src/main/dock.ts index 14cd29aa1b6..7b5fb5fd41b 100644 --- a/apps/macos/src/main/dock.ts +++ b/apps/macos/src/main/dock.ts @@ -1,4 +1,9 @@ -import { app, BrowserWindow, ipcMain } from "electron"; +import { app, ipcMain } from "electron"; + +import { + current as currentMainWindow, + onMainWindowVisibilityChange, +} from "./main-window"; /** * Dock integration: unread-count badge + visibility state machine. @@ -10,8 +15,11 @@ import { app, BrowserWindow, ipcMain } from "electron"; * * The state machine has two inputs: * - * 1. **Visible window count**, observed via the `browser-window-created` - * / per-window `closed` events. No renderer involvement. + * 1. **Main-window visibility**, subscribed via + * `onMainWindowVisibilityChange` on `./main-window`. Auxiliary + * windows (About, future thread pop-outs) deliberately do NOT + * drive dock policy — opening About while signed out would + * otherwise flicker the dock icon to `regular` and back. * 2. **Signed-in flag**, published by the renderer over the * `vellum:dock:setSignedIn` IPC channel. Renderer is the source of * truth today; this side of the bridge becomes a no-op once the @@ -19,10 +27,10 @@ import { app, BrowserWindow, ipcMain } from "electron"; * * Policy: * - * - Any visible window OR signed in → `regular` (Dock icon visible). + * - Main visible OR signed in → `regular` (Dock icon visible). * We keep the icon visible while signed in so the user can re-open * the window from the Dock after closing the last one. - * - No visible window AND signed out → `accessory` (Dock icon hidden, + * - Main hidden AND signed out → `accessory` (Dock icon hidden, * menu-bar-only). * * Transitions are debounced ~100ms so a fast close-then-open (e.g. @@ -45,11 +53,13 @@ export const formatBadge = (count: number): string => { const POLICY_DEBOUNCE_MS = 100; -// Gate the `accessory` (menu-bar-only) transition until a menu-bar -// (tray) entry point exists. Going accessory before then would hide -// the Dock icon with no replacement, leaving the user no way to re-open -// the window. Flip to `true` in the same change that lands the tray. -const ALLOW_ACCESSORY_MODE = false; +// When the user is signed out and no windows are visible, drop the +// Dock icon and run menu-bar-only — same accessory-mode UX Swift Vellum +// exposes. Safe to flip on now that `installTray` provides an +// always-available entry point back to the app (the Dock icon used to +// be the only one). Kept as a module-level constant rather than a +// setting so the build-time choice is reviewable. +const ALLOW_ACCESSORY_MODE = true; interface DockState { signedIn: boolean; @@ -65,20 +75,30 @@ const state: DockState = { let refreshTimer: NodeJS.Timeout | null = null; -const visibleWindowCount = (): number => - BrowserWindow.getAllWindows().filter((win) => !win.isDestroyed()).length; +// True iff the main window is the visible, focusable surface. The dock +// policy is conceptually about the main window — not about "any window +// being alive" — so we check by reference via `main-window.current()` +// rather than scanning every BrowserWindow. Auxiliary windows +// (About, future thread pop-outs, command palette) should NOT keep +// the dock icon visible when the user is signed out — that would be +// a UX bug: opening About would briefly show the dock icon and +// closing it would hide it again. +const isMainWindowVisible = (): boolean => { + const win = currentMainWindow(); + return !!win && !win.isDestroyed() && win.isVisible(); +}; -// Pure function of (visible window count, signed-in flag, accessory-mode -// gate). Factored out of the caller so tests can exercise the matrix -// without standing up a full Electron `BrowserWindow` registry — the -// caller passes `visibleWindowCount()` + `state.signedIn` + +// Pure function of (main-window visibility, signed-in flag, +// accessory-mode gate). Factored out so tests can exercise the +// 2×2×2 matrix without standing up an Electron BrowserWindow. +// Caller passes `isMainWindowVisible()` + `state.signedIn` + // `ALLOW_ACCESSORY_MODE` at the seam. export const computePolicy = ( - visibleWindows: number, + mainVisible: boolean, signedIn: boolean, allowAccessoryMode: boolean, ): DockState["policy"] => { - if (visibleWindows > 0) return "regular"; + if (mainVisible) return "regular"; if (signedIn) return "regular"; return allowAccessoryMode ? "accessory" : "regular"; }; @@ -88,12 +108,23 @@ export const computePolicy = ( // keeps the two surfaces in sync (await sequencing is the documented // pattern). The accessory transition is synchronous on the Electron // side — `hide()` returns void — so no await there. +// +// Re-check `state.policy` after the awaited `dock.show()`. Between +// the await and resume, another `applyPolicy("accessory")` may have +// run synchronously and flipped both the state and the activation +// policy. Without the re-check, the resuming `regular` call would +// stomp the newer `accessory` setActivationPolicy, leaving the +// activation policy and dock visibility out of sync. const applyPolicy = async (next: DockState["policy"]): Promise => { if (next === state.policy) return; state.policy = next; if (!app.dock) return; if (next === "regular") { await app.dock.show(); + // A concurrent `applyPolicy("accessory")` may have run while we + // were awaiting — bail out so we don't override its + // `setActivationPolicy("accessory")`. + if (state.policy !== "regular") return; app.setActivationPolicy("regular"); } else { app.dock.hide(); @@ -106,7 +137,7 @@ const scheduleRefresh = (): void => { refreshTimer = setTimeout(() => { refreshTimer = null; void applyPolicy( - computePolicy(visibleWindowCount(), state.signedIn, ALLOW_ACCESSORY_MODE), + computePolicy(isMainWindowVisible(), state.signedIn, ALLOW_ACCESSORY_MODE), ); }, POLICY_DEBOUNCE_MS); }; @@ -153,13 +184,12 @@ export const installDock = (): void => { scheduleRefresh(); }); - // Observe the visible-window count so closing the last window can - // transition us into accessory mode (once `ALLOW_ACCESSORY_MODE` is - // flipped), and opening the first one transitions us back to regular. - app.on("browser-window-created", (_event, win) => { - scheduleRefresh(); - win.once("closed", scheduleRefresh); - }); + // Subscribe to main-window visibility transitions (created, shown, + // hidden, closed). Auxiliary windows (About, future thread pop-outs, + // command palette) don't fire this hook, so they correctly don't + // affect dock policy — a signed-out user opening About would + // otherwise briefly flicker the dock icon to `regular` and back. + onMainWindowVisibilityChange(scheduleRefresh); // macOS convention: clear the Dock badge before the process exits so // a relaunch doesn't briefly show a stale count from the OS's cache. @@ -177,7 +207,7 @@ export const installDock = (): void => { // following `setActivationPolicy` call inside `applyPolicy`; the // caller has nothing to await on. void applyPolicy( - computePolicy(visibleWindowCount(), state.signedIn, ALLOW_ACCESSORY_MODE), + computePolicy(isMainWindowVisible(), state.signedIn, ALLOW_ACCESSORY_MODE), ); applyBadge(); }; diff --git a/apps/macos/src/main/index.ts b/apps/macos/src/main/index.ts index 35fc306ce1a..30a97b02dc2 100644 --- a/apps/macos/src/main/index.ts +++ b/apps/macos/src/main/index.ts @@ -1,43 +1,21 @@ -import { app, BrowserWindow, ipcMain, net, protocol, session, shell } from "electron"; +import { app, ipcMain, net, protocol, session, shell } from "electron"; import { spawn, type ChildProcess } from "node:child_process"; import fs from "node:fs/promises"; import { pathToFileURL } from "node:url"; import path from "node:path"; -import { installAbout } from "./about"; +import { installAbout, openAboutWindow } from "./about"; +import { APP_PROTOCOL } from "./app-config"; import { resolveAppProtocolPath } from "./app-protocol"; import { installDock } from "./dock"; +import { + ensureVisible as ensureMainWindowVisible, + installMainWindow, + toggleVisibility as toggleMainWindowVisibility, +} from "./main-window"; import { installApplicationMenu } from "./menu"; import { readSetting, writeSetting } from "./settings"; -import { restoreBounds, track as trackWindowState } from "./window-state"; - -// Dev-mode renderer URL. Honors `VELLUM_DEV_URL` so the launcher can -// point the BrowserWindow at whichever Vite-or-equivalent is actually -// up: -// -// - Standalone `bun run dev` → unset, falls back to -// `http://localhost:5173/assistant` (our own Vite, spawned by -// `dev:standalone`). -// - `bun run dev` while `vel up` is running → the probe shim sets it -// to `http://localhost:3000/assistant` (vel's edge proxy + the -// renderer path that Swift Vellum hits), so the renderer is -// same-origin with the running backends. -// - Future `vel up electron` → vel sets it directly when spawning us. -// -// The `/assistant` path matters: `apps/web/vite.config.ts` declares -// `base: "/assistant/"`, and vel's edge proxy reserves the `:3000` root -// for the marketing site and routes `/assistant/*` to apps/web. Loading -// the bare origin lands the BrowserWindow on the marketing page instead -// of the renderer. Callers (vel, the probe shim, a developer setting -// the env var by hand) are responsible for including the path — -// `VELLUM_DEV_URL` is treated as the full URL to load. -// -// The port-5173 fallback agrees with `dev:web` in package.json, which -// passes `--port 5173 --strictPort` so apps/web's `.env` defaults can't -// silently move it. -const DEV_SERVER_URL = - process.env.VELLUM_DEV_URL ?? "http://localhost:5173/assistant"; -const DEV_SERVER_ORIGIN = new URL(DEV_SERVER_URL).origin; +import { installTray } from "./tray"; // Dev-only: override the workspace `name` (`@vellumai/macos`) so the // menu bar's first submenu reads "Vellum Electron", and — more @@ -64,9 +42,6 @@ const DEV_SERVER_ORIGIN = new URL(DEV_SERVER_URL).origin; if (!app.isPackaged) { app.setName("Vellum Electron"); } -const APP_PROTOCOL = "app"; -const APP_HOST = "vellum.ai"; - const isDev = !app.isPackaged; // Single-instance lock: relaunches focus the existing window instead of @@ -93,81 +68,19 @@ protocol.registerSchemesAsPrivileged([ }, ]); -let mainWindow: BrowserWindow | null = null; - -const createWindow = (): void => { - mainWindow = new BrowserWindow({ - ...restoreBounds("main", { width: 1280, height: 800 }), - show: false, - webPreferences: { - preload: path.join(__dirname, "../preload/index.js"), - contextIsolation: true, - nodeIntegration: false, - sandbox: true, - webSecurity: true, - allowRunningInsecureContent: false, - experimentalFeatures: false, - devTools: isDev, - }, - }); - - // Subscribe to resize/move/close so the next launch can restore the - // user's last geometry. See `window-state.ts` for the persistence - // model (separate electron-store file, debounced saves, fullscreen - // tracked as a flag rather than as bounds). - trackWindowState("main", mainWindow); - - mainWindow.once("ready-to-show", () => { - mainWindow?.show(); - }); - - mainWindow.on("closed", () => { - mainWindow = null; - }); - - // Same-origin allowlist for top-level navigation. Scoped to the main - // window — popups (OAuth flows etc.) need to redirect between provider - // domains and our callback origin, so they're left unrestricted. - mainWindow.webContents.on("will-navigate", (event, url) => { - let target: URL; - try { - target = new URL(url); - } catch { - event.preventDefault(); - return; - } - const allowed = - (isDev && target.origin === DEV_SERVER_ORIGIN) || - (!isDev && target.protocol === `${APP_PROTOCOL}:` && target.host === APP_HOST); - if (allowed) return; - event.preventDefault(); - // External http(s) top-level navigations (e.g. `window.location.href = - // "https://billing.stripe.com/..."`) route to the system browser instead - // of silently failing. Other schemes stay blocked. - if (target.protocol === "https:" || target.protocol === "http:") { - void shell.openExternal(url); - } - }); - - const loadTarget = isDev ? DEV_SERVER_URL : `${APP_PROTOCOL}://${APP_HOST}/index.html`; - mainWindow.loadURL(loadTarget).catch((err: unknown) => { - console.error(`[window] loadURL failed for ${loadTarget}:`, err); - }); -}; - -const focusMainWindow = (): void => { - if (!mainWindow) return; - if (mainWindow.isMinimized()) mainWindow.restore(); - mainWindow.show(); - mainWindow.focus(); -}; - // Serve apps/web/dist/ as static files via `app://vellum.ai/...`. Route-like // paths (no file extension, or `.html`) fall back to index.html so React // Router can handle client-side routes on reload / deep-link; requests for // missing static assets return 404 so a stale or partial deploy surfaces as // a load error rather than silently serving HTML with a wrong Content-Type. // Reference: https://www.electronjs.org/docs/latest/api/protocol#protocolhandlescheme-handler +// `apps/web/vite.config.ts` sets `base: "/assistant/"`, so the built +// HTML emits asset URLs like `/assistant/assets/index.js`. The +// renderer files on disk live directly under `rendererRoot`, NOT +// under `rendererRoot/assistant/`. Pass the mount as a separate +// parameter so the protocol handler strips it before path resolution. +const RENDERER_MOUNT = "/assistant"; + const registerAppProtocol = (): void => { // The packaged renderer bundle lives next to the main bundle. When this app // is built into an .asar/Resources/app/ tree, apps/web/dist/ is copied to @@ -176,7 +89,11 @@ const registerAppProtocol = (): void => { const indexHtml = path.join(rendererRoot, "index.html"); protocol.handle(APP_PROTOCOL, async (request) => { - const result = resolveAppProtocolPath(rendererRoot, request.url); + const result = resolveAppProtocolPath( + rendererRoot, + request.url, + RENDERER_MOUNT, + ); if (result.kind === "forbidden") { return new Response("Forbidden", { status: 403 }); } @@ -333,18 +250,21 @@ app installAbout(); installApplicationMenu(); installDock(); + installTray({ + toggleMainWindow: toggleMainWindowVisibility, + ensureMainWindow: ensureMainWindowVisible, + openAbout: openAboutWindow, + }); spawnDaemon(); - createWindow(); + installMainWindow(); - // Dock-icon click / Cmd-Tab re-activation. Only the absence of the - // *main* window should trigger a recreate — auxiliary windows - // (About, future thread pop-outs) shouldn't count, otherwise closing - // main while an auxiliary stays open would leave the user stuck - // with no path back to the app. + // Dock-icon click / Cmd-Tab re-activation: bring the main window + // back to front, recreating it if it was previously closed. The + // primitive handles both the destroyed-window and the + // visible-but-not-focused cases, so we don't need to branch here + // on auxiliary window counts the way the old check did. app.on("activate", () => { - if (!mainWindow || mainWindow.isDestroyed()) { - createWindow(); - } + ensureMainWindowVisible(); }); }) .catch((err: unknown) => { @@ -352,7 +272,11 @@ app }); app.on("second-instance", () => { - focusMainWindow(); + // Behavior change vs prior code path: previously a second-instance + // launch was a no-op when the main window had been destroyed. Now + // we recreate so the user always sees a window in response to + // re-launching the app. + ensureMainWindowVisible(); }); app.on("web-contents-created", (_event, contents) => { diff --git a/apps/macos/src/main/main-window.test.ts b/apps/macos/src/main/main-window.test.ts new file mode 100644 index 00000000000..562058e2a9c --- /dev/null +++ b/apps/macos/src/main/main-window.test.ts @@ -0,0 +1,408 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; + +type StubWebContents = { + on: ReturnType; + once: (event: string, handler: () => void) => void; + emit: (event: string) => void; + send: ReturnType; +}; +type StubWindow = { + show: ReturnType; + hide: ReturnType; + focus: ReturnType; + restore: ReturnType; + loadURL: ReturnType; + isDestroyed: () => boolean; + isMinimized: () => boolean; + isVisible: () => boolean; + isFocused: () => boolean; + on: ReturnType; + once: (event: string, handler: () => void) => StubWindow; + webContents: StubWebContents; + // Test seam — emits a `closed` event so the production code's + // module-scope `mainWindow = null` cleanup runs. + emit: (event: string) => void; +}; + +type WindowState = { + destroyed: boolean; + minimized: boolean; + visible: boolean; + focused: boolean; +}; + +let constructed: Array<{ stub: StubWindow; state: WindowState }> = []; +let listeners: Map void>>; + +const makeWindow = (): StubWindow => { + const state: WindowState = { + destroyed: false, + minimized: false, + visible: false, + focused: false, + }; + listeners = new Map(); + const stub: StubWindow = { + show: mock(() => { + state.visible = true; + }), + hide: mock(() => { + state.visible = false; + state.focused = false; + }), + focus: mock(() => { + state.focused = true; + }), + restore: mock(() => { + state.minimized = false; + }), + loadURL: mock(() => Promise.resolve()), + isDestroyed: () => state.destroyed, + isMinimized: () => state.minimized, + isVisible: () => state.visible, + isFocused: () => state.focused, + on: mock((event: string, handler: () => void) => { + const arr = listeners.get(event) ?? []; + arr.push(handler); + listeners.set(event, arr); + return stub; + }), + once: (event, handler) => { + const arr = listeners.get(event) ?? []; + arr.push(handler); + listeners.set(event, arr); + return stub; + }, + webContents: ((): StubWebContents => { + const wcListeners = new Map void>>(); + const wc: StubWebContents = { + on: mock(() => undefined), + once: (event, handler) => { + const arr = wcListeners.get(event) ?? []; + arr.push(handler); + wcListeners.set(event, arr); + }, + emit: (event) => { + for (const h of wcListeners.get(event) ?? []) h(); + }, + send: mock(() => undefined), + }; + return wc; + })(), + emit: (event) => { + if (event === "closed") state.destroyed = true; + for (const h of listeners.get(event) ?? []) h(); + }, + }; + constructed.push({ stub, state }); + return stub; +}; + +mock.module("electron", () => ({ + app: { + isPackaged: false, + }, + BrowserWindow: class { + constructor(_opts: unknown) { + Object.assign(this, makeWindow()); + } + }, + shell: { openExternal: () => Promise.resolve() }, +})); + +mock.module("./window-state", () => ({ + restoreBounds: () => ({ width: 1280, height: 800 }), + track: () => undefined, +})); + +const { current, dispatchToMain, ensureVisible, hide, installMainWindow, isVisibleAndFocused, toggleVisibility } = + await import("./main-window"); + +const reset = (): void => { + // Force a fresh module-scope window between tests by emitting `closed` + // on whatever's currently alive. The production code listens for + // `closed` and nulls its `mainWindow` reference. + for (const { stub, state } of constructed) { + if (!state.destroyed) stub.emit("closed"); + } + constructed = []; +}; + +beforeEach(() => { + reset(); +}); + +afterEach(() => { + reset(); +}); + +describe("ensureVisible", () => { + test("creates a new BrowserWindow when none exists", () => { + ensureVisible(); + expect(constructed).toHaveLength(1); + expect(constructed[0]?.stub.loadURL).toHaveBeenCalledTimes(1); + }); + + test("recreates after the previous window was destroyed", () => { + ensureVisible(); + constructed[0]?.stub.emit("closed"); + ensureVisible(); + expect(constructed).toHaveLength(2); + }); + + test("restores from minimize, then shows + focuses", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + win.state.minimized = true; + win.state.visible = false; + win.state.focused = false; + + ensureVisible(); + expect(win.stub.restore).toHaveBeenCalledTimes(1); + expect(win.stub.show).toHaveBeenCalled(); + expect(win.stub.focus).toHaveBeenCalled(); + expect(constructed).toHaveLength(1); + }); + + test("shows + focuses an already-existing window without recreating", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + win.state.visible = false; + win.state.focused = false; + + ensureVisible(); + expect(constructed).toHaveLength(1); + expect(win.stub.show).toHaveBeenCalled(); + expect(win.stub.focus).toHaveBeenCalled(); + }); +}); + +describe("hide", () => { + test("hides a visible window", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + + hide(); + expect(win.stub.hide).toHaveBeenCalledTimes(1); + expect(win.state.visible).toBe(false); + }); + + test("is a no-op when no window exists", () => { + hide(); + // No window to act on, no construction triggered. + expect(constructed).toHaveLength(0); + }); + + test("is a no-op when the window was destroyed", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + win.stub.emit("closed"); + + const hideCallsBefore = win.stub.hide.mock.calls.length; + hide(); + expect(win.stub.hide.mock.calls.length).toBe(hideCallsBefore); + }); +}); + +describe("toggleVisibility", () => { + test("hides when visible and focused", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + win.state.visible = true; + win.state.focused = true; + + toggleVisibility(); + expect(win.stub.hide).toHaveBeenCalledTimes(1); + }); + + test("ensures visible when hidden", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + win.state.visible = false; + win.state.focused = false; + const showsBefore = win.stub.show.mock.calls.length; + + toggleVisibility(); + expect(win.stub.show.mock.calls.length).toBe(showsBefore + 1); + }); + + test("recreates when destroyed", () => { + ensureVisible(); + constructed[0]?.stub.emit("closed"); + + toggleVisibility(); + expect(constructed).toHaveLength(2); + }); +}); + +describe("isVisibleAndFocused", () => { + test("false when no window exists", () => { + expect(isVisibleAndFocused()).toBe(false); + }); + + test("true only when visible AND focused", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + win.state.visible = true; + win.state.focused = false; + expect(isVisibleAndFocused()).toBe(false); + + win.state.focused = true; + expect(isVisibleAndFocused()).toBe(true); + }); +}); + +describe("ensureVisible readiness gate", () => { + test("the returned promise waits for BOTH did-finish-load AND ready-to-show", async () => { + let resolved = false; + const promise = ensureVisible().then(() => { + resolved = true; + }); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + + // Only `did-finish-load` fired — promise should still be pending. + win.stub.webContents.emit("did-finish-load"); + await Promise.resolve(); + expect(resolved).toBe(false); + + // Now `ready-to-show` fires, gating the resolution. + win.stub.emit("ready-to-show"); + await promise; + expect(resolved).toBe(true); + }); + + test("resolves regardless of which event arrives first", async () => { + let resolved = false; + const promise = ensureVisible().then(() => { + resolved = true; + }); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + + win.stub.emit("ready-to-show"); + await Promise.resolve(); + expect(resolved).toBe(false); + + win.stub.webContents.emit("did-finish-load"); + await promise; + expect(resolved).toBe(true); + }); + + test("a second ensureVisible against an in-flight window waits on the same readiness", async () => { + let firstResolved = false; + let secondResolved = false; + void ensureVisible().then(() => { + firstResolved = true; + }); + void ensureVisible().then(() => { + secondResolved = true; + }); + // Same window was used for both calls — no new construction. + expect(constructed).toHaveLength(1); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + + await Promise.resolve(); + expect(firstResolved).toBe(false); + expect(secondResolved).toBe(false); + + win.stub.webContents.emit("did-finish-load"); + win.stub.emit("ready-to-show"); + // Yield to flush microtasks. + await Promise.resolve(); + await Promise.resolve(); + expect(firstResolved).toBe(true); + expect(secondResolved).toBe(true); + }); + + test("unblocks the awaiter if the window is destroyed before either event fires", async () => { + let resolved = false; + const promise = ensureVisible().then(() => { + resolved = true; + }); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + + // Simulate the destroyed-before-ready race (network failure during + // load, user quit mid-load). Neither did-finish-load nor + // ready-to-show ever fire. + win.stub.emit("closed"); + await promise; + expect(resolved).toBe(true); + }); + + test("ready-to-show shows AND focuses the window so dispatchToFocused targets it", () => { + void ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + + win.stub.emit("ready-to-show"); + expect(win.stub.show).toHaveBeenCalled(); + expect(win.stub.focus).toHaveBeenCalled(); + }); +}); + +describe("installMainWindow", () => { + test("creates the initial window on first call, no-op on subsequent calls", () => { + installMainWindow(); + installMainWindow(); + installMainWindow(); + expect(constructed).toHaveLength(1); + }); +}); + +describe("dispatchToMain", () => { + test("sends `vellum:command` to the main window's webContents", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + + dispatchToMain({ kind: "newConversation" }); + + expect(win.stub.webContents.send).toHaveBeenCalledWith( + "vellum:command", + { kind: "newConversation" }, + ); + }); + + test("no-ops when no main window exists", () => { + dispatchToMain({ kind: "newConversation" }); + expect(constructed).toHaveLength(0); + }); + + test("no-ops when the main window has been destroyed", () => { + ensureVisible(); + const win = constructed[0]; + if (!win) throw new Error("expected a window"); + win.stub.emit("closed"); + + const before = win.stub.webContents.send.mock.calls.length; + dispatchToMain({ kind: "newConversation" }); + expect(win.stub.webContents.send.mock.calls.length).toBe(before); + }); +}); + +describe("current", () => { + test("returns null before any window has been created", () => { + expect(current()).toBeNull(); + }); + + test("returns the live window reference after ensureVisible", () => { + ensureVisible(); + expect(current()).not.toBeNull(); + }); + + test("returns null again after the window is destroyed", () => { + ensureVisible(); + constructed[0]?.stub.emit("closed"); + expect(current()).toBeNull(); + }); +}); diff --git a/apps/macos/src/main/main-window.ts b/apps/macos/src/main/main-window.ts new file mode 100644 index 00000000000..99c5ea86a3f --- /dev/null +++ b/apps/macos/src/main/main-window.ts @@ -0,0 +1,313 @@ +import { BrowserWindow, app, shell } from "electron"; +import path from "node:path"; + +import { + APP_HOST, + APP_PROTOCOL, + RENDERER_BASE_PROD, + getDevRendererBase, +} from "./app-config"; +import { type VellumCommand } from "./commands"; +import { restoreBounds, track as trackWindowState } from "./window-state"; + +/** + * Main BrowserWindow lifecycle owner. + * + * Other modules (tray, application menu, deep link handler, command + * palette opener, etc.) used to reach into `index.ts`'s `mainWindow` + * variable and reinvent "what does it mean to make the window + * visible?" — recreate-if-destroyed checks, restore-from-minimize, + * show, focus, the whole dance — at each call site. This module + * collapses that into a single primitive every caller composes from. + * + * Public API: + * + * - `ensureVisible()` — the primitive. Recreate if destroyed, + * restore if minimized, show, focus. Use this anywhere you'd + * previously have called `createWindow()` + extra checks. Safe + * to call repeatedly. + * - `hide()` — hide if visible; no-op if destroyed. + * - `toggleVisibility()` — hide if visible and focused; otherwise + * `ensureVisible()`. The tray's left-click and the + * "Show / Hide Main Window" menu item compose from this. + * - `isVisibleAndFocused()` — for callers that need to branch on + * "is the user looking at the main window right now?". + * - `current()` — the underlying `BrowserWindow | null`. Reserved + * for narrow cases (`webContents.send` from `dispatchToFocused`, + * `before-quit` cleanup that needs to inspect the instance). + * Most callers should use `ensureVisible` / `hide` instead. + * + * Follows the same `installX` + named-export pattern as `dock.ts` / + * `about.ts` / `settings.ts` — module-scope state, public API, and + * a single bootstrap hook (`installMainWindow`) called once from + * `whenReady` to create the initial window. + */ + +let mainWindow: BrowserWindow | null = null; + +// Visibility-change subscribers. Used by the dock state machine to +// follow main-window show/hide/closed transitions without having to +// scan every BrowserWindow and identify "main." `browser-window-created` +// would fire BEFORE `mainWindow = win` lands (the Electron event is +// synchronous inside the constructor), so any identity check at that +// hook is racy. This subscription fires AFTER the assignment so +// `current()` is correct by the time the listener runs. +type VisibilityListener = () => void; +const visibilityListeners: VisibilityListener[] = []; + +export const onMainWindowVisibilityChange = ( + listener: VisibilityListener, +): void => { + visibilityListeners.push(listener); +}; + +const fireVisibilityChange = (): void => { + for (const listener of visibilityListeners) listener(); +}; + +// Per-window readiness state. The tray's command menu items await +// `ensureVisible()` before dispatching IPC, so each freshly-created +// window needs its own promise — keyed by the `BrowserWindow` +// instance via a `WeakMap` so two near-simultaneous `createWindow` +// calls can't have the second's `armRenderReady` overwrite the +// first's resolver (or vice versa). +interface ReadyState { + promise: Promise; + resolve: () => void; + didFinishLoad: boolean; + didShow: boolean; +} +const readyStates = new WeakMap(); + +const armReadyState = (win: BrowserWindow): ReadyState => { + let resolve: () => void = () => {}; + const promise = new Promise((res) => { + resolve = res; + }); + const state: ReadyState = { + promise, + resolve, + didFinishLoad: false, + didShow: false, + }; + readyStates.set(win, state); + return state; +}; + +// Already-resolved sentinel for the "no window exists" path — +// `ensureVisible` returns a Promise even when there's nothing to +// wait for, so callers can compose `await` uniformly. +const ALREADY_READY: Promise = Promise.resolve(); + +interface NavigationGuardConfig { + isDev: boolean; + devOrigin: string; +} + +const installSameOriginNavigationGuard = ( + win: BrowserWindow, + { isDev, devOrigin }: NavigationGuardConfig, +): void => { + // Scoped to the main window — popups (OAuth flows etc.) need to + // redirect between provider domains and our callback origin, so + // they're left unrestricted. + win.webContents.on("will-navigate", (event, url) => { + let target: URL; + try { + target = new URL(url); + } catch { + event.preventDefault(); + return; + } + const allowed = + (isDev && target.origin === devOrigin) || + (!isDev && + target.protocol === `${APP_PROTOCOL}:` && + target.host === APP_HOST); + if (allowed) return; + event.preventDefault(); + // External http(s) top-level navigations (e.g. + // `window.location.href = "https://billing.stripe.com/..."`) route + // to the system browser instead of silently failing. Other schemes + // stay blocked. + if (target.protocol === "https:" || target.protocol === "http:") { + void shell.openExternal(url); + } + }); +}; + +const createWindow = (): BrowserWindow => { + // Resolve the dev URL once per window construction so the loader + // and the navigation guard see a consistent string even if + // `VELLUM_DEV_URL` is mutated mid-process. + // + // The prod load target is the renderer base itself (no `/index.html` + // suffix). The `app://` protocol handler in `index.ts` falls back + // to `index.html` for paths without a file extension, so this + // serves the SPA — but with the browser URL staying at `/assistant`, + // which is where React Router's app-root route matches. Appending + // `/index.html` would land us at the NotFound route under + // `/assistant/*`. + const isDev = !app.isPackaged; + const devBase = isDev ? getDevRendererBase() : null; + const loadTarget = devBase ?? RENDERER_BASE_PROD; + const devOrigin = devBase ? new URL(devBase).origin : ""; + + const win = new BrowserWindow({ + ...restoreBounds("main", { width: 1280, height: 800 }), + show: false, + webPreferences: { + preload: path.join(__dirname, "../preload/index.js"), + contextIsolation: true, + nodeIntegration: false, + sandbox: true, + webSecurity: true, + allowRunningInsecureContent: false, + experimentalFeatures: false, + devTools: !app.isPackaged, + }, + }); + + trackWindowState("main", win); + + // Readiness resolves only after BOTH the renderer has loaded AND + // the window has shown + focused. Per-window state keyed via + // WeakMap so concurrent `createWindow` calls can't cross-resolve + // each other's promise (the prior module-scope `resolveRenderReady` + // had that race). + const ready = armReadyState(win); + const maybeResolveReady = (): void => { + if (ready.didFinishLoad && ready.didShow) ready.resolve(); + }; + win.webContents.once("did-finish-load", () => { + ready.didFinishLoad = true; + maybeResolveReady(); + }); + + win.once("ready-to-show", () => { + win.show(); + win.focus(); + ready.didShow = true; + maybeResolveReady(); + }); + + // Visibility transitions feed the dock state machine (via + // `onMainWindowVisibilityChange`). Subscribed here — not in the + // listener — so dock doesn't need to know how to identify "main". + win.on("show", fireVisibilityChange); + win.on("hide", fireVisibilityChange); + + win.on("closed", () => { + // Unblock any pending `await ensureVisible()` so callers that hit + // the destroyed-before-ready race (network failure during load, + // user quit mid-load) don't hang forever. The caller's follow-up + // dispatch then sees `current() === null` and no-ops; that's the + // right semantics — the user closed the window, nothing should + // happen. + ready.resolve(); + if (mainWindow === win) mainWindow = null; + // Subscribers (dock) re-read `current()` which is now null → + // `isMainWindowVisible()` returns false. + fireVisibilityChange(); + }); + + installSameOriginNavigationGuard(win, { isDev, devOrigin }); + + win.loadURL(loadTarget).catch((err: unknown) => { + console.error(`[main-window] loadURL failed for ${loadTarget}:`, err); + }); + + mainWindow = win; + // Fire AFTER assignment so subscribers see the new window via + // `current()` if they query it. The window itself isn't visible + // yet (created with `show: false`); the `show`/`hide` listeners + // above will drive subsequent transitions. + fireVisibilityChange(); + return win; +}; + +/** + * Recreate if destroyed, restore from minimize, show, focus. Returns + * a Promise that resolves once the new (or re-shown) main window is + * visible AND focused AND its renderer has finished loading, so a + * caller that immediately dispatches an IPC command via + * `dispatchToFocused` (the tray) lands the command on the right + * window. Callers that don't need the renderer (the `activate` + * handler, `second-instance`) can `void` the return. + * + * Residual race: `did-finish-load` fires after the bundle parses but + * before React's effect for `useVellumCommands` mounts. In practice + * the gap is small (~ms) — a proper renderer→main "ready for + * commands" IPC handshake is a separate ticket; for the tray's click + * rate this is good enough. + */ +export const ensureVisible = (): Promise => { + if (!mainWindow || mainWindow.isDestroyed()) { + const win = createWindow(); + return readyStates.get(win)?.promise ?? ALREADY_READY; + } + if (mainWindow.isMinimized()) mainWindow.restore(); + mainWindow.show(); + mainWindow.focus(); + // The existing window may still be mid-load (created very recently; + // `did-finish-load` and/or `ready-to-show` haven't both fired yet), + // so return its stored readiness instead of a fresh resolved + // promise. If it's already loaded, the stored promise has already + // resolved and the await is effectively free. + return readyStates.get(mainWindow)?.promise ?? ALREADY_READY; +}; + +export const hide = (): void => { + if (!mainWindow || mainWindow.isDestroyed()) return; + mainWindow.hide(); +}; + +export const isVisibleAndFocused = (): boolean => + !!mainWindow && + !mainWindow.isDestroyed() && + mainWindow.isVisible() && + mainWindow.isFocused(); + +export const toggleVisibility = (): void => { + if (isVisibleAndFocused()) { + hide(); + return; + } + void ensureVisible(); +}; + +/** + * Underlying `BrowserWindow | null`. Reserved for narrow callers that + * need the actual reference (e.g. `dispatchToFocused`'s + * `webContents.send`, `before-quit` cleanup inspecting the instance). + * Most callers should compose from `ensureVisible` / `hide` instead. + */ +export const current = (): BrowserWindow | null => mainWindow; + +/** + * Send a `vellum:command` IPC message directly to the main window, + * bypassing the application-menu `dispatchToFocused` "find focused + * window" lookup. The tray click happens with the app potentially + * backgrounded, so `getFocusedWindow()` can disagree with what the + * user just clicked on — even after we call `win.focus()`, the OS + * doesn't deliver the focus change synchronously. Targeting the + * main window by reference sidesteps that entirely. + * + * No-op if the main window doesn't currently exist; the caller is + * expected to have run `ensureVisible()` first. + */ +export const dispatchToMain = (command: VellumCommand): void => { + if (!mainWindow || mainWindow.isDestroyed()) return; + mainWindow.webContents.send("vellum:command", command); +}; + +/** + * Create the initial main window. Call once from `whenReady`. + * Idempotent: if a window is already alive, no-op. + */ +let installed = false; +export const installMainWindow = (): void => { + if (installed) return; + installed = true; + void ensureVisible(); +}; diff --git a/apps/macos/src/main/tray.test.ts b/apps/macos/src/main/tray.test.ts new file mode 100644 index 00000000000..7c87ec43eb7 --- /dev/null +++ b/apps/macos/src/main/tray.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, mock, test } from "bun:test"; + +// Capture every Tray instance constructed during a test run so we can +// assert on calls and on idempotency. The constructor proxies records +// into module scope; `installTray` itself is `installed`-gated, so +// across the whole test file we expect exactly one construction. +type TrayCall = { event: string; handler: (...args: unknown[]) => void }; +type StubTray = { + setIgnoreDoubleClickEvents: ReturnType; + setToolTip: ReturnType; + on: (event: string, handler: (...args: unknown[]) => void) => StubTray; + popUpContextMenu: ReturnType; + destroy: ReturnType; + events: TrayCall[]; +}; + +const trays: StubTray[] = []; + +const makeTray = (): StubTray => { + const events: TrayCall[] = []; + const stub: StubTray = { + setIgnoreDoubleClickEvents: mock(() => undefined), + setToolTip: mock(() => undefined), + on: (event, handler) => { + events.push({ event, handler }); + return stub; + }, + popUpContextMenu: mock(() => undefined), + destroy: mock(() => undefined), + events, + }; + return stub; +}; + +const buildFromTemplateMock = mock((_template: unknown) => ({ + popup: () => undefined, +})); + +const setTemplateImageMock = mock((_flag: boolean) => undefined); +const createFromBitmapMock = mock((_buf: unknown, _opts: unknown) => ({ + setTemplateImage: setTemplateImageMock, +})); + +mock.module("electron", () => ({ + app: { + name: "Vellum Electron", + on: () => undefined, + }, + // `./commands` imports `BrowserWindow` at the top level for + // `dispatchToFocused` — provide a stub so the import resolves even + // though our tests don't exercise that code path. + BrowserWindow: class { + static getFocusedWindow() { + return null; + } + static getAllWindows() { + return []; + } + }, + Menu: { + buildFromTemplate: buildFromTemplateMock, + }, + Tray: class { + constructor(_icon: unknown) { + const stub = makeTray(); + trays.push(stub); + Object.assign(this, stub); + } + }, + nativeImage: { + createFromBitmap: createFromBitmapMock, + }, +})); + +// Mock `./settings` so importing `./commands` (which `tray.ts` imports +// transitively for `resolveAccelerator`) doesn't try to construct an +// electron-store instance in the test environment. +mock.module("./settings", () => ({ + readSetting: () => null, +})); + +// Mock `./main-window` so `tray.ts`'s `dispatchToMain` import doesn't +// drag in the full BrowserWindow / window-state / ipc setup. The spy +// is asserted on below. +const dispatchToMainMock = mock((_command: unknown) => undefined); +mock.module("./main-window", () => ({ + dispatchToMain: dispatchToMainMock, +})); + +const { installTray } = await import("./tray"); + +describe("installTray", () => { + const handlers = { + toggleMainWindow: mock(() => undefined), + ensureMainWindow: mock(() => Promise.resolve()), + openAbout: mock(() => undefined), + }; + + test("constructs the Tray, ignores double-clicks, sets a tooltip, registers click and right-click — all on the first call only", () => { + installTray(handlers); + installTray(handlers); + installTray(handlers); + + expect(trays).toHaveLength(1); + const tray = trays[0]; + expect(tray).toBeDefined(); + expect(tray?.setIgnoreDoubleClickEvents).toHaveBeenCalledTimes(1); + expect(tray?.setIgnoreDoubleClickEvents.mock.calls[0]?.[0]).toBe(true); + expect(tray?.setToolTip).toHaveBeenCalledTimes(1); + + const eventNames = tray?.events.map((e) => e.event) ?? []; + expect(eventNames).toContain("click"); + expect(eventNames).toContain("right-click"); + }); + + test("uses a template image so macOS can auto-invert for dark mode", () => { + expect(setTemplateImageMock).toHaveBeenCalled(); + expect(setTemplateImageMock.mock.calls[0]?.[0]).toBe(true); + }); + + test("left-click routes through the toggleMainWindow handler", () => { + const tray = trays[0]; + const clickHandler = tray?.events.find((e) => e.event === "click")?.handler; + expect(clickHandler).toBeDefined(); + const before = handlers.toggleMainWindow.mock.calls.length; + clickHandler?.(); + expect(handlers.toggleMainWindow.mock.calls.length).toBe(before + 1); + }); + + test("right-click pops the context menu", () => { + const tray = trays[0]; + const rightClickHandler = tray?.events.find( + (e) => e.event === "right-click", + )?.handler; + expect(rightClickHandler).toBeDefined(); + const before = tray?.popUpContextMenu.mock.calls.length ?? 0; + rightClickHandler?.(); + expect(tray?.popUpContextMenu.mock.calls.length).toBe(before + 1); + }); + + test("builds a menu containing the canonical tray actions", () => { + expect(buildFromTemplateMock).toHaveBeenCalledTimes(1); + const template = buildFromTemplateMock.mock.calls[0]?.[0] as Array<{ + label?: string; + role?: string; + type?: string; + }>; + const labels = template.map((item) => item.label).filter(Boolean); + expect(labels).toContain("New Conversation"); + expect(labels).toContain("Current Conversation"); + expect(labels).toContain("Show / Hide Main Window"); + expect(labels).toContain("About Vellum Electron"); + expect(labels).toContain("Quit Vellum Electron"); + + const quitItem = template.find((item) => item.label?.startsWith("Quit")); + expect(quitItem?.role).toBe("quit"); + }); + + test("conversation menu items surface the main window before dispatching the renderer command", async () => { + const template = buildFromTemplateMock.mock.calls[0]?.[0] as Array<{ + label?: string; + click?: () => void | Promise; + }>; + const newConversation = template.find( + (item) => item.label === "New Conversation", + ); + const currentConversation = template.find( + (item) => item.label === "Current Conversation", + ); + expect(newConversation?.click).toBeDefined(); + expect(currentConversation?.click).toBeDefined(); + + const beforeEnsure = handlers.ensureMainWindow.mock.calls.length; + const beforeDispatch = dispatchToMainMock.mock.calls.length; + + await newConversation?.click?.(); + await currentConversation?.click?.(); + + expect(handlers.ensureMainWindow.mock.calls.length).toBe(beforeEnsure + 2); + expect(dispatchToMainMock.mock.calls.length).toBe(beforeDispatch + 2); + expect(dispatchToMainMock.mock.calls[beforeDispatch]?.[0]).toEqual({ + kind: "newConversation", + }); + expect(dispatchToMainMock.mock.calls[beforeDispatch + 1]?.[0]).toEqual({ + kind: "currentConversation", + }); + }); +}); diff --git a/apps/macos/src/main/tray.ts b/apps/macos/src/main/tray.ts new file mode 100644 index 00000000000..d0590b45ba0 --- /dev/null +++ b/apps/macos/src/main/tray.ts @@ -0,0 +1,183 @@ +import { Menu, Tray, app, nativeImage, type NativeImage } from "electron"; + +import { resolveAccelerator } from "./commands"; +import { dispatchToMain } from "./main-window"; + +/** + * macOS menu-bar (Tray) status item. + * + * Mirrors what the Swift app's `NSStatusItem` does in + * `AppDelegate+MenuBar.swift`: persistent menu-bar icon, single-click + * toggles the main window, right-click pops a quick-actions menu. The + * 5-state pulse status indicator that Swift renders on top of the icon + * lands in a follow-up — this PR puts the icon + click wiring in place + * so the rest of the app feels like Swift Vellum. + * + * Implementation notes carried over from `apps/macos/docs/PATTERNS.md` + * (state ownership) and the Electron tray gotchas: + * + * - Don't call `tray.setContextMenu()`. With it, left and right click + * both open the same menu — overriding the documented Linear / + * menu-bar-app pattern of "click toggles, right-click opens menu." + * Instead, register `click` + `right-click` listeners and call + * `tray.popUpContextMenu(menu)` manually from the right-click path. + * - `tray.setIgnoreDoubleClickEvents(true)` so two fast single clicks + * are treated as two `click` events instead of being coalesced into + * a swallowed double-click on macOS. + * - Template images only (PNG with alpha; black-on-transparent). + * macOS auto-inverts for dark mode and pressed state; colored icons + * look wrong against accent tints + * (https://github.com/electron/electron/issues/19006). + * - Hold a module-scope `Tray` reference. Without it Node's GC can + * collect the JS handle and the icon disappears from the menu bar + * even though the underlying NSStatusItem is still alive. + */ + +export interface TrayHandlers { + /** + * Bound to the tray's left click and the "Show / Hide Main Window" + * menu item: if the main window is visible and focused, hide it; + * otherwise show + focus + (recreate if previously destroyed). + */ + toggleMainWindow(): void; + /** + * Bound to the conversation menu items below. Renderer-bound + * commands (`newConversation`, `currentConversation`) only update + * state — without surfacing the window first, nothing visible + * happens when the user picks them from the tray. Returns a Promise + * that resolves once the renderer has finished loading, so the + * dispatched command isn't dropped on the floor if the BrowserWindow + * was just recreated. + */ + ensureMainWindow(): Promise; + /** + * Open (or focus the existing) About window. + */ + openAbout(): void; +} + +const ICON_SIZE = 16; + +/** + * Placeholder template icon — a thin outlined circle. The assistant + * avatar + status-dot rendering lands with the pulse-animation ticket + * and the assistant-identity bridge; until then, an outlined circle is + * enough that the menu bar shows *something* and the user can find the + * app. + * + * Generated programmatically so this PR ships no binary assets. The + * follow-up that introduces real avatar/state-dot artwork will switch + * to `nativeImage.createFromPath(...)` against shipped PNG resources. + */ +const buildPlaceholderIcon = (): NativeImage => { + const buf = Buffer.alloc(ICON_SIZE * ICON_SIZE * 4); + const center = ICON_SIZE / 2 - 0.5; + for (let y = 0; y < ICON_SIZE; y++) { + for (let x = 0; x < ICON_SIZE; x++) { + const dx = x - center; + const dy = y - center; + const dist = Math.sqrt(dx * dx + dy * dy); + // Anti-aliased ring at radius ~6 with width ~1.5px. Alpha drops + // off at the inner + outer edges so the icon doesn't look pixelated + // at standard density. + const inner = Math.max(0, Math.min(1, dist - 4.5)); + const outer = Math.max(0, Math.min(1, 6 - dist)); + const alpha = Math.round(255 * Math.min(inner, outer)); + const offset = (y * ICON_SIZE + x) * 4; + buf[offset + 0] = 0; + buf[offset + 1] = 0; + buf[offset + 2] = 0; + buf[offset + 3] = alpha; + } + } + const img = nativeImage.createFromBitmap(buf, { + width: ICON_SIZE, + height: ICON_SIZE, + }); + img.setTemplateImage(true); + return img; +}; + +const buildTrayMenu = (handlers: TrayHandlers): Menu => + Menu.buildFromTemplate([ + { + label: "New Conversation", + accelerator: resolveAccelerator("newConversation"), + click: async () => { + await handlers.ensureMainWindow(); + // Dispatch by reference (not `dispatchToFocused`'s + // `getFocusedWindow` lookup) — the tray click happens with + // the app potentially backgrounded, so even after our + // `win.focus()` the OS may not have delivered focus by the + // time this runs. Targeting main directly is unambiguous. + dispatchToMain({ kind: "newConversation" }); + }, + }, + { + label: "Current Conversation", + accelerator: resolveAccelerator("currentConversation"), + click: async () => { + await handlers.ensureMainWindow(); + dispatchToMain({ kind: "currentConversation" }); + }, + }, + { type: "separator" }, + { + label: "Show / Hide Main Window", + click: handlers.toggleMainWindow, + }, + { type: "separator" }, + { + label: `About ${app.name}`, + click: handlers.openAbout, + }, + { type: "separator" }, + { + label: `Quit ${app.name}`, + // `role: "quit"` carries its own accelerator on macOS; we still + // declare it explicitly so the menu reads consistently across + // locales and Electron version bumps. + accelerator: "CmdOrCtrl+Q", + role: "quit", + }, + ]); + +let installed = false; +let trayInstance: Tray | null = null; + +/** + * Wire the menu-bar status item. Call once from `whenReady`. Idempotent + * — repeated calls are no-ops, so it's safe under hot-reload of the + * main bundle in dev. + * + * The handlers are passed in (rather than imported) so the tray module + * stays decoupled from `index.ts`'s lifecycle state. The main process + * is the only place that knows what "toggle the main window" means + * today, and that knowledge stays there. + */ +export const installTray = (handlers: TrayHandlers): void => { + if (installed) return; + installed = true; + + trayInstance = new Tray(buildPlaceholderIcon()); + trayInstance.setIgnoreDoubleClickEvents(true); + trayInstance.setToolTip(app.name); + + trayInstance.on("click", () => { + handlers.toggleMainWindow(); + }); + + const menu = buildTrayMenu(handlers); + trayInstance.on("right-click", () => { + trayInstance?.popUpContextMenu(menu); + }); + + // Explicit destroy on quit. In production the OS releases the + // NSStatusItem when the process exits anyway; in dev with main-process + // hot reload, freeing the JS handle ourselves avoids a ghost menu-bar + // icon for a beat between reloads. + app.on("before-quit", () => { + trayInstance?.destroy(); + trayInstance = null; + }); +}; diff --git a/apps/macos/test-setup.ts b/apps/macos/test-setup.ts index 68a522e6edb..61a54ca1d1d 100644 --- a/apps/macos/test-setup.ts +++ b/apps/macos/test-setup.ts @@ -55,6 +55,19 @@ mock.module("electron", () => ({ buildFromTemplate: () => ({ popup: () => undefined }), setApplicationMenu: () => undefined, }, + Tray: class { + setIgnoreDoubleClickEvents() {} + setToolTip() {} + on() { + return this; + } + popUpContextMenu() {} + destroy() {} + }, + nativeImage: { + createFromBitmap: () => ({ setTemplateImage: () => undefined }), + createFromPath: () => ({ setTemplateImage: () => undefined }), + }, protocol: { handle: () => undefined, registerSchemesAsPrivileged: () => undefined,