Skip to content

fix(theme): prevent forced light mode switch after login#16221

Merged
charlesBochet merged 2 commits intotwentyhq:mainfrom
Anshgrover23:fix/theme
Dec 1, 2025
Merged

fix(theme): prevent forced light mode switch after login#16221
charlesBochet merged 2 commits intotwentyhq:mainfrom
Anshgrover23:fix/theme

Conversation

@Anshgrover23
Copy link
Copy Markdown
Contributor

Description

This pull request resolves an issue where the application switched to light mode after login even when the user’s system was set to dark mode. Before authentication the app correctly followed system preferences through CSS media queries, but that behavior broke once the user logged in.

Before

Screen.Recording.2025-12-01.at.9.59.46.PM.mov

After

after-video-dark.mov

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Dec 1, 2025

Greptile Overview

Greptile Summary

This PR fixes the theme switching issue where the application forced light mode after login even when the system was set to dark mode. The fix centralizes system color scheme resolution in BaseThemeProvider instead of UserThemeProviderEffect, and changes the default color scheme from 'Light' to 'System'.

Key Changes:

  • Moved system color scheme detection logic to BaseThemeProvider to ensure it's always active regardless of authentication state
  • Simplified UserThemeProviderEffect to pass through the user's color scheme preference without resolving 'System' to actual colors
  • Changed default color scheme fallback from 'Light' to 'System' in UserAndViewsProviderEffect

Issues Found:

  • DOM manipulation (document.documentElement.className) occurs during render phase in BaseThemeProvider, which violates React best practices and could cause issues with concurrent rendering

Confidence Score: 4/5

  • This PR is safe to merge with minor style concerns
  • The logic correctly fixes the theme switching bug by centralizing system color scheme resolution. The approach is sound and the default change from 'Light' to 'System' is appropriate. However, the DOM manipulation during render (rather than in useEffect) is a React best practice violation that should ideally be addressed, though it's unlikely to cause practical issues in this specific case.
  • The BaseThemeProvider.tsx file should have the DOM side effect wrapped in useEffect, but this is a style improvement rather than a blocking issue

Important Files Changed

File Analysis

Filename Score Overview
packages/twenty-front/src/modules/ui/theme/components/BaseThemeProvider.tsx 3/5 Moved system color scheme logic from UserThemeProviderEffect to BaseThemeProvider, but includes side effect (DOM manipulation) during render phase
packages/twenty-front/src/modules/ui/theme/components/UserThemeProviderEffect.tsx 5/5 Simplified by removing system color scheme resolution logic, now just passes through the color scheme value
packages/twenty-front/src/modules/users/components/UserAndViewsProviderEffect.tsx 5/5 Changed default color scheme from 'Light' to 'System' to respect system preferences

Sequence Diagram

sequenceDiagram
    participant App as AppRouterProviders
    participant Base as BaseThemeProvider
    participant System as useSystemColorScheme
    participant User as UserAndViewsProviderEffect
    participant Theme as UserThemeProviderEffect
    participant Recoil as persistedColorSchemeState

    Note over App,Recoil: Initial Load (Before Login)
    App->>Base: Render with BaseThemeProvider
    Base->>Recoil: Read persistedColorSchemeState (default: 'System')
    Base->>System: useSystemColorScheme()
    System-->>Base: Returns 'Dark' or 'Light' from media query
    Base->>Base: effectiveColorScheme = 'System' ? systemColorScheme : persistedColorScheme
    Base->>Base: Set document.documentElement.className
    Base->>Base: Select theme (THEME_DARK or THEME_LIGHT)

    Note over App,Recoil: After Login
    App->>User: Load user data
    User->>User: Query workspace member
    User->>User: Set default colorScheme to 'System' (line 154)
    User->>Recoil: Update currentWorkspaceMemberState
    
    App->>Theme: UserThemeProviderEffect renders
    Theme->>Theme: useColorScheme() returns user's colorScheme
    Theme->>Recoil: setPersistedColorScheme(colorScheme)
    
    Recoil->>Base: persistedColorSchemeState updated
    Base->>System: useSystemColorScheme() (if 'System')
    System-->>Base: Returns current system preference
    Base->>Base: Resolve effectiveColorScheme
    Base->>Base: Update document.documentElement.className
    Base->>Base: Apply correct theme
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 28 to +29
document.documentElement.className =
persistedColorScheme === 'Dark' ? 'dark' : 'light';
effectiveColorScheme === 'Dark' ? 'dark' : 'light';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: DOM manipulation during render phase violates React best practices. This side effect should be wrapped in a useEffect to avoid potential issues with concurrent rendering and StrictMode double-invocation.

Suggested change
document.documentElement.className =
persistedColorScheme === 'Dark' ? 'dark' : 'light';
effectiveColorScheme === 'Dark' ? 'dark' : 'light';
useEffect(() => {
document.documentElement.className =
effectiveColorScheme === 'Dark' ? 'dark' : 'light';
}, [effectiveColorScheme]);

Context Used: Context from dashboard - Use useEffect in EffectComponent or hooks only. Move effects to dedicated components when necess... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/ui/theme/components/BaseThemeProvider.tsx
Line: 28:29

Comment:
**style:** DOM manipulation during render phase violates React best practices. This side effect should be wrapped in a `useEffect` to avoid potential issues with concurrent rendering and StrictMode double-invocation.

```suggestion
  useEffect(() => {
    document.documentElement.className =
      effectiveColorScheme === 'Dark' ? 'dark' : 'light';
  }, [effectiveColorScheme]);
```

**Context Used:** Context from `dashboard` - Use `useEffect` in `EffectComponent` or hooks only. Move effects to dedicated components when necess... ([source](https://app.greptile.com/review/custom-context?memory=fb506a89-cfe0-41d0-96a8-c0cc5a883fcf))

How can I resolve this? If you propose a fix, please make it concise.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 1, 2025

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:10268

This environment will automatically shut down when the PR is closed or after 5 hours.

Copy link
Copy Markdown
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM indeed, ty!

@charlesBochet charlesBochet merged commit 2cdf5ae into twentyhq:main Dec 1, 2025
59 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 1, 2025

Thanks @Anshgrover23 for your contribution!
This marks your 3rd PR on the repo. You're top 10% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

NotYen pushed a commit to NotYen/twenty-ym that referenced this pull request Dec 4, 2025
)

This pull request resolves an issue where the application switched to
light mode after login even when the user’s system was set to dark mode.
Before authentication the app correctly followed system preferences
through CSS media queries, but that behavior broke once the user logged
in.

**Before**

https://github.com/user-attachments/assets/75e73712-9cb2-42f9-9e25-3a22dc911b8d

**After**

https://github.com/user-attachments/assets/6bba45ce-3817-4e3d-9b45-ff0c7725cf82
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