Skip to content

Commit d27abe2

Browse files
backnotpropclaude
andcommitted
refactor: memoize ThemeProvider context, deduplicate Mode type, clean up DiffViewer
From /simplify review: - Export Mode type from ThemeProvider, import in ThemeTab (was duplicated) - Add resolvedMode to context — consumers no longer re-query matchMedia - Memoize context value with useMemo, setters with useCallback (prevents unnecessary re-renders of all useTheme consumers) - Consolidate DiffViewer's two pierre state vars into single object - Use resolvedMode from context in DiffViewer instead of classList check - Format crammed single-line extended tokens in 4 theme CSS files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 05bbc54 commit d27abe2

7 files changed

Lines changed: 108 additions & 73 deletions

File tree

packages/review-editor/components/DiffViewer.tsx

Lines changed: 25 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
5353
canStage = false,
5454
stageError,
5555
}) => {
56-
const { theme, colorTheme } = useTheme();
56+
const { theme, colorTheme, resolvedMode } = useTheme();
5757
const containerRef = useRef<HTMLDivElement>(null);
5858

5959
const toolbar = useAnnotationToolbar({ patch, filePath, onLineSelection, onAddAnnotation, onEditAnnotation });
@@ -175,39 +175,36 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
175175
);
176176
}, [toolbar.handleLineSelectionEnd]);
177177

178-
// Determine theme type and inject resolved colors into @pierre/diffs shadow DOM.
179-
// CSS custom properties don't cross the shadow boundary, so we compute the
180-
// actual color values and pass them via unsafeCSS.
181-
const [pierreThemeType, setPierreThemeType] = useState<'dark' | 'light'>('dark');
182-
const [pierreUnsafeCSS, setPierreUnsafeCSS] = useState('');
178+
// Inject resolved colors into @pierre/diffs shadow DOM.
179+
// CSS custom properties don't cross the shadow boundary, so we read computed
180+
// values and pass them via unsafeCSS. Single state object avoids split renders.
181+
const [pierreTheme, setPierreTheme] = useState<{ type: 'dark' | 'light'; css: string }>({ type: 'dark', css: '' });
183182

184-
// Recompute after DOM class changes (useEffect runs after ThemeProvider's useEffect)
185183
useEffect(() => {
186-
// Small delay to ensure ThemeProvider has applied the class
187184
requestAnimationFrame(() => {
188-
const root = document.documentElement;
189-
setPierreThemeType(root.classList.contains('light') ? 'light' : 'dark');
190-
191-
const styles = getComputedStyle(root);
185+
const styles = getComputedStyle(document.documentElement);
192186
const bg = styles.getPropertyValue('--background').trim();
193187
const fg = styles.getPropertyValue('--foreground').trim();
194188
const muted = styles.getPropertyValue('--muted').trim();
195189
if (!bg || !fg) return;
196-
setPierreUnsafeCSS(`
197-
:host, [data-diff], [data-file], [data-diffs-header], [data-error-wrapper], [data-virtualizer-buffer] {
198-
--diffs-bg: ${bg} !important;
199-
--diffs-fg: ${fg} !important;
200-
--diffs-dark-bg: ${bg};
201-
--diffs-light-bg: ${bg};
202-
--diffs-dark: ${fg};
203-
--diffs-light: ${fg};
204-
}
205-
pre, code { background-color: ${bg} !important; }
206-
[data-file-info] { background-color: ${muted} !important; }
207-
[data-column-number] { background-color: ${bg} !important; }
208-
`);
190+
setPierreTheme({
191+
type: resolvedMode,
192+
css: `
193+
:host, [data-diff], [data-file], [data-diffs-header], [data-error-wrapper], [data-virtualizer-buffer] {
194+
--diffs-bg: ${bg} !important;
195+
--diffs-fg: ${fg} !important;
196+
--diffs-dark-bg: ${bg};
197+
--diffs-light-bg: ${bg};
198+
--diffs-dark: ${fg};
199+
--diffs-light: ${fg};
200+
}
201+
pre, code { background-color: ${bg} !important; }
202+
[data-file-info] { background-color: ${muted} !important; }
203+
[data-column-number] { background-color: ${bg} !important; }
204+
`,
205+
});
209206
});
210-
}, [theme, colorTheme]);
207+
}, [resolvedMode, colorTheme]);
211208

212209
return (
213210
<div ref={containerRef} className="h-full overflow-auto relative" onMouseMove={toolbar.handleMouseMove}>
@@ -228,8 +225,8 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
228225
key={filePath}
229226
fileDiff={augmentedDiff}
230227
options={{
231-
themeType: pierreThemeType,
232-
unsafeCSS: pierreUnsafeCSS,
228+
themeType: pierreTheme.type,
229+
unsafeCSS: pierreTheme.css,
233230
diffStyle,
234231
diffIndicators: 'bars',
235232
hunkSeparators: 'line-info',

packages/ui/components/ThemeProvider.tsx

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,16 @@
1-
import { createContext, useContext, useEffect, useState } from 'react';
1+
import { createContext, useCallback, useContext, useEffect, useMemo, useState } from 'react';
22
import { storage } from '../utils/storage';
33
import { BUILT_IN_THEMES, type ThemeInfo } from '../utils/themeRegistry';
44

5-
type Mode = 'dark' | 'light' | 'system';
5+
export type Mode = 'dark' | 'light' | 'system';
66

77
type ThemeProviderState = {
88
// Mode (dark/light/system) — backward-compatible with old "theme" API
99
theme: Mode;
1010
setTheme: (mode: Mode) => void;
1111
mode: Mode;
1212
setMode: (mode: Mode) => void;
13+
resolvedMode: 'dark' | 'light';
1314
// Color theme (palette)
1415
colorTheme: string;
1516
setColorTheme: (theme: string) => void;
@@ -21,6 +22,7 @@ const ThemeProviderContext = createContext<ThemeProviderState>({
2122
setTheme: () => null,
2223
mode: 'dark',
2324
setMode: () => null,
25+
resolvedMode: 'dark',
2426
colorTheme: 'plannotator',
2527
setColorTheme: () => null,
2628
availableThemes: BUILT_IN_THEMES,
@@ -49,8 +51,15 @@ export function ThemeProvider({
4951
() => storage.getItem(colorThemeStorageKey) || defaultColorTheme
5052
);
5153

52-
// Resolve whether the .light class should be applied, respecting theme's modeSupport
53-
const resolveClasses = (effectiveMode: 'dark' | 'light') => {
54+
const [systemIsLight, setSystemIsLight] = useState(
55+
() => typeof window !== 'undefined' && window.matchMedia('(prefers-color-scheme: light)').matches
56+
);
57+
58+
// Compute resolved mode once — consumers use this instead of re-querying matchMedia
59+
const resolvedMode: 'dark' | 'light' = mode === 'system' ? (systemIsLight ? 'light' : 'dark') : mode;
60+
61+
// Resolve classes from resolved mode + theme's modeSupport
62+
const resolveClasses = useCallback((effectiveMode: 'dark' | 'light') => {
5463
const themeInfo = BUILT_IN_THEMES.find(t => t.id === colorTheme);
5564
const modeSupport = themeInfo?.modeSupport ?? 'both';
5665

@@ -59,57 +68,44 @@ export function ThemeProvider({
5968
if (modeSupport === 'light-only') applyLight = true;
6069

6170
return `theme-${colorTheme}${applyLight ? ' light' : ''}`;
62-
};
71+
}, [colorTheme]);
6372

6473
// Apply theme class + mode class to document element
6574
useEffect(() => {
66-
const root = window.document.documentElement;
67-
68-
let effectiveMode: 'dark' | 'light' = 'dark';
69-
if (mode === 'system') {
70-
effectiveMode = window.matchMedia('(prefers-color-scheme: light)').matches
71-
? 'light'
72-
: 'dark';
73-
} else {
74-
effectiveMode = mode;
75-
}
76-
77-
root.className = resolveClasses(effectiveMode);
78-
}, [mode, colorTheme]);
75+
window.document.documentElement.className = resolveClasses(resolvedMode);
76+
}, [resolvedMode, resolveClasses]);
7977

8078
// Listen for system theme changes
8179
useEffect(() => {
8280
if (mode !== 'system') return;
8381

8482
const mediaQuery = window.matchMedia('(prefers-color-scheme: light)');
85-
const handleChange = () => {
86-
const root = window.document.documentElement;
87-
root.className = resolveClasses(mediaQuery.matches ? 'light' : 'dark');
88-
};
83+
const handleChange = () => setSystemIsLight(mediaQuery.matches);
8984

9085
mediaQuery.addEventListener('change', handleChange);
9186
return () => mediaQuery.removeEventListener('change', handleChange);
92-
}, [mode, colorTheme]);
87+
}, [mode]);
9388

94-
const setMode = (newMode: Mode) => {
89+
const setMode = useCallback((newMode: Mode) => {
9590
storage.setItem(storageKey, newMode);
9691
setModeState(newMode);
97-
};
92+
}, [storageKey]);
9893

99-
const setColorTheme = (newTheme: string) => {
94+
const setColorTheme = useCallback((newTheme: string) => {
10095
storage.setItem(colorThemeStorageKey, newTheme);
10196
setColorThemeState(newTheme);
102-
};
97+
}, [colorThemeStorageKey]);
10398

104-
const value: ThemeProviderState = {
99+
const value = useMemo<ThemeProviderState>(() => ({
105100
theme: mode,
106101
setTheme: setMode,
107102
mode,
108103
setMode,
104+
resolvedMode,
109105
colorTheme,
110106
setColorTheme,
111107
availableThemes: BUILT_IN_THEMES,
112-
};
108+
}), [mode, resolvedMode, colorTheme, setMode, setColorTheme]);
113109

114110
return (
115111
<ThemeProviderContext.Provider value={value}>

packages/ui/components/ThemeTab.tsx

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,8 @@
11
import React from 'react';
2-
import { useTheme } from './ThemeProvider';
3-
4-
type Mode = 'dark' | 'light' | 'system';
2+
import { useTheme, type Mode } from './ThemeProvider';
53

64
export const ThemeTab: React.FC = () => {
7-
const { mode, setMode, colorTheme, setColorTheme, availableThemes } = useTheme();
8-
9-
const resolvedMode: 'dark' | 'light' = mode === 'system'
10-
? (typeof window !== 'undefined' && window.matchMedia('(prefers-color-scheme: light)').matches ? 'light' : 'dark')
11-
: mode;
5+
const { mode, setMode, colorTheme, setColorTheme, availableThemes, resolvedMode } = useTheme();
126

137
return (
148
<>

packages/ui/themes/caffeine.css

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,13 @@
4646
--shadow-xl: 0 1px 3px 0px hsl(0 0% 0% / 0.1), 0 8px 10px -1px hsl(0 0% 0% / 0.1);
4747
--shadow-2xl: 0 1px 3px 0px hsl(0 0% 0% / 0.25);
4848

49-
/* Plannotator extended tokens */--success: oklch(0.72 0.17 150); --success-foreground: oklch(0.15 0.02 260); --warning: oklch(0.75 0.15 85); --warning-foreground: oklch(0.20 0.02 260); --code-bg: oklch(0.13 0.015 260); --focus-highlight: oklch(0.70 0.20 200);
49+
/* Plannotator extended tokens */
50+
--success: oklch(0.72 0.17 150);
51+
--success-foreground: oklch(0.15 0.02 260);
52+
--warning: oklch(0.75 0.15 85);
53+
--warning-foreground: oklch(0.20 0.02 260);
54+
--code-bg: oklch(0.13 0.015 260);
55+
--focus-highlight: oklch(0.70 0.20 200);
5056
}
5157

5258
.theme-caffeine.light {
@@ -97,5 +103,11 @@
97103
--shadow-xl: 0 1px 3px 0px hsl(0 0% 0% / 0.1), 0 8px 10px -1px hsl(0 0% 0% / 0.1);
98104
--shadow-2xl: 0 1px 3px 0px hsl(0 0% 0% / 0.25);
99105

100-
/* Plannotator extended tokens */--success: oklch(0.45 0.20 150); --success-foreground: oklch(1 0 0); --warning: oklch(0.55 0.18 85); --warning-foreground: oklch(0.18 0.02 260); --code-bg: oklch(0.96 0.005 260); --focus-highlight: oklch(0.70 0.20 200);
106+
/* Plannotator extended tokens */
107+
--success: oklch(0.45 0.20 150);
108+
--success-foreground: oklch(1 0 0);
109+
--warning: oklch(0.55 0.18 85);
110+
--warning-foreground: oklch(0.18 0.02 260);
111+
--code-bg: oklch(0.96 0.005 260);
112+
--focus-highlight: oklch(0.70 0.20 200);
101113
}

packages/ui/themes/doom-64.css

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,13 @@
4444
--shadow-xl: 0px 2px 5px 0px hsl(0 0% 0% / 0.6), 0px 8px 10px -1px hsl(0 0% 0% / 0.6);
4545
--shadow-2xl: 0px 2px 5px 0px hsl(0 0% 0% / 1.5);
4646

47-
/* Plannotator extended tokens */--success: oklch(0.72 0.17 150); --success-foreground: oklch(0.15 0.02 260); --warning: oklch(0.75 0.15 85); --warning-foreground: oklch(0.20 0.02 260); --code-bg: oklch(0.13 0.015 260); --focus-highlight: oklch(0.70 0.20 200);
47+
/* Plannotator extended tokens */
48+
--success: oklch(0.72 0.17 150);
49+
--success-foreground: oklch(0.15 0.02 260);
50+
--warning: oklch(0.75 0.15 85);
51+
--warning-foreground: oklch(0.20 0.02 260);
52+
--code-bg: oklch(0.13 0.015 260);
53+
--focus-highlight: oklch(0.70 0.20 200);
4854
}
4955

5056
.theme-doom-64.light {
@@ -93,5 +99,11 @@
9399
--shadow-xl: 0px 2px 4px 0px hsl(0 0% 0% / 0.4), 0px 8px 10px -1px hsl(0 0% 0% / 0.4);
94100
--shadow-2xl: 0px 2px 4px 0px hsl(0 0% 0% / 1.0);
95101

96-
/* Plannotator extended tokens */--success: oklch(0.45 0.20 150); --success-foreground: oklch(1 0 0); --warning: oklch(0.55 0.18 85); --warning-foreground: oklch(0.18 0.02 260); --code-bg: oklch(0.96 0.005 260); --focus-highlight: oklch(0.70 0.20 200);
102+
/* Plannotator extended tokens */
103+
--success: oklch(0.45 0.20 150);
104+
--success-foreground: oklch(1 0 0);
105+
--warning: oklch(0.55 0.18 85);
106+
--warning-foreground: oklch(0.18 0.02 260);
107+
--code-bg: oklch(0.96 0.005 260);
108+
--focus-highlight: oklch(0.70 0.20 200);
97109
}

packages/ui/themes/quantum-rose.css

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,13 @@
4444
--shadow-xl: 0px 3px 0px 0px hsl(300 80% 50% / 0.18), 0px 8px 10px -1px hsl(300 80% 50% / 0.18);
4545
--shadow-2xl: 0px 3px 0px 0px hsl(300 80% 50% / 0.45);
4646

47-
/* Plannotator extended tokens */--success: oklch(0.72 0.17 150); --success-foreground: oklch(0.15 0.02 260); --warning: oklch(0.75 0.15 85); --warning-foreground: oklch(0.20 0.02 260); --code-bg: oklch(0.13 0.015 260); --focus-highlight: oklch(0.70 0.20 200);
47+
/* Plannotator extended tokens */
48+
--success: oklch(0.72 0.17 150);
49+
--success-foreground: oklch(0.15 0.02 260);
50+
--warning: oklch(0.75 0.15 85);
51+
--warning-foreground: oklch(0.20 0.02 260);
52+
--code-bg: oklch(0.13 0.015 260);
53+
--focus-highlight: oklch(0.70 0.20 200);
4854
}
4955

5056
.theme-quantum-rose.light {
@@ -93,5 +99,11 @@
9399
--shadow-xl: 0px 3px 0px 0px hsl(330 70% 30% / 0.18), 0px 8px 10px -1px hsl(330 70% 30% / 0.18);
94100
--shadow-2xl: 0px 3px 0px 0px hsl(330 70% 30% / 0.45);
95101

96-
/* Plannotator extended tokens */--success: oklch(0.45 0.20 150); --success-foreground: oklch(1 0 0); --warning: oklch(0.55 0.18 85); --warning-foreground: oklch(0.18 0.02 260); --code-bg: oklch(0.96 0.005 260); --focus-highlight: oklch(0.70 0.20 200);
102+
/* Plannotator extended tokens */
103+
--success: oklch(0.45 0.20 150);
104+
--success-foreground: oklch(1 0 0);
105+
--warning: oklch(0.55 0.18 85);
106+
--warning-foreground: oklch(0.18 0.02 260);
107+
--code-bg: oklch(0.96 0.005 260);
108+
--focus-highlight: oklch(0.70 0.20 200);
97109
}

packages/ui/themes/solar-dusk.css

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,13 @@
4444
--shadow-xl: 0px 2px 3px 0px hsl(0 0% 5% / 0.18), 0px 8px 10px -1px hsl(0 0% 5% / 0.18);
4545
--shadow-2xl: 0px 2px 3px 0px hsl(0 0% 5% / 0.45);
4646

47-
/* Plannotator extended tokens */--success: oklch(0.72 0.17 150); --success-foreground: oklch(0.15 0.02 260); --warning: oklch(0.75 0.15 85); --warning-foreground: oklch(0.20 0.02 260); --code-bg: oklch(0.13 0.015 260); --focus-highlight: oklch(0.70 0.20 200);
47+
/* Plannotator extended tokens */
48+
--success: oklch(0.72 0.17 150);
49+
--success-foreground: oklch(0.15 0.02 260);
50+
--warning: oklch(0.75 0.15 85);
51+
--warning-foreground: oklch(0.20 0.02 260);
52+
--code-bg: oklch(0.13 0.015 260);
53+
--focus-highlight: oklch(0.70 0.20 200);
4854
}
4955

5056
.theme-solar-dusk.light {
@@ -93,5 +99,11 @@
9399
--shadow-xl: 0px 2px 3px 0px hsl(28 18% 25% / 0.18), 0px 8px 10px -1px hsl(28 18% 25% / 0.18);
94100
--shadow-2xl: 0px 2px 3px 0px hsl(28 18% 25% / 0.45);
95101

96-
/* Plannotator extended tokens */--success: oklch(0.45 0.20 150); --success-foreground: oklch(1 0 0); --warning: oklch(0.55 0.18 85); --warning-foreground: oklch(0.18 0.02 260); --code-bg: oklch(0.96 0.005 260); --focus-highlight: oklch(0.70 0.20 200);
102+
/* Plannotator extended tokens */
103+
--success: oklch(0.45 0.20 150);
104+
--success-foreground: oklch(1 0 0);
105+
--warning: oklch(0.55 0.18 85);
106+
--warning-foreground: oklch(0.18 0.02 260);
107+
--code-bg: oklch(0.96 0.005 260);
108+
--focus-highlight: oklch(0.70 0.20 200);
97109
}

0 commit comments

Comments
 (0)