Review 4121

Review 4121 — fix(chat): keep reader position when loading earlier messages

PR: facebook/astryx#4121
Author: AKnassa
Verdict: approve (drafted; no public PR action)

Head reviewed

75c0ca0991192a95d73534d65fcc34801aea5849

Versions

LOOP VERSION: 1.6.0
AUDIT RUBRIC: 1.13

Lane

LANE: full
WHY: this is an R1e re-review after our standing CHANGES_REQUESTED, in core runtime scroll behavior, with layout/a11y/perf checks and browser evidence owed.

Prior review

cixzhang requested changes on 70d24f7f: align="top" omitted the spacer, so the load-earlier anchor went null after the main merge and lost 1160px of reader position; the same review also noted that Outline already had the scrollable-ancestor walk. Current head 75c0ca09 fixes both: the anchor resolves to the first message after the spacer, loading spinner, or sentinel, and Outline and ChatMessageList share getScrollableAncestor while keeping different overflow requirements.

Problem

WHY 1: A reader scrolled to the top of a long chat to load older messages, and prepending those messages moved the content they were reading.
WHY 2: Losing the visible message forces the reader to search the transcript again after every page load.
WHY 3: Chat history is useful only if older context can be read incrementally without disorientation.

USER-FACING PROBLEM: a person reading older chat history loads the next page and loses the message they were using as their place marker.
PROBLEM SEVERITY: harmful friction — the task completes, but the reader loses context and repeats work.

VERDICT: clear

Solution

The list remembers the first real message before it asks for older history. When the new messages arrive above it, the list adjusts the scroll container by the amount that message moved. That responsibility stays inside ChatMessageList, with Outline only sharing the ancestor-finding helper.

SOLUTION (2 decisions · core runtime change plus tests/docs/story)

1. Load-earlier is single-flight, marks the log busy, anchors the first message, compensates after prepend, and auto-refills when the list is still underfilled.
2. The scrollable-ancestor walk moves to an internal utility, defaulting to Outline's overflow-required behavior while ChatMessageList opts out for underfilled lists.

BURDEN: medium — one observer effect, one layout-effect compensation pass while an anchor is pending, no new public prop/export.
BURDEN MATCH: proportionate — the behavior serves an existing prop whose current behavior loses reader position; focused tests and browser runs cover the moving parts.

VERDICT: clear

Architecture

OWNER: ChatMessageList owns load-earlier lifecycle; getScrollableAncestor owns the reusable DOM walk.
TIER 1: no new public system; reuses existing ChatLayout scroller context when present.
TIER 2: none.
SEAMS: ChatLayout scroller, standalone scroll container, legacy overflow-y: overlay, top/bottom alignment, underfilled list.
BEHAVIOR UNIT: inline for ChatMessageList, pure utility for ancestor lookup — the cross-component piece is testable separately.

seam	driven result
align="bottom"	first and repeated prepends kept the first fully visible message at 0px drift.
align="top"	first and repeated prepends kept the first fully visible message at 0px drift.
standalone ancestor utility	nearest overlay ancestor returned; default skips non-overflowing ancestor, requireOverflow: false returns it.
underfilled list	auto-refilled from 20 to 40 messages until the 2200px viewport could scroll.
mid-load user scroll	user-scrolled message stayed at the same top offset while native anchoring adjusted the scrollTop.

VERDICT: clear

Impact

End users of ChatMessageList with scrollToTopAction keep their visible message after older history prepends. Builders keep the same API, but now have a documented contract: stable keys, state applied before the promise resolves, and undefined when history is exhausted. Outline's behavior is intended to stay unchanged because the shared helper defaults to requiring actual overflow.

VERDICT: clear

API

No public API change. The existing scrollToTopAction?: () => Promise<void> contract is documented more precisely; the new utility is not re-exported from the public barrel.

VERDICT: clear

Theming

No new theme targets, tokens, variables, or style-driving props. The existing spinner appears during load and disappears after it resolves.

VERDICT: clear

Breaking

BEHAVIOR: changed intentionally for the existing load-earlier path; current head holds position for top/bottom, repeated prepend, underfilled refill, and mid-load user scroll.
API: no compile-time change and no new required field.
VISUAL: spinner appears during pending load; after-load anchor position returns to 0px drift.
THEME: no target/token removal.

VERDICT: clear

Performance & resources

EFFECTS: the existing sentinel observer now keys on action presence instead of action identity; the new layout-effect work is gated by a pending anchor and exits immediately otherwise.
RENDER: focused browser evidence showed one load at a time and no observer churn path; focused tests cover changing action identity.
LISTENERS/OBSERVERS: no document/window listener; one IntersectionObserver with cleanup.
LAYOUT: reads two rects and writes scrollTop only after a load-earlier anchor settles.
BUNDLE: one small internal utility, no dependency.

VERDICT: clear

Visual evidence

VISUAL CHECK: manual frames required
WHY: the review claims visible scroll position is preserved after a loading spinner and prepended messages.

During top-aligned load	After top-aligned prepend

After bottom-aligned prepend	RTL + reduced motion after top-aligned prepend

Sensor receipts passed for all four embedded frames: exact head 75c0ca09, Storybook stories core-chatlayout--load-earlier-history and core-chatlayout--load-earlier-history-top-aligned, neutral light theme, matching viewport/media/direction, and no page or Storybook errors.

Measured interaction rows:

path	result
bottom first prepend	Question #81 stayed first fully visible at 48px; drift 0; scrollTop compensated 1160px.
bottom repeated prepend	Question #61 stayed first fully visible at 48px; drift 0; scrollTop compensated 1160px.
top first prepend	Question #81 stayed first fully visible at 32px; drift 0; scrollTop compensated 1160px.
top repeated prepend	Question #61 stayed first fully visible at 32px; drift 0; scrollTop compensated 1160px.
loading spinner	top spinner role status appeared during aria-busy="true" and was gone afterward.
mid-load user scroll	Question #85 stayed first fully visible at 24px; native anchoring moved scrollTop 300 → 1400, with no snap-back.
underfilled refill	2200px viewport loaded 20 → 40 messages automatically, then stopped once scrollable.
utility overlay	overlay ancestor found; default skips non-overflowing inner ancestor; requireOverflow: false returns it.
RTL + reduced motion	top-aligned first prepend still drifted 0px.

VERDICT: clear

Remedy search

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

aria-busy is true only while the load-earlier transition is pending and the focused log remains focused in the main top/bottom runs. pr-a11y and pr-rtl both passed on the exact head. No new user-visible production strings were added; story text and docs are exempt where applicable, and the changed Chinese/dense docs cover the updated contract.

VERDICT: clear

Judgement

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	clear
IMPACT	clear
API	clear
THEMING	clear
BREAKING	clear
PERFORMANCE	clear
VISUAL	clear
A11Y & I18N	clear

GOAL: met — the prior top-aligned 1160px lost-position blocker is fixed; browser evidence shows 0px drift for the anchored message on first and repeated top-aligned prepends.
DISPOSITION: prior align="top" block → fixed; prior scroll-helper note → fixed by internal helper.
ADVICE: omitted — no remaining code change requested.
AUTHOR CAN PROCEED: yes — no PR changes requested by this review.
WORST OUTCOME: none found.

JUDGEMENT NEEDED: none — defect fix against an existing contract.

approve

Review

Thanks, this addresses my earlier block: align="top" now holds position through first/repeated prepends, and the shared helper keeps Outline's overflow requirement. Looks good.

[Reviewed by Robohands]

Inline

None.

Evidence I did not spend

• The PR head is behind current main, but no commits since its merge base touch any of the PR's files, and GitHub reports it mergeable.
• Focused Vitest passed: ChatMessageList.test.tsx and getScrollableAncestor.test.ts, 40 tests.
• CI is not fully green only because Vercel/initialize failed and review-required is pending this review; package checks, a11y, RTL, and visual acceptance passed.

Time

TIME total 20m
setup/rules 7m — mandatory docs, gate versions, PR metadata, safety scan
build/server 5m — fast install, one build-package build, warm main, exact-head Storybook
measuring 5m — focused Vitest and Chromium interaction matrix
writing/wiki 3m — draft, critic pass, wiki record
waste 1m — first Storybook readiness probe looked for a static build-sha on a dev server

What I could not verify

No local full-suite run; CI's test, build, Storybook, a11y, RTL, and visual jobs ran on the exact head.

What changed before posting

Not posted; drafted for Cindy.