From 096cf1dee874184ee81bb5d1326d5912db472ef7 Mon Sep 17 00:00:00 2001 From: CurlyPeter <56999118+CurlyPeter@users.noreply.github.com> Date: Sun, 23 Aug 2026 13:53:57 +0200 Subject: [PATCH] fix(desktop): route t3code:// thread deep links instead of only raising the window The app registers the t3code:// scheme (Info.plist on macOS, the Clerk bridge's setAsDefaultProtocolClient, the Linux .desktop entry) but routes exactly one URL family: Clerk's OAuth callback. Every other URL is accepted by the OS, delivered to the app and dropped, so opening a thread link brings the window to the front and leaves it wherever it already was. The renderer has had the route all along (/$environmentId/$threadId) and nothing connects the two. This accepts the shape the relay already emits for its push notifications (deepLink: "/threads//") and the mobile router already matches, so one link opens the same chat on a phone and on a desktop. Delivery reuses the existing menu-action channel rather than adding one: dispatchMenuAction already creates the window when there is none, defers the send until did-finish-load while the renderer boots, and reveals the window afterwards. A dedicated channel would have duplicated all of it plus a preload entry. The renderer rule lives in its own module so the decision is testable without rendering and AppSidebarLayout keeps a single call. The cold-start path needed three supporting fixes, because the launch is precisely when nothing is ready yet: - open-url is captured synchronously at process bootstrap. Electron buffers nothing and documents that a listener registered in response to `ready` misses the URL that launched the app; even registering before whenReady sits behind the whole layer graph plus the shell environment, user-data path, settings load and Clerk bootstrap, each of which yields to the run loop. - The preload buffers menu actions until the renderer subscribes. onMenuAction attaches its IPC listener lazily and its only caller renders inside the authenticated app shell, which is gated on an async bootstrap, while the main process sends on did-finish-load, which does not wait for it. On a cold start the action was otherwise sent into a channel with no listener. - DesktopWindow parks a menu action that arrives with no window and no ready backend, and replays it from handleBackendReady through the same delivery path. That state is invisible for a menu item, since the user can click again, and terminal for a launch URL. The drain sits in `ensuring` so a failed window creation cannot strand it. Parsing is deliberately at least as strict as the mobile app's normalizeThreadDeepLink: no query, no fragment, no credentials, exact segment count, and both ids must be UUIDs. The UUID requirement is a boundary rather than a format preference, because a URL handler is an unauthenticated entry point: without it t3code://threads/settings/connections resolves to /settings/connections, a real static route that outranks the thread route, so any local process could drive the window onto a settings page. The listener declines quietly for anything that is not a thread link and calls preventDefault only for URLs it owns, so Clerk's callbacks are untouched. open-url is macOS-only; Windows and Linux deliver the URL as argv to a second instance, and wiring that path is left for a follow-up rather than guessed at. --- apps/desktop/src/app/DesktopApp.ts | 7 + .../desktop/src/app/DesktopUrlRouting.test.ts | 156 +++++++++++ apps/desktop/src/app/DesktopUrlRouting.ts | 261 ++++++++++++++++++ apps/desktop/src/main.ts | 7 + apps/desktop/src/preload.ts | 18 ++ apps/desktop/src/window/DesktopWindow.test.ts | 32 +++ apps/desktop/src/window/DesktopWindow.ts | 71 ++++- apps/web/src/components/AppSidebarLayout.tsx | 10 + apps/web/src/menuActionNavigation.test.ts | 57 ++++ apps/web/src/menuActionNavigation.ts | 41 +++ 10 files changed, 645 insertions(+), 15 deletions(-) create mode 100644 apps/desktop/src/app/DesktopUrlRouting.test.ts create mode 100644 apps/desktop/src/app/DesktopUrlRouting.ts create mode 100644 apps/web/src/menuActionNavigation.test.ts create mode 100644 apps/web/src/menuActionNavigation.ts diff --git a/apps/desktop/src/app/DesktopApp.ts b/apps/desktop/src/app/DesktopApp.ts index 4101840530f6..f004e555eac9 100644 --- a/apps/desktop/src/app/DesktopApp.ts +++ b/apps/desktop/src/app/DesktopApp.ts @@ -22,6 +22,7 @@ import * as DesktopLinuxUrlHandler from "./DesktopLinuxUrlHandler.ts"; import * as DesktopObservability from "./DesktopObservability.ts"; import * as DesktopPreReadyPlatform from "./DesktopPreReadyPlatform.ts"; import * as DesktopShutdown from "./DesktopShutdown.ts"; +import * as DesktopUrlRouting from "./DesktopUrlRouting.ts"; import * as DesktopServerExposure from "../backend/DesktopServerExposure.ts"; import * as DesktopAppSettings from "../settings/DesktopAppSettings.ts"; import * as DesktopShellEnvironment from "../shell/DesktopShellEnvironment.ts"; @@ -224,6 +225,7 @@ const startup = Effect.gen(function* () { const electronApp = yield* ElectronApp.ElectronApp; const lifecycle = yield* DesktopLifecycle.DesktopLifecycle; const linuxUrlHandler = yield* DesktopLinuxUrlHandler.DesktopLinuxUrlHandler; + const urlRouting = yield* DesktopUrlRouting.DesktopUrlRouting; const clerk = yield* DesktopClerk.DesktopClerk; const shellEnvironment = yield* DesktopShellEnvironment.DesktopShellEnvironment; const desktopSettings = yield* DesktopAppSettings.DesktopAppSettings; @@ -271,6 +273,11 @@ const startup = Effect.gen(function* () { yield* appIdentity.configure; yield* lifecycle.register; yield* clerk.configure; + // BEFORE whenReady, deliberately. On macOS a cold-start `open-url` (the OS + // launching the app because the user opened a t3code:// link) can fire before + // the ready event, and a listener installed after it would never see the URL + // that caused the launch. + yield* urlRouting.register; yield* electronApp.whenReady.pipe( Effect.withSpan("desktop.electron.whenReady"), diff --git a/apps/desktop/src/app/DesktopUrlRouting.test.ts b/apps/desktop/src/app/DesktopUrlRouting.test.ts new file mode 100644 index 000000000000..af6ba37ddc79 --- /dev/null +++ b/apps/desktop/src/app/DesktopUrlRouting.test.ts @@ -0,0 +1,156 @@ +import { assert, describe, it } from "@effect/vitest"; + +import { + NAVIGATE_ACTION_PREFIX, + navigateAction, + parseThreadDeepLink, + threadRoutePath, +} from "./DesktopUrlRouting.ts"; + +const SCHEME = "t3code"; +const ENV = "11111111-2222-3333-4444-555555555555"; +const THREAD = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + +describe("parseThreadDeepLink", () => { + it("parses the shape the relay and the mobile router already use", () => { + assert.deepEqual(parseThreadDeepLink(`${SCHEME}://threads/${ENV}/${THREAD}`, SCHEME), { + environmentId: ENV, + threadId: THREAD, + }); + }); + + it("accepts the empty-authority form, where threads lands in the path", () => { + assert.deepEqual(parseThreadDeepLink(`${SCHEME}:///threads/${ENV}/${THREAD}`, SCHEME), { + environmentId: ENV, + threadId: THREAD, + }); + }); + + it("accepts uppercase uuids", () => { + assert.deepEqual( + parseThreadDeepLink(`${SCHEME}://threads/${ENV.toUpperCase()}/${THREAD}`, SCHEME), + { environmentId: ENV.toUpperCase(), threadId: THREAD }, + ); + }); + + /** + * The security case, and the reason both ids must be UUIDs. Without the shape + * constraint this resolves to `/settings/connections`, which is a real static + * route and outranks `/$environmentId/$threadId` — so any local process could + * drive the window onto a settings page through an unauthenticated OS entry + * point. + */ + it("declines ids that are not uuids, so a link cannot reach a static route", () => { + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/settings/connections`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/projects/some-key`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/${ENV}/not-a-uuid`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/env%20one/thread%2Ftwo`, SCHEME)); + }); + + it("declines credentials in the authority", () => { + assert.isNull(parseThreadDeepLink(`${SCHEME}://user@threads/${ENV}/${THREAD}`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://user:pw@threads/${ENV}/${THREAD}`, SCHEME)); + }); + + it("declines a malformed percent escape instead of throwing", () => { + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/${ENV}/%E0%A4%A`, SCHEME)); + }); + + /** + * The invariant is that this side is never LOOSER than mobile. Filtering + * empty segments would have been, so the split keeps them and an extra or + * trailing slash is a rejection. + */ + it("declines an extra or trailing slash, exactly as the mobile parser does", () => { + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/${ENV}//${THREAD}`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/${ENV}/${THREAD}/`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}:///threads/${ENV}/${THREAD}/`, SCHEME)); + }); + + /** + * The one knowing divergence: WHATWG normalises `..` away before this sees + * the path, so it resolves to the same thread here while mobile, which splits + * the raw string, rejects it. Asserted so the difference is deliberate rather + * than discovered later. + */ + it("accepts a dot-segment that normalisation already removed", () => { + assert.deepEqual(parseThreadDeepLink(`${SCHEME}://threads/../${ENV}/${THREAD}`, SCHEME), { + environmentId: ENV, + threadId: THREAD, + }); + }); + + // Every open-url listener sees every URL delivered to the app, so declining + // has to be the quiet, normal path rather than an error. + it("declines a Clerk OAuth callback on the same scheme", () => { + assert.isNull(parseThreadDeepLink(`${SCHEME}://app/oauth/callback?code=abc`, SCHEME)); + }); + + it("declines another scheme", () => { + assert.isNull(parseThreadDeepLink(`https://threads/${ENV}/${THREAD}`, SCHEME)); + assert.isNull(parseThreadDeepLink(`t3code-dev://threads/${ENV}/${THREAD}`, SCHEME)); + }); + + it("declines a wrong host", () => { + assert.isNull(parseThreadDeepLink(`${SCHEME}://app/${ENV}/${THREAD}`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://thread/${ENV}/${THREAD}`, SCHEME)); + }); + + it("declines the wrong number of segments", () => { + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/${ENV}`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/${ENV}/${THREAD}/terminal`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads`, SCHEME)); + }); + + // Mirrors the mobile app's normalizeThreadDeepLink, which rejects both. + it("declines a query or a fragment", () => { + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/${ENV}/${THREAD}?x=1`, SCHEME)); + assert.isNull(parseThreadDeepLink(`${SCHEME}://threads/${ENV}/${THREAD}#top`, SCHEME)); + }); + + it("declines an unparseable url", () => { + assert.isNull(parseThreadDeepLink("not a url", SCHEME)); + assert.isNull(parseThreadDeepLink("", SCHEME)); + }); + + it("respects the development scheme", () => { + assert.deepEqual(parseThreadDeepLink(`t3code-dev://threads/${ENV}/${THREAD}`, "t3code-dev"), { + environmentId: ENV, + threadId: THREAD, + }); + }); +}); + +describe("threadRoutePath", () => { + // The web routes a chat WITHOUT the threads/ prefix the link carries; that + // translation is the whole job of this function. + it("drops the threads prefix the link carries", () => { + assert.strictEqual( + threadRoutePath({ environmentId: ENV, threadId: THREAD }), + `/${ENV}/${THREAD}`, + ); + }); + + it("encodes ids that would otherwise break the path", () => { + assert.strictEqual(threadRoutePath({ environmentId: "a/b", threadId: "c d" }), "/a%2Fb/c%20d"); + }); +}); + +describe("navigateAction", () => { + it("prefixes the path so the renderer can tell it from a menu item", () => { + assert.strictEqual(navigateAction("/a/b"), `${NAVIGATE_ACTION_PREFIX}/a/b`); + }); + + /** + * The renderer declares the same prefix separately, in + * `apps/web/src/menuActionNavigation.ts`, because the renderer bundle must + * not import from the Electron main process and the two packages are separate + * TypeScript projects. Nothing else would notice if they drifted: both suites + * would stay green and deep links would silently stop working with nothing + * pointing at the cause. Both sides pin the literal instead, so whichever one + * is edited fails its own test. + */ + it("pins the literal the renderer also pins", () => { + assert.strictEqual(NAVIGATE_ACTION_PREFIX, "navigate:"); + }); +}); diff --git a/apps/desktop/src/app/DesktopUrlRouting.ts b/apps/desktop/src/app/DesktopUrlRouting.ts new file mode 100644 index 000000000000..c08cb42314fc --- /dev/null +++ b/apps/desktop/src/app/DesktopUrlRouting.ts @@ -0,0 +1,261 @@ +import * as Context from "effect/Context"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Scope from "effect/Scope"; + +import type * as Electron from "electron"; + +import * as ElectronApp from "../electron/ElectronApp.ts"; +import * as ElectronProtocol from "../electron/ElectronProtocol.ts"; +import * as DesktopWindow from "../window/DesktopWindow.ts"; +import * as DesktopEnvironment from "./DesktopEnvironment.ts"; +import { makeComponentLogger } from "./DesktopObservability.ts"; + +// The app already registers the t3code:// scheme (Info.plist on macOS, the +// Clerk bridge's setAsDefaultProtocolClient, the .desktop entry on Linux) and +// already routes one URL family: Clerk's OAuth callback. Everything else was +// accepted by the OS, delivered to the app and dropped, so opening a thread +// link raised the window and left it wherever it happened to be. +// +// The relay already emits exactly this shape for its push notifications +// (`deepLink: "/threads//"`) and the mobile app routes +// it, so a link that opens a chat on the phone should open the same chat here +// rather than being a second, desktop-only format. +const THREAD_HOST = "threads"; +const THREAD_PATH_SEGMENT = "threads"; + +// Both ids are UUIDs everywhere they are minted (thread ids in the orchestrator, +// environment ids in the environment descriptor), and requiring that shape is +// what makes this parse TOTAL rather than merely plausible. +// +// It is a security boundary, not a formatting preference. A URL handler is an +// unauthenticated entry point: any local process can hand the app a t3code:// +// URL. Without a shape constraint, `t3code://threads/settings/connections` +// resolves to the two-segment path `/settings/connections`, which is a real +// static route and wins over `/$environmentId/$threadId`, so anything on the +// machine could silently drive the window onto a settings page. A UUID pair can +// never collide with a static route. +// +// It also makes the parse strictly narrower than the mobile app's, which closes +// the cases where WHATWG normalisation would otherwise let something through +// that `normalizeThreadDeepLink` rejects (`//`, a trailing slash, a `..` +// segment): none of those survive as a UUID. +const UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +const { logInfo, logWarning } = makeComponentLogger("desktop-url-routing"); + +export interface ThreadDeepLink { + readonly environmentId: string; + readonly threadId: string; +} + +/** + * The renderer path a thread deep link resolves to. + * + * The web routes a chat as `/$environmentId/$threadId` (see + * `apps/web/src/routes/_chat.$environmentId.$threadId.tsx`), WITHOUT the + * `threads/` prefix the incoming URL carries. The prefix exists in the link + * because that is what the mobile router and the relay payload use; translating + * it here keeps that one difference in one place. + */ +export function threadRoutePath(link: ThreadDeepLink): string { + return `/${encodeURIComponent(link.environmentId)}/${encodeURIComponent(link.threadId)}`; +} + +/** + * Parse `t3code://threads//`, or `null` if the URL is + * anything else. + * + * `null` is the normal answer, not an error: every `open-url` listener sees + * every URL delivered to the app, including Clerk's OAuth callbacks, so this + * has to decline politely rather than warn. + * + * Strictness is modelled on the mobile app's `normalizeThreadDeepLink` + * (`apps/mobile/src/features/agent-awareness/notificationPayload.ts`) and is + * deliberately **at least as strict**, never looser: no query, no fragment, no + * credentials, exactly two segments, and both of them UUIDs. Being looser is + * what would matter, because a link the phone accepts and the desktop rejects + * is a bug with nothing to say which side is wrong; the reverse is only a + * narrower door. Mobile splits the raw string while this goes through WHATWG + * parsing, which normalises `//`, a trailing slash and `..` away, so the UUID + * requirement is what keeps those from becoming an accidental widening. + * + * One divergence is accepted knowingly: `t3code://threads/..//` + * is normalised by WHATWG parsing before this sees it, so it is accepted here + * and rejected by mobile, which splits the raw string. It resolves to the same + * thread, so the door is not wider in any meaningful sense; matching mobile + * there would mean re-parsing the raw string by hand for no gain. + * + * Both authority forms are accepted because both are what people actually + * produce: `t3code://threads/a/b` puts `threads` in the host, while + * `t3code:///threads/a/b` leaves the host empty and puts it in the path. + */ +export function parseThreadDeepLink(rawUrl: string, scheme: string): ThreadDeepLink | null { + let url: URL; + try { + url = new URL(rawUrl); + } catch { + return null; + } + + if (url.protocol !== `${scheme}:`) return null; + if (url.search !== "" || url.hash !== "") return null; + // Credentials in the authority would make `t3code://user@threads/a/b` parse + // with host "threads"; the mobile parser never sees an authority at all. + if (url.username !== "" || url.password !== "") return null; + + // Split without discarding empties, exactly as the mobile parser does, so an + // extra or trailing slash is a rejection rather than something normalised + // quietly away. Filtering empty segments here was looser than mobile and is + // the kind of asymmetry this parser exists to avoid. + const parts = url.pathname.split("/"); + const rest = + url.host === THREAD_HOST + ? parts.length === 3 && parts[0] === "" + ? parts.slice(1) + : null + : url.host === "" && parts.length === 4 && parts[0] === "" && parts[1] === THREAD_PATH_SEGMENT + ? parts.slice(2) + : null; + + if (rest === null) return null; + + let environmentId: string; + let threadId: string; + try { + environmentId = decodeURIComponent(rest[0] ?? ""); + threadId = decodeURIComponent(rest[1] ?? ""); + } catch { + return null; + } + + if (!UUID_PATTERN.test(environmentId) || !UUID_PATTERN.test(threadId)) return null; + return { environmentId, threadId }; +} + +/** + * The action string carried over the existing menu-action channel. + * + * Deliberately reusing that channel rather than adding a new one: it already + * exists on both sides, its payload is a plain string, and `dispatchMenuAction` + * already solves everything hard about delivery (create the window if there is + * none, wait for `did-finish-load` if the renderer is still booting, reveal the + * window afterwards). A second channel would duplicate all of it. + */ +export const NAVIGATE_ACTION_PREFIX = "navigate:"; + +export function navigateAction(path: string): string { + return `${NAVIGATE_ACTION_PREFIX}${path}`; +} + +/** + * URLs the OS delivered before the service existed, and the collector that + * catches them. + * + * Electron's guidance is to register `open-url` **early in application + * startup**, because "if you register the listener in response to a `ready` + * event, you'll miss URLs that trigger the launch of your application". Nothing + * in Electron buffers them. Registering inside the Effect startup is not early + * enough: by then the whole layer graph is built and the shell environment, the + * user-data path, the settings load and the Clerk bootstrap have each awaited, + * every one of which yields to the run loop where the launch event is + * dispatched. + * + * So the listener that catches a launch parks raw strings and nothing else, and + * `register` drains them once there is something able to route. + */ +const launchUrls: string[] = []; +let stopCapturingLaunchUrls: (() => void) | null = null; + +/** + * Start collecting `open-url` events immediately. Call once, synchronously, + * from the process entrypoint before any async work happens. + * + * Takes the app rather than importing electron, so this module stays importable + * in tests without an Electron runtime. + */ +export function captureLaunchUrlsSync(app: Electron.App): void { + if (stopCapturingLaunchUrls !== null) return; + const listener = (_event: Electron.Event, url: string) => { + launchUrls.push(url); + }; + app.on("open-url", listener); + stopCapturingLaunchUrls = () => app.removeListener("open-url", listener); +} + +export class DesktopUrlRouting extends Context.Service< + DesktopUrlRouting, + { + /** Install the `open-url` listener. Scoped: the listener dies with the app scope. */ + readonly register: Effect.Effect; + } +>()("@t3tools/desktop/app/DesktopUrlRouting") {} + +export const make = Effect.gen(function* () { + const environment = yield* DesktopEnvironment.DesktopEnvironment; + const electronApp = yield* ElectronApp.ElectronApp; + const desktopWindow = yield* DesktopWindow.DesktopWindow; + const context = yield* Effect.context(); + const runPromise = Effect.runPromiseWith(context); + + const scheme = ElectronProtocol.getDesktopScheme(environment.isDevelopment); + + const handleUrl = Effect.fn("desktop.urlRouting.handleUrl")(function* (rawUrl: string) { + const link = parseThreadDeepLink(rawUrl, scheme); + if (link === null) { + // Not ours. Clerk's OAuth callback arrives on the same event and is + // handled by its own listener, so silence is correct here. + return; + } + const path = threadRoutePath(link); + yield* logInfo("routing thread deep link", { + environmentId: link.environmentId, + threadId: link.threadId, + }); + yield* desktopWindow.dispatchMenuAction(navigateAction(path)).pipe( + // A deep link must never take the app down: the window layer already logs + // its own failures, and an unroutable link is a no-op, not a fatal + // condition. `catchCause`, not `catch`: the latter covers only the typed + // error channel, so a DEFECT under window creation would escape it, reach + // the `runPromise` below as an unhandled rejection and take the main + // process with it. + Effect.catchCause((cause) => + logWarning("failed to route thread deep link", { cause: String(cause) }), + ), + ); + }); + + const register = Effect.gen(function* () { + // `open-url` is macOS only. Windows and Linux deliver the URL as argv to a + // second instance, which the Clerk bridge's single-instance lock already + // forwards; wiring that path is a separate change and is deliberately not + // guessed at here. + yield* electronApp.on<[Electron.Event, string]>("open-url", (event, url) => { + // preventDefault only for URLs that are ours, so Clerk's listener still + // sees its own callbacks untouched. + if (parseThreadDeepLink(url, scheme) !== null) { + event.preventDefault(); + } + // The Effect absorbs its own failures; this guards the promise boundary + // itself, where an unhandled rejection is fatal to the main process. + void runPromise(handleUrl(url)).catch(() => {}); + }); + + // Hand over from the bootstrap collector, in this order: the real listener + // is live BEFORE the collector goes away, so nothing falls between the two. + // A URL arriving in that overlap is routed twice, which is one navigation + // to the same thread and therefore harmless. + stopCapturingLaunchUrls?.(); + stopCapturingLaunchUrls = null; + const buffered = launchUrls.splice(0, launchUrls.length); + for (const url of buffered) { + yield* handleUrl(url); + } + + yield* logInfo("url routing registered", { scheme, buffered: buffered.length }); + }).pipe(Effect.withSpan("desktop.urlRouting.register")); + + return DesktopUrlRouting.of({ register }); +}); + +export const layer = Layer.effect(DesktopUrlRouting, make); diff --git a/apps/desktop/src/main.ts b/apps/desktop/src/main.ts index 14caeed8a9a1..0404d0172e80 100644 --- a/apps/desktop/src/main.ts +++ b/apps/desktop/src/main.ts @@ -45,6 +45,7 @@ import * as DesktopEnvironment from "./app/DesktopEnvironment.ts"; import * as DesktopLifecycle from "./app/DesktopLifecycle.ts"; import * as DesktopLinuxUrlHandler from "./app/DesktopLinuxUrlHandler.ts"; import * as DesktopShutdown from "./app/DesktopShutdown.ts"; +import * as DesktopUrlRouting from "./app/DesktopUrlRouting.ts"; import * as DesktopObservability from "./app/DesktopObservability.ts"; import * as DesktopServerExposure from "./backend/DesktopServerExposure.ts"; import * as DesktopClientSettings from "./settings/DesktopClientSettings.ts"; @@ -64,6 +65,11 @@ import * as DesktopWslBackend from "./wsl/DesktopWslBackend.ts"; import * as DesktopWslEnvironment from "./wsl/DesktopWslEnvironment.ts"; import * as DesktopWslServerTree from "./wsl/DesktopWslServerTree.ts"; +// Synchronously, before any Effect runs: macOS dispatches the `open-url` that +// LAUNCHED the app during early startup, and Electron buffers nothing. See +// DesktopUrlRouting for why registering inside the Effect startup is too late. +DesktopUrlRouting.captureLaunchUrlsSync(Electron.app); + const desktopEnvironmentLayer = Layer.unwrap( Effect.gen(function* () { const metadata = yield* Effect.service(ElectronApp.ElectronApp).pipe( @@ -186,6 +192,7 @@ const desktopApplicationLayer = Layer.mergeAll( DesktopLifecycle.layer, DesktopApplicationMenu.layer, DesktopLinuxUrlHandler.layer, + DesktopUrlRouting.layer, DesktopShellEnvironment.layer, desktopSshLayer, ).pipe( diff --git a/apps/desktop/src/preload.ts b/apps/desktop/src/preload.ts index 407c7c3ef498..cfbab1bca80f 100644 --- a/apps/desktop/src/preload.ts +++ b/apps/desktop/src/preload.ts @@ -27,6 +27,24 @@ function unwrapEnsureSshEnvironmentResult(result: unknown) { return result as Awaited>; } +// Menu actions that arrived before the renderer subscribed. +// +// `onMenuAction` attaches its IPC listener lazily, when the app calls it, and +// the only caller renders inside the authenticated app shell. Authentication is +// resolved by an async bootstrap, while the main process sends on +// `did-finish-load`, which is the page load event and does not wait for it. In +// the steady state that gap does not exist, because the subscriber has been +// live for a long time. It exists exactly once, on the path this buffer was +// added for: the OS launching the app from a deep link, where the navigation +// would otherwise be sent into a channel nobody is listening on and lost. +// +// The preload runs before any page script, so a listener installed here cannot +// miss anything. +const pendingMenuActions: string[] = []; +ipcRenderer.on(IpcChannels.MENU_ACTION_CHANNEL, (_event, action: unknown) => { + if (typeof action === "string") pendingMenuActions.push(action); +}); + contextBridge.exposeInMainWorld("desktopBridge", { getAppBranding: () => { const result = ipcRenderer.sendSync(IpcChannels.GET_APP_BRANDING_CHANNEL); diff --git a/apps/desktop/src/window/DesktopWindow.test.ts b/apps/desktop/src/window/DesktopWindow.test.ts index 036eddd8db78..93e8d25f223c 100644 --- a/apps/desktop/src/window/DesktopWindow.test.ts +++ b/apps/desktop/src/window/DesktopWindow.test.ts @@ -1235,4 +1235,36 @@ describe("DesktopWindow", () => { }).pipe(Effect.provide(scenario.layer)); }), ); + + /** + * The park-and-replay path, which is what makes a URL that LAUNCHED the app + * work: at that moment there is no window and the backend is not ready, and + * the action has no second chance the way a menu item does. The test above + * only asserts that nothing is sent in that state, which passes equally well + * if the action is silently dropped. + */ + it.effect("replays a menu action parked before the backend was ready", () => + Effect.gen(function* () { + const splash = makeFakeBrowserWindow(); + const main = makeFakeBrowserWindow(); + const scenario = yield* makeSplashScenario([splash.window, main.window]); + + yield* Effect.gen(function* () { + const desktopWindow = yield* DesktopWindow.DesktopWindow; + + yield* desktopWindow.showConnectingSplash; + yield* desktopWindow.dispatchMenuAction("navigate:/env/thread"); + assert.equal(main.send.mock.calls.length, 0); + + yield* desktopWindow.handleBackendReady(new URL("http://127.0.0.1:3773")); + assert.deepEqual(main.send.mock.calls, [[MENU_ACTION_CHANNEL, "navigate:/env/thread"]]); + + // Taken, not read: a second readiness (a backend restart) must not + // resurrect a navigation the user asked for minutes ago. + yield* desktopWindow.handleBackendNotReady; + yield* desktopWindow.handleBackendReady(new URL("http://127.0.0.1:3773")); + assert.equal(main.send.mock.calls.length, 1); + }).pipe(Effect.provide(scenario.layer)); + }), + ); }); diff --git a/apps/desktop/src/window/DesktopWindow.ts b/apps/desktop/src/window/DesktopWindow.ts index 56411711eb6c..ac62631aaa27 100644 --- a/apps/desktop/src/window/DesktopWindow.ts +++ b/apps/desktop/src/window/DesktopWindow.ts @@ -280,6 +280,15 @@ export const make = Effect.gen(function* () { // createMainIfBackendReady, which gates the post-readiness window // open in development and the macOS "activate without windows" path. const backendReadyRef = yield* Ref.make(false); + // A menu action that arrives before there is anywhere to send it. Until now + // dispatchMenuAction returned silently in that state, which is invisible for + // a menu item (the user can just click it again) and fatal for a URL the OS + // used to LAUNCH the app: that is exactly the moment when no window exists + // and the backend is not ready yet, and the action has no second chance. + // Held as a single slot rather than a queue on purpose: these are navigation + // intents, and replaying a backlog of them would land on whichever happened + // to be last anyway. + const pendingMenuActionRef = yield* Ref.make>(Option.none()); // The transient "Connecting to WSL" splash window, tracked separately so it // is never mistaken for the real main window. const splashWindowRef = yield* Ref.make>(Option.none()); @@ -779,6 +788,30 @@ export const make = Effect.gen(function* () { return window; }).pipe(Effect.withSpan("desktop.window.revealOrCreateMain")); + // Send an action to the renderer, creating and revealing the window as + // needed. Extracted from dispatchMenuAction so a parked action can be + // replayed through exactly the same path once the backend comes up, rather + // than a second, subtly different one. + const deliverMenuAction = Effect.fn("desktop.window.deliverMenuAction")(function* ( + action: string, + ) { + const existingWindow = yield* focusedMainWindow; + const targetWindow = Option.isSome(existingWindow) ? existingWindow.value : yield* ensureMain; + + const send = () => { + if (targetWindow.isDestroyed()) return; + targetWindow.webContents.send(MENU_ACTION_CHANNEL, action); + void runPromise(electronWindow.reveal(targetWindow)); + }; + + if (targetWindow.webContents.isLoadingMainFrame()) { + targetWindow.webContents.once("did-finish-load", send); + return; + } + + send(); + }); + const createMainIfBackendReady = Effect.gen(function* () { const backendReady = yield* Ref.get(backendReadyRef); if (!backendReady) return; @@ -863,7 +896,26 @@ export const make = Effect.gen(function* () { handleBackendReady: Effect.fn("desktop.window.handleBackendReady")(function* (httpBaseUrl) { yield* Ref.set(backendReadyRef, true); yield* logWindowInfo("backend ready", { source: "http", url: httpBaseUrl.href }); - yield* createMainIfBackendReady; + // Replay whatever arrived before there was anywhere to send it. Taken + // rather than read, so a parked action fires once and never survives into + // a later backend restart. + // + // `ensuring`, because createMainIfBackendReady can FAIL and the pool + // swallows that failure: without this the slot would stay full for ever + // and the deep link would be silently lost even though the user goes on + // to open a window by hand. Delivery re-resolves the window itself, so it + // is still correct on the path where creation failed here and succeeded + // later. + yield* createMainIfBackendReady.pipe( + Effect.ensuring( + Effect.gen(function* () { + const pending = yield* Ref.getAndSet(pendingMenuActionRef, Option.none()); + if (Option.isSome(pending)) { + yield* deliverMenuAction(pending.value).pipe(Effect.catchCause(() => Effect.void)); + } + }), + ), + ); }), handleBackendNotReady: Ref.set(backendReadyRef, false).pipe( Effect.withSpan("desktop.window.handleBackendNotReady"), @@ -875,22 +927,11 @@ export const make = Effect.gen(function* () { yield* Effect.annotateCurrentSpan({ action }); const existingWindow = yield* focusedMainWindow; if (Option.isNone(existingWindow) && !(yield* Ref.get(backendReadyRef))) { + // Park it for handleBackendReady rather than dropping it. + yield* Ref.set(pendingMenuActionRef, Option.some(action)); return; } - const targetWindow = Option.isSome(existingWindow) ? existingWindow.value : yield* ensureMain; - - const send = () => { - if (targetWindow.isDestroyed()) return; - targetWindow.webContents.send(MENU_ACTION_CHANNEL, action); - void runPromise(electronWindow.reveal(targetWindow)); - }; - - if (targetWindow.webContents.isLoadingMainFrame()) { - targetWindow.webContents.once("did-finish-load", send); - return; - } - - send(); + yield* deliverMenuAction(action); }), zoomMain: Effect.fn("desktop.window.zoomMain")(function* (direction) { yield* Effect.annotateCurrentSpan({ direction }); diff --git a/apps/web/src/components/AppSidebarLayout.tsx b/apps/web/src/components/AppSidebarLayout.tsx index a3ba76679689..4b5ff81fa51e 100644 --- a/apps/web/src/components/AppSidebarLayout.tsx +++ b/apps/web/src/components/AppSidebarLayout.tsx @@ -24,6 +24,7 @@ import { useSidebarStageBackdropVariant, } from "./SidebarStageBackdrop"; import { useProjects } from "../state/entities"; +import { resolveMenuActionNavigation } from "../menuActionNavigation.ts"; import { resolveInitialThreadSidebarWidth, resolveThreadSidebarMaximumWidth, @@ -200,6 +201,15 @@ export function AppSidebarLayout({ children }: { children: ReactNode }) { if (!isSettingsRoute) { void navigate({ to: "/settings" }); } + return; + } + // Deep links (t3code://threads//) arrive as a + // navigate action rather than on a channel of their own: the main process + // has already resolved them to a renderer path, and this listener is the + // one place the router is reachable from it. + const target = resolveMenuActionNavigation(action, pathname); + if (target !== null) { + void navigate({ to: target }); } }); diff --git a/apps/web/src/menuActionNavigation.test.ts b/apps/web/src/menuActionNavigation.test.ts new file mode 100644 index 000000000000..5140f22e27f0 --- /dev/null +++ b/apps/web/src/menuActionNavigation.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { + MENU_ACTION_NAVIGATE_PREFIX, + resolveMenuActionNavigation, +} from "./menuActionNavigation.ts"; + +const nav = (target: string) => `${MENU_ACTION_NAVIGATE_PREFIX}${target}`; + +describe("MENU_ACTION_NAVIGATE_PREFIX", () => { + /** + * The main process declares the same prefix separately, in + * `apps/desktop/src/app/DesktopUrlRouting.ts`, because this bundle must not + * import from it and the two are separate TypeScript projects. Both sides pin + * the literal, so whichever one is edited fails its own test rather than both + * staying green while deep links quietly stop working. + */ + it("pins the literal the main process also pins", () => { + expect(MENU_ACTION_NAVIGATE_PREFIX).toBe("navigate:"); + }); +}); + +describe("resolveMenuActionNavigation", () => { + it("returns the path of a navigation action", () => { + expect(resolveMenuActionNavigation(nav("/env-1/thread-1"), "/")).toBe("/env-1/thread-1"); + }); + + it("ignores a non-navigation action", () => { + expect(resolveMenuActionNavigation("open-settings", "/")).toBeNull(); + expect(resolveMenuActionNavigation("", "/")).toBeNull(); + }); + + // Navigating to where we already are would push a duplicate history entry and + // remount the chat for nothing. + it("ignores a navigation to the current route", () => { + expect(resolveMenuActionNavigation(nav("/env-1/thread-1"), "/env-1/thread-1")).toBeNull(); + }); + + /** + * The target began life as a URL handed to the app by the operating system, + * so it is input. A protocol-relative target is the case worth naming: the + * router reads `//evil.example` as a path, a browser reads it as another + * origin, and that gap is where an open redirect lives. + */ + it("refuses anything that is not an absolute in-app path", () => { + expect(resolveMenuActionNavigation(nav("//evil.example/x"), "/")).toBeNull(); + expect(resolveMenuActionNavigation(nav("https://evil.example"), "/")).toBeNull(); + expect(resolveMenuActionNavigation(nav("env-1/thread-1"), "/")).toBeNull(); + expect(resolveMenuActionNavigation(nav(""), "/")).toBeNull(); + }); + + it("keeps a query or fragment the main process chose to send", () => { + expect(resolveMenuActionNavigation(nav("/env-1/thread-1?tab=files"), "/")).toBe( + "/env-1/thread-1?tab=files", + ); + }); +}); diff --git a/apps/web/src/menuActionNavigation.ts b/apps/web/src/menuActionNavigation.ts new file mode 100644 index 000000000000..feab3fc8e87f --- /dev/null +++ b/apps/web/src/menuActionNavigation.ts @@ -0,0 +1,41 @@ +// Menu actions arrive from the Electron main process as a plain string on one +// channel. Most are commands ("open-settings"); a deep link +// (t3code://threads//) arrives already resolved to a +// renderer path, because the main process is where the URL is parsed and the +// router is only reachable from here. +// +// Kept out of the component so the rule is testable without rendering, and so +// the component keeps a single call instead of a block of validation. + +// Mirrors NAVIGATE_ACTION_PREFIX in apps/desktop/src/app/DesktopUrlRouting.ts. +// Duplicated rather than imported: the renderer bundle must not depend on the +// Electron main process sources. +export const MENU_ACTION_NAVIGATE_PREFIX = "navigate:"; + +/** + * The path a menu action asks the router to go to, or `null` for anything else. + * + * `null` covers three separate cases on purpose, all of which mean "do not + * navigate": the action is not a navigation at all, the target is not something + * this app may route to, or it is where we already are. + * + * The target is treated as INPUT even though it comes from our own main + * process, because it began life as a URL handed to the app by the operating + * system. Only absolute in-app paths are accepted, and a protocol-relative + * `//host` is rejected explicitly: the router would treat it as a path while a + * browser would read it as another origin, and that difference is exactly where + * an open redirect lives. + */ +export function resolveMenuActionNavigation(action: string, pathname: string): string | null { + if (!action.startsWith(MENU_ACTION_NAVIGATE_PREFIX)) { + return null; + } + const target = action.slice(MENU_ACTION_NAVIGATE_PREFIX.length); + if (!target.startsWith("/") || target.startsWith("//")) { + return null; + } + if (target === pathname) { + return null; + } + return target; +}