[diffshub] Theme Switch Fixes Example Branch#738
Draft
amadeus wants to merge 24 commits into
Draft
Conversation
Adds a theme picker next to the gear icon in the diffshub header. Light and dark theme menus plus an Auto/Light/Dark color-mode toggle. Theme changes are pushed to the WorkerPool so the diff actually re-tokenizes with the selected themes.
After WorkerPoolManager.setRenderOptions changed the active theme, two caches kept the previous theme's :host CSS variables (--diffs-light, --diffs-dark, addition/deletion/modified colors) alive: 1. DiffHunksRenderer/FileRenderer's per-instance renderCache.result kept the themeStyles string from the previous tokenization. Collapsed files never re-tokenize (highlightDiffAST is gated on totalLines > 0), so their headers, line numbers, and indicator colors stayed on the old theme until the user toggled the file open. Add WorkerPoolManager.getCurrentThemeStyles() and have processDiffResult and processFileResult pull themeStyles + baseThemeType from it on every render, so the diff chrome refreshes immediately even when the tokens are still being recomputed. 2. FileDiff/File.applyThemeState trusted any data-theme-css <style> node adopted from the element pool (hasAdoptedThemeCSS=true) and skipped writing fresh CSS. When the pool's previous occupant was on a different theme, the recycled container kept showing that theme. Drop the short-circuit and always upsertHostThemeStyle once on adoption. The textContent write is cheap and only fires per pool reuse; the equality check above it still short-circuits steady-state renders.
The file tree had a hardcoded pierre-light-soft/pierre-dark-soft palette and the sidebar wrapper painted itself with the diffshub neutral chrome (--diffshub-sidebar-bg). Resolve the selected light/dark Shiki themes through resolveTheme/themeToTreeStyles in a small shared hook (useResolvedTreeThemeStyles), with a module-level cache so re-selecting a theme is synchronous. The hook feeds two consumers: - CodeViewFileTree applies the resolved TreeThemeStyles directly, so file names, git decorations, hover/selection chrome, etc. follow the theme's editor palette. - CodeViewSidebar inline-styles its wrapper with the theme's sideBar.background (falling back to editor.background) so the tabs row, diff stats panel, and footer share the same surface as the tree. The Tailwind chrome bg is kept as a fallback for the first render while the resolver returns. The diffshub-specific density and padding overrides stay; the color overrides that previously locked the sidebar to the neutral chrome were removed so the resolved theme can actually drive the surface.
The wrapper background already followed the Shiki theme but the sidebar chrome (Diff Stats labels, tab icons, "Powered by" footer) still used the global neutral --muted-foreground / --foreground from the app's ThemeProvider, so the text and icons looked unrelated to the diff and tree palette next to them. Extend useResolvedTreeTheme (formerly useResolvedTreeThemeStyles) to return the resolved Shiki theme's editor.foreground and sideBar.foreground / descriptionForeground alongside the TreeThemeStyles bundle, then override --color-foreground, --color-muted-foreground, --color-border, and --color-border-opaque on the sidebar wrapper: - Primary text uses editor.foreground — the highest-contrast color the theme defines, the same one the diff viewer next door uses for code. - Muted text (Diff Stats labels, file counts, "Powered by" footer) uses the theme's own sideBar.foreground / descriptionForeground shade, so it stays in the palette instead of being a fade-to-transparent of the primary text (which gets unreadable on themes whose editor.foreground sits close to the background). - Borders mix the primary fg at ~20% with the theme background so panel dividers stay subtle on both light and dark themes. The raw --foreground / --muted-foreground / --border variables are mirrored too in case any custom CSS reads them directly. The TreeThemeStyles-only export is preserved as a back-compat alias for callers that don't need the raw colors. Tailwind v4 bakes its @theme aliases out to concrete values at root level, so overriding the original variables alone wasn't enough — the --color-* names had to be overridden too for utilities like text-muted-foreground / border-border to pick up the theme.
Add a small usePersistedState hook that mirrors a string-valued useState to localStorage and rehydrates from it on mount (in an effect, not the useState initializer, so the SSR markup stays identical to the first client render and hydration doesn't mismatch). The rehydration only accepts values present in a `validValues` allowlist, so a stale or hand-edited key from a previous theme set can't push the picker into an unknown name. A hydrated-ref guard skips the first write so the initial commit's default value can't clobber the stored value before the read effect has run. Use it in ReviewUI for both LIGHT_THEME_STORAGE_KEY and DARK_THEME_STORAGE_KEY, replacing the inline read/write effects.
Themes like slack-ochin use opposite palettes for the editor and sidebar surfaces (white editor, dark navy sidebar). Picking `editor.foreground` for the sidebar's primary text and icons ended up writing #000 into --color-foreground on a #2D3E4C surface — making the file/comment tab icons, the Diff Stats ± icon, and the "Files"/"Lines" count values nearly invisible. Switch primaryFg to sideBar.foreground (with editor.foreground / theme.fg as fallbacks); muted text prefers descriptionForeground, otherwise the consumer fades primaryFg at 55%. Drop sideBarSectionHeader.foreground from the muted chain — slack-ochin defines it as #FFF, which is brighter than primary text and inverts the hierarchy.
Some themes ship a list.hoverBackground designed for their editor surface, whose palette can be the inverse of the sidebar's. Slack-ochin is the worst case I've seen: editor.background is white, but sideBar.background is dark navy with light gray text, and list.hoverBackground is #d5e1ea — a near-white wash. Applied to the sidebar that overlay lands on the same luminance band as sideBar.foreground and the hovered row's text disappears. Add a luminance-based heuristic to themeToTreeStyles: parse the hover bg, sidebar bg, and sidebar fg as hex (the dominant format in @shikijs/themes), compute relative luminance, and drop the hover bg when it sits closer in luminance to the fg than to the bg. Non-hex formats fall through unchanged so themes that use modern color syntax keep their intended hover. Also pick the rgba fallback's tint from the sidebar's actual luminance rather than `theme.type`. Otherwise slack-ochin (declared `light` for the editor) would land on `rgba(0,0,0,0.06)` over a dark navy surface — invisible. Reading the bg directly picks `rgba(255,255,255,0.08)` for the dark sidebar so users still get hover feedback. Cover the new behavior with focused unit tests for the slack-ochin shape, a typical light theme, the equal-color short-circuit, and a non-hex hover bg.
Extract the chrome CSS-variable logic that lived inline in CodeViewSidebar into a shared `useThemeChromeStyle` hook in `useResolvedTreeThemeStyles.ts`, and apply it to the header too. The header now lives on the same Shiki theme as the sidebar — bg, primary text/icons, muted text, and borders all follow the active theme's sideBar tokens instead of the global light/dark palette. Falls back to `--diffshub-sidebar-bg` during the first render before the theme resolves. Add three surface tokens to the chrome style for "cards" that sit on top of the sidebar bg (the comments list rows): `--diffshub-card-bg`, `--diffshub-card-hover-bg`, `--diffshub-card-border`, each a color-mix of primaryFg into the sidebar bg. Replaces the hardcoded `bg-card` / `dark:bg-neutral-800` / fixed-RGBA borders that broke on mixed light/dark themes like slack-ochin (classified "light" but renders a dark navy sidebar — the sidebar's foreground override then collided with a near-white card surface and the body text disappeared). Also derive `--diffshub-comment-add-fg` and `--diffshub-comment-del-fg` by parsing the surface's relative luminance: emerald-700/rose-700 on light surfaces, emerald-400/rose-400 on dark ones. CodeViewCommentsList reads these vars instead of using Tailwind's `dark:` variant for the +Line/-Line label tints, so additions and deletions stay legible on every theme — including slack-ochin's dark navy card where the global `dark:` variant would otherwise leave us with low-contrast 700 shades.
…ers" This reverts commit 4945fc0.
Basically with my fixes this stuff won't be required
The light/dark theme picker inside the header dropdown was using a plain `overflow-y-auto` container, so its scrollbar landed on the wide platform-default style instead of the slim thumb-on-hover that the comments list (and the rest of diffshub's secondary scrollables) uses. Reuse `cv-mini-scrollbar` from globals.css and add `overscroll-contain` so the wheel doesn't kick into the underlying CodeView when the list hits its top/bottom.
The diffshub WorkerPool is a long-lived singleton that survives across
client-side route changes. Until now ReviewUI fired its
`workerPool.setRenderOptions({ theme })` effect during the very first
render — when usePersistedState still returns DEFAULT_LIGHT_THEME /
DEFAULT_DARK_THEME because the localStorage rehydration effect hasn't
run yet. That was harmless on the initial load (the pool also started
on the defaults), but on a second visit (e.g. logo → home → another
PR) it would: (1) yank the pool back to the defaults, clearing its
caches and notifying every theme subscriber, (2) start tokenizing the
incoming files against the wrong theme, then (3) rehydrate and swap
back to the user's pick on the next render — leaving the diff visibly
unthemed until a full reload re-ran the chain in a different order.
Have usePersistedState expose an `isHydrated` flag (and use it as the
write-guard ref) so ReviewUI can compute `themesHydrated` from both
picks. The WorkerPool effect now bails out until that flag is true, and
the `viewerAvailable` gate withholds the CodeView mount until the same
condition holds — so the viewer is never constructed with a stale
theme prop and never asks the pool to tokenize against the wrong
palette. The hot path (initial load with no stored value) is
unaffected: the pool stays on its built-in defaults until hydration
completes, then a single setRenderOptions call applies the user's
choice.
The chrome and file tree both pulled text color straight from `sideBar.foreground`. Some themes ship a value that fails on the surface they're paired with (material-theme-ocean's #525975 on #0F111A is ~2.6:1; aurora-x doesn't set it at all). Add a contrast-aware picker that walks `sideBar.foreground` → `editor.foreground` → `theme.fg` in design-intent order, keeps the first candidate that clears 3:1 against the sidebar bg, and falls back to the highest-contrast option when none do. Apply the same gate to `descriptionForeground` so dim muted tokens (aurora-x's #576daf79, ~1.8:1) fall through to the primary fg instead of fading every label and icon into the surface. Mirror the resolved primary fg into the tree's tokens — `color`, `--trees-theme-sidebar-fg`, and the section-header / active-selection / focus-ring tokens when the theme doesn't set them explicitly — so the file rows track the chrome end-to-end.
When the header's light / dark theme picker opens, the currently selected row was wherever its alphabetical position placed it — often off-screen on a list of 40+ themes. Ref the scroll container and the selected row, and on view change scroll so the row sits one row below the top of the viewport (computed via bounding rects since the container isn't a positioned ancestor). The selected row lands right under the cursor and the row above it makes the prior theme a single arrow-key away, which helps sequential browsing.
The badge was hardcoded to `bg-neutral-200/700` + `text-neutral-700/200`, so on dark/themed sidebars it lit up as a neutral gray pill out of step with the rest of the chrome. Switch to a `color-mix(currentColor 18%, transparent)` background and drop the explicit text color so the badge inherits the tab button's color — it now tints with the active Shiki theme in both the unselected ghost state and the selected outline state.
A new button in the System Monitor row, left of the autoscroll toggle, drives a sweep through every light theme then every dark theme (and back), anchored on whichever theme is currently active so the visible state isn't yanked when the cycle starts. Plain click toggles cycling on/off. Shift-click bumps the per-step duration. Cycling state lives in a new `useThemeCycle` hook in ReviewUI and is prop-drilled through CodeViewSidebar to WorkerPoolStatus.
…containers
Three load-bearing pieces working together make code-view theme switching
reliable. Each addresses a distinct way theme state can fall out of sync,
and they are not interchangeable.
1. Keep `theme: { dark, light }` on the CodeView options.
`CodeView.setOptions` clears its element pool via `shouldClearPool`,
which compares `previousOptions.theme` to `nextOptions.theme`. Without
`theme` on the option object both sides resolve to `undefined`,
`areThemesEqual` returns true, and the pool is never cleared on theme
swaps. The pool then hands out containers carrying the previous
theme's shadow-root CSS. The option is also what flips
`areDiffRenderOptionsEqual` in `getRenderOptions`, which is how the
renderer detects a theme change and sets `forceHighlight=true`.
2. Refresh `themeStyles` and `baseThemeType` from the worker pool in
`processDiffResult` / `processFileResult` via
`WorkerPoolManager.getCurrentThemeStyles()`.
`setRenderOptions` clears the worker-side caches, but each renderer's
per-instance `renderCache.result` still holds the old themeStyles
string. Collapsed files never re-tokenize (the async highlight path
is the only thing that would refresh the cached result), so without
this override their headers, line numbers, and addition/deletion
colors stay on the previous theme until the user toggles the file
open. Pulling themeStyles from the worker manager on every render
refreshes only the chrome — the cached syntax tokens are left alone
and get replaced when `highlightDiffAST` completes.
3. Always write fresh CSS on element-pool adoption in `applyThemeState`
(do not trust the adopted shadow-root contents).
Theme changes clear the pool synchronously, but the new CSS for the
currently-visible containers is written by `CodeView.render()`, which
is queued via `queueRender` rather than running synchronously. A fast
scroll during a theme swap can release a visible container back into
the pool before that queued render writes the new CSS, so the pool
ends up holding containers whose shadow root still carries the
previous theme. When the next file adopts one of those containers and
`applyThemeState` shortcuts on `hasAdoptedThemeCSS`, the bookkeeping
updates but the stale `<style>` textContent never gets overwritten —
the wrong theme is then permanently locked in by the equality
fast-path. Falling through to `upsertHostThemeStyle` on first
adoption is a single cheap textContent write per pool reuse; the
equality check above it still short-circuits steady-state renders.
The contrast threshold for `descriptionForeground` was 3:1 (WCAG AA large text), inherited from the primary-fg picker where we deliberately honour the theme designer's dim sidebar values. That's the wrong bar for the sidebar's muted labels — "Diff Stats", "System Monitor", "Comments", file/addition/deletion counts, empty-state copy — which all render at normal body-text size and need 4.5:1 to stay legible. Ayu-dark's `descriptionForeground` is `#5a6378` on `#0d1017` (~3.27:1). It cleared the 3:1 floor and so was silently kept, but the labels read as dim shadow against the near-black sidebar. The previous fallback, when the threshold *did* fail, was the theme's primaryFg — flattening muted up to primary, which loses the visual hierarchy entirely. Split the muted threshold from the primary one (`MIN_MUTED_RATIO = 4.5`) and add `deriveMutedFg`: when the theme's value fails the bar, walk a 60% → 90% mix of primaryFg into bg and keep the first blend that clears 4.5:1, falling back to primaryFg only as a last resort. Ayu-dark now gets `#8a8986` on `#0d1017` (~5.8:1) — distinctly softer than its `#bfbdb6` primary but firmly readable. Other themes whose `descriptionForeground` was already legible keep their original value.
A theme change now flips header, file tree, comments, footer, and diff viewer together in the same paint frame. Previously the chrome trailed the diff (and on cold themes briefly flashed the pierre-soft fallback), and the comments cards / footer links visibly interpolated through the old palette for ~150ms on every swap. Four pieces, each fixing a distinct desync source: 1. `WorkerPoolManager.setRenderOptions` (packages/diffs): hoist the local-state update + cache clear + `themeSubscribers.rerender()` above the `await this.setRenderOptionsOnWorkers(...)`. The per-worker `postMessage` calls fire synchronously inside that Promise constructor, so each worker has the `set-render-options` task queued before this function returns — any highlight tasks the rerender submits are processed after the theme swap, preserving ordering. The await stays at the end so the public Promise still resolves on full completion. Effect: the diff's chrome (header/gutter/colors via `getCurrentThemeStyles()`) updates in the same synchronous run as the rerender, instead of waiting tens of ms for every worker to ACK. 2. `ReviewUI` worker-pool effect: `useEffect` → `useLayoutEffect`. The sidebar's inline-style update lands during React's commit phase, so running the worker-pool update in a paint-time effect kept the diff one frame behind the chrome on every swap. 3. `useResolvedThemeByName`: return the module cache directly during render instead of routing it through useState. `useState`'s initializer only runs on mount, so for cycled-through themes the hook was returning the *previous* render's value and waiting for the post-commit `useEffect` to call `setResolved` — one full React commit of chrome lag for every swap. Also keep the previously resolved theme visible while a cold theme loads, matching the diff side's behaviour (the diff stays on the previous theme during its own await), so chrome doesn't flash the pierre-soft fallback in the intermediate render. 4. Drop `transition-colors` from comment cards and `color, text-decoration-color` from `.inline-link`. Both are Tailwind hover- smoothness niceties, but they ALSO interpolate when the underlying CSS variables flip on a theme swap — the comment cards stepped through ~18 intermediate RGB values over 150ms before landing, visibly trailing the header / tree / diff which had no color transitions and snapped instantly. There's no CSS-level way to distinguish "user hovered" from "CSS variable changed", and a JS-driven `transition: none` class can't catch transitions that start during commit before any effect fires. Removing the transitions at the source costs only the smooth hover bg fade, which is far cheaper than a visibly desynchronised theme swap. Verified live by sampling `--color-foreground`, `--diffshub-card-bg`, the footer link's computed color, and a diff container's `--diffs-dark-bg` at 3ms intervals through a 7-second cycle: every transition lands in a single polling tick across all four surfaces, the footer no longer interpolates, and the diff is within 16ms of the chrome (one paint frame).
Apply resolved Shiki chrome tokens to the theme picker, display settings menu, comment sidebar cards, and inline diff comments. Scope inline annotation variables so the diff viewport keeps its own border and scrollbar colors.
* Adding initial tests * Phase 2: Extract Shared Layout Math Create small reusable utilities for the estimate helper and existing virtual layout code. - Extract or share the `getExpandedRegion` logic currently inside `iterateOverDiff` so expansion math cannot drift. - Add a separator estimate helper that mirrors renderer/CSS behavior. - Keep these helpers pure and independent of `VirtualizedFileDiff` instance state. Separator helper rules: - `simple`: render only when `hunkIndex > 0`; height is `metrics.hunkSeparatorHeight ?? 4`. - `metadata`: render only when `hunkSpecs != null`; height is `metrics.hunkSeparatorHeight ?? 32`. - `line-info-basic`: height is `metrics.hunkSeparatorHeight ?? 32`. - `line-info`: height is `metrics.hunkSeparatorHeight ?? 32`, plus top/bottom spacing based on first/last position. - `line-info`: omit top gap for `isFirstHunk`. - `line-info`: omit bottom gap for `isLastHunk`. - `custom`: estimate like `line-info` unless we decide to force custom separators to be measurement-only later. * Phase 3: Add The Baseline Estimate Utility * Phase 4: Integrate Estimated Height Caches In `VirtualizedFileDiff` * Phase 5: Convert Measured Heights To Sparse Deltas * final tests n stuff * Phase 1: Add Live Option Facades * Slight optimizations to rebuild. * optimize memory in the new facade system * Misc api change cleanups * Ensure CodeView properly evicts element pool on theme change * Fix a regression with no-newline rows * Ensure interactionOptions are properly synced
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
amadeus
changed the title
* Manually clear renderCache on theme change for WorkerPool This will ensure immediate plaintext updates * Fix a ton of WorkerPoolManager race conditions When it cames to render options updating, there were a ton of potential race conditions and extra delays that were incurred, this should fix most of them. * Proper invalidate render option updates if they overlap * Ensure that invalid highlighting jobs are discarded * Fix poisoned element pool There were a variety of race conditions where an element could be pooled and re-claimed when it's shadow dom was actually invalid. This should fix all of those cases from before Also improved the max size for the element tool to dynamically adjust to possible render windows to reduce general unnecessary pooling thrash.
amadeus
force-pushed
the
amadeus/theme-switch-fixes
branch
from
4b26f90 to
cfd5011
Compare
necolas
force-pushed
the
nicolas/diffshub-theme-switcher
branch
2 times, most recently
from
af91e91 to
85c361f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: Don't review this branch, review #737 instead. The goal of this branch is to show an amalgamation of main, plus the fixes from #737 and a reversion of workarounds, does work.
We probably don't want to actually merge this branch, what should happen is that once #737 is merged, main is merged or rebased into #731 and then associated fixes in diffs should be dropped or reverted.