test(electron): Initial Electron SDK integration test#9014
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 4f179e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an Electron Vite integration template, runtime bridge and renderer, Playwright auth fixtures and test, and CI wiring for a new ChangesElectron integration template and test flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
integration/templates/electron-vite/main.mjs (1)
25-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer keeping the renderer sandbox on.
sandbox: falseweakens the BrowserWindow isolation this template is meant to exercise. If@clerk/electronworks with the preload bridge under Electron’s default sandbox, flip this back totrue; otherwise document the incompatibility inline.🔒 Suggested adjustment
webPreferences: { contextIsolation: true, nodeIntegration: false, preload: path.resolve(__dirname, 'preload.mjs'), - sandbox: false, + sandbox: true, },🤖 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 `@integration/templates/electron-vite/main.mjs` around lines 25 - 30, The BrowserWindow configuration in main.mjs disables the renderer sandbox via webPreferences.sandbox = false, weakening the isolation this template is meant to test. Update the BrowserWindow/webPreferences setup to keep sandbox enabled if `@clerk/electron` still works with the preload bridge under Electron’s default sandbox; otherwise leave the value as-is but add an inline note in the same configuration block explaining the incompatibility. Use the BrowserWindow options and preload.mjs reference to locate the change.integration/templates/electron-vite/tsconfig.json (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTypecheck the root Vite config too.
"include": ["src"]leaves the newvite.config.tsoutside TS checking, so config regressions in the template will only surface when Vite executes it. Add that file here or split app/node tsconfigs.♻️ Proposed change
- "include": ["src"] + "include": ["src", "vite.config.ts"]🤖 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 `@integration/templates/electron-vite/tsconfig.json` at line 19, The Electron Vite template tsconfig currently only includes src, so the root vite.config.ts is excluded from TypeScript checking. Update the tsconfig include list used by the template to cover vite.config.ts as well, or split the app and Node/Vite configs so the config file is typechecked by the appropriate tsconfig.
🤖 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 `@integration/presets/electron.ts`:
- Around line 9-12: The Electron preset currently wires the package scripts in
`integration/presets/electron.ts` so `dev` only builds and `serve` is a noop,
which leaves no runnable app flow. Update the `addScript('dev', ...)` and
`addScript('serve', ...)` entries to point to the actual Electron development
and startup commands used by the template, keeping `setup` and `build`
unchanged. Use the existing script wiring in the preset as the place to fix this
so generated projects expose a real `dev` path and a real `serve` path.
In `@integration/templates/electron-vite/package.json`:
- Around line 23-25: The electron-vite template still declares a Node engine
floor below the repository baseline. Update the package.json engines.node value
in this template to match the required minimum Node.js version used elsewhere in
the repo (Node.js >=24.15.0), keeping it consistent with the root package.json
and .nvmrc so the template does not advertise unsupported runtimes.
In `@integration/templates/electron-vite/src/main.tsx`:
- Around line 7-8: The startup constants in main.tsx rely on TypeScript
assertions only, so missing VITE_CLERK_PUBLISHABLE_KEY or VITE_CLERK_UI_URL can
still reach Clerk as undefined. Update the initialization around PUBLISHABLE_KEY
and CLERK_UI_URL to validate both env vars immediately at startup and throw a
clear, developer-facing error if either is absent or empty. Keep the check close
to the existing env reads so the failure happens before Clerk setup, and make
the message specific enough to identify which required variable is missing.
In `@integration/tests/electron/fixtures.ts`:
- Around line 34-42: Skip clerkSetup for non-development publishable keys in the
Electron fixture setup so testing-token initialization only runs for development
instances. Update the helper in fixtures.ts to gate the clerkSetup call the same
way application.ts does, using the parsed instance type or an equivalent check
before calling clerkSetup. Keep the publishableKey/parsePublishableKey flow
intact, and only proceed with the testing-token setup path when the key is for
development.
- Around line 22-23: setupClerkTestingEnv() currently always invokes
clerkSetup(), but the app only uses that path for development publishable keys,
so Electron fixtures can break with staging/non-development keys. Update
setupClerkTestingEnv() in fixtures.ts to check the publishable key mode before
calling clerkSetup(), mirroring the runtime guard used by the app, and skip
Clerk setup when the key is not development so electronExecutable() and the rest
of the fixture setup still work.
---
Nitpick comments:
In `@integration/templates/electron-vite/main.mjs`:
- Around line 25-30: The BrowserWindow configuration in main.mjs disables the
renderer sandbox via webPreferences.sandbox = false, weakening the isolation
this template is meant to test. Update the BrowserWindow/webPreferences setup to
keep sandbox enabled if `@clerk/electron` still works with the preload bridge
under Electron’s default sandbox; otherwise leave the value as-is but add an
inline note in the same configuration block explaining the incompatibility. Use
the BrowserWindow options and preload.mjs reference to locate the change.
In `@integration/templates/electron-vite/tsconfig.json`:
- Line 19: The Electron Vite template tsconfig currently only includes src, so
the root vite.config.ts is excluded from TypeScript checking. Update the
tsconfig include list used by the template to cover vite.config.ts as well, or
split the app and Node/Vite configs so the config file is typechecked by the
appropriate tsconfig.
🪄 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), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d0ce248e-b18c-4057-91a6-21d3d4e243eb
📒 Files selected for processing (18)
.changeset/electron-e2e-tests.md.github/workflows/ci.ymlintegration/presets/electron.tsintegration/presets/index.tsintegration/templates/electron-vite/index.htmlintegration/templates/electron-vite/main.mjsintegration/templates/electron-vite/package.jsonintegration/templates/electron-vite/preload.mjsintegration/templates/electron-vite/src/main.tsxintegration/templates/electron-vite/src/style.cssintegration/templates/electron-vite/src/vite-env.d.tsintegration/templates/electron-vite/tsconfig.jsonintegration/templates/electron-vite/vite.config.tsintegration/templates/index.tsintegration/tests/electron/basic.test.tsintegration/tests/electron/fixtures.tspackage.jsonturbo.json
Comment on lines
+9
to
+12
| .addScript('setup', 'pnpm install') | ||
| .addScript('dev', 'pnpm build') | ||
| .addScript('build', 'pnpm build') | ||
| .addScript('serve', 'echo noop') |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wire dev and serve to real commands.
pnpm dev currently just performs a build, and pnpm serve exits immediately, so the generated Electron template has no runnable development or start path.
🤖 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 `@integration/presets/electron.ts` around lines 9 - 12, The Electron preset
currently wires the package scripts in `integration/presets/electron.ts` so
`dev` only builds and `serve` is a noop, which leaves no runnable app flow.
Update the `addScript('dev', ...)` and `addScript('serve', ...)` entries to
point to the actual Electron development and startup commands used by the
template, keeping `setup` and `build` unchanged. Use the existing script wiring
in the preset as the place to fix this so generated projects expose a real `dev`
path and a real `serve` path.
Comment on lines
+7
to
+8
| const PUBLISHABLE_KEY = import.meta.env.VITE_CLERK_PUBLISHABLE_KEY as string; | ||
| const CLERK_UI_URL = import.meta.env.VITE_CLERK_UI_URL as string; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the required Vite env vars at startup.
The as string assertions only silence TypeScript; an unset VITE_CLERK_* still flows through as undefined and fails later inside Clerk initialization. Throw a clear startup error here instead.
🛠️ Proposed change
-const PUBLISHABLE_KEY = import.meta.env.VITE_CLERK_PUBLISHABLE_KEY as string;
-const CLERK_UI_URL = import.meta.env.VITE_CLERK_UI_URL as string;
+const PUBLISHABLE_KEY = import.meta.env.VITE_CLERK_PUBLISHABLE_KEY;
+const CLERK_UI_URL = import.meta.env.VITE_CLERK_UI_URL;
+
+if (!PUBLISHABLE_KEY || !CLERK_UI_URL) {
+ throw new Error('Missing VITE_CLERK_PUBLISHABLE_KEY or VITE_CLERK_UI_URL for the Electron renderer');
+}As per coding guidelines, "Validate all inputs and sanitize outputs" and "Provide meaningful error messages to developers".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const PUBLISHABLE_KEY = import.meta.env.VITE_CLERK_PUBLISHABLE_KEY as string; | |
| const CLERK_UI_URL = import.meta.env.VITE_CLERK_UI_URL as string; | |
| const PUBLISHABLE_KEY = import.meta.env.VITE_CLERK_PUBLISHABLE_KEY; | |
| const CLERK_UI_URL = import.meta.env.VITE_CLERK_UI_URL; | |
| if (!PUBLISHABLE_KEY || !CLERK_UI_URL) { | |
| throw new Error('Missing VITE_CLERK_PUBLISHABLE_KEY or VITE_CLERK_UI_URL for the Electron renderer'); | |
| } |
🤖 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 `@integration/templates/electron-vite/src/main.tsx` around lines 7 - 8, The
startup constants in main.tsx rely on TypeScript assertions only, so missing
VITE_CLERK_PUBLISHABLE_KEY or VITE_CLERK_UI_URL can still reach Clerk as
undefined. Update the initialization around PUBLISHABLE_KEY and CLERK_UI_URL to
validate both env vars immediately at startup and throw a clear,
developer-facing error if either is absent or empty. Keep the check close to the
existing env reads so the failure happens before Clerk setup, and make the
message specific enough to identify which required variable is missing.
Source: Coding guidelines
wobsoriano
commented
Jun 26, 2026
Comment on lines
+490
to
+497
| - name: Ensure Xvfb is installed | ||
| if: ${{ matrix.test-name == 'electron' }} | ||
| run: | | ||
| if ! command -v xvfb-run &> /dev/null; then | ||
| sudo apt-get update | ||
| sudo apt-get install -y xvfb | ||
| fi | ||
| xvfb-run --help > /dev/null |
Member
Author
There was a problem hiding this comment.
We need xvfb because the Electron E2E launches a real BrowserWindow in Linux CI, where there's no display server by default. xvfb-run provides a virtual display, and it's only installed/used for the Electron matrix job
jeremy-clerk
left a comment
jeremy-clerk
left a comment
Contributor
There was a problem hiding this comment.
A few low-severity reliability/cleanup notes from a review pass. No correctness bugs; the Electron-specific details (privileged scheme timing, bundled clerk-js vs. served UI, pnpm-blocked binary install) all look correct.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
integration/tests/electron/basic.test.ts (2)
30-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapsing five bridge checks into one boolean hides which method is missing.
If this assertion fails, Playwright only reports
expected false to be true, giving no signal about which of the five bridge methods is absent. Returning the missing keys (or asserting each individually) makes regressions debuggable.♻️ Surface the missing methods
await expect( electronPage.evaluate(() => { const bridge = (window as ElectronWindow).__clerk_internal_electron; - return ( - typeof bridge?.tokenCache?.clearToken === 'function' && - typeof bridge?.tokenCache?.getToken === 'function' && - typeof bridge?.tokenCache?.saveToken === 'function' && - typeof bridge?.oauthTransport?.getRedirectUrl === 'function' && - typeof bridge?.oauthTransport?.open === 'function' - ); + return { + clearToken: typeof bridge?.tokenCache?.clearToken, + getToken: typeof bridge?.tokenCache?.getToken, + saveToken: typeof bridge?.tokenCache?.saveToken, + getRedirectUrl: typeof bridge?.oauthTransport?.getRedirectUrl, + open: typeof bridge?.oauthTransport?.open, + }; }), - ).resolves.toBe(true); + ).resolves.toEqual({ + clearToken: 'function', + getToken: 'function', + saveToken: 'function', + getRedirectUrl: 'function', + open: 'function', + });🤖 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 `@integration/tests/electron/basic.test.ts` around lines 30 - 46, The preload bridge test in the electron basic spec is too opaque because it collapses all five method checks on __clerk_internal_electron into one boolean. Update the test in the expose-bridge case so failures identify which symbol is missing, either by asserting each bridge method separately or by returning the missing keys from the electronPage.evaluate block; keep the checks focused on tokenCache.clearToken, tokenCache.getToken, tokenCache.saveToken, oauthTransport.getRedirectUrl, and oauthTransport.open.
62-76: 🩺 Stability & Availability | 🔵 TrivialMake relaunch explicit in these tests.
electronApplicationandelectronPageare test-scoped, so each case already gets a fresh Electron launch. That makes the current assertions rely on fixture scope; if either fixture becomes worker-scoped later, the relaunch coverage disappears while the tests still pass. Consider moving the close/relaunch into the test or locking the fixture scope.🤖 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 `@integration/tests/electron/basic.test.ts` around lines 62 - 76, The relaunch behavior in these Electron session tests is currently implicit because `electronApplication` and `electronPage` are test-scoped, so the cases only verify state on a fresh launch rather than an actual close-and-reopen flow. Update the tests in `basic.test.ts` to make the relaunch explicit by closing the app/page and reopening it within the test, or otherwise lock the fixture scope so `persists the signed-in session after relaunch` and `keeps the signed-out state after relaunch` truly cover relaunch behavior. Keep the existing assertions around `electronPage` and the Clerk sign-in/sign-out flow, but ensure they run after an explicit restart step.
🤖 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.
Nitpick comments:
In `@integration/tests/electron/basic.test.ts`:
- Around line 30-46: The preload bridge test in the electron basic spec is too
opaque because it collapses all five method checks on __clerk_internal_electron
into one boolean. Update the test in the expose-bridge case so failures identify
which symbol is missing, either by asserting each bridge method separately or by
returning the missing keys from the electronPage.evaluate block; keep the checks
focused on tokenCache.clearToken, tokenCache.getToken, tokenCache.saveToken,
oauthTransport.getRedirectUrl, and oauthTransport.open.
- Around line 62-76: The relaunch behavior in these Electron session tests is
currently implicit because `electronApplication` and `electronPage` are
test-scoped, so the cases only verify state on a fresh launch rather than an
actual close-and-reopen flow. Update the tests in `basic.test.ts` to make the
relaunch explicit by closing the app/page and reopening it within the test, or
otherwise lock the fixture scope so `persists the signed-in session after
relaunch` and `keeps the signed-out state after relaunch` truly cover relaunch
behavior. Keep the existing assertions around `electronPage` and the Clerk
sign-in/sign-out flow, but ensure they run after an explicit restart step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f6f4d51-ac6b-42ce-8b9b-4c7dd3c5f811
📒 Files selected for processing (1)
integration/tests/electron/basic.test.ts
jeremy-clerk
approved these changes
Jun 26, 2026
jeremy-clerk
left a comment
jeremy-clerk
left a comment
Contributor
There was a problem hiding this comment.
minor nits but looks good 👍 let me know what we need to do for passkey test support
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
test:integration:electron/Turbo task support.