fix(iced): pin the notification dots to the Mac accent #007aff (#311) - #316
Merged
Conversation
Iced rendered both notification dots — the tab-pill badge and the sidebar project-row dot, which share `chrome::badge()` — at `#4e9af1`, while both reference implementations deliberately use `#007aff`: the Mac dots are `NSColor.controlAccentColor` (`App.swift:4772`, `:5207`) and GTK hardcodes the same value for its tab badge rather than tracking the desktop accent, because `@accent_bg_color` renders teal on COSMIC. A dedicated `NOTIFICATION_BADGE` constant carries the corrected value, so the generic `NOTIFICATION` accent keeps `#4e9af1` and its three other uses (dragged-pill border, inline-rename focus ring, inline-rename selection) untouched — those are user-directed polish, not a cited divergence. The unit assertions compare color *literals*, not the constants against themselves: a test asserting `badge()` renders `NOTIFICATION_BADGE` passes for any value of that constant and would not catch a re-flip. The two inline-rename uses gain their first coverage. codex review finding: the `badge()`-renders-`NOTIFICATION_BADGE` assertion is tautological. Kept deliberately — it guards the *wiring* (badge being repointed at another constant) while the literals guard the value — and labeled as such in a comment so it isn't mistaken for the color check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
C1 corrected the constant; nothing proved it reaches the screen. `tab.list` exposes only the `has_notification` bit, so the colour the bit is painted in — and where — is invisible to every dump op. That is why #311 shipped undetected and why the parity inventory names a badge colour/position pixel assertion as the missing acceptance evidence. `test_notification_dots_paint_the_accent` covers both surfaces `chrome::badge()` styles: the tab-pill badge and the sidebar project-row dot. With one notified INACTIVE tab it asserts exactly one accent blob per region, each exactly #007aff at its centre (found fuzzily, asserted exactly), sized near NOTIFICATION_DOT_SIZE, band-centred, the tab badge bracketed between its own title's last glyph and the next pill's fill, and the sidebar dot at its derived PROJECT_PILL_INSET_X + PROJECT_DOT_INSET trailing reserve. A clean baseline before the notification and a clean frame after `tab.clear_notification` bound it on both sides — `clear_notification` rather than `focus` because the render guard reads `has_notification && !active`, so focusing would flip both inputs at once. Verified non-vacuous by two mutations, not one: reverting the constant to #4e9af1 makes the blob unfindable and fails the count path, while #0079ff — one off, deliberately inside COLOR_TOL — is still found and fails the exact-centre assertion, proving the tolerance is not swallowing the bug. Transcripts in the plan's artifact folder. `_blobs()` generalises the two ad-hoc finders in test_sidebar_pixels.py (neither supported a min-x or any y bound); both are refactored onto it rather than adding a third near-duplicate scan. Behaviour-equivalent: the old first-match/break is indistinguishable from independent per-colour passes because the lifecycle tolerance regions do not overlap. codex review findings, both fixed: the badge-to-pill gap had only an upper bound, so a badge drifted right toward the next pill would pass — now bounded on both sides; and the sidebar blob was only proven to be "in the sidebar" — now pinned to the project pill's trailing inset. No test-list wiring needed: this module is already enumerated in ICED_E2E_TESTS and all three ci.yml lists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
The parity inventory's Tab close/badge row and the roadmap's open-P1 audit list both carried the badge divergence as an open finding. Both now record the fix, and both record what the 2026-08-07 audit missed: `chrome::badge()` styles two surfaces, not one — the tab-pill badge and the sidebar project-row dot — and both have a Mac reference at `NSColor.controlAccentColor`, so both were corrected. The inventory row moves to the table's existing `P1 → 3h` marking, matching the sibling rows where the objective half is done and only the Charlie-directed item (here hover-close) remains. The roadmap's M4 paragraph now reads (c) and (d) as done, leaving the 3h items and the #302-blocked remainder. M4 is deliberately NOT declared entered, and the waiver-semantics sentence is untouched: a per-row waiver stays an owner decision recorded on the inventory row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIced notification badges now use the dedicated ChangesNotification badge correction
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
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.
Closes #311.
Plan 023, workstream A — the last objective item on M4's "no open P0/P1"
entry criterion. Part 1 of 3 (pre-release cleanup); siblings to follow for the
release-smoke extraction and the stale GTK documentation.
What changed
The constant. iced rendered its notification dots
#4e9af1while bothreference implementations deliberately use
#007aff— the Mac dots areNSColor.controlAccentColorand GTK hardcodes the same value rather thantracking the desktop accent, because
@accent_bg_colorrenders teal onCOSMIC. A dedicated
chrome::NOTIFICATION_BADGEnow carries the correctedvalue, used only by
chrome::badge().NOTIFICATIONkeeps#4e9af1and its three other uses — dragged-pillborder, inline-rename focus ring, inline-rename selection. Those are
user-directed polish with no cited divergence, so recoloring them here would
be scope creep into the fenced-off 3h cluster. Whether the generic accent
should follow is filed separately for the owner's call.
The plan's own review caught that this is two surfaces, not one.
chrome::badge()styles both the tab-pill badge (app.rs:1978-1986) and thesidebar project-row notification dot (
app.rs:1777-1786). The 2026-08-07audit that filed #311 recorded only the tab badge. Both were corrected, and
that is correct rather than incidental: both have a Mac counterpart at
controlAccentColor(App.swift:4772,:5207), so both move towardparity. Splitting
badge()into two styles was considered and rejected — itwould invent a second constant with no reference divergence to justify it.
The test.
tab.listexposes only thehas_notificationbit; the colourthat bit is painted in, and where, is invisible to every dump op. That is why
#311 shipped undetected, and why the parity inventory named a badge
colour/position pixel assertion as its missing acceptance evidence.
test_notification_dots_paint_the_accentasserts, off oneapp.screenshotwith a single notified inactive tab:
#007aff(found fuzzily atCOLOR_TOL, asserted exactly)pill's fill, with the gap bounded on both sides — relational rather than a
pinned x, because pill width is text-dependent
PROJECT_PILL_INSET_X + PROJECT_DOT_INSETtrailing reserve, so it is provably the project row's dot
tab.clear_notificationThe negative case uses
clear_notification, notfocus: the render guard ishas_notification && !active, so focusing flips both inputs at once and ablank result would not say which one did it.
Rust assertions compare literals, not constants against themselves. A test
asserting
badge()rendersNOTIFICATION_BADGEpasses for any value of thatconstant. The literal assertions are what pin the value; the wiring assertion
is kept and labelled as such. The two inline-rename uses gain their first
coverage.
Verification
make check-iced(with and withoutlinux-package),make test-rust,make e2e-iced-ci— 85 passed, 3 skipped (all three skips pre-existingand environmental).
constant to
#4e9af1makes the blob unfindable and fails the count path;#0079ff— one off, deliberately insideCOLOR_TOL— is still foundand fails the exact-centre assertion. Only the second mutation proves the
tolerance is not swallowing the bug.
badge-to-pill gap measured exactly the 9px the layout constants predict.
The size band's floor is the same 5px saturated core the repo already pins
for an 8px dot, since this module runs on four lanes and only one renderer
was available locally.
Review findings
Three panel reviewers on the plan, plus a codex correctness pass per commit.
chrome::badge()has two call sites — the plan claimed onefocus()clear_notificationinstead, plus a pre-notify baseline[6,9]size band was an unmeasured guess on a full circlebadge()-renders-NOTIFICATION_BADGEassertion is tautologicalImpact
No dependency, privacy, or secret impact. No wiring needed — this module is
already enumerated in
ICED_E2E_TESTSand all threeci.ymllists. It isdeliberately not in
ICED_RELEASE_E2E_TESTS: chrome colour is notprofile-dependent.
Plan 023 §D1 — the pinned design (excerpt)
D1.1 — one dedicated constant, shared by both call sites. The first draft
asserted only the tab badge changed. It was wrong:
badge()is shared. Thedecision stands anyway, now on evidence rather than by accident — both iced
surfaces have a Mac counterpart at
controlAccentColor, so recoloring bothmoves both toward parity. Splitting
badge()into two styles was consideredand rejected: it would create a second constant with no reference divergence
to justify it, and would leave the sidebar dot at a value no implementation
uses.
Alternative considered: flip
NOTIFICATIONwholesale. Real argument — theMac uses
controlAccentColorfor its drag indicator too(
DragReorder.swift:140) — but it recolors three user-visible surfacesnobody asked to change, inside the fenced-off 3h cluster. → filed as an issue.
D1.2 — Rust unit assertions: literals, not restatements. The first draft
proposed
badge(&Theme::Dark).background == Some(Background::Color( NOTIFICATION_BADGE)), which passes for any value of the constant. The sameflaw is already latent in the existing
chrome.rstest.D1.3 — the pixel test. Fixture with every precondition explicit; a
generalized blob finder (neither existing finder supported a min-x or any y
bound); captures polled rather than taken once; size deliberately not pinned
until measured, because the badge is a full circle where the sidebar's prior
art deliberately uses radius 3 "to retain a solid renderer-neutral edge".
§8 — two mutations, not one. One mutation is not enough: reverting the
constant makes the blob unfindable, so the test fails on the count
assertion and the exact-centre check — the thing that distinguishes this from
"some blue dot exists" — is never exercised.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Summary by CodeRabbit
Bug Fixes
Tests
Documentation