Remove the warm channel; the SDK has no unbranded value for a second accent - #51
Merged
Conversation
The token map's docblock promises neutral values — "grays + a generic blue, never NimbleBrain brand" — and then sets --nb-color-warm to #d4620a light and #fb923c dark. Those are oranges from a superseded brand generation, and nothing injects that var: the runtime's projection covers accent, processing and heading, but not warm. So every app rendering a warm-toned badge painted that orange in every host, including one whose palette is a single blue accent with no warm channel at all. It resolves to the same generic accent the rest of the map uses, so an unbacked emphasis badge still reads as emphasis without adding a second hue, and a host wanting a distinct warm injects one. tokens.fontHeading had the same shape in type: a Georgia serif fallback meant a host injecting no heading family rendered a display serif. That is the problem 0.13.0 addressed by removing the Fontshare import, one token over. Headings now fall back to the body sans stack — one family, hierarchy from weight and size. Two assertions had codified the serif as the contract. They now check the stack is sans and still carries no brand, current or historical.
The premise this branch started from does not hold. The token map's docblock claims "grays + a generic blue", but it has always shipped danger red, success green, warning amber and processing purple. The rule it actually follows is neutral and generic, never brand — and under that rule a generic orange would have been as legitimate as the amber sitting untouched beside it. What made #d4620a wrong was that it is a brand hex, not that it is orange. Both docblocks now say what the map is. Aliasing warm to the accent was the one answer the corrected premise does not argue for, and it was worse than either alternative. The two became value-identical when unbacked, so a warm badge and an accent badge rendered the same pixels — and in the real host they diverge the wrong way, because the host injects --color-text-accent but not --nb-color-warm. An accent pill takes the host's #0055FF while a warm pill keeps the SDK's #2563eb, which puts two near-matching blues side by side in the memory app's record list. That reads as a rendering bug. So the channel goes. Nothing injects it, the brand it serves has one accent and status hues, and the three real call sites have honest replacements. A library cannot ship a channel it has no unbranded value for. The colour side now has the guard typography got in 0.13.0: every hex in DEFAULT_THEME_VARS must be in a sanctioned set. It fails on #d4620a, which is how this would have been caught on the way in. An allowlist, because a denylist only catches the brand values someone thought to name. Also: the vendored Python IIFE is rebuilt — it is a second shipping path and it was still painting the orange — the published token and badge docs no longer document the removed tone, and one assertion that named a regression it could not catch is gone (the value it forbade ends `serif)`, so `"serif,"` was absent from it too).
mgoldsborough
changed the title
… prose The last round fixed what the Badge page says and left what it renders. BadgeDemo.tsx mounts the real Badge from the package and passed tone="warm"; after the removal TONE[tone] is undefined and Badge destructures it, so the published page's island throws rather than degrading. Nothing in this repo catches that: the docs site resolves the published 0.12.0, where the tone still exists, so the tree is self-consistent today and breaks on the version bump — which is a routine npm update, not a deliberate act. Removing the line now is the only moment it can be done ahead of the break. Three more sites carried the same debris. The page's frontmatter still described seven tones while its body said six, and that string is the meta description and the search-index blurb, so it ships. The preview host still injected --nb-color-warm: #ea580c, inert once nothing reads it but still a retired brand coral sitting in a file. And the docblock added last round pointed readers at theme-defaults.test.ts for the guard, which does not exist — the same failure as naming a generator that was never written, one repo over. The allowlist now states its admission rule rather than only its members. Every sanctioned hue is a stock Tailwind ramp step and every *-light ground a tint of one; the bar is that a value was not chosen for a brand, which is why #7c3aed passes as violet-600 even though a brand uses it, and #d4620a fails as a bespoke accent tuned for one product. Without the rule the next person has a list to append to instead of a criterion to fail.
Three rounds have had the same shape: a reviewer names a file, I fix that file, and the next round names another. The sweep should have been the opening move. It is the closing one now, and it is what found the last of these. Four shipping surfaces still told a reader the heading fallback is a serif. fonts.mdx cited the very page this branch had already corrected — tokens.mdx says the sans stack while the page pointing at it said Georgia. README.md ships in the npm tarball regardless of `files: ["dist"]`, so it is the package page as well as the repo's. heading.mdx and typography.tsx carried the same sentence. Both halves of that sentence were false, and they are the same clause. The reviewer scoped the Erode half out, which is the right instinct in general — but correcting "a serif fallback" and leaving "Erode in the NimbleBrain host" ships a sentence I edited that still misstates the host font, and the runtime has shipped Hanken Grotesk since the palette landed. Both halves go. The neutrality guard added last round read hexes out of a value and checked only what it found, so every other CSS colour notation walked past it: with --nb-color-warning set to rgb(212, 98, 10) — byte-for-byte #d4620a — all nine tests passed. The exact regression the guard exists to prevent, re-entered through a spelling. It now requires a hex literal first, which is what makes the membership check total. Also: the published token page used "Satoshi" as its host-injection example, the one font name on that page and a face this brand retired, read as the real stack rather than the placeholder it is. And the release steps now say that a Breaking entry moves the minor — nothing enforces it, the tag check cannot tell a minor from a patch, and caret-on-0.x not crossing a minor is the whole migration story the CHANGELOG rests on.
Three releases landed on main after this branch's last push: the cascade layer (#50) and its framing (#52), cut as npm v0.14.0, and the Python package's catch-up release (#53), cut as nimblebrain-synapse-v0.4.0. npm 0.14.0 is published, so the warm removal stays under `[Unreleased]` and releases as 0.15.0. The migration note's unaffected range widens to `^0.11.0`-`^0.14.0` to match what consumers can now be pinned to. The vendored Python IIFE is rebuilt from the merged source rather than resolved to either side, since #50 changed the build output. Both nimblebrain-synapse-v0.4.0 and v0.14.0 are tagged at commits that predate this merge, so both shipped artifacts still carry the warm channel their changelogs describe. theme-defaults.ts auto-merged: #50's cascade-layer docblock and this branch's correction to the neutrality claim are different hunks.
The colour neutrality guard covered DEFAULT_THEME_VARS only. #d4620a lived in both maps, and `tokens` is the more load-bearing of the two: DEFAULT_THEME_VARS is the block the SDK injects, while the `tokens` var() fallbacks are what every component resolves against when a host declares nothing. Guarding one left the other free to reintroduce exactly what the test exists to catch, under a docblock making the same unbranded claim with nothing behind it. Value-neutral today — all fifteen hexes in the fallbacks are already sanctioned. Mutation-verified: #d4620a planted at tokens.warning fails it. No anchor on this loop, and the extracting match() is load-bearing for it: these values are `var(--token, <fallback>)` strings rather than bare hexes, so the single-iteration collapse that the anchored loop above would allow is off the table here.
`main` carried 0.14.1 into the two files this branch also rewrites, so the merge needed deciding rather than resolving. `CHANGELOG.md`: this branch's entry was sitting under `## [Unreleased]`. It removes the `warm` tone and the `warm` / `warmLight` tokens, so it is a minor, not a patch — and 0.14.1 is already released and must keep its own heading below. Retitled `## [0.15.0]` and stacked above it. `_assets/synapse-ui.iife.js`: both sides re-vendored the bundle from their own source, so neither side is correct for the merged tree. Rebuilt from this tree instead — the only resolution that can be right, and the one CI's freshness diff checks. `src/theme-defaults.ts` auto-merged: this branch edits the module docblock and the two colour maps, 0.14.1 added a capability guard and touched the inline write, and the regions do not meet. `npm run ci` green — 499 tests, with both changes present: no `--nb-color-warm` in the defaults, and `canInstallStylesheet` still guarding the layer install.
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
1 participant
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.
theme-defaults.tspromises unbranded fallbacks and shipped--nb-color-warm: #d4620a/#fb923c— hexes from a retired NimbleBrain brand generation (BRAND_BOOK.md:26, under "Human action, energy").Nothing injects that var. The runtime's iframe projection emits ten colour vars — accent-foreground, danger, success, warning, processing, processing-light, info-light, the three
--color-*groups — and--nb-color-warmis not among them. So every app rendering<Badge tone="warm">painted that orange in every host, including one whose brand has a single blue accent and status hues and no warm channel at all.The channel is removed rather than re-valued. A library cannot ship a channel it has no unbranded value for, and the neutral answer to "a second accent" is that there isn't one.
tone="warm",tokens.warmandtokens.warmLightare gone; migration is in the CHANGELOG. Consumers pinned^0.11.0–^0.13.0are unaffected until they bump.tokens.fontHeadinghad the same shape in type — aGeorgia, 'Times New Roman', seriffallback rendered a display serif on any host injecting no heading family. That is what removing@nimblebrain/synapse/ui/fontsaddressed in 0.13.0, one token over. It now falls back to the body sans stack.New guard. Every hex in
DEFAULT_THEME_VARSmust be in a declared sanctioned set. Typography got a guard in 0.13.0; colour had none, which is how the orange survived two releases under a docblock claiming the values were unbranded. Mutation-verified: putting#d4620aback fails it.CI green on GitHub — all seven jobs, including the vendored-IIFE freshness diff.
Adjudication
Round 1 — 4 criticals, 2 delete-instead, 2 suggestions. All confirmed; none rejected.
python/nimblebrain_synapse/_assets/synapse-ui.iife.jsstill embedded#d4620a,ci.yml:73diffs it against a fresh build, and the Build job was failing. Rebuilt and re-vendored. My "bun run ci— 486/486 ✓" was literally true and materially misleading: that script is lint/typecheck/build/test and does not include the freshness diff. I reported a suite instead of checkinggh pr checks, on a PR whose whole subject is a second distribution path shipping stale values.tokens.mdxlisted#d4620aunder a column headed Neutral fallback — the docs site was asserting the exact claim this PR contradicts.badge.mdxdocumented the tone too; both updated for the removal.DEFAULTinjects--nb-color-warm: #ea580cwhile its own comment says "neutral, unopinionated".#ea580cis listed inBRAND_BOOK.mdunder What We Don't Use. Moot now — the var is dead, so all 16 warm entries are dropped from the presets andgallery/App.tsxno longer swatches a token that does not exist.dangerred,successgreen,warningamber andprocessingpurple have always been there. The real rule is neutral and generic, never brand, and under it a generic orange would have been as legitimate as the amber beside it. What made#d4620awrong is that it is a brand hex, not that it is orange. Both docblocks now describe the map as it is.warmto the accent is the one option the corrected premise does not argue for, and it is worse than either alternative: value-identical when unbacked, and divergent in the real host, because--color-text-accentis injected (projections.ts:48→#0055FF) and--nb-color-warmis not. Two near-matching blue pills inApp.tsx:292besideformat.ts:56's"note" → accent. Verified all three consumer sites live inplatform/mcp-servers/memory/ui, pinned^0.11.0.not.toContain("serif,")— the forbidden value endsserif), so the guard passed on the exact regression it named. Removed; line 30'stoContain("system-ui")was doing the work.#d4620a.--color-text-accentamong them, which is what makes the alias collide).Still open, filed not fixed
platform/mcp-servers/memory/uikeepstone="warm"atApp.tsx:292andRecordDetail.tsx:119andtokens.warmatforms.tsx:269–275. It is pinned^0.11.0, so caret will not carry it across this minor and nothing breaks today — but it needs the repoint before it bumps: accent for the two pins,warningfor the forms caution ("Saving replaces the whole record"). Separate repo, separate PR.Round 2 — 1 critical, 3 suggestions. All confirmed; none rejected.
web/src/components/ui/BadgeDemo.tsx:14— a live island mounting the realBadgefrom the package, passing the removed tone.TONE[tone]is undefined andBadge.tsx:27destructures it, so the published page's preview throws rather than degrading. This is what contradicts my "FIXED the published docs": I fixed what the Badge page says and left what it renders.web/resolves the published0.12.0(verified —node_modules/@nimblebrain/synapseis 0.12.0), where the tone still exists, so the docs tree is self-consistent today and breaks on the version bump.astro checkwouldn't help either, for the same reason. Removing the line now is the only moment it can be done ahead of the break, not a gate problem to solve.badge.mdx:3frontmatter still read "seven token-driven tones" against a body saying six — and that string is the page meta description and the Starlight search-index blurb, so it ships.web/src/components/ui/tokens.tspreview host still injected--nb-color-warm: #ea580c/#fb923c. Inert once nothing reads them, but#ea580cis the retired coral.theme-defaults.test.tsfor the guard. No such file; it is__tests__/ui/tokens.test.ts. I wrote a pointer to something that does not exist, in the same review round that a sibling PR was caught naming a generator that was never written.#7c3aedis listed inBRAND_BOOK.mdas a platform accent and it stays. The rule that holds is was this value chosen for a brand: every sanctioned hue is a stock Tailwind ramp step (blue-600, red-600/400, emerald-600/400, amber-500/400, violet-600/400, indigo-400) and every*-lightground a tint of one, so#7c3aedpasses as violet-600 off the shelf while#d4620afails as bespoke and brand-tuned. Without the criterion the next person has a list to append to instead of a test to fail.[Unreleased], per this repo's release-time-bump convention. The release commit must move the minor: caret-on-0.xnot crossing a minor is the entire migration story that entry rests on, and cutting it as a patch would carry the removal to every pinned consumer silently. (Round 4: 0.14.0 was taken by Make the default theme a default: ship it in a cascade layer #50/Say what 0.14.0 stops masking, not what it introduces #52 and is published, so this is 0.15.0.)CI green on GitHub — 7/7, including the vendored-IIFE freshness diff and the docs-site build.
Round 3 — 1 critical, 3 suggestions. All confirmed; none rejected.
The reviewer's meta-finding is the real one and I'm recording it as such. Round 1 fixed
tokens.mdx+badge.mdxafter a reviewer named them. Round 2 fixedBadgeDemo.tsxafter a reviewer named it. Round 3 named three more. Every fix went to the file in the finding rather than to the value, andgrep -rn "Georgia" .at round 1 would have returned all of them. The sweep is now the closing step, and it found one more the reviewer's own grep did not.fonts.mdx:55cited the very page this branch had already corrected —tokens.mdxsaid the sans stack while the page pointing at it saidGeorgia.README.md:125ships in the npm tarball regardless offiles: ["dist"], so it is the package page too. Plusheading.mdx:9andtypography.tsx:4.Erodehalf ofheading.mdx:9/typography.tsx:4to a separate PR, and that instinct is right in general. But both halves are the same clause — "Erode in the NimbleBrain host, a serif fallback elsewhere" — so correcting one and leaving the other ships a sentence I edited that still misstates the host font, which has been Hanken Grotesk since the palette landed. Both halves go. Calling this out rather than burying it: it is an expansion, and the reason is that the alternative is knowingly shipping a half-true sentence.--nb-color-warning: "rgb(212, 98, 10)"— byte-for-byte#d4620a— all nine tests passed. The exact regression the guard exists to prevent, re-entered through a spelling. It now requires a hex literal before checking membership, which is what makes the membership check total. Mutation-verified both ways.tokens.mdx:30used"Satoshi"as its host-injection example: the one font name on the published token page, a face this brand retired, read as the real stack rather than the placeholder it is. Now"Your Brand Sans", which makes the point better anyway.CLAUDE.md's release steps, where the person cutting the release is already reading — not just in this body. Nothing enforces it; the tag check cannot tell a minor from a patch.src/ui/tokens.ts.CI status — not claiming green
Locally:
bun run ci487/487 ✓, docs site build ✓, vendored IIFE byte-identical to a fresh build ✓, both guard mutations verified ✓.On GitHub, no workflow has run against
ea9f38f.check-runsfor the head commit returns 0, and closing/reopening the PR did not trigger one; the last run on this branch is round 2's14dee3ce. I do not know why — there is nopaths:filter onci.ymland the push landed cleanly. Flagging it rather than reporting a local suite as CI, which is the round-1 finding on this PR. Needs a green run before merge.Round 4 — merged
main; the one blocker was mergeability, not code.The open question at the end of round 3 — why no workflow ran against
ea9f38f— was that the branch had goneCONFLICTING. #50 merged at 15:48 HST, between round 2's green run (15:19) and the round-3 push (15:53). GitHub buildspull_requestruns againstrefs/pull/N/merge; with no computable merge ref, nothing queues, and reopening cannot help.Merged rather than rebased: three commits each touch the CHANGELOG and the generated IIFE, so a rebase re-resolves the same conflict three times.
CHANGELOG.md— the removal stays under[Unreleased]above[0.14.0]. The migration note's unaffected range widens^0.11.0–^0.13.0→^0.11.0–^0.14.0.python/…/synapse-ui.iife.js— rebuilt from the merged source, not resolved to a side. Make the default theme a default: ship it in a cascade layer #50 changed the build output, so both sides were stale against the merge; picking either fails the freshness diff.src/theme-defaults.ts— auto-merged. Make the default theme a default: ship it in a cascade layer #50's cascade-layer docblock and this branch's correction to the neutrality claim are different hunks.The release-ordering hazard closed on its own. #53 (
nimblebrain-synapse-v0.4.0) merged mid-review, and PyPI ships whatever_assets/synapse-ui.iife.jsis onmainat tag time. Had this PR merged first, that release would have carried the warm removal under a changelog documenting only the cascade layer andfontFaces. Both tags are cut at commits predating this merge —v0.14.0atd662fcb,nimblebrain-synapse-v0.4.0at5c085c8— and the tagged Python asset still contains--nb-color-warm, so both artifacts match their notes. Verified, not assumed.Also moot now: the earlier note that
CLAUDE.md's release steps omit__client_version__. Main bumped it to 0.14.0 alongsidepackage.json, so the pin gate passes on the merge.Verified on the merged tree:
npm run ci497/497 (36 files, up from 487 — main adds 10), lint clean, vendored IIFE byte-identical to a fresh build, version pin0.14.0 == 0.14.0, docs site 57 pages with all internal links valid. Re-swept forwarm/Georgia/Erode/#d4620a/#ea580cacross the merged tree — the only hit is the guard asserting their absence.One carry-over, unchanged from round 4's review: in
src/__tests__/ui/tokens.test.ts, the anchored/^#[0-9a-f]{3,8}$/imakes the innermatch(/g)loop provably single-iteration, so those five lines collapse to oneSANCTIONED.has(value.toLowerCase()). The generality was load-bearing before the anchor; it isn't now. Non-blocking.