Skip to content

fix(settings): require authentication for preferences - #125

Merged
cameroncuster merged 1 commit into
mainfrom
cameroncuster/authenticated-settings
Jul 28, 2026
Merged

fix(settings): require authentication for preferences#125
cameroncuster merged 1 commit into
mainfrom
cameroncuster/authenticated-settings

Conversation

@cameroncuster

Copy link
Copy Markdown
Owner

Summary

  • require an authenticated browser session before any Settings content renders; anonymous direct visits redirect home and the Settings link stays hidden while signed out
  • remove the app's theme localStorage cache entirely; signed-out startup always uses System and follows the OS
  • load and persist signed-in theme preferences through the existing Supabase user-preferences path
  • reset to System on sign-out and guard database reads against newer selections, sign-out, and account switches
  • synchronously hide and clear account-scoped Settings state before redirecting after a sign-out/account change
  • preserve the three-position accessible theme toggle and existing Supabase auth-session storage

Verification

  • pnpm run lint
  • pnpm run lint:es
  • pnpm run check
  • pnpm run test:coverage455 passed; 100% lines / branches / functions for src/**/*.ts
  • pnpm run build
  • pnpm run check:performance
  • full mocked desktop Playwright — 104 passed, 5 intentionally skipped
  • full mocked mobile Playwright — 107 passed, 2 intentionally skipped
  • focused Settings sign-out lifecycle — passed on desktop and mobile
  • two independent release reviews — no remaining blockers

No dependencies or database migrations.


Pull Request opened by Augment Code | View session

@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
gitgud Ready Ready Preview, Comment Jul 28, 2026 10:58pm

@cameroncuster
cameroncuster force-pushed the cameroncuster/authenticated-settings branch from efc377d to e8270b6 Compare July 28, 2026 22:57
@cameroncuster
cameroncuster marked this pull request as ready for review July 28, 2026 23:10
@cameroncuster
cameroncuster merged commit d0038f1 into main Jul 28, 2026
22 of 24 checks passed
@cameroncuster
cameroncuster deleted the cameroncuster/authenticated-settings branch July 28, 2026 23:10
@cameroncuster

cameroncuster commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

PR Risk Analyzer Agent🛡️

👀 Human Input Needed
A pair-review briefing is ready for you.

→ Pair Review Briefing

@cameroncuster cameroncuster left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Deep Code Review Agent🐛

Review completed with 1 suggestions.

}
authorizedUserId = actor.user.id;
const loaded = await fetchUserPreferences();
if (getCurrentActor().user?.id !== authorizedUserId) return;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Deep Code Review Agent🐛

This identity check can leave /settings stuck on the loading screen after a user signs out elsewhere and then navigates directly back to Settings in the same SPA session. resolveCurrentActor() returns the cached bootstrap result from before the auth change, so this path sets authorizedUserId to the old user, fetchUserPreferences() returns null for the current signed-out actor, and this guard returns without ever redirecting or clearing loading.

Severity: medium


🤖 Was this useful? React with 👍 or 👎

@cameroncuster

Copy link
Copy Markdown
Owner Author

Verifier🛰️

🛰️ Verifier Report

TL;DR: Drove the authenticated-settings gate end-to-end in a real browser against the app served on a local mock Supabase — anonymous visitors, a signed-in member, and the theme-cache removal.

🔍 Walkthrough

  • Anonymous /settingsredirected home with no Appearance/Privacy/Import UI and zero user_preferences reads (on mainline the page rendered and read the account; on head neither happens).

    redirect + no-account-read proof, driven live in chromium
    anon-settings  → finalUrl http://localhost:4173/   user_preferences requests: 0
    anon-home      → finalUrl http://localhost:4173/   user_preferences requests: 0
    

    Also asserted headlessly by e2e/smoke.spec.ts:348 "anonymous /settings redirects home without rendering settings or reading preferences" — passed desktop + mobile.

  • Signed-out header hides the Settings entry, showing only Sign in → no gear icon:
    anonymous home header: only Sign in, no Settings gear

  • Signed-in /settings renders the full page and loads prefs through Supabase → 6 user_preferences requests fired (the three-position System/Light/Dark toggle is preserved):
    signed-in settings: Appearance/Theme toggle, Privacy, Import sections

  • Theme cache removed → no localStorage theme read/write remains in the theme service, and startup follows System; exercised live by e2e/smoke.spec.ts:585 "ignores legacy local theme values and follows System" — passed.

📋 Scope

Under test: PR requires an authenticated session before Settings renders (anonymous /settings redirects home, Settings link hidden while signed out), removes the theme localStorage cache so signed-out startup uses System, and loads/persists signed-in theme via the Supabase user-preferences path.

Not tested:

  • Real GitHub OAuth sign-in/sign-out lifecycle (mock seeds the Supabase session directly — the sign-out-reset path is covered by the mocked suite, not real OAuth here).
  • Live Supabase behavior (SUPABASE_SMOKE=1) — off by default and read-only, out of scope.
  • Pure-logic units (theme resolution, preference normalization) — covered by tests/theme-service.test.ts / tests/settings-a11y.test.ts (CI), not re-run here.

📦 Artifacts · anon home · anon settings · signed-in settings

Full mocked Playwright suite (desktop + mobile): 211 passed, 7 skipped.


Don't like what you see? Add a verification skill so the next run tests this kind of change.

Want another run? Comment cosmos verify on this PR.

👍 / 👎 · View session

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.

1 participant