feat(web): Cool Dark theme, closed folder marks, and composer polish

[NoahHendrickson]

Summary

• Adds Cool Dark as a fourth appearance option (lighter cool-gray dark palette) with useTheme override, CSS tokens, pre-paint bootstrap, and fork guards — Dark stays unchanged
• Collapsed project groups show a closed folder mark at rest (open when expanded); hover still swaps to the chevron
• Composer polish: workspace select opens above the chip row so glass clears the branch pill; context-chip padding/radius and send/stop radius refinements; Search/All projects chrome stay at 14px while project headers use the 13px remap

Test plan

• Settings → Appearance: switch System / Light / Dark / Cool Dark; confirm Cool Dark is lighter/cooler and Dark is unchanged
• Reload on Cool Dark: no flash to the wrong stage color
• Grouped sidebar: collapse a project — closed folder at rest; expand — open folder; hover either — chevron
• Composer: open workspace (checkout) menu — popup clears the branch pill instead of overlapping it
• Sidebar Search and All projects read at 14px; project group headers remain at 13px
• vp test run apps/web/src/__fork_guards__/forkCoolDarkTheme.test.ts apps/web/src/__fork_guards__/forkComposerShell.test.ts apps/web/src/__fork_guards__/forkSidebarChrome.test.ts apps/web/src/__fork_guards__/sidebarV2ProjectGrouping.test.ts

Made with Cursor

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[cursor]

Thermo-nuclear code quality review — REQUEST CHANGES

Cool Dark works as a product feature, but the implementation introduces the fork’s first override as a near-full copy of a hot module for ~20 lines of real delta. That fails the approval bar: clear structural regression + an available code-judo that deletes the ownership cost.

Sidebar closed-folder marks and composer chip polish look fine and local. They are not the blocker.

Blockers

1. Full useTheme shadow — owns the entire theme store forever for a widened union.
2. Missed judo: Cool Dark is a palette, not a ThemePreference — CSS already keys off data-fork-theme while requiring .dark. Stuffing cool-dark into t3code:theme forces Schema widening, desktop remapping, Settings tilde-import hack, and the override.

Preferred fix

Keep upstream theme as light | dark | system. Persist palette separately (e.g. t3code:fork-theme = cool-dark | null) in custom/. Appearance can still show four options: Cool Dark → setTheme("dark") + set palette; other choices clear palette. Delete overrides/hooks/useTheme.ts. Rewrite guards around the palette key + attribute.

If a short-term bridge is required, a few fenced hunks in upstream hooks/useTheme.ts are still cheaper than owning 346 lines — but the separate-palette model is the real simplification.

Approval bar: not met while the full override remains and the palette judo is available.

  

_{Sent by Cursor Automation: Thermo-nuclear PR review}

[NoahHendrickson]

Review

Read the full diff against custom, the fork manifest, the override resolver, the guard tests, and the CI logs. The palette itself is careful work — I diffed every token upstream declares under .dark [data-sidebar-version="v2"] against the Cool Dark panel block and the only gap is --input, which Dark leaves to upstream too, and the only stage tokens Cool Dark doesn't restate are the five --sidebar-v2-status-* accents (shared on purpose) and the two --fork-context-chip-* vars (correctly derived from --fork-composer-bg, so they follow the cool value for free). The doubled stage/panel selector and the :root-level --popover for portalled menus are both right.

That said, this isn't landable as-is.

---

1. Blocking: CI is red, and the failure is in this diff

The Test job fails: 2 suites, 1 test. Not flake — a real defect with a wide blast radius.

TypeError: root.removeAttribute is not a function
 ❯ applyForkThemeAttribute  src/custom/forkTheme.ts:41
 ❯ applyTheme               src/overrides/hooks/useTheme.ts:202
 ❯ (module load)            src/overrides/hooks/useTheme.ts:253
 ❯ src/components/ChatMarkdown.tsx:60

applyForkThemeAttribute calls removeAttribute unconditionally on the non-cool path — i.e. on every theme, including the default. The override runs applyTheme(getStored()) at module load, so any node-environment test that transitively imports anything touching useTheme now needs a documentElement stub richer than upstream ever required. MessagesTimeline.test.tsx doesn't fail at an assertion; it fails on import, via ChatMarkdown. It has nothing to do with theming.

Narrow fix, in forkTheme.ts:

export function applyForkThemeAttribute(root: ForkThemeTarget, theme: ForkThemePreference): void {
  if (theme === COOL_DARK_THEME) {
    root.setAttribute(FORK_THEME_ATTRIBUTE, COOL_DARK_THEME);
    return;
  }
  // Every non-cool theme takes this path, including in tests that stub
  // documentElement with only what upstream's applyTheme touched.
  root.removeAttribute?.(FORK_THEME_ATTRIBUTE);
}

with removeAttribute optional on the structural target type. Fixing the two upstream test files instead would be the wrong direction — it puts fork breakage into files the sync has to keep absorbing.

The interesting part is why the guard didn't catch this. forkCoolDarkTheme.test.ts exercises applyForkThemeAttribute against a hand-built stub that implements both methods, so it is green while the real call site throws. A guard that only ever sees the object it was written against isn't testing the integration — it's restating the implementation.

2. Blocking: this is three PRs

A new theme (~500 lines, a shadowed hook, an index.html patch), a folder-icon state, and unrelated composer polish (radius, gap, padding, popup placement) — on a branch called fix/collapsed-project-group-closed-folder, which is the smallest of the three. CLAUDE.md: "One concern per PR. If the description says 'also', split it." The manifest diff touches three unrelated customization entries, which is the tell.

Split into fork-cool-dark-theme, the folder mark, and the composer polish. Each is independently reviewable and independently revertable; bundled, a bad theme merge drags the icon fix out with it. Also: no before/after images on a PR that is almost entirely visual, and the test plan is entirely unchecked — #40 shipped screenshots, so the bar exists.

---

3. Design: Cool Dark writes a value upstream doesn't understand into upstream's key

t3code:theme is upstream's. Storing "cool-dark" in it means every reader that isn't the override sees a value it rejects — and upstream's fallback is "system", not "dark". So on any path where the override isn't in play (a rollback of this customization, an upstream build on the same origin, a future upstream refactor that reads the key from somewhere new), a Cool Dark user on a light-mode OS silently lands in a light app. The failure mode is a full inversion, not a near-miss.

Store the preference in a fork-owned key instead, keeping t3code:theme = "dark":

• every non-fork reader degrades to Dark, the correct nearest theme, with no code needed anywhere;
• index.html's fenced hunk shrinks to one extra getItem plus the attribute stamp — the isDark/chromeColor ternaries stop needing fork branches at all;
• and the 346-line shadow largely collapses. Right now the override is a byte-for-byte copy of a hot upstream module carrying a ~12-line delta, which .fork/AGENTS.md explicitly discourages ("prefer shadowing small leaf components over large hot files"). With a separate key, the override can import … from "~upstream/hooks/useTheme" — the mechanism overrideResolver.ts provides for exactly this — and add only the attribute stamp. Upstream fixes to useTheme then flow in instead of needing a hand re-copy. Drift detection catching the copy (it's registered under shadows:, good) tells you the copy went stale; it doesn't merge it for you.

4. --fork-context-chip-bg: var(--fork-composer-bg) is a Dark regression, not polish

theme.custom.css:594 changes the dark chip fill from opaque #2a2a2a to var(--fork-composer-bg) — rgb(41 41 41 / 80%). The comment says "so checkout / branch read as the same surface," but they won't, because the surface carries backdrop-filter: blur(var(--glass-blur)) saturate(...) (:361) and the context row carries no filter and no background of its own. Same fill, no blur: over the message list that scrolls beneath the composer — the reason that blur exists — the chips become a 20%-transparent unblurred window onto moving text, next to a blurred one. Legibility drops and the two stop matching precisely where it matters.

Either give the chips the same backdrop-filter (a real cost: CLAUDE.md calls out GPU pressure, and this adds compositing layers to two always-mounted controls) or keep them opaque at the composer's resolved color. Note this hits everyone on Dark today, not just Cool Dark — "Do not change existing behavior as a side effect of a visual change."

5. Verification is text-shaped where behavior is available

Finding 1 is the proof. Three concrete gaps:

• expect(indexHtml).toContain(COOL_DARK_BACKGROUND) is satisfied by the JS const alone, so the second literal — html.dark[data-fork-theme="cool-dark"] body { background: #1c1e20 } — can drift to any value with every test green. The selector is asserted; the declaration isn't. test(fork): pin the terminal font fallback to the CSS mono stack #13 exists to close exactly this class of gap. Assert the declaration: /html\.dark\[data-fork-theme="cool-dark"\] body \{[^}]*background:\s*#1c1e20/u.
• The override is read as a string (useThemeOverride.toContain('"cool-dark"')), so a syntactically broken override passes. Nothing asserts the shadow actually resolves.
• And it's free to fix: overrideResolver.ts redirects relative imports, so src/hooks/useTheme.test.ts's await import("./useTheme") already loads the override — that's how CI caught this at all. A behavioral case in that harness — stored cool-dark survives readThemePreference, applyTheme keeps .dark and stamps the attribute, the bridge receives "dark" — is ~10 lines and would have caught finding 1 before the push.

Worth stating in the guard header that src/hooks/useTheme.ts is now shadowed dead code whose test secretly covers the fork copy. That's non-obvious to the next reader.

6. --muted-foreground misses AA in Cool Dark

#90949a on --muted/--accent #2c2f33 is 4.41:1 — under AA's 4.5 for body text, wherever secondary text sits on an accent fill (hovered select items, ghost-pill hover). On the stage it's fine at 5.48:1; it's the raised background that eats the margin. Dark doesn't have this problem because it inherits upstream's #a3a3a3 on #0a0a0a (~7.9:1). #93979d gets you to 4.58:1 and is visually indistinguishable. The guard checks luminance ordering and blue-ness but never contrast — worth an assertion, since the whole point of a lighter dark palette is that every foreground pair gets tighter.

---

Smaller

• Dead exports. COOL_DARK_LABEL and isCoolDarkTheme are referenced nowhere — not by app code, not by the guard. SettingsPanels hardcodes label: "Cool Dark" and the guard asserts the literal. COOL_DARK_BACKGROUND and FORK_THEME_ATTRIBUTE are guard-only. Drop the first two.
• side="top" in a shared component. BranchToolbarEnvModeSelector.tsx gets composer-specific placement hard-coded into it. One call site today, so nothing breaks — but the constraint belongs where the layout is known, and Base UI will collision-flip on a short viewport and silently reinstate the overlap the change exists to fix. Worth a line in the fence saying that's accepted.
• Guard regex is prop-order dependent. /SelectPopup[^>]*alignItemWithTrigger=\{false\}[^>]*side="top"/u fails if a formatter swaps the two props. Assert each independently.
• Type leak. Only SettingsPanels was switched to ~/hooks/useTheme. Relative importers (e.g. DiffWorkerPoolProvider.tsx:5) still typecheck against upstream's narrower Theme while receiving the override at runtime. Harmless today — that file only reads resolvedTheme, which is correctly collapsed to "light" | "dark" — but the next theme === … narrowing added under a relative import will typecheck as if cool-dark is impossible.
• Surfaces. Cool Dark is web/desktop only (t3code:theme is client-local, so mobile is genuinely unaffected) and toDesktopTheme correctly maps to nativeTheme's dark. Both are the right calls — say so in the manifest intent so the next reader doesn't re-derive it. One thing to check on desktop: DesktopWindow.ts:328 sets the BrowserWindow backgroundColor from shouldUseDarkColors, which can't distinguish Cool Dark, so launch/resize may flash #161616 against a #1c1e20 stage.

---

Fix CI, split the PR, and I'd focus the theme half on the storage-key question in §3 — it's the one decision that's expensive to reverse after users have a value persisted.

---

Generated by Claude Code

[NoahHendrickson]

Review — round 2 (f4950a51)

The rearchitecture is the right call and it's a real improvement: upstream ThemePreference is untouched, t3code:theme stays light|dark|system, the 346-line shadow is gone, the manifest correctly drops shadows:, and there's a legacy migration for anyone who already picked Cool Dark on 9d9a9bd2. resolveAppearanceOption is a clean seam. Round 1's §3 and the shadow half of the CI failure are resolved.

Round 1's §2 (three concerns in one PR), §4 (translucent context chips), §6 (contrast), and most of §5 (guard gaps) are untouched and still stand. New round-2 findings below — the rewrite introduced its own set, which is normal for a same-hour rewrite but they need addressing before this lands.

---

1. The CI defect was relocated, not fixed

(Check is green on f4950a51; Test is still running as of this comment, so treat the recovery as expected rather than confirmed.)

applyForkPaletteAttribute still calls removeAttribute unconditionally on the null path (custom/forkTheme.ts), and forkTheme.ts still runs it at module load. What changed is the import graph, not the code: forkTheme.ts is now reachable only from SettingsPanels.tsx, so ChatMarkdown no longer drags it into every node-environment suite. That's what should clear MessagesTimeline.test.tsx and hooks/useTheme.test.ts — the deleted override, not a fixed helper.

The defect class is identical. The first test that imports SettingsPanels (or anything that will one day import forkTheme) in the node environment with a partial documentElement stub reproduces it exactly. Note the module now also imports syncBrowserChromeTheme from ../hooks/useTheme, so importing forkTheme executes upstream's module-load applyTheme too — the import graph is wider than it looks.

Still a one-liner: make removeAttribute optional on AttributeRoot and call it as root.removeAttribute?.(FORK_THEME_ATTRIBUTE).

And the guard still can't catch it — it hand-builds a stub implementing both methods, then asserts against that stub. Same critique as round 1, verbatim.

2. The palette's DOM sync now lives in a Settings-only module

forkTheme.ts is imported by exactly one component. Everything that keeps data-fork-theme in sync after boot — the module-load re-stamp, subscribePalette, applyForkPalette — is therefore dead in any tab where the user never opened Settings. The old override was reachable from every surface because useTheme is; this isn't.

Concrete consequence, two tabs open (ordinary for this app):

1. Tab A: Appearance → Cool Dark.
2. Tab B: Appearance → System.
3. Tab B writes t3code:theme=system and removes the palette key.
4. Tab A's upstream useTheme storage handler re-applies the theme — but nothing re-stamps the attribute, because subscribePalette's handler only calls emitChange(). It notifies React; it never touches the DOM.
5. If the OS is dark, tab A keeps .dark and the stale data-fork-theme="cool-dark" — still painting Cool Dark while the stored preference is System. It corrects on reload.

And if tab A never opened Settings, there's no listener at all. The old override handled this in its storage handler by calling applyTheme.

Fix: have the storage handler re-stamp and re-sync chrome, not just emitChange — and move the stamp + subscription into a module main.tsx imports directly, next to applyForkMarker. That's where the fork already puts document-level stamping, and it decouples the palette from Settings being mounted.

3. "Is Cool Dark active" now has three disagreeing definitions

Site	Predicate
index.html bootstrap	forkPalette === "cool-dark" && isDark — where isDark includes system + prefers-color-scheme: dark
forkTheme.ts module-load re-stamp	palette !== null — no dark check at all
resolveAppearanceOption	palette === "cool-dark" && theme === "dark"

Any state with the palette key present and theme !== "dark" makes them disagree — e.g. the tab-A state above, or a writeForkPalette(null) whose removeItem throws (both storage helpers swallow failures silently, so a failed clear survives reload). In that state the app paints Cool Dark, the Appearance select reads "System", and the reset button is hidden because it keys off appearance !== "system". There is then no way to clear it from the UI — re-selecting "System" in the Select won't fire onValueChange for an unchanged value. CLAUDE.md: "Reverse states … A one-way door is a bug."

The guard currently blesses this — expect(resolveAppearanceOption("system", "cool-dark")).toBe("system") codifies the divergence rather than flagging it. Pick one predicate, export it, and use it from all three sites (the bootstrap can't import, but it can mirror one clearly-named expression). Either "the palette applies whenever the document is dark" or "the palette applies only when the stored theme is dark" is defensible — having all three is not.

4. getSnapshot writes to localStorage

getPaletteSnapshot → readForkPalette() → migrateLegacyCoolDarkTheme() → two localStorage.setItem calls. useSyncExternalStore requires getSnapshot to be pure and cheap; React calls it during render and may call it more than once per render. This does a getItem on every render minimum, and in the legacy case performs storage writes from the render phase.

The migration is a boot-time concern. Run it once — the index.html bootstrap already does exactly this migration, so the module-load block is the only other place that needs it — and drop it from the read path.

5. Dark ↔ Cool Dark skips the transition suppression every other theme switch gets

Upstream's setTheme calls applyTheme(next, true), which adds .no-transitions (index.css:858), forces a reflow, and clears it on the next frame — so a theme switch repaints instantly instead of animating every transitioned property in the tree.

setAppearance("cool-dark") from Dark stamps the attribute first (repaint happens there, unsuppressed), then calls setTheme("dark") — which early-returns on lastAppliedTheme, so .no-transitions is never applied. Same on the way back. The result is that Dark↔Cool Dark is the one theme switch in the app that animates a full-tree color transition, which is precisely the "no continuously repainting animations / GPU spike" case CLAUDE.md calls out.

applyForkPalette should do the same add-reflow-rAF-remove dance around its stamp.

6. Manifest no longer watches a file the fork depends on

Dropping shadows: apps/web/src/hooks/useTheme.ts is correct — nothing shadows it now. But forkTheme.ts imports syncBrowserChromeTheme from it, and that file is now absent from files:, shadows:, and watch:, so detect-drift.mjs will stay silent when an upstream sync touches it. TypeScript catches outright removal, so this is about review-time signal on sync PRs rather than silent breakage — but it's exactly what watch: exists for. Add it.

7. Smaller

• COOL_DARK_LABEL is now used (good), but isCoolDarkPalette is exported and called only from inside the module, and UPSTREAM_THEME_STORAGE_KEY is guard-only. Trim the surface.
• Round 1's unguarded index.html body-background literal is unchanged: expect(indexHtml).toContain(COOL_DARK_BACKGROUND) is still satisfied by the JS const alone, so html.dark[data-fork-theme="cool-dark"] body { background: #1c1e20 } can still drift silently.
• The migration is one-way by design, which is right — but nothing tests it. migrateLegacyCoolDarkTheme is the only code path in this PR that rewrites a value a user already has stored, and it's the piece with no coverage at all. Take the storage object as a parameter and it's testable in three lines, in the same node environment as the rest of the guard.
• Reverting SettingsPanels to the relative ../../hooks/useTheme import resolves round 1's type-leak note. Good.

---

Generated by Claude Code