feat(auth): add desktop login-code flow - #1303
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds desktop login-code authentication with PKCE, Cloud polling, Firebase custom-token sign-in, guarded session injection, legacy bridge fallback, cancellation handling, structured failure telemetry, and expanded Vitest coverage. ChangesDesktop authentication
Sequence Diagram(s)sequenceDiagram
participant EmbeddedView
participant DesktopLoginFlow
participant Cloud
participant Browser
participant Firebase
EmbeddedView->>DesktopLoginFlow: intercept authentication URL
DesktopLoginFlow->>Cloud: create desktop login code
Cloud-->>DesktopLoginFlow: grant and PKCE-bound code
DesktopLoginFlow->>Browser: open Cloud login URL
DesktopLoginFlow->>Cloud: poll code exchange
Cloud-->>DesktopLoginFlow: pending or custom token
DesktopLoginFlow->>Firebase: sign in and look up account
Firebase-->>DesktopLoginFlow: persisted user
DesktopLoginFlow->>EmbeddedView: inject session and reload
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
This was referenced Jul 24, 2026
…ogin-code # Conflicts: # src/main/lib/comfyDownloadManager.ts
benceruleanlu
marked this pull request as ready for review
July 28, 2026 16:45
![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: b23f752d35
ℹ️ 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".
Comment on lines
+49
to
+51
| if (activeInjection) invalidateInjection(activeInjection) | ||
| const injection = { contents, frame: null, token: randomUUID() } | ||
| activeInjection = injection |
There was a problem hiding this comment.
When a second sign-in begins while the first renderer-side IndexedDB transaction is still running, invalidateInjection only queues an executeJavaScript call and ownership transfers immediately. The old script can pass its final isCurrent() check, persist the superseded credentials, and call location.reload() before the queued null assignment runs; the later main-process ownership check cannot undo those side effects. If the newer attempt then fails or is abandoned, the embedded view can remain signed into the superseded account, so ownership transfer needs to wait for invalidation or otherwise synchronously prevent the stale script from committing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@src/main/auth/desktopLoginCode/index.test.ts`:
- Around line 324-343: Replace the in-test consent loop in the “omits
installation_id when telemetry consent is off or undecided” case with an it.each
table covering false and undefined, so each consent value runs as a separately
reported test with isolated setup and mocks. Keep the existing assertions and
sign-in flow unchanged for both cases.
In `@src/main/auth/desktopLoginCode/testHelpers.ts`:
- Around line 9-16: Update hangingFetch so it checks args[1]?.signal?.aborted
before registering the abort listener and immediately rejects with the same
AbortError for an already-aborted signal; retain the existing listener behavior
for signals that abort after the fetch call.
In `@src/main/auth/firebaseBridge/flowState.ts`:
- Around line 65-95: Update showCopyLinkBanner to invoke the existing
activeBannerCleanup teardown before installing a new banner, then replace it
with the current banner’s cleanup function. Preserve the existing listener
removal and stale-banner DOM cleanup behavior, making the singleton
self-enforcing without relying on callers to run cleanup first.
In `@src/main/auth/firebaseBridge/index.test.ts`:
- Around line 234-247: Add positive assertions to the legacy-flow test around
handleFirebasePopup so it verifies the successful path reaches injectSession,
bindUserId, and restoreParentWindow after openExternal rejects. Keep the
existing resolution and no-emission assertions unchanged.
- Around line 157-162: Update the timer advancement in the stale-flow test to
use the exported POST_SIGNIN_HOLD_MS value from the module under test, ensuring
the delay exceeds the configured hold period before checking the negative
assertions. Remove the hardcoded 3000 timing value while preserving the existing
stale-flow and supersession assertions.
In `@src/main/auth/firebaseBridge/index.ts`:
- Around line 84-107: Guard the legacy sign-in pre-flight in the surrounding
sign-in function by returning immediately when comfyContents is destroyed,
before calling beginFirebaseSessionInjection, runBannerCleanup, or getURL. Move
those setup calls, including flow-dependent initialization and startOrigin
assignment, inside the existing try/catch so lifecycle errors are handled rather
than becoming unhandled rejections.
In `@src/main/auth/firebaseBridge/inject.test.ts`:
- Around line 88-114: Add a test for injectFirebaseSession where
contents.mainFrame is replaced or the captured frame becomes destroyed during
the first executeJavaScript call; assert the injection resolves false and the
captured frame executes only the owner script, confirming the IDB credential
script is skipped. Keep the existing session setup and cleanup via
beginFirebaseSessionInjection and releaseFirebaseSessionInjection.
- Around line 63-85: Replace the test’s hand-rolled cleanup simulation with an
assertion focused on isFirebaseSessionInjectionRecordOwnedBy: verify it returns
false when the record carries replacementOwner but staleOwner is supplied.
Rename the test to describe foreign-owner rejection, or instead execute
buildIndexedDbInjectScript against a fake IndexedDB to cover the actual
cleanupStore.delete guard; do not assert behavior derived solely from
assignments in the test.
In `@src/main/auth/firebaseBridge/inject.ts`:
- Around line 112-141: Update the IndexedDB error handling in the request’s
onsuccess flow to close db before rejecting, including the tx.onerror path and
any later transaction or operation failures after req.onsuccess. Preserve the
existing error messages and resolution behavior while ensuring every rejection
releases the connection.
In `@src/main/lib/desktopDetect.test.ts`:
- Around line 16-29: Update the desktop detection test setup around stubProcess
and afterEach to snapshot the pre-test APPDATA and LOCALAPPDATA values, then
restore those exact values during cleanup instead of always deleting them.
Preserve deletion only when a variable was originally absent, while continuing
to restore process.platform.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b454cfc0-05d2-4410-8690-93019e9e96e4
📒 Files selected for processing (24)
src/main/auth/desktopLoginCode/client.test.tssrc/main/auth/desktopLoginCode/client.tssrc/main/auth/desktopLoginCode/customTokenSignIn.test.tssrc/main/auth/desktopLoginCode/customTokenSignIn.tssrc/main/auth/desktopLoginCode/index.test.tssrc/main/auth/desktopLoginCode/index.tssrc/main/auth/desktopLoginCode/origins.test.tssrc/main/auth/desktopLoginCode/origins.tssrc/main/auth/desktopLoginCode/pkce.test.tssrc/main/auth/desktopLoginCode/pkce.tssrc/main/auth/desktopLoginCode/testHelpers.tssrc/main/auth/firebaseBridge/flowControl.tssrc/main/auth/firebaseBridge/flowShared.tssrc/main/auth/firebaseBridge/flowState.tssrc/main/auth/firebaseBridge/index.test.tssrc/main/auth/firebaseBridge/index.tssrc/main/auth/firebaseBridge/inject.test.tssrc/main/auth/firebaseBridge/inject.tssrc/main/auth/firebaseBridge/oauth.tssrc/main/auth/firebaseBridge/restoreParentWindow.test.tssrc/main/auth/firebaseBridge/restoreParentWindow.tssrc/main/host/createHostWindow.test.tssrc/main/host/createHostWindow.tssrc/main/lib/desktopDetect.test.ts
Comment on lines
+324
to
+343
| it('omits installation_id when telemetry consent is off or undecided', async () => { | ||
| for (const consent of [false, undefined]) { | ||
| h.settingsGet.mockReturnValue(consent) | ||
| h.createDesktopLoginCode.mockResolvedValue(GRANT) | ||
| h.exchangeDesktopLoginCode.mockResolvedValue({ | ||
| status: 'complete', | ||
| custom_token: 'custom-token-value' | ||
| }) | ||
| mockSignInChain({ uid: 'uid-1' }) | ||
| const mod = await loadOrchestrator() | ||
|
|
||
| const promise = mod.signInViaDesktopLoginCode(AUTH_URL, fakeContents(), {}) | ||
| await vi.runAllTimersAsync() | ||
| await promise | ||
|
|
||
| const request = h.createDesktopLoginCode.mock.lastCall![1] as Record<string, unknown> | ||
| expect(request).not.toHaveProperty('installation_id') | ||
| expect(h.getDeviceId).not.toHaveBeenCalled() | ||
| } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer it.each over an in-test for loop for the consent matrix.
A failure in the undefined iteration reports as the same test as false, and mocks aren't reset between iterations, so state can bleed. Two cases, two clean runs — no need to make them share a cage.
♻️ Proposed refactor
- it('omits installation_id when telemetry consent is off or undecided', async () => {
- for (const consent of [false, undefined]) {
- h.settingsGet.mockReturnValue(consent)
- h.createDesktopLoginCode.mockResolvedValue(GRANT)
- h.exchangeDesktopLoginCode.mockResolvedValue({
- status: 'complete',
- custom_token: 'custom-token-value'
- })
- mockSignInChain({ uid: 'uid-1' })
- const mod = await loadOrchestrator()
-
- const promise = mod.signInViaDesktopLoginCode(AUTH_URL, fakeContents(), {})
- await vi.runAllTimersAsync()
- await promise
-
- const request = h.createDesktopLoginCode.mock.lastCall![1] as Record<string, unknown>
- expect(request).not.toHaveProperty('installation_id')
- expect(h.getDeviceId).not.toHaveBeenCalled()
- }
- })
+ it.each([false, undefined])(
+ 'omits installation_id when telemetry consent is %s',
+ async (consent) => {
+ h.settingsGet.mockReturnValue(consent)
+ h.createDesktopLoginCode.mockResolvedValue(GRANT)
+ h.exchangeDesktopLoginCode.mockResolvedValue({
+ status: 'complete',
+ custom_token: 'custom-token-value'
+ })
+ mockSignInChain({ uid: 'uid-1' })
+ const mod = await loadOrchestrator()
+
+ const promise = mod.signInViaDesktopLoginCode(AUTH_URL, fakeContents(), {})
+ await vi.runAllTimersAsync()
+ await promise
+
+ const request = h.createDesktopLoginCode.mock.lastCall![1] as Record<string, unknown>
+ expect(request).not.toHaveProperty('installation_id')
+ expect(h.getDeviceId).not.toHaveBeenCalled()
+ }
+ )📝 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
| it('omits installation_id when telemetry consent is off or undecided', async () => { | |
| for (const consent of [false, undefined]) { | |
| h.settingsGet.mockReturnValue(consent) | |
| h.createDesktopLoginCode.mockResolvedValue(GRANT) | |
| h.exchangeDesktopLoginCode.mockResolvedValue({ | |
| status: 'complete', | |
| custom_token: 'custom-token-value' | |
| }) | |
| mockSignInChain({ uid: 'uid-1' }) | |
| const mod = await loadOrchestrator() | |
| const promise = mod.signInViaDesktopLoginCode(AUTH_URL, fakeContents(), {}) | |
| await vi.runAllTimersAsync() | |
| await promise | |
| const request = h.createDesktopLoginCode.mock.lastCall![1] as Record<string, unknown> | |
| expect(request).not.toHaveProperty('installation_id') | |
| expect(h.getDeviceId).not.toHaveBeenCalled() | |
| } | |
| }) | |
| it.each([false, undefined])( | |
| 'omits installation_id when telemetry consent is %s', | |
| async (consent) => { | |
| h.settingsGet.mockReturnValue(consent) | |
| h.createDesktopLoginCode.mockResolvedValue(GRANT) | |
| h.exchangeDesktopLoginCode.mockResolvedValue({ | |
| status: 'complete', | |
| custom_token: 'custom-token-value' | |
| }) | |
| mockSignInChain({ uid: 'uid-1' }) | |
| const mod = await loadOrchestrator() | |
| const promise = mod.signInViaDesktopLoginCode(AUTH_URL, fakeContents(), {}) | |
| await vi.runAllTimersAsync() | |
| await promise | |
| const request = h.createDesktopLoginCode.mock.lastCall![1] as Record<string, unknown> | |
| expect(request).not.toHaveProperty('installation_id') | |
| expect(h.getDeviceId).not.toHaveBeenCalled() | |
| } | |
| ) |
🤖 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 `@src/main/auth/desktopLoginCode/index.test.ts` around lines 324 - 343, Replace
the in-test consent loop in the “omits installation_id when telemetry consent is
off or undecided” case with an it.each table covering false and undefined, so
each consent value runs as a separately reported test with isolated setup and
mocks. Keep the existing assertions and sign-in flow unchanged for both cases.
Comment on lines
+9
to
+16
| export function hangingFetch(): typeof fetch { | ||
| return (...args: Parameters<typeof fetch>) => | ||
| new Promise<Response>((_resolve, reject) => { | ||
| args[1]?.signal?.addEventListener('abort', () => | ||
| reject(new DOMException('This operation was aborted', 'AbortError')) | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
hangingFetch hangs forever if the signal is already aborted.
The promise only settles from the abort listener, so a pre-aborted signal never rejects and the suite stalls until Vitest's timeout instead of failing fast. Currently unreached (client.test.ts aborts after the call), but it's a one-liner to make the stub honest — a stub that stalls is a stall that's dull.
🛡️ Proposed fix
export function hangingFetch(): typeof fetch {
return (...args: Parameters<typeof fetch>) =>
new Promise<Response>((_resolve, reject) => {
- args[1]?.signal?.addEventListener('abort', () =>
- reject(new DOMException('This operation was aborted', 'AbortError'))
- )
+ const abortError = (): DOMException =>
+ new DOMException('This operation was aborted', 'AbortError')
+ const signal = args[1]?.signal
+ if (signal?.aborted) {
+ reject(abortError())
+ return
+ }
+ signal?.addEventListener('abort', () => reject(abortError()), { once: true })
})
}📝 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
| export function hangingFetch(): typeof fetch { | |
| return (...args: Parameters<typeof fetch>) => | |
| new Promise<Response>((_resolve, reject) => { | |
| args[1]?.signal?.addEventListener('abort', () => | |
| reject(new DOMException('This operation was aborted', 'AbortError')) | |
| ) | |
| }) | |
| } | |
| export function hangingFetch(): typeof fetch { | |
| return (...args: Parameters<typeof fetch>) => | |
| new Promise<Response>((_resolve, reject) => { | |
| const abortError = (): DOMException => | |
| new DOMException('This operation was aborted', 'AbortError') | |
| const signal = args[1]?.signal | |
| if (signal?.aborted) { | |
| reject(abortError()) | |
| return | |
| } | |
| signal?.addEventListener('abort', () => reject(abortError()), { once: 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 `@src/main/auth/desktopLoginCode/testHelpers.ts` around lines 9 - 16, Update
hangingFetch so it checks args[1]?.signal?.aborted before registering the abort
listener and immediately rejects with the same AbortError for an already-aborted
signal; retain the existing listener behavior for signals that abort after the
fetch call.
Comment on lines
+65
to
+95
| export function showCopyLinkBanner(comfyContents: WebContents, loginUrl: string): void { | ||
| if (comfyContents.isDestroyed()) return | ||
|
|
||
| const labels = { | ||
| message: i18n.t('cloud.signInBanner.message'), | ||
| copy: i18n.t('cloud.signInBanner.copy'), | ||
| copied: i18n.t('cloud.signInBanner.copied'), | ||
| openAgain: i18n.t('cloud.signInBanner.openAgain'), | ||
| dismiss: i18n.t('cloud.signInBanner.dismiss') | ||
| } | ||
|
|
||
| void comfyContents | ||
| .insertCSS(COPY_LINK_BANNER_CSS) | ||
| .then(() => comfyContents.executeJavaScript(buildCopyLinkBannerScript(loginUrl, labels), true)) | ||
| .catch(() => {}) | ||
|
|
||
| const onConsoleMessage = ( | ||
| details: Electron.Event<Electron.WebContentsConsoleMessageEventParams> | ||
| ): void => { | ||
| if (details.frame?.parent != null || details.message !== OPEN_LINK_SENTINEL) return | ||
| openExternalSafely(loginUrl) | ||
| } | ||
| comfyContents.on('console-message', onConsoleMessage) | ||
|
|
||
| activeBannerCleanup = () => { | ||
| comfyContents.off('console-message', onConsoleMessage) | ||
| if (!comfyContents.isDestroyed()) { | ||
| void comfyContents.executeJavaScript(buildRemoveCopyLinkBannerScript(), true).catch(() => {}) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
showCopyLinkBanner silently orphans a previous banner's teardown.
Line 89 overwrites activeBannerCleanup unconditionally, so if any caller forgets the runBannerCleanup() prelude, the earlier console-message listener is leaked on the WebContents for its lifetime and the stale banner DOM never goes away. Today both call sites happen to do it (index.ts Line 98 and the login-code flow), but that contract is enforced by comment only. Own the teardown here and the convention can't rot. Better to sweep the old card off the table before you deal a new one.
♻️ Make the singleton self-enforcing
export function showCopyLinkBanner(comfyContents: WebContents, loginUrl: string): void {
+ // Owning the teardown here means a caller can never stack two cards or
+ // leak the previous attempt's console listener.
+ runBannerCleanup()
if (comfyContents.isDestroyed()) return📝 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
| export function showCopyLinkBanner(comfyContents: WebContents, loginUrl: string): void { | |
| if (comfyContents.isDestroyed()) return | |
| const labels = { | |
| message: i18n.t('cloud.signInBanner.message'), | |
| copy: i18n.t('cloud.signInBanner.copy'), | |
| copied: i18n.t('cloud.signInBanner.copied'), | |
| openAgain: i18n.t('cloud.signInBanner.openAgain'), | |
| dismiss: i18n.t('cloud.signInBanner.dismiss') | |
| } | |
| void comfyContents | |
| .insertCSS(COPY_LINK_BANNER_CSS) | |
| .then(() => comfyContents.executeJavaScript(buildCopyLinkBannerScript(loginUrl, labels), true)) | |
| .catch(() => {}) | |
| const onConsoleMessage = ( | |
| details: Electron.Event<Electron.WebContentsConsoleMessageEventParams> | |
| ): void => { | |
| if (details.frame?.parent != null || details.message !== OPEN_LINK_SENTINEL) return | |
| openExternalSafely(loginUrl) | |
| } | |
| comfyContents.on('console-message', onConsoleMessage) | |
| activeBannerCleanup = () => { | |
| comfyContents.off('console-message', onConsoleMessage) | |
| if (!comfyContents.isDestroyed()) { | |
| void comfyContents.executeJavaScript(buildRemoveCopyLinkBannerScript(), true).catch(() => {}) | |
| } | |
| } | |
| } | |
| export function showCopyLinkBanner(comfyContents: WebContents, loginUrl: string): void { | |
| // Owning the teardown here means a caller can never stack two cards or | |
| // leak the previous attempt's console listener. | |
| runBannerCleanup() | |
| if (comfyContents.isDestroyed()) return | |
| const labels = { | |
| message: i18n.t('cloud.signInBanner.message'), | |
| copy: i18n.t('cloud.signInBanner.copy'), | |
| copied: i18n.t('cloud.signInBanner.copied'), | |
| openAgain: i18n.t('cloud.signInBanner.openAgain'), | |
| dismiss: i18n.t('cloud.signInBanner.dismiss') | |
| } | |
| void comfyContents | |
| .insertCSS(COPY_LINK_BANNER_CSS) | |
| .then(() => comfyContents.executeJavaScript(buildCopyLinkBannerScript(loginUrl, labels), true)) | |
| .catch(() => {}) | |
| const onConsoleMessage = ( | |
| details: Electron.Event<Electron.WebContentsConsoleMessageEventParams> | |
| ): void => { | |
| if (details.frame?.parent != null || details.message !== OPEN_LINK_SENTINEL) return | |
| openExternalSafely(loginUrl) | |
| } | |
| comfyContents.on('console-message', onConsoleMessage) | |
| activeBannerCleanup = () => { | |
| comfyContents.off('console-message', onConsoleMessage) | |
| if (!comfyContents.isDestroyed()) { | |
| void comfyContents.executeJavaScript(buildRemoveCopyLinkBannerScript(), true).catch(() => {}) | |
| } | |
| } | |
| } |
🤖 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 `@src/main/auth/firebaseBridge/flowState.ts` around lines 65 - 95, Update
showCopyLinkBanner to invoke the existing activeBannerCleanup teardown before
installing a new banner, then replace it with the current banner’s cleanup
function. Preserve the existing listener removal and stale-banner DOM cleanup
behavior, making the singleton self-enforcing without relying on callers to run
cleanup first.
Comment on lines
+157
to
+162
| await vi.advanceTimersByTimeAsync(3000) | ||
| await staleFlow | ||
|
|
||
| expect(h.injectSession).not.toHaveBeenCalled() | ||
| expect(h.bindUserId).not.toHaveBeenCalled() | ||
| expect(h.restoreParentWindow).not.toHaveBeenCalled() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hardcoded 3000 must out-run POST_SIGNIN_HOLD_MS, or this test goes false-green.
The assertions on Lines 160-162 are all negative. If POST_SIGNIN_HOLD_MS is ever raised above 3000, the stale flow is still parked inside abortableSleep when they run, so they pass for entirely the wrong reason and the supersession guard stops being tested. POST_SIGNIN_HOLD_MS is already exported from the module under test — advance by it instead of guessing. Don't let a magic number keep the test asleep on the job.
As per coding guidelines: "Flaky tests must be fixed immediately; avoid brittle timing assertions".
💚 Proposed fix
import {
closeActiveBridge,
handleFirebasePopup,
+ POST_SIGNIN_HOLD_MS,
runBannerCleanup,
showCopyLinkBanner
} from './index' showCopyLinkBanner(contents, 'https://cloud.comfy.org/new-login')
- await vi.advanceTimersByTimeAsync(3000)
+ await vi.advanceTimersByTimeAsync(POST_SIGNIN_HOLD_MS + 1)
await staleFlow📝 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
| await vi.advanceTimersByTimeAsync(3000) | |
| await staleFlow | |
| expect(h.injectSession).not.toHaveBeenCalled() | |
| expect(h.bindUserId).not.toHaveBeenCalled() | |
| expect(h.restoreParentWindow).not.toHaveBeenCalled() | |
| await vi.advanceTimersByTimeAsync(POST_SIGNIN_HOLD_MS + 1) | |
| await staleFlow | |
| expect(h.injectSession).not.toHaveBeenCalled() | |
| expect(h.bindUserId).not.toHaveBeenCalled() | |
| expect(h.restoreParentWindow).not.toHaveBeenCalled() |
🤖 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 `@src/main/auth/firebaseBridge/index.test.ts` around lines 157 - 162, Update
the timer advancement in the stale-flow test to use the exported
POST_SIGNIN_HOLD_MS value from the module under test, ensuring the delay exceeds
the configured hold period before checking the negative assertions. Remove the
hardcoded 3000 timing value while preserving the existing stale-flow and
supersession assertions.
Source: Coding guidelines
Comment on lines
+234
to
+247
| it('continues through the banner when the initial browser open rejects', async () => { | ||
| h.openExternal.mockRejectedValueOnce(new Error('no default browser')) | ||
| h.startBridgeServer.mockResolvedValue({ | ||
| url: 'http://localhost:9876/', | ||
| signInPromise: Promise.resolve({ user: { uid: 'user-1' }, apiKey: 'api-key' }), | ||
| close: vi.fn() | ||
| }) | ||
|
|
||
| const flow = handleFirebasePopup(AUTH_URL, fakeContents()) | ||
| await vi.runAllTimersAsync() | ||
|
|
||
| await expect(flow).resolves.toBeUndefined() | ||
| expect(h.emit).not.toHaveBeenCalled() | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No test asserts the legacy happy path actually completes.
Every legacy-flow case here is a negative assertion. This one proves a rejected openExternal doesn't emit a failure, but never checks that the flow still reached injectSession / bindUserId / restoreParentWindow — so a regression that made the legacy path silently return early would keep the whole suite green. Two extra positive assertions close the hole.
♻️ Suggested additions
await expect(flow).resolves.toBeUndefined()
expect(h.emit).not.toHaveBeenCalled()
+ expect(h.injectSession).toHaveBeenCalledOnce()
+ expect(h.bindUserId).toHaveBeenCalled()
+ expect(h.restoreParentWindow).toHaveBeenCalledOnce()
})📝 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
| it('continues through the banner when the initial browser open rejects', async () => { | |
| h.openExternal.mockRejectedValueOnce(new Error('no default browser')) | |
| h.startBridgeServer.mockResolvedValue({ | |
| url: 'http://localhost:9876/', | |
| signInPromise: Promise.resolve({ user: { uid: 'user-1' }, apiKey: 'api-key' }), | |
| close: vi.fn() | |
| }) | |
| const flow = handleFirebasePopup(AUTH_URL, fakeContents()) | |
| await vi.runAllTimersAsync() | |
| await expect(flow).resolves.toBeUndefined() | |
| expect(h.emit).not.toHaveBeenCalled() | |
| }) | |
| it('continues through the banner when the initial browser open rejects', async () => { | |
| h.openExternal.mockRejectedValueOnce(new Error('no default browser')) | |
| h.startBridgeServer.mockResolvedValue({ | |
| url: 'http://localhost:9876/', | |
| signInPromise: Promise.resolve({ user: { uid: 'user-1' }, apiKey: 'api-key' }), | |
| close: vi.fn() | |
| }) | |
| const flow = handleFirebasePopup(AUTH_URL, fakeContents()) | |
| await vi.runAllTimersAsync() | |
| await expect(flow).resolves.toBeUndefined() | |
| expect(h.emit).not.toHaveBeenCalled() | |
| expect(h.injectSession).toHaveBeenCalledOnce() | |
| expect(h.bindUserId).toHaveBeenCalled() | |
| expect(h.restoreParentWindow).toHaveBeenCalledOnce() | |
| }) |
🤖 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 `@src/main/auth/firebaseBridge/index.test.ts` around lines 234 - 247, Add
positive assertions to the legacy-flow test around handleFirebasePopup so it
verifies the successful path reaches injectSession, bindUserId, and
restoreParentWindow after openExternal rejects. Keep the existing resolution and
no-emission assertions unchanged.
Comment on lines
+84
to
107
| mainTelemetry.capture('comfy.desktop.auth.sign_in_started', { | ||
| provider: providerId, | ||
| flow: LEGACY_AUTH_FLOW | ||
| }) | ||
| const env = detectFirebaseEnv(url) | ||
|
|
||
| // Kill any stale bridge from a prior sign-in attempt the user | ||
| // didn't complete. Without this, the second Sign-in click hits an | ||
| // EADDRINUSE on the fixed loopback port and the user sees an | ||
| // unhelpful auth/popup-blocked error from the embedded view (we | ||
| // denied the popup but couldn't open the replacement bridge). | ||
| if (activeBridge) { | ||
| try { | ||
| activeBridge.close() | ||
| } catch { | ||
| // best-effort | ||
| } | ||
| activeBridge = null | ||
| } | ||
| // Kill any stale bridge from a prior sign-in attempt the user didn't | ||
| // complete — otherwise the user sees an unhelpful auth/popup-blocked | ||
| // error from the embedded view (we denied the popup but couldn't open | ||
| // the replacement bridge on the taken port). | ||
| const flow = beginActiveBridgeFlow() | ||
| const sessionInjection = beginFirebaseSessionInjection(comfyContents) | ||
| // Clear a prior attempt's "copy link" card + its console listener so a | ||
| // new attempt doesn't stack a second card or leak a stale listener. | ||
| runBannerCleanup() | ||
|
|
||
| let handle: Awaited<ReturnType<typeof startBridgeServer>> | null = null | ||
| const { signal } = flow.controller | ||
| // Origin the embedded view sits on when the flow starts. The inject below | ||
| // writes the Firebase refresh token into whatever page is loaded, and the | ||
| // browser sign-in in between can take minutes — so pin it now and re-check | ||
| // before injecting rather than assuming the view stayed put. | ||
| const startOrigin = originOf(comfyContents.getURL()) | ||
| let handle: BridgeHandle | null = null | ||
| try { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Do other main-process call sites guard WebContents access with isDestroyed()
# or try/catch before calling getURL()?
rg -nP --type=ts -C3 '\.getURL\s*\(' src/main | head -80
ast-grep outline src/main/auth/firebaseBridge --items allRepository: Comfy-Org/Comfy-Desktop
Length of output: 8873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant firebaseBridge index.ts =="
sed -n '1,180p' src/main/auth/firebaseBridge/index.ts
echo
echo "== relevant firebaseBridge inject.ts =="
sed -n '1,140p' src/main/auth/firebaseBridge/inject.ts
echo
echo "== relevant flowState.ts =="
sed -n '1,180p' src/main/auth/firebaseBridge/flowState.ts
echo
echo "== relevant desktopLoginCode handleFirebasePopup call site =="
sed -n '1,180p' src/main/auth/desktopLoginCode/index.ts
echo
echo "== package electron version =="
if [ -f package.json ]; then jq -r '.dependencies.electron // .devDependencies.electron // empty' package.json; fi
if [ -f package-lock.json ]; then jq -r '(.packages["node_modules/electron"] // .dependencies.electron // .["dependencies"]?.electron // .["devDependencies"]?.electron) as $x| if $x then (if (type=="string") then $x elif $x.version then $x.version else empty end) else empty end' package-lock.json | head -5; fiRepository: Comfy-Org/Comfy-Desktop
Length of output: 24179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== login-code banner injection and destroy handling continuation =="
sed -n '200,420p' src/main/auth/desktopLoginCode/index.ts
echo
echo "== destroy/event handlers for cloud contents =="
rg -n --type=ts -C2 "isDestroyed|destroyed|did-fail-load|showCopyLinkBanner|bindSignedInUser|releaseFirebaseSessionInjection|webContents" src/main src/shared 2>/dev/null | head -220Repository: Comfy-Org/Comfy-Desktop
Length of output: 18964
Guard the legacy bridge pre-flight by the embedded view’s lifecycle.
The login-code path can await outside here, but the legacy fallback still touches destroyed WebContents before the try: beginFirebaseSessionInjection(comfyContents), runBannerCleanup(), and comfyContents.getURL() can throw and create an unhandled rejection. Return early when destroyed, then move the remaining pre-try setup inside the try/catch so no sign-in attempt sails ashore without its guard rails. 🏝️
🤖 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 `@src/main/auth/firebaseBridge/index.ts` around lines 84 - 107, Guard the
legacy sign-in pre-flight in the surrounding sign-in function by returning
immediately when comfyContents is destroyed, before calling
beginFirebaseSessionInjection, runBannerCleanup, or getURL. Move those setup
calls, including flow-dependent initialization and startOrigin assignment,
inside the existing try/catch so lifecycle errors are handled rather than
becoming unhandled rejections.
Comment on lines
+63
to
+85
| it('does not let stale cleanup delete a replacement flow record', () => { | ||
| const staleOwner = 'stale-owner' | ||
| const replacementOwner = 'replacement-owner' | ||
| let currentRecord: Record<string, unknown> | null = { | ||
| fbase_key: 'firebase:authUser:key:[DEFAULT]', | ||
| value: { uid: 'old-user' }, | ||
| [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: staleOwner | ||
| } | ||
| expect(isFirebaseSessionInjectionRecordOwnedBy(currentRecord, staleOwner)).toBe(true) | ||
|
|
||
| // Replacement write wins before the stale flow's compensating cleanup. | ||
| currentRecord = { | ||
| fbase_key: 'firebase:authUser:key:[DEFAULT]', | ||
| value: { uid: 'new-user' }, | ||
| [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: replacementOwner | ||
| } | ||
| if (isFirebaseSessionInjectionRecordOwnedBy(currentRecord, staleOwner)) currentRecord = null | ||
|
|
||
| expect(currentRecord).toMatchObject({ | ||
| value: { uid: 'new-user' }, | ||
| [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: replacementOwner | ||
| }) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This test asserts on its own re-implementation, not on the shipped cleanup logic.
Lines 74-79 hand-roll the stale-cleanup rule in the test body and then line 81 asserts the record the test itself just assigned. The real guard lives inside the string returned by buildIndexedDbInjectScript (current.result[recordOwnerKey] === ownerToken → cleanupStore.delete), and nothing here exercises it. Rename it for what it covers — isFirebaseSessionInjectionRecordOwnedBy returns false for a foreign owner — or drive the actual script against a fake IDB. A test that grades its own homework always scores an A.
♻️ Minimal honest version of the assertion
- it('does not let stale cleanup delete a replacement flow record', () => {
+ it('does not treat a replacement flow record as owned by the stale flow', () => {
const staleOwner = 'stale-owner'
const replacementOwner = 'replacement-owner'
- let currentRecord: Record<string, unknown> | null = {
+ const staleRecord = {
fbase_key: 'firebase:authUser:key:[DEFAULT]',
value: { uid: 'old-user' },
[FIREBASE_SESSION_INJECTION_OWNER_FIELD]: staleOwner
}
- expect(isFirebaseSessionInjectionRecordOwnedBy(currentRecord, staleOwner)).toBe(true)
-
- // Replacement write wins before the stale flow's compensating cleanup.
- currentRecord = {
+ const replacementRecord = {
fbase_key: 'firebase:authUser:key:[DEFAULT]',
value: { uid: 'new-user' },
[FIREBASE_SESSION_INJECTION_OWNER_FIELD]: replacementOwner
}
- if (isFirebaseSessionInjectionRecordOwnedBy(currentRecord, staleOwner)) currentRecord = null
-
- expect(currentRecord).toMatchObject({
- value: { uid: 'new-user' },
- [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: replacementOwner
- })
+ expect(isFirebaseSessionInjectionRecordOwnedBy(staleRecord, staleOwner)).toBe(true)
+ // The stale flow's compensating cleanup must not match the replacement write.
+ expect(isFirebaseSessionInjectionRecordOwnedBy(replacementRecord, staleOwner)).toBe(false)
+ expect(isFirebaseSessionInjectionRecordOwnedBy(null, staleOwner)).toBe(false)
})📝 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
| it('does not let stale cleanup delete a replacement flow record', () => { | |
| const staleOwner = 'stale-owner' | |
| const replacementOwner = 'replacement-owner' | |
| let currentRecord: Record<string, unknown> | null = { | |
| fbase_key: 'firebase:authUser:key:[DEFAULT]', | |
| value: { uid: 'old-user' }, | |
| [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: staleOwner | |
| } | |
| expect(isFirebaseSessionInjectionRecordOwnedBy(currentRecord, staleOwner)).toBe(true) | |
| // Replacement write wins before the stale flow's compensating cleanup. | |
| currentRecord = { | |
| fbase_key: 'firebase:authUser:key:[DEFAULT]', | |
| value: { uid: 'new-user' }, | |
| [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: replacementOwner | |
| } | |
| if (isFirebaseSessionInjectionRecordOwnedBy(currentRecord, staleOwner)) currentRecord = null | |
| expect(currentRecord).toMatchObject({ | |
| value: { uid: 'new-user' }, | |
| [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: replacementOwner | |
| }) | |
| }) | |
| it('does not treat a replacement flow record as owned by the stale flow', () => { | |
| const staleOwner = 'stale-owner' | |
| const replacementOwner = 'replacement-owner' | |
| const staleRecord = { | |
| fbase_key: 'firebase:authUser:key:[DEFAULT]', | |
| value: { uid: 'old-user' }, | |
| [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: staleOwner | |
| } | |
| const replacementRecord = { | |
| fbase_key: 'firebase:authUser:key:[DEFAULT]', | |
| value: { uid: 'new-user' }, | |
| [FIREBASE_SESSION_INJECTION_OWNER_FIELD]: replacementOwner | |
| } | |
| expect(isFirebaseSessionInjectionRecordOwnedBy(staleRecord, staleOwner)).toBe(true) | |
| // The stale flow's compensating cleanup must not match the replacement write. | |
| expect(isFirebaseSessionInjectionRecordOwnedBy(replacementRecord, staleOwner)).toBe(false) | |
| expect(isFirebaseSessionInjectionRecordOwnedBy(null, staleOwner)).toBe(false) | |
| }) |
🤖 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 `@src/main/auth/firebaseBridge/inject.test.ts` around lines 63 - 85, Replace
the test’s hand-rolled cleanup simulation with an assertion focused on
isFirebaseSessionInjectionRecordOwnedBy: verify it returns false when the record
carries replacementOwner but staleOwner is supplied. Rename the test to describe
foreign-owner rejection, or instead execute buildIndexedDbInjectScript against a
fake IndexedDB to cover the actual cleanupStore.delete guard; do not assert
behavior derived solely from assignments in the test.
Comment on lines
+88
to
+114
| describe('injectFirebaseSession', () => { | ||
| it('executes on the captured main frame and invalidates it when superseded', async () => { | ||
| const executeJavaScript = vi | ||
| .fn() | ||
| .mockResolvedValueOnce(undefined) | ||
| .mockResolvedValueOnce(true) | ||
| .mockResolvedValue(undefined) | ||
| const frame = { | ||
| executeJavaScript, | ||
| isDestroyed: vi.fn(() => false) | ||
| } | ||
| const contents = { | ||
| getURL: vi.fn(() => `${ORIGIN}/workspaces/test`), | ||
| isDestroyed: vi.fn(() => false), | ||
| mainFrame: frame | ||
| } as unknown as Electron.WebContents | ||
|
|
||
| const first = beginFirebaseSessionInjection(contents) | ||
| await expect(injectFirebaseSession(first, ORIGIN, SAMPLE_USER, 'AIzaTEST')).resolves.toBe(true) | ||
|
|
||
| const second = beginFirebaseSessionInjection(contents) | ||
| expect(executeJavaScript).toHaveBeenLastCalledWith( | ||
| expect.stringContaining('= null') | ||
| ) | ||
|
|
||
| releaseFirebaseSessionInjection(second) | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The frame-replacement guard — a headline of this PR — is never exercised.
contents.mainFrame is a fixed object here, so injectFirebaseSession's post-owner-script re-check (injection.contents.mainFrame !== frame and frame.isDestroyed()) always passes. The test title claims "executes on the captured main frame", but nothing swaps the frame out mid-flight. Add a case where mainFrame is replaced (or isDestroyed flips to true) between the two executeJavaScript resolutions and assert the result is false and that the IDB script never ran. Otherwise the guard against frame swaps is guarded by nothing.
🧪 Sketch of the missing case
it('refuses to write credentials when the main frame is replaced mid-injection', async () => {
const frame = { executeJavaScript: vi.fn(async () => undefined), isDestroyed: vi.fn(() => false) }
const contents = {
getURL: vi.fn(() => `${ORIGIN}/workspaces/test`),
isDestroyed: vi.fn(() => false),
mainFrame: frame
} as unknown as Electron.WebContents
// Swap the frame while the owner script is in flight.
frame.executeJavaScript.mockImplementationOnce(async () => {
;(contents as unknown as { mainFrame: unknown }).mainFrame = { ...frame }
})
const injection = beginFirebaseSessionInjection(contents)
await expect(injectFirebaseSession(injection, ORIGIN, SAMPLE_USER, 'AIzaTEST')).resolves.toBe(
false
)
expect(frame.executeJavaScript).toHaveBeenCalledTimes(1)
releaseFirebaseSessionInjection(injection)
})🤖 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 `@src/main/auth/firebaseBridge/inject.test.ts` around lines 88 - 114, Add a
test for injectFirebaseSession where contents.mainFrame is replaced or the
captured frame becomes destroyed during the first executeJavaScript call; assert
the injection resolves false and the captured frame executes only the owner
script, confirming the IDB credential script is skipped. Keep the existing
session setup and cleanup via beginFirebaseSessionInjection and
releaseFirebaseSessionInjection.
Comment on lines
112
to
141
| req.onerror = () => reject(new Error('open: ' + (req.error && req.error.message || 'unknown'))); | ||
| req.onsuccess = () => { | ||
| const db = req.result; | ||
| if (!isCurrent()) { db.close(); resolve(false); return; } | ||
| const tx = db.transaction('firebaseLocalStorage', 'readwrite'); | ||
| tx.oncomplete = () => { db.close(); resolve(); }; | ||
| tx.oncomplete = () => { | ||
| if (isCurrent()) { db.close(); resolve(true); return; } | ||
| const cleanup = db.transaction('firebaseLocalStorage', 'readwrite'); | ||
| cleanup.oncomplete = () => { db.close(); resolve(false); }; | ||
| cleanup.onerror = () => { db.close(); resolve(false); }; | ||
| const cleanupStore = cleanup.objectStore('firebaseLocalStorage'); | ||
| const current = cleanupStore.get(storageKey); | ||
| current.onsuccess = () => { | ||
| if (current.result && current.result[recordOwnerKey] === ownerToken) { | ||
| cleanupStore.delete(storageKey); | ||
| } | ||
| }; | ||
| }; | ||
| tx.onerror = () => reject(new Error('tx: ' + (tx.error && tx.error.message || 'unknown'))); | ||
| const store = tx.objectStore('firebaseLocalStorage'); | ||
| store.put({ fbase_key: storageKey, value: userValue }); | ||
| if (isCurrent()) { | ||
| store.put({ | ||
| fbase_key: storageKey, | ||
| value: userValue, | ||
| [recordOwnerKey]: ownerToken | ||
| }); | ||
| } | ||
| else { tx.abort(); db.close(); resolve(false); } | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the IndexedDB connection on the reject paths.
tx.onerror (Line 130) rejects while leaving db open, and the same holds for any later failure after req.onsuccess. A lingering open connection in the page blocks a future versionchange/onupgradeneeded for firebaseLocalStorageDb, so a leaked handle can quietly wedge a later upgrade. Every other exit already tidies up — don't let the error door swing open.
🧹 Proposed fix
- tx.onerror = () => reject(new Error('tx: ' + (tx.error && tx.error.message || 'unknown')));
+ tx.onerror = () => {
+ const message = tx.error && tx.error.message || 'unknown';
+ db.close();
+ reject(new Error('tx: ' + message));
+ };📝 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
| req.onerror = () => reject(new Error('open: ' + (req.error && req.error.message || 'unknown'))); | |
| req.onsuccess = () => { | |
| const db = req.result; | |
| if (!isCurrent()) { db.close(); resolve(false); return; } | |
| const tx = db.transaction('firebaseLocalStorage', 'readwrite'); | |
| tx.oncomplete = () => { db.close(); resolve(); }; | |
| tx.oncomplete = () => { | |
| if (isCurrent()) { db.close(); resolve(true); return; } | |
| const cleanup = db.transaction('firebaseLocalStorage', 'readwrite'); | |
| cleanup.oncomplete = () => { db.close(); resolve(false); }; | |
| cleanup.onerror = () => { db.close(); resolve(false); }; | |
| const cleanupStore = cleanup.objectStore('firebaseLocalStorage'); | |
| const current = cleanupStore.get(storageKey); | |
| current.onsuccess = () => { | |
| if (current.result && current.result[recordOwnerKey] === ownerToken) { | |
| cleanupStore.delete(storageKey); | |
| } | |
| }; | |
| }; | |
| tx.onerror = () => reject(new Error('tx: ' + (tx.error && tx.error.message || 'unknown'))); | |
| const store = tx.objectStore('firebaseLocalStorage'); | |
| store.put({ fbase_key: storageKey, value: userValue }); | |
| if (isCurrent()) { | |
| store.put({ | |
| fbase_key: storageKey, | |
| value: userValue, | |
| [recordOwnerKey]: ownerToken | |
| }); | |
| } | |
| else { tx.abort(); db.close(); resolve(false); } | |
| }; | |
| }); | |
| req.onerror = () => reject(new Error('open: ' + (req.error && req.error.message || 'unknown'))); | |
| req.onsuccess = () => { | |
| const db = req.result; | |
| if (!isCurrent()) { db.close(); resolve(false); return; } | |
| const tx = db.transaction('firebaseLocalStorage', 'readwrite'); | |
| tx.oncomplete = () => { | |
| if (isCurrent()) { db.close(); resolve(true); return; } | |
| const cleanup = db.transaction('firebaseLocalStorage', 'readwrite'); | |
| cleanup.oncomplete = () => { db.close(); resolve(false); }; | |
| cleanup.onerror = () => { db.close(); resolve(false); }; | |
| const cleanupStore = cleanup.objectStore('firebaseLocalStorage'); | |
| const current = cleanupStore.get(storageKey); | |
| current.onsuccess = () => { | |
| if (current.result && current.result[recordOwnerKey] === ownerToken) { | |
| cleanupStore.delete(storageKey); | |
| } | |
| }; | |
| }; | |
| tx.onerror = () => { | |
| const message = tx.error && tx.error.message || 'unknown'; | |
| db.close(); | |
| reject(new Error('tx: ' + message)); | |
| }; | |
| const store = tx.objectStore('firebaseLocalStorage'); | |
| if (isCurrent()) { | |
| store.put({ | |
| fbase_key: storageKey, | |
| value: userValue, | |
| [recordOwnerKey]: ownerToken | |
| }); | |
| } | |
| else { tx.abort(); db.close(); resolve(false); } | |
| }; | |
| }); |
🤖 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 `@src/main/auth/firebaseBridge/inject.ts` around lines 112 - 141, Update the
IndexedDB error handling in the request’s onsuccess flow to close db before
rejecting, including the tx.onerror path and any later transaction or operation
failures after req.onsuccess. Preserve the existing error messages and
resolution behavior while ensuring every rejection releases the connection.
Comment on lines
+16
to
+29
| const originalPlatform = process.platform | ||
|
|
||
| function stubProcess(platform: NodeJS.Platform, env: NodeJS.ProcessEnv = {}): void { | ||
| Object.defineProperty(process, 'platform', { configurable: true, value: platform }) | ||
| delete process.env.APPDATA | ||
| delete process.env.LOCALAPPDATA | ||
| Object.assign(process.env, env) | ||
| } | ||
|
|
||
| afterEach(() => { | ||
| Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) | ||
| delete process.env.APPDATA | ||
| delete process.env.LOCALAPPDATA | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore inherited environment variables after each test.
The cleanup permanently removes pre-existing APPDATA and LOCALAPPDATA; later tests in this worker can see an invalid Windows environment. Snapshot and restore both values—no test-state ghosts.
As per coding guidelines, “Flaky tests must be fixed immediately.”
Proposed fix
const originalPlatform = process.platform
+const originalAppData = process.env.APPDATA
+const originalLocalAppData = process.env.LOCALAPPDATA
+
+function restoreEnv(name: 'APPDATA' | 'LOCALAPPDATA', value: string | undefined): void {
+ if (value === undefined) delete process.env[name]
+ else process.env[name] = value
+}
afterEach(() => {
Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform })
- delete process.env.APPDATA
- delete process.env.LOCALAPPDATA
+ restoreEnv('APPDATA', originalAppData)
+ restoreEnv('LOCALAPPDATA', originalLocalAppData)
})📝 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 originalPlatform = process.platform | |
| function stubProcess(platform: NodeJS.Platform, env: NodeJS.ProcessEnv = {}): void { | |
| Object.defineProperty(process, 'platform', { configurable: true, value: platform }) | |
| delete process.env.APPDATA | |
| delete process.env.LOCALAPPDATA | |
| Object.assign(process.env, env) | |
| } | |
| afterEach(() => { | |
| Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) | |
| delete process.env.APPDATA | |
| delete process.env.LOCALAPPDATA | |
| }) | |
| const originalPlatform = process.platform | |
| const originalAppData = process.env.APPDATA | |
| const originalLocalAppData = process.env.LOCALAPPDATA | |
| function stubProcess(platform: NodeJS.Platform, env: NodeJS.ProcessEnv = {}): void { | |
| Object.defineProperty(process, 'platform', { configurable: true, value: platform }) | |
| delete process.env.APPDATA | |
| delete process.env.LOCALAPPDATA | |
| Object.assign(process.env, env) | |
| } | |
| function restoreEnv(name: 'APPDATA' | 'LOCALAPPDATA', value: string | undefined): void { | |
| if (value === undefined) delete process.env[name] | |
| else process.env[name] = value | |
| } | |
| afterEach(() => { | |
| Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) | |
| restoreEnv('APPDATA', originalAppData) | |
| restoreEnv('LOCALAPPDATA', originalLocalAppData) | |
| }) |
🤖 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 `@src/main/lib/desktopDetect.test.ts` around lines 16 - 29, Update the desktop
detection test setup around stubProcess and afterEach to snapshot the pre-test
APPDATA and LOCALAPPDATA values, then restore those exact values during cleanup
instead of always deleting them. Preserve deletion only when a variable was
originally absent, while continuing to restore process.platform.
Source: Coding guidelines
Member
Author
|
I left everything nonblocking off to keep the stack sanitary and get this in ASAP |
deepme987
approved these changes
Jul 28, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Split B of #1249.
Summary
This slice is behaviorally independent of the identity-core work in C, even though the GitHub stack is linear for clean diffs.
Stack
mainArchitecture
Validation
pnpm run typecheckpnpm run lintpnpm run buildpnpm run test— 2,948 passed, 1 skippedRC gate
Validate packaged login, fallback, cancellation/re-entry, origin navigation, and frame/session replacement before rollout.