chore(ui): prune the unused vendored color tokens - #183
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/update-color-tokens #183 +/- ##
=========================================================
Coverage 68.57% 68.57%
=========================================================
Files 210 210
Lines 8593 8593
=========================================================
Hits 5893 5893
Misses 2700 2700 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
18f6421 to
f735efa
Compare
renefloor
changed the title
b1c76eb to
c662af3
Compare
| static const baseTransparent0 = Color(0x00FFFFFF); | ||
| static const baseTransparentWhite10 = Color(0x1AFFFFFF); | ||
| static const baseTransparentWhite20 = Color(0x33FFFFFF); | ||
| static const baseTransparentWhite30 = Color(0x4DFFFFFF); |
There was a problem hiding this comment.
do we not want white 30 and black 70 in stream colors? I think its good to have the primitive colors even if we are not using them in themes. Or maybe better to only have these and remove the different ones from StreamColors
There was a problem hiding this comment.
Good catch, and the second half of it turned out to be the bigger one.
Agreed on keeping the primitives — but the token file is the wrong home for that. internal/tokens/ is excluded from the barrels and the class has a private constructor, so a constant left there unreferenced is not "available even though themes don't use it", it is unreachable by anyone. StreamColors is the public primitive palette, so that is where a primitive has to surface. Added white30 and black70 there, which also makes them referenced, so check:tokens stays honest without an allowlist entry.
On "remove the different ones" — they are not different, which is what makes this worth doing. All three baked hexes in StreamColors match an upstream token exactly:
| member | value | upstream token |
|---|---|---|
white50 |
0x80FFFFFF |
baseTransparentWhite50 |
black50 |
0x80000000 |
baseTransparentBlack50 |
black75 |
0xBF000000 |
baseTransparentBlack75 |
Every other member of the class already read a token. So these three were the same baked-hex-instead-of-a-token hazard the skill warns about for component defaults, one layer up in the public palette — the values happened to be right, with nothing keeping them right. They read their tokens now.
Net: five tokens added to both modes, StreamColors has no Color(0x…) literals left, and light/ and dark/ stay in agreement at 348 each.
One thing I did not do: vendor all 19 transparent primitives upstream publishes. That widens the public API permanently and felt like your call rather than something to fold into a prune — happy to add the rest if you want the full set.
backgroundElevationElevation0..3 are unread, and the names no longer exist upstream — the core/chat/video namespace split renamed them to background/core/elevation-N, which generates as backgroundCoreElevation0..3. Carrying an unread constant under a name the source of truth has retired is worse than not carrying it: it reads as the current name to anyone checking whether a token is vendored. The colorScheme fields that mirror them (backgroundElevation0..3) resolve through chrome[0] and are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
124 unread color constants for badge, button, avatar, control, chat, input, reaction and presence components. They are dead by architecture, not by neglect: this package vendors only the root semantics, and a component's values are re-derived from those in its own defaults. StreamBadgeNotification reads colorScheme.accentError rather than a badgeBgError constant, so upstream's derived tokens have no counterpart here on purpose. Keeping them invited the opposite reading — that a component should reach for a token constant — which is the one thing a component must not do, since a constant bypasses the seedable color scheme and a custom brand or chrome would stop applying. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
31 unread constants whose StreamColorScheme field resolves through the generated brand or chrome swatch instead — textPrimary through chrome.shade900, accentPrimary through brand.shade500, backgroundCoreApp through chrome[0]. That indirection is deliberate: the swatches are regenerated from a seed color, so resolving a field through the swatch is what lets StreamColorScheme.light(brand: ...) actually repaint the SDK, where a baked hex would silently ignore it. The constant is therefore redundant by construction, and the field is the single source. Verified before deleting that this is redundancy and not a competing value: all 29 mechanically resolvable pairs hold the same color as the swatch shade their field reads, in both light and dark. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sync-state note named `backgroundElevationElevation0` and `avatarPaletteBg1` as constants that survive only here, and said `light/` and `dark/` do not hold the same set. This PR deletes the first two and makes the sets identical, so both claims are now false. The relationship they described is also simpler than it was: with the unread constants gone, the vendored set is a strict subset of upstream — every name here exists upstream under the same spelling. That is worth stating, since it is the property a future sync can actually check. The naming table had the same problem one layer down: it taught the group-segment transform using `borderUtilityWarning` and `backgroundCoreOnAccent`, both deleted here. Swapped for `borderUtilitySelected` and `backgroundCoreHighlight`, which survive precisely because a field reads them — so the table now demonstrates the rule it sits next to rather than contradicting it. Also two American-English fixes: `recognise` (mine, from the previous PR) and `Multicolour`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review asked whether `white30` and `black70` should survive the prune. Looking at why they had no reader turned up a larger inconsistency: three members of `StreamColors` were baked hexes, and each one exactly equals an upstream token nobody had vendored. white50 0x80FFFFFF = baseTransparentWhite50 black50 0x80000000 = baseTransparentBlack50 black75 0xBF000000 = baseTransparentBlack75 Every other member of that class already reads a token, so these three were the baked-hex hazard the skill warns about, sitting in the public palette. They now read their tokens. `white30` and `black70` come back for the same reason, as public members rather than as unread constants — the vendored files are internal, so a constant left there unreferenced is not "available to users", it is invisible to everyone. Exposing them through `StreamColors` is what makes them reachable, and it keeps them referenced, so the check stays honest without an allowlist entry. Five tokens added to both modes, ordered with their neighbours. No baked hexes remain in `StreamColors`; light/ and dark/ stay in agreement at 348 each. Not done: vendoring all 19 transparent primitives upstream publishes. That widens the public API permanently, which is the owner's call rather than something to fold into a cleanup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3a85f80 to
90d3b2e
Compare
2 participants
Stacked on #182 — review and merge that first; this PR's base is
feat/update-color-tokens, so its diff is only the cleanup.stream_tokens.dartcarried 502 constants of which 163 were read. This PR removes the 159 unread color constants (158 inlight/, 159 indark/— see the asymmetry note below). The dimension constants are deliberately left alone — see the note at the bottom.The unread constants were unreachable as well as unread: the files sit under
internal/, are excluded from the barrel contract, and are imported by exactly two files —stream_colors.dart(light only) andstream_color_scheme.dart(both). Nothing public is touched.One commit per category, so a bisect stays meaningful:
c46b2dabackgroundElevationElevation0..3— unread, and the names no longer exist upstream after the core/chat/video namespace split.1f12b4bStreamColorSchemefield.StreamBadgeNotificationreadscolorScheme.accentError, not abadgeBgErrorconstant.c662af3textPrimary ??= chrome.shade900). Verified against the resolved swatch shade in both modes before deleting — redundancy, not a competing value.38f70623a85f80Every
StreamColorsprimitive is now token-backedReview asked whether
white30andblack70should survive the prune. Checking why they had no reader turned up a larger inconsistency: three members ofStreamColorswere baked hexes, and each exactly equals an upstream token nobody had vendored.StreamColorswhite500x80FFFFFFbaseTransparentWhite50black500x80000000baseTransparentBlack50black750xBF000000baseTransparentBlack75Every other member already read a token, so these three were the baked-hex hazard one layer up, in the public palette. They read their tokens now, and
white30/black70come back as public members rather than unread constants — the vendored files are internal, so a constant left there unreferenced is not "available to users", it is invisible to everyone. Exposing them is what makes them reachable and keeps them referenced.Five tokens added to both modes; no baked hexes remain in
StreamColors.Not done: vendoring all 19 transparent primitives upstream publishes. That widens the public API permanently, which is the owner's call rather than part of a cleanup.
Also fixes a light/dark asymmetry
dark/carriedborderUtilityFocus(0x4078A8FF) with no counterpart inlight/— 501 light vs 502 dark. Upstream has onlyborder/utility/**focused**({brand.150}), which generates#C3D9FFlight /#19418Ddark, exactly whatborderFocus ??= brand.shade150already resolves to. So it was a stale name carrying a stale value, and nothing read it.Both files now declare 348 identical constants. That symmetry is what lets the
check:tokensguard in the follow-up PR pass.Verification
analyzeis the load-bearing check here: these arestatic constmembers reached only through a prefixed import, so a deleted-but-used constant is anundefined_gettercompile error, not a lint. It passes across all four packages, and--ignore="*example*"does not exclude the gallery, so that is analyzed too.The deletion is exactly minimal rather than approximate: 168 color constants remain and all 168 have a reader. No deleted name has a reference anywhere in the repo — including
apps/,scripts/, tests, and the tracked generated output.format:verifyandcheck:barrelspass.flutter testis+397 -56, but that is a null result rather than evidence: no test, golden, or source file changes, so the count could not have moved. The 56 are the pre-existing macOS-golden drift.Note if you grep this yourself: a bare-name search for
accentPrimaryortextPrimaryreturns ~1100 hits, because those are alsoStreamColorSchemefield names. Every hit iscolorScheme.X; the only reference form for a token isStreamTokens.X.Scope: colors only
The "every constant has a reader" property now holds for colors. 180 dimension constants (
size*,space*,radius*,font*,lineHeight*,typography*, and the component dimension groups) remain unread, deliberately.An earlier revision of this branch deleted those too, on the grounds that the package does not sync dimensions from upstream. That was wrong — those values mirror upstream exactly, ordering included, and typography specifically comes from the
webflavor, the only one carrying theGeistfamily this package ships.They are unreferenced for a different reason:
StreamSpacing,StreamRadiusandStreamTokensTypographyhard-code the same numbers instead of reading the constants. That is the swatch-versus-baked-hex hazard one layer up — nothing ties the class to the constant it mirrors, so the two can drift silently.Deleting them would remove the evidence of that gap rather than the gap, so it is handled separately by wiring those classes to the constants. The
check_tokens.dartguard that fails CI on an unreferenced token moved to that PR too, since it can only pass once the dimensions are wired.This also resolves an apparent tension in the skill: "do not vendor
button/*" governs button colors, which this PR drops, while the dimensions carve-out keepsbuttonPaddingX*and friends.No changelog entry
Deliberate. The policy is conditional on changing package behavior, and nothing read these constants.
StreamTokensis internal — private constructor, underinternal/, listed incheck_barrels.yaml'sinternal_dirs, and exported from no barrel — so the removals are outside the deprecation policy too.