From aa4fe76e0b2b6e2c80c4e3e512cc5201692790dd Mon Sep 17 00:00:00 2001 From: wsp Date: Fri, 31 Jul 2026 20:58:46 +0800 Subject: [PATCH] fix(flowchat): fix arbitrary turn navigation across renderer handoff --- flowchat-turn-navigation-fix-plan.md | 149 +++++++ .../modern/FLOWCHAT_SCROLL_STABILITY.md | 68 ++- .../components/modern/ScrollAnchor.test.tsx | 90 ++++ .../components/modern/ScrollAnchor.tsx | 13 +- ...rtualMessageList.session-boundary.test.tsx | 402 +++++++++++++++++- .../components/modern/VirtualMessageList.tsx | 286 ++++++++++--- 6 files changed, 951 insertions(+), 57 deletions(-) create mode 100644 flowchat-turn-navigation-fix-plan.md create mode 100644 src/web-ui/src/flow_chat/components/modern/ScrollAnchor.test.tsx diff --git a/flowchat-turn-navigation-fix-plan.md b/flowchat-turn-navigation-fix-plan.md new file mode 100644 index 0000000000..6b9f328693 --- /dev/null +++ b/flowchat-turn-navigation-fix-plan.md @@ -0,0 +1,149 @@ +# FlowChat Turn Navigation Fix Plan + +## Confirmed Root Cause + +The failure is not caused by an explicit streaming-time navigation block. + +Before a new turn starts, a restored historical session may use the static +initial-history list. Arbitrary turn navigation can then materialize a target +by changing `staticAnchorWindowTurnId` and rendering a bounded window around +the target. + +Starting a new turn completes backend context restoration. The session changes +from `contextRestoreState: 'pending'` to `ready`, which disables +`useStaticInitialHistoryList` and replaces the static scroller with Virtuoso. + +After this transition, a far arbitrary turn uses Virtuoso's +`scrollToIndex` fallback. In the reproduced failure: + +- Header navigation returned `accepted: true`. +- Both during streaming and after streaming, pin status stayed `pending`. +- `isFollowingOutput` was `false` in both cases. +- The target turn remained unrendered. +- The viewport stayed at the physical bottom. + +The visible drop to the bottom is a secondary effect. The navigation path +clears the new-turn sticky pin/footer reservation before the target has been +materialized. When materialization fails, the browser clamps the scroll +position to the physical bottom and there is no successful target alignment to +replace it. + +There is also a separate scroller handoff bug: `ScrollAnchor` listens through a +stable ref object and does not rebind when `.current` changes from the static +scroller to the Virtuoso scroller. + +## Recommended Design + +### 1. Use one turn-navigation transaction + +Header navigation, right-side anchor navigation, and search navigation should +share one transaction with: + +- `targetTurnId` +- `targetVirtualIndex` +- `sessionId` +- `phase: materializing | aligning | settled | canceled` +- a generation for cancellation and stale-request rejection + +The callers should request a transaction; the viewport implementation should +own the lifecycle. + +### 2. Carry the pending target across the static-to-Virtuoso handoff + +When Virtuoso mounts while a turn navigation is pending, use the pending target +as `initialTopMostItemIndex` with `align: 'start'` instead of defaulting to the +latest turn. After the target DOM exists, perform the exact pinned-top +alignment using the shared `57px` header offset. + +This avoids depending on a post-mount reverse jump from the latest tail to a +far, unmeasured target. + +### 3. Preserve the physical range until target alignment succeeds + +The navigation sequence should be: + +1. Capture the target and start the navigation transaction. +2. Exit follow mode. +3. Preserve the current physical range and existing footer reservation while + the target is being materialized. +4. Materialize the target. +5. Align the target user message to the pinned top offset. +6. Confirm stable geometry for a few frames. +7. Only then release or transfer the pin reservation. + +If the request is canceled, restore the appropriate physical range and clear +the transaction without silently moving to the bottom. + +### 4. Make `rangeChanged` part of materialization completion + +Do not rely only on blind per-frame retries. On `rangeChanged` or virtual-item +updates: + +- check whether the target DOM node exists; +- retry materialization only while it is absent; +- enter `aligning` when it appears; +- settle after stable top alignment. + +Keep a bounded timeout as a final guard, but make expiry a controlled cancel +and recovery path rather than a silent fall to the physical bottom. + +### 5. Rebind `ScrollAnchor` to the actual scroller element + +Pass the current scroller element as state instead of relying only on a stable +ref object: + +```tsx + +``` + +The listener effect should depend on `scrollerElement`, so static-to-Virtuoso +replacement removes the old listener and attaches to the new node. + +## Implementation Order + +1. Add the shared navigation transaction and preserve-range semantics. +2. Feed the pending target into Virtuoso's initial positioning during the + renderer handoff. +3. Resolve the target through `rangeChanged` and exact DOM alignment. +4. Fix `ScrollAnchor` scroller rebinding. +5. Remove temporary debug instrumentation after the user confirms the issue + is fixed. + +## Regression Coverage + +Add focused tests for: + +- arbitrary navigation in the static history window; +- navigation after a new turn switches static rendering to Virtuoso; +- navigation during streaming; +- navigation after streaming ends; +- an unrendered far target becoming rendered and then aligning at the top; +- preservation of the sticky pin/footer range until alignment succeeds; +- cancellation via jump-to-latest, user scroll, session switch, or a newer + navigation request; +- `ScrollAnchor` rebinding after the scroller DOM node changes. + +## Implementation Status + +Implemented in the modern FlowChat viewport: + +- pending navigation generations are invalidated without removing the active + footer range; stale sticky reconciliation is suspended until the new target + settles +- Virtuoso receives pending static/Virtuoso targets through + `initialTopMostItemIndex`, and `rangeChanged` participates in materialization +- timeout and replacement paths release semantic ownership without silently + falling to the physical bottom +- `ScrollAnchor` rebinds to the concrete scroller element +- regression coverage covers range preservation, static-to-Virtuoso handoff, + and scroller rebinding + +Automated verification passed: + +- `pnpm run type-check:web` +- `pnpm --dir src/web-ui run lint` +- 51 focused FlowChat tests passed + +Interactive reproduction remains the final acceptance step. Temporary +interactive-debug instrumentation is intentionally retained until that +reproduction is confirmed fixed. 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 8e23b2cc67..412e675f0f 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 @@ -260,6 +260,25 @@ reservation. User navigation drops a provisional sticky range instead of transferring it into protected collapse. Established pins keep the existing protected-range handoff. +Arbitrary-turn navigation is a materialize-then-align transaction. Starting a +new request exits tail follow, but it does not remove the previous established +pin reservation before the target DOM exists. That reservation remains only as +physical scroll range; the active request prevents the old sticky target from +reconciling it. Once the requested user message is rendered, the shared pin +resolver replaces the old reservation, aligns the message to the 57px viewport +offset, and starts bounded transient stabilization. An expired request releases +semantic ownership while preserving the current physical range, so failure +cannot silently clamp the pane to the bottom. + +`rangeChanged` is a target-materialization signal, not a source of turn +identity. It retries the active generation against real DOM geometry. RAF +retries remain as a bounded fallback for browsers that coalesce range updates. +When the static initial-history renderer hands off to Virtuoso with a pending +target, that target becomes Virtuoso's `initialTopMostItemIndex`; the normal pin +resolver then performs exact alignment after mount. The right-side +`ScrollAnchor` must bind to the actual scroller element, because the stable ref +object does not change identity when its `.current` node is replaced. + Mounting an already-streaming session is not a new-turn event. Session entry resumes tail follow directly, while sticky pinning remains reserved for a new turn that appears in the currently mounted session. @@ -344,6 +363,53 @@ If a shrink happens without a collapse intent: This path is safer than doing nothing, but it is more likely to show visible movement than the pre-compensation path. +## C. Static Initial-History Turn Navigation + +The initial-history path renders a bounded static window plus estimated leading +and trailing spacers before handing the complete projection to Virtuoso. Header +turn selection and the right-side scroll anchor may target a turn outside that +window. The list first materializes a new window around the target and then +issues the requested scroll. + +A smooth scroll does not update `scrollTop` synchronously. The window swap can +therefore emit a scroll event at the old, browser-clamped physical bottom before +the target motion begins. That geometry is not evidence that the user returned +to the latest turn. While a static anchor window is active: + +- physical `atBottom` does not make the viewport semantically latest +- programmatic turn navigation never releases the anchor window +- programmatic turn navigation must not start older-history pagination when it + crosses the leading spacer; only an explicit upward user gesture may do so +- explicit jump-to-latest navigation releases it directly +- wheel, touch, keyboard, or scrollbar motion must establish a recent downward + user intent before arrival at the physical bottom may release it +- session reset clears the anchor window and any pending bottom-return intent + +Keep the user-intent window bounded. It exists only to classify the scroll event +that follows an input gesture; it must not become a persistent scroll lock or a +second viewport writer. + +## D. Arbitrary Turn Navigation Through Virtuoso + +Header turn selection and the right-side scroll anchor use the same top-aligned +pin transaction: + +1. record generation, session, target turn, behavior, and pin mode +2. exit tail follow without removing established physical range +3. if the target is absent, issue an immediate `scrollToIndex(..., align: + 'start')` +4. retry from `rangeChanged` and bounded RAF work until the target user message + exists +5. replace the prior pin reservation with the target's measured reservation +6. align the target to the shared 57px header offset and stabilize delayed + Virtuoso measurements +7. cancel stale work on a newer request, user intent, session switch, jump to + latest, or timeout + +Do not clear the previous pin/footer range in step 2. The target may be outside +the current Virtuoso range, and removing the footer first lets the browser clamp +the old position to the physical bottom before materialization succeeds. + ## Why Transition Tracking Exists User-initiated expand/collapse still uses animated layout properties such as: @@ -379,7 +445,7 @@ current `scrollTop`, without retaining provisional pixels above that range. These owner-specific transactions prevent both clear-and-reacquire frames and cumulative provisional whitespace. Any deferred follow is then replayed. -## C. Follow-Output Mode (continuous tail) +## E. Follow-Output Mode (continuous tail) When the viewport is in follow-output mode and the latest turn is still streaming, the user's intent is "keep the tail visible". After the viewport diff --git a/src/web-ui/src/flow_chat/components/modern/ScrollAnchor.test.tsx b/src/web-ui/src/flow_chat/components/modern/ScrollAnchor.test.tsx new file mode 100644 index 0000000000..715e150f1d --- /dev/null +++ b/src/web-ui/src/flow_chat/components/modern/ScrollAnchor.test.tsx @@ -0,0 +1,90 @@ +// @vitest-environment jsdom + +import React from 'react'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { ScrollAnchor } from './ScrollAnchor'; + +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +vi.mock('../../store/modernFlowChatStore', () => ({ + useVirtualItems: () => [{ + type: 'user-message', + turnId: 'turn-a', + data: { + id: 'user-turn-a', + content: 'turn-a', + timestamp: 1, + }, + }], +})); + +vi.mock('@/infrastructure/i18n', () => ({ + i18nService: { + formatDate: () => 'now', + }, +})); + +describe('ScrollAnchor', () => { + let container: HTMLDivElement; + let root: Root; + + beforeEach(() => { + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + vi.useRealTimers(); + }); + + it('rebinds scroll listeners when the concrete scroller element changes', () => { + vi.useFakeTimers(); + const firstScroller = document.createElement('div'); + const secondScroller = document.createElement('div'); + const onAnchorNavigate = vi.fn(); + + act(() => { + root.render( + , + ); + }); + + act(() => { + firstScroller.dispatchEvent(new Event('scroll')); + }); + expect(container.querySelector('.scroll-anchor')?.className).toContain('scrolling'); + + act(() => { + vi.advanceTimersByTime(801); + }); + expect(container.querySelector('.scroll-anchor')?.className).not.toContain('scrolling'); + + act(() => { + root.render( + , + ); + }); + + act(() => { + firstScroller.dispatchEvent(new Event('scroll')); + }); + expect(container.querySelector('.scroll-anchor')?.className).not.toContain('scrolling'); + + act(() => { + secondScroller.dispatchEvent(new Event('scroll')); + }); + expect(container.querySelector('.scroll-anchor')?.className).toContain('scrolling'); + + }); +}); diff --git a/src/web-ui/src/flow_chat/components/modern/ScrollAnchor.tsx b/src/web-ui/src/flow_chat/components/modern/ScrollAnchor.tsx index 7eaa014cef..f377fe1042 100644 --- a/src/web-ui/src/flow_chat/components/modern/ScrollAnchor.tsx +++ b/src/web-ui/src/flow_chat/components/modern/ScrollAnchor.tsx @@ -11,6 +11,7 @@ import './ScrollAnchor.scss'; interface ScrollAnchorProps { onAnchorNavigate: (turnId: string) => void; scrollerRef?: React.RefObject; + scrollerElement?: HTMLElement | null; } interface AnchorPoint { @@ -27,6 +28,7 @@ interface AnchorPoint { export const ScrollAnchor: React.FC = ({ onAnchorNavigate, scrollerRef, + scrollerElement, }) => { const virtualItems = useVirtualItems(); const [hoveredAnchor, setHoveredAnchor] = useState(null); @@ -37,7 +39,10 @@ export const ScrollAnchor: React.FC = ({ const [isHovering, setIsHovering] = useState(false); useEffect(() => { - const scroller = scrollerRef?.current; + // The ref object is stable while the static-history renderer hands off to + // Virtuoso. Depend on the actual element as well so the listener follows + // that DOM replacement instead of staying attached to a disconnected node. + const scroller = scrollerElement ?? scrollerRef?.current; if (!scroller) return; const handleScroll = () => { @@ -60,10 +65,10 @@ export const ScrollAnchor: React.FC = ({ clearTimeout(scrollTimeoutRef.current); } }; - }, [scrollerRef]); + }, [scrollerElement, scrollerRef]); useEffect(() => { - const scroller = scrollerRef?.current; + const scroller = scrollerElement ?? scrollerRef?.current; if (!scroller) return; if (isHovering) { @@ -75,7 +80,7 @@ export const ScrollAnchor: React.FC = ({ return () => { scroller.classList.remove('anchor-hovering'); }; - }, [scrollerRef, isHovering]); + }, [isHovering, scrollerElement, scrollerRef]); const anchorPoints = useMemo(() => { if (virtualItems.length === 0) return []; diff --git a/src/web-ui/src/flow_chat/components/modern/VirtualMessageList.session-boundary.test.tsx b/src/web-ui/src/flow_chat/components/modern/VirtualMessageList.session-boundary.test.tsx index 17c2c0e3ca..860bf99ae3 100644 --- a/src/web-ui/src/flow_chat/components/modern/VirtualMessageList.session-boundary.test.tsx +++ b/src/web-ui/src/flow_chat/components/modern/VirtualMessageList.session-boundary.test.tsx @@ -48,6 +48,8 @@ const virtuosoMocks = vi.hoisted(() => ({ renderedRange: null as { start: number; end: number } | null, scrollerScrollTo: vi.fn(), scrollToIndex: vi.fn(), + initialTopMostItemIndex: null as unknown, + rangeChanged: null as (() => void) | null, })); const flowStoreMocks = vi.hoisted(() => ({ hasPendingSessionHistoryCompletion: vi.fn(() => false), @@ -65,6 +67,9 @@ const flowDiagnosticsMocks = vi.hoisted(() => ({ enabled: false, trace: vi.fn(), })); +const scrollAnchorMocks = vi.hoisted(() => ({ + targetTurnId: null as string | null, +})); vi.mock('@/infrastructure/diagnostics/flowChatDiagnostics', () => ({ flowChatDiagnostics: { @@ -93,11 +98,13 @@ vi.mock('react-virtuoso', () => ({ Virtuoso: React.forwardRef((props: any, ref) => { const scrollerRef = React.useRef(null); const [, rerender] = React.useReducer((value: number) => value + 1, 0); + virtuosoMocks.initialTopMostItemIndex = props.initialTopMostItemIndex; + virtuosoMocks.rangeChanged = props.rangeChanged ?? null; React.useImperativeHandle(ref, () => ({ scrollTo: vi.fn(), scrollToIndex: vi.fn((options: { index: number }) => { virtuosoMocks.scrollToIndex(options); - const localIndex = Math.max(0, options.index - (props.firstItemIndex ?? 0)); + const localIndex = Math.max(0, options.index); virtuosoMocks.renderedRange = { start: localIndex, end: Math.min(props.data?.length ?? 0, localIndex + 4), @@ -249,7 +256,17 @@ vi.mock('../StickyTaskIndicator', () => ({ })); vi.mock('./ScrollAnchor', () => ({ - ScrollAnchor: () => null, + ScrollAnchor: ({ onAnchorNavigate }: { onAnchorNavigate: (turnId: string) => void }) => ( +