fix(electron): handle Linux OAuth deep links - #9278
Conversation
🦋 Changeset detectedLatest commit: 26e0b8f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
thanks for the quick solution 🥇
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/electron/src/main/create-clerk-bridge.ts (1)
51-65: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the single-instance lock in
finally.createClerkBridge()acquires it before later setup steps, andcleanup()only releases it on the normal path. If initialization or teardown throws, the bridge can leave the process lock behind. Wrap the setup/cleanup path intry/finallyand add failure-path coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/electron/src/main/create-clerk-bridge.ts` around lines 51 - 65, Update createClerkBridge() so ownership of the single-instance lock is released in a finally block covering initialization and cleanup, including when setup or teardown throws. Preserve the existing early return when requestSingleInstanceLock() fails, and add coverage for lock release on failure paths.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/electron/src/main/create-clerk-bridge.ts`:
- Around line 51-65: Update createClerkBridge() so ownership of the
single-instance lock is released in a finally block covering initialization and
cleanup, including when setup or teardown throws. Preserve the existing early
return when requestSingleInstanceLock() fails, and add coverage for lock release
on failure paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 01f3426d-2037-476f-9770-fdb5ee53cc1a
📒 Files selected for processing (5)
.changeset/calm-taxis-return.mdpackages/electron/README.mdpackages/electron/src/main/__tests__/create-clerk-bridge.test.tspackages/electron/src/main/create-clerk-bridge.tspackages/electron/src/shared/types.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)clerk/cli(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/calm-taxis-return.md
- packages/electron/README.md
Bring the initiating renderer window forward when a deep-link callback resolves the pending flow. Windows and Linux hand the callback to a background process, so the browser otherwise stays in front of the signed-in window; macOS activates the app itself via open-url. Warn when manageSingleInstanceLock is false and the application holds no single-instance lock, since second-instance is the only delivery path on Windows and Linux and cannot fire without it. Release the single-instance lock when initialization or cleanup throws, and make the release idempotent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/electron/src/main/create-clerk-bridge.ts`:
- Around line 79-121: Update the bridge initialization flow around
setupTokenCacheIpcHandlers, setupPasskeysMain, and
setupOAuthTransportIpcHandlers to track each completed teardown as it becomes
available. In the catch path, invoke registered teardowns in reverse order,
including cleanup for partially initialized handlers when setup throws, then
release the single-instance lock and rethrow the original error. Preserve the
normal returned cleanup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ed8d4b4f-8df4-43c5-ad5d-e58807b28a7e
📒 Files selected for processing (3)
packages/electron/src/main/__tests__/create-clerk-bridge.test.tspackages/electron/src/main/create-clerk-bridge.tspackages/electron/src/main/oauth-transport.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)clerk/cli(auto-detected)
… macOS Return isPrimaryInstance from createClerkBridge so applications can stop their own bootstrap in a secondary process. app.quit() is asynchronous, so whenReady can still fire in a process that is on its way out, loading a window on a scheme that was never registered as privileged. Stop acquiring the single-instance lock on macOS, where deep links reach the running instance through open-url. The lock only buys callback delivery where second-instance is the delivery path, so taking it on macOS made apps single-instance for no functional gain. Track teardowns on a stack and drain them in reverse when initialization throws, so handlers registered before the failing step are removed instead of leaking. Teardown errors during that drain are swallowed to keep the original initialization error.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/electron/src/main/create-clerk-bridge.ts (1)
81-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
runTeardownsaborts on the first failing teardown, leaking the rest.
teardowns.pop()?.()isn't wrapped per-iteration, so if one teardown throws (e.g.ipcMain.removeHandleror the OAuth transport listener cleanup), thewhileloop exits and any remaining teardowns (registered earlier, e.g. token-cache/passkeys handlers) never run. In the successcleanup()path this also lets the exception escape uncaught sincefinallyonly guarantees the lock is released, not that the error is handled. In the init-failurecatchpath the outercatch {}only stops the teardown error from masking the original error — it doesn't resume cleaning up the rest of the stack.🐛 Proposed fix: isolate each teardown call
const teardowns: Array<() => void> = []; const runTeardowns = (): void => { while (teardowns.length > 0) { - teardowns.pop()?.(); + try { + teardowns.pop()?.(); + } catch (teardownErr) { + console.warn('Clerk: a teardown handler failed while cleaning up createClerkBridge resources.', teardownErr); + } } };With this, the
catch { /* swallow */ }wrapper aroundrunTeardowns()in the init-failure branch (lines 133-137) becomes unnecessary but harmless to keep.Consider also adding a regression test that registers multiple teardowns (e.g.
passkeys: true+ renderer) and makes one throw mid-stack, asserting the others still run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/electron/src/main/create-clerk-bridge.ts` around lines 81 - 141, Update runTeardowns to isolate each popped teardown invocation with its own try/catch, swallowing individual teardown errors so the loop continues until every registered cleanup runs. Preserve cleanup()’s lock release and the initialization catch path’s original-error propagation, and add a regression test covering multiple teardowns where one throws while the others still execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/electron/src/main/create-clerk-bridge.ts`:
- Around line 81-141: Update runTeardowns to isolate each popped teardown
invocation with its own try/catch, swallowing individual teardown errors so the
loop continues until every registered cleanup runs. Preserve cleanup()’s lock
release and the initialization catch path’s original-error propagation, and add
a regression test covering multiple teardowns where one throws while the others
still execute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f8ff2eec-f61d-47e8-a066-c38d0202a75c
📒 Files selected for processing (6)
.changeset/calm-taxis-return.mdintegration/templates/electron-vite/main.mjspackages/electron/README.mdpackages/electron/src/main/__tests__/create-clerk-bridge.test.tspackages/electron/src/main/create-clerk-bridge.tspackages/electron/src/shared/types.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)clerk/cli(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/calm-taxis-return.md
- packages/electron/README.md
Isolate each teardown in its own try/catch so the stack drains fully, then rethrow the first error. Previously a throwing teardown abandoned the rest: with passkeys and a renderer configured, the stack drains in reverse, so a failure in the OAuth transport cleanup left the passkey and token cache IPC handlers registered. cleanup() still releases the lock and still surfaces the failure, and the initialization catch path still propagates the original error.
2 participants
Description
Acquire Electron’s single-instance lock for OAuth deep links and forward secondary-instance callbacks to the primary process. Hosts that manage the process-wide lock can opt out with
manageSingleInstanceLock: false.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change