Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
|
||
| // On a cold start the renderer is still loading and would miss an immediate | ||
| // send, so wait for the first load to finish before handing the link over. | ||
| if (targetWindow.webContents.isLoadingMainFrame()) { |
There was a problem hiding this comment.
🟠 High window/DesktopWindow.ts:829
The cold-start path in deliverDeepLink waits for did-finish-load and then immediately calls webContents.send(DEEP_LINK_CHANNEL, target). The renderer only registers its DEEP_LINK_CHANNEL listener later from a React useEffect in AppSidebarLayout, after the auth gate loads. Because Electron IPC has no replay buffer, the one-shot message is lost whenever it fires before that listener is registered, so opening a deep link from a closed app silently fails to navigate. Consider replacing the did-finish-load heuristic with an explicit renderer-ready handshake (e.g., the renderer sends a ready signal before main delivers the link) or buffering the pending link on the renderer side.
Also found in 1 other location(s)
apps/web/src/components/AppSidebarLayout.tsx:194
The deep-link IPC listener is only registered in a post-render
useEffect, but cold-start delivery sendsDEEP_LINK_CHANNELas soon as Electron emitsdid-finish-load. The preload implementation uses a plainipcRenderer.onlistener and does not queue/replay messages, so the main process can send the pending launch link before this effect runs (or beforeAppSidebarLayoutmounts while the auth gate loads). In that case the one-shot IPC message is lost and opening a deep link from a closed app does not navigate to the thread.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/window/DesktopWindow.ts around line 829:
The cold-start path in `deliverDeepLink` waits for `did-finish-load` and then immediately calls `webContents.send(DEEP_LINK_CHANNEL, target)`. The renderer only registers its `DEEP_LINK_CHANNEL` listener later from a React `useEffect` in `AppSidebarLayout`, after the auth gate loads. Because Electron IPC has no replay buffer, the one-shot message is lost whenever it fires before that listener is registered, so opening a deep link from a closed app silently fails to navigate. Consider replacing the `did-finish-load` heuristic with an explicit renderer-ready handshake (e.g., the renderer sends a ready signal before main delivers the link) or buffering the pending link on the renderer side.
Also found in 1 other location(s):
- apps/web/src/components/AppSidebarLayout.tsx:194 -- The deep-link IPC listener is only registered in a post-render `useEffect`, but cold-start delivery sends `DEEP_LINK_CHANNEL` as soon as Electron emits `did-finish-load`. The preload implementation uses a plain `ipcRenderer.on` listener and does not queue/replay messages, so the main process can send the pending launch link before this effect runs (or before `AppSidebarLayout` mounts while the auth gate loads). In that case the one-shot IPC message is lost and opening a deep link from a closed app does not navigate to the thread.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — New feature adding deep link navigation with complex cross-platform handling and main/renderer IPC coordination. Multiple High severity findings identify timing issues in cold-start scenarios where links may be silently dropped. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20eb2eae01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Registered before bootstrap so a link that launched the app is picked up | ||
| // and held, rather than being missed while the backend starts. | ||
| yield* deepLinkRouter.register; |
There was a problem hiding this comment.
On a macOS cold start, Electron can emit open-url before ready, and its API requires this listener to be installed before that point. Here deepLinkRouter.register does not run until after electronApp.whenReady has resolved, so the launch URL is lost and cannot be recovered from process.argv on macOS; opening a thread link while the app is closed therefore still fails.
Useful? React with 👍 / 👎.
| if (targetWindow.webContents.isLoadingMainFrame()) { | ||
| targetWindow.webContents.once("did-finish-load", send); |
There was a problem hiding this comment.
When a deep link creates a still-loading window, this sends the only IPC notification directly from did-finish-load, but the renderer does not call onDeepLink until AppSidebarLayout's passive useEffect runs. Electron does not queue renderer IPC events for listeners registered later, so on cold starts where that effect has not run by did-finish-load, the target is silently dropped; use a preload-side buffer or an explicit renderer-ready handshake before consuming the pending link.
Useful? React with 👍 / 👎.
|
Ran this branch from a packaged Linux build (CachyOS, AppImage) — the wiring holds end to end, cold-start argv path included. One number worth having before it lands, because it bounds what can be built on top. On Linux a caller's only entry point is A local listener in the main process removes that second process entirely. I added one on top of this branch: // in DesktopDeepLinkRouter, alongside open-url / second-instance
const server = NodeNet.createServer((connection) => {
/* accumulate, first line wins, capped */ handle(url, "socket");
});
server.listen(socketPath, () => NodeFS.chmodSync(socketPath, 0o600));
~1.4s → ~10ms, same machine, same link. In practice the tab switch is indistinguishable from a click inside the app. Shape, briefly:
Happy to open it as a follow-up once this merges, or to hand you the diff to fold in here — whichever you prefer. Either way the measurement is the part worth keeping: without something like it, every Linux deep link pays 1.4s. |
The deep link was off because nothing on T3's side answered it. Something does now, so the default flips: landing on the thread is the point of the click, and every way it can miss lands somewhere harmless — a build that does not route `t3code://threads/<environment>/<thread>` reveals its window, which is what the click did anyway. `t3.deep_link = false` remains for the one case that is not harmless: a machine with no T3 desktop app, where nothing claims the scheme and the desktop would ask which application to open the URL with. Upstream T3 does not route the link yet (pingdotgg/t3code#6008), so the README says so rather than implying every build answers.
Implements pingdotgg#4996. `t3code://threads/<environmentId>/<threadId>` is already produced by buildAgentAwarenessDeepLink and consumed by mobile, but desktop never acted on it: there was no `open-url` listener, and the `second-instance` handler ignores its argv. The main process now validates such links and hands a typed target to the renderer, which navigates through the existing `/$environmentId/$threadId` route. Delivery reuses the dispatchMenuAction path (existing window, otherwise ensureMain; wait for did-finish-load while the frame is loading; then reveal), and additionally holds a link that arrives before the backend is ready so a link that *launches* the app is not dropped. Parsing is strict and rejects anything that is not this app's own scheme with a `threads` host and exactly two clean segments, so OAuth callbacks and the renderer bundle URL continue to be handled where they are today.
Why: - macOS can emit open-url before Electron is ready, and renderer IPC sent at did-finish-load can precede React subscription. - The old backend-ready check and pending write could also race and strand a link. What: - Register deep links before app readiness. - Buffer renderer-bound targets in preload until a subscriber exists. - Queue before checking backend readiness and drain with an atomic take. Validation: - pnpm exec vp test run apps/desktop/src/app/DesktopDeepLink.test.ts apps/desktop/src/app/DesktopDeepLinkBuffer.test.ts apps/desktop/src/window/DesktopWindow.test.ts - pnpm exec vp run --filter @t3tools/desktop --filter @t3tools/web --filter @t3tools/contracts typecheck Notes: - Addresses the three cold-start correctness findings on PR pingdotgg#6008.
20eb2ea to
699f356
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 699f356f89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const existingWindow = yield* focusedMainWindow; | ||
| if (Option.isSome(existingWindow)) { | ||
| yield* sendDeepLink(existingWindow.value, target); |
There was a problem hiding this comment.
When macOS delivers open-url while a browser preview's picture-in-picture window is focused, focusedMainWindow resolves to that separate BrowserWindow created by PreviewManager.openPictureInPicture. The deep-link IPC is then sent to the PiP window's different preload, which has no desktopBridge.onDeepLink listener, so the main renderer never navigates; resolve the registered main window instead and create/reveal it when absent.
Useful? React with 👍 / 👎.
| }); | ||
|
|
||
| const flushPendingDeepLink = Effect.fn("desktop.window.flushPendingDeepLink")(function* () { | ||
| const pending = yield* Ref.getAndSet(pendingDeepLinkRef, Option.none()); |
There was a problem hiding this comment.
🟠 High window/DesktopWindow.ts:872
flushPendingDeepLink permanently drops the pending target when ensureMain or sendDeepLink fails, because Ref.getAndSet(..., Option.none()) clears it before delivery succeeds. Keep the target pending until the window is created and the link is delivered, or restore it when either operation fails so a later activation can replay it.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/window/DesktopWindow.ts around line 872:
`flushPendingDeepLink` permanently drops the pending target when `ensureMain` or `sendDeepLink` fails, because `Ref.getAndSet(..., Option.none())` clears it before delivery succeeds. Keep the target pending until the window is created and the link is delivered, or restore it when either operation fails so a later activation can replay it.
| ) { | ||
| yield* Effect.annotateCurrentSpan({ environmentId: target.environmentId }); | ||
| const send = () => { | ||
| if (targetWindow.isDestroyed()) return; |
There was a problem hiding this comment.
🟠 High window/DesktopWindow.ts:856
When targetWindow is destroyed before send runs, sendDeepLink returns without delivering or re-queuing target; closing the focused window during the did-finish-load wait therefore makes dispatchDeepLink succeed while silently dropping the link. Re-queue the target and flush it when a replacement main window is created instead of returning.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/window/DesktopWindow.ts around line 856:
When `targetWindow` is destroyed before `send` runs, `sendDeepLink` returns without delivering or re-queuing `target`; closing the focused window during the `did-finish-load` wait therefore makes `dispatchDeepLink` succeed while silently dropping the link. Re-queue the target and flush it when a replacement main window is created instead of returning.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 699f356. Configure here.
|
|
||
| // Replay a link that arrived before the backend came up. Taken (not read) | ||
| // so a later backend restart cannot deliver the same link a second time. | ||
| yield* flushPendingDeepLink(); |
There was a problem hiding this comment.
Pending deep link stuck on create failure
High Severity
handleBackendReady sets backendReadyRef and runs createMainIfBackendReady before flushPendingDeepLink. The backend pool swallows window-create failures, so a failed create skips the flush and leaves the cold-start target in pendingDeepLinkRef. A later dock activate can open the window without replaying it, and a later successful readiness can navigate to that stale target. deliverDeepLink also sends to an existing window without clearing the pending ref, which compounds the stale replay.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 699f356. Configure here.
|
Note 🤖 GPT-5.6 Sol responding on behalf of Theo We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together. This adds If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. |
4 participants
Summary
t3code://threads/<environmentId>/<threadId>links are already produced bybuildAgentAwarenessDeepLinkand consumed by the mobile app, but opening one on desktop does nothing beyond (at most) focusing the window — the user still has to find the thread by hand.open-url(macOS) and reads the URL out ofargv(Windows/Linux, both cold start andsecond-instance), validates it, and hands a typed target to the renderer, which navigates through the existing/$environmentId/$threadIdroute.DesktopClerk's existingsecond-instancereveal listener, to the deep-link producer, to mobile, or to the packaged Linux.desktopfile. The external URL contract is unchanged — this only teaches desktop to consume what mobile already consumes.On CONTRIBUTING
I read
CONTRIBUTING.mdbefore opening this, so to address its points directly:DesktopWindow.open-urlonly and drop the Windows/Linux argv paths, or split the pure parser out as its own PR. Say which you prefer rather than closing it, and I will resubmit.Entirely fine if the answer is "not now" — no obligation implied.
Change Type / Scope
Linked Issue/PR
second-instancedropping the URL for SSO callbacks. This PR adds a secondsecond-instancelistener for thread links and does not touch the Clerk path, so [Bug]: second-instance handler drops deep-link URL, breaking SSO login when app is already running #5978 is not fixed here.checkout-prlink kind can be added without re-parsing raw URLs in the renderer.What changed
Before — nothing consumes the URL.
DesktopClerkregisters the onlysecond-instancelistener and ignores its arguments:There is no
open-urllistener at all, so on macOS the URL never reaches the app.After — a new
DesktopDeepLinkRouterowns link delivery, andDesktopWindowgainsdispatchDeepLink:Delivery reuses the
dispatchMenuActionshape (existing window → otherwiseensureMain, wait fordid-finish-loadwhen the frame is still loading, thenreveal), plus one addition that menu actions do not need:handleBackendReadythen replays it once viaRef.getAndSet(pendingDeepLinkRef, Option.none()). Without this, a link that launches the app would be discarded, which is half of what #4996 asks for.Renderer navigates through the typed route rather than a raw hash:
Test plan
DesktopDeepLink.test.ts— 15 cases over the two pure functions.https://,t3codex://); other hosts, explicitly includingt3code://app/...(the renderer bundle) andt3code://oauth/callback— so this cannot swallow links that belong to other handlers./,?or#— a crafted link must not address a different route than its two visible segments suggest.Reproduction
Environment
Steps
environmentId/threadId.open "t3code://threads/<environmentId>/<threadId>".Three existing
DesktopWindowstubs (DesktopLifecycle.test.ts,DesktopBackendPool.test.ts,DesktopApplicationMenu.test.ts) each gained one line, because theysatisfiesthe service type and it now has one more method. That is the whole reason those three files appear in the diff.Commands run in this working tree:
Evidence
isCleanSegmentto accept any non-empty string turns exactly two cases red ("rejects segments that decode back into path separators", "rejects blank or whitespace-padded segments") and leaves the other 13 green — the guards are actually exercised, not incidentally satisfied.Security Impact
threadshost, exactly two segments, and no segment that decodes back into a path separator. Everything else returnsnulland falls through to existing behaviour.Human Verification
open-urlfiring,second-instanceargv contents, the cold-start replay throughhandleBackendReady) is reasoned from the existingdispatchMenuActionpath rather than observed. If you would like me to attach a recording from a local build before merging, say the word.Failure Recovery
apps/desktop/src/{app/DesktopApp.ts,ipc/channels.ts,main.ts,preload.ts,window/DesktopWindow.ts},apps/web/src/components/AppSidebarLayout.tsx,packages/contracts/src/ipc.ts; deleteapps/desktop/src/app/DesktopDeepLink*.ts.t3code://link that used to reach another handler (OAuth) no longer doing so — the parser rejects any host other thanthreads, and there is a test for theoauthandapphosts specifically.Risks and Mitigations
threadshosts returnnulland are ignored, with tests coveringt3code://app/...andt3code://oauth/callback.Ref.getAndSet(..., Option.none())takes the value, so a laterhandleBackendReadyhas nothing to replay.resolveThreadRouteRenderState) apply unchanged.🤖 Authored with Claude Code. The parsing rules and the hold-until-ready behaviour were derived by reading the existing
dispatchMenuAction/handleBackendReadypaths in this repo; tests were written to fail without the change.Note
Add
t3code://deep link routing to open threads inDesktopAppDesktopDeepLinkRouterparsest3code://threads/{envId}/{threadId}URLs from macOSopen-url, Windows/Linuxsecond-instance, and cold-startprocess.argv.DesktopWindow.dispatchDeepLinkholds the parsed target until the backend is ready, then sends it overDEEP_LINK_CHANNELto the renderer.window.desktopBridge.onDeepLinkand navigates to/$environmentId/$threadId.DesktopWindowinterface extended withdispatchDeepLink; test mock layers updated to include this method.Macroscope summarized 699f356.
Note
Medium Risk
Touches Electron protocol events (
open-url,second-instance, argv) and IPC into the renderer. Parsing is strict and OAuth/app hosts are rejected, but a mistake could steal other scheme handlers or navigate to the wrong thread.Overview
Desktop now opens
t3code://threads/<environmentId>/<threadId>links (the same contract mobile already uses) instead of only focusing the window.A new router listens on macOS
open-url(registered beforeready) and on Windows/Linux via launch argv plussecond-instance. URLs are parsed strictly (registered scheme,threadshost, exactly two clean segments) so OAuth and renderer-bundle URLs are ignored.DesktopWindowholds a pending target until the backend is ready, then sends it overdesktop:deep-link.The preload buffers the typed target until the renderer subscribes;
AppSidebarLayoutnavigates to/$environmentId/$threadId. Clerk/OAuth handling is unchanged.Reviewed by Cursor Bugbot for commit 699f356. Bugbot is set up for automated code reviews on this repo. Configure here.