diff --git a/src/web-ui/src/flow_chat/components/modern/AGENTS.md b/src/web-ui/src/flow_chat/components/modern/AGENTS.md index 7bce35f0cc..91a508c3c3 100644 --- a/src/web-ui/src/flow_chat/components/modern/AGENTS.md +++ b/src/web-ui/src/flow_chat/components/modern/AGENTS.md @@ -50,6 +50,14 @@ before reporting a defect as new. and lands outright past it. Measured in viewports, never in pixels: the question is whether the reader can follow the movement, and what they can follow is a share of what they can see. +- Scrolling up loses the follow only once the reader is past the end of real + content. Until then they are in the reserved blank, having missed nothing, and + output growing to fill it hands the viewport back — unless their own gesture + claim is still live, in which case the crossing is deferred, not spent. Judged + by which side moved further between two samples, never by geometry alone. +- That watch runs for as long as the reader holds the viewport, not for one + crossing. A reader may climb out of the blank and scroll back down into it any + number of times, and each time is another chance for output to reach them. - Footer height represents only the current input-stack layout and real footer content such as history state and `RuntimeStatusSlot`. The tail spacer is a separate sibling and must not be folded into it. diff --git a/src/web-ui/src/flow_chat/components/modern/FLOWCHAT_SCROLL_STABILITY.md b/src/web-ui/src/flow_chat/components/modern/FLOWCHAT_SCROLL_STABILITY.md index dfd50f9988..904ac3368e 100644 --- a/src/web-ui/src/flow_chat/components/modern/FLOWCHAT_SCROLL_STABILITY.md +++ b/src/web-ui/src/flow_chat/components/modern/FLOWCHAT_SCROLL_STABILITY.md @@ -190,6 +190,89 @@ picks for the same viewport state — having the two disagree would be worse tha either choice. The exemption therefore outlives the Turn: a short Turn stays pinned until a newer one replaces it. +## Output Catching Up With a Reader in the Blank + +The snap back covers the reader who came to rest *below* the follow target. +This covers the other edge of the same region: the reader who scrolled up out of +the tail but is still looking at reserved blank, and whom output then overtakes. + +Scrolling up gives the follow away permanently, and that is right only for a +reader who left the live region. A small scroll up may not have. The blank is up +to `tailHoldMaxGapPx` under `hold-tail` and the whole gap under a pinned Turn, +so the reader can be a few hundred pixels off the tail with nothing hidden from +them at all — until output grows past the bottom edge, and they silently stop +seeing it with no affordance saying so. + +So a watch runs for as long as the reader holds the viewport — from the scroll +that took it to whatever hands it back. `scrollTop > contentEnd` is the +predicate for "the blank is on screen", deliberately *not* the snap back's, +which is relative to the follow target and therefore reports a reader above a +pin as having nothing to snap back from. The watch keeps one bit between +samples: whether the blank was on screen at the previous one. A crossing is that +bit going from set to clear, and which side moved decides who keeps the viewport +(`resolveTailDepartureCrossing`): + +- **`content-caught-up`** — the content end rose further than the reader moved. + Output filled the space under someone who was standing still, so follow takes + the viewport back. +- **`reader-left-blank`** — the reader moved further than the content end rose. + They went to read history, and are left alone. The latch clears, so nothing + asks again until they put the blank back on screen themselves. + +A crossing does **not** end the watch. Scoping it to one was tried and is wrong: +a reader who climbs out of the blank, reads for a while and scrolls back down to +sit in it again is in exactly the position the rule exists for, and had already +spent the single crossing they were given. The watch also opens on any exit that +finds none open, not only the one that took the follow — every wheel notch +exits, and only the first of them finds anything to give up. + +Streaming does not stop because the reader scrolled, so both sides usually move +between samples and the tie-break above is doing real work. Two guards keep it +honest: + +**A live gesture vetoes the resume.** Measured over one session's twenty +departures: nine left blank on screen, and two of those ended `content-caught-up` +— one a reader who had been still for 1.6s and was overtaken, one 320ms into a +live gesture with the reader climbing 200px while content grew 237. The +tie-break called the second for the content, correctly, and acting on it would +have taken the viewport back mid-scroll. `user-gesture` on the register +separates them, because a wheel claim lapses `USER_DRIVEN_SCROLL_WINDOW_MS` +after the last notch and notches arrive faster than that. A veto *defers* the +crossing rather than settling it — the latch stays set, so the next sample +judges the same transition again: a reader who carries on climbing out-moves the +content and is let go by a verdict that never needed the veto, and one who has +stopped is followed as soon as the claim lapses. + +**A travelling snap back is not the reader.** It crosses the same line from the +wrong side — downwards through the blank — so samples taken while one is in +flight update the offsets and take no verdict. + +**Geometry read from two moments is not geometry.** `scrollTop` is clamped to +`scrollHeight - clientHeight`, so no settled viewport is more than the spacer +past the content end (`isTailBlankMeasurable`). A larger reading means the +transcript is mid-restructure: a history prepend shifts the viewport by the +height it inserted before that height is in the scroll range, and the blank +between the two reads as thousands of pixels. Measured on a session that paged +its whole history on the first scroll up — 14252px of shift against a content +end still reading 989, a blank of 6239px. Read at face value that is a reader +deep in the blank, which is the one state this acts on, so the next sample would +have pulled them out of the history they had just asked for. Such a sample takes +no verdict and does not touch the latch: a restructure says nothing about where +the reader was standing before it. + +Resuming does not scroll. The blank closing *is* the two offsets meeting, so +what remains is one sample of growth, and the follow loop's ease covers it; a +one-shot scroll here would be a snap the reader can see for a correction they +cannot. It also retires the pin, even when the departure happened under one: the +pin's reservation is the blank the reader just scrolled out of, and restoring it +would pull them back down to the offset they left. + +The whole watch is a ref. `followOutput.tailWatch` and +`followOutput.tailWatchEnded` bracket it — the second carrying `crossings`, so a +watch that never resolved anything can be told from one that resolved against +the reader — and `followOutput.tailCrossing` records each transition with its +verdict, both raw deltas, and whether a gesture was live. + ## The Follow Eases Its Write, Never Its Target The follow target moves when the transcript reflows, and Markdown reflows a @@ -456,6 +539,49 @@ content such as history state and `RuntimeStatusSlot`. reserved blank at all, and take the empty-range gap below with it. - A collapse larger than `tailHoldMaxGapPx` still moves the viewport, by the excess only. +- **A reader who scrolls out of the blank and stops at the content end keeps no + follow.** Measured, and more common than the case above covers: four of nine + watched departures moved almost exactly the blank's height and stopped — + `readerMovedPx` −665.3 against a 664px blank, −294.7 against 294.7, −296 + against 296 — then sat there for up to 2.9s. That is not reading history, it + is getting rid of the blank. But it ends the departure as `reader-left-blank`, + and the snap back cannot help either, because under a pin the reader is + *above* the follow target. Output then accumulates below the bottom edge + unseen. Deliberately not covered yet: acting on it means reading intent from a + resting position that has content in it, which is the thing the snap back's + licence explicitly does not extend to. +- **Paging a long history while still scrolling throws the reading position a + long way.** Reproduced four times: a session opened on its three-Turn tail + pages the rest on the first scroll up, the prepend is compensated, and the + reader keeps wheeling through the settle that follows. Their Turn is out of + the rendered window for the whole of it, so `captureAnchorForScroll` cannot + re-capture and carries instead — adding each scroll to the Turn's expected + offset with no DOM to check the result against. Measured: three attempts, + 5532.7px, an anchor agreed at 112.6px from the viewport top becoming one + agreed at 3356px in a 985px viewport, and a −1349px correction computed + against that. Worst case it clamps at `scrollTop: 0` with the session's first + Turn at the top. `ANCHOR_MISSING_TURN_ATTEMPTS` does not bound it — that is + frames, not distance. + + **Three fixes were tried and reverted; do not re-derive them.** *Refuse a + correction whose offset left the viewport* — an anchored offset legitimately + leaves it, both when the reader scrolls past their own Turn and when a + registered writer's travel is taken out, and each has a test asserting the + correction that follows. *Subtract the browser's clamp,* + `previousScrollTop - max(0, scrollHeight - clientHeight)` — unsound, because + the previous offset was valid at the previous range, so any shrink at all + makes it exceed the new maximum whether or not the viewport was ever clamped; + measured, it took 2790.7px off a descent the reader had made themselves. + *Track the smallest range since the last credit* — the floor came back equal + to the current range, so no dip was ever observed. `overflow-anchor` is a dead + end too: already `none` on every scrolling box here. + + What the four logs do establish: the reader is wheeling continuously + throughout (the `user-gesture` claims are coalesced, so read `suppressedCount` + before concluding they stopped), and the descent is largely theirs. The next + attempt should probably bound the *result* — an offset several screens outside + the viewport is wrong whatever put it there — rather than try to attribute the + movement, which is what all three failures had in common. - An animated scroll aims at the target it was issued for. Jumping to latest while output is arriving therefore ends with one catch-up step covering whatever content grew during the animation — under the ease's snap threshold diff --git a/src/web-ui/src/flow_chat/components/modern/FLOWCHAT_VERIFICATION.md b/src/web-ui/src/flow_chat/components/modern/FLOWCHAT_VERIFICATION.md index f16dd0adc3..90bb74b68a 100644 --- a/src/web-ui/src/flow_chat/components/modern/FLOWCHAT_VERIFICATION.md +++ b/src/web-ui/src/flow_chat/components/modern/FLOWCHAT_VERIFICATION.md @@ -126,6 +126,25 @@ group does not renumber the others. be gone — this is the one path where the viewport arrives at the tail without a scroll event to notice it. +### Output catching up with a reader in the blank + +1. Send a message so a short Turn pins with blank below it, then wheel up a + little — far enough to leave the tail, not far enough to push the blank off + screen — and take your hand off. As the answer grows past the bottom edge the + transcript must resume following, easing rather than snapping, and the + jump-to-latest affordance must disappear with it. +2. The same scroll, but keep wheeling. The transcript must not take the viewport + back mid-gesture, and once you are above the end of content it must leave you + there however much more arrives — until you go back down yourself. +3. Scroll up past the end of content, stop, and let output arrive. Nothing may + move. This is the case the rule must never claim. +4. From there — above the end of content, follow long gone — scroll back *down* + until the blank shows again and stop. Follow must resume as output reaches + the bottom edge. Repeat it a third time. Nothing about this is once-only. +5. With no blank on screen at all — a transcript that fills the viewport, mid + answer — scroll up a little. Follow must not resume until you bring the blank + back on screen; nothing was crossed. + ### The scrollbar 1. Drag the scrollbar to the very bottom. The screen must not be entirely diff --git a/src/web-ui/src/flow_chat/components/modern/flowChatTailFollow.test.ts b/src/web-ui/src/flow_chat/components/modern/flowChatTailFollow.test.ts index dfb5c916f3..049a4ea6cc 100644 --- a/src/web-ui/src/flow_chat/components/modern/flowChatTailFollow.test.ts +++ b/src/web-ui/src/flow_chat/components/modern/flowChatTailFollow.test.ts @@ -3,10 +3,13 @@ import { contentEndScrollTop, FLOWCHAT_ANIMATED_JUMP_MAX_VIEWPORTS, FLOWCHAT_AT_CONTENT_END_THRESHOLD_PX, + isTailBlankMeasurable, isViewportAtTail, memorylessFollowState, nextTailFollowState, resolveAnimatedJumpBehavior, + resolveTailDepartureCrossing, + shouldResumeFollowAfterDeparture, tailHoldMaxGapPx, tailSnapBackScrollTop, tailSpacerPxForViewport, @@ -411,3 +414,130 @@ describe('resolveAnimatedJumpBehavior', () => { })).toBe('auto'); }); }); + +describe('resolveTailDepartureCrossing', () => { + it('keeps watching while the blank is still on screen', () => { + expect(resolveTailDepartureCrossing({ + blankPx: 180, + contentDeltaPx: 40, + scrollDeltaPx: 0, + })).toBe('watching'); + }); + + it('reads content rising to meet a stationary reader as the tail catching up', () => { + // The case the resume acts on: the reader has not moved, and the empty + // space they were left looking at has just been filled. + expect(resolveTailDepartureCrossing({ + blankPx: -12, + contentDeltaPx: 190, + scrollDeltaPx: 0, + })).toBe('content-caught-up'); + }); + + it('reads a reader climbing out past a still content end as reading history', () => { + expect(resolveTailDepartureCrossing({ + blankPx: -240, + contentDeltaPx: 0, + scrollDeltaPx: -420, + })).toBe('reader-left-blank'); + }); + + it('gives a crossing where both moved to whichever moved further', () => { + // Streaming does not stop because the reader scrolled, so both sides move + // between samples and the tie-break decides. Callers record the two deltas + // beside the verdict so this line can be revisited from the trail. + expect(resolveTailDepartureCrossing({ + blankPx: -5, + contentDeltaPx: 120, + scrollDeltaPx: -40, + })).toBe('content-caught-up'); + expect(resolveTailDepartureCrossing({ + blankPx: -5, + contentDeltaPx: 40, + scrollDeltaPx: -120, + })).toBe('reader-left-blank'); + }); + + it('counts the blank as gone the moment content reaches the bottom edge', () => { + // `blankPx` is `scrollTop - contentEnd`, so zero is the content end exactly + // on the viewport's bottom edge — no blank, and the departure is over. + expect(resolveTailDepartureCrossing({ + blankPx: 0, + contentDeltaPx: 30, + scrollDeltaPx: 0, + })).toBe('content-caught-up'); + expect(resolveTailDepartureCrossing({ + blankPx: 0.5, + contentDeltaPx: 30, + scrollDeltaPx: 0, + })).toBe('watching'); + }); + + it('does not read a shrinking transcript as the reader leaving', () => { + // A tool card collapsing moves the content end *down*, which cannot end a + // departure — and if it somehow coincides with a crossing, a negative + // content delta must not out-vote the reader. + expect(resolveTailDepartureCrossing({ + blankPx: -30, + contentDeltaPx: -200, + scrollDeltaPx: -60, + })).toBe('reader-left-blank'); + }); +}); + +describe('isTailBlankMeasurable', () => { + it('accepts a blank the reserved spacer can account for', () => { + expect(isTailBlankMeasurable({ blankPx: SPACER, tailSpacerPx: SPACER })).toBe(true); + expect(isTailBlankMeasurable({ blankPx: -400, tailSpacerPx: SPACER })).toBe(true); + }); + + it('refuses a blank larger than the whole spacer, which cannot have been seen', () => { + /* + * `scrollTop` is clamped to `scrollHeight - clientHeight`, so a settled + * viewport is never more than the spacer past the content end. Measured on + * a session that paged its whole history on the first scroll up: the + * prepend shifted the viewport 14252px to hold the reader's Turn still, + * against a content end still reading 989 — a blank of 6239px in a + * transcript reserving a few hundred. + */ + expect(isTailBlankMeasurable({ blankPx: 6239, tailSpacerPx: SPACER })).toBe(false); + }); +}); + +describe('shouldResumeFollowAfterDeparture', () => { + it('takes the viewport back when output caught up with a reader who had stopped', () => { + expect(shouldResumeFollowAfterDeparture({ + crossing: 'content-caught-up', + gestureLive: false, + })).toBe(true); + }); + + it('leaves a reader who is still scrolling alone, whatever the geometry says', () => { + /* + * Measured, and the reason the gesture is an input at all: 320ms into a + * live gesture the reader had climbed 200px while content grew 237, so the + * tie-break called the crossing for the content — correctly — and acting on + * it would have taken the viewport back mid-scroll. + */ + expect(shouldResumeFollowAfterDeparture({ + crossing: 'content-caught-up', + gestureLive: true, + })).toBe(false); + }); + + it('never resumes for a reader who climbed out past the content end', () => { + // Reading history is the case the whole departure exists to tell apart, and + // a lapsed gesture claim does not make it something else. + expect(shouldResumeFollowAfterDeparture({ + crossing: 'reader-left-blank', + gestureLive: false, + })).toBe(false); + }); + + it('decides nothing while the departure is still open', () => { + expect(shouldResumeFollowAfterDeparture({ + crossing: 'watching', + gestureLive: false, + })).toBe(false); + }); +}); diff --git a/src/web-ui/src/flow_chat/components/modern/flowChatTailFollow.ts b/src/web-ui/src/flow_chat/components/modern/flowChatTailFollow.ts index d2455022bd..ace416fc9b 100644 --- a/src/web-ui/src/flow_chat/components/modern/flowChatTailFollow.ts +++ b/src/web-ui/src/flow_chat/components/modern/flowChatTailFollow.ts @@ -228,6 +228,119 @@ export function tailSnapBackScrollTop(input: TailSnapBackInput): number | null { : null; } +export type TailDepartureCrossing = + /** The blank is still on screen; nothing has been crossed. */ + | 'watching' + /** Output grew until the blank was gone under a reader who stayed put. */ + | 'content-caught-up' + /** The reader scrolled up past the end of content. Reading history. */ + | 'reader-left-blank'; + +/** + * Where a reader stands relative to the reserved blank, and who moved them + * there. + * + * Losing the follow permanently is right only for a reader who left the live + * region, and `scrollTop > contentEnd` says they have not. The reserved blank + * is up to `tailHoldMaxGapPx` under `hold-tail` and the whole gap under a + * pinned Turn, so a small scroll up can leave the reader looking at empty space + * below the newest output — nothing hidden from them yet — until output grows + * past the bottom edge and they silently stop seeing it. + * + * Note this is deliberately *not* the predicate `tailSnapBackScrollTop` uses. + * That one is relative to the follow target, and under a pin the target sits + * inside the blank, so a reader above it is reported as having nothing to snap + * back from. These are the two edges of the same region. + * + * `watching` is a *position*, not a pending outcome: the blank is on screen + * right now. It says nothing about whether the reader has ever left it, and the + * caller is the one that remembers — the transition from `watching` to either + * verdict is the event, and a reader who has been above the content end for + * minutes reports `reader-left-blank` on every sample without anything having + * happened. + * + * The two ways out are told apart by which side moved. Content rising to meet a + * stationary reader is the case `shouldResumeFollowAfterDeparture` acts on; a + * reader climbing out past a stationary content end is the case it must leave + * alone. When both moved, the larger one is the cause — and callers record the + * raw deltas beside the verdict, so the tie-break can be revisited from the + * trail. + */ +export function resolveTailDepartureCrossing(input: { + /** `scrollTop - contentEndScrollTop` now. Positive while blank is on screen. */ + blankPx: number; + /** How far the content end rose since the previous sample. */ + contentDeltaPx: number; + /** How far the reader moved the viewport since the previous sample. */ + scrollDeltaPx: number; +}): TailDepartureCrossing { + if (input.blankPx > 0) { + return 'watching'; + } + return input.contentDeltaPx >= Math.abs(input.scrollDeltaPx) + ? 'content-caught-up' + : 'reader-left-blank'; +} + +/** + * Whether a departure that has just ended hands the viewport back to follow. + * + * `content-caught-up` is the case the whole rule exists for, but it is not + * sufficient on its own. Streaming does not stop because the reader scrolled, + * so a reader still working the wheel can cross the same line by scrolling into + * output that is rising to meet them — and resuming there takes the viewport + * out of their hands while their hand is still on it. + * + * Measured over one session's twenty departures: nine left blank on screen, and + * exactly two of those ended `content-caught-up`. One was a reader who had + * stopped 1.6s earlier and was overtaken by output, which is the case to act + * on. The other was 320ms into a live gesture, with the reader climbing 200px + * while content grew 237 — the tie-break called it for the content, correctly, + * and acting on it would have yanked the viewport back mid-scroll. The register + * separates the two cleanly, because a wheel claim lapses + * `USER_DRIVEN_SCROLL_WINDOW_MS` after the last notch and notches arrive faster + * than that. + * + * A refusal here defers the crossing rather than settling it. The caller keeps + * its "blank was visible" latch, so the same transition is judged again on the + * next sample: a reader who carries on climbing out-moves the content and is + * let go by a verdict that never needed the veto, and one who has stopped is + * followed as soon as the claim lapses. Nothing has to be given up to stay off + * a live gesture — only postponed by one sample. + */ +export function shouldResumeFollowAfterDeparture(input: { + crossing: TailDepartureCrossing; + /** Whether the reader's own claim on the viewport is still live. */ + gestureLive: boolean; +}): boolean { + return input.crossing === 'content-caught-up' && !input.gestureLive; +} + +/** + * Whether the blank a sample just measured can be real. + * + * `scrollTop` is clamped to `scrollHeight - clientHeight`, and the spacer is + * everything below the end of real content, so no settled viewport can be more + * than `tailSpacerPx` past it. A larger reading means the two halves were read + * from different moments — the transcript has been restructured and the browser + * has not clamped `scrollTop` to the new range yet, or something wrote the + * offset to compensate for content that has not been measured. + * + * Measured on a session that paged its whole history on the first scroll up: + * the prepend shifted the viewport 14252px to hold the reader's Turn still, + * against a content end that was still 989 — a blank of 6239px in a transcript + * whose spacer reserves a few hundred. Read as a reader sitting deep in the + * blank, which is the one state the follow rule is watching for, so the next + * sample would have handed the viewport to follow and pulled them out of the + * history they had just asked for. + */ +export function isTailBlankMeasurable(input: { + blankPx: number; + tailSpacerPx: number; +}): boolean { + return input.blankPx <= input.tailSpacerPx; +} + export interface ViewportAtTailInput { scrollTop: number; contentEndScrollTop: number; diff --git a/src/web-ui/src/flow_chat/components/modern/useFlowChatFollowOutput.test.tsx b/src/web-ui/src/flow_chat/components/modern/useFlowChatFollowOutput.test.tsx index a3fac54376..c55bbceaee 100644 --- a/src/web-ui/src/flow_chat/components/modern/useFlowChatFollowOutput.test.tsx +++ b/src/web-ui/src/flow_chat/components/modern/useFlowChatFollowOutput.test.tsx @@ -1544,4 +1544,237 @@ describe('useFlowChatFollowOutput', () => { expect(controller?.isFollowingOutputNow()).toBe(true); }); }); + + /* + * Scrolling up gives the follow away, and the reserved blank is what makes + * that too blunt on its own: a reader can leave the tail by a hundred pixels + * and still be looking at empty space below the newest output, having missed + * nothing. The departure watches those readers until the blank is gone, and + * which side closed it decides who keeps the viewport. + */ + describe('resuming a follow that output caught up with', () => { + const CONTENT_END = 1_000; + let viewportOwner: FlowChatViewportOwnerApi | null = null; + + /** Move the end of real content, leaving the viewport where it is. */ + function setContentEnd(contentEndPx: number) { + Object.defineProperty(scroller, 'scrollHeight', { + configurable: true, + value: contentEndPx + VIEWPORT + TAIL_SPACER, + }); + } + + /** + * Mount a following transcript, then have the reader take the viewport from + * `blankPx` into the reserved blank — `0` being the end of real content, on + * the viewport's bottom edge. Returns the one-shot scroll, so a test can + * assert nothing reached for it. + */ + function departWithBlank(blankPx: number) { + const scrollToContentEnd = vi.fn(); + setScrollerMetrics(scroller, { + scrollHeight: CONTENT_END + VIEWPORT + TAIL_SPACER, + clientHeight: VIEWPORT, + scrollTop: CONTENT_END, + }); + act(() => { + root.render( + { controller = next; }} + onViewportOwner={next => { viewportOwner = next; }} + />, + ); + }); + expect(controller?.isFollowingOutput).toBe(true); + scrollToContentEnd.mockClear(); + frames.length = 0; + + scroller.scrollTop = CONTENT_END + blankPx; + act(() => controller?.handleUserScrollIntent()); + expect(controller?.isFollowingOutput).toBe(false); + return scrollToContentEnd; + } + + it('takes the viewport back when output fills the blank under a stationary reader', () => { + departWithBlank(100); + + // The resize observer's signal, which is the only one that arrives when + // the content moves and the reader does not. + setContentEnd(CONTENT_END + 150); + act(() => controller?.scheduleFollowToLatest()); + + expect(controller?.isFollowingOutput).toBe(true); + }); + + it('resumes where the reader is instead of scrolling them to the end', () => { + const scrollToContentEnd = departWithBlank(100); + setContentEnd(CONTENT_END + 150); + act(() => controller?.scheduleFollowToLatest()); + + // Nothing to travel that the follow loop is not already about to cover: + // the blank closing *is* the two offsets meeting. What is left is the one + // sample of growth that closed it, and the loop eases that away — a + // quarter of the 50px on the first frame it runs, which is the one the + // resume's own re-render asks for. + expect(scrollToContentEnd).not.toHaveBeenCalled(); + expect(scroller.scrollTop).toBe(CONTENT_END + 100 + 50 * TAIL_EASE_ALPHA); + }); + + it('leaves the viewport with a reader whose hand is still on it', () => { + /* + * Measured: 320ms into a live gesture, a reader had climbed 200px while + * content grew 237, so the crossing was attributed to the content — which + * is correct, and acting on it would still have been wrong. + */ + departWithBlank(400); + viewportOwner?.claim('user-gesture', { holdForMs: USER_DRIVEN_SCROLL_WINDOW_MS }); + + scroller.scrollTop = CONTENT_END + 200; + setContentEnd(CONTENT_END + 237); + act(() => controller?.handleScroll()); + + expect(controller?.isFollowingOutput).toBe(false); + }); + + it('leaves a reader who climbed out past the content end reading history', () => { + departWithBlank(100); + + scroller.scrollTop = CONTENT_END - 300; + act(() => controller?.handleScroll()); + // Output arriving afterwards must not fetch them back down. + setContentEnd(CONTENT_END + 150); + act(() => controller?.scheduleFollowToLatest()); + + expect(controller?.isFollowingOutput).toBe(false); + }); + + it('leaves a reader who was never in the blank where they are', () => { + // Nothing crossed: they were above the end of content when they took the + // viewport and they are still above it. Only a transition is a question. + departWithBlank(0); + + setContentEnd(CONTENT_END + 150); + act(() => controller?.scheduleFollowToLatest()); + + expect(controller?.isFollowingOutput).toBe(false); + }); + + it('follows a reader who left the blank and came back down into it', () => { + /* + * The case a per-departure watch got wrong, and the reason the watch runs + * for as long as the reader holds the viewport. Measured as a report: + * scroll up until the blank is gone, scroll back down until it shows + * again, wait — and nothing happened, because the one crossing the watch + * was given had been spent on the way up. + */ + departWithBlank(400); + + // Up and out of the blank entirely. + scroller.scrollTop = CONTENT_END - 200; + act(() => controller?.handleScroll()); + expect(controller?.isFollowingOutput).toBe(false); + + // Back down until the blank is on screen again, and stop. + scroller.scrollTop = CONTENT_END + 300; + act(() => controller?.handleScroll()); + expect(controller?.isFollowingOutput).toBe(false); + + setContentEnd(CONTENT_END + 300); + act(() => controller?.scheduleFollowToLatest()); + + expect(controller?.isFollowingOutput).toBe(true); + }); + + it('watches a reader who took the viewport from a follow that was already gone', () => { + // Every wheel notch exits, and only the first finds follow to give up. + // Gating the watch on that notch left the rest of the gesture unwatched. + departWithBlank(0); + act(() => controller?.handleUserScrollIntent()); + + scroller.scrollTop = CONTENT_END + 300; + act(() => controller?.handleScroll()); + setContentEnd(CONTENT_END + 300); + act(() => controller?.scheduleFollowToLatest()); + + expect(controller?.isFollowingOutput).toBe(true); + }); + + it('reads nothing into a viewport a history prepend has just shifted', () => { + /* + * Measured: a session opened on three Turns, one scroll up paged the whole + * seven, and the prepend shifted the viewport 14252px to hold the reader's + * Turn still — before those heights were in the scroll range, so the blank + * between the two read as 6239px against a spacer of a few hundred. Taken + * at face value that is a reader sitting deep in the blank, which is the + * one state this rule acts on, and the next sample would have pulled them + * out of the history they had just asked for. + */ + departWithBlank(0); + + // The compensation: the viewport moves, the content end has not caught up. + scroller.scrollTop = CONTENT_END + TAIL_SPACER + 5_000; + act(() => controller?.handleScroll()); + // ...and now it has. Nothing crossed, because nothing was ever measured. + setContentEnd(CONTENT_END + TAIL_SPACER + 5_000); + act(() => controller?.scheduleFollowToLatest()); + + expect(controller?.isFollowingOutput).toBe(false); + }); + + it('judges a vetoed crossing again rather than spending it', () => { + /* + * The veto is about the reader's hand being on the wheel, not about the + * crossing being wrong. Deferring costs one sample; settling it would cost + * the whole episode. + */ + departWithBlank(400); + viewportOwner?.claim('user-gesture', { holdForMs: USER_DRIVEN_SCROLL_WINDOW_MS }); + + setContentEnd(CONTENT_END + 400); + act(() => controller?.scheduleFollowToLatest()); + expect(controller?.isFollowingOutput).toBe(false); + + // The claim lapses, the reader has not moved, and output is still coming. + viewportOwner?.release('user-gesture'); + setContentEnd(CONTENT_END + 420); + act(() => controller?.scheduleFollowToLatest()); + + expect(controller?.isFollowingOutput).toBe(true); + }); + + it('lets a reader who kept climbing through a vetoed crossing go', () => { + departWithBlank(400); + viewportOwner?.claim('user-gesture', { holdForMs: USER_DRIVEN_SCROLL_WINDOW_MS }); + + setContentEnd(CONTENT_END + 400); + act(() => controller?.scheduleFollowToLatest()); + expect(controller?.isFollowingOutput).toBe(false); + + // Still climbing, and now out-moving the content: the deferred crossing + // resolves against them, and the latch is spent for good. + scroller.scrollTop = CONTENT_END + 100; + setContentEnd(CONTENT_END + 410); + act(() => controller?.handleScroll()); + viewportOwner?.release('user-gesture'); + setContentEnd(CONTENT_END + 430); + act(() => controller?.scheduleFollowToLatest()); + + expect(controller?.isFollowingOutput).toBe(false); + }); + + it('keeps watching while the blank is only partly filled', () => { + departWithBlank(400); + + setContentEnd(CONTENT_END + 150); + act(() => controller?.scheduleFollowToLatest()); + expect(controller?.isFollowingOutput).toBe(false); + + setContentEnd(CONTENT_END + 400); + act(() => controller?.scheduleFollowToLatest()); + expect(controller?.isFollowingOutput).toBe(true); + }); + }); }); diff --git a/src/web-ui/src/flow_chat/components/modern/useFlowChatFollowOutput.ts b/src/web-ui/src/flow_chat/components/modern/useFlowChatFollowOutput.ts index 0b9ba8a151..179b4052aa 100644 --- a/src/web-ui/src/flow_chat/components/modern/useFlowChatFollowOutput.ts +++ b/src/web-ui/src/flow_chat/components/modern/useFlowChatFollowOutput.ts @@ -2,6 +2,7 @@ import { useCallback, useEffect, useRef, useState, type RefObject } from 'react' import { isViewportDiagnosticsEnabled, roundViewportPx, + traceViewport, traceViewportRepeating, } from '@/infrastructure/diagnostics/flowChatViewportDiagnostics'; import { @@ -19,9 +20,12 @@ import { SNAP_BACK_HOLD_MS } from './flowChatViewportOwnership'; import { contentEndScrollTop, FLOWCHAT_AT_CONTENT_END_THRESHOLD_PX, + isTailBlankMeasurable, memorylessFollowState, nextTailFollowState, resolveAnimatedJumpBehavior, + resolveTailDepartureCrossing, + shouldResumeFollowAfterDeparture, tailHoldMaxGapPx, tailSnapBackScrollTop, type TailFollowState, @@ -32,6 +36,7 @@ export type FollowOutputEnterReason = | 'new-turn' | 'session-open' | 'streaming-resumed' + | 'tail-caught-up' | 'tail-snap-back' | 'turns-rolled-back'; export type FollowOutputExitReason = @@ -40,6 +45,21 @@ export type FollowOutputExitReason = | 'scroll-to-turn' | 'scroll-to-index'; +/** + * Why a watch over a reader who owns the viewport ended. + * + * All four are follow taking it back or the transcript going away. A *crossing* + * is deliberately not on this list: the reader can climb out of the reserved + * blank and scroll back down into it any number of times before either happens, + * and each of those is another chance for output to catch up with them. + */ +type TailWatchOutcome = + /** Something handed the viewport back — a new Turn, a jump, a snap, this. */ + | 'followed-again' + | 'navigated' + | 'session-changed' + | 'unmounted'; + interface UseFlowChatFollowOutputOptions { activeSessionId?: string; latestTurnId: string | null; @@ -400,6 +420,150 @@ export function useFlowChatFollowOutput({ resolveFollowState(scroller).target ), [resolveFollowState]); + /* + * --------------------------------------------------------------------------- + * A viewport in the reader's hands, watched for output catching up with them. + * + * Scrolling up gives the follow away, and that is right only if the reader + * actually left the live region. They may not have. The reserved blank is up + * to 60% of a viewport under `hold-tail` and the whole gap under a pinned + * Turn, so a small scroll up can leave the reader still looking at empty space + * below the newest output — nothing is being hidden from them yet — and then + * output grows past the bottom edge and they silently stop seeing it. + * + * "Still looking at the blank" is `scrollTop > contentEnd`, because + * `contentEnd` is by definition the offset that puts the end of real content + * on the viewport's bottom edge. Note this is *not* the predicate the snap + * back uses: that one is relative to the follow target, and under a pin the + * target sits inside the blank, so a reader above it is reported as having + * nothing to snap back from. These are the two edges of the same region, and + * between them they cover it: the snap back returns a reader who came to rest + * *below* the follow target, and this returns one whom output caught up with + * from above. + * + * The watch therefore runs for as long as the reader holds the viewport, not + * for one crossing. `blankWasVisible` is the whole state it keeps between + * samples, and the rule is the obvious one: the blank was on screen and now it + * is not. Scoping it to a single crossing was tried and is wrong — a reader + * who climbs out of the blank, reads for a while and scrolls back down to sit + * in it again is in exactly the position the rule exists for, and had already + * spent the one crossing it was given. + * + * `content-caught-up` is what hands the viewport back, subject to + * `shouldResumeFollowAfterDeparture`; `reader-left-blank` is a reader who + * really did go and read history, and only clears the latch. Both raw deltas + * and the gesture claim are traced at each crossing rather than only the + * verdict, so the tie-break can still be revisited from a trail. + * --------------------------------------------------------------------------- + */ + const tailWatchRef = useRef<{ + openedAtMs: number; + /** Blank on screen when the reader took the viewport. */ + exitBlankPx: number; + exitScrollTopPx: number; + exitContentEndPx: number; + exitPinned: boolean; + exitStreaming: boolean; + /** Previous sample, so a crossing can be attributed to whatever moved. */ + lastScrollTopPx: number; + lastContentEndPx: number; + /** Whether the reserved blank was on screen as of the previous sample. */ + blankWasVisible: boolean; + samples: number; + crossings: number; + } | null>(null); + + const closeTailWatch = useCallback(( + outcome: TailWatchOutcome, + extra?: Record, + ) => { + const watch = tailWatchRef.current; + if (!watch) return; + tailWatchRef.current = null; + const scroller = scrollerRef.current; + const scrollTopPx = scroller?.scrollTop ?? watch.lastScrollTopPx; + const contentEndPx = scroller + ? readContentEndScrollTop(scroller) + : watch.lastContentEndPx; + traceViewport({ + location: 'followOutput.tailWatchEnded', + message: 'follow-output has the viewport back', + data: () => ({ + outcome, + viewportId, + forMs: Math.round(performance.now() - watch.openedAtMs), + samples: watch.samples, + crossings: watch.crossings, + blankAtExitPx: roundViewportPx(watch.exitBlankPx), + blankNowPx: roundViewportPx(scrollTopPx - contentEndPx), + // The two movements, over the whole life of the watch. + contentGrewPx: roundViewportPx(contentEndPx - watch.exitContentEndPx), + readerMovedPx: roundViewportPx(scrollTopPx - watch.exitScrollTopPx), + pinnedAtExit: watch.exitPinned, + streamingAtExit: watch.exitStreaming, + streamingNow: isStreamingRef.current, + ...(extra ?? {}), + }), + }); + }, [readContentEndScrollTop, scrollerRef, viewportId]); + /* + * Held by identity for the unmount cleanup below. + * + * That cleanup must run when the transcript goes away and at no other time, + * and depending on the callback would instead run it whenever the callback is + * rebuilt — which is whenever `getTailSpacerPx` changes identity, and so + * potentially on every render of whoever owns this hook. A watch lives for as + * long as the reader keeps the viewport and spans many renders by + * construction, so an effect that tears down with the callback closes every + * one of them a frame after it opens. + */ + const closeTailWatchRef = useRef(closeTailWatch); + closeTailWatchRef.current = closeTailWatch; + + /** + * Start watching a viewport the reader has taken. + * + * Opened whether or not blank is on screen at the time. Where the reader was + * standing when they took it settles nothing — they can scroll down into the + * blank at any point afterwards, and the whole question is where they are when + * output next reaches the bottom edge. + */ + const openTailWatch = useCallback(() => { + const scroller = scrollerRef.current; + if (!scroller) return; + const contentEndPx = readContentEndScrollTop(scroller); + const blankPx = scroller.scrollTop - contentEndPx; + const pinned = pinTurnIdRef.current !== null; + traceViewport({ + location: 'followOutput.tailWatch', + message: 'the reader has the viewport, and is watched for output catching up', + data: () => ({ + viewportId, + blankPx: roundViewportPx(blankPx), + blankVisible: blankPx > 0, + scrollTopPx: roundViewportPx(scroller.scrollTop), + contentEndPx: roundViewportPx(contentEndPx), + clientHeightPx: scroller.clientHeight, + // A pin is the case where the gap between the two predicates is widest. + pinned, + isStreaming: isStreamingRef.current, + }), + }); + tailWatchRef.current = { + openedAtMs: performance.now(), + exitBlankPx: blankPx, + exitScrollTopPx: scroller.scrollTop, + exitContentEndPx: contentEndPx, + exitPinned: pinned, + exitStreaming: isStreamingRef.current, + lastScrollTopPx: scroller.scrollTop, + lastContentEndPx: contentEndPx, + blankWasVisible: blankPx > 0, + samples: 0, + crossings: 0, + }; + }, [readContentEndScrollTop, scrollerRef, viewportId]); + /** * Stand the frame loop down for an animated scroll this hook is about to * issue. Taken from the viewport as it is now, so the first frame of the @@ -788,6 +952,10 @@ export function useFlowChatFollowOutput({ isFollowingOutputRef.current = true; setIsFollowingOutput(true); + // Whatever this entry is, follow has the viewport, so there is nothing left + // to watch for. The crossing route comes through here too, having already + // traced why. + closeTailWatch('followed-again', { enterReason: reason }); settleFramesRef.current = SETTLE_FRAMES; traceViewportRepeating(`follow|enter|${reason}`, { location: 'followOutput.enter', @@ -886,25 +1054,42 @@ export function useFlowChatFollowOutput({ endSmoothScrollYield('superseded'); followStateRef.current = { mode: 'pin-turn-top', target: scroller?.scrollTop ?? contentEnd }; } else { + /* + * The pin is dropped here even when the departure happened under one, and + * that is deliberate. The pin's reservation is the blank the reader just + * scrolled out of; restoring it would pull them back down to the offset + * they left. Following the content end keeps them where they put + * themselves and shows what arrives below. + */ retirePin(); followStateRef.current = { mode: 'hold-tail', target: contentEnd }; /* - * Only a jump to latest is ever a candidate for an animation, and only a - * near one. Every other entry reason is the transcript resuming a follow - * it already owned, where an animation would be a movement the reader did - * not ask for. + * Output that caught up with a stationary reader is already at the end — + * the blank between them closing is what raised this — so the whole + * distance left is one sample of growth, and the frame loop's ease covers + * it on the next few frames. A one-shot scroll would spend that as a snap + * the reader can see, for a correction they cannot. */ - runContentEndScroll( - reason === 'jump-to-latest' && scroller - ? resolveJumpBehavior(scroller, contentEnd) - : 'auto', - ); + if (reason !== 'tail-caught-up') { + /* + * Only a jump to latest is ever a candidate for an animation, and only + * a near one. Every other entry reason is the transcript resuming a + * follow it already owned, where an animation would be a movement the + * reader did not ask for. + */ + runContentEndScroll( + reason === 'jump-to-latest' && scroller + ? resolveJumpBehavior(scroller, contentEnd) + : 'auto', + ); + } } startFollowFrame(); }, [ viewportOwner, beginSmoothScrollYield, + closeTailWatch, endSmoothScrollYield, readContentEndScrollTop, readPinScrollTop, @@ -950,7 +1135,152 @@ export function useFlowChatFollowOutput({ viewportOwner.release('follow-output'); viewportOwner.release('snap-back'); stopFollowFrame(); - }, [endSmoothScrollYield, scrollerRef, stopFollowFrame, viewportId, viewportOwner]); + + /* + * A watch is open exactly while the reader holds the viewport, which is what + * every branch below maintains. `wasFollowing` is deliberately not the gate: + * this runs on every wheel notch rather than once per gesture, so gating on + * it would open a watch on the notch that took the viewport and nowhere + * else, and any exit that found follow already released — a reader scrolling + * on from a navigation, say — would be watched by nothing. + * + * Re-opening is what must not happen instead. The offsets a crossing is + * judged against are the previous sample's, and re-baselining them halfway + * through the gesture being judged is how a reader climbing steadily reads + * as one standing still. + */ + if (reason === 'session-changed') { + closeTailWatch('session-changed'); + return; + } + if (reason === 'scroll-to-turn' || reason === 'scroll-to-index') { + // A navigation puts the reader somewhere they did not travel to, so the + // watch starts again from there rather than carrying offsets across it. + closeTailWatch('navigated', { navigationReason: reason }); + } + if (tailWatchRef.current === null) { + openTailWatch(); + } + }, [ + closeTailWatch, + endSmoothScrollYield, + openTailWatch, + scrollerRef, + stopFollowFrame, + viewportId, + viewportOwner, + ]); + + /** + * One sample of an open watch, from wherever the geometry can change. + * + * Costs a null check whenever follow owns the viewport, which is when nothing + * is being watched. Otherwise it reads `scrollHeight` — the same measurement + * the follow loop takes every frame while it *does* own the viewport, so this + * is that measurement continuing across the gap where it does not. + * + * Defined below `enterFollowOutput` because a crossing can hand the viewport + * back, which is the whole point of watching. + */ + const sampleTailWatch = useCallback(() => { + const watch = tailWatchRef.current; + if (!watch) return; + const scroller = scrollerRef.current; + if (!scroller) return; + + const scrollTopPx = scroller.scrollTop; + const contentEndPx = readContentEndScrollTop(scroller); + const blankPx = scrollTopPx - contentEndPx; + const contentDeltaPx = contentEndPx - watch.lastContentEndPx; + const scrollDeltaPx = scrollTopPx - watch.lastScrollTopPx; + watch.lastScrollTopPx = scrollTopPx; + watch.lastContentEndPx = contentEndPx; + watch.samples += 1; + + /* + * Geometry read from two different moments, so neither the latch nor a + * verdict can be taken from it. A history prepend is the case: it shifts the + * viewport by the height it inserted, before that height has been measured + * into the scroll range, and the blank between the two reads as thousands of + * pixels. Left alone rather than cleared — a restructure says nothing about + * where the reader was standing before it, and the next honest sample is a + * frame away. + */ + if (!isTailBlankMeasurable({ blankPx, tailSpacerPx: getTailSpacerPx() })) return; + + const crossing = resolveTailDepartureCrossing({ + blankPx, + contentDeltaPx, + scrollDeltaPx, + }); + /* + * A snap back of ours is travelling, so the viewport is moving under a rule + * rather than under the reader — and it is travelling *down* through the + * blank, which crosses this line from the wrong side. The latch and the + * offsets are kept current so the next real sample compares against + * something recent; no verdict is taken, because the snap hands the viewport + * to follow by its own route the moment it lands. + */ + if (pendingSnapBackTargetRef.current !== null) { + watch.blankWasVisible = crossing === 'watching'; + return; + } + if (crossing === 'watching') { + watch.blankWasVisible = true; + return; + } + /* + * The blank is gone, but it was already gone last time: the reader is above + * the end of content and staying there, which is reading history and is not + * a crossing of anything. Only the transition is a question. + */ + if (!watch.blankWasVisible) return; + + // Whether the reader's hand is still on it. The claim lapses on its own, + // so this separates a crossing during a gesture from one after it. + const gestureLive = viewportOwner.currentOwner() === 'user-gesture'; + const resumed = shouldResumeFollowAfterDeparture({ crossing, gestureLive }); + watch.crossings += 1; + /* + * A crossing the gesture vetoed keeps the latch, so the next sample judges + * the same transition again: the reader either keeps climbing, and is let go + * by a verdict that no longer needs the veto, or stops, and is followed once + * the claim lapses. Every other verdict is final for this crossing — coming + * back is what the reader has to do to raise the question again. + */ + if (!(crossing === 'content-caught-up' && gestureLive)) { + watch.blankWasVisible = false; + } + traceViewport({ + location: 'followOutput.tailCrossing', + message: resumed + ? 'output caught up with the reader, so follow takes the viewport back' + : 'the blank closed under the reader, and follow leaves it alone', + data: () => ({ + crossing, + resumed, + gestureLive, + viewportId, + blankPx: roundViewportPx(blankPx), + contentDeltaPx: roundViewportPx(contentDeltaPx), + scrollDeltaPx: roundViewportPx(scrollDeltaPx), + scrollTopPx: roundViewportPx(scrollTopPx), + contentEndPx: roundViewportPx(contentEndPx), + crossingIndex: watch.crossings, + intoWatchMs: Math.round(performance.now() - watch.openedAtMs), + }), + }); + if (resumed) { + enterFollowOutput('tail-caught-up'); + } + }, [ + enterFollowOutput, + getTailSpacerPx, + readContentEndScrollTop, + scrollerRef, + viewportId, + viewportOwner, + ]); /** * Re-assert ownership after a layout change. This deliberately does not force @@ -960,12 +1290,21 @@ export function useFlowChatFollowOutput({ */ const scheduleFollowToLatest = useCallback(() => { if (!isFollowingOutputRef.current || !isViewportActiveRef.current) { + /* + * This is the transcript's content-change signal — the resize observer + * calls it before paint on every content box change — and for a viewport + * nobody is following it is the one place a blank closing under a + * stationary reader can be seen at all. Without it the departure would + * only ever be sampled when the reader moved, which is the case it is not + * watching for. + */ + sampleTailWatch(); return; } settleFramesRef.current = SETTLE_FRAMES; applyFollowTarget(); startFollowFrame(); - }, [applyFollowTarget, startFollowFrame]); + }, [applyFollowTarget, sampleTailWatch, startFollowFrame]); const handleUserScrollIntent = useCallback(() => { pendingSnapBackTargetRef.current = null; @@ -1004,7 +1343,13 @@ export function useFlowChatFollowOutput({ // Scroll events describe the resulting viewport position, but do not prove user intent. // Layout growth and virtualizer remeasurement can emit them while output follow still owns // the viewport. Explicit wheel, touch, and keyboard handlers release that ownership instead. - }, []); + // + // The other half of the departure sampler. Without it the remembered offset + // goes stale whenever the reader moves and the content does not, and the + // next crossing is attributed to the wrong one — which here would mean + // handing the viewport back to a reader who is still climbing out of it. + sampleTailWatch(); + }, [sampleTailWatch]); /** * A scroll gesture has come to rest. @@ -1386,6 +1731,9 @@ export function useFlowChatFollowOutput({ useEffect(() => stopFollowFrame, [stopFollowFrame]); useEffect(() => clearSnapBackRetry, [clearSnapBackRetry]); + // A watch the transcript outlived is not an outcome, and leaving it open + // would drop it from the trail entirely. + useEffect(() => () => closeTailWatchRef.current('unmounted'), []); return { isFollowingOutput,