GTM-93: add desktop login attribution callback#12983
Closed
benceruleanlu wants to merge 7 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds desktop login request handling, PostHog identity queuing, route guard changes, and setup-time completion hooks in cloud login and signup views. ChangesDesktop Login Bridge
Possibly related PRs
Suggested reviewers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: ✅ 1681 passed, 0 failed · 1 flaky📊 Browser Reports
📦 Bundle: 7.5 MB gzip 🔴 +2.5 kBDetailsSummary
Category Glance App Entry Points — 46 kB (baseline 46.7 kB) • 🟢 -715 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.25 MB (baseline 1.25 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 95.7 kB (baseline 95.2 kB) • 🔴 +568 BTop-level views, pages, and routed surfaces
Status: 12 added / 12 removed Panels & Settings — 526 kB (baseline 526 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 25 added / 25 removed / 1 unchanged User & Accounts — 25.1 kB (baseline 19.9 kB) • 🔴 +5.22 kBAuthentication, profile, and account management bundles
Status: 10 added / 9 removed Editors & Dialogs — 112 kB (baseline 112 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 5 added / 5 removed UI Components — 57.2 kB (baseline 57.2 kB) • ⚪ 0 BReusable component library chunks
Status: 13 added / 13 removed Data & Services — 271 kB (baseline 271 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 16 added / 16 removed Utilities & Hooks — 3.33 MB (baseline 3.33 MB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 31 added / 31 removed Vendor & Third-Party — 15.3 MB (baseline 15.3 MB) • 🔴 +1.06 kBExternal libraries and shared vendor chunks
Status: 13 added / 12 removed / 4 unchanged Other — 10.6 MB (baseline 10.6 MB) • 🔴 +226 BBundles that do not match a named category
Status: 150 added / 150 removed / 3 unchanged ⚡ Performance
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/platform/cloud/onboarding/desktopLoginBridge.ts (2)
16-29: 💤 Low valueConsider IPv6 loopback support.
The current loopback validation only accepts
localhostand127.0.0.1. If desktop applications bind to IPv6, they might use::1(IPv6 loopback). Consider adding it to the allowlist or documenting why IPv6 is intentionally excluded.Optional: Add IPv6 loopback support
function parseLoopbackCallback(rawUrl: string): URL | null { let url: URL try { url = new URL(rawUrl) } catch { return null } if (url.protocol !== 'http:') return null - if (!['localhost', '127.0.0.1'].includes(url.hostname)) return null + if (!['localhost', '127.0.0.1', '::1'].includes(url.hostname)) return null if (url.pathname !== '/callback') return null return url }🤖 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/platform/cloud/onboarding/desktopLoginBridge.ts` around lines 16 - 29, The parseLoopbackCallback function currently only validates IPv4 loopback addresses (localhost and 127.0.0.1) but does not account for IPv6 loopback addresses. Update the hostname validation check in parseLoopbackCallback where the includes method checks for valid hostnames to also include the IPv6 loopback address '::1' in addition to the existing IPv4 addresses.
65-84: ⚡ Quick winConsider adding a fetch timeout.
The
fetchcall to the desktop loopback URL has no timeout. If the desktop application crashes or becomes unresponsive, the POST could hang indefinitely, blocking the browser login flow.Add AbortSignal with timeout
+ const abortController = new AbortController() + const timeoutId = setTimeout(() => abortController.abort(), 10000) // 10s + const response = await fetch(request.callbackUrl.href, { method: 'POST', mode: 'cors', credentials: 'omit', + signal: abortController.signal, headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ state: request.state, apiKey: firebaseConfig.apiKey, user: user.toJSON() }) }) + clearTimeout(timeoutId)🤖 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/platform/cloud/onboarding/desktopLoginBridge.ts` around lines 65 - 84, The fetch call to request.callbackUrl.href lacks timeout protection, which can cause the browser login flow to hang indefinitely if the desktop application becomes unresponsive. Add an AbortController with a timeout mechanism to the fetch request. Create the AbortController, set a timeout that aborts the request after a reasonable duration (e.g., 30 seconds), and pass the abort signal in the fetch options alongside the existing method, mode, credentials, headers, and body properties. This ensures the POST request will be cancelled if the desktop application does not respond within the specified timeframe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/platform/cloud/onboarding/desktopLoginBridge.ts`:
- Around line 16-29: The parseLoopbackCallback function currently only validates
IPv4 loopback addresses (localhost and 127.0.0.1) but does not account for IPv6
loopback addresses. Update the hostname validation check in
parseLoopbackCallback where the includes method checks for valid hostnames to
also include the IPv6 loopback address '::1' in addition to the existing IPv4
addresses.
- Around line 65-84: The fetch call to request.callbackUrl.href lacks timeout
protection, which can cause the browser login flow to hang indefinitely if the
desktop application becomes unresponsive. Add an AbortController with a timeout
mechanism to the fetch request. Create the AbortController, set a timeout that
aborts the request after a reasonable duration (e.g., 30 seconds), and pass the
abort signal in the fetch options alongside the existing method, mode,
credentials, headers, and body properties. This ensures the POST request will be
cancelled if the desktop application does not respond within the specified
timeframe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 457bdadc-b145-492b-b535-7965628ef251
📒 Files selected for processing (8)
src/platform/cloud/onboarding/CloudLoginView.vuesrc/platform/cloud/onboarding/composables/usePostAuthRedirect.tssrc/platform/cloud/onboarding/desktopLoginBridge.test.tssrc/platform/cloud/onboarding/desktopLoginBridge.tssrc/platform/cloud/onboarding/onboardingCloudRoutes.tssrc/platform/telemetry/providers/cloud/PostHogTelemetryProvider.tssrc/platform/telemetry/providers/cloud/posthogIdentity.test.tssrc/platform/telemetry/providers/cloud/posthogIdentity.ts
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #12983 +/- ##
==========================================
- Coverage 76.37% 75.88% -0.50%
==========================================
Files 1569 1591 +22
Lines 102782 105739 +2957
Branches 31977 32425 +448
==========================================
+ Hits 78499 80237 +1738
- Misses 23452 24645 +1193
- Partials 831 857 +26
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 112 files with indirect coverage changes 🚀 New features to boost your workflow:
|
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
can use local storage and have a generalized client anon id. need to send that in identify calls i believe
benceruleanlu
marked this pull request as ready for review
June 23, 2026 01:13
dosubot
Bot
added
the
size:L
Contributor
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/platform/cloud/onboarding/CloudLoginView.vue`:
- Line 125: The call to completeDesktopLoginForExistingSession on line 125 is
intentionally unawaited but can reject through its onAuthSuccess callback,
creating a potential unhandled promise rejection. Add a terminal .catch()
handler to the completeDesktopLoginForExistingSession(...) call to catch and
handle any unexpected failures, ensuring the promise chain is properly
terminated and compliant with the floating promises restriction in the coding
guidelines.
In `@src/platform/cloud/onboarding/CloudSignupView.vue`:
- Around line 166-167: The call to
completeDesktopLoginForExistingSession(route.query, onAuthSuccess) on line 166
is a fire-and-forget promise that lacks error handling, which can result in
unhandled promise rejections. Attach a .catch() handler to this promise chain to
gracefully handle any errors that may occur from the onAuthSuccess() callback,
ensuring failures remain user-visible and bounded rather than escaping as
unhandled rejections.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 97d0c744-2cf2-4f04-b83b-495a1783e99d
📒 Files selected for processing (4)
src/platform/cloud/onboarding/CloudLoginView.vuesrc/platform/cloud/onboarding/CloudSignupView.vuesrc/platform/cloud/onboarding/composables/useDesktopLoginCompletion.test.tssrc/platform/cloud/onboarding/composables/useDesktopLoginCompletion.ts
✅ Files skipped from review due to trivial changes (1)
- src/platform/cloud/onboarding/composables/useDesktopLoginCompletion.test.ts
Contributor
|
@christian-byrne Could you check the PR now vs your comment earlier for if you need anything here changing (I think we can include the anon id in a separate PR? Or is it required for something here) |
christian-byrne
approved these changes
Jun 25, 2026
Contributor
|
Did not mean to approve, was on wrong tab |
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.
4 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
Links
Test plan