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("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/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: "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/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("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/env-1/thread-1"), "/")).toBe("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/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("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/env-1/thread-1"), "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/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("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/env-1/thread-1?tab=files"), "/")).toBe( + "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/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; +}