fix(ux): restore auth CTA, minimal loading, and move theme control to Settings - #116
Conversation
… settings - Restore signed-out CTA to 'Continue with GitHub' (desktop, mobile, tests) - Restore minimal auth-callback loading treatment (spinner + 'Signing you in…') in place of the page-like card, keeping the hardened redirect logic - Remove the header theme cycle control on desktop and mobile - Add a compact single-target theme cycle to the Settings page (system → light → dark → system), preserving Supabase/localStorage persistence; no radio panels reintroduced - Preserve the compact Settings gear icon - Update focused unit and Playwright coverage
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 I've got this PR. I'll handle review feedback, CI failures, and merge conflicts, and ping you the moment it's ready for review. Drop a comment anytime. Picking reviewers is your call — I won't request anyone myself. |
|
👀 Human Input Needed → Pair Review Briefing |
cameroncuster
left a comment
There was a problem hiding this comment.
Review completed with 1 suggestions.
Recent work (#108) replaced the Settings loading state's shared animate-spin spinner with a bare 'Loading settings…' line, leaving a janky page-like interim state. Restore the exact canonical spinner markup still used by the contests, leaderboard, and problem loading surfaces, with the matching 'Loading settings...' copy. Add unit + Playwright regression coverage that the spinner renders and the regressed plain page copy does not.
|
Verifier 🛰️ 🛰️ Verifier ReportTL;DR: Built the PR head and drove it in a real browser (desktop + mobile) — checked the restored auth CTA, the minimal sign-in callback, the removal of the header theme control, and the new Settings theme cycle — against the repo's hermetic mock-Supabase preview. 🔍 Walkthrough
📋 ScopeUnder test: Restores the Not tested:
📦 Artifacts · targeted e2e ( Don't like what you see? Add a verification skill so the next run tests this kind of change. Want another run? Comment 👍 / 👎 · View session |
Deep Code Review flagged (PR #116, r3653003232) that with the header theme control removed and /settings redirecting signed-out users, anonymous visitors had no reachable UI for the localStorage theme preference. - Stop redirecting anonymous visitors from /settings; render the Appearance theme cycle for everyone. - Gate the account-only Privacy and Import sections behind {#if user}, and only call fetchUserPreferences() when a session exists, so no account query fires for anonymous visitors. - Add the compact Settings gear to the signed-out header (desktop + mobile) so anonymous users can reach Settings; the theme picker stays out of the header. - Preserve system default and localStorage persistence; keep responsive footer spacing by moving the bottom margin onto the last visible card. - Update/extend unit + Playwright coverage: anonymous can reach Settings, cycle system->light->dark->system, persist across reload, sees no account-only sections and triggers no account read; signed-in Supabase persistence retained.
🛰️ Verifier — kickoffStarting an end-to-end runtime verification of this PR at exact head SHA Scenario plan (desktop + mobile where applicable):
Also explicitly checking that blocking review issue Milestone updates and a final per-scenario findings comment (evidence for human judgment, not a verdict) will follow. 🤖 Verifier · View session |
🛰️ Verifier — milestone 1/2 (anonymous desktop path, SHA
|
🛰️ Verifier — findings (evidence, not a verdict)TL;DR: Drove this PR's UX changes end-to-end against a live browser — the anonymous path on a real Vite dev server through the actual UI, and the signed-in / transient-loading / callback states through the app's own headless-Chromium e2e suite — all at exact head SHA SHA: 🔍 Per-scenario evidence
✅ Blocking review issue
|







Problem
Recent changes introduced signed-out/loading/theme UX regressions: the auth CTA was renamed to
Sign up, the OAuth callback became a page-like card, the Settings loading state lost its spinner (bareLoading settings…text), and the theme control lived in the header as a prominent picker.Solution
Restore the prior behavior from git history and consolidate theme selection in Settings:
Continue with GitHub(desktop, mobile, e2e) — reverts fix(auth): simplify sign-up label #111.Signing you in…instead of the page-like card, keeping feat(settings): add system theme and Kattis solve import #108's hardened redirect logic.animate-spinmarkup still used by the contests, leaderboard, and problem loading surfaces (with matchingLoading settings...copy) — reverts the feat(settings): add system theme and Kattis solve import #108 plain-text regression. Audited all loading surfaces touched by the recent header/settings/theme/auth work; Settings was the only regressed one (ProblemDisplay/contests/leaderboard spinners were untouched).system → light → dark → system), ≥44×44px, keyboard/SR-accessible via the existingThemeCycleButton; default stayssystem. Persistence unchanged: signed-in → Supabase, anonymous → localStorage. No radio panels reintroduced.No new dependencies. Historical spinner/copy/style used as the source of truth — no new loading design invented.
Testing
pnpm run lint,pnpm run lint:es,pnpm run check— clean.pnpm run test— 400/400 pass (updatedsettings-a11y+theme-service; new settings-spinner regression test).pnpm exec playwright test e2e/smoke.spec.ts e2e/auth-sanity.spec.ts— 78 passed / 6 skipped (desktop + mobile), covering exact auth copy, minimal callback markup, absence of the header theme control, Settings theme cycling/default/persistence/a11y, and the Settings loading spinner (asserts the spinner renders and the regressedLoading settings…page copy does not).pnpm run buildpasses when the public Supabase build-time vars are supplied (as CI/Playwright do). See caveat.Misc
pnpm run buildfails on this branch and on cleanmainwithoutPUBLIC_SUPABASE_URL/PUBLIC_SUPABASE_PUBLISHABLE_KEY(inlined from$env/static/publicat build time). Pre-existing environment/secrets requirement, not a regression from this PR; the Playwright config injects these itself, so the e2e build/run is green.