Repository navigation
Give chartreuse clear color rules for links, mentions, selection and unread - #1309
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) lanes.
Reviewed head da95f354171dea00723bf83467f6c0b29b31fb88 against base a3752f0.
No concrete correctness, security, or agreed-contract defect requiring changes found. Source assessment: 9/10 for minimalness, ownership and correctness. The shared link-role move preserves own-message pairing; removing the sidebar override preserves unread ink without changing selection, preparation or focus handlers. The near-black guard adjustment matches the stated product policy. One optional browser-coverage improvement is inline.
Public description and commit material inspected; no attached images and no disclosure finding in those surfaces.
Validation limits: source-only, immutable Git objects; no PR code, tests, builds or app executed. The hosted check snapshot at this head has CI required failed, with WebKit shards 2/9 and 6/9 failed; their causes were not investigated. Windows native validation was skipped. Rendered contrast, both themes/viewport sizes/enlarged text, keyboard/pointer behavior, native acceptance and human testing remain unverified here. The PR description also records deferred visual/human validation. This COMMENT is not approval or a merge-readiness claim.
| return color; | ||
| }); | ||
| await expect(ordinaryLabel).toHaveCSS("color", unreadInk); | ||
| await ordinary.hover(); |
There was a problem hiding this comment.
🤖 Optional coverage improvement: this row is already selected (aria-current=page at line 214), so hovering it retains the selected fill and does not exercise the unselected-hover branch in ChannelSidebarRow.module.css:28–31. Consider asserting the unread color on a separate unselected unread row while hovered. The existing selected-state assertion is useful; this is a coverage suggestion, not a demonstrated product defect.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/concurrency), and Groot (product/accessibility) lanes.
Reviewed head 73dd32a6a108b403ce16fec3c975ec056cd977ed against base a3752f0. This follow-up covers the two browser-test files changed since reviewed head da95f354171dea00723bf83467f6c0b29b31fb88; the production patch is unchanged.
No new actionable findings. Source assessment: 9/10. The reconnect test now settles the thread scroller, drains admitted read-state writes before the deliberate outage, recovers a reply withheld from live delivery, and waits for completed arrival accounting before checking focus, draft and reading position. The drag test waits for the actual insertion cue to clear before releasing the pointer. Existing keyboard/assistive activation assertions remain intact. No browser cases were added or removed.
The public PR description and both commit messages were inspected; no attached images or disclosure finding in those surfaces. The previous optional unread-hover coverage suggestion remains optional and is not duplicated here.
Validation limits: source-only; no PR code, tests, builds or apps executed. The one hosted-check snapshot still had four browser shards running; completed checks were successful except Windows native validation was skipped. Completed browser-job wall times were roughly 4–9 minutes, but check outputs supplied no test-count, summed-execution or slowest-test summaries; this review does not establish test-cost parity or eliminate flakes. The author reports 66 local browser cases passing in 2.2 minutes; that report was not independently reproduced. Rendered contrast, themes/viewports/enlarged text, native behavior and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) lanes.
Reviewed head ba1a37ffdc602d2285d6f2a3b32bbc1c6314cf55 against base 7d50425. This follow-up covers new commit 20ba36b58 and its integration with main since the previously reviewed 73dd32a6; unchanged feedback is not duplicated.
Two low-priority visual-state defects are inline. Source assessment: 8/10 pending those small state-mapping repairs. The implementation keeps shared semantic-role ownership, profile actions, unread styling and focus handlers. No security or concurrency defect found in the new changes.
Public description and new commit messages inspected; no attached images or disclosure finding in those surfaces.
Validation limits: source-only, immutable Git objects; no PR code, tests, builds or app executed. The single hosted-check snapshot had JavaScript, Rust, browser measurements/journeys and flake checks still running; completed non-skipped checks were successful, and Windows validation was skipped. No completed test-count, summed-execution or slowest-test evidence was available from that snapshot; no cost-parity or flake-elimination claim. Author-reported runtime tests were not independently reproduced. Rendered contrast, narrow/wide layouts, enlarged text, keyboard/pointer and error/retry focus behavior, native acceptance and human testing remain unverified here. This COMMENT is not approval or merge authorization.
| --text-mention: var(--text-standard); | ||
| --affordance-mention-self: var(--neutral-6); | ||
| --affordance-mention-self-hover: var(--neutral-5); | ||
| --text-mention-self: var(--affordance-prominent-mark); |
There was a problem hiding this comment.
🤖 [P3] Preserve the self-mention distinction in dark Snoozin’
At the neutral/Snoozin’ level, --affordance-prominent-mark resolves through --affordance-prominent to --neutral-12, which is also --text-standard. Both ordinary and self mentions therefore use neutral-12 ink on neutral-6, with the same neutral-5 hover fill. In a received message, a mention of the viewer looks exactly like a mention of someone else, despite the PR’s stated attention distinction and DESIGN.md’s “Link contrast” rule. Give the neutral dark self-mention pair a distinct, readable treatment while retaining the neutral palette; cover this level with an assertion that self and ordinary treatments differ.
| /* A highlight would vanish into the brand-filled bubble: invert the | ||
| bubble's own pair so the mention reads as a chip on it. */ | ||
| --affordance-mention-own: var(--text-message-own); | ||
| --affordance-mention-own-hover: var(--text-message-own); |
There was a problem hiding this comment.
🤖 [P3] Keep a distinct hover state for own-bubble mentions at Buzzin’
Here and in the dark full-level block at line 1397, the own-mention hover fill is identical to its resting fill. Messages.module.css passes these through --mention-fill/--mention-hover, so hovering a clickable profile mention inside an outgoing Buzzin’ bubble produces no fill change in either theme. That leaves this surface outside the PR’s promised chip-hover repair. Choose a distinct paired hover treatment in both full-level blocks and extend the browser assertion to an own-message bubble; retain the existing readable inverted resting pair and keyboard ring.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) reviews.
Reviewed head db615c87ae38863d607f03c6a042b07b7e1d8b39 against base 7d50425. This follow-up checks the 11-file delta from previously reviewed ba1a37ffdc602d2285d6f2a3b32bbc1c6314cf55 in the full feature context.
One new P2 is inline: selected Solo overflow actions lose their glyph contrast on hover, press, and while their menu is open for light saturated accents. Overall 8/10 pending that paired-state fix and the existing mention repairs. The semantic token ownership and reuse of the unread-edge state are appropriately scoped.
The previous dark Snoozin’ self-mention distinction and Buzzin’ own-mention hover P3 findings remain unchanged. Those comments are not duplicated here.
Optional documentation consistency: DESIGN.md’s Rows entry still says selected unread labels remain white in dark mode, while ChannelSidebarRow.module.css:118 now intentionally uses the selected accent ink and retains semibold weight. Align that sentence with the intended selected-row rule.
Validation: immutable source and callers, CSS cascade, focus/activation and failure/recovery paths, tests as source, and public PR description/new commit were inspected. No attached images in the description and no actionable public-material disclosure found. No PR code, tests, builds, or apps were executed. One hosted-check snapshot showed JavaScript, Rust, browser journeys and several flake lanes still running; completed non-skipped checks were successful, Windows was skipped. This is not an all-green CI or runtime acceptance claim. Rendered contrast, keyboard/pointer behavior, narrow/wide layouts, enlarged text, and native acceptance remain unverified. The follow-up adds no browser cases; test cost and flake elimination were not measured. This is non-blocking COMMENT feedback, not approval or merge authorization.
| accent, and row actions take the same ink as the label. */ | ||
| .sidebar [data-selected], | ||
| .sidebar :has(> * > [aria-current="page"]) { | ||
| --text-standard: var(--text-row-selected); |
There was a problem hiding this comment.
🤖 [P2] Pair selected row-action ink with its interactive fills
In light Flame (also the other white-ink accents) at Hummin’/Swarmin’, this override makes the selected Solo chat’s overflow glyph white. The live caller is SoloProjects.tsx:817–827: its ghost IconButton is a sibling of the DraggableChannel wrapper containing the selected NavigationItem, so the new :has selector reaches the action. But the existing rules in this same stylesheet at lines 99–108 still paint ghost hover, active, and aria-expanded states with --affordance-panel-hover (#f5f5f6 at these levels). The icon therefore becomes white on near-white when hovered/pressed and stays that way while its menu is open. Buzzin’ likewise pairs the white glyph with a pale accent-3 fill. This violates DESIGN.md’s Contrast/State requirement to keep a control identifiable on its actual interactive fill.
Give selected ghost actions a paired hover/pressed/expanded treatment (fill and ink together), and cover a selected Solo overflow action using a white-ink accent in those states. Preserve the readable resting selected pair and focus ring.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) lanes.
Reviewed head 902d5a573415614e3642f6fc9ea0ef0973cd3543 against base 7d50425. The follow-up from db615c87ae38863d607f03c6a042b07b7e1d8b39 changes only ThemePicker.tsx and settings-controls.css: one decorative sidebar stroke now samples the selected-row color in each preview mode.
No new actionable defect in this delta. Theme scoping resolves the appropriate light fill/dark ink, and radio labels, keyboard selection, persistence and retry ownership remain intact. The delta is appropriately small. Overall source assessment remains 8/10 because the existing selected Solo overflow contrast P2, dark Snoozin’ self-mention distinction P3, and Buzzin’ own-mention hover P3 are unchanged. Their repair advice remains on the original comments; this review does not duplicate them.
Optional visual refinement: at Snoozin’, the new light-preview stroke samples neutral-3 (#e8e8e8) on the white panel, so it is much fainter than the neighboring metadata strokes. Consider a more legible neutral depiction if that detail should remain prominent. This is a decorative thumbnail sampling the real row fill; labelled radios and the selection outline remain available, so this is not an accessibility violation or required change.
Public PR description and new commit inspected; no attached images or actionable disclosure found in those surfaces. Source-only: no PR code, tests, builds or apps executed. The single hosted-check snapshot had 33 checks running, one queued, five successful, and Windows native validation skipped. Author-reported tests and desktop inspection were not independently reproduced. Rendered contrast, keyboard/pointer and error/retry focus behavior, narrow/wide layouts, enlarged text and native acceptance remain unverified. No tests changed in this follow-up; test cost and flake elimination were not measured. This is non-blocking COMMENT feedback, not approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) lanes.
Reviewed head 23a02e2c47f9fb913004c4455c770dd65b2e14d3 against base 7d50425. This follow-up checks the 13-file delta from previously reviewed 902d5a573415614e3642f6fc9ea0ef0973cd3543 in the feature and caller context.
No new actionable findings. The previous selected Solo overflow contrast and own-bubble mention hover issues have source-level repairs: bare actions retain the row’s paired fill/ink across hover and menu-open states, and own mentions now have distinct resting/hover fills. The selected-row badge pair, retained label clipping and touch trailing layout are consistent in source. The documented Swarmin’/Buzzin’ rebalance is an intentional product change.
The existing P3 dark Snoozin’ self-mention distinction remains: tokens.css:644–649, 662, 694 and 700 still resolve ordinary and self mentions to the same neutral fill, hover and ink in dark neutral mode. The original repair advice remains applicable; no duplicate inline comment is added. Overall source assessment: 8/10 pending that remaining distinction. The implementation otherwise keeps these changes with the existing semantic-token, shared-button and row-layout owners.
Validation: immutable source, callers, CSS cascade, menu dismissal/focus restoration and error/retry owners, and tests as source inspected; git diff --check passed. The public PR description and three new commit messages were inspected, with no attached images or actionable disclosure found. No PR code, tests, builds or apps executed. One hosted-check snapshot showed only Semgrep, zizmor and DCO successful; broader validation for this head was not established. Author-reported tests and desktop acceptance were not independently reproduced. Rendered contrast, narrow/wide and enlarged-text layouts, touch/keyboard and error/retry focus behavior, screen-reader/native behavior, performance and test cost remain unverified. The follow-up extends existing browser cases with geometry, clipping and menu-state checks; it adds no new browser test declarations. This is non-blocking COMMENT feedback, not approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) lanes.
Reviewed head ad656d0921263aafba89e4abffa0d7c7f909106c against base 7d50425. This follow-up covers the three-file delta from reviewed head 23a02e2c47f9fb913004c4455c770dd65b2e14d3 in its caller context.
No new actionable findings. The row-owner selectors stop hover and selected-text styling from spreading across a containing list. Selected rows retain their paired ink when their attached action is hovered. Existing NavigationItem, channel-row and Solo menu owners remain responsible for selection, activation, dismissal and focus recovery; the production delta adds no lifecycle machinery.
The existing P3 dark Snoozin’ self-mention distinction remains unchanged: ordinary and self mentions still resolve to the same neutral fill, hover and ink in dark neutral mode. Its original repair advice remains applicable; no duplicate inline comment is added. Overall source assessment remains 8/10 pending that distinction.
Validation: immutable source, callers, CSS cascade, error/retry and focus owners, and browser tests as source inspected; git diff --check passed. The public description and new commit message were inspected; no attached images or actionable disclosure found. The new browser case exercises real hover/computed colors and checks a sibling retains resting ink; the Solo case adds an assertion after action hover. One browser test declaration was added, none removed. Author-reported fail-before/pass-after evidence and desktop testing were not independently reproduced.
Source-only: no PR code, tests, builds or apps executed. The hosted-check snapshot showed only Semgrep, zizmor and DCO success; broader CI completion for this head was not established. Test-count/timing summaries were unavailable from those checks, so runtime cost and flake elimination remain unverified. Rendered contrast, keyboard/pointer behavior, narrow/enlarged layouts, screen-reader and native acceptance remain unverified here. This non-blocking COMMENT is not approval or merge authorization.
ad656d0 to
4eebc01
Compare
Keep neutral-surface links blue through Swarmin’ and reserve paired accent links for colored surfaces. Preserve semibold unread ink when hovering or selecting the shared Solo and Team sidebar row. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Observe the rendered self-drop target before releasing the pointer. Fence incidental read-state writes around the deliberate socket outage, and wait for recovered reply arrival accounting before checking thread focus, draft and reading position. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Ordinary mentions are a neutral chip at every level; a mention of you joins the strong group (brand pill in light, chartreuse ink on a neutral chip in dark). System notices name people as plain text. Dark rows keep neutral fills with the selected label in the accent, and links take each family's link partner from Hummin' (chartreuse: blue in light, bright chartreuse in dark). Co-authored-by: Vogue (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Light Hummin' and Swarmin' select the one sidebar destination with the brand fill and its ink, while hover and other selection stay neutral until Buzzin'. Chips, dots and row actions on that selection take its paired ink on every accent. The sidebar unread cue is strong only when a DM, a mention of you or a thread reply waits past its edge; ordinary unread uses a light gray cue. Co-authored-by: Vogue (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
One sidebar line in each thumbnail takes the selection color for its mode: the brand fill in light, the accent ink in dark, neutral at Snoozin'. Co-authored-by: Vogue (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
- Selected-row badges (working, typing, DM counts) use a paired quiet detail surface: pale accent with black ink in light mode, raised neutral with the selection ink in dark mode. - The row owns one trailing coordinate for the menu, unread dots and working status, so status is never offset or clipped by the select button. The working pill shares the overflow glyph's trailing edge. - The row overflow menu uses IconButton bare: the row's own hover and selected fill supply its state. Bare now changes only the fill; artwork size stays with the caller. Co-authored-by: Vogue (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Swarmin’ now adds only your own messages, on the brand fill with its olive ink, as Buzzin’ already did; links and mentions on the bubble use the same paired roles. The shell (background, dots and chrome on it) moves to Buzzin’, so each vibe level adds one step of color. Pale and dark chartreuse tints read khaki or olive, so the bubble uses the brand fill rather than accent 4. Co-authored-by: Vogue (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
On the brand-filled own bubble, a mention chip takes a deeper step of the bubble's fill (accent 10) with the bubble's ink, deepening to the pressed step on hover, instead of inverting to a dark chip. Token tests now require the own-bubble hover to differ from its resting fill. Co-authored-by: Vogue (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
The sidebar's hover and selection ink rules used a structural selector (`:hover > * > row`) to reach a row from its attached actions. A hovered list also matched it, so pointing at one page row inked every sibling. The rules now name the row owners (channel rows and Solo chat rows), and an owner's hover no longer overrides a selected row's ink. Co-authored-by: Vogue (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Keep the Inbox unread toggle at its original artwork size without resizing ordinary bare actions such as the Solo overflow menu. Scope the larger artwork to small bare controls with aria-pressed, document the distinction, and preserve the normal Solo glyph in its browser test. Co-authored-by: Carl (AI agent) <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
ef42d91 to
6099dd9
Compare
* origin/main: Limit message action hover to the bubble (#1306) Add chat and project creation to the Solo sidebar (#1338) Give chartreuse clear color rules for links, mentions, selection and unread (#1309) Let agent media tools read and attach any type Buzz supports (#1278) Signed-off-by: jtennant <jtennant@squareup.com> # Conflicts: # tests/browser/inbox-edit.spec.mjs
…-session-preview-live * commit 'f14605aa7aec82bd407900a9d217deb8c9f46322': Limit message action hover to the bubble (#1306) Add chat and project creation to the Solo sidebar (#1338) fix(inbox): keep tab ownership for focus in a portalled dialog test(inbox): assert settled successor focus after Archive advances Give chartreuse clear color rules for links, mentions, selection and unread (#1309) Let agent media tools read and attach any type Buzz supports (#1278) fix(channels): focus the composer of a tab a page opens (#1336) fix(inbox): focus the successor's tab when Archive advances fix(sidebar): stop the sidebar jumping and re-sorting when a channel is deleted (#1327) fix(inbox): keep focus when Inbox closes a tab in the main pane fix(inbox): keep focus when a retained tab's room becomes a Huddle feat(search): sort conversation search by newest and page results (#1206) Polish appearance controls, panel dividers, and message actions (#1337) Stop saving agent activity and metrics locally (#1347) fix(composer): place the caret after retained mentions on explicit focus (#1344)
What this does
Gives chartreuse a clear job in each place it shows up, so the brand color reads as a deliberate mark instead of an olive or beige stain. This follow-up to #1292 changes how links, mentions, unread cues and sidebar selection use color. It doesn't change the light-mode pale chartreuse steps themselves; that gets its own decision.
Why it matters
Chartreuse only works as a bright color. Darkened, it turns olive; mixed toward white, it reads as cream next to gray. The old rules used it in exactly those failing forms: olive link text, beige selection, muddy dark rows, and a highlighter on every mention. The color now goes where it can glow (dark-mode text, strong fills) and to what deserves attention. A mention of you stands out because ordinary mentions no longer compete with it.
New rule: In dark mode, fills stay neutral and chartreuse is the ink. In light mode, chartreuse is a fill and never text, and the full brand fill is spent only where it is rare: the selected destination, strong actions, and things that need your attention.
How it works
Everything is expressed through semantic roles, so components pick no colors themselves. Links use the family’s existing link pair. Mentions get a role trio for ordinary mentions, one for mentions of you, and one for inside your own bubble, which goes tone-on-tone on the brand-filled bubble: a deeper step of chartreuse with the bubble's ink, deeper again on hover. Dark rows restate the neutral row group explicitly, because the light vibe-level rules also match a dark page. A new selected-row ink lets dark selection carry the accent, and a paired selected-row detail surface and ink covers small badges on a selection. IconButton's fill-free
barestyle now also covers actions revealed by a row. Ordinary row actions keep standard artwork sizing; small glyph-state toggles retain their larger artwork so their on/off state stays clear. The contrast guard checks every new pair for every accent, level and mode. Near-black ink on chartreuse counts as black, not gray.The PR also repairs two WebKit browser tests that raced on CI: a drag released before its target rendered, and an incidental read save landed during a deliberate disconnect. Both now wait for the right moment, without retries, error allowances or weaker assertions.
Verification
Head
6099dd946, rebased on main atb2787b2a3.a051b79c3before the final clean rebase onto fix(sidebar): stop the sidebar jumping and re-sorting when a channel is deleted #1327. The 190-case browser run passed 183 cases; seven WebKit failures coincided with laptop sleep (trace stalls matched power-management logs). All five affected WebKit files then passed unchanged at6099dd946: 55 cases, default worker count, no retries or timeout increases. This does not turn the initial interrupted run into a full pass. Mandatory push checks passed on the final head.