Fix session validation and null userId in localStorage operations#273
Fix session validation and null userId in localStorage operations#273
Conversation
- Fix null userId in onMutate by getting session directly instead of using ref - Add userId validation in validateActiveSession to prevent cross-user leakage - Wrap getSession() calls in try/catch to handle transient errors gracefully - Add isMounted guard in CourseCard to prevent state updates after unmount - Replace dynamic import with static import for Supabase client - Use logger.dev instead of console.debug for consistency Co-authored-by: devakesu <61821107+devakesu@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens client-side user settings persistence by preventing null/incorrect user-scoped localStorage keys and adding session/user validation to avoid stale async writes leaking settings across users.
Changes:
- Derives
userIdfromsupabase.auth.getSession()for user-scoped localStorage keys and skips writes whenuserIdis unavailable. - Adds cross-user validation to storage sync (
session.user.id === expectedUserId) and wraps the async sync effect in try/catch to avoid unhandled rejections. - Updates
CourseCardto use static Supabase imports,logger.dev, and anisMountedguard to prevent setState after unmount.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/providers/user-settings.ts | Prevents null-key localStorage writes; adds expected-user session validation and error handling around async sync logic. |
| src/components/attendance/course-card.tsx | Prevents unmount race in async setting load; replaces dynamic import + console.debug with static imports + logger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (userId) { | ||
| if (newSettings.bunk_calculator_enabled !== undefined) { | ||
| localStorage.setItem(`showBunkCalc_${userId}`, newSettings.bunk_calculator_enabled.toString()); | ||
| window.dispatchEvent(new CustomEvent("bunkCalcToggle", { detail: newSettings.bunk_calculator_enabled })); | ||
| } |
There was a problem hiding this comment.
onMutate performs localStorage.setItem(...) writes without guarding for storage failures (e.g., Safari private mode / blocked storage / quota exceeded). If this throws, it can reject onMutate and potentially abort/rollback the mutation even though storage persistence is intended as best-effort. Consider wrapping the localStorage writes (and related side effects) in a try/catch and logging via logger.dev, similar to how storage writes are handled in login-form.tsx.
| const validateActiveSession = async (expectedUserId: string, isComponentMounted: boolean): Promise<boolean> => { | ||
| if (!isComponentMounted) return false; | ||
| try { | ||
| const { data: { session } } = await supabase.auth.getSession(); | ||
| return !!(session && isComponentMounted); | ||
| // Verify session exists, user matches, and component is still mounted | ||
| return !!(session && session.user.id === expectedUserId && isComponentMounted); |
There was a problem hiding this comment.
validateActiveSession takes isComponentMounted: boolean, but passing isMounted here only passes a snapshot of the value at call time. If the effect unmounts while awaiting getSession(), validateActiveSession(...) can still return true, and callers may continue with side effects (notably the mutateSettings(...) call in the settings === null branch) after unmount. Prefer closing over the isMounted flag (or passing a () => boolean / AbortSignal) and re-checking mount state immediately after awaiting before doing side effects/mutations.
* feat: update localStorage handling for user-specific settings - Upgrade glob package to version 13.0.1. - Modify CourseCard component to use user ID for scoped localStorage keys for the bunk calculator setting. - Update useUserSettings hook to sync user settings to localStorage using user-specific keys. - Implement cleanup of legacy localStorage keys after migrating to user-scoped keys. - Ensure session validation checks are performed correctly to prevent race conditions during user logout. * Fix session validation and null userId in localStorage operations (#273) * Initial plan * fix: address PR review comments for localStorage and session validation - Fix null userId in onMutate by getting session directly instead of using ref - Add userId validation in validateActiveSession to prevent cross-user leakage - Wrap getSession() calls in try/catch to handle transient errors gracefully - Add isMounted guard in CourseCard to prevent state updates after unmount - Replace dynamic import with static import for Supabase client - Use logger.dev instead of console.debug for consistency Co-authored-by: devakesu <61821107+devakesu@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: devakesu <61821107+devakesu@users.noreply.github.com> * Fix localStorage race conditions and error handling in user settings (#274) * Initial plan * Address review comments: add error handling, fix race conditions, remove dead code Co-authored-by: devakesu <61821107+devakesu@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: devakesu <61821107+devakesu@users.noreply.github.com> --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Addresses race conditions and null reference bugs in user settings persistence that could cause cross-user data leakage and localStorage corruption.
Changes
Prevent null userId in localStorage keys (user-settings.ts:196-204)
onMutatenow derivesuserIdfromsupabase.auth.getSession()instead ofcurrentUserIdRef.currentuserIdis unavailable (preventsshowBunkCalc_nullkeys)Add cross-user validation in session checks (user-settings.ts:254-259)
validateActiveSession(expectedUserId, isComponentMounted)now verifiessession.user.id === expectedUserIdHandle getSession() failures gracefully (user-settings.ts:269-275)
Prevent unmount race in CourseCard (course-card.tsx:75-84)
isMountedguard to skipsetShowBunkCalcafter component unmountsUse static imports (course-card.tsx:1-14)
import("@/lib/supabase/client")with static importconsole.debug→logger.devfor consistency💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.