UI/theme#51
Conversation
|
contains redundant artifacts, duplicated logic, and a few quality issues that should be cleaned up before merge.
|
- Remove generation artifact (IMPLEMENTATION_COMPLETE.md) - Consolidate duplicate theme checks in settings page - Simplify theme precedence logic (localStorage > backend) - Remove all console.log statements from theme code - Add debounced auto-save (500ms) to prevent API spam - Centralize theme API with subscriptions and constants - Improve error handling (silent fails for localStorage) - Guard all localStorage access for SSR compatibility Addresses all feedback from @scrrlt review: - No more redundant theme logic - Single source of truth for theme operations - Proper error handling without console noise - Debounced network calls for better performance
|
@scrrlt Thank you for the thorough code review! I've addressed all the issues you identified. Here's what was fixed: Issues ResolvedRemoved Generation Artifact
Consolidated Duplicate Logic
Fixed Theme State Sync
Removed Console Noise
Centralized Theme API
Debounced Backend Saves
Improved Error Handling
Latest CommitAll changes committed in: |
There was a problem hiding this comment.
Pull request overview
This PR enhances the theme persistence system and settings page UX in the DeepTutor web application. It introduces a comprehensive theme management system with client-side persistence, prevents theme flash on page load, and improves the visibility of the save button by moving it to a sticky header position.
- Implements robust theme persistence with localStorage-first priority and backend sync
- Adds pre-hydration theme script to eliminate white flash on page load
- Moves settings save button to sticky top position for better discoverability
- Introduces auto-save functionality for theme changes
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| web/lib/theme.ts | New centralized theme utilities with localStorage handling and system preference detection |
| web/lib/theme-utils.ts | New helper functions for theme operations (toggle, get classes, watch changes) |
| web/lib/debounce.ts | New debounce utility for delayed function execution |
| web/hooks/useTheme.ts | New React hook for theme management across components |
| web/components/ThemeScript.tsx | New component that applies theme before React hydration to prevent flash |
| web/app/layout.tsx | Integrates ThemeScript into the root layout |
| web/app/settings/page.tsx | Updates settings page with sticky save button, auto-save theme, and localStorage priority |
| web/context/GlobalContext.tsx | Updates global context to use new theme utilities and prioritize localStorage |
| config/main.yaml | Formatting change for YAML list items (unrelated to theme changes) |
Comments suppressed due to low confidence (2)
web/app/settings/page.tsx:251
- The useEffect has
uiSettingsin its dependency array, but the effect callsfetchSettings()which can indirectly updateuiSettingsthrough the global context'srefreshSettings()function. This creates a potential infinite loop where settings fetch triggers a state update, which triggers another settings fetch. Consider removinguiSettingsfrom the dependency array or restructuring the logic to avoid this circular dependency.
useEffect(() => {
fetchSettings();
fetchSettings();
fetchEnvConfig();
if (activeTab === "llm_providers") {
fetchProviders();
}
}, [uiSettings, activeTab]);
web/app/settings/page.tsx:748
- The "System Settings" heading appears twice: once in the sticky header at the top (line 715) and again in the main content area (line 744-748). This creates a redundant and confusing user experience. Consider removing one of these headings or restructuring the layout so the sticky header doesn't duplicate the main heading.
{/* Sticky Save Button at Top */}
<div className="sticky top-0 z-50 bg-white dark:bg-slate-900 border-b border-slate-200 dark:border-slate-700 shadow-md">
<div className="max-w-4xl mx-auto p-6 flex items-center justify-between">
<div>
<h1 className="text-2xl font-bold text-slate-900 dark:text-slate-100">System Settings</h1>
</div>
<button
onClick={handleSave}
disabled={saving}
className={`py-2 px-6 rounded-lg font-medium flex items-center gap-2 transition-all ${
saving
? "bg-slate-100 dark:bg-slate-700 text-slate-400 dark:text-slate-500"
: saveSuccess
? "bg-green-500 text-white"
: "bg-blue-600 text-white hover:bg-blue-700"
}`}
>
{saving ? (
<Loader2 className="w-4 h-4 animate-spin" />
) : saveSuccess ? (
<Check className="w-4 h-4" />
) : (
<Save className="w-4 h-4" />
)}
{saving ? t("Saving...") : saveSuccess ? t("Saved") : t("Save All Changes")}
</button>
</div>
</div>
<div className="max-w-4xl mx-auto p-8">
{/* Header */}
<div className="flex items-center justify-between mb-6">
<div>
<h1 className="text-2xl font-bold text-slate-900 dark:text-slate-100 flex items-center gap-3">
<div className="p-2 bg-blue-50 dark:bg-blue-900/30 rounded-xl">
<SettingsIcon className="w-6 h-6 text-blue-600 dark:text-blue-400" />
</div>
{t("System Settings")}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const storedTheme = localStorage.getItem('deeptutor-theme'); | ||
| if (storedTheme === 'light' || storedTheme === 'dark') { | ||
| uiData.theme = storedTheme; |
There was a problem hiding this comment.
Direct localStorage access bypasses the centralized theme utilities. Use getStoredTheme() from '@/lib/theme' instead of directly accessing localStorage. This ensures consistency with the rest of the codebase and proper error handling.
| export function onThemeChange(callback: (theme: Theme) => void): () => void { | ||
| const handleStorageChange = (e: StorageEvent) => { | ||
| if (e.key === "deeptutor-theme" && (e.newValue === "light" || e.newValue === "dark")) { | ||
| callback(e.newValue); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("storage", handleStorageChange); | ||
|
|
||
| // Return cleanup function | ||
| return () => { | ||
| window.removeEventListener("storage", handleStorageChange); | ||
| }; | ||
| } |
There was a problem hiding this comment.
The onThemeChange function relies on the storage event, which only fires when localStorage is modified in a different tab/window, not in the current one. This means theme changes made in the same tab won't trigger the callback. Consider using the existing subscribeToThemeChanges function from '@/lib/theme' instead, which uses an internal event system that works within the same tab, or document this limitation in the function's JSDoc comment.
| allowed_roots: | ||
| - ./data/user | ||
| - ./src/tools | ||
| - ./data/user | ||
| - ./src/tools |
There was a problem hiding this comment.
The indentation format for the YAML list has changed from inline format (key: [item1, item2]) to block format (key:\n - item1\n - item2). While both are valid YAML, this change appears unrelated to the theme improvements described in the PR. If this is an intentional formatting change, it should be mentioned in the PR description. Otherwise, consider reverting this incidental change to keep the PR focused on its stated purpose.
| "use client"; | ||
|
|
There was a problem hiding this comment.
The "use client" directive is unnecessary here since this component only renders a static script tag with inline JavaScript. The component doesn't use any client-side React hooks or browser APIs. Remove the "use client" directive to allow this to be a server component, which is more appropriate for script injection during SSR.
| "use client"; |
| return theme === "dark" | ||
| ? "text-slate-100 dark:text-slate-100" | ||
| : "text-slate-900 dark:text-slate-900"; |
There was a problem hiding this comment.
The function returns Tailwind classes that are redundant. The class string includes both the base class and the dark variant of the same value (e.g., "text-slate-100 dark:text-slate-100"). The dark variant is unnecessary since both light and dark modes use the same value. Simplify to return just "text-slate-100" for dark theme and "text-slate-900" for light theme without the duplicate dark: prefix.
| return theme === "dark" | |
| ? "text-slate-100 dark:text-slate-100" | |
| : "text-slate-900 dark:text-slate-900"; | |
| return theme === "dark" ? "text-slate-100" : "text-slate-900"; |
| // Then fetch from backend and sync | ||
| refreshSettings(); | ||
| } | ||
| }, [isInitialized]); |
There was a problem hiding this comment.
The useEffect has isInitialized in its dependency array, but the effect sets this state variable. This creates an unnecessary dependency since after the first render when isInitialized is false, it will be set to true and the effect should never run again. Remove isInitialized from the dependency array and rely on the condition check inside the effect, or simplify to an empty dependency array with the initialization logic only.
| }, [isInitialized]); | |
| }, []); |
| * useTheme hook for managing theme throughout the application | ||
| */ | ||
| import { useEffect, useState } from "react"; | ||
| import { setTheme, getStoredTheme, initializeTheme, type Theme } from "@/lib/theme"; |
There was a problem hiding this comment.
Unused import getStoredTheme.
| import { setTheme, getStoredTheme, initializeTheme, type Theme } from "@/lib/theme"; | |
| import { setTheme, initializeTheme, type Theme } from "@/lib/theme"; |
Co-authored-by: Copilot <[email protected]>
Description
This PR improves the theme persistence system and settings UI/UX. Users can now toggle between light and dark themes with automatic backend synchronization, and the save button is more discoverable with a sticky top placement.
Changes Made
1. Robust Theme Persistence System
lib/theme.ts) - Single source of truth for all theme operations (get, set, apply)2. Auto-Save Theme to Backend
3. Improved Settings Page UX
4. Cleanup
Module(s) Affected
Files Changed
web/components/ThemeScript.tsx(new)web/lib/theme.ts(new)web/lib/theme-utils.ts(new)web/hooks/useTheme.ts(new)web/app/layout.tsx(updated)web/app/settings/page.tsx(updated)web/context/GlobalContext.tsx(updated)Checklist
pre-commit run --all-filesTesting
To verify theme persistence:
Previous behavior: Theme would revert to light after refresh because the save button at the bottom was missed/easy to overlook.
Additional Notes
The sticky save button placement addresses a UX issue where users would make settings changes but forget to save because the button wasn't visible. Theme changes now auto-save, making the experience seamless while other settings still require manual save via the top button.