feat: Add theme switching and group client settings#72
Conversation
Add theme selector Dark/Light in settings page Group Mouse Sensitivity, Invert Scroll, and Theme under 'Client Settings' section Persist theme to localStorage (rein-theme)
📝 WalkthroughWalkthroughAdds client-side theme support: a ThemeInit component applies persisted theme at app startup and the Settings page gains a "Client Settings" block with a theme selector that persists and applies the chosen theme via localStorage and document data-theme. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage as Settings Page
participant RootInit as ThemeInit (Root)
participant State as React State
participant Storage as localStorage
participant DOM as document.documentElement
Note over RootInit,Storage: App startup
RootInit->>Storage: read APP_CONFIG.THEME_STORAGE_KEY
Storage-->>RootInit: storedTheme
RootInit->>DOM: set data-theme (validated or default)
User->>SettingsPage: choose theme from dropdown
SettingsPage->>State: setTheme(selected)
State->>Storage: write APP_CONFIG.THEME_STORAGE_KEY
State->>DOM: set data-theme to selected
DOM->>DOM: apply theme styles
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/routes/settings.tsx`:
- Around line 234-238: The helper label currently rendering the invertScroll
helper text should be changed to a non-form label element (e.g., a <span> or
<p>) so it isn't treated as a form label, and the Theme label must be associated
with its control by adding htmlFor on the label and a matching id on the
<select> element (update the Theme label and the corresponding <select> in the
settings component, referencing the invertScroll helper text and the theme
<select> control by their existing JSX locations/names) — replace the decorative
<label> for invertScroll with a <span> or <p>, and add id="theme" (or another
unique id) to the <select> and htmlFor="theme" to the Theme <label>.
🧹 Nitpick comments (2)
src/routes/__root.tsx (1)
28-36: Theme may flash (FOUC) on page load.
useEffectruns after the first paint, so users may briefly see the default/unstyled theme beforedata-themeis applied. For a seamless experience, consider moving the theme initialization to a synchronous inline<script>in the<head>(before any body content renders) that readslocalStorageand setsdata-themeon<html>.That said, this is a minor UX polish item and fine to defer.
src/routes/settings.tsx (1)
15-30: Consider centralizing localStorage keys inAPP_CONFIG.
'rein_invert'(line 18) and'rein_sensitivity'(line 27) are hardcoded strings, while the theme key usesAPP_CONFIG.THEME_STORAGE_KEY. For consistency and easier refactoring, consider moving all localStorage keys intoAPP_CONFIG.
| <label className="label"> | ||
| <span className="label-text-alt opacity-50"> | ||
| {invertScroll ? 'Traditional scrolling enabled' : 'Natural scrolling'} | ||
| </span> | ||
| </label> |
There was a problem hiding this comment.
Accessibility: labels not associated with their controls.
The static analysis tool correctly flags these two <label> elements. The helper-text label at line 234 and the "Theme" label at line 242 lack htmlFor attributes and don't wrap their associated controls.
For the Theme label, add htmlFor pointing to the <select>, and give the select an id. The helper-text label for Invert Scroll is purely decorative — consider using a <span> or <p> instead of <label>.
Proposed fix
- <label className="label">
+ <span className="label">
<span className="label-text-alt opacity-50">
{invertScroll ? 'Traditional scrolling enabled' : 'Natural scrolling'}
</span>
- </label>
+ </span>- <label className="label">
+ <label className="label" htmlFor="theme-select">
<span className="label-text">Theme</span>
</label>
<select
className="select select-bordered w-full"
+ id="theme-select"
value={theme}
onChange={(e) => setTheme(e.target.value)}
>Also applies to: 242-244
🧰 Tools
🪛 Biome (2.3.14)
[error] 234-238: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
🤖 Prompt for AI Agents
In `@src/routes/settings.tsx` around lines 234 - 238, The helper label currently
rendering the invertScroll helper text should be changed to a non-form label
element (e.g., a <span> or <p>) so it isn't treated as a form label, and the
Theme label must be associated with its control by adding htmlFor on the label
and a matching id on the <select> element (update the Theme label and the
corresponding <select> in the settings component, referencing the invertScroll
helper text and the theme <select> control by their existing JSX
locations/names) — replace the decorative <label> for invertScroll with a <span>
or <p>, and add id="theme" (or another unique id) to the <select> and
htmlFor="theme" to the Theme <label>.
|
hi @alishair7071 Changes looks good and great work, Please resolve the merge conflicts and apply suggested improvements. @imxade LGTM |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/settings.tsx (1)
138-209:⚠️ Potential issue | 🟡 MinorPort setting is visually grouped under "Client Settings" but it's a server setting.
The PR objective is to separate client-only settings from server settings. The "Port" form control (lines 198–209) and the "Save Config" button appear below the "Client Settings" heading, making them look like client settings. Consider moving the Port input and Save Config button above the divider (alongside the Server IP field), or adding a second "Server Settings" heading above the server-related controls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 138 - 209, The Port input (state/frontendPort, placeholder CONFIG.FRONTEND_PORT) and the "Save Config" button are currently under the "Client Settings" header; move these server-related controls out of that block by either relocating the Port input and the "Save Config" button next to the Server IP control above the divider or by inserting a new "Server Settings" heading above the Port input and button; update the JSX so the Port input (value={frontendPort} onChange={setFrontendPort}) and the button with label "Save Config" are grouped under the server section to visually separate client vs server settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/routes/settings.tsx`:
- Around line 138-209: The Port input (state/frontendPort, placeholder
CONFIG.FRONTEND_PORT) and the "Save Config" button are currently under the
"Client Settings" header; move these server-related controls out of that block
by either relocating the Port input and the "Save Config" button next to the
Server IP control above the divider or by inserting a new "Server Settings"
heading above the Port input and button; update the JSX so the Port input
(value={frontendPort} onChange={setFrontendPort}) and the button with label
"Save Config" are grouped under the server section to visually separate client
vs server settings.
---
Duplicate comments:
In `@src/routes/settings.tsx`:
- Around line 185-187: The Theme label in the settings UI is not linked to its
select control; update the label element in src/routes/settings.tsx by adding
htmlFor="theme-select" and add id="theme-select" to the corresponding <select>
element (the Theme label and its select control) so the <label> and <select> are
properly associated for accessibility.
|
I think make the touch area also white in light mode and dark in dark mode. |
Summary
Added theme switching functionality and grouped all client-side settings in a dedicated section below the Save Config button.
Fixes #70
Changes
Testing
Rein.Theme.Switching.mp4
Summary by CodeRabbit
New Features
Style