fix(flowchat): settle the transcript viewport across reopen, history paging and navigation - #2255
Merged
Merged
Conversation
… rule Reported as: open a 12-Turn session, switch away, switch back — and the transcript comes up with Turn 5 pinned to the top instead of at the end where it was left. Offset 0 of a history window starting at ordinal 4 *is* Turn 5 at the top, and the window was one nobody asked for. The list is keyed on the session, so switching back is a full remount: fresh scroller at offset 0, empty measurement cache, opening reveal re-armed. 144ms into that reveal, with the viewport still at 0, the head boundary read as reached — a tail of three Turns is shorter than one viewport — and paged. The first visit had taken 674ms to load that window and landed after the reveal had settled; the second found it cached and answered in 6ms, which is the whole difference between the two. What arrived was 22301px of history above a viewport that had not been placed yet. The compensation for that is deliberately a shift rather than a write, refused only by an owner that holds a target — and it was refused, `heldBy: follow-output`. Follow-output was not going to act: followOutput.enter session-open, viewportId 3, scrollTop 0 followOutput.frameStoodDown not-following, settleFrames 90 prependCompensated shiftedPx 22301, followTargetPx null openReveal settled false, frames 40, scrollTop 0 The loop stood down on its first frame with its whole 90-frame budget untouched, and there is no `followOutput.exit` anywhere in the trail because nothing exited. `isFollowingOutputRef` was assigned from the `isFollowingOutput` state on every render. Ownership is written imperatively by enter and exit; the state is only how the rest of the component hears about it. Between the two, React is free to render the value from before the update — an update scheduled from a passive effect sits below a synchronous render — and session-open enters from a mount effect, so it hits this every time. The register still recorded follow-output as the holder, which is the state nothing recovers from: one side says the viewport is owned, the other says it is not following. The other two entries above are the same open, one and two switches earlier. Both were clobbered too; both were rescued by a resize callback restarting the loop before it mattered. Three fixes, independent on purpose — each one alone leaves the viewport at the end of the transcript: - The mirror is gone. Every other ref on that line is a prop, which is consistent with the render that assigned it by construction; this one had two writers. - A refused shift now wakes whoever it was left to. Follow-output is the only holder that can be asleep — its ownership outlives its frame loop so streaming can resume without re-entering, and the loop stops once the transcript settles — so a page landing after that was left to a writer that would never act, with or without the clobber above. - Paging is refused while the transcript is still being placed, and the reveal settling asks again. At offset 0 of an unplaced viewport the head is trivially reached, and what the page then does is prepend history above that same placement. Deferred rather than dropped: the boundary evaluation now re-runs when the reveal ends, which is the only thing a session shorter than its viewport can wait for — it emits no scroll events, having nowhere to scroll. The trail is the other half of this. It said `followOutput.enter` and then nothing, and the six lines that would have named the cause were all absent by construction: an entry declined for an inactive viewport returned silently, an exit was traced only when it had something to give up, a frame loop that stood down said nothing at all, and no line carried which transcript it came from. All of those are recorded now, behind the existing `app.logging.flow_chat_diagnostics` switch — `traceViewport` returns before evaluating its payload and `traceViewportRepeating` checks first, and the one probe on the per-frame path guards its own key the way the register's write already does. Verified: 198 files / 1660 tests, type-check and lint clean. Each fix has a test that fails without it — the clobber's reproduces the priority window with `flushSync`, and the paging tests now drive the reveal to its end the way half a second of animation frames does in the app.
Two faults on the same correction, found one behind the other by fixing
the first: a reader who paged history in from a short tail was left
sitting in the reserved blank, and once that was fixed, left 364px short
of the content end with a Turn cut off at the top of the viewport.
**The snap back gave up on one refusal.**
It asks from inside the window it is least likely to be granted. The
gesture's claim is a lapse timer — a wheel has no end of its own, so
`notifyUserScrollIntent` holds the viewport for 200ms past each notch —
and this correction is triggered *by* coming to rest, which is inside
that window by construction. Measured: 22301px of history arrived above
a viewport at offset 0, the compensation was refused as it should be
("left the displacement to whoever holds a target"), the real heights
then landed ~1670px shorter, the browser clamped the offset to the end of
the shrunken range, and the snap back that exists for exactly this was
refused `heldBy: user-gesture` 57ms into the hold. Nothing asked again.
The reader kept 841px of blank — the whole spacer — for the four minutes
the recording lasted.
Releasing the hold on the settle was the first attempt and was wrong: a
settle also lands *between* two notches of a gesture still in progress.
Measured on a session of short Turns, a dozen times over: the snap fired
110ms after a notch, the reader wheeled again 40ms later, and the
transcript stuttered under their hands every 800ms.
So the hold stands and the ask repeats. A reader still scrolling refuses
every retry and moves nothing, and their own next settle re-arms the
whole thing; a reader who has stopped is snapped back one window after
their last notch. Five attempts covers the case the retry is actually
for — the reader has stopped while something else is still moving the
transcript under them, where no further scroll event is coming.
**And then it resumed on the offset the animation stopped at.**
Which is not the offset the follow rule owns, and the two differ for the
same reason the snap was needed: a history page whose items are still
measuring. `enterFollowOutput('tail-snap-back')` took `scrollTop` as the
hold rule's memory, and the hold rule *defends* its memory — it tolerates
a gap below the content end of up to 60% of the viewport, so the
difference was not corrected, it was kept.
Measured on a 43-Turn session paged from a three-Turn tail: issued for
12336 while the content end was there, landed 608ms later against a
content end of 11972, and the first follow frame after it read `desired
11972, target 12336, onTarget true` and never moved again.
`resolveFollowState` is `resolveFollowTargetScrollTop` with the mode kept
rather than dropped, so the resume goes through the same rule the snap
back aimed with, evaluated when it lands instead of when it was issued.
Identical when nothing moved in between, which is the ordinary case.
Verified: 445 files / 3161 tests, type-check and lint clean. Three tests,
each red without its fix — the retry's asserts that the first ask *is*
refused, since forcing it through is the fault it replaces.652 boundary asks in one recording, 648 of them answered `precondition`, and every one of them re-armed the direction so the next scroll event asked again. 664 of the 3077 lines in that log were this. The transcript was a session with every Turn it was ever going to have, and the reader was resting at its head. The precondition guard for "no history window is open" was one `||` covering three unrelated facts — wrong direction, session not partial, catalog not loaded — and answered all of them `cancelled`. Only the last of the three is transient. A session that is not partial *is* the transcript on screen: its head is the first Turn, and nothing will ever be before it. That is `exhausted`, which is the answer that latches the direction quiet until the window moves, and it is also what the ordinal path one step further on would have said — `getSessionCanonicalTailRange` returns `startOrdinal: 0` for such a session, so `resolveHistoryBoundaryTarget` answers `reached-start`. The guard was standing in front of the right answer with a temporary one. `resolveTailBoundaryPrecondition` joins its siblings in `flowChatLiveTailWindow.ts`, where the three outcomes can be stated and tested apart from the component. `after` deliberately keeps answering `cancelled` even though nothing follows the canonical tail: it reaches that branch only through the retained continuous projection, which renders as a history window while the viewport intent is the live tail — and the latch is cleared by the window bounds changing, which that projection has none of. Latching it would be permanent for the rest of the presentation, to save asks that were not the ones being measured. Two probes were lying in the same recording, which is worth more than the noise they made — the next investigation reads these first: - `latched-exhausted-while-partial` fired four times, all `after`. The alarm means "history has gone silently missing", which is a question about the *head*; `after` latches the moment the reader reaches the newest Turn, and a session stays partial throughout the paging that gets them there. It was warning about the ordinary case. `resolveHistoryBoundaryTarget` already draws this line one layer down, between `reached-latest` and `beyond-known-total`. - `turnNavigation.placed.outcome` reported `driftPx: -892.7`, the largest displacement in the log, for a navigation that behaved correctly. The unrendered branch places the Turn instantly and then clamps it out of the reserved blank 20ms later, in the same task and as one visible movement — but it traced them as two placements, so the first one's outcome sample compared where the viewport came to rest against a target this function had itself superseded. The clamp is now part of the placement it belongs to, and reports `clampedToContentEnd` and the offset it clamped from. Verified: 445 files / 3161 tests, type-check and lint clean. The new unit tests cover both directions of the precondition that must not be confused — a catalog still on its way, and a session absent from the store, both of which have to stay `cancelled`.
A focus request — a usage-report entry, a cross-session jump — is resolved against three keys in turn: the Turn's id, its index among the rendered items, and its position in the transcript on screen. The first two placed the viewport instantly. The third, `scrollToTurn`, animated. Which one runs depends only on what the request happened to carry, so the same click animated or jumped depending on whether the report knew the Turn's id. Nothing the reader did chose between them. Instant is the side to settle on: it is what the other two already do, what turn-rail navigation and search navigation do, and the only behaviour `navigateToTurnWithStatus` can honour for a Turn that is not rendered yet — that branch has to read its landing back to clamp it out of the reserved blank, and an animation has not arrived to be read. The header button's jump to the current Turn is unaffected: it asks for an animation itself, over a Turn that is on screen by definition.
…owed Jumping to latest was animated at any distance, and past a few screens that is not an animation. The frame loop stands down for a bounded time and takes the viewport back wherever the animation has got to: measured, a jump issued for 8717px animated 5480 of them and was finished in a single 3290px write. Two thirds of a scroll and then a jump, which is worse than either half on its own — and no yield budget fixes it, since a longer one only makes the reader wait through more of something they cannot read. So `resolveAnimatedJumpBehavior` animates within `FLOWCHAT_ANIMATED_JUMP_MAX_VIEWPORTS` and lands outright past it. Counted in viewports rather than pixels: the question is whether the reader can follow the movement, and what they can follow is a share of what they can see. The same 2000px is two and a half screens on a laptop and most of one on a tall display. Three reasons the far case is not worth animating even when it finishes: - The animation exists for spatial continuity, and three screens on the transcript in between goes past faster than anyone can read it. What is left is a wait where the answer was. - Animating across N screens of a virtualized transcript renders and measures every item passed while the animation runs, and heights are estimates until measured — so the content end moves under an animation aimed at where it used to be, and the follow loop corrects it afterwards as a second, visible movement. - Every other navigation in the transcript is instant already, so the rule is now one sentence rather than a list of entry points. Both jump-to-latest branches go through it, including the one that resumes a pinned Turn — that one is under a viewport by construction and keeps its animation. The post-streaming settle is left alone for the same reason: the gap it closes is bounded by `tailHoldMaxGapPx`, 60% of one viewport. `followOutput.jumpBehavior` records the decision and the distance in viewports, because an animation that was never issued and one the loop cut short look identical afterwards and call for opposite fixes. It is also how the constant gets checked: a `backstop` reason in `followOutput.animatedScrollEnded` means an animation ran out its yield without arriving, and the number is too high.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five viewport fixes in the modern FlowChat transcript: the root cause of a reopened session landing on the wrong Turn, where the viewport comes to rest after a history page, a boundary that asked for history forever, and two navigations whose motion did not match anything the reader did. Three diagnostics that were reporting the ordinary case as a fault are corrected along the way.
Fixes #
Type and Areas
Type: bug fix / regression fix / docs
Areas: web UI —
src/web-ui/src/flow_chat/components/modern(transcript viewport, follow rule, history paging, viewport diagnostics)Motivation / Impact
1. Reopening a session no longer lands on the wrong Turn. Reported: open a 12-Turn session, switch away, switch back, and the transcript comes up with Turn 5 pinned to the top instead of at the end where it was left.
isFollowingOutputRefwas assigned from render state, while ownership is written imperatively by enter/exit — an update queued from a passive effect can be skipped by a higher-priority synchronous render, and session-open enters from a mount effect. The register recorded follow-output as the holder while the follow loop believed it was not following, which is a state neither side recovers from. Three independent fixes, each of which alone leaves the viewport at the end: the mirror is gone; a refused prepend shift now wakes the holder it was left to; paging is refused while the transcript is still being placed, and the reveal settling asks again.2. A reader is no longer stranded in the reserved blank after a history page. A refused snap back now retries within a bound instead of giving up — what refuses it is the reader's own gesture claim, which lapses.
3. The snap back lands where the transcript ended up, not where the animation stopped.
tail-snap-backseeded the hold-tail memory withscrollTop, and hold-tail tolerates a gap of up to 60% of the viewport — so the difference was not corrected, it was kept. Measured on a 43-Turn session: 364px of reserved blank below the transcript and the fourth-from-last Turn cut off at the top.4. A session that holds every Turn it has is no longer asked for more. 652 boundary asks in one recording, 648 answered
precondition, each one re-arming the direction so the next scroll event asked again — 664 of 3077 log lines. The guard was one||over three unrelated facts, only one of which is transient.5. Navigation motion is consistent. A focus request (usage-report entry, cross-session jump) resolves against three keys; the first two placed the viewport instantly and the third animated, so the same click animated or jumped depending on what the request happened to carry. Now uniformly instant. Jump to latest is decided by distance instead: animated within three viewports, instant beyond. Measured, a jump issued for 8717px animated 5480 of them and was finished by the frame loop in a single 3290px write — two thirds of a scroll and then a jump, which is worse than either half, and a longer yield budget only makes the reader wait through more of an animation they cannot read.
For developers: three decision functions move out of the components into pure, separately tested form (
resolveTailBoundaryPrecondition,resolveAnimatedJumpBehavior,resolveFollowState). Probes that were absent by construction are added — an entry declined for an inactive viewport, an exit with nothing to give up, a frame loop standing down, and which transcript instance every viewport line came from — all behind the existingapp.logging.flow_chat_diagnosticsswitch, with the one per-frame probe guarding its own key the way the register's write already does.Verification
The single full-suite failure is
src/infrastructure/config/services/providerPresets.test.ts, a dynamic import exceeding the 5s test timeout under full-suite load. It passes in 1.18s on its own and is untouched by this PR (nothing here is outsideflow_chat).Every fix has a test that fails without it. The ownership clobber reproduces the priority window with
flushSync; the paging tests drive the opening reveal to its end through a manual rAF queue, the way half a second of animation frames does in the app; the snap back has both "asks again once the gesture that refused it has lapsed" and "stops asking rather than chase a reader who keeps scrolling"; the distance rule covers both sides of the boundary, a reverse-direction jump, and an unmeasured scroller.Manual checks, all added to
FLOWCHAT_VERIFICATION.md:Reviewer Notes
Read the pure functions first.
flowChatLiveTailWindow.tsandflowChatTailFollow.tscarry the reasoning and the measurements behind each decision, and they are faster to review than the component diffs that call them.Two deliberate asymmetries, both commented in place but worth naming here:
resolveTailBoundaryPreconditionanswerscancelledforafterrather thanexhausted, even though nothing follows the canonical tail. That branch is only reached through the retained continuous projection, which has no window bounds — and the latch is cleared by the bounds changing, so latching would be permanent for the rest of the presentation.tailHoldMaxGapPx, 60% of a viewport.One regression was introduced and fixed inside this series. Releasing the
user-gestureclaim fromhandleScrollSettledmade the snap back fire between wheel notches, which the reader saw as a shudder. It was replaced by the bounded retry; the route and its cost are recorded ina41232644. Reviewing "just release the other writer's claim" with the same suspicion is the useful takeaway.The threshold is tunable and has an acceptance signal.
FLOWCHAT_ANIMATED_JUMP_MAX_VIEWPORTS = 3comes from the sustained rate in the measurement above (~4570px/s). IffollowOutput.animatedScrollEndedstill reports abackstopreason, an animation ran out its yield without arriving and the number is too high.Known and deliberately out of scope: the pagination junction still shows one visible lurch — the compensation can only use the virtualizer's estimated heights, and the shrink happens in the tens-to-hundreds of milliseconds when nobody holds the viewport. The cure is capturing the anchor before the compensating shift, which touches the settle loop. Remembering the reading position across a session switch is also still unimplemented.
Checklist
AGENTS.md,FLOWCHAT_SCROLL_STABILITY.mdandFLOWCHAT_VERIFICATION.mdare updated.)