Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 149 additions & 0 deletions flowchat-turn-navigation-fix-plan.md
Original file line number Diff line number Diff line change
@@ -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
<ScrollAnchor scrollerElement={scrollerElement} />
```

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.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
90 changes: 90 additions & 0 deletions src/web-ui/src/flow_chat/components/modern/ScrollAnchor.test.tsx
Original file line number Diff line number Diff line change
@@ -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(
<ScrollAnchor
onAnchorNavigate={onAnchorNavigate}
scrollerElement={firstScroller}
/>,
);
});

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(
<ScrollAnchor
onAnchorNavigate={onAnchorNavigate}
scrollerElement={secondScroller}
/>,
);
});

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');

});
});
13 changes: 9 additions & 4 deletions src/web-ui/src/flow_chat/components/modern/ScrollAnchor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import './ScrollAnchor.scss';
interface ScrollAnchorProps {
onAnchorNavigate: (turnId: string) => void;
scrollerRef?: React.RefObject<HTMLElement | null>;
scrollerElement?: HTMLElement | null;
}

interface AnchorPoint {
Expand All @@ -27,6 +28,7 @@ interface AnchorPoint {
export const ScrollAnchor: React.FC<ScrollAnchorProps> = ({
onAnchorNavigate,
scrollerRef,
scrollerElement,
}) => {
const virtualItems = useVirtualItems();
const [hoveredAnchor, setHoveredAnchor] = useState<AnchorPoint | null>(null);
Expand All @@ -37,7 +39,10 @@ export const ScrollAnchor: React.FC<ScrollAnchorProps> = ({
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 = () => {
Expand All @@ -60,10 +65,10 @@ export const ScrollAnchor: React.FC<ScrollAnchorProps> = ({
clearTimeout(scrollTimeoutRef.current);
}
};
}, [scrollerRef]);
}, [scrollerElement, scrollerRef]);

useEffect(() => {
const scroller = scrollerRef?.current;
const scroller = scrollerElement ?? scrollerRef?.current;
if (!scroller) return;

if (isHovering) {
Expand All @@ -75,7 +80,7 @@ export const ScrollAnchor: React.FC<ScrollAnchorProps> = ({
return () => {
scroller.classList.remove('anchor-hovering');
};
}, [scrollerRef, isHovering]);
}, [isHovering, scrollerElement, scrollerRef]);

const anchorPoints = useMemo<AnchorPoint[]>(() => {
if (virtualItems.length === 0) return [];
Expand Down
Loading
Loading