-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(ui): bottom-align short VP content so blank space is at top #9305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
35b7051
e4ab668
53c0178
c26ef42
a5403de
23988d6
d3abb95
dd0c773
05c073f
5e38caf
047021a
c68e20b
fa7f5c9
9cacb01
c7e46dc
754d17b
ee6f77e
839a873
6c1ada4
9b1afb2
cf04b3f
d530b3d
be2358f
9361c28
4960a7e
d0ce0bb
d21e8f5
a95525e
0e5a87e
efd5ca1
5599fb9
bc395c4
88d4d10
3047e02
0bc876f
185e3b3
cf43c2c
98b7ddd
3904c6d
57afd05
6705926
a03f6ca
45f59b6
829564b
a1e6801
e5dd22d
31fcd02
6796923
38559cb
a6cf115
ff8da3b
f648122
633924d
c402ce2
969ff98
59d87dc
cfd844b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,6 +145,7 @@ export const MainContent = ({ footerRef }: MainContentProps) => { | |
| staticAreaMaxItemHeight, | ||
| availableTerminalHeight, | ||
| historyRemountKey, | ||
| sessionStats, | ||
| } = uiState; | ||
|
|
||
| // Filter out items whose display is suppressed (e.g. /history collapse). | ||
|
|
@@ -511,6 +512,12 @@ export const MainContent = ({ footerRef }: MainContentProps) => { | |
| return ( | ||
| <OverflowProvider> | ||
| <ScrollableList | ||
| // Session id is the dataset identity: /clear and /resume each | ||
| // start a new session and replace the whole history. Remount the | ||
| // list on that boundary so carried scroll state (anchor, | ||
| // sticking, park marks) resets by construction instead of | ||
| // leaking into the new dataset (#9305 review R18-1). | ||
| key={sessionStats.sessionId} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R42-1: [fails-closed] [regression] (still stands — re-confirmed this round by code identity, and the core-config fact re-read at this commit) The session id is not the whole dataset identity: a same-session
Witness (round 42's executed end-to-end probe): The Key on the dataset-replacement signal The fix must not violate the VP path's deliberate refusal of Please extend the new 中文说明R42-1:[fails-closed] [regression](仍然存在——本轮以代码同一性重新确认,并在该提交上重读了 core 配置事实)会话 id 并不构成完整的数据集身份:同会话的 故障场景: 证据见上方英文部分的探针输出: 建议修复:以 修复约束:不得违反 VP 路径对 修复验证:请扩展 — qwen3.8-max via Qwen Code /review (v0.23.0)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R42-1: [fails-closed] [regression] (still stands — the four PR files are byte-identical between the commit round 43 reviewed and this head, and all four in-place paths were re-verified at this commit) The session id is not the whole dataset identity. Four live paths replace the entire VP history without changing On those paths the viewport lands on an arbitrary index of the rebuilt history instead of the session's live bottom, and the in-component clamp and Witness — all four paths verified at this commit: Severity note, recorded rather than acted on: this round's arm A/B found that remounting does NOT restore follow (arm A in-place and arm B remount are both dead). That measurement is confounded rather than exculpatory — arm B lands on a banner-only dataset, which is R40-1's entrance (b), so the run measures R40-1 instead of isolating this entry's harm. Settling whether the Critical rests on the key gap alone needs a shape where remounting does change the outcome, which could not be constructed this round. State the invariant once, correctly, and name the paths the key does not cover: session id is the dataset identity for The fix must not switch the key to Fix witness: extend the new 中文说明[Critical] R42-1:[fails-closed] [regression](依然成立 —— 本轮 head 与第 43 轮所审查的 commit 之间四个文件逐字节相同,且四条就地替换路径均已在本次 commit 上重新核实)session id 并不构成完整的数据集身份。有四条实际存在的路径会在 在这些路径上,视口会落在重建后历史的任意索引处,而不是该会话的实时底部;而组件内部的 clamp 与 证据 —— 四条路径均已在本次 commit 上核实:见上方英文部分(Ctrl+L 的 关于严重级别,此处仅记录、不据此改动结论:本轮的 A/B 双分支实测发现「remount 并不能恢复跟随」(就地分支 A 与 remount 分支 B 都是死的)。该测量存在混淆因素而非免责证据 —— 分支 B 落在「仅剩 banner」的数据集上,而这正是 R40-1 的入口 (b),因此该实验测量到的是 R40-1,而未能隔离出本条目自身的危害。要判定该 Critical 是否仅由 key 覆盖不足造成,需要一个「remount 确实会改变结果」的形态,本轮无法构造。 修复方向:把该不变量正确陈述一次,并点名 key 覆盖不到的路径 —— session id 是 修复不得把 key 换成 修复验收测试:在 — qwen3.8-max via Qwen Code /review (v0.23.0) |
||
| ref={scrollRef} | ||
| hasFocus={viewportInteractive} | ||
| data={allVirtualItems} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical] R42-1: [fails-closed] [regression] (new this round — confirmed by an executed end-to-end probe at
a03f6ca63a; the underlying key-scoping gap was recorded as round-28 deferral D28-1 at Suggestion level, and is escalated here because the frame outcome is now measured rather than hand-traced) The session id is not the whole dataset identity: a same-session/resumereplaces the entire history without changingsessionStats.sessionId, so the list is not remounted and the positionalreAnchorClampMark, the carried anchor, sticking and theh-<id>height cache all carry into the swapped dataset — falsifying the invariant this diff states two files away at VirtualizedList.tsx:418-419, "Dataset swaps (/clear, /resume) never reach the mark because MainContent keys the list by session and remounts it."handleResume(sessionId)has no same-id early-out (useResumeCommand.ts:101-241): it callsstartNewSession(sessionId)with the selected id at :235, thenclearItems(); loadHistory(uiHistoryItems)at :239-241 in the same synchronous block. When the user picks the session they are already in — a pathAppContainer.tsx:1858-1860explicitly handles, "same-id resume (no sessionId change → no effect re-arm)" —createDefaultStats(sessionId)produces the same string, React sees an unchanged key, and no remount happens. Two facts make the carried state survive rather than fall out: item 0 is always the constant-key banner sentinel (VP_BANNER_ITEM, key'vp-banner', MainContent.tsx:110-127), so a mark at index 0 validates against any dataset, and history keys areh-${item.id}while the rebuilt history re-mints ids from 1, so the height cache carries too and the mark now sits on a different message. Measured outcome: the resumed session opens frozen on the pre-resume banner rows with zero resumed items visible, and the first streamed reply of the resumed session never reaches the screen — while a different-id resume of the same data (which does remount) shows the resumed tail correctly, and the merge base follows throughout.Witness:
The
PR MOUNTarm is the discriminator: the same resumed dataset renders correctly when the list is remounted, so the freeze is the missing remount and not the dataset. One limit on this evidence, stated rather than hidden: the probe drivesVirtualizedListdirectly, modelling the swap as one batched dataset replacement with the banner held at index 0 — which is whatclearItems(); loadHistory()in one synchronous block produces — but it does not drive the realDialogManager→handleResumepath (DialogManager.tsx:534), so the frame outcome is measured and the product wiring is code-read.Key on the dataset-replacement signal
/resumealready emits rather than on session identity alone —remount?.()(refreshStatic) bumpshistoryRemountKeyat the end of both the resume and branch flows — or invalidatereAnchorClampMarkinsideVirtualizedListwhen dataset identity changes instead of relying on the host to remount. Whichever is chosen, the comment at VirtualizedList.tsx:418-419 and the three test comments that assert the opposite wiring (VirtualizedList.test.tsx:301, :1033-1034, :1187-1188) need correcting in the same change, since they are what tells the next reader that this path is impossible.The fix must not violate the VP path's deliberate refusal of
historyRemountKey— MainContent.tsx:255-262 states it ignores those bumps on purpose ("chunked-replay reset for VP users so a Ctrl+O / model-change bump"), so folding that key into the VP list key also remounts on every Ctrl+O and model switch, discarding the measured-height cache and the user's scroll position there; if that cost is unwanted, invalidate the mark inVirtualizedListinstead.Please extend the new
MainContent.test.tsxtestremounts the VP list when the session id changes (/clear, /resume) (#9305)with the same-id swap it currently omits: rerender with an unchangedsessionIdbut a replacedhistory, assertingscrollableListMountSpygoes to 2 (or, for the in-component fix, that a mark carried at index 0 no longer validates against the swapped dataset). With the key as shipped the spy stays at 1 and that assertion is red.中文说明
R42-1:[fails-closed] [regression](本轮新发现——在
a03f6ca63a上由已执行的端到端探针确认;其底层的「键作用域」缺口曾以第 28 轮延后项 D28-1 记录为 Suggestion 级,此处升级是因为帧结果现在由实测得出而非手工推演)会话 id 并不构成完整的数据集身份:同会话的/resume会替换整段历史却不改变sessionStats.sessionId,因此列表不会重挂载,位置型的reAnchorClampMark、承载的锚点、贴底状态与h-<id>高度缓存都会带入被替换的数据集——这与本 diff 在另一文件中声明的不变量相矛盾,见 VirtualizedList.tsx:418-419:「Dataset swaps (/clear, /resume) never reach the mark because MainContent keys the list by session and remounts it.」故障场景:
handleResume(sessionId)没有同 id 的提前返回(useResumeCommand.ts:101-241):它在 :235 以被选中的 id 调用startNewSession(sessionId),随后在 :239-241 于同一同步块中执行clearItems(); loadHistory(uiHistoryItems)。当用户选择自己当前所在的会话时——AppContainer.tsx:1858-1860明确处理了该路径("same-id resume (no sessionId change → no effect re-arm)")——createDefaultStats(sessionId)产生相同字符串,React 看到键未变,于是不发生重挂载。有两个事实使被承载的状态存活下来而非失效:索引 0 永远是常量键的 banner 哨兵(VP_BANNER_ITEM,键'vp-banner',MainContent.tsx:110-127),因此位于索引 0 的标记对任何数据集都校验通过;而历史条目的键是h-${item.id},重建的历史又从 1 开始重新生成 id,因此高度缓存同样被带入,且该标记现在落在另一条消息上。实测结果:恢复后的会话打开时冻结在恢复前的 banner 行上,可见的恢复条目数为零,且恢复后会话的第一条流式回复永远不会出现在屏幕上——而对相同数据执行一次不同 id 的恢复(会重挂载)则正确显示恢复后的末尾,merge base 全程正常跟随。证据见上方英文部分的探针输出:
PR MOUNT臂是判别依据——同一份恢复数据在列表被重挂载时渲染正确,因此冻结的原因是缺失的重挂载,而不是数据本身。对该证据的一点限制,明确说明而非隐藏:探针直接驱动VirtualizedList,把替换建模为一次批处理的数据集整体替换并保持 banner 位于索引 0——这正是同一同步块中clearItems(); loadHistory()所产生的形态——但它没有驱动真实的DialogManager→handleResume路径(DialogManager.tsx:534),所以帧结果是实测的,而产品接线是代码阅读得出的。建议修复:以
/resume本身已经发出的「数据集被替换」信号为键,而不仅以会话身份为键——remount?.()(refreshStatic)会在 resume 与 branch 两个流程末尾提升historyRemountKey——或者在VirtualizedList内部于数据集身份变化时使reAnchorClampMark失效,而不是依赖宿主重挂载。无论选择哪种,都需在同一次改动中修正 VirtualizedList.tsx:418-419 的注释以及三处断言相反接线的测试注释(VirtualizedList.test.tsx:301、:1033-1034、:1187-1188),因为正是它们告诉后来的读者这条路径不可能发生。修复约束:不得违反 VP 路径对
historyRemountKey的刻意拒绝——MainContent.tsx:255-262 说明 VP 路径是有意忽略这些提升的("chunked-replay reset for VP users so a Ctrl+O / model-change bump"),因此把该键并入 VP 列表键会导致每次 Ctrl+O 与模型切换也重挂载,在那里丢弃已测量的高度缓存与用户的滚动位置;若不愿承担该代价,应改为在VirtualizedList内部使标记失效。修复验证:请扩展
MainContent.test.tsx中新增的remounts the VP list when the session id changes (/clear, /resume) (#9305)用例,补上它当前遗漏的同 id 替换:在sessionId不变但history被替换的情况下重渲染,断言scrollableListMountSpy变为 2(若采用组件内修复,则断言位于索引 0 的承载标记不再对替换后的数据集校验通过)。按当前提交的键,该 spy 会停在 1,断言为红。— qwen3.8-max via Qwen Code /review (v0.23.0)