Skip to content

feat(web): conversation turn-rail minimap - #2285

Merged
serena-ruan merged 8 commits into
mainfrom
message_scroller
Jul 13, 2026
Merged

feat(web): conversation turn-rail minimap#2285
serena-ruan merged 8 commits into
mainfrom
message_scroller

Conversation

@serena-ruan

Copy link
Copy Markdown
Collaborator

Related issue

N/A

Summary

Adds a left-edge turn-rail minimap to the conversation view: one tick per user turn, with a hover preview and click-to-scroll. The rail tracks your position like a scrollbar thumb and eagerly pages older history so it shows a useful run of ticks on load.

While building it I found and fixed several issues:

  • Chronological history ordering — the eager loader prepended fetched blocks one-by-one, which reversed each page and scrambled the transcript (a mid-conversation prompt could surface at the top with a hard scroll-stop above it, hiding real history). Now each page is prepended as a group.
  • Rail parks on click — tracking now scrolls the active run into view instead of always re-centering, so clicking a tick you scrolled to leaves the rail put while the transcript navigates.
  • Loads at the bottom — tracking re-runs when the tick count changes, so a fresh load lands with the last turn active (not stuck at the top).
  • No load flash — the rail fades in once the eager back-fill settles, instead of flashing 2→N ticks.
  • Ergonomics — wider hover preview; full-pitch clickable tick band so the click hit-area matches the hover zone.

Responsive: desktop shows the rail and drops the floating ↑/↓ nav buttons; mobile hides the rail and keeps the buttons (no hover on touch). Keyboard ⌘⌥↑↓ nav is unchanged on all sizes.

Test Plan

  • cd web && npm test — full suite green (3804 pass); new tests below.
  • cd web && npm run buildtsc -b && vite build clean.
  • Live verification via headless Chrome + CDP against a local dev server across several real sessions (dense multi-turn, short, and 84-turn):
    • transcript loads in correct chronological order (previously scrambled);
    • rail lands at the bottom with the last tick active on load;
    • clicking a scrolled-to tick keeps the rail parked while the transcript scrolls;
    • normal scrolling still tracks the rail to top/bottom;
    • fade-in reveal (no 2→N flash); wider preview; clicking anywhere in a tick's band navigates;
    • desktop hides the ↑/↓ buttons, mobile hides the rail and keeps them.

Demo

Verified via headless-browser (CDP) measurements and screenshots during development. Behaviour summary — desktop: rail visible, nav buttons removed, gap between ticks and text; mobile: rail hidden, ↑/↓ nav retained.

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

Unit tests added: chronological-order regression + eager-load coverage in chatStore.test.ts (verified they fail if the reversal bug is reintroduced), TurnRail render/interaction contract in TurnRail.test.tsx, and nav className forwarding in UserMessageNav.test.tsx.

The rail's scroll-positioning behaviours (thumb tracking, park-on-click, load-at-bottom, fade-in reveal, desktop gap) depend on real layout — offsetTop/clientHeight are 0 in jsdom — so those were verified live via CDP rather than unit-tested, hence the manual-verification box.

Changelog

Conversation view gains a turn-rail minimap for jumping between messages

This pull request and its description were written by Isaac.

@github-actions github-actions Bot added the size/XL Pull request size: XL label Jul 9, 2026
Comment thread web/src/pages/TurnRail.tsx Fixed
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

UI Snapshot doesn't match the committed baseline.

If this UI change is intentional, update the baseline — each path renders in the same pinned image, so the result matches this gate:

  • Label the PR (recommended): add the update-ui-snapshot label — the bot regenerates the baseline in the pinned image, pushes it back here, and re-runs the checks.
  • Locally with Docker: run tests/e2e_ui/visual/regen_baseline_docker.sh, review the PNG, then commit + push.

Diff PNGs (expected_=baseline, actual_=your render, diff_) are in the run artifact. Full guide: tests/e2e_ui/visual/README.md.

@omnigent-ci

omnigent-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Review: feat(web): conversation turn-rail minimap

Two independent cross-vendor reviews converged on one genuine blocking bug; the rest of the change is sound.

1. Blocking issues

B1 — Persistent eager-load fetch failure causes an unbounded retry loop (and permanently hides the rail).
web/src/store/chatStore.ts (catch/commit in loadHistoryUntilUserMessages) + web/src/pages/TurnRail.tsx (eager-load effect).

The eager-load effect auto-fires with no user gesture whenever the invariant holds:

useEffect(() => {
  if (turns.length >= INITIAL_TURNS || !hasMoreHistory || loadingMoreHistory) return;
  void useChatStore.getState().loadHistoryUntilUserMessages(INITIAL_TURNS);
}, [turns.length, hasMoreHistory, loadingMoreHistory]);

On a persistent fetch failure (e.g. a 500 from fetchSessionItemsPage), the store's catch path commits progress but deliberately keeps hasMore true and clears loadingMoreHistory:

} catch {
  if (stale()) return;
  // Keep hasMore true so scroll-up can retry the rest; commit progress.
  commit();   // loadingMoreHistory:false, hasMoreHistory:true, blocks unchanged
}

Trace: effect fires → loadingMoreHistory goes truefalse, hasMoreHistory stays true, turns.length unchanged (empty older) → the effect's dependencies changed and its guard still fails → it re-fires immediately, hammering the failing endpoint in a tight loop for as long as the rail is mounted. MAX_EAGER_PAGES does not help — the throw exits the inner page loop before the cap matters, and the effect re-invokes the whole function each round.

This is worse than a scroll-triggered fetch because it needs no user input. It also causes a secondary wedge: with turns.length < INITIAL_TURNS && hasMoreHistory stuck true, revealed never latches, so the rail stays opacity-0 (permanently invisible) while still burning network. Notably, the existing loadMoreHistory does the opposite on error — set({ loadingMoreHistory: false, hasMoreHistory: false }) — with a comment saying it exists precisely to stop the listener re-triggering; the new function reintroduces that guarded-against failure mode.

Fix direction: for the eager auto-loader, don't preserve hasMoreHistory: true on error (or gate the effect behind an "eager attempt errored" ref / backoff), so a failed fetch can't

Review: feat(web): conversation turn-rail minimap

Two independent cross-vendor reviews converged on one genuine blocking bug; the rest of the change is sound.

1. Blocking issues

B1 — Persistent eager-load fetch failure causes an unbounded retry loop (and permanently hides the rail).
web/src/store/chatStore.ts (catch/commit in loadHistoryUntilUserMessages) + web/src/pages/TurnRail.tsx (eager-load effect).

The eager-load effect auto-fires with no user gesture whenever the invariant holds:

useEffect(() => {
  if (turns.length >= INITIAL_TURNS || !hasMoreHistory || loadingMoreHistory) return;
  void useChatStore.getState().loadHistoryUntilUserMessages(INITIAL_TURNS);
}, [turns.length, hasMoreHistory, loadingMoreHistory]);

On a persistent fetch failure (e.g. a 500 from fetchSessionItemsPage), the store's catch path commits progress but deliberately keeps hasMore true and clears loadingMoreHistory:

} catch {
  if (stale()) return;
  // Keep hasMore true so scroll-up can retry the rest; commit progress.
  commit();   // loadingMoreHistory:false, hasMoreHistory:true, blocks unchanged
}

Trace: effect fires → loadingMoreHistory goes truefalse, hasMoreHistory stays true, turns.length unchanged (empty older) → the effect's dependencies changed and its guard still fails → it re-fires immediately, hammering the failing endpoint in a tight loop for as long as the rail is mounted. MAX_EAGER_PAGES does not help — the throw exits the inner page loop before the cap matters, and the effect re-invokes the whole function each round.

This is worse than a scroll-triggered fetch because it needs no user input. It also causes a secondary wedge: with turns.length < INITIAL_TURNS && hasMoreHistory stuck true, revealed never latches, so the rail stays opacity-0 (permanently invisible) while still burning network. Notably, the existing loadMoreHistory does the opposite on error — set({ loadingMoreHistory: false, hasMoreHistory: false }) — with a comment saying it exists precisely to stop the listener re-triggering; the new function reintroduces that guarded-against failure mode.

Fix direction: for the eager auto-loader, don't preserve hasMoreHistory: true on error (or gate the effect behind an "eager attempt errored" ref / backoff), so a failed fetch can't immediately re-arm the effect.

2. Security vulnerabilities

No issues found.

  • Both DOM-selector construction sites interpolate itemId through CSS.escape (useUserMessageNav.ts, TurnRail.tsx), so a crafted id can't break out of the attribute selector.
  • userText / responsePreview / aria-label render as React text nodes / plain attributes — no dangerouslySetInnerHTML, no XSS from conversation content.
  • No secrets or unsafe DOM sinks.

3. Non-blocking notes

  • Over-fetch / overshoot in the stop condition (chatStore.ts). The early-return guard checks countUsers(start.blocks) >= minUserMessages, but the loop break uses countUsers(older) >= minUserMessages — counting only newly buffered users and ignoring those already in state. If state already holds N users, it fetches up to minUserMessages more, overshooting by N and violating the "≤20 ticks initially" intent (one reviewer rated this blocking, but the effect is only a handful of extra items — functionally harmless). Prefer stopping on existing + newly-fetched real-turn count.
  • Blank preview when a system-marker user bubble precedes the reply (ChatPage.tsx turns useMemo). The preview scan breaks on any next.kind === "user", including system bubbles that are themselves excluded from ticks, so a tick can end up with an empty responsePreview.
  • Inconsistent error contract between the two loadersloadMoreHistory self-heals by disabling fetches on error; loadHistoryUntilUserMessages keeps hasMore true. Worth reconciling / sharing the policy (directly related to B1).
  • previewTop doesn't follow a rail auto-scroll — computed once on hover; if thumb-tracking smooth-scrolls the rail while the pointer is stationary, the preview box detaches until the next mouseenter. Minor visual nit.
  • Magic-number coupling — the 32px fade edge lives both in index.css (.turn-rail-fade) and as const FADE = 32 in TurnRail.tsx; changing one silently desyncs the thumb-tracking math from the visual mask.

Cleanly verified as correct (called out because they were scrutinized): hooks all run before the turns.length < 2 early return (no conditional-hook violation); scroll/thumb/fetch-on-top effects clean up rAF + listeners and carry turns in their deps (no stale closure, null-guarded on scrollEl/railRef); seenNew/seen dedup prevents duplicate or dropped blocks; cursor advancement and the empty-page break are correct; oldestItemId stays consistent.

4. Summary

A well-structured, well-tested feature with good comments and no security concerns. There is one blocking bug: on a persistent history-fetch failure, the auto-firing eager-load effect and the store's "keep hasMore true on error" policy combine into an unbounded retry loop that also leaves the rail permanently hidden. Fix the eager-loader's error handling (don't re-arm the effect on failure), and address the over-fetch overshoot; the remaining notes are minor. Recommend changes before merge.


Automated review by Polly · workflow run

serena-ruan added a commit that referenced this pull request Jul 10, 2026
Addresses the Polly review's blocking bug and non-blocking notes plus the
CodeQL warning on PR #2285:

- Blocking: loadHistoryUntilUserMessages now clears hasMoreHistory on fetch
  failure (matching loadMoreHistory), so the rail's auto-firing eager-load
  effect can't re-arm into an unbounded retry loop that also left the rail
  permanently hidden.
- Over-fetch overshoot: count users already in state toward the target so we
  only top up to minUserMessages instead of overshooting by the existing count.
- Blank preview: the preview scan now stops only at a real (non-system) user
  turn, so a system-marker bubble before the reply no longer strands a turn
  with an empty preview.
- CodeQL useless assignment: drop the always-overwritten `next` initializer.
- FADE magic-number coupling: drive the CSS fade mask from --turn-rail-fade so
  the mask width and thumb-tracking math share one constant.
- previewTop drift: reposition the hover preview when the rail auto-scrolls
  under a stationary pointer.

Co-authored-by: Isaac
@serena-ruan serena-ruan added the update-ui-snapshot Used to tell CI that the baseline UI snapshot should be regenerated label Jul 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Regenerated the visual baseline(s) in the pinned Playwright image and pushed to this PR. CI will re-run on the new commit.

@github-actions github-actions Bot removed the update-ui-snapshot Used to tell CI that the baseline UI snapshot should be regenerated label Jul 10, 2026
serena-ruan added a commit that referenced this pull request Jul 10, 2026
Addresses the Polly review's blocking bug and non-blocking notes plus the
CodeQL warning on PR #2285:

- Blocking: loadHistoryUntilUserMessages now clears hasMoreHistory on fetch
  failure (matching loadMoreHistory), so the rail's auto-firing eager-load
  effect can't re-arm into an unbounded retry loop that also left the rail
  permanently hidden.
- Over-fetch overshoot: count users already in state toward the target so we
  only top up to minUserMessages instead of overshooting by the existing count.
- Blank preview: the preview scan now stops only at a real (non-system) user
  turn, so a system-marker bubble before the reply no longer strands a turn
  with an empty preview.
- CodeQL useless assignment: drop the always-overwritten `next` initializer.
- FADE magic-number coupling: drive the CSS fade mask from --turn-rail-fade so
  the mask width and thumb-tracking math share one constant.
- previewTop drift: reposition the hover preview when the rail auto-scrolls
  under a stationary pointer.

Co-authored-by: Isaac
@serena-ruan serena-ruan added the update-ui-snapshot Used to tell CI that the baseline UI snapshot should be regenerated label Jul 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Regenerated the visual baseline(s) in the pinned Playwright image and pushed to this PR. CI will re-run on the new commit.

@github-actions github-actions Bot removed the update-ui-snapshot Used to tell CI that the baseline UI snapshot should be regenerated label Jul 10, 2026
@serena-ruan

Copy link
Copy Markdown
Collaborator Author

/review

@omnigent-ci

omnigent-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

1. Rail silently fails to render (and stops paging) on sessions with [System: …] bubbles — a count-definition mismatch between the loader and the ticks.

The eager loader and the rail disagree on what "a user turn" is:

  • The turns memo in ChatPage.tsx builds one tick per non-system user bubble — it skips isSystemBubble(b).
  • But loadHistoryUntilUserMessages in chatStore.ts measures progress with countUsers(), which counts every user_message block — and itemsToBlocks turns every role === "user" item (including [System: …] markers) into a user_message block.

So countUsers() counts system messages that never become ticks. On a session with enough interspersed system notifications (common in long agent runs):

  • loadHistoryUntilUserMessages(20) hits 20 block-users and stops, leaving hasMoreHistory: true but real turns.length < 20.
  • The eager-load effect (turns.length < INITIAL_TURNS && hasMoreHistory && !loadingMoreHistory) fires again → but inside the loader existingUsers (block count) >= minUserMessages early-returns before any fetch → no more history is ever loaded.
  • The reveal latch needs turns.length >= INITIAL_TURNS || !hasMoreHistory; neither ever becomes true → revealed stays false → the rail is pinned at opacity-0 and never appears, even with many real turns.

There's no infinite loop (deps are stable so the effect won't re-run), but the feature quietly disappears on exactly the long, system-message-heavy conversations it's most useful for. Fix by making the loader's progress metric count the same thing the ticks do (non-system user turns), so the target, the reveal latch, and the tick count all agree.

2. Eager loader can drain the entire conversation on sparse-turn sessions.

Because the effect re-fires whenever loadingMoreHistory flips back to false while turns.length < INITIAL_TURNS && hasMoreHistory, a session whose 20th real user turn lies far back (few user turns spread across a large history) will page back-to-back — up to MAX_EAGER_PAGES × EAGER_PAGE_LIMIT (10×200) items per invocation, invocation after invocation — until it either reaches 20 turns or exhausts history. The per-call MAX_EAGER_PAGES backstop doesn't bound the across-invocation total, so on such sessions the "≤20 ticks initially" intent degrades into eagerly loading the whole transcript on load. Bounded (it terminates at history exhaustion) but a real load-cost regression worth gating (e.g. cap total eager invocations, or stop once a page yields no new rail turns).

Security vulnerabilities

None. CSS.escape is used on the data-user-message-id selector; no injection, auth, secret-exposure, path, or SSRF surface is touched. No dependency/lockfile or extras changes in this diff.

Non-blocking notes

  • scrollToUserMessage refactor is contract-safe. Both callers (nav hook and rail) still pass flashUserMessage; the if (!flash) return guard and flash?.() preserve prior behavior. The console.warn rename is cosmetic.
  • UserMessageNavConnected → md:hidden is correct. Keyboard ⌘⌥↑↓ is a window keydown listener independent of button visibility, so it works at all sizes. Edge case: a single-turn desktop conversation shows no nav UI at all (buttons md:hidden, rail returns null for turns.length < 2) — but there's nothing to navigate, so likely intended.
  • TurnRail effect cleanup is complete — every addEventListener has a matching remove, the rAF is cancelled, the settle setTimeout is cleared, and tickRefs entries are deleted via the ref-callback else branch. No leaks spotted.
  • Group-prepend ordering is correct. older.unshift(...pageBlocks) with backward cursoring preserves global chronological order across pages; the added regression tests confirm it and the seenNew + commit-time dedupe is adequate. The error-path hasMore = false; commit() correctly mirrors loadMoreHistory's anti-retry-loop policy. The stale early-returns that leave loadingMoreHistory: true self-heal because every path flipping conversationId/historyGeneration also resets it.

Summary

Solid, well-tested, well-commented feature with clean cross-file plumbing and no security concerns. The blocking issue is a genuine definitional mismatch: the eager loader counts all user blocks (including [System: …] markers) while the rail counts only real user turns, which on system-message-heavy sessions leaves the rail permanently hidden and stops paging — and, on sparse-turn sessions, the same eager-load effect can drain far more history than the "≤20 ticks" design intends. Reconciling the loader's target metric with the rail's turn definition (and bounding total eager paging) resolves both. Everything else — the scroll refactor, responsive gating, ordering/dedup logic, and effect cleanup — checks out.


Automated review by Polly · workflow run

serena-ruan and others added 6 commits July 13, 2026 12:19
A left-edge vertical minimap: one tick per user turn, with a hover
preview and click-to-scroll. The rail tracks your position like a
scrollbar thumb and eagerly pages older history so it shows a useful
run of ticks on load.

Fixes found while building it:
- History pages now load in chronological order. The eager loader used
  to prepend fetched blocks one-by-one, reversing each page and
  scrambling the transcript (a mid-conversation prompt could surface at
  the top with a hard scroll stop above it).
- Rail tracking scrolls the active run into view instead of always
  re-centering, so clicking a tick you scrolled to leaves the rail
  parked while the transcript navigates.
- Tracking re-runs when the tick count changes, so a fresh load lands
  at the bottom with the last turn active.
- Rail fades in once the eager back-fill settles (no 2→N tick flash).
- Wider hover preview; full-pitch clickable tick band (hover == click
  hit area).

Responsive: desktop shows the rail and drops the floating up/down nav
buttons; mobile hides the rail and keeps the buttons (no hover on
touch). Keyboard nav is unchanged.

Tests: chronological-order regression + eager-load coverage in
chatStore, TurnRail render/interaction contract, and nav className
forwarding.

Co-authored-by: Isaac
Addresses the Polly review's blocking bug and non-blocking notes plus the
CodeQL warning on PR #2285:

- Blocking: loadHistoryUntilUserMessages now clears hasMoreHistory on fetch
  failure (matching loadMoreHistory), so the rail's auto-firing eager-load
  effect can't re-arm into an unbounded retry loop that also left the rail
  permanently hidden.
- Over-fetch overshoot: count users already in state toward the target so we
  only top up to minUserMessages instead of overshooting by the existing count.
- Blank preview: the preview scan now stops only at a real (non-system) user
  turn, so a system-marker bubble before the reply no longer strands a turn
  with an empty preview.
- CodeQL useless assignment: drop the always-overwritten `next` initializer.
- FADE magic-number coupling: drive the CSS fade mask from --turn-rail-fade so
  the mask width and thumb-tracking math share one constant.
- previewTop drift: reposition the hover preview when the rail auto-scrolls
  under a stationary pointer.

Co-authored-by: Isaac
Scrolling the rail up near its top triggers loadMoreHistory, which grows
`turns` and re-runs the thumb-tracking effect. That effect would smooth-scroll
the rail back to the transcript's visible run, yanking the user away from the
older ticks they were browsing. Track pointer-over-rail state and skip the
auto-scroll while the user is interacting, so a history fetch can't fight the
scroll.

Co-authored-by: Isaac
Scrolling the rail drags ticks under a stationary cursor, firing onMouseEnter
on each and flickering the preview through every turn. Suppress hover updates
while the rail is mid-scroll and settle onto the tick under the cursor once
scrolling comes to rest, so the preview only changes when the user stops.

Co-authored-by: Isaac
Scrolling the rail drags ticks under a stationary cursor, firing onMouseEnter
on each and flickering the preview through every turn. A real hover moves the
cursor; a scroll-induced enter does not — so ignore enter events whose cursor
position matches the last accepted hover, and settle onto the tick under the
cursor once scrolling comes to rest. The preview now only changes when the
user actually moves the pointer.

Adds tests for both the moved-cursor hover and the ignored same-position enter.

Co-authored-by: Isaac
@serena-ruan serena-ruan added the update-ui-snapshot Used to tell CI that the baseline UI snapshot should be regenerated label Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Regenerated the visual baseline(s) in the pinned Playwright image and pushed to this PR. CI will re-run on the new commit.

@github-actions github-actions Bot removed the update-ui-snapshot Used to tell CI that the baseline UI snapshot should be regenerated label Jul 13, 2026
@serena-ruan

Copy link
Copy Markdown
Collaborator Author

/review

@omnigent-ci

omnigent-ci Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

TurnRail minimap — review

Scope reviewed: new web/src/pages/TurnRail.tsx, the new loadHistoryUntilUserMessages action in chatStore.ts, the jumpToscrollToUserMessage refactor in useUserMessageNav.ts, the ChatPage.tsx wiring, and the new tests. Cross-checked against source at main.

1. Blocking issues

B1 — Count-basis mismatch: the rail can stay permanently hidden. The rail's turns memo (ChatPage.tsx) excludes system-marker user bubbles (isSystemBubble), but loadHistoryUntilUserMessages counts every user_message block via countUsers (chatStore.ts:1858) and tops up to minUserMessages = INITIAL_TURNS (20). [System: …] / tool-reply messages arrive as user-role blocks — counted by the loader, dropped from turns. In any conversation whose recent window holds ≥1 system-marker user message while older real history still exists:

  1. loadHistoryUntilUserMessages(20) hits 20 user_message blocks and early-returns at existingUsers >= minUserMessages (chatStore.ts:1864) without clearing hasMoreHistory.
  2. turns.length is still < 20, so the eager-load effect (TurnRail.tsx:193, gated on turns.length >= INITIAL_TURNS) keeps re-firing the loader, which no-ops each time (settles — no infinite loop).
  3. The reveal effect (TurnRail.tsx:202-203) needs turns.length >= INITIAL_TURNS || !hasMoreHistory. Both stay false → revealed never latches and the rail is stuck at opacity-0 forever.

This is a common shape for agent/sub-agent sessions, not a rare edge. Fix by counting the same "real user turn" predicate in both places (have loadHistoryUntilUserMessages apply the isSystemBubble exclusion), or gate the eager-load/reveal effects on the loader's own "history exhausted" signal (!hasMoreHistory) rather than turns.length.

B2 — Mobile runs the desktop-only eager backfill for no benefit. TurnRail is mounted unconditionally (ChatPage.tsx:1790) and is only CSS-hidden on mobile (max-md:hidden, TurnRail.tsx:316). CSS hiding does not stop effects, so the eager-load effect (TurnRail.tsx:193) still fires loadHistoryUntilUserMessages on mobile — fetching up to MAX_EAGER_PAGES × EAGER_PAGE_LIMIT (10 × 200 = 2000) older items per chat open, on the smallest-bandwidth clients, to populate a rail the user can never see. Gate the mount or the eager-load effect on the viewport (useIsMobileViewport, already imported in ChatPage.tsx), not just CSS. (Note: this compounds B1 — on mobile the wasted fetch has zero upside.)

2. Security vulnerabilities

None found. scrollToUserMessage runs the itemId through CSS.escape before the selector; elementFromPoint/closest reads are DOM-local. No injection, auth, or data-exposure surface in the diff. No lockfile/dependency changes.

3. Non-blocking notes

  • Invisible-but-clickable rail before reveal. The inner rail is pointer-events-auto while the outer stays opacity-0 until revealed. An opacity-0 element still receives clicks, so ticks are silent hit targets before reveal (and permanently, under B1). Consider gating pointer-events on revealed.
  • Settle timer can re-hover after mouse-leave. onMouseLeave clears hoveredId, but the ~120ms settle timer (TurnRail.tsx scroll-settle effect) can fire afterward using pointerRef + elementFromPoint and re-handleHover a tick the pointer already left. Minor visual flicker.
  • pointerRef initial {0,0} — a scroll settle before any pointermove calls elementFromPoint(0,0). Low-risk since a rail scroll implies prior interaction.
  • Chronological-order test doesn't exercise what it claims. chatStore.test.ts's "keeps items in chronological order" test uses 15 turns × 3 = 45 items with EAGER_PAGE_LIMIT = 200, so the loader fetches everything in one page. The cross-page older.unshift(...) assembly and the MAX_EAGER_PAGES loop — the actual regression surface the comment names — are never hit; only within-page order is tested. Seed more items than the page limit (or shrink the limit) to genuinely cover multi-page assembly. The other two new store tests (target-met no-op, failure-disables-history) do cover their claims.
  • tickRefs ref-callback churn — the inline ref callback changes identity every render, so React detach/re-attaches every tick each render (Map thrash). Correct (removed turns cleaned via the null call), just wasteful; a stable callback avoids it.

4. Summary

Well-structured feature with genuinely careful edge-case handling (stale guards, commit-on-error to avoid a fetch loop, listener cleanup, empty-preview handling) and good comments. Two blocking items should be resolved before merge, both stemming from the same root cause — the rail derives turns from non-system user bubbles while the eager loader and its gating count all user-role blocks: (B1) that mismatch can leave the rail permanently hidden in ordinary agent/system-message conversations, and (B2) the mount/effect isn't viewport-gated so mobile pays a large eager-fetch cost for an invisible rail. No security concerns. Fixing the count-basis mismatch (and viewport-gating the effect) addresses the bulk of the risk; the test-coverage gap on cross-page ordering is worth closing while you're in there.


Automated review by Polly · workflow run

Addresses the second Polly review on the turn-rail PR:

- B1: the rail derives ticks from non-system user turns, but the eager history
  loader counted every user-role block — including [System: …] markers. In
  agent/sub-agent sessions the loader could hit its target on marker blocks and
  early-return while the rail had too few ticks, leaving hasMoreHistory set and
  the rail stuck at opacity-0 forever. Share one isSystemUserContent predicate
  (new in systemMessage.ts) between ChatPage's turn derivation and the loader's
  count so both agree on what a real turn is.
- B2: TurnRail was only CSS-hidden on mobile, so its eager backfill (up to 2000
  items/open) still ran on the smallest-bandwidth clients for a rail they can't
  see. Gate the mount on useIsMobileViewport so mobile skips it entirely.
- Gate the inner rail's pointer-events on `revealed` so the invisible rail is
  not a silent click target before it fades in.
- Skip the scroll-settle re-hover once the pointer has left the rail; start
  pointerRef off-screen so a pre-move settle resolves to no element.
- Use a stable tick ref callback to avoid per-render Map churn.

Tests: isSystemUserContent unit tests; a chatStore regression proving markers
don't count toward the target; a genuine multi-page (>200 item) cross-page
assembly/order test; and TurnRail pointer-events reveal-gating tests.

Co-authored-by: Isaac
@serena-ruan

Copy link
Copy Markdown
Collaborator Author

/review

@omnigent-ci

omnigent-ci Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Review: feat(web): conversation turn-rail minimap

Reviewed the full diff plus surrounding source across two independent passes. This is a carefully written, well-tested frontend change. The genuinely tricky paths — stale-generation guards, error-path hasMoreHistory disabling, dedup, chronological multi-page assembly, and the scroll-vs-hover distinction — are all handled correctly.

1. Blocking issues

None. Each risk area was checked against the actual code:

  • loadHistoryUntilUserMessages pagination (chatStore.ts) terminates correctly: the loop guards on hasMore && cursor, breaks on an empty page (!page.items[0]?.id), and is bounded by MAX_EAGER_PAGES. The cursor advances via page.items[0]?.id (oldest item), so it strictly pages backward. When hasMoreHistory is already false, the loop no-ops and commit() runs with no fetch — no wasted request path or error re-entry.
  • Error path commits gathered pages and clears hasMoreHistory, breaking the auto-fire retry loop from the rail's effect (covered by the conv_fail test).
  • Chronological ordering fix — whole-page group older.unshift(...pageBlocks) with in-order per-page assembly is correct and regression-tested (conv_order, conv_multi).
  • isSystemUserContent refactor is behavior-identical to the old inline isSystemBubble (same attachment short-circuit → text join → ATTACHED_RE strip → parseSystemMessage). ATTACHED_RE is only used with .replace(), so the /g lastIndex statefulness footgun doesn't apply. The loader counts turns with the same predicate the rail ticks off, so they can't drift and wedge the rail hidden (conv_sys test).
  • Mobile mount-gatinguseIsMobileViewport is a real useSyncExternalStore-backed reactive hook, so !isMobileViewport && <TurnRail> mounts/unmounts (with effect cleanup) across the md breakpoint, and the eager backfill never runs on mobile.
  • Effect cleanup — every addEventListener, requestAnimationFrame, and setTimeout in TurnRail.tsx has a matching removal/cancel; thumb-tracking math clamps to [0, scrollHeight - clientHeight] and no-ops on sub-pixel deltas.

2. Security vulnerabilities

None. No new network calls, deserialization, auth boundaries, or injection surfaces. DOM anchor lookups use CSS.escape on the itemId, and itemIds are internally generated. The binary PNG snapshot change is a test baseline update. No lockfile/dependency changes in this diff.

3. Non-blocking notes

  1. tickRefs retains detached DOM nodes across session switches (TurnRail.tsx). setTickRef never deletes on unmount (by design), but on switchTo all itemIds change, so old entries are never overwritten and leak references to detached <button>s for the component's lifetime. Bounded and minor; a WeakMap-keyed-by-id or a cleanup keyed to the current turns set would avoid it.
  2. Per-token forced layout during streaming (visible-tracking effect, deps [scrollEl, turns]). turns is a fresh array on every bubbles change (every stream token), and the effect-triggered recompute() runs synchronously (only the scroll handler is rAF-throttled), doing a querySelector + getBoundingClientRect per turn. On a long, scrolled-back rail this is repeated layout work per token — consider a more stable turns memo or throttling the recompute.
  3. isSystemUserContent vs UserBubble edge case. For a message combining an @-mention chip and literal [System: …] text (e.g. [Attached: relative/path] [System: …]), isSystemUserContent returns true (marker) while UserBubble renders it as a normal bubble (because mentionedChips.length > 0 short-circuits its system check). This is pre-existing — behavior-identical to the old isSystemBubble — and the scenario is contrived, but the two predicates could be unified (or the helper could account for mention chips) to remove the latent disagreement.
  4. Keyboard focus leaves the preview stuck (onFocus sets hoveredId, nothing clears it on blur, and focus doesn't set interactingRef). Tabbing through ticks leaves the preview visible after focus moves away and doesn't suppress thumb-tracking auto-scroll. Minor UX / a11y polish.
  5. Duplicated ATTACHED_RE now lives in both ChatPage.tsx and systemMessage.ts. The mirroring is intentional and commented, but the two can silently drift; exporting one would be safer.

4. Summary

Solid, defensively engineered PR with good regression coverage on exactly the bugs it set out to fix (chronological ordering, park-on-click, load-at-bottom, no-flash reveal). No blocking correctness or security issues found. The layout-dependent scroll-positioning behaviors are reasonably deferred to live CDP verification given jsdom's 0-valued layout metrics. The non-blocking items (tick-ref retention, per-token layout thrash on the tracking effect) are worth a follow-up but don't gate merge. Recommend merge once the author weighs notes 1–2.


Automated review by Polly · workflow run

@serena-ruan
serena-ruan merged commit e8bee52 into main Jul 13, 2026
32 of 33 checks passed
@serena-ruan
serena-ruan deleted the message_scroller branch July 13, 2026 06:43
@github-actions github-actions Bot added the no-doc-update Merged PR does not need a docs update label Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ Doc impact: no-doc-update

This adds an internal web UI TurnRail minimap component plus supporting store/hook refactors and tests — a frontend enhancement that doesn't change install, integrations, built-in agents/policies, or any documented config surface.

Auto-classified on merge. Set the label manually before merging to override. · run

serena-ruan added a commit that referenced this pull request Jul 13, 2026
Non-blocking follow-ups from the #2285 review, all scoped to TurnRail.tsx:

- rAF-throttle the visible-tracking recompute. `turns` is a fresh array on
  every stream token, and the effect-triggered recompute ran synchronously
  (only the scroll handler was throttled), forcing a querySelector +
  getBoundingClientRect per turn per token on a long scrolled-back rail.
  Schedule the initial recompute through the same rAF gate so a burst of
  token-level changes coalesces to at most one layout read per frame.
- Prune tickRefs to the live turn id-set on every `turns` change. setTickRef
  never deletes on unmount (to avoid churn), so a session switch — where every
  itemId changes — would otherwise leak references to detached buttons for the
  component's lifetime.
- Clear the hover preview on tick blur so tabbing away doesn't strand it, with
  a guard so a stale blur can't wipe a preview a newer focus just opened.

Adds vitest coverage for the focus-shows / blur-clears preview behavior and
the stale-blur guard.

Co-authored-by: Isaac
yours-aditya pushed a commit to yours-aditya/omnigent that referenced this pull request Jul 16, 2026
* feat(web): add conversation turn-rail minimap with fixes

A left-edge vertical minimap: one tick per user turn, with a hover
preview and click-to-scroll. The rail tracks your position like a
scrollbar thumb and eagerly pages older history so it shows a useful
run of ticks on load.

Fixes found while building it:
- History pages now load in chronological order. The eager loader used
  to prepend fetched blocks one-by-one, reversing each page and
  scrambling the transcript (a mid-conversation prompt could surface at
  the top with a hard scroll stop above it).
- Rail tracking scrolls the active run into view instead of always
  re-centering, so clicking a tick you scrolled to leaves the rail
  parked while the transcript navigates.
- Tracking re-runs when the tick count changes, so a fresh load lands
  at the bottom with the last turn active.
- Rail fades in once the eager back-fill settles (no 2→N tick flash).
- Wider hover preview; full-pitch clickable tick band (hover == click
  hit area).

Responsive: desktop shows the rail and drops the floating up/down nav
buttons; mobile hides the rail and keeps the buttons (no hover on
touch). Keyboard nav is unchanged.

Tests: chronological-order regression + eager-load coverage in
chatStore, TurnRail render/interaction contract, and nav className
forwarding.

Co-authored-by: Isaac

* fix(web): address turn-rail PR review comments

Addresses the Polly review's blocking bug and non-blocking notes plus the
CodeQL warning on PR omnigent-ai#2285:

- Blocking: loadHistoryUntilUserMessages now clears hasMoreHistory on fetch
  failure (matching loadMoreHistory), so the rail's auto-firing eager-load
  effect can't re-arm into an unbounded retry loop that also left the rail
  permanently hidden.
- Over-fetch overshoot: count users already in state toward the target so we
  only top up to minUserMessages instead of overshooting by the existing count.
- Blank preview: the preview scan now stops only at a real (non-system) user
  turn, so a system-marker bubble before the reply no longer strands a turn
  with an empty preview.
- CodeQL useless assignment: drop the always-overwritten `next` initializer.
- FADE magic-number coupling: drive the CSS fade mask from --turn-rail-fade so
  the mask width and thumb-tracking math share one constant.
- previewTop drift: reposition the hover preview when the rail auto-scrolls
  under a stationary pointer.

Co-authored-by: Isaac

* fix(web): stop turn-rail snapping back while user scrolls it

Scrolling the rail up near its top triggers loadMoreHistory, which grows
`turns` and re-runs the thumb-tracking effect. That effect would smooth-scroll
the rail back to the transcript's visible run, yanking the user away from the
older ticks they were browsing. Track pointer-over-rail state and skip the
auto-scroll while the user is interacting, so a history fetch can't fight the
scroll.

Co-authored-by: Isaac

* test(e2e-ui): regenerate visual baselines

* fix(web): freeze turn-rail preview while scrolling the rail

Scrolling the rail drags ticks under a stationary cursor, firing onMouseEnter
on each and flickering the preview through every turn. Suppress hover updates
while the rail is mid-scroll and settle onto the tick under the cursor once
scrolling comes to rest, so the preview only changes when the user stops.

Co-authored-by: Isaac

* fix(web): freeze turn-rail preview while scrolling the rail

Scrolling the rail drags ticks under a stationary cursor, firing onMouseEnter
on each and flickering the preview through every turn. A real hover moves the
cursor; a scroll-induced enter does not — so ignore enter events whose cursor
position matches the last accepted hover, and settle onto the tick under the
cursor once scrolling comes to rest. The preview now only changes when the
user actually moves the pointer.

Adds tests for both the moved-cursor hover and the ignored same-position enter.

Co-authored-by: Isaac

* test(e2e-ui): regenerate visual baselines

* fix(web): count real turns for turn-rail, gate mount on viewport

Addresses the second Polly review on the turn-rail PR:

- B1: the rail derives ticks from non-system user turns, but the eager history
  loader counted every user-role block — including [System: …] markers. In
  agent/sub-agent sessions the loader could hit its target on marker blocks and
  early-return while the rail had too few ticks, leaving hasMoreHistory set and
  the rail stuck at opacity-0 forever. Share one isSystemUserContent predicate
  (new in systemMessage.ts) between ChatPage's turn derivation and the loader's
  count so both agree on what a real turn is.
- B2: TurnRail was only CSS-hidden on mobile, so its eager backfill (up to 2000
  items/open) still ran on the smallest-bandwidth clients for a rail they can't
  see. Gate the mount on useIsMobileViewport so mobile skips it entirely.
- Gate the inner rail's pointer-events on `revealed` so the invisible rail is
  not a silent click target before it fades in.
- Skip the scroll-settle re-hover once the pointer has left the rail; start
  pointerRef off-screen so a pre-move settle resolves to no element.
- Use a stable tick ref callback to avoid per-render Map churn.

Tests: isSystemUserContent unit tests; a chatStore regression proving markers
don't count toward the target; a genuine multi-page (>200 item) cross-page
assembly/order test; and TurnRail pointer-events reveal-gating tests.

Co-authored-by: Isaac

---------

Co-authored-by: omnigent-ci[bot] <294685417+omnigent-ci[bot]@users.noreply.github.com>
Signed-off-by: Aditya Devarapalli <adityareddyd2@gmail.com>
yours-aditya pushed a commit to yours-aditya/omnigent that referenced this pull request Jul 16, 2026
…2476)

Non-blocking follow-ups from the omnigent-ai#2285 review, all scoped to TurnRail.tsx:

- rAF-throttle the visible-tracking recompute. `turns` is a fresh array on
  every stream token, and the effect-triggered recompute ran synchronously
  (only the scroll handler was throttled), forcing a querySelector +
  getBoundingClientRect per turn per token on a long scrolled-back rail.
  Schedule the initial recompute through the same rAF gate so a burst of
  token-level changes coalesces to at most one layout read per frame.
- Prune tickRefs to the live turn id-set on every `turns` change. setTickRef
  never deletes on unmount (to avoid churn), so a session switch — where every
  itemId changes — would otherwise leak references to detached buttons for the
  component's lifetime.
- Clear the hover preview on tick blur so tabbing away doesn't strand it, with
  a guard so a stale blur can't wipe a preview a newer focus just opened.

Adds vitest coverage for the focus-shows / blur-clears preview behavior and
the stale-blur guard.

Co-authored-by: Isaac
Signed-off-by: Aditya Devarapalli <adityareddyd2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-doc-update Merged PR does not need a docs update size/XL Pull request size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant