fix(fork): flat white release send button - #22
Conversation
Release builds were still using upstream primary purple; pin a pure white fill and black icon while Dev/Nightly keep channel art. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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. |
There was a problem hiding this comment.
Thermo-nuclear code quality
Not approved — no structural spaghetti or 1k-crossing regression, but this misses an obvious simplification.
Release send is painted three ways (tone attribute + Tailwind white/black + theme.custom.css #ffffff/#000000) for one fill change. The composer-shell manifest already notes fork CSS is unlayered and beats Tailwind utilities, so CSS alone would override bg-primary/90. Keeping all three adds a tone concept and a second source of truth without buying resilience.
Required: pick one paint owner — (1) attribute + CSS, leave Tailwind fill alone, or (2) Tailwind-only and delete attribute/CSS/tone. Also drop the redundant :hover:enabled twin if CSS stays.
theme.custom.css was already >1k; don't grow it for colors Tailwind already sets.
Sent by Cursor Automation: Thermo-nuclear PR review
Review: aggressive passThe visual intent is right and the Dev/Nightly path is provably untouched. But the styling is declared twice, only one copy is live, and the live copy is missing the Verdict: request changes. One blocker, plus a structural argument that most of the 🔴 Blocker — light mode renders an invisible send button
:root[data-fork="noahhendrickson-t3code"]
[data-fork-composer-action="send"][data-fork-composer-send-tone="flat"] {
background: #ffffff;
color: #000000;
box-shadow: none;
}In light mode the composer surface is Before this PR that path was This also contradicts the file's own stated convention in three places:
Fix is one token — add 🟠 The entire
|
| class | overridden by | live? |
|---|---|---|
bg-white |
background: #ffffff |
❌ |
text-black |
color: #000000 |
❌ |
shadow-none |
box-shadow: none |
❌ |
hover:bg-white |
the :hover:enabled rule |
❌ |
Every one of them is unreachable. They are a second, unguarded source of truth for the same four declarations — the new guard asserts the CSS and the attribute, and nothing asserts these. That is exactly the drift shape #13 was written to close, reintroduced.
.fork/AGENTS.md ranks this explicitly: theme/styling → theme.custom.css (rank 3); inline edit of an upstream file → "last resort… keep the hunk small" (rank 4). The only inline edit this change actually needs is the data-fork-composer-send-tone attribute, which is a legitimate CSS hook. Recommend dropping the className ternary edit entirely and letting the ternary keep returning upstream's bg-primary/90 … for the flat case.
🟠 The base-class refactor is pure churn on a watch: file
I diffed the union of base + branch classes on both sides — old and new are the same 27 utilities, so Dev/Nightly is byte-for-byte identical behaviour. Good. But that means hoisting text-primary-foreground shadow-xs enabled:inset-shadow-[…] active:inset-shadow-[…] active:shadow-none disabled:shadow-none out of the shared base bought nothing, while rewriting both arms of a ternary in a file listed under watch: (customizations.yaml:720) — i.e. maximum upstream merge-conflict surface for zero behaviour change.
It wasn't even necessary for the Tailwind-only path: cn() is tailwind-merge, so shadow-xs in base followed by shadow-none in the branch already resolves to shadow-none. Per CLAUDE.md — "Prefer fixing over rewriting", "smallest change that satisfies the request" — this hunk should revert.
🟡 The :hover:enabled rule is a no-op
theme.custom.css:448-452 re-declares the identical background / color as the base rule. Nothing else targets the send button's background on hover — the channel branch uses brightness-110 (a filter), and hover:bg-white loses to unlayered CSS anyway. Delete it, or if it's intended as anti-drift armour, say so in the comment, because as written it reads as a real hover state and isn't one.
🟡 "flat" means "not dev/nightly", not "release"
resolveSidebarStageBackdropVariant (SidebarStageBackdrop.tsx:17-24) returns non-null only for exactly "nightly" and "dev"; everything else falls to null. So stageBackdropVariant ? "channel" : "flat" gives flat white to any unrecognised stage label — a future Beta/Canary/Alpha channel silently inherits the release treatment with no one deciding that.
That also makes the new manifest prose subtly untrue: it now asserts "On a release build the send button is flat pure white" (customizations.yaml:618), but the invariant the code enforces is "on a build with no stage art". intent is supposed to state what must stay true — worth wording it as the latter.
🟡 The guard tests assert formatting, not behaviour
expect(primaryActions).toContain('data-fork-composer-send-tone={stageBackdropVariant ? "channel" : "flat"}');Two problems. It pins an exact source line, so vp fmt wrapping the attribute (one more tone, or a longer expression crossing print width) fails the guard for a formatting reason; and a behaviour-preserving refactor — const sendTone = stageBackdropVariant ? … — fails it too. Compare the sibling guards in this file, which match structure via regex.
More importantly, the guards don't cover the thing that actually broke. Both CSS assertions match on [data-fork-composer-action="send"][data-fork-composer-send-tone="flat"] without anchoring to the :root[data-fork=…]…dark prefix, so they pass identically whether or not the rule is dark-scoped — which is precisely why the light-mode regression landed green. If you take the .dark fix, extend the regex to require it, the way forkSurfacePalette.test.ts anchors on MARKER.
🟡 The CSS comment's rationale doesn't describe what the rule does
"the attribute is set from ComposerPrimaryActions so a missing art wrapper cannot silently leave the release button on upstream's primary purple"
StageBackdropButtonArt only renders when stageBackdropVariant is truthy — i.e. exactly when tone is "channel". A missing art wrapper is not a failure mode the flat rule can be reached by. What the CSS genuinely protects against is an upstream sync reverting the className ternary while the fenced fork:begin/fork:end attribute survives. That's a good reason; it's just not the one written down. In a manifest-driven fork where the prose is load-bearing, an inaccurate rationale is a real defect.
🟡 No pressed affordance on the flat button
Channel keeps enabled:hover:brightness-110 plus both inset shadows. Flat pins the background on hover and zeroes the shadow, leaving hover:scale-105 as the only feedback and nothing at all on :active. Plausibly intended ("flat"), but it's an interaction change riding along with a colour change and isn't mentioned in the description — please confirm it's deliberate.
Verification
- All four boxes in the test plan are unchecked, including
vp test run apps/web/src/__fork_guards__/forkComposerShell.test.ts— the test this PR adds. CLAUDE.md also requires an integrated web pass via thetest-t3-appskill for user-visible frontend changes; nothing here indicates one ran. Given the blocker is only visible when you actually look at the button in light mode, that gap is the proximate cause. - I could not run the suite in this environment (deps unresolved —
vite-plus,@tailwindcss/vite). I executed the three new assertions' regexes directly against the working tree instead: all three pass. The guard is internally sound; it just doesn't assert the scope that matters. - Manifest hygiene is correct —
ComposerPrimaryActions.tsxis already underwatch:and the guard underverify:. No change needed there.
Heads-up
#23 is open against the same base SHA and touches the same three files (theme.custom.css, forkComposerShell.test.ts, customizations.yaml) in different hunks. Should auto-merge, but whichever lands second is worth re-running the full guard file on rather than trusting the merge.
Suggested shape
- Add
.darkto both new selectors. (blocking) - Revert the whole
classNamehunk; keep only thedata-fork-composer-send-toneattribute. - Drop the redundant
:hover:enabledrule. - Anchor both CSS guard regexes on the dark-scoped marker prefix, and match the attribute structurally instead of by exact source text.
- Reword the manifest intent and the CSS comment to state the invariant that's actually enforced.
- Run the guard, and look at the composer in both themes on a release build.
Items 1 and 2 together shrink this to roughly a four-line CSS block plus a one-line attribute — which is the change the fork's rules ask for.
Generated by Claude Code
Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the duplicate Tailwind white/black classes and the hover twin rule so attribute + unlayered CSS are the single paint owner. Co-authored-by: Cursor <cursoragent@cursor.com>
Re-review @
|
- .dark on the flat rule: light builds keep upstream's primary fill instead of white-on-white (re-review blocker 1) - revert the className hoist: base + ternary arms match upstream again, so the flat branch keeps bg-primary/90 as the fallback paint if the fork rule ever fails to match (re-review blocker 2) - guard regexes anchor the dark-scoped marker prefix and the attribute assertion is structural, not source-text - CSS comment and manifest intent state the enforced invariant: flat = no stage-channel art, dark mode only Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-review blockers addressed @ 9548cdbAll six items from the re-review's minimal path:
Integrated verification (test-t3-app, isolated env on the combined tree of PRs 22–25)
Stop button untouched (guard still pins |


Summary
data-fork-composer-send-tone="channel".Test plan
Alphabuild (or force stage label off Dev/Nightly) and confirm send is#ffffffwith a black return arrow, no shadow/purple tint#ea3150)vp test run apps/web/src/__fork_guards__/forkComposerShell.test.tsMade with Cursor