fix(web): follow-tail + jump-to-bottom in console run log view#1946
fix(web): follow-tail + jump-to-bottom in console run log view#1946leex279 wants to merge 4 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
RunDetailPage's log view fired auto-scroll only on array-length changes, so actively streaming nodes did not follow the tail (streaming tokens grow message height without changing array length). There was also no scroll-detach detection and no jump-to-bottom affordance. Changes: - Add NEAR_BOTTOM_PX = 120 module constant - Replace array-length useEffect with ResizeObserver on contentRef so intra-message streaming height growth re-pins to bottom - Add atBottom state + handleScroll detach + scrollToBottom - Wrap log scroll container in relative div, add jump-to-bottom pill button mirroring ChatPage exactly Fixes #1945
Windows requires Developer Mode or elevated permissions to create symlinks. Skip the affected tests on win32 rather than fail the suite. - packages/providers/src/claude/binary-resolver.test.ts - packages/workflows/src/load-command-prompt.test.ts
38578d4 to
d4d2558
Compare
🔍 Comprehensive PR ReviewPR: #1946 SummaryThe approach is right — ResizeObserver is the correct tool for follow-tail during intra-message streaming growth, and the jump-to-bottom pill is token-compliant and consistent with ChatPage. However, 4 of 5 agents independently converged on the same critical defect: the ResizeObserver effect uses Verdict:
🔴 Critical Issues (Auto-fixing)ResizeObserver never attaches in the primary flow — follow-tail silently dead📍 The effect runs once (
Also a regression: the removed View fix (callback ref — recommended by 3 of 4 agents)// ResizeObserver: scroll to bottom on any content height change (new items OR
// intra-message streaming growth) when pinned to the tail. Attached via callback
// ref so it survives the loading early-return and log/graph view toggles.
const roRef = useRef<ResizeObserver | null>(null);
const contentRef = useCallback((node: HTMLDivElement | null): void => {
roRef.current?.disconnect();
roRef.current = null;
if (node === null) return;
const ro = new ResizeObserver(() => {
if (!lastBottomRef.current) return;
const el = scrollRef.current;
if (el !== null) el.scrollTop = el.scrollHeight;
});
ro.observe(node);
roRef.current = ro;
}, []);
// JSX unchanged: <div ref={contentRef} className="w-full px-6">Couples observer lifetime to DOM-node lifetime instead of re-deriving render conditions (the page has 3 early returns + a 3-way view branch — a deps list would be a maintenance trap). React invokes callback refs with Alternative considered: key the effect on 🟠 High IssuesManual validation plan was never executed — feature is unverified📍 process gap — plan lives in the workflow run's
Required before un-drafting — run against
🟡 Medium Issues (Needs Decision)1. Windows
|
| Issue | Location | Suggestion |
|---|---|---|
Stale atBottom after log→graph→log round-trip (pill visibility lies) |
RunDetailPage.tsx:242-249, 409-411 |
Resolved as a side effect of the CRITICAL callback-ref fix — no separate change |
| Header comment documents detach but not re-attach | RunDetailPage.tsx:217-218 |
Extend: "…detaches on scroll-up and re-attaches when scrolled back within NEAR_BOTTOM_PX of the end." |
| dx-quirks.md doesn't document the symlink-skip convention | packages/docs-web/.../dx-quirks.md |
Optional follow-up only — do NOT add to this PR (scope); moot if the capability probe lands |
✅ What's Good
- Right tool, wrong lifecycle: ResizeObserver correctly catches intra-message streaming growth the old length-keyed effect missed — only the attach wiring is broken, not the approach.
- Jump-to-bottom pill is markup-identical to ChatPage (classes,
aria-label,aria-hiddenarrow);NEAR_BOTTOM_PX = 120matches ChatPage's constant — threshold drift is now greppable. - Brand-token compliant (
border-border,bg-surface-elevated,text-text-secondary,text-text-primary) — verified againstindex.css; no ad-hoc values. - Clean hygiene:
ro.disconnect()cleanup, null-guards in all three DOM accessors, no RO feedback-loop risk, strict TS with explicit(): void, zero lint disables. - Test skips are surgical: bodies, assertions, and
try/finallycleanup preserved exactly; accurate skip-reason comments; nomock.module()risk. - Discipline honored: no speculative
useFollowTailextraction (Rule of Three), stable hook order above early returns, honestimplementation.mddisclosure, correctly drafted.
CLAUDE.md compliance: 8/9 PASS (scope discipline FAIL — Medium #1).
📋 Suggested Follow-up Issues
- "Replace blanket win32 symlink test skips with a capability probe" (P3 — if "keep as-is" chosen on Medium Model stucked at response stream text #1)
- "dx-quirks: document platform-conditional test skip convention" (P3 — optional, moot if probe lands)
Next Steps
- ⚡ Auto-fix step will address the CRITICAL observer fix + comment corrections
- 🧪 Execute the 6-point manual validation checklist (requires a browser — cannot be auto-fixed)
- 📝 Decide Medium Model stucked at response stream text #1 (test-skip scope) — split recommended
- 🎯 Un-draft once validation results are recorded
Reviewed by Archon comprehensive-pr-review workflow
Artifacts: artifacts/runs/972f3b5b354aeff787c09ea740fa87d4/review/
Fixes applied from review (CRITICAL/HIGH): - ResizeObserver was created in a mount-only ([] deps) effect, but the content div is conditionally rendered behind async data loading and the log/graph view toggle — on cold loads and after view round-trips the observer never attached and follow-tail silently did nothing. Attach it via a callback ref so observer lifetime tracks DOM-node lifetime. - Corrected "before every render" comment (useEffect runs after commit) and documented the snapshot/observer ordering invariant. - Documented re-attach half of the handleScroll contract. Review artifacts: artifacts/runs/972f3b5b354aeff787c09ea740fa87d4/review/ Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
⚡ Auto-Fix ReportStatus: PARTIAL — all auto-fixable items fixed; manual validation still required Fixes Applied
What Was Fixed
Tests AddedNone — ❌ Not Fixed (Manual Action Required)
🟡 MEDIUM Issues (Your Decision)
📋 Suggested Follow-up Issues
Validation✅ Type check (all packages) | ✅ Lint ( Auto-fixed by Archon comprehensive-pr-review workflow |
The follow-tail ResizeObserver only snapped to the tail when lastBottomRef was already true, but a post-commit effect recomputed that ref from the live scroll position on every render. On a cold load of a run whose log is already taller than the viewport (a completed run, or an in-progress run opened late), the first commit measured the user as "not at bottom" before the observer could fire, so the view stayed pinned to the TOP and never followed streaming output. Async content arriving in waves (events, then messages) made it worse: a one-shot snap landed near the tail, then the next wave detached again. Drive the follow intent purely from user action (scroll / jump-to-bottom pill) instead of a post-commit position snapshot, and re-pin to the tail on every content-div (re)mount via the callback ref. The log now opens at its newest output on cold load and graph->log round-trips, follows growth while pinned, and still detaches cleanly on scroll-up. Verified in-browser against a 9.3k-px completed run: cold load lands at bottom (no pill), scroll-up reveals the pill, pill click and follow-on-growth re-pin, detached growth does not yank back.
|
Thanks for the ResizeObserver fixes — the callback-ref approach looks right and CI is green. One remaining ask before this merges: please drop the two Windows If non-elevated local Windows dev is the pain point, a capability probe ( Once the skips are reverted and the 5-step manual browser verification from the review is recorded, this should be good to un-draft. |
Summary
RunDetailPagelog view auto-scrolled only on array-length changes (messages?.length,detail?.events.length). Token streaming grows message height without changing array length, so actively-streaming nodes never followed the tail. No scroll-detach detection and no jump-to-bottom affordance existed.useEffectauto-scroll hooks with aResizeObserver-based follow-tail (catches both new items and intra-message height growth), addedatBottomstate +handleScrolldetach detection, and added a jump-to-bottom pill button — all mirroring the pattern already shipped inChatPage(PR feat(web/console): chat workflow-completion card + SSE-lock + jump-to-bottom #1885).ChatPage), noRunStream.tsxchanges, no changes outside theview === 'log'branch.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
contentRefcontent divatBottomstatehandleScrollscrollToBottomuseEffectLabel Snapshot
risk: lowsize: Swebweb:experiments/consoleChange Metadata
bugwebLinked Issue
Validation Evidence (required)
All checks passed:
check:bundled/check:bundled-skill/check:bundled-schemabun run type-checkbun run lint --max-warnings 0bun run format:checkbun run test(per-package)Pre-commit
lint-staged(eslint --fix+prettier --write) ran on the staged file without behavioral change.bun run validateexit 0 across all 10 packages.Security Impact (required)
Compatibility / Migration
Human Verification (required)
Reviewer should open the dev server (
bun run dev) and verify:atBottomtotrue.Side Effects / Blast Radius (required)
@archon/webconsole experiment —RunDetailPagelog view only.view === 'log'branch).ResizeObservercleanup function disconnects on unmount — no leaks.sticky top-0toolbar continues to work correctly; sticky anchor is the newoverflow-y-autoinner div (correct CSS stacking context).Rollback Plan (required)
git revert a0acc184(reverts theRunDetailPage.tsxchange)Risks and Mitigations
ResizeObserverfires beforescrollRef.currentis setif (el !== null)guard already in the callback; LOW likelihood.scrollTopassignment is idempotent; LOW likelihood.sticky top-0toolbar breaks when scroll root changesoverflowancestor — the new inneroverflow-y-autodiv is the correct ancestor; LOW likelihood.