Complete Desktop auth and PostHog identity stitching (GTM-93) - #1249
Closed
deepme987 wants to merge 19 commits into
Closed
Complete Desktop auth and PostHog identity stitching (GTM-93)#1249deepme987 wants to merge 19 commits into
deepme987 wants to merge 19 commits into
Conversation
…in codes (GTM-93)
…t timings Review follow-ups on the desktop-login-code flow: - signInWithCustomToken/lookupAccount now go through the same timeout-guarded postJson as the login-code endpoints, so a hung identitytoolkit call fails the flow instead of stalling it forever with the banner up and no sign_in_failed. - createDesktopLoginCode rejects grants with expires_in or poll_interval <= 0 (tight-poll spin / instant expiry) while the legacy-bridge fallback is still available.
# Conflicts: # src/main/auth/firebaseBridge/index.ts
Review follow-ups on #1222. Each fix has a test that fails without it. - Re-check the view's origin before injecting the session. The inject script carries the Firebase refresh token into the page's main world, and minutes elapse between flow start and injection (browser sign-in + post-sign-in hold) — ample time for the view to have navigated away. Applied to the legacy bridge too, which has the same gap; there it only enforces when the start origin was determinable, so it cannot regress sign-in. - Bound the server-supplied grant timings. `> 0` alone admits Infinity, which makes the code deadline unreachable — the poll loop's only exit — so the loop never terminates and its banner/listener cleanup never runs. A seconds/milliseconds unit slip is the realistic source, so clamp rather than trust: poll_interval to [1,30]s, expires_in to <=900s. - Retry 408/429. Each sign-in issues tens of exchange polls, so a rate limiter on that endpoint is expected; treating it as terminal killed a sign-in whose code and browser tab were both still valid. - Send code_challenge_method=S256. RFC 7636 4.3 defaults an omitted method to `plain`. Ingest pins S256 server-side, so this is not exploitable today — it makes the contract explicit on the wire instead of an implicit cross-repo convention. - Emit error_status on sign_in_failed. The raw message stays out (it can carry response bodies), but without the HTTP status every failure collapses into one error_class/error_bucket pair — an old backend (404), a verifier mismatch (403), and a 5xx are indistinguishable exactly when the rollout needs them separated. - Fail safe if signInViaDesktopLoginCode throws. It was the only await outside a try, and the call site is fire-and-forget, so an unexpected throw meant no flow, no fallback, no sign_in_failed, and an unhandled rejection — a Sign in button that silently did nothing. Tests: assert the PKCE binding across create/exchange (the verifier presented at exchange is the preimage of the challenge presented at create). Mutation-checked: downgrading the challenge to plaintext, sending an unrelated verifier, and removing the origin guard each fail the suite. 123 tests green in src/main/auth; typecheck + lint clean.
📝 WalkthroughWalkthroughThe PR adds Firebase authentication consensus reporting, strengthens login and origin validation, replaces download attribution with website-derived anonymous identity persistence, refactors telemetry identity handling, removes identity parameters from cloud URLs, and adds model-presence checks for bundled templates. ChangesDesktop authentication and consensus
Anonymous identity and telemetry
Model and template handling
Sequence Diagram(s)sequenceDiagram
participant CloudFrame
participant ComfyPreload
participant TelemetryIPC
participant FirebaseAuthIdentity
CloudFrame->>ComfyPreload: reportFirebaseAuthState(state)
ComfyPreload->>TelemetryIPC: telemetry:firebaseAuthState
TelemetryIPC->>FirebaseAuthIdentity: validate frame and state
FirebaseAuthIdentity->>FirebaseAuthIdentity: reconcile reporter consensus
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
vi.fn(async () => ...) with no declared parameters types mock.calls as an empty tuple, so calls[0][1] fails typecheck. The file's other fetch mocks declare Parameters<typeof fetch>; match them.
deepme987
marked this pull request as ready for review
July 14, 2026 22:07
benceruleanlu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/desktopAdopt.ts`:
- Around line 933-981: Add a concise comment near generatedDefaultArgs in
reconcileAdoptedSettings documenting that legacy pre-version launch defaults
were generated without selectedDevice, so the device-less deriveLaunchArgs call
must remain unchanged.
In `@src/main/lib/modelAccessPage.ts`:
- Around line 6-27: Confirm Hugging Face route and case-sensitivity behavior,
then update the model-access route classification to compare the owner
case-insensitively while preserving existing reserved-route handling. Ensure
HUGGING_FACE_RESERVED_ROUTES is documented or maintained as the authoritative
manually synchronized list for new top-level Hugging Face routes.
🪄 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: 0865ec82-3611-42f6-a139-c5946fe8b41a
📒 Files selected for processing (65)
.github/workflows/ci-cursor-review.ymllocales/en.jsonlocales/zh.jsonpackage.jsonpackages/comfyui-desktop-bridge-types/comfyDesktopBridge.d.tspackages/comfyui-desktop-bridge-types/package.jsonscripts/installer.nshsrc/main/auth/desktopLoginCode/client.test.tssrc/main/auth/desktopLoginCode/client.tssrc/main/auth/desktopLoginCode/index.test.tssrc/main/auth/desktopLoginCode/index.tssrc/main/auth/firebaseBridge/flowShared.tssrc/main/auth/firebaseBridge/index.test.tssrc/main/auth/firebaseBridge/index.tssrc/main/host/attach.tssrc/main/host/createHostWindow.tssrc/main/index.tssrc/main/lib/anonymousIdentity.test.tssrc/main/lib/anonymousIdentity.tssrc/main/lib/cloudCapacity.tssrc/main/lib/cloudUrl.test.tssrc/main/lib/cloudUrl.tssrc/main/lib/comfyDownloadManager.test.tssrc/main/lib/comfyDownloadManager.tssrc/main/lib/desktopAdopt.test.tssrc/main/lib/desktopAdopt.tssrc/main/lib/deviceId.test.tssrc/main/lib/deviceId.tssrc/main/lib/downloadAttribution.test.tssrc/main/lib/downloadAttribution.tssrc/main/lib/downloadFilename.tssrc/main/lib/experiments.test.tssrc/main/lib/experiments.tssrc/main/lib/firebaseAuthIdentity.test.tssrc/main/lib/firebaseAuthIdentity.tssrc/main/lib/ipc/index.tssrc/main/lib/ipc/registerDownloadHandlers.test.tssrc/main/lib/ipc/registerDownloadHandlers.tssrc/main/lib/ipc/registerTelemetryHandlers.test.tssrc/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/modelAccessPage.test.tssrc/main/lib/modelAccessPage.tssrc/main/lib/modelDownloadPaths.tssrc/main/lib/opaqueIdentifier.test.tssrc/main/lib/opaqueIdentifier.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/trustedCloudUrl.test.tssrc/main/lib/trustedCloudUrl.tssrc/main/lib/userTier.test.tssrc/main/lib/websiteAnonymousIdentity.tssrc/main/sources/standalone/index.tssrc/main/sources/standalone/templateInputAssets.tssrc/main/sources/standalone/templateModels.tssrc/preload/api.tssrc/preload/comfyPreload.test.tssrc/preload/comfyPreload.tssrc/renderer/src/assets/main.csssrc/renderer/src/components/TemplatePickerStep.vuesrc/renderer/src/components/settings/ComfyUISettingsContent.vuesrc/renderer/src/lib/rendererBootstrap.tssrc/renderer/src/views/comfyUISettings/ArgsBuilderPage.vuesrc/types/comfyDesktopBridge.tssrc/types/ipc.ts
💤 Files with no reviewable changes (4)
- src/main/lib/downloadAttribution.test.ts
- src/main/lib/downloadAttribution.ts
- src/renderer/src/views/comfyUISettings/ArgsBuilderPage.vue
- src/preload/api.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/lib/desktopAdopt.ts`:
- Around line 933-981: Add a concise comment near generatedDefaultArgs in
reconcileAdoptedSettings documenting that legacy pre-version launch defaults
were generated without selectedDevice, so the device-less deriveLaunchArgs call
must remain unchanged.
In `@src/main/lib/modelAccessPage.ts`:
- Around line 6-27: Confirm Hugging Face route and case-sensitivity behavior,
then update the model-access route classification to compare the owner
case-insensitively while preserving existing reserved-route handling. Ensure
HUGGING_FACE_RESERVED_ROUTES is documented or maintained as the authoritative
manually synchronized list for new top-level Hugging Face routes.
🪄 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: 0865ec82-3611-42f6-a139-c5946fe8b41a
📒 Files selected for processing (65)
.github/workflows/ci-cursor-review.ymllocales/en.jsonlocales/zh.jsonpackage.jsonpackages/comfyui-desktop-bridge-types/comfyDesktopBridge.d.tspackages/comfyui-desktop-bridge-types/package.jsonscripts/installer.nshsrc/main/auth/desktopLoginCode/client.test.tssrc/main/auth/desktopLoginCode/client.tssrc/main/auth/desktopLoginCode/index.test.tssrc/main/auth/desktopLoginCode/index.tssrc/main/auth/firebaseBridge/flowShared.tssrc/main/auth/firebaseBridge/index.test.tssrc/main/auth/firebaseBridge/index.tssrc/main/host/attach.tssrc/main/host/createHostWindow.tssrc/main/index.tssrc/main/lib/anonymousIdentity.test.tssrc/main/lib/anonymousIdentity.tssrc/main/lib/cloudCapacity.tssrc/main/lib/cloudUrl.test.tssrc/main/lib/cloudUrl.tssrc/main/lib/comfyDownloadManager.test.tssrc/main/lib/comfyDownloadManager.tssrc/main/lib/desktopAdopt.test.tssrc/main/lib/desktopAdopt.tssrc/main/lib/deviceId.test.tssrc/main/lib/deviceId.tssrc/main/lib/downloadAttribution.test.tssrc/main/lib/downloadAttribution.tssrc/main/lib/downloadFilename.tssrc/main/lib/experiments.test.tssrc/main/lib/experiments.tssrc/main/lib/firebaseAuthIdentity.test.tssrc/main/lib/firebaseAuthIdentity.tssrc/main/lib/ipc/index.tssrc/main/lib/ipc/registerDownloadHandlers.test.tssrc/main/lib/ipc/registerDownloadHandlers.tssrc/main/lib/ipc/registerTelemetryHandlers.test.tssrc/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/modelAccessPage.test.tssrc/main/lib/modelAccessPage.tssrc/main/lib/modelDownloadPaths.tssrc/main/lib/opaqueIdentifier.test.tssrc/main/lib/opaqueIdentifier.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/trustedCloudUrl.test.tssrc/main/lib/trustedCloudUrl.tssrc/main/lib/userTier.test.tssrc/main/lib/websiteAnonymousIdentity.tssrc/main/sources/standalone/index.tssrc/main/sources/standalone/templateInputAssets.tssrc/main/sources/standalone/templateModels.tssrc/preload/api.tssrc/preload/comfyPreload.test.tssrc/preload/comfyPreload.tssrc/renderer/src/assets/main.csssrc/renderer/src/components/TemplatePickerStep.vuesrc/renderer/src/components/settings/ComfyUISettingsContent.vuesrc/renderer/src/lib/rendererBootstrap.tssrc/renderer/src/views/comfyUISettings/ArgsBuilderPage.vuesrc/types/comfyDesktopBridge.tssrc/types/ipc.ts
💤 Files with no reviewable changes (4)
- src/main/lib/downloadAttribution.test.ts
- src/main/lib/downloadAttribution.ts
- src/renderer/src/views/comfyUISettings/ArgsBuilderPage.vue
- src/preload/api.ts
🛑 Comments failed to post (2)
src/main/lib/desktopAdopt.ts (1)
933-981: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Device-less
generatedDefaultArgsis the right call — but worth a one-liner to make it defect-proof.The equality guard at Line 958 compares
existing.launchArgsagainstderiveLaunchArgs({}).launchArgs, which omitsselectedDevice. This is correct because reconcile only touches records whoseadoptedSettingsVersion !== 1— i.e. legacy adoptions whose args were generated by the old device-lessderiveLaunchArgs, so a CPU install's stored default is still--port 8000 --enable-manager(no--cpu). The behavior matches the "repairs a missing settings file" test, so no change is needed to the code.Since the correctness hinges entirely on this "pre-version defaults never carried
--cpu" invariant, a short comment on Line 937 would keep a future refactor from innocently threadingselectedDevicein and silently breaking the match. A small note now saves a head-scratch later — measure once, comment twice.🤖 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/desktopAdopt.ts` around lines 933 - 981, Add a concise comment near generatedDefaultArgs in reconcileAdoptedSettings documenting that legacy pre-version launch defaults were generated without selectedDevice, so the device-less deriveLaunchArgs call must remain unchanged.src/main/lib/modelAccessPage.ts (1)
6-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Reserved-route denylist needs manual upkeep as Hugging Face adds routes.
Any future HF top-level path not added to
HUGGING_FACE_RESERVED_ROUTESwill be misclassified as a valid model repository. Navigation still stays sandboxed to huggingface.co, so it's not an exploit path — but it's a UX/correctness gap that silently drifts as HF's routing evolves. Also worth normalizing case in theownercomparison for good measure, no need to be so case-picky (case-y?) about it.♻️ Optional case-normalization hardening
function modelRepositoryPath(url: URL): string | null { const segments = url.pathname.split('/').filter(Boolean) if (segments.length !== 2) return null const [owner, repository] = segments - if (!owner || !repository || HUGGING_FACE_RESERVED_ROUTES.has(owner)) return null + if (!owner || !repository || HUGGING_FACE_RESERVED_ROUTES.has(owner.toLowerCase())) return null return `/${owner}/${repository}` }Since Hugging Face's actual route list isn't something I can verify from the repo alone, could you confirm (or have someone check) whether HF's routing is case-sensitive, and whether this list is expected to be kept manually in sync with any new top-level routes HF introduces?
Also applies to: 48-54
🤖 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/modelAccessPage.ts` around lines 6 - 27, Confirm Hugging Face route and case-sensitivity behavior, then update the model-access route classification to compare the owner case-insensitively while preserving existing reserved-route handling. Ensure HUGGING_FACE_RESERVED_ROUTES is documented or maintained as the authoritative manually synchronized list for new top-level Hugging Face routes.
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 `@src/main/auth/desktopLoginCode/index.ts`:
- Around line 232-251: Extract the shared origin validation predicate into a
helper such as requireOnOrigin in flowShared.ts, treating a missing origin or
contents on a different origin as failure. In
src/main/auth/desktopLoginCode/index.ts lines 232-251, replace the inline check
with the helper and preserve the existing throw behavior; in
src/main/auth/firebaseBridge/index.ts line 140, use the same helper and preserve
the existing return-on-failure 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 018af769-170f-4db3-8b48-54def02b0fe0
📒 Files selected for processing (13)
src/main/auth/desktopLoginCode/index.test.tssrc/main/auth/desktopLoginCode/index.tssrc/main/auth/firebaseBridge/index.test.tssrc/main/auth/firebaseBridge/index.tssrc/main/index.tssrc/main/lib/anonymousIdentity.test.tssrc/main/lib/deviceId.tssrc/main/lib/firebaseAuthIdentity.test.tssrc/main/lib/firebaseAuthIdentity.tssrc/main/lib/pendingIdentityMerge.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/websiteAnonymousIdentity.ts
Comment on lines
+232
to
+251
| // The backend/login origin and the embedded session target are distinct: | ||
| // local and preview views authenticate through production Cloud, but the | ||
| // resulting Firebase session must be injected back into the exact origin | ||
| // that initiated the flow. Pin that origin before any awaits and fail if | ||
| // the view moved while the browser flow was open. | ||
| if (!sessionTargetOrigin || !isOnOrigin(comfyContents, sessionTargetOrigin)) { | ||
| throw new Error('embedded view changed origin before session injection') | ||
| } | ||
| await comfyContents.executeJavaScript( | ||
| buildIndexedDbInjectScript(user, firebaseConfig.apiKey), | ||
| true | ||
| ) | ||
| if (controller.signal.aborted) return 'handled' | ||
| // Bind only after the session was successfully installed. Hosted Cloud | ||
| // views wait for their declarative auth reporter; local/legacy views use | ||
| // the main-verified fallback, so this produces exactly one success. | ||
| bindSignedInUser(user) | ||
| mainTelemetry.capture('comfy.desktop.identity.login_attributed', { | ||
| via: 'desktop_login_code' | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Extract the repeated origin-guard predicate into flowShared.ts. Both auth flows duplicate the same !origin || !isOnOrigin(contents, origin) check guarding session injection; a shared helper (e.g. requireOnOrigin(contents, origin)) would prevent this security-critical invariant from silently diverging between the two flows as they evolve.
src/main/auth/desktopLoginCode/index.ts#L232-L251: replace the inline!sessionTargetOrigin || !isOnOrigin(...)check with the shared helper, keeping thethrowon failure.src/main/auth/firebaseBridge/index.ts#L140-L140: replace the inline!startOrigin || !isOnOrigin(...)check with the same shared helper, keeping thereturnon failure.
♻️ Proposed shared helper
+export function requireOnOrigin(contents: WebContents, expectedOrigin: string | null): boolean {
+ return !!expectedOrigin && isOnOrigin(contents, expectedOrigin)
+}As per coding guidelines, "After creating or modifying code, check for duplicated logic and extract it into shared variables, computed properties, or helpers to prevent divergence."
📍 Affects 2 files
src/main/auth/desktopLoginCode/index.ts#L232-L251(this comment)src/main/auth/firebaseBridge/index.ts#L140-L140
🤖 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.ts` around lines 232 - 251, Extract the
shared origin validation predicate into a helper such as requireOnOrigin in
flowShared.ts, treating a missing origin or contents on a different origin as
failure. In src/main/auth/desktopLoginCode/index.ts lines 232-251, replace the
inline check with the helper and preserve the existing throw behavior; in
src/main/auth/firebaseBridge/index.ts line 140, use the same helper and preserve
the existing return-on-failure behavior.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/telemetry.ts (1)
792-821: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicated "rotate away from bound user" logic — extract a helper.
The not-granted-consent branch (~792-804) and the granted-consent branch (~809-821) both reset
boundUserId/nextAnonymousDistinctIdto a "safe anonymous id" (or fully null out identity on failure) with identical logic, differing only by the leadingcapturePersonProperties({ is_authenticated: false }, null)call. Two copies means two chances to drift apart the next time this "abandon-safely" behavior needs tweaking.♻️ Proposed extraction
+function rotateAwayFromBoundUser(): boolean { + const safeAnonymousId = nextAnonymousDistinctId + boundUserId = null + nextAnonymousDistinctId = null + if (!safeAnonymousId) { + distinctId = null + anonymousDistinctId = null + return false + } + anonymousDistinctId = safeAnonymousId + distinctId = safeAnonymousId + return true +} + if (consentState !== 'granted') { if (boundUserId) { - const safeAnonymousId = nextAnonymousDistinctId - boundUserId = null - nextAnonymousDistinctId = null - if (!safeAnonymousId) { - distinctId = null - anonymousDistinctId = null - return - } - anonymousDistinctId = safeAnonymousId - distinctId = safeAnonymousId + if (!rotateAwayFromBoundUser()) return } queuePendingUserBinding(normalizedUserId, emitLoginEvent, properties) return } if (boundUserId) { capturePersonProperties({ is_authenticated: false }, null) - const safeAnonymousId = nextAnonymousDistinctId - boundUserId = null - nextAnonymousDistinctId = null - if (!safeAnonymousId) { - distinctId = null - anonymousDistinctId = null - return - } - anonymousDistinctId = safeAnonymousId - distinctId = safeAnonymousId + if (!rotateAwayFromBoundUser()) return }As per coding guidelines, "After creating or modifying code, check for duplicated logic and extract it into shared variables, computed properties, or helpers to prevent divergence."
🤖 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/telemetry.ts` around lines 792 - 821, Extract the duplicated bound-user rotation and anonymous-identity reset logic from the consent branches into a shared helper, preserving its safe-ID fallback and full nulling behavior. Call the helper from both branches, keeping capturePersonProperties({ is_authenticated: false }, null) only in the granted-consent path.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.
Inline comments:
In `@src/main/lib/firebaseAuthIdentity.ts`:
- Around line 95-118: Replace repeated disk reads in
refreshReporterAuthScope/localExpectedUserIdForUrl with an in-memory cache owned
by verifiedLocalFirebaseAuth.ts, so loopback navigation checks reuse the cached
verified user instead of calling readVerifiedLocalFirebaseUser synchronously
each time. Update the cache whenever the verified identity is written or
cleared, including clearAcceptedLocalSignOut’s clear path, while preserving
existing origin-specific authorization behavior.
In `@src/preload/localFirebaseAuthMonitor.test.ts`:
- Around line 1-61: Add tests for the exported startLocalFirebaseAuthMonitor
entrypoint, using vi.useFakeTimers() and stubbing location and indexedDB to
cover loopback gating, the initial pending emission, serialized-state
deduplication, polling updates, and the returned stop function. Keep the
existing readLocalFirebaseAuthState tests unchanged and restore timers and
globals after each test.
In `@src/preload/localFirebaseAuthMonitor.ts`:
- Around line 78-92: Update the poll function’s await flow around
readLocalFirebaseAuthState so polling is always reset in a finally block,
including when the read rejects. Preserve the existing stopped check, state
serialization, and report behavior after a successful read.
---
Outside diff comments:
In `@src/main/lib/telemetry.ts`:
- Around line 792-821: Extract the duplicated bound-user rotation and
anonymous-identity reset logic from the consent branches into a shared helper,
preserving its safe-ID fallback and full nulling behavior. Call the helper from
both branches, keeping capturePersonProperties({ is_authenticated: false },
null) only in the granted-consent path.
🪄 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: b8741092-d590-4c1b-b1ff-f71cbdbb9f77
📒 Files selected for processing (12)
src/main/lib/anonymousIdentity.test.tssrc/main/lib/firebaseAuthIdentity.test.tssrc/main/lib/firebaseAuthIdentity.tssrc/main/lib/ipc/registerTelemetryHandlers.test.tssrc/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/pendingIdentityMerge.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/verifiedLocalFirebaseAuth.tssrc/preload/comfyPreload.tssrc/preload/localFirebaseAuthMonitor.test.tssrc/preload/localFirebaseAuthMonitor.ts
Comment on lines
+95
to
+118
| function localExpectedUserIdForUrl(url: string): string | null { | ||
| const origin = originOf(url) | ||
| return origin && isLoopbackOrigin(origin) ? readVerifiedLocalFirebaseUser(origin) : null | ||
| } | ||
|
|
||
| function refreshReporterAuthScope(reporter: Reporter, url: string): boolean { | ||
| const expectedUserId = localExpectedUserIdForUrl(url) | ||
| reporter.localExpectedUserId = expectedUserId | ||
| reporter.localReportingAuthorized = expectedUserId !== null | ||
| return isTrustedCloudUrl(url) || reporter.localReportingAuthorized | ||
| } | ||
|
|
||
| function clearAcceptedLocalSignOut( | ||
| webContents: WebContents, | ||
| reporter: Reporter, | ||
| origin: string | null, | ||
| state: ComfyDesktop2FirebaseAuthState | ||
| ): void { | ||
| if (!origin || !isLoopbackOrigin(origin) || state.status !== 'signed_out') return | ||
| clearVerifiedLocalFirebaseUser(origin) | ||
| reporter.localExpectedUserId = null | ||
| mainVerifiedStates.delete(webContents) | ||
| } | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Synchronous disk read on every navigation event for loopback views.
refreshReporterAuthScope → localExpectedUserIdForUrl → readVerifiedLocalFirebaseUser does a blocking fs.readFileSync + JSON.parse on Electron's main process, and it now fires on did-frame-navigate, settleFailedNavigation, and activateFirebaseAuthReporter for every loopback-origin view. The file is tiny today, but this is new blocking I/O on a hot navigation path in the main process — worth an in-memory cache (invalidated on write) in verifiedLocalFirebaseAuth.ts rather than re-reading from disk each time.
Also applies to: 394-400, 476-476, 330-331
🤖 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/firebaseAuthIdentity.ts` around lines 95 - 118, Replace repeated
disk reads in refreshReporterAuthScope/localExpectedUserIdForUrl with an
in-memory cache owned by verifiedLocalFirebaseAuth.ts, so loopback navigation
checks reuse the cached verified user instead of calling
readVerifiedLocalFirebaseUser synchronously each time. Update the cache whenever
the verified identity is written or cleared, including
clearAcceptedLocalSignOut’s clear path, while preserving existing
origin-specific authorization behavior.
Comment on lines
+1
to
+61
| import { afterEach, describe, expect, it } from 'vitest' | ||
| import { readLocalFirebaseAuthState } from './localFirebaseAuthMonitor' | ||
|
|
||
| const originalIndexedDb = globalThis.indexedDB | ||
|
|
||
| function successfulRequest<T>(result: T): IDBRequest<T> { | ||
| const request = { result } as IDBRequest<T> | ||
| queueMicrotask(() => request.onsuccess?.(new Event('success'))) | ||
| return request | ||
| } | ||
|
|
||
| function installIndexedDb(entries: unknown[] | null): void { | ||
| const database = { | ||
| close: () => {}, | ||
| objectStoreNames: { contains: () => entries !== null }, | ||
| transaction: () => ({ | ||
| objectStore: () => ({ | ||
| getAll: () => successfulRequest(entries ?? []) | ||
| }) | ||
| }) | ||
| } as unknown as IDBDatabase | ||
| globalThis.indexedDB = { | ||
| databases: async () => (entries === null ? [] : [{ name: 'firebaseLocalStorageDb' }]), | ||
| open: () => successfulRequest(database) | ||
| } as unknown as IDBFactory | ||
| } | ||
|
|
||
| afterEach(() => { | ||
| globalThis.indexedDB = originalIndexedDb | ||
| }) | ||
|
|
||
| describe('local Firebase auth monitor', () => { | ||
| it('reports the single persisted Firebase user', async () => { | ||
| installIndexedDb([ | ||
| { | ||
| fbase_key: 'firebase:authUser:api-key:[DEFAULT]', | ||
| value: { uid: 'firebase-user' } | ||
| } | ||
| ]) | ||
|
|
||
| await expect(readLocalFirebaseAuthState()).resolves.toEqual({ | ||
| status: 'signed_in', | ||
| userId: 'firebase-user' | ||
| }) | ||
| }) | ||
|
|
||
| it('fails pending when multiple Firebase projects disagree', async () => { | ||
| installIndexedDb([ | ||
| { fbase_key: 'firebase:authUser:a:[DEFAULT]', value: { uid: 'user-a' } }, | ||
| { fbase_key: 'firebase:authUser:b:[DEFAULT]', value: { uid: 'user-b' } } | ||
| ]) | ||
|
|
||
| await expect(readLocalFirebaseAuthState()).resolves.toEqual({ status: 'pending' }) | ||
| }) | ||
|
|
||
| it('reports signed out without Firebase persistence', async () => { | ||
| installIndexedDb(null) | ||
|
|
||
| await expect(readLocalFirebaseAuthState()).resolves.toEqual({ status: 'signed_out' }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for startLocalFirebaseAuthMonitor itself.
Only readLocalFirebaseAuthState is tested; the exported polling entrypoint that's actually wired into comfyPreload.ts (loopback gating, initial pending emit, dedup-by-serialized-state, and the returned stop function) has no tests. With vi.useFakeTimers() and a location/indexedDB stub this is straightforward to cover and would catch regressions in the dedup/stop logic that gates real auth-state reporting.
🤖 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/preload/localFirebaseAuthMonitor.test.ts` around lines 1 - 61, Add tests
for the exported startLocalFirebaseAuthMonitor entrypoint, using
vi.useFakeTimers() and stubbing location and indexedDB to cover loopback gating,
the initial pending emission, serialized-state deduplication, polling updates,
and the returned stop function. Keep the existing readLocalFirebaseAuthState
tests unchanged and restore timers and globals after each test.
Comment on lines
+78
to
+92
| const poll = async (): Promise<void> => { | ||
| if (stopped || polling) return | ||
| polling = true | ||
| const state = await readLocalFirebaseAuthState() | ||
| polling = false | ||
| if (stopped) return | ||
| const serialized = JSON.stringify(state) | ||
| if (serialized === lastState) return | ||
| lastState = serialized | ||
| report(state) | ||
| } | ||
| report({ status: 'pending' }) | ||
| lastState = JSON.stringify({ status: 'pending' }) | ||
| void poll() | ||
| const interval = setInterval(() => void poll(), POLL_INTERVAL_MS) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Reset polling in a finally for future-proofing.
polling is only reset after await readLocalFirebaseAuthState() completes normally; if that function's own try/catch is ever weakened, an unhandled rejection would permanently deadlock polling with no visible symptom. Currently safe, but cheap insurance against a silent, hard-to-diagnose stall.
🛡️ Proposed fix
const poll = async (): Promise<void> => {
if (stopped || polling) return
polling = true
- const state = await readLocalFirebaseAuthState()
- polling = false
+ let state: ComfyDesktop2FirebaseAuthState
+ try {
+ state = await readLocalFirebaseAuthState()
+ } finally {
+ polling = false
+ }
if (stopped) 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
| const poll = async (): Promise<void> => { | |
| if (stopped || polling) return | |
| polling = true | |
| const state = await readLocalFirebaseAuthState() | |
| polling = false | |
| if (stopped) return | |
| const serialized = JSON.stringify(state) | |
| if (serialized === lastState) return | |
| lastState = serialized | |
| report(state) | |
| } | |
| report({ status: 'pending' }) | |
| lastState = JSON.stringify({ status: 'pending' }) | |
| void poll() | |
| const interval = setInterval(() => void poll(), POLL_INTERVAL_MS) | |
| const poll = async (): Promise<void> => { | |
| if (stopped || polling) return | |
| polling = true | |
| let state: ComfyDesktop2FirebaseAuthState | |
| try { | |
| state = await readLocalFirebaseAuthState() | |
| } finally { | |
| polling = false | |
| } | |
| if (stopped) return | |
| const serialized = JSON.stringify(state) | |
| if (serialized === lastState) return | |
| lastState = serialized | |
| report(state) | |
| } | |
| report({ status: 'pending' }) | |
| lastState = JSON.stringify({ status: 'pending' }) | |
| void poll() | |
| const interval = setInterval(() => void poll(), POLL_INTERVAL_MS) |
🤖 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/preload/localFirebaseAuthMonitor.ts` around lines 78 - 92, Update the
poll function’s await flow around readLocalFirebaseAuthState so polling is
always reset in a finally block, including when the read rejects. Preserve the
existing stopped check, state serialization, and report behavior after a
successful read.
This was referenced Jul 24, 2026
Member
|
Since I made this unreviewable (sorry Deep) I opened a stack of PRs that keeps changes somewhat more reviewable. I would consider this PR superseded. |
Contributor
|
Superseded — the login-code flow landed via #1303 (v1.0.31), with the identity/consensus half in #1306 and follow-ups in #1353, #1356 and #1359. Per the author's own note above ("I would consider this PR superseded"), this can be closed so the remaining auth work is just the open drafts #1357 and #1358. |
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.
5 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.
Summary
This is now the single Desktop PR for GTM-93. It restores the reverted login-code implementation from #1222, keeps the useful hardening from the original #1249, and incorporates the complete Desktop identity-stitching work from #1272.
After this lands, the intended journey is one PostHog person:
comfy.org anonymous visit -> Desktop download/install/open -> Firebase sign-inWhat is included
Telemetry.reportFirebaseAuthStatepreload bridge and declarative signed-in/signed-out consensus across live Cloud views; active hosted views bind only after restored renderer consensus, while local/legacy views retain a main-verified fallback.installation_idas a stable property rather than a PostHog distinct ID.main, including feat: open gated model access pages in the Comfy session #1275'sopenModelAccessPagepreload bridge.Accepted boundaries
The Cloud contract is S256-only, so this PR verifies the verifier/challenge wiring but does not send an unmodeled
code_challenge_methodfield. It also does not add number matching or manual code confirmation: the short-lived code is auto-redeemed by the signed-in browser session.The installer carries the raw website PostHog ID. Copying the same carrier-named installer can therefore replay that analytics identity across fresh installations. This limitation is explicitly accepted for this rollout; replacing it with a single-use router redemption token is optional follow-up work, not a blocker for this PR.
Rollout
The frontend auth-state bridge and router redemption path are prerequisites for the RC integration test. Once this PR is reviewed and merged, cut a Desktop RC and validate the full packaged flow before stable promotion. This PR supersedes #1272.
Validation
pnpm run typecheckpnpm run lintpnpm run buildpnpm run test— 201 files passed; 3,046 tests passed, 1 skippedCloses GTM-93 after packaged RC and PostHog verification, not on merge alone.