fix: route Desktop loopback links and modifier-clicks to system browser - #187
Conversation
initExternalLinks now returns a cleanup function. On Tauri Desktop, loopback (localhost preview) links and Command/Ctrl-clicks are routed through the system browser since the webview cannot open new tabs; Browser Web keeps native modifier-click and loopback navigation behavior. Add unit tests covering both environments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesExternal link handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant initExternalLinks
participant TauriOpenUrl
participant SystemBrowser
User->>initExternalLinks: Click external or loopback link
initExternalLinks->>TauriOpenUrl: Open eligible desktop link
TauriOpenUrl->>SystemBrowser: Open URL
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
web/src/lib/useDesktop.test.ts (1)
39-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest listener removal after
cleanup().Line 53 calls
cleanup(), but the test does not verify that the delegated listener was removed. The test can pass if a stale listener remains registered. Add a second click after cleanup. Assert that the router does not prevent the event or callmocks.openUrl.🤖 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 `@web/src/lib/useDesktop.test.ts` around lines 39 - 53, Extend the initExternalLinks test to dispatch a second click on the anchor after cleanup(). Assert that this post-cleanup event is not prevented and that mocks.openUrl is not called again, verifying the delegated listener was removed.
🤖 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 `@web/src/lib/useDesktop.test.ts`:
- Around line 39-53: Extend the initExternalLinks test to dispatch a second
click on the anchor after cleanup(). Assert that this post-cleanup event is not
prevented and that mocks.openUrl is not called again, verifying the delegated
listener was removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 48e33739-19c1-4e36-8887-7fd2a39aab51
📒 Files selected for processing (2)
web/src/lib/useDesktop.test.tsweb/src/lib/useDesktop.ts
![jcode-cloud-app[bot]](https://avatars.githubusercontent.com/in/4395162?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
No high-confidence findings
No findings met the configured confidence threshold.
Summary
Clean. initExternalLinks now returns a cleanup fn and routes Desktop loopback links + Cmd/Ctrl-clicks to the system browser. Browser Web behavior is provably unchanged (modifier/shift/alt still early-return; loopback still early-returns; external still calls openUrl). In Desktop the page origin is tauri://localhost while the sidecar API is http://127.0.0.1:<port>, so no http link is same-origin and in-app routing uses relative links (skipped by the http/https guard) — API traffic uses fetch/WebSocket, never anchor nav, so routing loopback clicks outward fixes the prior dead webview navigation without regressions. The sole caller (main.tsx) ignores the new return value, which is valid TS. No defect meets the 80% confidence bar.
🔍 Checks performed · 9
- Read web/src/lib/useDesktop.ts (initExternalLinks, isLoopback, isTauri, openUrl)
- Read web/src/lib/useDesktop.test.ts (Desktop + Browser Web cases)
- Read web/src/main.tsx: sole caller ignores the new cleanup return (valid)
- Read web/src/lib/apiBase.ts: Desktop sidecar = http://127.0.0.1:<port>
- Read web/src/App.tsx:303: Desktop page served from tauri://localhost (cross-origin to API)
- Grepped web/src for href={ and loopback references (SettingsView external URLs, ArtifactsPanel share URL)
- Inspected ArtifactsPanel.tsx: result.url is a backend/cloud share URL, not loopback
- Traced before/after logic per environment: Browser Web identical; Desktop adds loopback + modifier routing
- node_modules absent and pnpm unavailable in env; vitest not executed, relied on static analysis
jcode posts a non-blocking COMMENT review. Merge decisions remain with your team.
1 participant
What
initExternalLinksnow returns a cleanup function that removes the delegated click listener.Why
In the Desktop app, localhost preview links and modifier-clicks did nothing because Tauri's webview denies new-window creation, and the old listener skipped both modifier-clicks and loopback URLs entirely. API traffic uses fetch/WebSocket rather than anchor navigation, so routing loopback anchors externally is safe.
Testing
npx vitest run src/lib/useDesktop.test.ts— 5/5 passed🤖 Generated with Claude Code
Summary by CodeRabbit