fix(auth): scope clearAllAppData to active user; fix re-onboarding race; drop dead API call#1816
Conversation
…boarding-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. 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 from OnboardingLayout — the endpoint
does not exist on the backend and was silently swallowed on every
onboarding completion.
Closes tinyhumansai#983
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemoves the backend onboardingComplete API call, scopes localStorage clearing to the active user (null fallback clears all), adds a DefaultRedirect post-login guard to avoid race redirects, and updates tests to validate user-scoped clearing and routing behavior. ChangesAccount Data Isolation and Onboarding Cleanup
Sequence DiagramsequenceDiagram
participant OnboardingLayout
participant LocalStorage
participant Analytics
participant Joyride
participant Router
OnboardingLayout->>LocalStorage: persist `onboarding_completed` flag
OnboardingLayout->>Analytics: fire `onboarding_complete` event
OnboardingLayout->>Joyride: set walkthrough pending flag (best-effort)
OnboardingLayout->>Router: navigate to /home
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/clearAllAppData.ts`:
- Around line 25-46: The current try block in clearAllAppData contains both
localStorage operations and sessionStorage.clear(), so if any localStorage call
throws the catch executes and sessionStorage.clear() is skipped; fix this in
clearAllAppData by moving sessionStorage.clear() into a finally block (or into
its own try/catch after the existing catch) so it always runs regardless of
localStorage errors, while keeping the existing localStorage logic (prefix loop,
toRemove array, removal of ACTIVE_USER_KEY) and preserving the existing catch
logging for localStorage failures.
🪄 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: 73bb3dd7-bfe7-4294-bac7-fc9262c957aa
📒 Files selected for processing (4)
app/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
sessionStorage.clear() was inside the try block so a localStorage throw would skip it via the catch. Moved it into a finally block so it always runs regardless of localStorage failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n snapshot After logout, toSignedOutSnapshot() sets onboardingCompleted: false and currentUser: null. On re-login, the session token can arrive via core-state:session-token-updated before refresh() resolves — leaving a window where sessionToken is truthy but the snapshot is still in the signed-out cleared state. DefaultRedirect was routing to /onboarding during this window, forcing re-onboarding for users who had already completed it. Fix: if sessionToken is set but currentUser is null the snapshot hasn't settled yet — show the loading screen and wait for the next refresh cycle before making the routing decision. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
YellowSnnowmann
changed the title
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Walkthrough
Solid three-bug fix for #983 — scoped localStorage clear, post-login race guard, and dead API removal. The approach is sound and the scoped deletion logic is well-implemented (collecting keys before removal to avoid mutation-during-iteration is a nice touch). Tests cover the core scenarios well. Two things need attention before this ships: the new loading guard needs a safety net for failed refreshes, and the OnboardingLayout test still mocks the removed API call.
Change Summary
| File | Change type | Description |
|---|---|---|
app/src/utils/clearAllAppData.ts |
Modified | Added clearUserScopedStorage() — scoped localStorage deletion by userId prefix, replacing the nuclear localStorage.clear() |
app/src/components/DefaultRedirect.tsx |
Modified | Added !currentUser null guard to prevent premature onboarding redirect during post-login race |
app/src/components/__tests__/DefaultRedirect.test.tsx |
New | 5 test cases covering bootstrap, no-session, post-login race, new-user, and returning-user states |
app/src/pages/onboarding/OnboardingLayout.tsx |
Modified | Removed dead userApi.onboardingComplete() call (endpoint never existed) |
app/src/services/api/userApi.ts |
Modified | Removed onboardingComplete method and apiClient import |
app/src/utils/__tests__/clearAllAppData.test.ts |
Modified | Updated tests to assert scoped key deletion and cross-account preservation |
| // would be wrong for any returning user whose flag is already true. | ||
| if (!snapshot.currentUser) { | ||
| return <RouteLoadingScreen />; | ||
| } |
There was a problem hiding this comment.
[major] This guard correctly prevents the race condition, but has no timeout or fallback. If refresh() fails after login (network error, core crash, etc.), currentUser stays null forever and the user is stuck on <RouteLoadingScreen /> with no way out.
Looking at CoreStateProvider, refresh() failures are caught and logged but don't clear the session token — so this guard would hold indefinitely.
Consider adding a timeout that falls back to clearing the session:
const [waitingForUser, setWaitingForUser] = useState(false);
useEffect(() => {
if (snapshot.sessionToken && !snapshot.currentUser) {
setWaitingForUser(true);
const timer = setTimeout(() => {
// refresh() likely failed — fall back to signed-out state
clearSession();
}, 10_000);
return () => clearTimeout(timer);
}
setWaitingForUser(false);
}, [snapshot.sessionToken, snapshot.currentUser]);Or at minimum, add debug logging so the stuck state is diagnosable:
if (!snapshot.currentUser) {
debug('default-redirect')('waiting for currentUser — token set but snapshot not yet refreshed');
return <RouteLoadingScreen />;
}| expect(screen.queryByText('Home')).not.toBeInTheDocument(); | ||
| expect(screen.queryByText('Welcome')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[minor] This test only asserts negatives (nothing is visible), but doesn't positively assert the loading screen is shown. If DefaultRedirect accidentally rendered nothing at all, this test would still pass.
Consider adding a positive assertion — e.g., checking for a loading indicator or a data-testid on RouteLoadingScreen.
Addresses graycyrus PR review on tinyhumansai#1816: - DefaultRedirect: add console.debug when waiting on currentUser so a stuck state (refresh() never resolves after session-token-updated) is diagnosable from logs rather than appearing as a silent loading screen. - DefaultRedirect.test.tsx: positively assert the loading screen renders in the post-login race case (previously only asserted negatives, which would pass even if the component rendered nothing).
…ce; drop dead API call (tinyhumansai#1816) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
3 participants
Summary
window.localStorage.clear()inclearAllAppDatawith a targeted per-user key sweep — only${userId}:*keys andOPENHUMAN_ACTIVE_USER_IDare removed; falls back to full clear only for pre-login recovery (no userId)sessionStorage.clear()into afinallyblock so it runs even when a localStorage error is thrownuserApi.onboardingComplete()call fromOnboardingLayoutand theonboardingCompletemethod fromuserApi.ts—POST /settings/onboarding-completedoes not exist on the backendDefaultRedirect's onboarding-vs-home decision oncurrentUser !== null— prevents routing to/onboardingduring the post-login window where the session token has arrived butrefresh()hasn't resolved yetProblem
Three separate bugs all contributed to #983:
clearAllAppDatacalledlocalStorage.clear(), destroying every account's persisted Redux state. Logging out on account B silently wiped account A's chat history, connections, and onboarding flag.toSignedOutSnapshot()resetsonboardingCompleted: falseandcurrentUser: nullon logout. Thecore-state:session-token-updatedevent setssessionTokenbeforerefresh()resolves, leaving a window whereDefaultRedirectsawsessionTokentruthy +onboardingCompleted: falseand routed returning users to/onboardinginstead of/home.POST /settings/onboarding-completeis not registered in the backend router. Every onboarding completion fired a silent 404.Solution
clearUserScopedStorage(userId)iterates localStorage keys and removes only those prefixed${userId}:. Other accounts' keys survive.sessionStorage.clear()is in afinallyblock so it always runs.DefaultRedirectnow shows<RouteLoadingScreen />whensessionTokenis set butcurrentUseris null — waits for the snapshot to settle before making any routing decision.onboardingCompletefromuserApi.tsand its call-site inOnboardingLayout.tsx.clearAllAppData.test.tsasserts cross-account key preservation; newDefaultRedirect.test.tsx(5 tests) covers bootstrapping, no-session, post-login race, new-user onboarding, and returning-user home redirect.Submission Checklist
Closes #983Impact
Related
AI Authored PR Metadata
Commit & Branch
fix/account-switch-scoped-clearValidation Run
pnpm --filter openhuman-app format:check— passedpnpm debug unit src/utils/__tests__/clearAllAppData.test.ts src/pages/onboarding src/components/__tests__/DefaultRedirect.test.tsx— all passValidation Blocked
react-hooks/set-state-in-effectwarnings in unrelated files (BootCheckGate, RotatingTetrahedronCanvas, CommandProvider, etc.) — present onmainbefore this branch; pushed with--no-verifyper CLAUDE.mdBehavior Changes
clearAllAppData({ userId: 'x' })now removes onlyx-prefixed keys; other accounts' data is preservedDefaultRedirectshows a loading screen during post-login snapshot settling instead of routing prematurely to/onboarding/settings/onboarding-completeSummary by CodeRabbit
Bug Fixes
Tests