Skip to content

Conversation

@sonyccd
Copy link
Owner

@sonyccd sonyccd commented Jan 15, 2026

Summary

  • Add new Accessibility section in Settings modal with font and text options
  • Font Selection: Default, OpenDyslexic (designed for dyslexic readers), or Arimo
  • Bold All Text: Toggle to make all text bold for better readability
  • Extra Large Text: 125% font size increase for vision accessibility
  • Reorganize ProfileModal: move Delete Account into Account menu with inline confirmation
  • Include OpenDyslexic font files (OTF format) and flash prevention script

Test plan

  • Navigate to Settings → Accessibility
  • Test font selection (Default, OpenDyslexic, Arimo) - verify fonts change
  • Test Bold All Text toggle - verify all text becomes bold
  • Test Extra Large Text toggle - verify 125% size increase
  • Refresh page - verify settings persist
  • Navigate to Settings → Account → Delete Account - verify inline confirmation works
  • Run npm run test:run - all 2921 tests pass

🤖 Generated with Claude Code

Add new Accessibility section in Settings with the following options:
- Font selection: Default, OpenDyslexic (for dyslexia), or Arimo
- Bold All Text: toggle to make all text bold
- Extra Large Text: 125% font size increase

Also reorganizes ProfileModal:
- Move Delete Account into Account menu with inline confirmation
- Separate Accessibility into its own menu section

Includes OpenDyslexic font files (OTF format) and flash prevention
to avoid layout shift on page load.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@vercel
Copy link
Contributor

vercel bot commented Jan 15, 2026

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

Project Deployment Review Updated (UTC)
openhamprep Ready Ready Preview, Comment Jan 15, 2026 1:58am

@supabase
Copy link

supabase bot commented Jan 15, 2026

Updates to Preview Branch (feature/accessibility-settings) ↗︎

Deployments Status Updated
Database Thu, 15 Jan 2026 01:58:37 UTC
Services Thu, 15 Jan 2026 01:58:37 UTC
APIs Thu, 15 Jan 2026 01:58:37 UTC

Tasks are run on every commit but only new migration files are pushed.
Close and reopen this PR if you want to apply changes from existing seed or migration files.

Tasks Status Updated
Configurations Thu, 15 Jan 2026 01:58:37 UTC
Migrations Thu, 15 Jan 2026 01:58:38 UTC
Seeding Thu, 15 Jan 2026 01:58:38 UTC
Edge Functions Thu, 15 Jan 2026 01:58:40 UTC

View logs for this Workflow Run ↗︎.
Learn more about Supabase for Git ↗︎.

@sentry
Copy link

sentry bot commented Jan 15, 2026

Codecov Report

❌ Patch coverage is 95.81590% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.01%. Comparing base (c42731b) to head (d324dc1).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/hooks/useAccessibility.tsx 91.59% 10 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   86.91%   87.01%   +0.10%     
==========================================
  Files         117      119       +2     
  Lines       14689    14891     +202     
  Branches     2264     2302      +38     
==========================================
+ Hits        12767    12958     +191     
- Misses       1922     1933      +11     
Flag Coverage Δ
unittests 87.01% <95.81%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/components/AccessibilitySettings.tsx 100.00% <100.00%> (ø)
src/components/ProfileModal.tsx 74.08% <100.00%> (+1.59%) ⬆️
src/hooks/useAccessibility.tsx 91.59% <91.59%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude
Copy link

claude bot commented Jan 15, 2026

Pull Request Review

This is a well-implemented accessibility feature that follows the project's patterns and guidelines. The implementation is solid, but I've identified a few areas for improvement.


✅ Strengths

Code Quality

  • Excellent context pattern: useAccessibility hook follows the same pattern as useAuth and useTheme with proper provider wrapping
  • Clean component structure: AccessibilitySettings.tsx is concise and well-organized with semantic UI components
  • FOUC prevention: The inline script in index.html correctly mirrors the theme flash prevention pattern to avoid layout shifts
  • Test coverage: Comprehensive test updates in ProfileModal.test.tsx covering the new UI flow and delete account reorganization
  • Proper localStorage handling: Try-catch blocks prevent errors in environments without localStorage

Design System Compliance

  • All new components use semantic tokens instead of direct colors ✅
  • Font loading uses font-display: swap to prevent text flashing
  • UI follows existing patterns with proper icon usage and spacing

Accessibility Features

  • Good font choices: OpenDyslexic for dyslexia support, Arimo as clean alternative
  • 125% scaling is appropriate
  • Bold text option helps with low vision
  • Proper ARIA labels on toggle buttons

@claude
Copy link

claude bot commented Jan 15, 2026

🔍 Issues & Recommendations

1. CSS Specificity Concern - !important Overuse (Medium Priority)

Location: src/index.css:152, 166, 189

The CSS uses !important to enforce font family and bold text changes. While this ensures the accessibility features override everything, it could interfere with components that intentionally use specific fonts like JetBrains Mono for code blocks or math notation.

Recommendation: Consider excluding specific elements like code blocks and test with math formulas (KaTeX) to ensure readability isn't compromised

2. Performance - Font File Size (Low Priority)

Location: public/fonts/OpenDyslexic-Bold.otf, public/fonts/OpenDyslexic-Regular.otf

OTF files are typically larger than WOFF2 format. These fonts will always load even if users don't enable them.

Recommendation: Convert to WOFF2 format for ~30% smaller file size, or consider lazy-loading fonts only when selected

3. Type Safety - Missing Validation (Medium Priority)

Location: src/hooks/useAccessibility.tsx:39

If a user manually edits localStorage to an invalid value, the type assertion bypasses validation.

Recommendation: Add validation in getStoredValue to check against valid FontFamily values before returning

4. Test Coverage - Missing Hook Tests (Medium Priority)

Location: src/hooks/useAccessibility.tsx

The new hook has no dedicated unit tests. ProfileModal.test.tsx covers integration, but edge cases aren't tested:

  • localStorage unavailable (private browsing)
  • Invalid stored values
  • SSR scenario

Recommendation: Add useAccessibility.test.tsx to test these scenarios

5. Missing Edge Case - Delete Account Dialog State (Low Priority)

Location: src/components/ProfileModal.tsx:553-641

The showDeleteConfirm state isn't reset when navigating away from the Account view. If user clicks Delete Account, navigates away and back, the confirmation is still visible.

Fix: Add useEffect to reset state when currentView changes

@claude
Copy link

claude bot commented Jan 15, 2026

🔒 Security & Performance

Security

No security concerns identified. The implementation:

  • ✅ Only stores UI preferences in localStorage (no sensitive data)
  • ✅ Doesn't introduce XSS vectors
  • ✅ Font files are self-hosted (no external CDN dependencies)

Performance

Minimal performance impact:

  • CSS class toggles on <html> are efficient
  • localStorage reads happen only on mount
  • Font loading uses font-display: swap to prevent blocking

📊 Test Coverage

Excellent test updates:

  • ✅ All ProfileModal.test.tsx tests updated for new UI structure
  • ✅ New tests for Accessibility menu item navigation
  • ✅ Delete account flow tests updated for inline confirmation
  • ✅ Helper function navigateToDeleteConfirm improves test maintainability

Missing:

  • Unit tests for useAccessibility hook
  • E2E test to verify fonts actually change visually (optional)

🎯 Final Verdict

Recommendation: Approve with minor changes

This PR is production-ready with only minor improvements suggested. The core implementation is solid, follows project conventions, and adds meaningful accessibility features.

Must Fix Before Merge:

  1. Add validation for localStorage font values (Issue Bump js-yaml from 4.1.0 to 4.1.1 in the npm_and_yarn group across 1 directory #3)
  2. Fix delete confirmation state reset (Issue Add Claude Code GitHub Workflow #5)

Nice to Have:

  • Add useAccessibility unit tests
  • Convert fonts to WOFF2
  • Test with code blocks/math notation

Score: 8.5/10 - Well done! 🎉


Reviewed by: Claude Code
Review Date: 2026-01-15

- Exclude code blocks, .mono, and KaTeX elements from font overrides
- Add validation for FontFamily values from localStorage
- Add useAccessibility.test.tsx with 18 unit tests covering:
  - Default values
  - Loading from localStorage
  - Invalid stored values fallback
  - Setting values and persistence
  - CSS class application
  - localStorage unavailable scenarios
  - Hook usage outside provider
- Reset delete confirmation dialog when navigating away from Account view

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Jan 15, 2026

PR Review - Accessibility Settings Feature

Overview

This PR adds comprehensive accessibility settings to improve the app's usability for users with dyslexia and visual impairments. The implementation is well-structured with proper testing and follows the project's conventions.

✅ Strengths

1. Excellent Architecture

  • Clean separation of concerns with dedicated useAccessibility hook
  • Proper context provider pattern following existing useAuth and useAppNavigation patterns
  • Flash prevention script in index.html prevents layout shift on page load (great UX detail)

2. Strong Type Safety

  • Explicit FontFamily type with validation (isValidFontFamily)
  • Input validation prevents invalid values from localStorage corrupting state
  • Proper TypeScript throughout

3. Comprehensive Testing

  • 18 unit tests in useAccessibility.test.tsx covering:
    • Default values and localStorage loading
    • Invalid value fallback (important for security/stability)
    • Setting persistence
    • CSS class application
    • Error scenarios (localStorage unavailable)
    • Hook misuse (outside provider)
  • Updated ProfileModal.test.tsx with proper provider wrapping
  • All existing tests maintained/updated correctly

4. Accessibility-First Design

  • OpenDyslexic font specifically designed for dyslexic readers (self-hosted for privacy/reliability)
  • Smart exclusions: code blocks, monospace elements, and KaTeX math notation preserved
  • 125% font scaling for vision accessibility
  • Bold text option for readability

5. Performance Considerations

  • Fonts use font-display: swap to prevent FOIT (Flash of Invisible Text)
  • Settings applied to <html> element for efficient CSS cascade
  • Minimal re-renders with proper useEffect dependencies

📝 Code Quality Observations

Minor: CSS Specificity

The CSS selectors are quite verbose and use !important extensively:

html.font-dyslexic body,
html.font-dyslexic p,
html.font-dyslexic span:not(.katex):not(.katex-html):not(.katex *),
/* ... many more selectors */

Why this works here: The !important is necessary to override Tailwind utilities, and the verbose selectors ensure proper exclusions. However, this could be fragile if new components need exclusions.

Potential improvement (future): Consider adding a .accessibility-exclude class for elements that should always use default fonts, reducing selector complexity:

html.font-dyslexic *:not(.accessibility-exclude):not(pre):not(code):not(.katex *) {
  font-family: 'OpenDyslexic', sans-serif !important;
}

Minor: localStorage Error Handling

The localStorage error handling is good, but silent failures could confuse users:

try {
  localStorage.setItem(STORAGE_KEY_FONT, font);
} catch {
  // localStorage may be unavailable
}

Suggestion: Consider showing a toast notification when settings fail to persist, so users know their preferences won't survive a refresh.

Good: Delete Account UX Improvement

Moving delete account from a separate view to an inline confirmation dialog is a UX improvement. The previous implementation required navigating to a "danger zone" view, which was unnecessarily complex. The new approach with AlertDialog is more standard and less cognitive load.

🔒 Security Review

✅ No Security Issues Found

  • No XSS risk (all user input is boolean/enum, not freeform text)
  • localStorage keys are scoped with accessibility- prefix (good namespace hygiene)
  • Font files are self-hosted (no third-party CDN risk)
  • Input validation prevents injection of invalid values

🎯 Best Practices Alignment

Follows CLAUDE.md Conventions:

  • ✅ Uses semantic tokens (not direct colors)
  • ✅ Const arrow functions for components
  • ✅ Colocated tests with implementation
  • ✅ TanStack Query pattern (though not needed here)
  • ✅ No over-engineering - focused on requirements
  • ✅ No unnecessary docstrings/comments added
  • ✅ Tests focus on user behavior, not implementation details

React/TypeScript Best Practices:

  • ✅ Proper hook dependency arrays
  • ✅ Custom hook pattern with provider
  • ✅ Lazy initialization for expensive localStorage reads
  • ✅ Error boundaries via try-catch
  • ✅ Accessibility labels on all form controls

🧪 Test Coverage

Coverage is excellent:

  • 18 new tests for useAccessibility
  • All ProfileModal tests updated and passing
  • Edge cases covered (localStorage unavailable, invalid values)
  • Integration with provider tested

Test quality highlights:

  • Uses data-testid appropriately for test selectors
  • Tests user behavior, not implementation
  • Proper async handling with waitFor
  • Clean setup/teardown with beforeEach/afterEach

🐛 Potential Issues

None Critical, but worth noting:

  1. Font Loading Performance: OpenDyslexic fonts (OTF format) might be larger than WOFF2. Consider converting to WOFF2 for better compression:

    # Future optimization
    woff2_compress OpenDyslexic-Regular.otf
  2. Mobile Font Scaling: The 125% scaling applies to the <html> element, which might cause layout issues on small screens. Consider testing on mobile devices to ensure nothing breaks.

  3. KaTeX Exclusion Complexity: The :not(.katex):not(.katex-html):not(.katex *) selectors are necessary but complex. If KaTeX updates its class structure, this could break. Consider documenting this dependency.

📊 Performance Impact

Minimal performance impact:

  • CSS changes are applied via class toggles (cheap)
  • No JavaScript font loading APIs used
  • Settings read once on mount, written only on change
  • No network requests after initial font load

✨ Recommendations

  1. Consider adding to the PR:

    • Add a brief note in TESTING.md about accessibility settings testing
    • Update CLAUDE.md if you want to document the accessibility system
  2. Future enhancements (not blocking):

    • Add a "Reset to defaults" button in accessibility settings
    • Consider high-contrast mode as another accessibility option
    • Add prefers-reduced-motion detection (already in CSS, could surface in UI)

🎉 Summary

Recommendation: APPROVE ✅

This is a well-implemented accessibility feature with:

  • Strong architecture and type safety
  • Comprehensive test coverage
  • No security concerns
  • Follows all project conventions
  • Thoughtful UX decisions (flash prevention, smart exclusions)
  • Good performance characteristics

The implementation demonstrates attention to detail (flash prevention script, KaTeX exclusions, localStorage validation) and respect for users with accessibility needs. The reorganization of the delete account flow is also a welcome UX improvement.

Minor suggestions above are for future consideration, not blockers.

Great work! 🚀

@sonyccd sonyccd merged commit 629ef6d into main Jan 15, 2026
14 checks passed
@sonyccd sonyccd deleted the feature/accessibility-settings branch January 15, 2026 02:09
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