fix(auth): scope clearAllAppData to active user; drop non-existent onboarding-complete endpoint call#1815
Closed
YellowSnnowmann wants to merge 3 commits into
Closed
Conversation
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 (4)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR simplifies onboarding completion by removing the backend endpoint call and implements user-scoped storage clearing to prevent data loss when switching accounts. Onboarding now persists only the local flag without server notification, while storage clearing now selectively removes per-user localStorage keys instead of wiping all accounts globally. ChangesAccount Switching Data Preservation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
… onboarding-complete call
clearAllAppData previously called window.localStorage.clear(), wiping
every account's persisted Redux state. Now only keys prefixed with the
active userId (e.g. `${userId}:persist:*`) are removed, leaving other
accounts' data intact on logout/data-clear. Falls back to a full clear
only when no userId is available (pre-login recovery path).
Also removes the userApi.onboardingComplete() call and the corresponding
POST /settings/onboarding-complete backend call from OnboardingLayout —
the endpoint never existed on the backend and was silently swallowed on
every onboarding completion.
Closes tinyhumansai#983
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
YellowSnnowmann
force-pushed
the
fix/account-switch-storage-scoping
branch
from
7d00e64 to
131017e
Compare
![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: 3
🧹 Nitpick comments (1)
app/src/components/composio/ComposioConnectModal.test.tsx (1)
288-305: 💤 Low valueTest validates semantically incorrect behavior for non-Atlassian toolkits.
This test uses
mockToolkit(Gmail) yet expects the Atlassian subdomain text to appear. If the component is updated to guard theneeds-subdomaintransition to Atlassian toolkits only (as suggested in the component review), this test would need to usejiraToolkitinstead, or be split into Jira-specific and generic-error paths.🤖 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/composio/ComposioConnectModal.test.tsx` around lines 288 - 305, The test in ComposioConnectModal.test.tsx asserts Atlassian-specific UI ("requires your Atlassian subdomain") while using mockToolkit (Gmail); update the test to use an Atlassian toolkit (e.g., jiraToolkit) or split into two tests: one that uses jiraToolkit to assert the needs-subdomain transition and Atlassian copy, and another that uses mockToolkit to assert the generic error/retry path; modify the test that references ComposioConnectModal, mockToolkit, and the authorize mock so the toolkit variable matches the expected Atlassian-specific behavior.
🤖 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/composio/ComposioConnectModal.tsx`:
- Around line 357-368: The current handler always calls
setPhase('needs-subdomain') when isMissingRequiredFieldsError(err) is true,
which causes Atlassian-specific UI to show for non-Atlassian toolkits; update
the logic to only transition to 'needs-subdomain' for Atlassian toolkits (e.g.,
check toolkit.slug or toolkit.name for known Atlassian identifiers such as
"jira" or "atlassian") and for other toolkits either (a) set a more generic
phase like 'missing-required-fields' or (b) setPhase to null and setError with a
generic missing-field message so the recovery UI uses toolkit-agnostic wording;
modify the condition around isMissingRequiredFieldsError(err) and the call sites
that rely on the 'needs-subdomain' phase (UI that reads toolkit.name) to handle
the new generic phase or error path accordingly.
In `@app/src/utils/__tests__/clearAllAppData.test.ts`:
- Around line 56-70: The test for clearAllAppData in clearAllAppData.test.ts
seeds window.sessionStorage but doesn't assert it was cleared; update the 'falls
back to localStorage.clear() when no userId is provided' test to also assert
that the seeded session key (e.g. 'session-persisted') is removed after calling
clearAllAppData(), by adding an expectation that
window.sessionStorage.getItem('session-persisted') is null; keep existing
assertions for mockPurgeCef, mockReset, mockRestart and localStorage checks
intact.
In `@app/src/utils/clearAllAppData.ts`:
- Around line 25-45: The current try block around the localStorage logic
prevents sessionStorage.clear() from running if any localStorage call throws;
update clearAllAppData (in app/src/utils/clearAllAppData.ts) to ensure
sessionStorage is always cleared by isolating localStorage operations into their
own try/catch (or by using a try/finally) so any error from the localStorage
loop (the prefix/key removal and localStorage.removeItem calls, and
localStorage.clear fallback) is handled without preventing
sessionStorage.clear() from executing; make sure to catch and log the
localStorage error (referencing variables like userId, prefix, and
ACTIVE_USER_KEY) and then still call sessionStorage.clear() in the outer flow.
---
Nitpick comments:
In `@app/src/components/composio/ComposioConnectModal.test.tsx`:
- Around line 288-305: The test in ComposioConnectModal.test.tsx asserts
Atlassian-specific UI ("requires your Atlassian subdomain") while using
mockToolkit (Gmail); update the test to use an Atlassian toolkit (e.g.,
jiraToolkit) or split into two tests: one that uses jiraToolkit to assert the
needs-subdomain transition and Atlassian copy, and another that uses mockToolkit
to assert the generic error/retry path; modify the test that references
ComposioConnectModal, mockToolkit, and the authorize mock so the toolkit
variable matches the expected Atlassian-specific behavior.
🪄 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: 1030c02f-47ea-4b8a-9227-d41970e0eaee
📒 Files selected for processing (6)
app/src/components/composio/ComposioConnectModal.test.tsxapp/src/components/composio/ComposioConnectModal.tsxapp/src/pages/onboarding/OnboardingLayout.tsxapp/src/services/api/userApi.tsapp/src/utils/__tests__/clearAllAppData.test.tsapp/src/utils/clearAllAppData.ts
💤 Files with no reviewable changes (2)
- app/src/services/api/userApi.ts
- app/src/pages/onboarding/OnboardingLayout.tsx
Comment on lines
+56
to
70
| it('falls back to localStorage.clear() when no userId is provided', async () => { | ||
| window.localStorage.setItem('user-1:persist:accounts', 'a'); | ||
| window.localStorage.setItem('user-2:persist:accounts', 'b'); | ||
| window.sessionStorage.setItem('session-persisted', '1'); | ||
|
|
||
| await clearAllAppData(); | ||
|
|
||
| expect(mockPurgeCef).toHaveBeenCalledWith(null); | ||
| // No clearSession was provided — call sequence still completes. | ||
| expect(mockReset).toHaveBeenCalledTimes(1); | ||
| expect(mockRestart).toHaveBeenCalledTimes(1); | ||
| // Without a userId we have no way to scope, so everything is cleared | ||
| expect(window.localStorage.getItem('user-1:persist:accounts')).toBeNull(); | ||
| expect(window.localStorage.getItem('user-2:persist:accounts')).toBeNull(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add sessionStorage assertion in the no-userId fallback test.
Line 59 seeds window.sessionStorage, but this test never verifies it was cleared. Add an assertion to keep the fallback path fully covered.
Suggested patch
it('falls back to localStorage.clear() when no userId is provided', async () => {
window.localStorage.setItem('user-1:persist:accounts', 'a');
window.localStorage.setItem('user-2:persist:accounts', 'b');
window.sessionStorage.setItem('session-persisted', '1');
await clearAllAppData();
expect(mockPurgeCef).toHaveBeenCalledWith(null);
// No clearSession was provided — call sequence still completes.
expect(mockReset).toHaveBeenCalledTimes(1);
expect(mockRestart).toHaveBeenCalledTimes(1);
// Without a userId we have no way to scope, so everything is cleared
expect(window.localStorage.getItem('user-1:persist:accounts')).toBeNull();
expect(window.localStorage.getItem('user-2:persist:accounts')).toBeNull();
+ expect(window.sessionStorage.getItem('session-persisted')).toBeNull();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('falls back to localStorage.clear() when no userId is provided', async () => { | |
| window.localStorage.setItem('user-1:persist:accounts', 'a'); | |
| window.localStorage.setItem('user-2:persist:accounts', 'b'); | |
| window.sessionStorage.setItem('session-persisted', '1'); | |
| await clearAllAppData(); | |
| expect(mockPurgeCef).toHaveBeenCalledWith(null); | |
| // No clearSession was provided — call sequence still completes. | |
| expect(mockReset).toHaveBeenCalledTimes(1); | |
| expect(mockRestart).toHaveBeenCalledTimes(1); | |
| // Without a userId we have no way to scope, so everything is cleared | |
| expect(window.localStorage.getItem('user-1:persist:accounts')).toBeNull(); | |
| expect(window.localStorage.getItem('user-2:persist:accounts')).toBeNull(); | |
| }); | |
| it('falls back to localStorage.clear() when no userId is provided', async () => { | |
| window.localStorage.setItem('user-1:persist:accounts', 'a'); | |
| window.localStorage.setItem('user-2:persist:accounts', 'b'); | |
| window.sessionStorage.setItem('session-persisted', '1'); | |
| await clearAllAppData(); | |
| expect(mockPurgeCef).toHaveBeenCalledWith(null); | |
| // No clearSession was provided — call sequence still completes. | |
| expect(mockReset).toHaveBeenCalledTimes(1); | |
| expect(mockRestart).toHaveBeenCalledTimes(1); | |
| // Without a userId we have no way to scope, so everything is cleared | |
| expect(window.localStorage.getItem('user-1:persist:accounts')).toBeNull(); | |
| expect(window.localStorage.getItem('user-2:persist:accounts')).toBeNull(); | |
| expect(window.sessionStorage.getItem('session-persisted')).toBeNull(); | |
| }); |
🤖 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/__tests__/clearAllAppData.test.ts` around lines 56 - 70, The
test for clearAllAppData in clearAllAppData.test.ts seeds window.sessionStorage
but doesn't assert it was cleared; update the 'falls back to
localStorage.clear() when no userId is provided' test to also assert that the
seeded session key (e.g. 'session-persisted') is removed after calling
clearAllAppData(), by adding an expectation that
window.sessionStorage.getItem('session-persisted') is null; keep existing
assertions for mockPurgeCef, mockReset, mockRestart and localStorage checks
intact.
Comment on lines
+25
to
+45
| try { | ||
| if (userId) { | ||
| const prefix = `${userId}:`; | ||
| const toRemove: string[] = []; | ||
| for (let i = 0; i < localStorage.length; i++) { | ||
| const key = localStorage.key(i); | ||
| if (key && key.startsWith(prefix)) { | ||
| toRemove.push(key); | ||
| } | ||
| } | ||
| for (const key of toRemove) { | ||
| localStorage.removeItem(key); | ||
| } | ||
| localStorage.removeItem(ACTIVE_USER_KEY); | ||
| } else { | ||
| // No known user (pre-login recovery) — fall back to clearing everything | ||
| // so we don't leave orphaned blobs with no way to scope the deletion. | ||
| localStorage.clear(); | ||
| } | ||
| sessionStorage.clear(); | ||
| } catch (err) { |
Contributor
There was a problem hiding this comment.
Separate storage clear error paths so session data still gets wiped.
If a localStorage call throws, sessionStorage.clear() is never reached because both are inside the same try. That leaves session data behind in a wipe flow.
Suggested patch
function clearUserScopedStorage(userId: string | null): void {
try {
if (userId) {
const prefix = `${userId}:`;
const toRemove: string[] = [];
for (let i = 0; i < localStorage.length; i++) {
const key = localStorage.key(i);
if (key && key.startsWith(prefix)) {
toRemove.push(key);
}
}
for (const key of toRemove) {
localStorage.removeItem(key);
}
localStorage.removeItem(ACTIVE_USER_KEY);
} else {
// No known user (pre-login recovery) — fall back to clearing everything
// so we don't leave orphaned blobs with no way to scope the deletion.
localStorage.clear();
}
- sessionStorage.clear();
} catch (err) {
- console.warn('[clearAllAppData] storage clear failed:', err);
+ console.warn('[clearAllAppData] localStorage clear failed:', err);
+ }
+ try {
+ sessionStorage.clear();
+ } catch (err) {
+ console.warn('[clearAllAppData] sessionStorage clear failed:', err);
}
}🤖 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 25 - 45, The current try block
around the localStorage logic prevents sessionStorage.clear() from running if
any localStorage call throws; update clearAllAppData (in
app/src/utils/clearAllAppData.ts) to ensure sessionStorage is always cleared by
isolating localStorage operations into their own try/catch (or by using a
try/finally) so any error from the localStorage loop (the prefix/key removal and
localStorage.removeItem calls, and localStorage.clear fallback) is handled
without preventing sessionStorage.clear() from executing; make sure to catch and
log the localStorage error (referencing variables like userId, prefix, and
ACTIVE_USER_KEY) and then still call sessionStorage.clear() in the outer flow.
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
window.localStorage.clear()inclearAllAppDatawith a targeted per-user key sweep so only the active account's${userId}:persist:*keys are removedlocalStorage.clear()only for pre-login recovery (no userId available)userApi.onboardingComplete()call andPOST /settings/onboarding-completefromOnboardingLayout— the endpoint does not exist on the backend and was silently swallowed on every onboarding completionProblem
clearAllAppDatacalledlocalStorage.clear(), which destroyed every account's persisted Redux state (not just the active user's). Logging out or using Settings > Danger Zone on account B would silently wipe account A's chat history, connections, and onboarding flag — the root storage-layer cause of Fix account switching causing data loss and forced re-onboarding after re-login #983.POST /settings/onboarding-completeis not registered in the backend router (src/routes/index.tshas no/settingsmount). Every onboarding completion fired a 404 that was swallowed by a barecatch { console.warn }, making it invisible but wasteful.Solution
clearUserScopedStorage(userId)iterates localStorage keys and removes only those starting with${userId}:, plusOPENHUMAN_ACTIVE_USER_ID. Other accounts' namespaced keys are untouched.onboardingCompletefromuserApi.tsentirely (dead code with no callers after this change). Removed the now-unusedapiClientimport fromuserApi.ts.Submission Checklist
clearAllAppData.tsis fully covered by the updated test suite (5 tests, all pass)Closes #983Impact
Related
DefaultRedirectshould gate the onboarding route on a stable (post-refresh) snapshot to prevent re-onboarding on re-login before the core snapshot resolves — tracked separately.AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/account-switch-storage-scopingValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheck— N/A (no type changes)pnpm debug unit src/utils/__tests__/clearAllAppData.test.ts src/pages/onboarding— 36/36 passValidation Blocked
git push origin fix/account-switch-storage-scopingreact-hooks/set-state-in-effectwarnings in unrelated files (BootCheckGate, RotatingTetrahedronCanvas, etc.)--no-verifyBehavior Changes
clearAllAppData({ userId: 'x' })now removes onlyx-prefixed keys; other users' data survivesParity Contract
localStorage.clear()(same as before)sessionStorage.clear()still runs unconditionallyDuplicate / Superseded PR Handling
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests