Group comment threads on one block under a single marker #300
- MA
## Integration and validation
One marker now represents all comment threads on a block. Preserve main’s current sidebar and composer; keep touch targets, offscreen clamping, keyboard thread selection and focus restoration. Reviewed the combined geometry and presence boundaries.
Formatting/lint/design checks, types, 24 focused tests and all 52 responsive-comment/sidecar browser tests pass locally. Required CI will be rerun on the final rebased head before merge.
<details> <summary>Original proposal and screenshots</summary>
Stacked on #277.
## Why
Every comment thread had its own gutter button. When a block had several threads the buttons stacked down the page, spilled past the block and covered words in the next paragraph. On phones the 44px buttons hid a third of each line. A new comment's marker could land inline at the end of its quote, on top of text. And when a thread card was open, other threads' markers painted over it (author line, reply field, Dismiss), because each marker and card were siblings in one stacking context.
## What changed
- **One marker per block.** Threads are grouped by their top-level block. The marker sits in the right gutter, centred on the block's first line, never at the end of the quote. Two or more threads show the comment icon plus a count (`text-xs`, tabular numbers). A marker uses `--color-brand-wash` when any of its threads has replies (the existing "replies waiting" state). - **Phones.** When the gutter can't hold a 24px chip (16px of padding at 390px), the chip narrows to 14px and sits inside the content padding beside the block: a count for groups, a small icon for one thread. It never covers text. On coarse pointers the touch area is 44px wide and as tall as the block's first line (24–44px), kept inside the document, so a tap at the end of the second line reaches the text. With 10+ threads the slim chip widens to 18px so two digits fit. - **Grouped list.** Clicking a grouped marker opens the existing card surface with a compact list (author, first line, reply count). Selecting a thread shows it as today, with an "All N comments" back button that returns focus to the item you came from. Arrow keys, Home and End move through the list, and Tab still works. Escape closes the card and returns focus to the marker. The accessible name is `3 comments on “<block excerpt>”`. Single-thread markers keep their old name and behaviour. - **Card above markers.** The pinned card (thread or list) renders once, after every marker. While a card or draft is open, other markers dim to 0.4 opacity and are `inert`. - **Geometry.** `blockMarkerPoints` in `comment-geometry.ts` replaces `markerPoints`. It is pure and unit-tested: gutter placement, slim fallback, stacking of short adjacent blocks, offscreen hiding, and #277's `held` clamping. A group is held when any of its threads is pinned or its marker has focus.
## Screenshots
Before, desktop: one button per thread, stacked over the next paragraph: 
After, desktop: one marker per block with a count: 
Before, phone 390×844: 
After, phone 390×844: slim chips inside the content padding: 
After: a grouped marker opens the list: 
After: a thread opened from the list, with the way back: 
After: an open thread dims the other markers, and the card sits above all of them: 
After, phone: the list in the comment sheet: 
## Testing
- `bun test packages/editor`: 447 pass - `bun test apps/web scripts`: 569 pass - `bun run types`: pass - `bun run fix`: clean (only existing warnings) - `bun scripts/check-design-record.ts`: pass - `bun run ci`: fails only on the review hash for `comment-layer.tsx` (see below). The base branch #277 already fails this check. - E2E: added grouping tests (desktop and phone), list keyboard order and focus return, and a card-above-markers test to `e2e/responsive-comments.e2e.ts`. Updated one `sidecar.e2e.ts` test whose second comment now joins the existing block marker. Local E2E is suspended for this run, so CI runs these. I ran the same steps against a local fake-GitHub server with a headless Playwright script, and all passed.
## Needs a design-contract review
- `packages/editor/src/comment-layer.tsx` is pinned in `scripts/design-contract/exceptions/dynamic-editor.json` (both comment-layer entries). New hash: `68ce978878c94de6a4e594b21c08f6dc58fc99ca2702d49d841746982d91dd88`. The dynamic styles are still measured geometry (marker `top`, `left`, `width`, `height` and the chip `width`) plus the same presence classes. There are no new class or token logic.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
</details>
- MA
**Review:** Looks good
Checked on 8925 (desktop 1440x900 and iPhone 13 touch) with 1, 2 and 5 threads per block. CI: only the `comment-layer.tsx` review-hash failure; container and e2e pass.
- No marker covers text: chips are in the gutter on desktop (0 overlap with text rects) and in the content padding on phone. - Phone touch area: the 44px button overlaps text by up to 14px (a 14px strip x 44px tall at the end of the first 1-2 lines). Mis-tap on a line's last word is possible but only in that strip, and long-press selection there is lost. Acceptable; if it bites, clip the hit height to the first line box. - `comment-layer.tsx` `chipWidth`/`blockMarkerPoints`: a slim chip is capped at 14px, but the count for 10+ threads needs about 14px of digits plus padding and will clip. Rare; consider hiding the digit and keeping an aria-only count, or letting slim width grow to 18px. - Brand wash: there is no unread/"replies waiting" concept elsewhere for comments (`data-unread` is Planner change marks only). The only prior meaning was the old aria-label "N replies waiting", so wash = "any reply exists" is the right reuse. But it persists forever and never clears, so it reads as "has replies", not "waiting". Group aria-description uses the same repliesWaiting text; wording is fine. - Keyboard: list opens with focus on the first item, Arrow/Home/End roam (wraps), Enter opens the thread with focus on "All N comments", back returns to the list, Escape closes and focus returns to the marker. Verified. Missing: after "back", focus is not placed on the item just left (it goes to the first item / nothing); restore it. Phone sheet focuses the grabber, not the list (by design). - Accessible names: marker "N comments on “excerpt”" and the card labels "Comments" / "Comment thread" are fine. The list items have no per-item accessible context beyond author + first line; fine. - `opacity: 0.4` at `styles.css` `.plan-comment-button[data-dimmed]`: no opacity token exists; 0.4 matches the existing `:disabled` convention in `apps/web/src/theme.css:50`. OK. Dimmed markers are also `inert`, so contrast is a non-issue. - Open card renders above all markers, others dimmed and inert (4 of 4 in e2e, 2 of 2 here). - e2e coverage is good: grouping, desktop and phone, keyboard, card-above-markers, offscreen/held retained. Gap: no e2e for the 10+ count and no phone assertion on text overlap. Unit tests cover `blockMarkerPoints`.
MaggieAppletonreferenced this in Keep the comment card off its passage and match Chat's composer and author row #301last month on GitHub
- MA
I've pushed the three follow-ups in 33cce206, as a new commit on top so history stays linear.
1. **Phone touch area:** On a coarse pointer the marker's hit area is still 44px wide, but no taller than the block's first line (between 24 and 44px). A tap at the end of the second line now reaches the text. The phone grouping e2e test checks this with `elementFromPoint` at the end of the second line. 2. **Two-digit counts:** `BlockMarker.minimum` sets the narrowest a slim chip may be. With 10 or more threads the chip widens to 18px and sits flush with the document edge, so "10" fits. There is a new unit test in `comment-geometry.test.ts` and a new e2e test that seeds 10 threads on one block. 3. **Back focus:** Back now focuses the list item you came from, on desktop and in the phone sheet. Reopening the list from the marker starts at the top again. The keyboard e2e test covers it.
CI: e2e and container pass. The validation job still fails only on the `comment-layer.tsx` design-contract review hash. The new hash is `68ce978878c94de6a4e594b21c08f6dc58fc99ca2702d49d841746982d91dd88`, and the PR body is updated.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
lavaman131referenced b327206c993alast month on GitHub
MaggieAppletonbase-ref-force-pushedlast month on GitHub
MaggieAppletonbase-ref-force-pushedlast month on GitHub
MaggieAppletonautomatic-base-change-succeededlast month on GitHub
- MA
<!-- pr-rebase:33cce2067691e684565b41e67bd672c3f50c088f:8675f8e57182b28d0b2dc34c8089d0d928d00191 --> PR babysitter could not rebase this branch cleanly onto main. The branch is unchanged. Please resolve the rebase conflict locally; automation will try again after the branch changes.
- MA
CI babysitter checked the recorded head and found no safe code fix to queue. The current failed `ci` run still stops in `check-design-contract` after lint/tokens, with reviewed-owner hash changes only in `packages/editor/src/comment-layer.tsx`.
No commit was queued. Next action: review and renew the relevant design-contract exceptions if the `comment-layer.tsx` data-flow changes are approved, or revise that file if review rejects them.
> [!WARNING] > <details> > <summary>Firewall blocked 1 domain</summary> > > The following domain was blocked by the firewall during workflow execution: > > - `releaseassets.githubusercontent.com` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "releaseassets.githubusercontent.com" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details>
> Generated by [PR CI fixer](https://github.com/githubnext/chopin/actions/runs/37619025602) · gpt54 · 134 AIC · ⌖ 10 AIC · ⊞ 16.4K · [◷](https://github.com/search?q=repo%3Agithubnext%2Fchopin+%22gh-aw-workflow-call-id%3A+githubnext%2Fchopin%2Fpr-ci-fix%22&type=issues)
<!-- gh-aw-agentic-workflow: PR CI fixer, engine: codex, model: gpt-5.4, id: 37619025602, workflow_id: pr-ci-fix, run: https://github.com/githubnext/chopin/actions/runs/37619025602 --> <!-- gh-aw-workflow-call-id: githubnext/chopin/pr-ci-fix -->
- MA
<!-- pr-rebase:33cce2067691e684565b41e67bd672c3f50c088f:e6b7bb333ce8948d0dac0c8aff61cf3d4fb8dc8b --> PR babysitter could not rebase this branch cleanly onto main. The branch is unchanged. Please resolve the rebase conflict locally; automation will try again after the branch changes.
- MA
<!-- pr-rebase:33cce2067691e684565b41e67bd672c3f50c088f:b919317f14f29799be257eee49ba5ab753592371 --> PR babysitter could not rebase this branch cleanly onto main. The branch is unchanged. Please resolve the rebase conflict locally; automation will try again after the branch changes.
- MA
CI babysitter rechecked the current failed `ci` run for `33cce2067691e684565b41e67bd672c3f50c088f`. The head still matches the recorded candidate, and the failure remains the reviewed design-contract hash check for `packages/editor/src/comment-layer.tsx`, while `e2e` and `container` are green.
I did not queue a commit from this runner because `bun` and `mise` are not installed here, so I could not run the required local reproduction and verification commands (`bun install --frozen-lockfile`, `bun run fix`, `bun run ci`). Next action for a human: on a machine with Bun 1.4.2 available, rerun those commands on this head, review the `comment-layer.tsx` design-contract owner change, and renew the exact exception only if it is approved.
`babysit-head:33cce2067691e684565b41e67bd672c3f50c088f`
> Generated by [PR CI fixer](https://github.com/githubnext/chopin/actions/runs/37627355771) · gpt54 · 18.2 AIC · ⌖ 4.44 AIC · ⊞ 16.4K · [◷](https://github.com/search?q=repo%3Agithubnext%2Fchopin+%22gh-aw-workflow-call-id%3A+githubnext%2Fchopin%2Fpr-ci-fix%22&type=issues)
<!-- gh-aw-agentic-workflow: PR CI fixer, engine: codex, model: gpt-5.4, id: 37627355771, workflow_id: pr-ci-fix, run: https://github.com/githubnext/chopin/actions/runs/37627355771 --> <!-- gh-aw-workflow-call-id: githubnext/chopin/pr-ci-fix -->
MaggieAppletonhead-ref-force-pushed e17759874727last month on GitHub
MaggieAppletonhead-ref-force-pushed 39c0b9807d75last month on GitHub
MaggieAppletonmerged e8cd962eeb7alast month on GitHub
MaggieAppletonhead-ref-deletedlast month on GitHub