fix(desktop): forward second-instance deep links to the running app - #7180
maslinedwin wants to merge 6 commits into
Conversation
Fixes pingdotgg#5978. Second-instance argv and macOS open-url now extract t3code:// URLs and dispatch them instead of only focusing the window.
|
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}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces new runtime behavior for forwarding deep links to an already-running desktop instance, including new event handlers ( You can customize Macroscope's approvability policy. Learn more. |
Cold-launch open-url and WSL splash both left protocol URLs undelivered: the handler returned after preventDefault, or loadURL hit the splash. Queue the latest URL and apply it on the registered main window after createMain.
github-actions
Bot
added
size:L
There was a problem hiding this comment.
Reviewed the Effect service changes in DesktopClerk.ts, DesktopWindow.ts, and the new desktopProtocolUrl.ts against the Effect service conventions. One finding: the pending protocol URL is shared between two Effect services through module-level mutable state instead of the Effect environment.
Posted via Macroscope — Effect Service Conventions
| // Latest protocol URL received before a real main window exists (cold launch | ||
| // or WSL connecting splash). DesktopWindow.createMain applies it after setMain. | ||
| let pendingDesktopProtocolUrl: string | null = null; |
There was a problem hiding this comment.
This module-level mutable slot is a hidden channel between two Effect services: DesktopClerk.make writes it via queuePendingDesktopProtocolUrl and DesktopWindow.createMain reads it via applyPendingDesktopProtocolUrl, so the dependency never appears in either service's environment or type (and the tests have to reset it in beforeEach).
Consider holding this state in a Ref created inside DesktopWindow.make (alongside splashWindowRef/backendReadyRef) and exposing something like queuePendingProtocolUrl(url) on the DesktopWindow service; DesktopClerk.configure already acquires yield* DesktopWindow.DesktopWindow, so it can call that instead of the global. The pure helpers (isDesktopProtocolUrl, extractDesktopProtocolUrl, loadDesktopProtocolUrl) can stay in this module.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Acknowledged. The queue still has to be readable from DesktopWindow.createMain after setMain. I left the helpers in desktopProtocolUrl.ts and serialized the create path in Clerk; moving the slot onto DesktopWindow is a follow-up if you want the dependency in the service type.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
Concurrent open-url and second-instance events now share one create gate so they cannot each open an untracked main window.
Second-instance and open-url callbacks could lose an SSO URL: createMain loaded home first, development retries reloaded home, and a gate waiter could apply an older fiber-local URL after a newer one was already queued.
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.
Reviewed by Cursor Bugbot for commit 5526327. Configure here.
currentLoadUrl kept the SSO callback after did-finish-load, so renderer crash recovery replayed the one-shot protocol URL instead of the app home.
There was a problem hiding this comment.
One convention issue in apps/desktop/src/app/desktopProtocolUrl.ts: service state/behavior is exchanged through module globals rather than the Effect environment. The pending-URL slot was already raised earlier in this PR and acknowledged as a follow-up; the new window-loader registry added since then is flagged below.
Posted via Macroscope — Effect Service Conventions
| const desktopProtocolWindowLoaders = new WeakMap<object, DesktopProtocolWindowLoader>(); | ||
|
|
||
| export function registerDesktopProtocolWindowLoader( | ||
| window: object, | ||
| load: DesktopProtocolWindowLoader, | ||
| ): void { | ||
| desktopProtocolWindowLoaders.set(window, load); | ||
| } |
There was a problem hiding this comment.
This adds a second module-global channel between the two services: DesktopWindow.make writes per-window loaders into this WeakMap and DesktopClerk reaches them indirectly through loadDesktopProtocolUrl, so the dependency is invisible in both service types (and in DesktopWindow's own state, which already keeps window bookkeeping in refs).
Consider owning the "navigate the main window to a URL" behavior on the DesktopWindow service (e.g. a loadProtocolUrl(url) operation whose implementation closes over currentLoadUrl/clearDevelopmentLoadRetry for the window it created) and having DesktopClerk.configure call it via yield* DesktopWindow.DesktopWindow, which it already acquires. isDesktopProtocolUrl / extractDesktopProtocolUrl remain fine as pure helpers here.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Same follow-up as the pending-URL slot. The WeakMap only exists so development retries and crash recovery share the window's intended URL after Clerk applies a deep link. DesktopClerk already acquires DesktopWindow; moving loadProtocolUrl onto that service (and folding the pending slot in with it) is the cleanup if you want the dependency in the service type. Not changing behavior in this PR.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
The Clerk create gate only serialized open-url / second-instance handlers. handleBackendReady and activate could still pass the splash-filtered empty check before setMain and open a second untracked main, dropping the queued OAuth deep link. Share one DesktopWindow permit and re-check before create.
|
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. We are keeping OPEN #8246 as the complete review path for desktop thread deep links. This branch forwards second-instance links but does not own the full environment and thread route. If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. |
2 participants
Fixes #5978. Second-instance argv and macOS open-url now extract
t3code://URLs and dispatch them instead of only focusing the window.Note
Medium Risk
Touches OAuth/SSO routing, macOS
open-urlhandling, and main-window creation races; mistakes could drop callbacks, load links on the wrong window, or regress WSL splash behavior.Overview
Forwards custom-scheme deep links (
t3code/t3code-dev) into the already-running desktop app instead of only focusing a window.Adds
desktopProtocolUrl: parse URLs from argv (last match wins), queue/apply a single pending link when no registered main exists, and per-window loaders so dev load retries keep the deep link instead of reverting to home.DesktopClerk.configurenow handlessecond-instanceargv (Windows/Linux) andopen-url(macOS, withpreventDefault) viarevealAndDispatch: load onelectronWindow.mainonly (not the WSL splash), queue +createMainIfBackendReadywhen needed, and reveal with no URL when argv has no link.DesktopWindowconsumes the pending URL on first main load, trackscurrentLoadUrlfor retries/recovery, resets to app home after a deep linkdid-finish-load, and uses aSemaphoreoncreateMain/ensureMain/createMainIfBackendReadyso concurrent deep-link and backend-ready paths cannot open duplicate mains.Reviewed by Cursor Bugbot for commit 7087b0d. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Forward deep-link URLs to the running desktop app on second-instance and open-url events
second-instanceevents, extracts a custom-scheme URL fromargvand loads it in the existing main window via new helpers indesktopProtocolUrl.ts.open-urlapp event inDesktopClerk.ts, callspreventDefault(), and dispatches the URL the same way.DesktopWindow.tsnow tracks the current deep-link URL, resets to the home URL after the deep link finishes loading, and serializes main window creation to prevent duplicate windows during races.createMain,ensureMain,createMainIfBackendReady) are now gated by a semaphore, which changes their concurrency behavior.Macroscope summarized 7087b0d.