fix(desktop): use deep links for auth on macOS, localhost callback on Linux#1550
Conversation
… Linux PR #1477 unconditionally sent local_callback for all platforms, which bypassed the reliable deep link flow on macOS. Only send the localhost callback on Linux where deep links are unreliable, restoring the original macOS auth behavior.
📝 WalkthroughWalkthroughModified OAuth sign-in flow in the auth router to conditionally include platform-specific callback URL parameters. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/auth/index.ts (2)
92-98: Consider using!PLATFORM.IS_MACinstead ofPLATFORM.IS_LINUXfor future-proofing.Currently, Windows (and any other platform) will also skip
local_callbackand attempt deep links. If deep links are only reliable on macOS, a safer guard would be!PLATFORM.IS_MAC, so any non-macOS platform falls back to the localhost callback automatically.Suggested diff
- // Only send local_callback on Linux where deep links are unreliable - if (PLATFORM.IS_LINUX) { + // Send local_callback on non-macOS platforms where deep links may be unreliable + if (!PLATFORM.IS_MAC) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/auth/index.ts` around lines 92 - 98, Replace the platform check that conditions adding the local_callback (currently using PLATFORM.IS_LINUX) with a non-macOS check so all non-mac platforms use the localhost fallback; update the if in the auth router around connectUrl.searchParams.set("local_callback", `http://127.0.0.1:${sharedEnv.DESKTOP_NOTIFICATIONS_PORT}/auth/callback`) to use !PLATFORM.IS_MAC instead of PLATFORM.IS_LINUX, keeping connectUrl, "local_callback" and sharedEnv.DESKTOP_NOTIFICATIONS_PORT intact.
68-72: Consider documenting Windows behavior.The JSDoc mentions macOS (deep link) and Linux (localhost callback), but
PLATFORMalso definesIS_WINDOWS. If the desktop app ships on Windows, deep links may also be unreliable there. Worth a brief note on the intended Windows behavior—or an explicitIS_MACguard instead ofIS_LINUXif macOS is the only platform where deep links are known to work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/auth/index.ts` around lines 68 - 72, The JSDoc currently mentions macOS (deep link) and Linux (localhost callback) but omits Windows; update the comment around the start OAuth sign-in flow to either (a) explicitly document the intended Windows behavior (e.g., treat Windows like Linux and use localhost callback if deep links are unreliable) or (b) change the wording to use an explicit IS_MAC guard language instead of IS_LINUX so it’s clear deep links are only assumed to work on macOS; reference the PLATFORM / IS_WINDOWS / IS_MAC symbols used in the auth router to ensure the doc matches the implemented platform branching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/auth/index.ts`:
- Around line 92-98: Replace the platform check that conditions adding the
local_callback (currently using PLATFORM.IS_LINUX) with a non-macOS check so all
non-mac platforms use the localhost fallback; update the if in the auth router
around connectUrl.searchParams.set("local_callback",
`http://127.0.0.1:${sharedEnv.DESKTOP_NOTIFICATIONS_PORT}/auth/callback`) to use
!PLATFORM.IS_MAC instead of PLATFORM.IS_LINUX, keeping connectUrl,
"local_callback" and sharedEnv.DESKTOP_NOTIFICATIONS_PORT intact.
- Around line 68-72: The JSDoc currently mentions macOS (deep link) and Linux
(localhost callback) but omits Windows; update the comment around the start
OAuth sign-in flow to either (a) explicitly document the intended Windows
behavior (e.g., treat Windows like Linux and use localhost callback if deep
links are unreliable) or (b) change the wording to use an explicit IS_MAC guard
language instead of IS_LINUX so it’s clear deep links are only assumed to work
on macOS; reference the PLATFORM / IS_WINDOWS / IS_MAC symbols used in the auth
router to ensure the doc matches the implemented platform branching.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
local_callbackon all platforms — bypassing the reliable deep link flow on macOSsuperset://auth/callback), Linux uses the localhost port callback (http://127.0.0.1:{port}/auth/callback)Changes
apps/desktop/src/lib/trpc/routers/auth/index.ts:PLATFORMfromshared/constantslocal_callbacksearch param inif (PLATFORM.IS_LINUX)so it's only sent on LinuxsignInto reflect platform-conditional behaviorTest Plan
superset://auth/callbackdeep link (no localhost navigation)http://127.0.0.1:{port}/auth/callback(localhost with HTML success page)Summary by CodeRabbit