Repository navigation
feat(conversation): add ConversationMap and useReadingPosition - #123
TheGreatAxios merged 10 commits into
Conversation
ConversationMap shows one tick per turn, marked faint, on screen or current, previews a turn on hover or focus and jumps to it with the conversation's glide. The rail is one tab stop with arrow keys and Enter, and keeping the current tick in view scrolls only the rail. useReadingPosition reports the current and on-screen turns from one IntersectionObserver over the item registry, measuring on the conversation's per-frame scroll snapshots. A jumped-to turn stays current until the reader scrolls.
c2c582c to
20deb73
Compare
20deb73 to
0fc6401
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Automated multi-agent review: independent review + second-agent validation; only confirmed items listed (map reading-position math was verified by execution; everything here is minor/nit).
- minor (MEDIUM)
src/hooks/use-reading-position.ts:92-116(src/lib/reading-position.ts:69) — an unseen pin survives container detach: a pin is released only onelse if (pin.seen),measure()yields nothing withscroller === null, andcurrentTurnreturnspinnedIdunconditionally — so a tick clicked mid-glide followed by a fast detach (before the IntersectionObserver entry lands) leaves a stale "current" tick. Smallest fix: release the pin when the scroller detaches (or whenmeasureyields NOTHING with!pin.seen). - minor (test gap)
src/ui/conversation-map.tsx:115-132— the keep-in-view effect has no resize trigger (deps[currentId, hidden, reducedMotion], no ResizeObserver): a viewport resize can leave the current tick out of view untilcurrentIdchanges. Observe the rail's height. - nit
conversation-map.tsx:250—data-currentis written every tick and read nowhere (no CSS, no test); drop it or use it (aria-current="location"already carries the state). - nit
conversation-map.tsx:231, 255— tick targets are 14px × 24px, below WCAG 2.5.8's 24×24 touch minimum; not an axe failure, but worth a note in the a11y lane.
0fc6401 to
ea5bea3
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Request changes
Adds a conversation minimap and useReadingPosition, which pins the current turn across a jump.
Findings
-
src/hooks/use-reading-position.ts:202-206— A jump that never arrives leaves that turn pinned asaria-current.jumpTosets the pin witharmed: false(use-reading-position.ts:219-220). A reader gesture still inside its quiet window cancels the glide (conversation.tsx:493-497, andopenGestureatconversation.tsx:472-474cancels even whenscrollTopdoes not move). The pin only drops when it is armed and the snapshot isbyReaderwithdelta !== 0, so a cancel from momentum that started before the click, or a wheel/touch that stops the glide without moving, keeps the highlight on the target while the view stays put.currentTurntrustspinnedId(reading-position.ts:69). Clear the pin when the glide is cancelled before the target is seen, or release it on the same input that cancels the glide. -
src/hooks/use-reading-position.ts:220— A tick for a turn that is not mounted yet does not jump, including whenonSelectmounts it.jumpTosets the pin tonullwhenitems.get(id)is missing, andscrollToItemglides toitemTarget, which isnulluntil the row is registered (conversation.tsx:656-658). The tick callsjumpTobeforeonSelect(conversation-map.tsx:288-291), so a host that mounts the row fromonSelect(the history window) gets no second attempt. Keep the pin and scroll when that id appears in the registry the hook already subscribes to.
d3e99fb to
9b71d32
Compare
A jump pins its turn as current until the reader scrolls or the turn leaves the view. When momentum, a tap or a wheel that moved nothing stopped the glide before the turn came on screen, nothing released the pin, and the map marked a turn that was not on screen as current until the reader next scrolled. The jump now hears from the glide whether it arrived, and drops a pin whose turn was never seen. Document that the map's turns must be mounted: a tick for a turn that is not mounted does not jump, even when onSelect mounts it.
9b71d32 to
e85396c
Compare
|
Thanks for the review. The first point is fixed. The second is now documented. 1. A jump that was cut short left its turn marked current
2. A tick for a turn that isn't mounted doesn't jump
Commit: |
When the turn of the focused tick left `turns`, as when a history window trims it or the host deletes it, its button unmounted and focus fell to the page body; the next Tab started from the top of the page. A layout effect now notices the focused turn is gone and, if focus is on the body, moves it to the rail's tab stop (the current tick, or the first), or to the conversation's log when the rail itself is gone.
Escape was handled only by the map's own keydown, so a preview card opened by resting the mouse on a tick stayed over the content while focus was in the composer or the log. Content shown on hover has to be dismissable without moving the pointer or focus. While a preview is open, Escape anywhere in the document now closes it.
When the rail scrolled, the preview card's position was measured inside a setPreview updater. React runs a queued updater during the next render, so the DOM was read during render, and StrictMode ran the reads twice. The scroll handler now measures the tick of the preview it sees and passes a pure updater that applies the result only if that preview is still open, so a close queued by a wheel in the same frame still wins.
With a mouse, map ticks are under 24px tall, and the comment said only that this was fine for a mouse. It now names the WCAG 2.5.8 exception the dense rail relies on: scrolling the conversation is an equivalent way to reach every turn a tick jumps to. Touch keeps its 24px ticks.
The map spec declared its own container, settled, offsetOf and wheel, copies of the helpers the other conversation specs import. It now imports them from the shared module.
ConversationMap took its props as the nav's, ref included, and spread them onto the nav. React 19 passes ref as a prop, but React 18 strips it, so a ref never reached the nav on React 18, the lower end of the peer range. It is now wrapped in forwardRef and the ref set on the nav.
e85396c to
246f9ce
Compare
Round 3 fixesA fresh review pass over the stack turned up the points below; each is fixed in a new commit at the end of this branch, and the stack is rebased onto it.
|
246f9ce to
9e56da9
Compare
|
Triage: Fail Waiting on:
Stacked on #122: only this pull request's own changes were checked. Notes:
|
Superseded by the completed stacked review; blocking fixes were addressed and the rebased stack is being landed.
ConversationMap shows one tick per turn, marked faint, on screen or current, previews a turn on hover or focus and jumps to it with the conversation's glide. The rail is one tab stop with arrow keys and Enter, and keeping the current tick in view scrolls only the rail. useReadingPosition reports the current and on-screen turns from one IntersectionObserver over the item registry, measuring on the conversation's per-frame scroll snapshots. A jumped-to turn stays current until the reader scrolls.
Stacked on #122: review only this commit. Part of #94.
Notes for review
Verification
bun run typecheckandbun run linton its own (the stack is checked commit by commit).bun run check,bun run build,bun run dep-guard,bun run contrast-testandbun run test:e2e(Playwright specs and the Storybook axe sweep in light and dark) pass.Closes #101