feat: add layered theme system for plugins#122
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…try store Introduce ThemeDefinition type, CORE_THEME_TOKENS whitelist, and validation helpers that reject unknown CSS tokens. Add ThemeRegistry Zustand vanilla store for registering/unregistering themes with active theme management. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t method Wire theme system into the app: useThemeOverrides applies active theme tokens to document root, and registerTheme lets plugins register full themes through the PluginContext with proper lifecycle cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire up Electron's nativeTheme API so the renderer can set the theme source (dark/light/system) and receive system theme change notifications via IPC instead of relying solely on window.matchMedia. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add activeThemeId to AppearanceSettings schema, plugin theme selector in AppearanceSection (shown only when plugin themes are registered), and startup restoration of the saved theme in App.tsx. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover isValidThemeToken, validateThemeTokens, and full themeRegistryStore lifecycle (register, unregister, unregisterAll, setActive, getActiveTheme). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive theme system enabling plugins to register custom themes and manage them through a centralized registry. It integrates Electron's native theme support with IPC-based synchronization, adds theme persistence to appearance settings, and updates the renderer UI to support theme selection alongside token validation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer
participant Main as Main Process
participant System as System Theme
User->>Renderer: Select theme in settings
Renderer->>Renderer: Call setActive(themeId)
Renderer->>Renderer: Apply theme tokens to DOM
Renderer->>Main: IPC: theme:set-source
Main->>Main: Set nativeTheme.themeSource
System-->>Main: System theme changes
Main->>Renderer: Broadcast: theme:system-changed
Renderer->>Renderer: Call onSystemChanged callback
Renderer->>Renderer: Apply system theme via applyAppearance()
Renderer->>Renderer: Update DOM with new theme tokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/preload/index.ts`:
- Around line 949-952: The implementation of theme.setSource uses a loose string
parameter; change its signature to setSource: (source: 'dark' | 'light' |
'system') => void to match the interface and provide compile-time safety,
leaving the ipcRenderer.send('theme:set-source', source) call unchanged; update
any local callers if they pass other strings to conform to the union type.
In `@apps/desktop/src/renderer/App.tsx`:
- Around line 87-96: The effect that restores the saved theme (useEffect
watching appearance?.activeThemeId) can miss restoring if plugins register
themes later; update the effect to also react to themeRegistryStore
changes—either by adding the themes array length (e.g.,
themeRegistryStore.getState().themes.length) to the dependency list or by
subscribing to themeRegistryStore inside the effect and re-checking
existence—then call themeRegistryStore.getState().setActive(savedThemeId) once
the savedThemeId appears in themeRegistryStore.getState().themes; ensure you
reference appearance?.activeThemeId, themeRegistryStore.getState().themes, and
setActive when implementing the fix.
In `@apps/desktop/src/renderer/hooks/useAppearanceSettings.ts`:
- Around line 8-18: When theme === 'system' the applyAppearance(theme,
accentColor, zoomLevel, isDark) function currently falls back to
window.matchMedia on re-applies and ignores the IPC-supplied nativeTheme;
persist the IPC value (e.g., stash the incoming isDark from preload/IPC into a
module-level variable like initialIsDark or expose a preload getter such as
getNativeThemeSnapshot) and update applyAppearance to prefer that persisted
snapshot when theme === 'system' instead of calling window.matchMedia so
subsequent accent/zoom updates continue to use the main-process nativeTheme
state.
- Around line 8-18: The applyAppearance function currently writes the resolved
light/dark state to document.documentElement via data-theme, which conflicts
with useThemeOverrides writing plugin theme values; change applyAppearance to
set a separate attribute (e.g.,
document.documentElement.setAttribute('data-color-scheme', resolved)) and update
any consumers that read data-theme to instead read data-color-scheme for
color-scheme behavior; alternatively, have useThemeOverrides write its
plugin-provided value to a distinct attribute like data-plugin-theme rather than
data-theme so plugin theme identity is not clobbered when applyAppearance (with
params theme, accentColor, zoomLevel) reruns.
In `@apps/desktop/src/renderer/pages/settings/sections/AppearanceSection.tsx`:
- Line 10: The import order is wrong per linting rules: move the external
package import "import { themeRegistryStore } from '@readied/plugin-api';" so it
appears before the relative imports from ../../../stores/settings (e.g., the
settings-related import used in AppearanceSection). Update the top of
AppearanceSection.tsx to place the `@readied/plugin-api` import above the relative
settings import and ensure your editor/formatter preserves the new order.
In `@docs/plans/2026-03-11-theme-system-implementation.md`:
- Line 13: The document jumps from an h1 to an h3 at the heading "Task 1: Define
theme types and token whitelist"; change that heading (and other task headings
at the same level) from ### to ## to restore proper heading hierarchy (e.g.,
update "### Task 1: Define theme types and token whitelist" to "## Task 1:
Define theme types and token whitelist" and do the same for subsequent "Task"
headings).
In `@packages/plugin-api/src/lifecycle/PluginRegistry.ts`:
- Around line 275-281: The registerTheme implementation ignores the boolean
return from themeRegistryStore.getState().register, so failures when a theme has
no valid tokens are swallowed; update registerTheme (the registerTheme arrow
function) to capture the result of themeRegistryStore.getState().register({... ,
pluginId: id}) and handle failures: either call processLogger.warn /
console.warn (or the existing logger) with context including id and theme.id
when register returns false, or change the API to return a tuple/object like {
success: boolean, dispose: () => void } so callers receive the boolean result
alongside the disposer; ensure you still call
themeRegistryStore.getState().unregister(theme.id) in the disposer regardless of
result.
In `@packages/plugin-api/src/theme/themeRegistryStore.ts`:
- Around line 36-38: The log call in themeRegistryStore.ts uses console.debug
which is disallowed by static analysis; replace the console.debug call that logs
`[ThemeRegistry] Registered: "${theme.name}" (${Object.keys(validTokens).length}
tokens)` with an allowed method (e.g., console.warn or console.error) or remove
the log entirely for production builds—locate the statement that references
theme.name and validTokens in the theme registration code and update or strip
the logging accordingly.
In `@packages/plugin-api/src/theme/useThemeOverrides.ts`:
- Around line 12-15: getSnapshot currently returns a fresh object every call
causing useSyncExternalStore to always detect a changed snapshot; modify
getSnapshot (used with useSyncExternalStore and themeRegistryStore) to memoize
the returned value by caching the last activeThemeId and themes and only
create/return a new object when either activeThemeId or themes differs from the
cached values (or alternatively return a primitive/tuple like [activeThemeId,
themes] or just activeThemeId if only that is needed) so the reference remains
stable across unchanged state and prevents unnecessary re-renders.
In `@packages/storage-sqlite/src/repositories/SQLiteNoteRepository.ts`:
- Around line 849-854: Prettier formatting failed across this file; run the
formatter and fix formatting issues (especially the method signature and any
broken method chains around getTagsPendingSync in the SQLiteNoteRepository
class) by executing the project's formatting commands (e.g., pnpm format:check
to see failures and prettier --write to apply fixes) so the file conforms to the
repo Prettier rules and CI passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 062aef14-839b-45ea-b848-39a2fd3c2432
📒 Files selected for processing (18)
apps/desktop/src/main/index.tsapps/desktop/src/main/services/syncService.tsapps/desktop/src/preload/index.tsapps/desktop/src/renderer/App.tsxapps/desktop/src/renderer/hooks/useAppearanceSettings.tsapps/desktop/src/renderer/pages/settings/sections/AppearanceSection.tsxapps/desktop/src/renderer/stores/settings/schema.tsdocs/plans/2026-03-11-theme-system-implementation.mdpackages/api/src/routes/sync.tspackages/plugin-api/src/index.tspackages/plugin-api/src/lifecycle/PluginRegistry.tspackages/plugin-api/src/theme/themeRegistryStore.tspackages/plugin-api/src/theme/themeTypes.tspackages/plugin-api/src/theme/useThemeOverrides.tspackages/plugin-api/src/types.tspackages/plugin-api/tests/themeRegistryStore.test.tspackages/plugin-api/tests/themeTypes.test.tspackages/storage-sqlite/src/repositories/SQLiteNoteRepository.ts
| theme: { | ||
| setSource: (source: string) => { | ||
| ipcRenderer.send('theme:set-source', source); | ||
| }, |
There was a problem hiding this comment.
Type mismatch between interface declaration and implementation.
The interface declares setSource: (source: 'dark' | 'light' | 'system') => void (line 690), but the implementation uses (source: string). While the main process validates the value, enforcing the type in the preload provides better type safety.
🔧 Proposed fix
theme: {
- setSource: (source: string) => {
+ setSource: (source: 'dark' | 'light' | 'system') => {
ipcRenderer.send('theme:set-source', source);
},📝 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.
| theme: { | |
| setSource: (source: string) => { | |
| ipcRenderer.send('theme:set-source', source); | |
| }, | |
| theme: { | |
| setSource: (source: 'dark' | 'light' | 'system') => { | |
| ipcRenderer.send('theme:set-source', source); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/preload/index.ts` around lines 949 - 952, The implementation
of theme.setSource uses a loose string parameter; change its signature to
setSource: (source: 'dark' | 'light' | 'system') => void to match the interface
and provide compile-time safety, leaving the
ipcRenderer.send('theme:set-source', source) call unchanged; update any local
callers if they pass other strings to conform to the union type.
| useEffect(() => { | ||
| const savedThemeId = appearance?.activeThemeId; | ||
| if (savedThemeId) { | ||
| // Only restore if the theme is actually registered (plugin loaded) | ||
| const exists = themeRegistryStore.getState().themes.some(t => t.id === savedThemeId); | ||
| if (exists) { | ||
| themeRegistryStore.getState().setActive(savedThemeId); | ||
| } | ||
| } | ||
| }, [appearance?.activeThemeId]); |
There was a problem hiding this comment.
Theme restoration may fail due to race condition with plugin loading.
This effect runs when activeThemeId changes, but it doesn't re-run when plugins register their themes. If the component mounts before plugins finish activating, the exists check (line 91) will fail because themes is still empty, and the saved theme will never be restored.
The dependency array [appearance?.activeThemeId] doesn't include the themes array, so even when a plugin registers the theme later, this effect won't re-execute.
Consider subscribing to themeRegistryStore changes or adding themes.length to dependencies:
🔧 Suggested approach: Subscribe to registry changes
+import { useSyncExternalStore } from 'react';
// Restore saved plugin theme on startup
const appearance = useSettingsStore(selectAppearance);
+const registeredThemes = useSyncExternalStore(
+ themeRegistryStore.subscribe,
+ () => themeRegistryStore.getState().themes
+);
useEffect(() => {
const savedThemeId = appearance?.activeThemeId;
if (savedThemeId) {
// Only restore if the theme is actually registered (plugin loaded)
- const exists = themeRegistryStore.getState().themes.some(t => t.id === savedThemeId);
+ const exists = registeredThemes.some(t => t.id === savedThemeId);
if (exists) {
themeRegistryStore.getState().setActive(savedThemeId);
}
}
-}, [appearance?.activeThemeId]);
+}, [appearance?.activeThemeId, registeredThemes]);This ensures the effect re-runs when plugins register themes, allowing proper restoration once the theme becomes available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/App.tsx` around lines 87 - 96, The effect that
restores the saved theme (useEffect watching appearance?.activeThemeId) can miss
restoring if plugins register themes later; update the effect to also react to
themeRegistryStore changes—either by adding the themes array length (e.g.,
themeRegistryStore.getState().themes.length) to the dependency list or by
subscribing to themeRegistryStore inside the effect and re-checking
existence—then call themeRegistryStore.getState().setActive(savedThemeId) once
the savedThemeId appears in themeRegistryStore.getState().themes; ensure you
reference appearance?.activeThemeId, themeRegistryStore.getState().themes, and
setActive when implementing the fix.
| function applyAppearance( | ||
| theme: string, | ||
| accentColor: string, | ||
| zoomLevel: string, | ||
| isDark?: boolean | ||
| ): void { | ||
| let resolved: string; | ||
| if (theme === 'system') { | ||
| return window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; | ||
| resolved = | ||
| (isDark ?? window.matchMedia('(prefers-color-scheme: dark)').matches) ? 'dark' : 'light'; | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the theme bridge exposes a current system-theme value
# or only future change notifications.
rg -n -C3 "setSource|onSystemChanged|theme:system-changed|shouldUseDarkColors|themeSource" \
apps/desktop/src/main/index.ts \
apps/desktop/src/preload/index.ts \
apps/desktop/src/renderer/hooks/useAppearanceSettings.tsRepository: tomymaritano/readide
Length of output: 4031
🏁 Script executed:
cat -n apps/desktop/src/renderer/hooks/useAppearanceSettings.tsRepository: tomymaritano/readide
Length of output: 2881
Keep 'system' mode on a single source of truth.
Line 58 re-applies appearance without the IPC-supplied isDark whenever accent or zoom changes. When theme='system', this causes line 17 to fall back to window.matchMedia instead of using the main-process nativeTheme state. Persist the IPC value locally or expose an initial getter/snapshot from preload to avoid diverging from the main process state during updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/hooks/useAppearanceSettings.ts` around lines 8 -
18, When theme === 'system' the applyAppearance(theme, accentColor, zoomLevel,
isDark) function currently falls back to window.matchMedia on re-applies and
ignores the IPC-supplied nativeTheme; persist the IPC value (e.g., stash the
incoming isDark from preload/IPC into a module-level variable like initialIsDark
or expose a preload getter such as getNativeThemeSnapshot) and update
applyAppearance to prefer that persisted snapshot when theme === 'system'
instead of calling window.matchMedia so subsequent accent/zoom updates continue
to use the main-process nativeTheme state.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all writers/readers of the root `data-theme` attribute.
rg -n -C2 "setAttribute\\(['\"]data-theme['\"]|dataset\\.theme\\s*=|\\[data-theme"Repository: tomymaritano/readide
Length of output: 9107
🏁 Script executed:
# Check what values theme.colorScheme can have and understand the plugin theme layer
cat -n packages/plugin-api/src/theme/useThemeOverrides.ts | head -40Repository: tomymaritano/readide
Length of output: 1598
🏁 Script executed:
# Inspect useAppearanceSettings hook dependencies and full implementation
cat -n apps/desktop/src/renderer/hooks/useAppearanceSettings.tsRepository: tomymaritano/readide
Length of output: 2881
🏁 Script executed:
# Check useTheme.ts to see if there's a relationship
cat -n apps/desktop/src/renderer/hooks/useTheme.ts | head -40Repository: tomymaritano/readide
Length of output: 1464
Separate the color-scheme attribute from the plugin-theme attribute.
Both useAppearanceSettings.ts (line 21) and useThemeOverrides.ts (line 27) write to documentElement[data-theme]. The first sets it to 'dark' | 'light' and reruns whenever appearance settings change (theme, accent color, or zoom level). The second sets it to a plugin-provided theme.colorScheme. Because applyAppearance has [theme, accentColor, zoomLevel] as dependencies, changes to zoom or accent color will unconditionally overwrite the plugin theme value, losing the plugin-supplied theme state.
Use distinct attributes such as data-color-scheme and data-plugin-theme, or keep plugin theme identity out of data-theme.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/hooks/useAppearanceSettings.ts` around lines 8 -
18, The applyAppearance function currently writes the resolved light/dark state
to document.documentElement via data-theme, which conflicts with
useThemeOverrides writing plugin theme values; change applyAppearance to set a
separate attribute (e.g.,
document.documentElement.setAttribute('data-color-scheme', resolved)) and update
any consumers that read data-theme to instead read data-color-scheme for
color-scheme behavior; alternatively, have useThemeOverrides write its
plugin-provided value to a distinct attribute like data-plugin-theme rather than
data-theme so plugin theme identity is not clobbered when applyAppearance (with
params theme, accentColor, zoomLevel) reruns.
| import { useSyncExternalStore } from 'react'; | ||
| import { useSettingsStore, selectAppearance } from '../../../stores/settings'; | ||
| import { usePerformanceStore } from '../../../stores/performanceStore'; | ||
| import { themeRegistryStore } from '@readied/plugin-api'; |
There was a problem hiding this comment.
Fix import order per linting rules.
Static analysis indicates @readied/plugin-api import should occur before relative imports from ../../../stores/settings.
🔧 Proposed fix
import { useSyncExternalStore } from 'react';
+import { themeRegistryStore } from '@readied/plugin-api';
import { useSettingsStore, selectAppearance } from '../../../stores/settings';
import { usePerformanceStore } from '../../../stores/performanceStore';
-import { themeRegistryStore } from '@readied/plugin-api';
import { SettingGroup } from '../components/SettingGroup';📝 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.
| import { themeRegistryStore } from '@readied/plugin-api'; | |
| import { useSyncExternalStore } from 'react'; | |
| import { themeRegistryStore } from '@readied/plugin-api'; | |
| import { useSettingsStore, selectAppearance } from '../../../stores/settings'; | |
| import { usePerformanceStore } from '../../../stores/performanceStore'; | |
| import { SettingGroup } from '../components/SettingGroup'; |
🧰 Tools
🪛 GitHub Check: lint
[warning] 10-10:
@readied/plugin-api import should occur before import of ../../../stores/settings
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/pages/settings/sections/AppearanceSection.tsx` at
line 10, The import order is wrong per linting rules: move the external package
import "import { themeRegistryStore } from '@readied/plugin-api';" so it appears
before the relative imports from ../../../stores/settings (e.g., the
settings-related import used in AppearanceSection). Update the top of
AppearanceSection.tsx to place the `@readied/plugin-api` import above the relative
settings import and ensure your editor/formatter preserves the new order.
|
|
||
| --- | ||
|
|
||
| ### Task 1: Define theme types and token whitelist |
There was a problem hiding this comment.
Fix heading hierarchy for markdown compliance.
Static analysis indicates the heading jumps from # (h1) directly to ### (h3), skipping h2. Consider using ## for "Task 1" and subsequent task headings to maintain proper document structure.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-11-theme-system-implementation.md` at line 13, The
document jumps from an h1 to an h3 at the heading "Task 1: Define theme types
and token whitelist"; change that heading (and other task headings at the same
level) from ### to ## to restore proper heading hierarchy (e.g., update "###
Task 1: Define theme types and token whitelist" to "## Task 1: Define theme
types and token whitelist" and do the same for subsequent "Task" headings).
| registerTheme: (theme): (() => void) => { | ||
| themeRegistryStore.getState().register({ | ||
| ...theme, | ||
| pluginId: id, | ||
| }); | ||
| return () => themeRegistryStore.getState().unregister(theme.id); | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The register() return value is ignored.
themeRegistryStore.register() returns false if the theme has no valid tokens (see themeRegistryStore.ts lines 27-31). The current implementation doesn't propagate this failure to the plugin.
Consider either:
- Logging a warning if registration fails
- Returning a result indicator alongside the disposer
♻️ Proposed enhancement to log registration failures
registerTheme: (theme): (() => void) => {
- themeRegistryStore.getState().register({
+ const success = themeRegistryStore.getState().register({
...theme,
pluginId: id,
});
+ if (!success) {
+ console.warn(`[${id}] Theme registration failed for "${theme.id}" (no valid tokens)`);
+ }
return () => themeRegistryStore.getState().unregister(theme.id);
},📝 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.
| registerTheme: (theme): (() => void) => { | |
| themeRegistryStore.getState().register({ | |
| ...theme, | |
| pluginId: id, | |
| }); | |
| return () => themeRegistryStore.getState().unregister(theme.id); | |
| }, | |
| registerTheme: (theme): (() => void) => { | |
| const success = themeRegistryStore.getState().register({ | |
| ...theme, | |
| pluginId: id, | |
| }); | |
| if (!success) { | |
| console.warn(`[${id}] Theme registration failed for "${theme.id}" (no valid tokens)`); | |
| } | |
| return () => themeRegistryStore.getState().unregister(theme.id); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/plugin-api/src/lifecycle/PluginRegistry.ts` around lines 275 - 281,
The registerTheme implementation ignores the boolean return from
themeRegistryStore.getState().register, so failures when a theme has no valid
tokens are swallowed; update registerTheme (the registerTheme arrow function) to
capture the result of themeRegistryStore.getState().register({... , pluginId:
id}) and handle failures: either call processLogger.warn / console.warn (or the
existing logger) with context including id and theme.id when register returns
false, or change the API to return a tuple/object like { success: boolean,
dispose: () => void } so callers receive the boolean result alongside the
disposer; ensure you still call
themeRegistryStore.getState().unregister(theme.id) in the disposer regardless of
result.
| console.debug( | ||
| `[ThemeRegistry] Registered: "${theme.name}" (${Object.keys(validTokens).length} tokens)` | ||
| ); |
There was a problem hiding this comment.
Replace console.debug with an allowed console method.
Static analysis indicates only console.warn and console.error are allowed. Consider using console.info or removing the debug log in production builds.
🔧 Proposed fix
- console.debug(
+ console.info(
`[ThemeRegistry] Registered: "${theme.name}" (${Object.keys(validTokens).length} tokens)`
);Or remove the log entirely if it's only useful during development.
🧰 Tools
🪛 GitHub Check: lint
[warning] 36-36:
Unexpected console statement. Only these console methods are allowed: warn, error
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/plugin-api/src/theme/themeRegistryStore.ts` around lines 36 - 38,
The log call in themeRegistryStore.ts uses console.debug which is disallowed by
static analysis; replace the console.debug call that logs `[ThemeRegistry]
Registered: "${theme.name}" (${Object.keys(validTokens).length} tokens)` with an
allowed method (e.g., console.warn or console.error) or remove the log entirely
for production builds—locate the statement that references theme.name and
validTokens in the theme registration code and update or strip the logging
accordingly.
| const getSnapshot = () => ({ | ||
| activeThemeId: themeRegistryStore.getState().activeThemeId, | ||
| themes: themeRegistryStore.getState().themes, | ||
| }); |
There was a problem hiding this comment.
getSnapshot creates new object references, causing unnecessary re-renders.
useSyncExternalStore compares snapshots by reference. Since getSnapshot creates a new object on every call, the hook will re-render on every store notification regardless of whether activeThemeId or themes actually changed.
🐛 Proposed fix: memoize or use shallow comparison
-const getSnapshot = () => ({
- activeThemeId: themeRegistryStore.getState().activeThemeId,
- themes: themeRegistryStore.getState().themes,
-});
+const getSnapshot = () => themeRegistryStore.getState();Alternatively, if you only need activeThemeId for the dependency, subscribe to that specifically:
+const getActiveThemeId = () => themeRegistryStore.getState().activeThemeId;
+const getThemes = () => themeRegistryStore.getState().themes;
export function useThemeOverrides(): void {
- const state = useSyncExternalStore(subscribe, getSnapshot);
+ const activeThemeId = useSyncExternalStore(subscribe, getActiveThemeId);
+ const themes = useSyncExternalStore(subscribe, getThemes);
useEffect(() => {
// ...
- }, [state.activeThemeId, state.themes]);
+ }, [activeThemeId, themes]);
}📝 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.
| const getSnapshot = () => ({ | |
| activeThemeId: themeRegistryStore.getState().activeThemeId, | |
| themes: themeRegistryStore.getState().themes, | |
| }); | |
| const getSnapshot = () => themeRegistryStore.getState(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/plugin-api/src/theme/useThemeOverrides.ts` around lines 12 - 15,
getSnapshot currently returns a fresh object every call causing
useSyncExternalStore to always detect a changed snapshot; modify getSnapshot
(used with useSyncExternalStore and themeRegistryStore) to memoize the returned
value by caching the last activeThemeId and themes and only create/return a new
object when either activeThemeId or themes differs from the cached values (or
alternatively return a primitive/tuple like [activeThemeId, themes] or just
activeThemeId if only that is needed) so the reference remains stable across
unchanged state and prevents unnecessary re-renders.
| getTagsPendingSync( | ||
| limit: number | ||
| ): Array<{ | ||
| tag: { id: number; uuid: string; name: string; color: string | null }; | ||
| localVersion: number; | ||
| }> { |
There was a problem hiding this comment.
Fix Prettier formatting to pass CI.
The pipeline indicates a Prettier formatting check failure on this file. The changes here and throughout the file appear to be formatting-related (breaking method chains across lines). Run pnpm format:check locally to identify the exact formatting issues, then apply fixes with prettier --write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/storage-sqlite/src/repositories/SQLiteNoteRepository.ts` around
lines 849 - 854, Prettier formatting failed across this file; run the formatter
and fix formatting issues (especially the method signature and any broken method
chains around getTagsPendingSync in the SQLiteNoteRepository class) by executing
the project's formatting commands (e.g., pnpm format:check to see failures and
prettier --write to apply fixes) so the file conforms to the repo Prettier rules
and CI passes.
- Fix race condition in theme restore by subscribing to themeRegistryStore changes via useSyncExternalStore (App.tsx) - Separate data-color-scheme (dark/light) from data-theme (plugin themes) to prevent attribute conflicts between useAppearanceSettings and useThemeOverrides - Persist IPC nativeTheme isDark value to avoid matchMedia fallback on accent/zoom re-applies (useAppearanceSettings.ts) - Fix unnecessary re-renders in useThemeOverrides by using separate subscriptions for activeThemeId and themes instead of creating new object references - Fix type mismatch in preload theme.setSource to use union type - Fix import order in AppearanceSection.tsx per lint rules - Log warning on failed theme registration in PluginRegistry - Replace disallowed console.debug with guarded console.warn in themeRegistryStore - Fix Prettier formatting in SQLiteNoteRepository.ts - Update CSS selectors in tokens.css and code-highlight.css to use data-color-scheme Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CodeRabbit Review Comments — All Resolved ✅All 9 CodeRabbit comments have been reviewed. Here's what was addressed: Fixed (commit
|
Summary
--bg-*,--text-*,--border-*, etc.) plus extension scopes (--syntax-*,--preview-*,--ui-*) that plugins can override:rootwithdata-themeattributenativeTheme.themeSourcesynced via IPC between main/renderer, replacing unreliableprefers-color-schememedia queriesArchitecture
Layered system: Base CSS tokens → Theme overrides (ThemeRegistry) → Accent color → Plugin CSS vars (cssVariableStore). Each layer has a clear purpose and doesn't interfere with others.
Test plan
pnpm typecheck— 18/18 packages passpnpm test— 167 plugin-api tests pass (including 14 new theme tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores