feat(web): standardize theme system with auto sync option

[boojack]

This commit addresses issue #5190 by implementing a standardized theme system following GitHub's approach and fixing theme persistence bugs.

Key changes:

• Add "Sync with system" theme option that auto-switches between light/dark
• Rename theme options: "Default Light" → "Light", "Default Dark" → "Dark"
• Fix theme persistence: changes now update instanceStore immediately
• Fix initialization: respect user's saved theme instead of hardcoding "default"
• Add OS preference listener: auto-update when system theme changes
• Set "system" as the default theme for new users

The theme system now offers:

• System (auto) - follows OS dark/light preference (NEW, default)
• Light - always light theme
• Dark - always dark theme
• Paper - custom light theme
• Whitewall - custom light theme

Technical improvements:

• Added resolveTheme() to handle "system" → actual theme resolution
• Added setupSystemThemeListener() for OS preference change detection
• Updated validation in instance store to include "system"
• Fixed PreferencesSection to immediately apply theme changes
• Updated ThemeSelect with Monitor icon for system theme

Fixes #5190

[claude]

Pull Request Review

This PR successfully addresses issue #5190 by implementing a comprehensive theme system with auto-sync functionality. The implementation is solid overall, but I have a few suggestions for improvement.

✅ Strengths

1. Well-structured solution: The PR properly separates concerns with resolveTheme() handling the "system" → actual theme resolution, and setupSystemThemeListener() managing OS preference changes.

2. Good UX improvements:

   • Immediate UI feedback via instanceStore.state.setPartial() in PreferencesSection.tsx:31
   • Clear theme naming ("Light"/"Dark" instead of "Default Light"/"Default Dark")
   • Appropriate Monitor icon for system theme

3. Backward compatibility: The validateTheme() function ensures invalid themes fall back gracefully to "default".

4. Proper cleanup: The setupSystemThemeListener() correctly returns a cleanup function used in the useEffect hook.

🐛 Potential Issues

1. Race condition in theme initialization (Medium Priority)

Location: web/src/App.tsx:69-86

The current implementation has two separate useEffect hooks that could race:

• Lines 69-78: Updates instanceStore.state.theme from userGeneralSetting
• Lines 81-86: Calls loadTheme() based on instanceStore.state.theme

If userGeneralSetting changes, there's a brief moment where the first effect hasn't updated the instance store yet, but the second effect might read the old value.

Suggestion: Consider merging these effects or using a more deterministic approach:

useEffect(() => {
  if (!userGeneralSetting) {
    return;
  }
  
  const newTheme = userGeneralSetting.theme || instanceStore.state.theme;
  instanceStore.state.setPartial({
    locale: userGeneralSetting.locale || instanceStore.state.locale,
    theme: newTheme,
  });
  loadTheme(newTheme);
}, [userGeneralSetting?.locale, userGeneralSetting?.theme]);

2. Missing TypeScript type in ThemeSelect (Low Priority)

Location: web/src/components/ThemeSelect.tsx:23

The parameter newTheme is typed as Theme, but Theme is imported from @/store/instance (not visible in this file). The function should handle any string since it accepts user input.

Suggestion: Change the type to string or import the Theme type explicitly.

3. Default theme validation inconsistency (Low Priority)

Location: web/src/store/instance.ts:93

When an invalid theme is detected, the code falls back to "default" instead of "system", which is inconsistent with the new default. This could cause confusion if a user somehow has an invalid theme stored.

Suggestion: Consider falling back to "system" to match the new default:

console.warn(`Invalid theme "${themeStr}", falling back to "system"`);
finalState.theme = "system";

🎯 Code Quality Observations

1. Good documentation: The JSDoc comments in theme.ts clearly explain each function's purpose.

2. Cross-browser compatibility: The setupSystemThemeListener() properly handles both modern (addEventListener) and legacy (addListener) APIs.

3. Defensive programming: Proper null checks and fallbacks throughout (e.g., typeof window !== "undefined", try-catch for localStorage).

🔒 Security Considerations

No security concerns identified. The code:

• Properly validates theme values before applying them
• Uses whitelisted theme options
• Doesn't execute user-provided code

⚡ Performance Considerations

1. LocalStorage access: The loadTheme() function writes to localStorage on every theme change. This is acceptable for theme changes but verify this doesn't cause issues if called frequently during system theme oscillation.

2. Multiple effects: Three separate useEffect hooks in App.tsx all depend on theme. Consider if they could be optimized, though the current implementation is reasonable.

🧪 Test Coverage

Missing: No tests are included for the new functionality. Consider adding tests for:

• resolveTheme() with various inputs
• setupSystemThemeListener() callback behavior
• Theme persistence across page reloads
• System theme change detection

📝 Minor Nitpicks

1. The comment on line web/src/utils/theme.ts:9 says "System theme dynamically chooses" but could be more specific: "System theme is resolved at runtime based on OS preference"

2. Consider extracting the magic string "memos-theme" to a constant for easier maintenance.

✅ Summary

This is a well-implemented PR that successfully addresses the reported issue. The theme system now properly:

• Detects and respects system theme preferences
• Persists theme changes correctly
• Updates dynamically when OS preferences change
• Provides clear user-facing options

Recommendation: Approve with minor suggested improvements. The race condition issue should be addressed, but it's not blocking since it would only manifest in edge cases.

Great work! 🎉