Skip to content

fix: critical UI issues from full-project audit#25

Merged
vayungodara merged 4 commits into
mainfrom
fix/audit-ui-fixes
Mar 16, 2026
Merged

fix: critical UI issues from full-project audit#25
vayungodara merged 4 commits into
mainfrom
fix/audit-ui-fixes

Conversation

@vayungodara

@vayungodara vayungodara commented Mar 16, 2026

Copy link
Copy Markdown
Owner

Summary

UI fixes (from full-project audit)

  • Sidebar overlap: Added paddingLeft: 16 to main content area so text is no longer clipped by the 72px collapsed sidebar
  • 404 page: Created branded app/not-found.js with gradient styling and navigation back to dashboard
  • Login redirect: Added /login/ permanent redirect in next.config.mjs
  • Grammar fix: "1 minutes" → "1 minute" on Focus Timer recent sessions
  • Page titles: Added metadata exports for Focus Timer, Stats, and Settings pages
  • Color contrast: Adjusted dark mode --text-tertiary and --text-muted to meet WCAG AA 4.5:1 ratio

Tech debt cleanup

Issues closed (already implemented)

Note: Supabase security fixes (revoke anon grants, auth checks on SECURITY DEFINER functions, search_path fixes) and performance fixes (indexes, NOT NULL constraints, autovacuum tuning, ANALYZE) were applied directly via SQL and are not part of this PR.

Test plan

  • Verify dashboard text is no longer clipped by sidebar in collapsed state
  • Visit /nonexistent-page and confirm branded 404 page appears
  • Visit /login and confirm redirect to landing page
  • Complete a 1-minute focus session and verify it shows "1 minute" not "1 minutes"
  • Check browser tab titles on Focus, Stats, and Settings pages
  • Toggle dark mode and verify timestamp/label text contrast is improved
  • Verify no broken images anywhere in the app
  • CI workflow runs on this PR

🤖 Generated with Claude Code

- Fix sidebar overlapping main content (added padding to main area)
- Add branded 404 page with navigation back to dashboard
- Add /login → / redirect for users expecting a login route
- Fix "1 minutes" grammar bug on Focus Timer recent sessions
- Add consistent page titles for Focus, Stats, Settings pages
- Fix dark mode color contrast to meet WCAG AA 4.5:1 ratio
  (--text-tertiary: #6E6990 → #9590AD, --text-muted: #4A4568 → #7E79A0)

Supabase security and performance fixes were applied directly via SQL.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 16, 2026 16:29
@vercel

vercel Bot commented Mar 16, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
lockin Ready Ready Preview, Comment Mar 16, 2026 5:12pm

This comment was marked as resolved.

- Delete 7 unused landing PNGs from repo root (~2.5MB), no code references them
- Add CI workflow: build + lint on push/PR, e2e tests after build passes

Closes #15, closes #19

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vayungodara

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Mar 16, 2026

Copy link
Copy Markdown

Claude finished @vayungodara's task in 1m 46s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze code
  • Post review feedback

Overall this is a solid cleanup PR. The grammar fix, metadata exports, redirect, and dark mode contrast improvements are all clean. A few real bugs to address below.


🔴 Bug — paddingLeft: 16 reduces content padding

app/dashboard/DashboardLayout.js:47

style={{ marginLeft: sidebarExpanded ? 260 : 72, paddingLeft: 16 }}

.main in the CSS module has padding: var(--space-8) (32px all sides). The inline paddingLeft: 16 overrides the CSS left padding with a smaller value — so instead of preventing sidebar clipping, it actually reduces left padding from 32px → 16px.

The sidebar offset is already handled by marginLeft. If content was being clipped, the root cause is more likely that --space-8 padding on the left side (32px) wasn't enough, or that the sidebar was overlapping the content region. A safer fix without breaking the other sides:

style={{ marginLeft: sidebarExpanded ? 260 : 72 }}
// and in DashboardLayout.module.css:
// paddingLeft: calc(var(--space-8) + 16px)

Or just verify whether this is actually needed — with marginLeft: 72 the content region starts after the sidebar, so 32px of left padding should be plenty.

Fix this →


🔴 Bug — CI build job missing env vars

.github/workflows/ci.yml:22

The build-and-lint job runs npx next build without any env vars. Next.js will likely fail or produce broken output without NEXT_PUBLIC_SUPABASE_URL / NEXT_PUBLIC_SUPABASE_ANON_KEY. The e2e job has them, but the build job doesn't:

- run: npx next build
  # missing env: block here

Add the same env: block to the build step, or the CI will fail on every PR unless you have dummy values baked in.

Fix this →


🟡 Convention — not-found.js uses inline styles instead of CSS module

app/not-found.js

Every other component in the project uses ComponentName.module.css. This file has ~40 lines of inline styles. Project conventions say file naming is ComponentName.js + ComponentName.module.css. Not a blocker, but worth a quick extraction to keep things consistent.


🟡 UX — 404 CTA is misleading for signed-out users

app/not-found.js:50

<Link href="/dashboard">Back to Dashboard</Link>

Unauthenticated users who hit a 404 will be redirected from /dashboard back to / — so the button works, but says "dashboard" and lands them on the landing page. Either link to / with copy like "Go home", or keep /dashboard but note that auth middleware will handle the redirect gracefully (lower priority).


🟡 Dead CSS transition on 404 CTA

app/not-found.js:47

transition: 'transform 200ms, box-shadow 200ms',

No hover/focus/active styles exist that change transform or box-shadow, so this transition never fires. Either add :hover styles (requires moving to a CSS module) or remove the transition.


🟡 100vh vs 100dvh on 404 page

app/not-found.js:8

minHeight: '100vh',

Mobile browsers with dynamic URL bars shrink the viewport, causing 100vh to overflow. The rest of the app uses 100dvh (with 100vh as a fallback in CSS modules). For an inline style you can use:

minHeight: 'var(--100dvh, 100dvh)'
// or just move to a .module.css with the fallback pattern

✅ What's good

  • Grammar fix (duration_minutes === 1 ? 'minute' : 'minutes') is correct and handles 0 gracefully
  • Dark mode contrast — bumping --text-tertiary to #9590AD and --text-muted to #7E79A0 on #08090D background is a real improvement (previous values were likely borderline)
  • Metadata exports on focus/stats/settings pages — clean, no issues
  • /login redirect — correct use of permanent: true for a stable alias
  • CI pipeline structure — gating e2e on build-and-lint via needs: is the right pattern
  • PNG cleanup — removing 7 orphaned images from repo root is a good housekeeping win

| Branch preview

Review fixes:
- Sidebar: use marginLeft: 80 instead of paddingLeft override (was clobbering CSS module's 32px)
- 404 page: extract to CSS module, use 100dvh, link to "/" not "/dashboard"
- CI: add env vars to build job so Next.js build succeeds

Pre-existing lint fixes (exposed by new CI):
- ActivityComments: move fetchComments to click handler instead of useEffect
- DailySummaryCard: rename useStreakFreeze → applyStreakFreeze (not a React hook)
- XPBar: move xpChanged logic into fetch callback to avoid sync setState in effect

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vayungodara

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Mar 16, 2026

Copy link
Copy Markdown

Claude finished @vayungodara's task in 1m 52s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze code
  • Post review feedback

All issues from the previous review have been addressed. Here's the full picture:


✅ Previous issues — all fixed

Issue Resolution
paddingLeft: 16 overriding CSS module padding Removed entirely; marginLeft updated to 80 (sidebar collapsed width)
CI build job missing env vars NEXT_PUBLIC_SUPABASE_URL + NEXT_PUBLIC_SUPABASE_ANON_KEY added to build step
not-found.js using inline styles instead of CSS module Extracted to not-found.module.css
404 CTA linking to /dashboard for unauthenticated users Changed to / with copy "Go home" ✅
Dead transition with no hover styles .link:hover added to CSS module with matching transform/box-shadow
100vh only on 404 page CSS module uses min-height: 100vh; min-height: 100dvh; fallback pattern ✅

One minor note

app/not-found.js:47marginLeft: 80 vs CLAUDE.md "72px collapsed"

The sidebar collapsed marginLeft was changed from 72 to 80. This is correct if the sidebar is actually 80px wide when collapsed — CLAUDE.md just has a stale value. Worth a quick update to the docs (not a blocker for this PR).


✅ Everything else looks good

  • Grammar fix (duration_minutes === 1 ? 'minute' : 'minutes') — correct, handles 0 gracefully
  • Dark mode contrast#9590AD on #08090D ≈ 6.9:1 and #7E79A0 on #08090D ≈ 5.2:1, both pass WCAG AA ✅
  • Metadata exports on focus/stats/settings pages — clean
  • /login redirect — correct use of permanent: true
  • CI pipeline — env vars now present on both build and e2e jobs, needs: gating is correct
  • PNG cleanup — 7 orphaned images removed cleanly
  • not-found.module.css — consistent with project conventions, uses design tokens throughout

This PR is ready to merge.


| Branch preview

Security:
- Add user_id filter to focus_sessions query (defense in depth, not just RLS)

Performance:
- Clean up XPBar flash setTimeout on unmount via ref

Architecture:
- Change /login redirect from permanent (301) to temporary (302)
- Remove dead metadata export from not-found.js (Next.js ignores it)
- Move CI e2e env vars to job level so Playwright dev server inherits them

Accessibility:
- Add forced-colors media query so 404 heading is visible in High Contrast Mode
- Add :focus-visible style on 404 CTA link for keyboard users
- Use <main> landmark on 404 page for screen readers

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vayungodara vayungodara merged commit 275863b into main Mar 16, 2026
4 of 5 checks passed
@vayungodara vayungodara deleted the fix/audit-ui-fixes branch March 16, 2026 17:24
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.

Add CI/CD pipeline (build + lint + test) Move images from repo root to /public/images/

2 participants