Repository navigation
fix(web): reuse timeline rows while thinking and plans stream - #24121
Batuhan1999 wants to merge 1 commit into
Conversation
ApprovabilityVerdict: Needs Human Approval Macroscope's review found this PR requires human approval — This production web optimization adds nontrivial incremental fast paths for streaming reasoning and proposed-plan updates across timeline-entry and row derivation. It automatically bypasses full rebuilding based on streamed data while preserving selected row identities, so the processing gate and its broader runtime implications warrant human review. Needs human approval because:
Notes:
You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
… identity loss These tests reproduce pingdotgg#24120. When a streaming reasoning or proposed_plan item's text changes, every other timeline entry and every row not holding that item should keep its identity, as streamed assistant text already does. On main they fail with Object.is identity assertions while deep parity with a full derivation still holds. Fixture and tests are pared down from Batuhan1999's PR pingdotgg#24121. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eline row Streamed reasoning and proposed-plan text fell out of the entry and row fast paths, so every coalesced update re-derived the whole timeline. Both projections now patch only the entry and row holding the streamed item, and fall back to full derivation whenever blankness, streaming state, or row placement could change. Ports the source changes of pingdotgg#24121 onto current main and adds a fallback parity test across blank and completion transitions. Refs pingdotgg#24120 Co-Authored-By: Batuhan1999 <Batuhan.Ckr@outlook.de> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
juliusmarminge
left a comment
There was a problem hiding this comment.
Thanks for the careful work and evidence. Main now re-derives changed work entries in place and retains the other entries via #26727 (29f33af). Please rebase and trim the entry-reuse work already covered there, keeping the changes still needed for streamed plan artifacts and incremental reasoning/plan rows; main still falls back to full row derivation for those updates.
…eline row pingdotgg#26727 re-derives a changed reasoning or plan entry in place, but two paths still rebuild everything while those stream: - Each proposed-plan delta rebuilds the plan artifacts, so the entry reuse check (shallow plans) fails. Entries read only each plan's id, kind and status, so compare those instead. - Row derivation only reused rows for assistant message text. Patch the live work row holding streamed thinking, and the proposed-plan row, in place. Settled groups, folded turns and toggles summarize thinking text, so they keep the full rebuild. isStreamingTurnItemTextUpdate now accepts reasoning and proposed-plan items for the row check; entry reuse keeps patching only assistant text and leaves the rest to the in-place re-derive. Fixes pingdotgg#24120 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
db84725 to
2e547ee
Compare
|
Thanks! Rebased onto main as one commit and removed the entry-level reasoning/plan reuse that #26727 covers. What remains:
Entry reuse now patches only assistant text and leaves reasoning and plan items to the in-place re-derive. With main's code, 5 of the PR's tests fail (plan entries plus the reasoning/plan row cases) and the reasoning-entry test passes. On the head all 155 pass and |
Problem
Streaming thinking and proposed-plan text still force full web timeline row derivation on each text update. In a real Codex Plan-mode turn with prior history, repeated updates to one proposed-plan item re-entered full row derivation roughly every 400–600 ms.
#26727 now re-derives a changed reasoning or plan entry in place and keeps the other entries. Two paths it does not cover still rebuild everything:
plansarray, so the entry reuse check (shallow(input.plans, …)) fails and every entry is re-derived.Change
Rebased onto main and trimmed to what #26727 leaves open:
samePlanStatuses) instead of the array.replaceStreamingMessageRowsnow also patches the live work row holding a streamed thought, and the proposed-plan row, in place. Settled groups, folded turns and work toggles derive labels from thinking text, so they keep the full rebuild, as do blankness, completion and metadata changes.isStreamingTurnItemTextUpdateaccepts streaming reasoning and proposed-plan items for that row check. Entry reuse keeps patching only assistant text and leaves reasoning and plan items to perf(web): long-thread timeline rebuilds 45 → 9 ms, tool updates 12 → 0.3 ms #26727's in-place re-derive.The entry-level reuse this PR used to add for reasoning and plans is removed; #26727 covers it.
Scope and approval
Fixes #24120
Web and desktop share these projections; native mobile does not use them. No wire or persisted-state changes.
Verification
Head 2e547ee, one commit on current main.
session-logic.tsandMessagesTimeline.logic.ts, 5 tests fail and 150 pass:keeps every other entry when streaming proposed_plan text changes,keeps every row not holding streaming reasoning/proposed_plan text when it changes, and the twomatches a full derivation when streaming thinking sits in …cases (expanded live group, superseded by a running tool). The reasoning-entry test passes on main, which confirms perf(web): long-thread timeline rebuilds 45 → 9 ms, tool updates 12 → 0.3 ms #26727 covers that part.vp test run src/session-logic.test.ts src/components/chat/MessagesTimeline.logic.test.ts(fromapps/web). They cover equivalence with full derivation, blank-text transitions, completion, changing Codex plan artifacts, and expanded and superseded work groups.node scripts/check-changed.ts: format, lint, knip, typecheck, tests and react compiler all pass.Implemented with Claude Opus 5.5 via Claude Code (running in T3 Code).
🤖 Generated with Claude Code