Repository navigation
fix(chat): avoid dirty placement IDs for equal height reports - #33662
Conversation
Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-d43daca7ac1e4cde2f3d
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: green on the base 280f8ab; do not merge until green; see lane-rules Shipped in:
|
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude opus-5.5 medium): SHIP at 4ce856a
Visual verdict: unchanged by this diff.
Checked flushDrawn, the other setDrawn writers and the measured consumer. setDrawn still runs on every flush, so a lower-priority height that is still queued can still be corrected by an equal one. The id is now added inside the updater, against the state React rebases onto. Adding to the set is safe to repeat, the Minimal Mode writer already does the same, and measured reads the set after the updaters run in the same render, so no changed row is missed. The new test runs the production callback under React's real priority queues, and the naive guard mutation turns it red.
Follow-ups:
- [follow-up] The test cuts the callback out of
App.tsxon comment and declaration text (/// RowFrame redraws,const observer = useMemo(), which breaks if either is reworded. An exported pure updater would be sturdier.
Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-d43daca7ac1e4cde2f3d
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude opus-5.5 medium): SHIP at d879536
Visual verdict: unchanged by this diff.
Carried from the SHIP at 4ce856a. The delta to this head is formatting only (drawn-height-priority.test.tsx, +5/-17, wrapped lines joined). The cut markers and the expected values are unchanged, and App.tsx is untouched.
Follow-ups:
- [follow-up] The test cuts the callback out of
App.tsxby matching source text (#33667).
|
Found 7 test failures on Blacksmith runners: Failures
|
Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-d43daca7ac1e4cde2f3d
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude opus-5.5 medium): SHIP at 62e8a7a
Visual verdict: unchanged by this diff.
Delta from d879536 is test-only: the test builds its hook with node:vm compileFunction and passes in only useCallback, instead of new Function, which clears no-implied-eval. The code it takes from App.tsx (sameDrawn, changesDrawn, flushDrawn) uses no other hook, so dropping useRef and useLayoutEffect is safe. The cut markers, the naive-guard mutation and the assertions are unchanged, and so is the product diff.
Follow-ups:
- [follow-up] The test cuts the callback out of
App.tsxby matching source text (#33667).
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Equal DOM height confirmations currently mark rows dirty even when the state updater keeps their existing measurement. This makes later placement consider unchanged rows. Mark only entries that differ from the state React applies the updater to.
Keep scheduling equal corrective reports: a lower-priority height of 200 can still be queued while the rendered height is 100. Dropping a new report of 100 against the rendered value loses that correction. The regression reproduces this with React's real priority queues; enabling the naive guard makes the final height incorrectly become 200. Row pruning and Minimal Mode also write this state, so this change deliberately leaves scheduling intact.
Leo: “i also think we've done a lot of great work wrt perf and loading but i don't think we've pushed to the limit yet. things should be instant and not shift layouts AND not take many resources”.
The change stays inside
flushDrawn's equality/dirty-marking lines. #33624 (80865735, mounted measurement batching) and #33629 (9e3e8347, first motion) were inspected: neither edits that body, so this is based independently on5dee1ab988e. No new loop, observer, timer or cache. Settled transcript geometry anddesign/chat-transcript.mdremain unchanged. The overbroad dirty marking originated in #19648.Performance
Same production-callback React harness, 120 equal-height confirmation ticks. These are bookkeeping counts, not browser CPU or frame-time measurements.
webviews/test/drawn-height-priority.test.tsxThe idle CPU increase tracked by #33368 remains unresolved; this PR makes no claim about it.
Validation: focused single-file unit test: 3 passed, 9 assertions. Base: equal/mixed dirty-ID cases fail. Naive rendered-value guard: priority regression fails (200 instead of 100). Fleet replay, compile check and the finished webview verdict will be recorded before landing. Risk: changed IDs must survive React's updater rebasing; correction scheduling is preserved and tested.
🤖 Generated with Codex
Agent session: agent session
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes equal-height transcript confirmations marking rows dirty, so placement no longer reprocesses unchanged rows (120 dirty IDs per run drop to 0).
The equality check now runs inside React's state updater against the rebased state rather than the last committed measurement, so an equal report can still supersede a lower-priority height queued in another lane; scheduling is deliberately preserved.
Adds a test harness that compiles the production
flushDrawncallback through node vm, covering idle equal confirmations, mixed batches, and the lower-priority correction regression.Performance
Same production-callback React harness, 120 equal-height confirmation ticks. These are bookkeeping counts, not browser CPU or frame-time measurements.
Validation: focused single-file unit test, 3 passed and 9 assertions.
Written for commit 62e8a7a. Summary will update on new commits.