fix(auth): offer "clear app data" recovery when login decryption fails#1652
Merged
senamakel merged 7 commits intoMay 13, 2026
Merged
Conversation
When the core sidecar fails to decrypt locally stored secrets during sign-in (key rotated, profile copied across machines, tampered storage), the deep-link auth flow used to surface a generic "Sign-in failed. Please try again." with no path forward. The user had to know about the hidden Settings → Danger Zone option, which is unreachable while logged out. Detect the decryption error in the deep-link handler and expose a "Clear app data & restart" button directly in the Welcome screen's error block. Extracts the existing Settings clear-app-data logic into a shared util so both call sites stay in sync.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDetect decryption/key-mismatch failures during deep-link OAuth auth, set a new requiresAppDataReset flag on failure, add a centralized clearAllAppData utility, and surface a destructive “Clear app data & restart” UI in Welcome/Settings wired to that utility. ChangesApp data reset on decryption failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (3)
app/src/store/deepLinkAuthState.ts (1)
51-54: ⚡ Quick winExtract
failDeepLinkAuthProcessingoptions into a named interface.Line 53 defines an inline object shape; prefer a dedicated interface for this options contract.
Suggested refactor
+interface FailDeepLinkAuthProcessingOptions { + requiresAppDataReset?: boolean; +} + export const failDeepLinkAuthProcessing = ( message: string, - options: { requiresAppDataReset?: boolean } = {} + options: FailDeepLinkAuthProcessingOptions = {} ): void => {As per coding guidelines "Prefer interface for defining object shapes in TypeScript".
🤖 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 `@app/src/store/deepLinkAuthState.ts` around lines 51 - 54, Extract the inline options object into a named interface: create an interface (e.g., DeepLinkAuthFailOptions) that declares requiresAppDataReset?: boolean, update the function signature of failDeepLinkAuthProcessing to use that interface (options: DeepLinkAuthFailOptions = {}), and update any related types/usages to reference DeepLinkAuthFailOptions so the options shape is defined via an interface rather than an inline type.app/src/utils/clearAllAppData.ts (1)
42-43: ⚡ Quick winUse namespaced debug logging instead of
console.warnin this reset path.Line 42 and Line 52 use raw console logging. Please switch to a namespaced
debuglogger (dev-focused detail) for consistency with app logging policy.As per coding guidelines "Use namespaced debug logs in TypeScript (e.g., via the debug npm package with namespace per area) plus dev-only detail where useful in app/src".
Also applies to: 52-53
🤖 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 `@app/src/utils/clearAllAppData.ts` around lines 42 - 43, Replace the raw console.warn calls in the clearAllAppData reset path with a namespaced debug logger: import and instantiate the debug logger for this module (e.g., namespace like "app:clearAllAppData") and use it instead of console.warn in the error handling in the clearAllAppData function (the two occurrences currently logging "[clearAllAppData] Failed to queue CEF profile purge:" and the other at lines ~52-53). Ensure the debug calls include the error object and preserve the existing message text so devs can enable the specific "app:clearAllAppData" namespace for detailed output.app/src/pages/__tests__/Welcome.test.tsx (1)
93-97: ⚡ Quick winExtract a shared deep-link auth mock helper to avoid repeated state literals.
The new field had to be updated in many places; centralizing this mock setup will reduce future drift and test maintenance cost.
♻️ Proposed refactor
+const defaultDeepLinkAuthState = { + isProcessing: false, + errorMessage: null, + requiresAppDataReset: false, +}; + +const mockDeepLinkAuthState = ( + overrides: Partial<typeof defaultDeepLinkAuthState> = {}, +) => { + vi.mocked(useDeepLinkAuthState).mockReturnValue({ + ...defaultDeepLinkAuthState, + ...overrides, + }); +}; beforeEach(() => { oauthButtonSpy.mockReset(); oauthOverrideSpy.mockReset(); - vi.mocked(useDeepLinkAuthState).mockReturnValue({ - isProcessing: false, - errorMessage: null, - requiresAppDataReset: false, - }); + mockDeepLinkAuthState(); vi.mocked(clearCoreRpcUrlCache).mockReset(); vi.mocked(clearBackendUrlCache).mockReset(); vi.mocked(storeRpcUrl).mockReset(); });🤖 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 `@app/src/pages/__tests__/Welcome.test.tsx` around lines 93 - 97, Several tests in Welcome.test.tsx duplicate the same deep-link auth state literal; extract a single helper (e.g., createDeepLinkAuthMock or buildDeepLinkAuthState) that returns the canonical mock object and replace inline literals in tests with calls to that helper; update imports/uses in any describe/it blocks and any helpers that assert against the deep-link auth shape (e.g., where deepLinkAuth, mockDeepLinkState, or similar identifiers are referenced) so future field additions only require updating the helper.
🤖 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 `@app/src/pages/Welcome.tsx`:
- Line 38: Replace the raw console.error call in the Welcome component
(Welcome.tsx) with the app’s namespaced debug logger (e.g., create/get a logger
for "app:welcome") and log only sanitized error details — for example
err.message and err.code or a trimmedMessage — rather than the full err object;
ensure the call is gated by the debug/logger’s enabled/dev-only checks so
PII/secrets are not emitted in production and remove any usage of the raw err
object from the log call.
---
Nitpick comments:
In `@app/src/pages/__tests__/Welcome.test.tsx`:
- Around line 93-97: Several tests in Welcome.test.tsx duplicate the same
deep-link auth state literal; extract a single helper (e.g.,
createDeepLinkAuthMock or buildDeepLinkAuthState) that returns the canonical
mock object and replace inline literals in tests with calls to that helper;
update imports/uses in any describe/it blocks and any helpers that assert
against the deep-link auth shape (e.g., where deepLinkAuth, mockDeepLinkState,
or similar identifiers are referenced) so future field additions only require
updating the helper.
In `@app/src/store/deepLinkAuthState.ts`:
- Around line 51-54: Extract the inline options object into a named interface:
create an interface (e.g., DeepLinkAuthFailOptions) that declares
requiresAppDataReset?: boolean, update the function signature of
failDeepLinkAuthProcessing to use that interface (options:
DeepLinkAuthFailOptions = {}), and update any related types/usages to reference
DeepLinkAuthFailOptions so the options shape is defined via an interface rather
than an inline type.
In `@app/src/utils/clearAllAppData.ts`:
- Around line 42-43: Replace the raw console.warn calls in the clearAllAppData
reset path with a namespaced debug logger: import and instantiate the debug
logger for this module (e.g., namespace like "app:clearAllAppData") and use it
instead of console.warn in the error handling in the clearAllAppData function
(the two occurrences currently logging "[clearAllAppData] Failed to queue CEF
profile purge:" and the other at lines ~52-53). Ensure the debug calls include
the error object and preserve the existing message text so devs can enable the
specific "app:clearAllAppData" namespace for detailed output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75f2eabd-0330-479c-8c27-039bea1e32bd
📒 Files selected for processing (9)
app/src/components/oauth/__tests__/OAuthProviderButton.test.tsxapp/src/components/settings/SettingsHome.tsxapp/src/pages/Welcome.tsxapp/src/pages/__tests__/Welcome.test.tsxapp/src/store/__tests__/deepLinkAuthState.test.tsapp/src/store/deepLinkAuthState.tsapp/src/utils/__tests__/desktopDeepLinkListener.test.tsapp/src/utils/clearAllAppData.tsapp/src/utils/desktopDeepLinkListener.ts
…ilure
Per CodeRabbit + frontend logging guideline: replace raw `console.error`
with `debug('app:welcome')` and log a sanitized error message rather
than the full error object.
…l path Adds focused unit tests for the diff-coverage gate: - clearAllAppData: full sequence, best-effort skips on CEF/clearSession failure, abort on workspace reset failure, default `userId=null`. - Welcome: renders/hides the recovery button via `requiresAppDataReset`, invokes clearAllAppData on click, surfaces inline error on failure. - desktopDeepLinkListener: decryption-error branch flips the new flag and rewrites the message; non-decryption failures stay generic. - SettingsHome: confirms the Danger Zone flow forwards `clearSession` + current `userId` to the shared util.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/components/settings/__tests__/SettingsHome.test.tsx (1)
265-267: ⚡ Quick winAvoid DOM-order coupling when selecting the confirm button.
Using “last button wins” is brittle. Scope the lookup to the modal/dialog so this stays stable if button order changes.
Suggested refactor
+import { render, screen, within } from '@testing-library/react'; ... -const confirmButtons = screen.getAllByRole('button', { name: /Clear App Data/i }); -// The last one is the modal confirm button (first is the menu item we just clicked). -await user.click(confirmButtons[confirmButtons.length - 1]); +const dialog = await screen.findByRole('dialog'); +await user.click(within(dialog).getByRole('button', { name: /Clear App Data/i }));🤖 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 `@app/src/components/settings/__tests__/SettingsHome.test.tsx` around lines 265 - 267, The test currently finds all buttons named /Clear App Data/i and clicks the last one (confirmButtons[confirmButtons.length - 1]), which couples it to DOM order; instead locate the modal/dialog element after opening the menu (e.g., using screen.getByRole('dialog') or similar) and scope the query with within(dialog).getByRole('button', { name: /Clear App Data/i }) so you click the confirm button inside the modal — update the code in SettingsHome.test.tsx to replace the global getAllByRole lookup and indexed click with a scoped query and click using user.click on the modal-scoped button.
🤖 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 `@app/src/components/settings/__tests__/SettingsHome.test.tsx`:
- Around line 43-46: The top-level mockClearAllAppData is referenced inside the
vi.mock factory and must be hoisted to avoid TDZ; replace its declaration with a
hoisted wrapper using vi.hoisted to return the mock (keep the same mock behavior
of resolved undefined) and continue to reference mockClearAllAppData in the
vi.mock('../../../utils/clearAllAppData', ...) factory so the mock factory runs
after the hoisted initializer; target the mockClearAllAppData symbol and the
vi.mock call in SettingsHome.test.tsx when applying the change.
In `@app/src/utils/__tests__/desktopDeepLinkListener.test.ts`:
- Line 84: Replace the flaky setTimeout-based waits in the test with explicit
waiting on the deepLinkAuthState subscription: remove the await new
Promise(resolve => setTimeout(resolve, 0)) calls and instead create a Promise
that subscribes via subscribeDeepLinkAuthState, resolves when deepLinkAuthState
reaches the expected value, then unsubscribes; await that Promise in the test
before asserting. Target the test logic that triggers state changes and use the
subscription-based wait for determinism (use deepLinkAuthState and
subscribeDeepLinkAuthState identifiers).
---
Nitpick comments:
In `@app/src/components/settings/__tests__/SettingsHome.test.tsx`:
- Around line 265-267: The test currently finds all buttons named /Clear App
Data/i and clicks the last one (confirmButtons[confirmButtons.length - 1]),
which couples it to DOM order; instead locate the modal/dialog element after
opening the menu (e.g., using screen.getByRole('dialog') or similar) and scope
the query with within(dialog).getByRole('button', { name: /Clear App Data/i })
so you click the confirm button inside the modal — update the code in
SettingsHome.test.tsx to replace the global getAllByRole lookup and indexed
click with a scoped query and click using user.click on the modal-scoped button.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5631e768-33a5-4a29-b0dd-f032d6f70df6
📒 Files selected for processing (4)
app/src/components/settings/__tests__/SettingsHome.test.tsxapp/src/pages/__tests__/Welcome.test.tsxapp/src/utils/__tests__/clearAllAppData.test.tsapp/src/utils/__tests__/desktopDeepLinkListener.test.ts
- Wrap `mockClearAllAppData` in `vi.hoisted(...)` in Welcome and SettingsHome tests so the symbol is initialized before `vi.mock`'s hoisted factory runs (avoids TDZ ReferenceError under Vitest v4). - Replace `setTimeout(resolve, 0)` waits in the deep-link listener tests with a subscription-driven `waitForAuthSettled` helper that resolves when `deepLinkAuthState.isProcessing` flips false. Deterministic and doesn't rely on scheduler variance.
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.
Summary
clearAllAppData()util used by both call sites.requiresAppDataResetflag throughDeepLinkAuthStateso the Welcome screen only shows the destructive action when it's actually applicable.Problem
When the local core sidecar can't decrypt secrets at sign-in (e.g. encryption key rotated, profile copied between machines, tampered/corrupt storage), the deep-link auth handler used to swallow the underlying
Decryption failed — wrong key or tampered dataerror and surface a generic "Sign-in failed. Please try again." with no path forward. The Settings → "Clear App Data" escape hatch is unreachable while logged out, so users were effectively stuck on the Welcome screen.Solution
desktopDeepLinkListener.tsinspects the caught error fromhandleAuthDeepLinkfordecryption failed/wrong key or tampered data/corrupt datasubstrings. On match, it flips the newrequiresAppDataResetflag onDeepLinkAuthStateand shows a more useful message ("Clear app data to start fresh" + reassurance the cloud account is untouched).Welcome.tsxreads the flag and renders a red "Clear app data & restart" button inside the existing error block, with an in-flight spinner and inline failure surface.app/src/utils/clearAllAppData.ts(CEF profile purge → optional coreclearSession→ workspace reset/core restart → redux-persist + window storage wipe → app restart). Welcome calls it withoutclearSession(no live session pre-login);SettingsHome.tsxcalls it with the current user's session clear callback. Both call sites stay in sync.deepLinkAuthState, the existing snapshot tests, and the listener's error-classification path.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.Impact
clearAllAppDataare Tauri-specific; web/CLI surfaces are untouched.requiresAppDataResetfield defaults tofalseand is recomputed on every transition.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
Validation Blocked
Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests