Skip to content

fix(ui): preserve tail pin and correct reader scroll assertion - #4831

Merged
Astro-Han merged 2 commits into
mainfrom
fix/storybook-scroll-growth
Sep 5, 2026
Merged

fix(ui): preserve tail pin and correct reader scroll assertion#4831
Astro-Han merged 2 commits into
mainfrom
fix/storybook-scroll-growth

Conversation

@Astro-Han

@Astro-HanAstro-Han commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

A pending tail-write scroll event can arrive after content shrinks above the viewport and grows below it. Native anchoring moves scrollTop upward while the net scrollHeight grows, so the authority mistakes its own pending event for reader input and permanently releases the tail pin. Disable native anchoring while pinned, restore it when the reader leaves the tail, and restore the original inline style on detach.

The reader-position story also measured the first mounted turn, thousands of pixels above the viewport. Native anchoring can legitimately move that offscreen turn while preserving what the reader sees. Measure the first turn intersecting the viewport instead, retaining the 4px threshold and the separate check against returning to the tail. Refs #4766.

Verification

  • New Chromium story: 3/3 failures before, with one false reader event and 599px tail lag; 3/3 passes after, with no reader event and 0px lag. Removing the pending scroll event also makes the old implementation pass, isolating the event-ordering condition.
  • Reader-anchor discrimination: changing an above-viewport turn's height made the old assertion fail 3/3 (59px) while the visible-anchor assertion passed 3/3. Forcing a real pullback still failed 3/3. These temporary perturbations were removed before the final smoke run.
  • Full Storybook smoke: 290 stories / 316 renders passed, including outside-turn growth, reader scroll-up, streaming tail, history requests, and native anchoring/rounding.
  • UI suite: 373 tests passed (including all 13 scroll-authority tests).
  • npm run build, npm run typecheck, npm run lint, npm run format:check, and ASF header check passed.

The original Linux CI failure (main run, 509px lag) was verified in its log but not reproduced locally. The new browser regression proves the authority defect and its fix; it does not establish that every other reported scroll flake shares this cause.

Scope and prior work

#4793 was closed because its rebased Electron A/B showed no improvement for large streaming arrivals combined with small upward gestures. This PR does not claim to fix that remaining #4269 scenario. Its browser regression exercises a different condition: an outstanding tail-write event followed by opposite height changes above and below the viewport.

A same-browser comparison against that regression gave:

ImplementationRuns losing the pinTail lag
Base before this PR3/3599px
Base with only #4834's echo-baseline deletion3/3599px
This PR0/30px

The smaller echo-only change therefore does not replace this fix. No second fix, new scroll state, timer, or compensation loop is added to production. #4259 currently owns sub-turn containment and its cold geometry checks; #4560 owns bounded-history navigation. Neither changes this policy implementation. Consolidating the remaining imperative transcript scroll writers stays outside this PR.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex investigated the failure, implemented the fix and browser regression, and ran validation.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actionsgithub-actionsBot added the effort/S Under 100 readable lines label Sep 5, 2026
@Astro-HanAstro-Han changed the title fix(ui): keep native anchoring from releasing the tail pinfix(ui): preserve tail pin and correct reader scroll assertionSep 5, 2026

@jackwenerjackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at exact head 5de82736. No findings.

The two mechanisms are divided rather than stacked

This touches the same file as #4786 and #4793, so the first question is what happens to the band and unexplained residue that #4786 landed. They are intact, and the comment draws the line explicitly: while pinned, native anchoring is disabled so this authority is the only writer; once released, anchoring is restored and geometry accounts for it before an unexplained movement is reported as reader input.

That is what makes this different from #4793's shape. The objection there was that keeping only the boolean moved would drop the released-path discriminator — a reader scrolling in the same event as content landing above them would be swallowed. Here the released path keeps the band; the disabling is confined to the interval where the pin already owns scrollTop.

The root cause is stated more precisely than "the guard swallows the reader": net height cannot explain anchoring when content shrinks above the viewport while growing below it. The two changes cancel in the total, so a pending tail-write event arrives looking like reader input, and the pin is released permanently.

previousOverflowAnchor being captured means the original inline style is restored on detach, so the authority does not leave a mark on a host it no longer owns.

The verification isolates the condition rather than only demonstrating the fix

Removing the pending scroll event makes the old implementation pass. That is the control that turns "the fix works" into "this is the condition" — without it, 3/3 → 3/3 would only show that something changed.

The reader-anchor assertion change is the part that would normally deserve suspicion, since moving an assertion from the first mounted turn to the first turn intersecting the viewport could simply be measuring something easier. The evidence rules that out: the old assertion fails 3/3 at 59px when an above-viewport turn's height changes, the visible-anchor assertion passes 3/3 there, and forcing a real pullback still fails 3/3. The new assertion is narrower in what it accepts, not looser.

Scope, stated plainly

The description does two things worth noting. It records why #4793 was closed — its rebased A/B showed no improvement for large streaming arrivals with small upward gestures — and it does not claim to fix that remaining #4269 scenario. It also states that the original Linux CI failure was verified in its log but not reproduced locally, and that this regression does not establish that every reported scroll flake shares this cause.

Both are the right boundaries to draw. A fix in this area that claimed more than its regression exercises would be very hard to disprove later.

Storybook smoke at 290 stories / 316 renders and the UI suite at 373 tests, including all 13 scroll-authority tests.

This is a fix, so the merge decision remains a human's.

简体中文

5de82736 上批准。没有发现问题。

两套机制是分工,不是叠加

这次改动与 #4786#4793 触及同一个文件,所以第一个该问的是:#4786 落地的那条带与 unexplained 残差会怎样。它们完好无损,而且注释把界线画得很明确:pinned 期间禁用原生锚定,使本 authority 成为唯一的写者;一旦释放,恢复锚定,并在把一次无法解释的位移报告为读者输入之前,先由几何来解释它。

这正是它与 #4793 的形状不同之处。 那边的反对意见是:只保留布尔 moved 会丢掉 released 路径的判别器——一个在「内容落于其上方」的同一事件中滚动的读者会被吞掉。而这里 released 路径保留了那条带;禁用只被限制在「pin 本就拥有 scrollTop」的那段区间内。

根因的表述也比「守卫吞掉了读者」更精确:当内容在视口上方收缩、同时在下方增长时,净高度无法解释锚定。 两处变化在总量上相互抵消,于是一个待处理的 tail-write 事件到达时看起来就像读者输入,而 pin 被永久释放。

捕获 previousOverflowAnchor 意味着 detach 时会还原原始的 inline 样式,所以这个 authority 不会在一个它已不再拥有的宿主上留下痕迹。

验证隔离了条件,而不只是演示了修复

移除那个待处理的 scroll 事件,会让旧实现也通过。 这正是把「修复有效」变成「这就是那个条件」的对照——没有它,3/3 → 3/3 只能说明有什么东西变了。

reader-anchor 那处断言改动,通常最该被怀疑:把断言从「第一个挂载的 turn」挪到「第一个与视口相交的 turn」,完全可能只是换了个更容易测的对象。证据排除了这一点:当视口上方某个 turn 的高度变化时,旧断言以 59px 3/3 失败,可见锚点断言在同一情形下 3/3 通过,而强制一次真实的回拉,它仍然 3/3 失败新断言在「接受什么」上更窄,而不是更松。

范围,明确写出

描述做了两件值得称道的事。它记录了 #4793 为何被关闭——其 rebase 后的 A/B 对「大规模流式到达叠加小幅向上手势」没有改善——并且明确不声称修复 #4269 剩下的那个场景。它还写明,原始的 Linux CI 失败只在日志中得到核实、本地未能复现,而这条回归并不能确立「每一个已报告的滚动 flake 都出于同一原因」

这两条边界都划得对。 在这个区域,一个宣称超出其回归所验证范围的修复,日后会非常难以证伪。

Storybook smoke 290 stories / 316 renders,UI 套件 373 项测试,含全部 13 条 scroll-authority 测试。

这是一个 fix,合并与否仍由人决定。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@Astro-Han
Astro-Han merged commit 3fdaddb into mainSep 5, 2026
1 check passed
@Astro-Han
Astro-Han deleted the fix/storybook-scroll-growth branch September 5, 2026 08:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/SUnder 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Astro-Han@jackwener