Skip to content

fix(#1360): cluster pill shows letter+count — restore count visibility regressed by #1357#1362

Merged
Kpa-clawbot merged 3 commits into
masterfrom
fix/issue-1360
May 25, 2026
Merged

fix(#1360): cluster pill shows letter+count — restore count visibility regressed by #1357#1362
Kpa-clawbot merged 3 commits into
masterfrom
fix/issue-1360

Conversation

@Kpa-clawbot
Copy link
Copy Markdown
Owner

@Kpa-clawbot Kpa-clawbot commented May 25, 2026

Red commit: c0de33a (CI: https://github.com/Kpa-clawbot/CoreScope/actions/runs/26416117686)
Green commit: c268248 — CI: https://github.com/Kpa-clawbot/CoreScope/actions/runs/26416069319

What

Fix #1360 regression: cluster role pills on /map show ONLY the role letter (R/C/M/S/O); the per-role count number that was visible pre-#1357 is gone. This PR restores the count by concatenating it after the letter inside the pill body, so each pill renders as R60, C30, M5, etc.

  • public/map.js makeClusterIcon: pill body becomes letter + n (was letter).
  • aria-label / title ("60 repeaters") untouched — already correct.
  • DOM, classes, CSS, --mc-* constants, border-style ramp, multi-byte labels — untouched.

Adversarial follow-up (commit on top of green)

  • JS cap: makeClusterIcon clamps n > 999"999+", so pathological clusters render as e.g. R999+ instead of R10000. Pill width stays bounded.
  • CSS guard on .mc-pill: max-width: 4ch; overflow: hidden; text-overflow: ellipsis; as defense-in-depth if a render slips past the JS cap.
  • +3 test assertions: one for the JS cap, two for the CSS guard. Mutation-verified (removing the cap fails ONLY the new cap assertion).

Why

#1357 fixed WCAG 1.4.1 for cluster role pills by promoting the role letter to the pill body, but in doing so dropped the count number that sighted operators relied on for at-a-glance per-role counts. The letter is the WCAG carrier; the count is the data. Both belong in the pill body — they always did before #1357. The audit's intent was to PAIR them, not REPLACE one with the other.

TDD red→green

Visual verification

Open https://analyzer.00id.net/#/map after deploy and confirm cluster pills display R<count>, C<count>, M<count>, etc. (e.g. R60 C30 M5) instead of bare letters. aria-label="60 repeaters" remains for screen readers. For very large clusters, pills cap at R999+ / C999+ etc.

Fixes #1360

openclaw-bot added 2 commits May 25, 2026 19:12
Pure-string assertions over public/map.js mirroring #1356 pattern.
Fails on current master where pill body is just `letter`.

Wires test-issue-1360-pill-letter-count.js into deploy.yml right after
test-issue-1356-map-a11y.js.
Restore the count number that #1357 inadvertently dropped from cluster
role pills. The pill body now concatenates the role letter (WCAG
carrier from #1356) with the per-role count (operator-facing data),
producing R60 / C30 / M5 / S1 / O2 style rendering on the map.

aria-label and title unchanged ("60 repeaters") — already correct.
DOM, classes, CSS variables, border-style ramp, multi-byte labels —
all untouched.

Verified locally:
- test-issue-1360-pill-letter-count.js: PASS
- test-issue-1356-map-a11y.js: 40/40 PASS (letter still first char)
- test-issue-1293-marker-shapes.js: 23/23 PASS
- test-marker-outline-weight.js: 6/6 PASS
@Kpa-clawbot
Copy link
Copy Markdown
Owner Author

Kent Beck Gate (round 1) — TDD + test quality

Verdict: APPROVED
TDD history: PASS

TDD red→green history (verified)

  • Branch commits (oldest→newest):

    • c0de33a9test(#1360): RED — assert pill body emits letter + count — touches only .github/workflows/deploy.yml + test-issue-1360-pill-letter-count.js. Tests-only ✓.
    • c268248dfix(#1360): GREEN — pill body emits letter + count — touches only public/map.js. One-line production change ✓.
  • Red CI run 26416117686conclusion=failure, log shows:

    • === #1360: pill body emits letter + count (not letter alone) ===
    • ✗ map.js concatenates letter + n (or letter + String(n)) for pill body
    • ✗ pill body is no longer just letter (no '+ letter + "</span>"' pattern)

    Assertion-level failure (not compile/import). Test file ran to completion and failed on its assertions. Red-bar quality bar met ✓.

Six Questions

1. Show me the test that fails when this change is reverted.
Reverting letter + nletter flips assertions A (/letter\s*\+\s*(?:String\()?\s*n\b/), B (/\+\s*letter\s*\+\s*['"]<\/span>/), and C (template-shape regex). CI run 26416117686 is the proof. ✓

2. Smallest test for the regression.
Assertion A alone (one regex) is the minimal gate. B is a useful negative complement. C duplicates A with looser context. Trio is defensible; not over-elaborate.

3. Could a wrong impl pass?
Stress-tested the harness against plausible wrong concatenations:

  • letter + '|' + n, letter + ' ' + n, letter + ', ' + n — A's letter\s*\+\s*(?:String\()?\s*n requires direct adjacency (only String( allowed between); these all fail A. ✓
  • n + letter — fails A (no letter + n) AND fails D (/\bn\s*\+\s*letter\b/). ✓
  • letter + (n + '...') — fails A (open paren not in alternation). ✓
  • \bn\b word boundary prevents accidental match on nbsp, null, etc. ✓
    Harness is appropriately tight for a pure-string test.

4. Edge cases NOT tested.

  • 0-count role: verified inert — if (n <= 0) continue; at public/map.js:1515 suppresses zero-count roles before any pill is rendered. Not a regression risk.
  • 3-digit count (100+): not tested. Could overflow pill width or wrap badly. Not asserted in DOM/CSS.
  • Single-digit n: implicit in regex [RCMSO]\d+$ mentioned in comments, but no assertion actually evaluates the rendered output against that regex (template not invoked).
  • Missing ROLE_LETTERS entry: production falls back to '?' → pill body becomes ?5. Not asserted; behavior preserved but undocumented.
  • All of these are out-of-scope for a 1-line regression fix — flagging as nits, not must-fix.

5. Test names describe behavior or implementation?
Mixed but defensible. "concatenates letter + n for pill body" leans implementation-y, but since the pure-string approach IS the test methodology, the name accurately describes what's being asserted. "pill body is no longer just letter" and "letter precedes count in concatenation" are behavior-oriented. Acceptable.

6. Pure-string approach defensible?
Yes. public/map.js is a Leaflet-bound browser IIFE that cannot be require()'d in Node. Pattern mirrors the already-accepted test-issue-1356-map-a11y.js and test-issue-1357-* precedents. DOM is built via string concatenation, so source-grep ≈ output-grep. Same answer as the #1357 polish: defensible, with the well-known caveat that a runtime rename of letter/n would defeat the harness. Not blocking.

Must-fix

0.

Out-of-scope nits (for future hardening, not this PR)

  • Add a 3-digit count assertion (e.g., construct a synthetic call to confirm pill body matches /^[RCMSO]\d{1,3}$/).
  • Consider one tiny "execute the template" test via Function-eval'd extraction of makeClusterIcon, but that's a larger refactor than this PR warrants.

Gate verdict: PASS — ship it.

@Kpa-clawbot
Copy link
Copy Markdown
Owner Author

Independent review (round 1)

Verdict: NEEDS-WORK (2 must-fix)

Tiny, well-scoped diff. The 1-line change in makeClusterIcon correctly concatenates letter + n before </span>; ARIA / title / class / style strings are unchanged; if (n <= 0) continue; already suppresses zero-count pills (so the new shape R60 / C30 won't ever render as bare R0). Workflow wiring is correct (slots in right after the #1356 test). Confirmed test-issue-1356-map-a11y.js and test-issue-1293-marker-shapes.js are byte-unchanged on the branch (no quiet edits). Red commit c0de33a9 failed on assertion in CI (proper TDD red, not a build error); green commit c268248d passes.

Must-fix

  1. PR body: Red commit URL missing. Body says c0de33a9... (CI: pending — see checks). AGENTS.md requires Red commit URL in PR body. Add https://github.com/Kpa-clawbot/CoreScope/actions/runs/26416117686 (or the failed Go Build & Test job link) and note the failed assertion line.
  2. Acceptance criterion WS handlers trigger full page reloads on every packet — no debouncing #7 not addressed (4+ digit graceful handling). Issue explicitly calls for it; public/style.css:3346 .mc-pill has only min-width: 12px; padding: 0 3px with no overflow, no max-width, no text-overflow, and .mc-pills has no flex-wrap / overflow: hidden. Cluster wrap is 48px (56px mc-lg); 4 pills of R1000-shape (~5 chars × ~5px + 6px padding ≈ 31px each → ~124px) will overflow the cluster bubble. Either (a) add overflow: hidden; text-overflow: clip + max-width on .mc-pill and overflow: hidden on .mc-pills, or (b) cap rendered count (e.g. n > 999 ? '999+' : n) in map.js, or (c) explicitly state in PR body that WS handlers trigger full page reloads on every packet — no debouncing #7 is deferred and open a follow-up issue.

Out-of-scope (not blockers)

  • Test assertion E (ROLE_LETTERS map contents) is mostly tautological — asserts the map is defined but not that the pill body actually emits one of those letters. Assertions B and D are the ones doing real work; the test as a whole is gating (Red commit proved it). Acceptable per the "mirrors a11y(map): cluster bubbles + role pills + multi-byte hash labels encode signal by color only (WCAG 1.4.1) #1356 pattern" note, but a future hardening could shell out to a DOM-grep style check that the rendered HTML matches /^[RCMSO]\d+$/.
  • The pillTemplateRe (assertion C) is essentially a duplicate of assertion A with a 400-char window — non-harmful, but trim-worthy.

Confirmed clean

Adversarial follow-up to PR #1362:

JS cap (public/map.js, makeClusterIcon):
- If per-role count n > 999, render pill body as letter+"999+"
  (e.g. "R999+") instead of letter+raw digits. Bounds visual width
  for pathological clusters.

CSS guard (public/style.css, .mc-pill):
- Add max-width: 4ch + overflow:hidden + text-overflow:ellipsis as
  defense-in-depth if a render slips past the JS cap.
- Replaces prior overflow:visible (SC 1.4.12 letter-spacing) — the
  4-char-cap content (letter + ≤4 digits) does not need aggressive
  letter-spacing overrides, so the tradeoff is acceptable.

Tests (test-issue-1360-pill-letter-count.js):
- +1 assertion: makeClusterIcon source contains the n > 999 → "999+" cap.
- +2 assertions: .mc-pill rule declares max-width AND text-overflow:ellipsis.
- Mutation-verified: removing the cap fails ONLY the new cap assertion.
- Existing 6 #1360 assertions and full #1356 (40) suite stay green.
@Kpa-clawbot Kpa-clawbot merged commit 40aa02b into master May 25, 2026
6 checks passed
@Kpa-clawbot Kpa-clawbot deleted the fix/issue-1360 branch May 25, 2026 19:59
Kpa-clawbot pushed a commit that referenced this pull request May 25, 2026
The defense-in-depth max-width:4ch added in #1362 clamps the BOX
(including the 1px 3px padding), leaving ~2.5ch for text — enough for
'R6' but not 'R60', so multi-digit cluster pills render as 'R…'.

Drop the max-width entirely. JS in map.js already caps counts at '999+'
(max 5 chars: 'R999+'), which is the load-bearing safety. Keep
overflow:hidden + text-overflow:ellipsis as belt-only graceful-degrade
if the JS cap is ever bypassed.

Also updates the #1360 follow-up test to match — the max-width assertion
codified the over-aggressive guard, not behavior we want to preserve.
Kpa-clawbot added a commit that referenced this pull request May 25, 2026
…igit count visibility (#1365)

Red commit: 482ffe6 (CI: pending)

## What

Drops `max-width: 4ch` from `.mc-cluster .mc-pill` in
`public/style.css`. Keeps `overflow: hidden` + `text-overflow: ellipsis`
as belt-only graceful degradation.

## Why

#1362 added `max-width: 4ch` as defense-in-depth for the `999+` JS cap.
But `4ch` is applied to the BOX including the `1px 3px` padding, so
effective text width is ~2.5ch — enough for `R6` but not `R60`. Result:
post-merge regression on staging where multi-digit cluster pills render
`R…` instead of `R60`/`C30`.

The JS cap in `public/map.js` already clamps counts to `999+` (max 5
chars: `R999+`). That's the load-bearing safety. The CSS `max-width` was
overcaution and went too aggressive. Option A from the issue: drop the
cap entirely, keep ellipsis as graceful-degrade if JS ever fails.

## TDD red→green

- RED: `test-issue-1364-pill-no-clamp.js` asserts `.mc-pill` CSS does
NOT contain `max-width: 4ch` (regression guard) and DOES contain
`overflow: hidden` + `text-overflow: ellipsis` (graceful degradation).
Fails on the unchanged CSS.
- GREEN: deletes the `max-width: 4ch;` line from `.mc-pill`. Test
passes.

Wired into `.github/workflows/deploy.yml` alongside the #1360 test.

## Visual verification

Open `/map` zoomed-out on staging. Cluster pills must render full counts
(`R60`, `C30`, `R250`, capped `R999+`) — no `R…` ellipsis. No horizontal
scrollbar even on synthetic 4-digit injection.

Fixes #1364

---------

Co-authored-by: openclaw-bot <bot@openclaw.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

regression(map): #1357 cluster role pills lost the count number — show letter+count not letter-only

1 participant