DO NOT MERGE VSCode theme support in T3 Code#2550
Conversation
- Document discovery, mapping, and persistence strategy - Outline desktop bridge, settings UI, and rollout phases
- Discover editor themes from desktop installs - Map theme colors into shared app and syntax tokens - Wire theme state through desktop, web, and settings
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XXL
bmdavis419
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. The PR title explicitly indicates 'DO NOT MERGE'. Additionally, this introduces a significant new feature (VSCode/Cursor theme discovery and sync) with complex async state management, and there are unresolved review comments identifying potential race conditions and error handling issues. You can customize Macroscope's approvability policy. Learn more. |
- Reset stored theme preference when the requested VS Code theme cannot be loaded - Reuse the resolved destructive color for both destructive and destructive-foreground mapping
- Add preference equality guard in theme hook - Prevent outdated async theme loads from overriding newer selections
| localStorage.setItem( | ||
| BOOTSTRAP_THEME_CACHE_KEY, | ||
| JSON.stringify({ id: theme.id, kind: theme.kind, appVariables: theme.appVariables }), | ||
| ); | ||
| recomputeSnapshot({ preference, resolvedColorTheme: theme, status: "ready", message: null }); | ||
| applyResolvedTheme(theme, suppressTransitions); |
There was a problem hiding this comment.
🟡 Medium hooks/useTheme.ts:315
If localStorage.setItem on line 315 throws (e.g., QuotaExceededError or SecurityError in private browsing), the exception propagates unhandled. This prevents recomputeSnapshot and applyResolvedTheme on lines 319-320 from executing, leaving the state stuck at status: "loading" while the successfully loaded theme is never applied.
- localStorage.setItem(
- BOOTSTRAP_THEME_CACHE_KEY,
- JSON.stringify({ id: theme.id, kind: theme.kind, appVariables: theme.appVariables }),
- );
- recomputeSnapshot({ preference, resolvedColorTheme: theme, status: "ready", message: null });
- applyResolvedTheme(theme, suppressTransitions);
+ try {
+ localStorage.setItem(
+ BOOTSTRAP_THEME_CACHE_KEY,
+ JSON.stringify({ id: theme.id, kind: theme.kind, appVariables: theme.appVariables }),
+ );
+ } catch {
+ // Cache failure should not block theme application
+ }
+ recomputeSnapshot({ preference, resolvedColorTheme: theme, status: "ready", message: null });
+ applyResolvedTheme(theme, suppressTransitions);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/hooks/useTheme.ts around lines 315-320:
If `localStorage.setItem` on line 315 throws (e.g., `QuotaExceededError` or `SecurityError` in private browsing), the exception propagates unhandled. This prevents `recomputeSnapshot` and `applyResolvedTheme` on lines 319-320 from executing, leaving the state stuck at `status: "loading"` while the successfully loaded `theme` is never applied.
Evidence trail:
apps/web/src/hooks/useTheme.ts lines 275-322 (REVIEWED_COMMIT): `loadPreference` function. Line 290 sets status to 'loading'. try/catch on lines 291-301 only wraps `bridge.loadColorTheme`. Line 315-318 `localStorage.setItem(BOOTSTRAP_THEME_CACHE_KEY, ...)` is outside try/catch. Lines 319-320 set 'ready' status and apply theme. Callers at lines 392, 409, 421 all use `void loadPreference(...)` (fire-and-forget).
| function loadThemePreference(preference: ThemePreference, suppressTransitions = true) { | ||
| const key = themePreferenceKey(preference); | ||
| const existing = preferenceLoadPromises.get(key); | ||
| if (existing) return existing; | ||
|
|
||
| const promise = loadPreference(preference, suppressTransitions).finally(() => { | ||
| if (preferenceLoadPromises.get(key) === promise) { | ||
| preferenceLoadPromises.delete(key); | ||
| } | ||
| }); | ||
| preferenceLoadPromises.set(key, promise); | ||
| return promise; | ||
| } |
There was a problem hiding this comment.
🟡 Medium hooks/useTheme.ts:345
When a user rapidly switches themes A→B→A, the third call returns the cached promise from the first call without updating state.preference to A. Meanwhile the B call already set state.preference = B. When the original A promise completes, isSamePreference(state.preference, preference) compares B to A, returns false, and bails. Result: the user's final selection A is ignored and theme B remains applied.
function loadThemePreference(preference: ThemePreference, suppressTransitions = true) {
const key = themePreferenceKey(preference);
const existing = preferenceLoadPromises.get(key);
- if (existing) return existing;
+ if (existing) {
+ state.preference = preference;
+ return existing;
+ }
const promise = loadPreference(preference, suppressTransitions).finally(() => {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/hooks/useTheme.ts around lines 345-357:
When a user rapidly switches themes A→B→A, the third call returns the cached promise from the first call without updating `state.preference` to A. Meanwhile the B call already set `state.preference = B`. When the original A promise completes, `isSamePreference(state.preference, preference)` compares B to A, returns false, and bails. Result: the user's final selection A is ignored and theme B remains applied.
Evidence trail:
apps/web/src/hooks/useTheme.ts lines 346-358 (loadThemePreference with preferenceLoadPromises cache), lines 292-343 (loadPreference with recomputeSnapshot and isSamePreference staleness check at lines 308, 317), line 349 (early return of cached promise without calling loadPreference).
1 participant
auto-detects vscode/cursor themes installed on ur machine. u pick one, it just works in t3 code
enjoy the 5.5 xHigh slop pr, do with it what u will I just want this to exist at some point in some way
Summary
Testing
bun fmtbun lintbun typecheckbun run testNote
Add VSCode/Cursor editor theme discovery and application to T3 Code
ThemePreferencetype (system, builtin, external, follow-editor) stored in client settings; the theme runtime loads and caches external themes asynchronously with deduplication and localStorage bootstrapping.mapVscodeColorsToAppVariablesutility inpackages/shared/src/themeMapping.ts; desktop window chrome colors (background, title bar) update to match the active theme.ShikiCodeBlockthat caches results per code+language+theme and falls back gracefully for unsupported languages.resolveSyntaxThemeNameregisters content-addressed custom Shiki themes derived from the resolved color theme.Macroscope summarized bb538d7.