Dynamic theme switching feature#2318
Conversation
📝 WalkthroughWalkthroughAdds a reactive ChangesRuntime theme store
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant themeStore
participant ThemeProvider
Caller->>themeStore: setActiveTheme(themeInput)
themeStore->>ThemeProvider: resolve and set next theme
ThemeProvider-->>themeStore: notify theme update
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/tss/src/themeStore.ts (1)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
activeThemeNamegoes stale whenThemeProvider.setTheme()is called externally.The subscriber only syncs
theme, notactiveThemeName. If any code callsThemeProvider.setTheme()directly (bypassing the store),activeThemeNamewill hold an incorrect value. Consider documenting this limitation or tracking the source of updates so the name stays consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tss/src/themeStore.ts` around lines 18 - 20, Update the ThemeProvider.subscribe callback in themeStore so externally supplied themes also keep activeThemeName synchronized with theme, deriving the name from the new theme or tracking update origin as needed; alternatively document the limitation if synchronization cannot be guaranteed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tss/src/themeStore.ts`:
- Around line 36-38: Remove the theme property from the state update in the
theme-switching method, likely within the theme store’s setTheme implementation,
so it only sets activeThemeName before calling
ThemeProvider.setTheme(nextTheme); rely on the ThemeProvider subscriber to
update theme and notify subscribers once.
- Line 24: Initialize activeThemeName consistently with
ThemeProvider.getTheme(): use detectDark() to select defaultDark for dark-mode
systems and defaultLight otherwise, rather than hardcoding defaultDark. Update
the relevant initialization in the theme store while preserving the existing
theme state behavior.
---
Nitpick comments:
In `@packages/tss/src/themeStore.ts`:
- Around line 18-20: Update the ThemeProvider.subscribe callback in themeStore
so externally supplied themes also keep activeThemeName synchronized with theme,
deriving the name from the new theme or tracking update origin as needed;
alternatively document the limitation if synchronization cannot be guaranteed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc8fccdb-2e4b-4f39-b4af-845c65a20690
📒 Files selected for processing (2)
packages/tss/src/index.tspackages/tss/src/themeStore.ts
|
|
||
| return { | ||
| theme: ThemeProvider.getTheme(), | ||
| activeThemeName: 'defaultDark', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Initial activeThemeName is hardcoded and may mismatch the actual initial theme.
ThemeProvider.getTheme() returns defaultLight when detectDark() is false, but activeThemeName is always 'defaultDark'. This produces incorrect state on initialization for light-mode systems.
🔧 Proposed fix
+import { defaultDark } from './tokens.js';
import type { ThemeTokens } from './tokens.js';
@@
- activeThemeName: 'defaultDark',
+ activeThemeName: ThemeProvider.getTheme() === defaultDark ? 'defaultDark' : 'defaultLight',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| activeThemeName: 'defaultDark', | |
| import { defaultDark } from './tokens.js'; | |
| activeThemeName: ThemeProvider.getTheme() === defaultDark ? 'defaultDark' : 'defaultLight', |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/tss/src/themeStore.ts` at line 24, Initialize activeThemeName
consistently with ThemeProvider.getTheme(): use detectDark() to select
defaultDark for dark-mode systems and defaultLight otherwise, rather than
hardcoding defaultDark. Update the relevant initialization in the theme store
while preserving the existing theme state behavior.
| set({ activeThemeName: name, theme: nextTheme }); | ||
| // This triggers the global listeners bound in components | ||
| ThemeProvider.setTheme(nextTheme); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Redundant double state update causes double re-render.
set({ activeThemeName: name, theme: nextTheme }) updates theme in the store, then ThemeProvider.setTheme(nextTheme) synchronously triggers the subscriber at line 18 which calls set({ theme: newTheme }) again with the same value. All store subscribers are notified twice, triggering duplicate global re-renders for every theme switch.
Remove theme from the set call and let the ThemeProvider subscriber handle it.
♻️ Proposed fix
- set({ activeThemeName: name, theme: nextTheme });
+ set({ activeThemeName: name });
// This triggers the global listeners bound in components
ThemeProvider.setTheme(nextTheme);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set({ activeThemeName: name, theme: nextTheme }); | |
| // This triggers the global listeners bound in components | |
| ThemeProvider.setTheme(nextTheme); | |
| set({ activeThemeName: name }); | |
| // This triggers the global listeners bound in components | |
| ThemeProvider.setTheme(nextTheme); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/tss/src/themeStore.ts` around lines 36 - 38, Remove the theme
property from the state update in the theme-switching method, likely within the
theme store’s setTheme implementation, so it only sets activeThemeName before
calling ThemeProvider.setTheme(nextTheme); rely on the ThemeProvider subscriber
to update theme and notify subscribers once.
1 participant
Description
This PR introduces the
themeStorein@termuijs/tss. Previously, changing the active TSS dictionary at runtime required cumbersome imperative calls that didn't automatically re-render declarative JSX trees smoothly. By wrappingThemeProviderin a@termuijs/storereactive singleton, users can now swap themes usingthemeStore.setActiveTheme('dracula')natively!The store transparently maps named strings to their corresponding
ThemeTokenspalettes (e.g.catppuccin,tokyo-night) while ensuring the underlying layout engine globally recalculates any component styles relying on CSS-like variables (e.g.var(--bg)).Related Issue
Closes #2262
Which package(s)?
@termuijs/tssType of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/YOUR_PROFILE_ID_HEREScreenshots / Recordings (UI changes)
Notes for the Reviewer
The implementation uses
Object.assign()directly on thecreateStorehook so the user doesn't have touseStore()just to swap the theme; they can dothemeStore.setActiveTheme(...)purely procedurally from outside of the React-style render loops.Summary by CodeRabbit