Skip to content

Fix session validation and null userId in localStorage operations#273

Merged
devakesu merged 2 commits into1.4.8from
copilot/sub-pr-272
Feb 6, 2026
Merged

Fix session validation and null userId in localStorage operations#273
devakesu merged 2 commits into1.4.8from
copilot/sub-pr-272

Conversation

Copy link
Contributor

Copilot AI commented Feb 6, 2026

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)

  • onMutate now derives userId from supabase.auth.getSession() instead of currentUserIdRef.current
  • Skips localStorage writes when userId is unavailable (prevents showBunkCalc_null keys)

Add cross-user validation in session checks (user-settings.ts:254-259)

  • validateActiveSession(expectedUserId, isComponentMounted) now verifies session.user.id === expectedUserId
  • Prevents stale async operations from writing settings for the wrong user after logout/login

Handle getSession() failures gracefully (user-settings.ts:269-275)

  • Wrapped sync effect IIFE in try/catch to prevent unhandled rejections on transient auth/network errors

Prevent unmount race in CourseCard (course-card.tsx:75-84)

  • Added isMounted guard to skip setShowBunkCalc after component unmounts

Use static imports (course-card.tsx:1-14)

  • Replaced dynamic import("@/lib/supabase/client") with static import
  • Switched console.debuglogger.dev for consistency
// Before: null userId creates invalid keys
localStorage.setItem(`showBunkCalc_${currentUserIdRef.current}`, ...);  // "showBunkCalc_null"

// After: skip writes when no session
const { data: { session } } = await supabase.auth.getSession();
const userId = session?.user?.id;
if (userId) {
  localStorage.setItem(`showBunkCalc_${userId}`, ...);
}

💡 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.

- 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>
Copilot AI changed the title [WIP] Update localStorage handling for user-specific settings Fix session validation and null userId in localStorage operations Feb 6, 2026
Copilot AI requested a review from devakesu February 6, 2026 18:38
@devakesu devakesu marked this pull request as ready for review February 6, 2026 18:39
Copilot AI review requested due to automatic review settings February 6, 2026 18:39
@devakesu devakesu merged commit bbdf468 into 1.4.8 Feb 6, 2026
@devakesu devakesu deleted the copilot/sub-pr-272 branch February 6, 2026 18:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 userId from supabase.auth.getSession() for user-scoped localStorage keys and skips writes when userId is 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 CourseCard to use static Supabase imports, logger.dev, and an isMounted guard 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.

Comment on lines +202 to +206
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 }));
}
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +261 to +266
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);
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
devakesu added a commit that referenced this pull request Feb 6, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants