Revert "fix(desktop): queue early macOS deep links to prevent auth redirect loss"#1440
Closed
Kitenite wants to merge 1 commit into
Closed
Revert "fix(desktop): queue early macOS deep links to prevent auth redirect loss"#1440Kitenite wants to merge 1 commit into
Kitenite wants to merge 1 commit into
Conversation
…direct l…" This reverts commit 4753178.
Collaborator
Author
|
Doesn't fix initial issue |
Contributor
📝 WalkthroughWalkthroughThe PR modifies the Electron main process to enhance deep-linking with protocol registration and auth/non-auth handling, add SQLite-based quit-confirmation preferences, introduce termination signal handling for development, register an icon protocol, and reorganize startup initialization order to ensure proper sequencing of dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Electron App
participant Event as open-url Event
participant Auth as Auth Handler
participant Renderer as Renderer Process
participant Window as Main Window
Event->>App: Receive deep-link URL
App->>App: processDeepLink(url)
alt Auth Deep Link
App->>Auth: Handle auth callback
Auth->>App: Return result
App->>Window: focusMainWindow()
Window->>Renderer: Focus & activate
else Non-Auth Deep Link
App->>Renderer: Send deep-link-navigate IPC
Renderer->>Renderer: Navigate to URL
App->>Window: focusMainWindow()
end
sequenceDiagram
participant User as User
participant App as Electron App
participant DB as SQLite DB
participant Dialog as Dialog
participant Cleanup as Cleanup Handler
User->>App: Initiate quit
App->>DB: getConfirmOnQuitSetting()
alt Confirmation Enabled
App->>Dialog: Show quit confirmation
Dialog->>User: User response
alt User Cancels
Dialog-->>App: Cancel signal
App->>App: Prevent quit
else User Confirms
Dialog-->>App: Proceed signal
App->>Cleanup: Cleanup & exit
end
else Confirmation Disabled
App->>Cleanup: Cleanup & exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/index.ts`:
- Around line 95-99: The macOS open-url handler (app.on("open-url", ...) calling
processDeepLink) can fire before initialization (initAppState) and before
windows exist, causing cold-start deep links to be lost; modify the handler to
detect when the app is not ready or initAppState hasn't run and stash the
incoming URL (e.g., in a module-level pendingDeepLink variable or queue) and
then process it after startup (reuse existing processDeepLink call in the
post-init path that runs findDeepLinkInArgv), ensuring handleAuthCallback can
access initialized state and BrowserWindow.getAllWindows() will find windows; if
this is intentionally deferred, add a TODO comment here referencing the known
gap and link to the follow-up issue.
🧹 Nitpick comments (1)
apps/desktop/src/main/index.ts (1)
57-70: Variablepathshadows thenode:pathimport; deep-link navigation uses direct IPC instead of tRPC.Two issues in this segment:
Line 60:
const pathshadows the top-levelimport path from "node:path"(line 1). No functional bug today, but it will bite ifnode:pathis ever needed inside this function.Line 69:
webContents.send("deep-link-navigate", path)is raw Electron IPC. As per coding guidelines, Electron IPC should always go through tRPC (src/lib/trpc).Suggested fix for the shadowing
- const path = `/${url.split("://")[1]}`; + const deepLinkPath = `/${url.split("://")[1]}`; focusMainWindow(); // Navigate in renderer via loading the route directly const windows = BrowserWindow.getAllWindows(); if (windows.length > 0) { const mainWindow = windows[0]; // Send navigation request to renderer - mainWindow.webContents.send("deep-link-navigate", path); + mainWindow.webContents.send("deep-link-navigate", deepLinkPath); }
Comment on lines
+95
to
99
| // Handle deep links when app is already running (macOS) | ||
| app.on("open-url", async (event, url) => { | ||
| event.preventDefault(); | ||
| if (appReady) { | ||
| await processDeepLink(url); | ||
| } else { | ||
| pendingDeepLinkUrl = url; | ||
| } | ||
| await processDeepLink(url); | ||
| }); |
Contributor
There was a problem hiding this comment.
macOS cold-start deep links will be silently lost.
On macOS, open-url fires before app.whenReady(). At that point:
- Auth deep links:
handleAuthCallbackmay not have the required state (e.g.,initAppStateat line 266 hasn't run yet). - Non-auth deep links:
BrowserWindow.getAllWindows()returns[], so the navigation at line 66–70 is silently skipped.
Windows/Linux cold-start deep links are handled correctly via findDeepLinkInArgv at lines 285–289 (post-init), but macOS has no equivalent deferred path after this revert.
If the revert is intentional and temporary, consider adding a // TODO here documenting the known gap and linking to the follow-up issue.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/index.ts` around lines 95 - 99, The macOS open-url
handler (app.on("open-url", ...) calling processDeepLink) can fire before
initialization (initAppState) and before windows exist, causing cold-start deep
links to be lost; modify the handler to detect when the app is not ready or
initAppState hasn't run and stash the incoming URL (e.g., in a module-level
pendingDeepLink variable or queue) and then process it after startup (reuse
existing processDeepLink call in the post-init path that runs
findDeepLinkInArgv), ensuring handleAuthCallback can access initialized state
and BrowserWindow.getAllWindows() will find windows; if this is intentionally
deferred, add a TODO comment here referencing the known gap and link to the
follow-up issue.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Reverts #1436
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores