fix(v2): close #1582 UX follow-ups - #1585
lilyshen0722 wants to merge 4 commits into
Conversation
…strip, thread band, quote link, +N reactions The Thread-states artboards (direction C walk 3), on top of PR 2a: - Agent rows carry a mono runtime tag (`.v2-msg__tag`, from `agent.runtime.runtimeType`) and the two-tone avatar; humans carry neither. - Actions are one strip on hover (react · reply · thread · more), 30px r4, with a More menu (copy link / copy text). At ≤760 a ~500ms long-press reveals the strip inline under the body (28px targets); a tap on the text dismisses it. The click that ends a long-press on touch stacks is latched so it cannot hide the strip the instant it appeared (found in the 390 walk). - A thread is a chip under its root (`N replies · last 2h`); open, the band is #f9fafb with a 2px rule, newest 8 replies + `N more replies`, and a Collapse foot beside Reply in thread. - A quoted reply is a link that lands on its source (`#message-<id>`, `landOnMessage`), paging older history until the row is loaded. - Reactions past six fold into a `+N` chip that expands the row. - i18n en/zh-CN for strip, thread, quote and reactions strings; tests in V2ThreadRestyle (6) + invariants; walked at 1440 and 390 (real touch via CDP). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JavWvyVL478UfEhJjcfMgX
…ct in the strip, flat avatars @ux-lead CHANGES_REQUESTED at fe160e2, all seven applied and walked: 1. Transcript avatars are flat two-tone squares (`tone="flat"`): human tint #f2f4f7 + #4f5c6e initials, agent cobalt + white; a photo still wins, and the illustrated character tier stays out of the transcript. The cobalt allowlist moves to `.v2-thread .v2-avatar--flat-agent`. 2. The ROOT renders inside its band — the block is a full-width child and the indent moved to `.v2-thread-block__inner`, so the rule lands at the text column (measured: rule x419 == body x419; it was 421 vs 469). 3. The chip is faces + count + `· last` and nothing else. Reply in thread, Follow and Collapse live in the band's foot; the chevron is gone, and the 44px touch floor moved with them. 4. A thread rests as a chip. Which roots are open is session state in the transcript; the server `collapsed` row is written to match on open/close via a new `setCollapsed` (it still drives the wake path) rather than deciding the resting view. 5. React is the strip's first icon whenever the strip renders — disabled, not absent, when the row cannot take a reaction. 6. A quote to a reply folded behind `N more replies` REVEALS: the transcript opens that thread and clears its fold, then the landing effect re-runs. It no longer pages older history for a message that is not old (sprint-review 64477). 7. Mention token carries the #e8ecfb wash with cobalt text. Also @sprint-review 64468 + 64485: the reveal's placement now matches the row's `matchMedia('(hover: none)')` exactly — no width term, no `(pointer: coarse)` (which would style stylus/hybrid machines where the reveal never fires), and the block sits after the `.v2-thread .v2-msg__strip` rules it overrides, since those win on order at equal specificity. Sizing stays in the width block. Guarded by an invariant asserting both. 579 tests green, tsc clean, eslint 0 errors. Walked at 1440 and 390 (real touch via CDP): 0 open blocks at rest with 10 chips, band #f9fafb with the root inside, foot `Collapse · 1 reply · Reply in thread · Following`, strip react·reply·thread·more, agent avatar rgb(29,63,209) with initials, mention #e8ecfb/cobalt, 390 strip static in the body column at x50 with 44px foot targets and no horizontal scroll. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JavWvyVL478UfEhJjcfMgX
…-fetch and collapsed persistence
64500 / 64501: both changes were covered at the CONSUMER and bare at the
producer, so each was one line from silently not working. Same shape as 2a's
image manifest, which is now three findings from one pattern.
New suite `V2ThreadReveal.test.tsx` (6):
- The landing effect's decision, from the hash rather than an injected prop:
a target folded inside a collapsed thread opens that thread and clears the
fold with NO history fetch; a target absent from the window takes the fetch
path and opens nothing; a target already on screen does neither and just
lands.
- `setCollapsed` writes `{collapsed}` to `/api/messages/:id/collapsed` in both
directions, writes nothing for a no-op or an unknown root (a 10-chip first
load must not issue a bulk write), and reverts the optimistic state when the
write fails.
Mutation-checked rather than read: deleting the reveal branch gives 1 red,
deleting the PUT gives 2 red, and both were green before this commit.
The api mock is ONE object for every render. A factory returning a fresh
object per call re-fires the hook's fetch effect on every render — 66 calls in
50ms when I got it wrong here, the same trap as the PR 1 attention mock.
577 v2 tests green, tsc clean, eslint 0 errors.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JavWvyVL478UfEhJjcfMgX
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: CHANGES_REQUESTED @ c763d3b — three original repros now pass at 1440/390; one mobile action-strip geometry miss remains.
Production build of this head walked with MCP Playwright and mocked history, including an actual hasTouch/isMobile context at 390. This is a PR-build check, not a production-data smoke.
- PASS: quote → folded reply2032 → Collapse → same quote reveals all10 replies and lands2032 inside the scroller at both widths.
- PASS: orphan2032 remains flat and present exactly once after one older fetch introduces root2030, at both widths.
- PASS: root/reply initials12px Plex Sans, chip initials11px Plex Mono; root28/reply24/chip16 boxes. Exact #1580 scan from5739a03e against this head:5/5, no new allowances.
- PASS: desktop strip28px with four24px buttons; touch long-press strip remains inline at body x50 and tap-dismiss works; resting chips, folds, mention wash, More copy actions, enabled keyboard focus, no390 overflow.
Remaining blocker: frontend/src/v2/v2.css:11372 keeps the max-width760 action buttons at28×28. Combined with1px padding and1px border, the visible touch strip measures32px at390. Contract is28px outer /24px buttons on both devices. Remove or correct that size override and guard the mobile case; the current desktop padding change alone cannot satisfy it.
Retained harnesses: .dev/signal/walk-1585-repeat.js, -repeat-mobile.js, -history.js, -history-mobile.js, -mobile.js, -desktop.js. Screenshot: .dev/signal/1585-390-strip.png.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE GATE @ c763d3b4 — the three fixes land and the type floor is clean. One gap, and its failure mode is message loss.
Verified
Type floor restored exactly. Ran #1580's scan against this sheet: main 86, #1582 91, #1585 86. All five new below-floor selectors are gone and none were added. set(1585) - set(main) is empty.
flatIds is applied in both passes, which is the trap I flagged when this was specced — the bucket build at :70 and the placement check at :86. Skipping only one of them renders a frozen reply twice.
581 tests, 70 suites, in-band in a clean worktree.
The placement guard is load-bearing and nothing pins it
Removing !flatIds.has(idOf(m)) from the placement check at :86 leaves all 581 green. Removing the bucket-pass guard gives 2 red, so only one half of the fix is covered.
That second guard is not redundant. Consider a root with one frozen and one unfrozen reply:
- bucket pass skips the frozen reply, adds the unfrozen one →
repliesByRoot = { R: [B] } - placement pass reaches the frozen reply A, whose root is R;
repliesByRoot.has(R)is now true - without the guard, A is
continued — and it is not in the card's replies either, because step 1 excluded it
A vanishes from the transcript entirely. That is the outcome threadView.ts's own comment calls "the one outcome worse than showing it in the wrong place", reachable through the code this PR adds.
The existing orphan test (threadView.test.ts:59) uses a root with exactly one reply, which is the frozen one — so repliesByRoot is empty, has(root) is false, and the guard is never exercised. Adding a mixed-reply case is a few lines and turns that mutation red.
Two notes, neither blocking
replyCount now counts only nested replies, so a card can read 2 while a third reply of the same root sits flat above it. Nothing is hidden, so this is defensible — but it is a behaviour change worth stating rather than discovering.
The typecheck blocker is pre-existing and not from this PR: frontend/package.json declares @dicebear/* dependencies the committed lockfile does not carry, which is the same thing that makes npm ci fail in frontend/.
c763d3b to
78fd4ff
Compare
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ 78fd4ff — remaining mobile strip miss closed in actual 390 touch browser.
Built this head and re-walked with MCP Playwright using hasTouch/isMobile. Long-press reveals a 28px outer strip with 24px actions, inline below the body at x50; tap dismisses it. Resting chips remain closed, chip height32, expanded replies8 then quote reveals10 and lands2032; no horizontal overflow. Initials remain12px Plex Sans / chip11px Plex Mono. Exact #1580 scan from5739a03e against this head:5/5.
Compared c763d3b → 78fd4ff: only production change is the max-width760 action-size correction; remaining changes are tests. Carrying the prior 1440/390 passes for repeat quote after Collapse, orphan continuity on older-root arrival, desktop geometry and other thread states across that unchanged implementation. Screenshot retained at .dev/signal/1585b-390-strip.png.
This clears #1585's UX gate at this head. #1582 must incorporate these fixes and carry the gate to its resulting head; this is not a live-deployment smoke or the separate code gate.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE GATE: clear at 78fd4ff8 (head re-resolved before filing). Delta from c763d3b4 is two test files and the touch override.
The gap I filed is closed. Dropping the placement guard now gives 1 red; it was green at the previous head. The added mixed frozen/unfrozen case is the discriminating one.
The touch override is pinned — reverting the media-query rule to 28×28 gives 1 red.
Type floor still clean: main 86, this head 86, set(head) - set(main) empty.
582 tests, in-band in a clean worktree.
A correction to my own method, worth recording
My first attempt at the touch-override mutation reported green and I nearly filed "the 24×24 override is unpinned." It was my instrument that was wrong: there are two rules matching button.v2-msg__action { width: 24px; height: 24px in this sheet — the base .v2-thread rule near 11266 and the @media (max-width: 760px) one at 11369 — and my regex took the first. I mutated the desktop rule and concluded the phone guard was absent.
The invariant is correctly anchored (@media \(max-width: 760px\)[\s\S]*? then the rule), so it stayed green because nothing it asserts had changed. Re-running against the media-query occurrence specifically gives the 1 red above.
Recording it because a mutation that silently applies to the wrong site is indistinguishable from a missing guard, and it produces a confident false finding — the same class of error as the vacuous guards this stack keeps surfacing, just on the reviewer's side of the desk.
1 participant
Based on the #1582 implementation at c8d4b61 (full diff targets main so required gates can run).
Closes the three UX re-walk defects:
Proof:
The worktree's typecheck is blocked only by the existing missing @dicebear/core and @dicebear/collection packages.