fix: address Codex review findings for sync and deep link - #178
Conversation
1. Preserve recovery-wrapped CEK on passphrase change — only overwrite wrappedCekRecovery in the upsert when explicitly provided, so passphrase changes don't silently erase recovery data. 2. Parse deep-link URL from process.argv on cold start (Windows/Linux) — when the app isn't running and the user clicks a readied:// link, the token is now captured from startup args and queued as pendingAuthToken for delivery once the window loads. 3. Guard syncNow() with encryption readiness check — if CEK is not loaded (fresh install, no passphrase set), sync returns a clear error instead of crashing with 'Encryption service not initialized'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThree targeted enhancements across the desktop and API layers: the main process now parses deep link arguments during cold start for authentication token extraction, the sync service validates encryption readiness before initiating sync cycles, and the API conditionally updates wrapped CEK recovery data only when explicitly provided rather than overwriting with null. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec5afd4a47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Another instance already has the lock — quit this one. | ||
| // The deep link URL was passed to the existing instance via second-instance event. | ||
| app.quit(); | ||
| } else { | ||
| // Check startup args for deep link URL (cold start on Windows/Linux). |
There was a problem hiding this comment.
The new startup deep-link parsing and second-instance listener are now nested under if (!gotTheLock), which is the short-lived secondary process that immediately calls app.quit(). This means the primary instance (gotTheLock === true) no longer subscribes to second-instance, so links opened while the app is already running are not handled, and cold-start argv handling also runs in the wrong process. This should stay in the primary-instance path (the else branch) so the surviving process receives and processes deep links.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes 3 issues flagged by Codex bot review on PR #177.
Changes
1. Preserve recovery key on passphrase change
The
POST /sync/keysupsert was overwritingwrappedCekRecoverywithnullwhen not provided (e.g., during passphrase change). Now only overwrites if explicitly sent.2. Parse deep link on cold start (Windows/Linux)
When the app isn't running and user clicks a
readied://magic link, the OS launches a new instance with the URL inprocess.argv. Previously onlysecond-instanceparsed this. Now startup args are checked too.3. Guard sync with encryption readiness
syncNow()now checksencryptionService.isReady()before attempting encrypt/decrypt. Fresh installs without passphrase setup get a clear error instead of crashing.Test plan
pnpm typecheckpasses (17/17)pnpm testpasses (42/42)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes