-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(web-shell): reduce streaming thought render jank #9914
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6e4e15c
fix(web-shell): reduce streaming thought render jank
ee3e076
test(web-shell): expand historical question result
a1d2ea3
fix(web-shell): address streaming review findings
511ee1e
test(web-shell): strengthen streaming review follow-up tests
51548f2
test(web-shell): pin structural snapshot opt-in and matched insight p…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # Web Shell collapsed thinking performance | ||
|
|
||
| ## Problem | ||
|
|
||
| Pure assistant and thought tail appends wake the top-level `App`, even though | ||
| only the transcript row needs the new text. Compact activity summaries also | ||
| keep their complete tool and thought subtree mounted while collapsed, so hidden | ||
| rows continue reconciling streamed thought props. | ||
|
|
||
| ## Design | ||
|
|
||
| The top-level app consumes a structural transcript snapshot. A store change | ||
| summary proves when an update is only an append to the active assistant or | ||
| thought block; those app-level notifications are ignored. Stores without a | ||
| change summary retain the existing behavior. | ||
|
|
||
| The message list separately consumes the live throttled snapshot and applies | ||
| the existing streaming-tail projector against the app's latest structural | ||
| messages. This updates only the visible tail without starting a second | ||
| background-agent reconciliation loop. Structural changes still flow through | ||
| the app and replace the baseline immediately. Insight protocol markers use a | ||
| full projection while retaining the unchanged message prefix. | ||
|
|
||
| Compact tool summaries mount their detail subtree only while expanded, except | ||
| for MCP Apps whose iframe state must survive a collapse. The summary button | ||
| remains live while collapsed; expanding reconstructs the current tool and | ||
| thought rows from props. Collapse and expansion are immediate and unanimated. | ||
|
|
||
| ## Compatibility | ||
|
|
||
| Tool, permission, terminal, reset, history, and session changes remain | ||
| structural. Transcript callbacks continue receiving live snapshots from the | ||
| message-list boundary. Collapsing a compact group no longer preserves local | ||
| expanded state inside its hidden detail rows. | ||
|
|
||
| ## Verification | ||
|
|
||
| - Prove structural snapshots ignore pure tail appends and resume on the next | ||
| structural change. | ||
| - Prove a collapsed compact group has no detail subtree and restores current | ||
| details when expanded. | ||
| - Run the deterministic folded-thought performance scenario and targeted unit | ||
| tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[Suggestion] The snapshot mock now branches on
options?.structuralOnly, but no assertion depends on which option the App-level caller passes — every top-levelblocksconsumer is mocked to ignore its argument or sees no difference betweenblocksandliveBlocks. That leaves the App-level{ structuralOnly: true }opt-in (App.tsx:2290), the wiring this PR exists to deliver, unpinned: if a future change drops that option, the top-level render path re-subscribes to every streamed tail append — exactly the jank this PR fixes — and the whole suite stays green. An A/B probe at this commit confirms it: dropping{ structuralOnly: true }from App.tsx leaves all 684 tests of the four changed suites passing on both arms (Tests 684 passed (684)).Record the options each call site passes into the mock and assert on them in the boundary test, e.g.:
中文说明
快照 mock 现在会根据
options?.structuralOnly分支,但没有任何断言依赖 App 层调用者传入的选项——所有顶层blocks消费者要么被 mock 成忽略参数,要么对blocks与liveBlocks看不出差别。因此 App 层的{ structuralOnly: true }订阅(App.tsx:2290)——本 PR 的核心接线——没有被任何测试锁定:未来若有改动去掉该选项,顶层渲染路径会重新订阅每一次流式尾部追加——正是本 PR 要修复的卡顿——而整个测试套件仍全绿。在本提交上的 A/B 探针证实了这一点:去掉 App.tsx 中的{ structuralOnly: true }后,四个改动套件的 684 个测试在两臂上均全部通过(Tests 684 passed (684))。建议将各调用点传入的选项记录到 mock(如 push 进
testState.snapshotCallOptions,并在beforeEach中声明/重置),并在边界测试中分别断言 App 层调用带{ structuralOnly: true }、LiveMessageList调用不带(见上方英文部分的ts代码块)。— qwen3.8-max via Qwen Code /review (v0.22.0)