feat(storybook): expose the full brand theme set in the toolbar - #393
Conversation
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 98792eec
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-08-06T08:53:57Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 327.1s (2 bridge agents) |
| Total | 327.1s |
💰 Value — sound
Extends the existing storybook theme-bridge pattern to expose the full brand named-theme vocabulary, with correct per-scope-shape handling (full/partial/unpublished) — every technical claim verified against the real brand CSS.
- What it does: Adds arena, intelligence, tangle-dark, and brand's canonical 'light' to the Storybook theme toolbar (6→10 entries), grouped by darkness. arena joins the existing full-scope CSS bridge; intelligence gets a partial bridge (only the 9 surface tokens its brand scope defines); tangle-dark gets a full bridge with agent-app-.dark-mirroring fallbacks plus a guarded vite alias to a local brand worktree whe
- Goals it achieves: Make the Storybook toolbar reflect the complete brand theme vocabulary so every product skin is previewable in one place, ahead of the brand bump that publishes tangle-dark. Fixes a latent correctness bug too: intelligence is a dark-only PARTIAL scope, so without the .dark class and a partial bridge it either matched nothing or would invalidate un-defined --hsl-* tokens.
- Assessment: A careful, correct change built squarely in the grain established by the prior commit (c1b12a6): the same three files own this concern, and the bridge mechanism is extended, not reinvented. The three scope shapes (full/partial/unpublished) are each handled the right way — I verified all 16 tangle-dark fallbacks byte-match agent-app's .dark scope (tokens.css:283-326), the intelligence partial bridg
- Better / existing approach: none — this is the right approach. Searched for an existing equivalent (rg agentTheme/AGENT_THEMES/named-themes across src/ and fleet storybook configs; checked tests/theme/tokens-contract.test.ts which is token-completeness, not theme switching) and found nothing that already does this. The bridge pattern is the one the prior commit established and is the correct mechanism for re-pointing agent-a
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
A careful, in-grain extension of the existing Storybook theme toolbar to cover the full brand named-theme vocabulary, with correct per-scope cascade handling (partial intelligence scope, fallback'd tangle-dark) and a guarded dev-only alias.
- Integration: Fully reachable: every one of the 64 stories renders through the withAgentTheme decorator (preview.ts:59-81), which reads the AGENT_THEMES map (preview.ts:33-44) the PR extends. Selecting any new theme (arena/intelligence/tangle-dark/light) from the toolbar sets data-theme + toggles .dark on documentElement, which the brand-themes.css bridge blocks resolve. The legacy dark/light remap (preview.ts:
- Fit with existing patterns: Extends the established pattern (AGENT_THEMES map + brand-themes.css cascade bridge + withAgentTheme decorator) rather than competing with it. The intelligence partial-scope block (brand-themes.css:86-97) and the tangle-dark fallback block (brand-themes.css:113-129) are MORE careful than the existing full-bridge list, correctly handling the nuance that intelligence defines only 9 surface tokens an
- Real-world viability: Holds up across environments: the existsSync guard (main.ts:28) makes the machine-local brand worktree alias silently no-op on CI and other machines (falling back to the published package), the tangle-dark var() fallbacks prevent a page of invalid tokens where the scope is absent, and intelligence's .dark-prefixed selector (brand-themes.css:86) correctly matches brand's dark-only scope. The only e
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 The vite alias points at a machine-local absolute worktree path [maintenance] ``
.storybook/main.ts:18-19 hardcodes /home/drew/code/.worktrees/brand-tangle-dark-theme/... — only present on one machine. It IS existsSync-guarded (degrades to published package elsewhere) and marked TEMP with delete-on-bump instructions, and the tangle-dark bridge fallbacks mean the absent case renders as agent-dark rather than broken. So it is safe, but it is a piece of disposable scaffolding that must be remembered on the brand bump. Not a better-architecture issue — the alias is the minimal w
🟡 'light' toolbar entry renders identically to 'agent-light' [proportion] ``
Both map to {dataTheme:'light', dark:false} (preview.ts:39,43), so selecting 'Light · Brand Light' produces the exact same tokens as 'Light · Agent Light'. The PR documents this as intentional vocabulary-mirroring (brand has a canonical light spine; agent-app's tokens.css owns the :root light values so they coincide here). It is observably redundant in this storybook today, but harmless and explicitly justified — flagging only so a reviewer knows the two are the same render.
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 83 | 92 | 80 | 80 |
| Confidence | 75 | 75 | 75 | 75 |
| Correctness | 83 | 92 | 80 | 80 |
| Security | 83 | 92 | 80 | 80 |
| Testing | 83 | 92 | 80 | 80 |
| Architecture | 83 | 92 | 80 | 80 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.
🟡 LOW Bridge contract (every bridged var() must resolve inside the active scope) is unverified by any test — .storybook/brand-themes.css
The full-bridge list claims 'verified: background/foreground/... so none of these resolve empty', but this is hand-verified prose. The failure mode this file exists to avoid is real and silent: agent-app's tokens.css defines zero --hsl-* tokens (rg -- '--hsl-' src/theme/tokens.css: no matches), so a scope added to the full-bridge list that is partial (like intelligence, or a future brand scope) would make the bridged var() guaranteed-invalid and drop the property — colors disappear with no error, no log, no test failure. The repo already pins the identical contract class for tokens.css in tests/theme/tokens-contract.test.ts (every var(--...) a component references must be defined). Add a sibling test: parse brand-themes.css's bridge lists + the imported brand named scopes and assert every
🟡 LOW tangle-dark fallback values are a hand-copied snapshot of .dark and can silently drift — .storybook/brand-themes.css
The 16 fallback values (var(--neutral-16-hsl) background, 239 84% 74% primary/ring, etc.) are duplicated verbatim from the .dark scope in src/theme/tokens.css (lines 283-320). On any machine without the local brand worktree alias (CI, other developers), these fallbacks are the ONLY source for the toolbar's tangle-dark entry. tokens.css's .dark values are actively tuned (the file's own comments document contrast-driven revisions to --primary), so the copy can drift: tangle-dark would then render as a stale agent-dark that diverges from agent-dark itself, with no signal. Fix: a contract test comparing the fallback literals against parsed .dark declarations (same
🟡 LOW Committed machine-local absolute path with a silent-failure guard — .storybook/main.ts
A hardcoded /home/drew/code/.worktrees/brand-tangle-dark-theme/... absolute path is committed to a shared repo. It is guarded by existsSync (line 28) and documented as TEMP with a delete-on-bump instruction, so CI and other machines are functionally unaffected and this is dev-only. The risks: (a) the guard is SILENT — if the worktree is re-checked-out at a different path or the branch is deleted, the alias silently no-ops and Drew's local Storybook quietly diverges from what CI renders (tangle-dark falls back to agent-dark per brand-themes.css, so it degrades benignly but invisibly); (b) after the brand bump, if the block is not deleted (comment-only instruction), the primary
🟡 LOW Hardcoded /home/drew path with no removal tracker — .storybook/main.ts
The absolute machine-local path is documented as TEMP and gated by existsSync, so it is inert on CI and other machines (verified: the path resolves on this workstation only). The only residual risk is the 'delete on brand bump' instruction living in a comment with no mechanical gate. Low-impact if forgotten (one extra fs.stat per storybook boot); mentioning so it is on record. Optional: grep-guard via a CI step that fails when @tangle-network/brand ≥ the tangle-dark release still sees this block.
🟡 LOW One-time startup existsSync: no HMR for external file, no pickup mid-session — .storybook/main.ts
existsSync runs exactly once at config load. The aliased file lives outside the project root (/home/drew/code/.worktrees/...), so Vite's watcher does not watch it: edits to named-themes.css while
pnpm storybookis running do not hot-reload, and if the file is created AFTER the dev server starts (e.g. the brand worktree is checked out mid-session), the alias was already skipped for the whole session — the comment's claim 'picks the scope up the moment it lands there' is only true across a restart. Low impact (dev ergonomics only); fix is to add the file to server.watch force-include or restart after the worktree changes.
🟡 LOW viteFinal alias merge assumes record shape; corrupts array-form alias — .storybook/main.ts
viteConfig.resolve.aliasisAlias[] | Record<string, string>in Vite.{ ...viteConfig.resolve.alias, '@tangle-network/brand/styles/named-themes.css': LOCAL_BRAND_NAMED_THEMES }produces a valid config ONLY when alias is already a record; if the react-vite preset or an addon leaves it as an array, the spread yields{0: aliasEntry, 1: aliasEntry, …, '@tangle-network/…': path}and Vite resolution breaks. Fix: branch on Array.isArray and append{ find: '@tangle-network/brand/styles/named-themes.css', replacement: LOCAL_BRAND_NAMED_THEMES }in the array case. Impact is contained to the one machine where existsSync returns true, so severity stays low — but the merge is silently wrong rather than loud if alias ever ships as an array.
🟡 LOW AGENT_THEMES keys are not pinned to the brand scope list by any test — .storybook/preview.ts
The repo's own doctrine (AGENTS.md: 'a test that cannot fail is worse than no test') and the per-theme
.darkrequirements (intelligence breaks without.dark) make this map's correctness load-bearing for the Storybook. Nothing fails if a future brand bump drops[data-theme='intelligence']from named-themes.css — the toolbar would still set the attribute + .dark class and silently fall back to agent-dark values with no signal. No new test is required to merge this Storybook-only file, but if the team relies on the toolbar to QA theme work, a one-shot test reading named-themes.css selectors and diffing against AGENT_THEMES keys would catch the drift the way tokens-contract.test.ts catches token drift. Cosmetic-only impact keeps this low.
🟡 LOW Legacy 'light' remap shadows the new 'light' toolbar entry — .storybook/preview.ts
LEGACY_THEME_VALUES still has
light: 'agent-light'(line 50). The lookupkey = LEGACY_THEME_VALUES[raw] ?? rawtherefore intercepts the NEW toolbar value 'light' (line 104) and reroutes it to 'agent-light'. Both resolve identically (data-theme='light', no .dark class — verified: tokens.css has no [data-theme='light'] scope so both fall through to :root), so there is no observable rendering bug, but the resolution path the new toolbar item takes is not the one a reader of the comment block ([lines 23-32](https://github.com/tangle-network/agent-app/blob
🟡 LOW New light theme entry is dead code — legacy alias shadows it — .storybook/preview.ts
In withAgentTheme (preview.ts:61)
LEGACY_THEME_VALUES[raw] ?? rawruns BEFORE the AGENT_THEMES lookup, and LEGACY_THEME_VALUES maps 'light' -> 'agent-light' (preview.ts:49). The new toolbar item{ value: 'light', ... }(preview.ts:104) therefore resolves to the agent-light entry and AGENT_THEMES['light'] (preview.ts:43) is never consulted. Today both entries are identical ({dataTheme:'light', dark:false}, and brand's[data-theme='light']scope defines only syntax tokens, not the --hsl-* spine, so the DOM output is byte-for-byte the same — no visible bug). But the doc comment's claim that the map mirrors the brand vocabulary one-for-one is false at the lookup level, and if agent-light's dataTheme or darkness ever diverges from the brand 'light' entry, the legacy alias silently masks i
🟡 LOW light entry in AGENT_THEMES unreachable via toolbar (shadowed by legacy mapping) — .storybook/preview.ts
The toolbar item
{ value: 'light', title: 'Light · Brand Light' }(line 104) is intercepted byLEGACY_THEME_VALUES['light']→'agent-light'at line 61 before reachingAGENT_THEMES['light']at line 43. Thelightkey is dead code in the current flow. Both entries produce{ dataTheme: 'light', dark: false }, so the visual result is identical and no user-facing bug exists. The entry appears to be a forward-compatibility placeholde
tangletools · 2026-08-06T09:28:42Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 10 non-blocking findings — 98792eec
Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-08-06T09:28:42Z · immutable trace
The toolbar listed 6 of the brand named themes; brand 1.1.0 ships arena and intelligence darks that were never wired, and tangle-dark lands in the brand repo next. The toolbar now carries the whole vocabulary, grouped by darkness — Dark: agent-dark (still the default), aubergine, arena, intelligence, tangle-dark; Light: agent-light, aubergine-light, arena-light, tangle-light, light — with the .dark class driven per theme darkness (arena and intelligence are dark; intelligence's brand selector .dark[data-theme="intelligence"] requires the class to match at all). The brand bridge gains arena (full --hsl-* set, joins the main list), a dedicated intelligence block bridging only the nine surface tokens that dark-only partial scope defines — its foreground/primary/ring deliberately fall through to agent-app's .dark tokens, which are the same neutral tangle dark brand intends — and a tangle-dark block whose var() fallbacks render agent-dark until the scope resolves, instead of a page of guaranteed-invalid tokens. tangle-dark exists today only in the brand repo's feat/tangle-dark-theme checkout, so .storybook/main.ts aliases the named-themes.css @import to that file where it exists (skipped elsewhere — CI and other machines keep resolving the published package). Delete the alias and the tangle-dark fallbacks on the brand bump.
…opies
- AGENT_THEMES owns title/dark/dataTheme per theme and the toolbar item
list is derived from it, so the map and the toolbar cannot disagree;
dataTheme defaults to the key (only agent-dark/agent-light rename)
- drop the dead `light` legacy alias: it shadowed AGENT_THEMES['light'],
and the map entry resolves old URLs identically
- tangle-dark joins the full bridge; the decorator probes for the brand
scope and remaps to agent-dark where neither brand >1.1.0 nor the local
alias provides it — loudly, once — replacing 16 fallback literals copied
from tokens.css's dark scope that were free to drift from it
- vite alias appends in record or {find,replacement}[] shape instead of
assuming a record (spreading a list would corrupt every existing alias),
and logs when the local alias is active
98792ee to
16f66f8
Compare
|
signoff PASS 16f66f8 (rebased onto main with the template-pin fix) — all steps ok. Clean tree, Node 22.23.1. |
2 participants
Summary
The Storybook theme toolbar listed 6 themes;
@tangle-network/brand1.1.0 ships named scopes that were never wired (arena and intelligence darks), and tangle-dark lands in the brand repo next (sibling PR,feat/tangle-dark-theme). The toolbar now carries the full brand vocabulary, grouped by darkness:.darkclass handling is now correct per darkness: arena and intelligence are dark themes, and intelligence's brand selector (.dark[data-theme="intelligence"]) matches nothing without the class.lightis brand's canonical light spine — in this Storybook it resolves to the same tokens as agent-light (agent-app'stokens.cssowns the:rootlight values); it's listed so the toolbar mirrors the brand vocabulary one for one. Legacydark/lighttoolbar values still remap, so existing bookmarks keep working.Bridge changes (
.storybook/brand-themes.css)--hsl-*shadcn set, like aubergine/arena-light/tangle-light).background/card/popover/secondary/muted/muted-foreground/accent/border/input+--canvas-backdrop). Putting it in the full list would redeclare the rest against undefined--hsl-*sources and invalidate them; its foreground/primary/ring deliberately fall through to agent-app's.darktokens — the same neutral tangle dark (indigo accent) brand's scope intends ("the accent stays Tangle").var()fallbacks are agent-app's.darkvalues (its own--neutral-*ramp tokens): until the scope resolves, the toolbar entry renders as agent-dark instead of guaranteed-invalid tokens. Once the scope resolves, the fallbacks are dead code.How tangle-dark resolves
feat/tangle-dark-themecheckout but not in a published release. A conditional vite alias in.storybook/main.tspoints thenamed-themes.css@importat/home/drew/code/.worktrees/brand-tangle-dark-theme/packages/brand/src/styles/named-themes.css(the main/home/drew/code/brandcheckout is on an older branch whose tree predatesnamed-themes.css; the worktree is where the sibling PR lands it). The alias is skipped wherever the path is absent (CI, other machines), so nothing breaks elsewhere — those builds resolve the published package and tangle-dark degrades to agent-dark via the fallbacks.@tangle-network/brand, delete the alias block inmain.tsand the tangle-dark fallbacks inbrand-themes.css(both comments say so). The published scope then resolves for everyone.Verification
Gates:
tsc --noEmit✓ ·knip✓ (exit 0; only pre-existing config hints) ·build-storybook✓Alias resolution proven with vite's own resolver: with the alias,
@tangle-network/brand/styles/named-themes.cssresolves to the local brand checkout; without it, to the published 1.1.0 package.Every theme screenshot-verified on the
App Shell/AppShellstory (playwright, 1440×900), saved to/tmp/themes-full/<theme>.pngon the build machine — all legible (canvas, text, muted meta, status chips, composer checked in each):The tangle-dark wiring was additionally proven against a deliberately loud temporary local scope (cyan) before the sibling's scope landed, so a fallback render could not masquerade as a pass; the temp block was removed before committing, and the final screenshot runs the real brand scope.