EPIC 7: retained sub-display-lists via per-instance render packets#220
Merged
RtlZeroMemory merged 6 commits intomainfrom Feb 26, 2026
Merged
EPIC 7: retained sub-display-lists via per-instance render packets#220RtlZeroMemory merged 6 commits intomainfrom
RtlZeroMemory merged 6 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds a render-packet system (types, recorder, key computation, emission), threads renderPacketKey/renderPacket through RuntimeInstance and commit, integrates packet reuse into renderTree for selected widgets, refines damage-tracking to optionally avoid marking selfDirty, and updates tests and drawlist utilities. Changes
Sequence DiagramsequenceDiagram
participant RT as renderTree
participant Node as RuntimeInstance
participant RPR as RenderPacketRecorder
participant DLB as DrawlistBuilder
rect rgba(100,200,100,0.5)
Note over RT: Render widget (sparkline / barChart / miniChart)
RT->>RT: computeRenderPacketKey(node, theme, style, layout, state)
RT->>Node: read node.renderPacketKey & node.renderPacket
alt cache hit (key matches & packet exists)
RT->>DLB: emitRenderPacket(packet, originX, originY)
DLB->>DLB: replay recorded ops -> draw
else cache miss or invalid
RT->>RPR: new RenderPacketRecorder(target=builder, origin)
RT->>RPR: render widget into recorder (record ops/resources)
RPR->>RPR: buildPacket() -> packet
RPR-->>RT: packet
RT->>Node: store renderPacket & renderPacketKey
RPR->>DLB: emit recorded ops -> draw
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/app/widgetRenderer/damageTracking.ts (1)
256-258: Consider renamingmarkedSelfDirtyfor clarity.The variable name
markedSelfDirtyis now slightly misleading since it represents whether the predicate considers the node dirty, not whetherselfDirtywas actually set (which now depends on themarkSelfDirtyparameter). A name likepredicateDirtyorisDirtyByPredicatewould better reflect its purpose.♻️ Suggested rename for clarity
- const markedSelfDirty = isNodeDirty(node); - if (markSelfDirty && markedSelfDirty) node.selfDirty = true; - let dirty = node.dirty || markedSelfDirty; + const predicateDirty = isNodeDirty(node); + if (markSelfDirty && predicateDirty) node.selfDirty = true; + let dirty = node.dirty || predicateDirty;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer/damageTracking.ts` around lines 256 - 258, Rename the local variable markedSelfDirty to a clearer name (e.g., predicateDirty or isDirtyByPredicate) to reflect that it holds the result of the isNodeDirty(node) predicate rather than whether node.selfDirty was set; update its occurrences in the same block (where it is assigned from isNodeDirty(node), used in the conditional that sets node.selfDirty conditioned on markSelfDirty, and when computing let dirty = node.dirty || markedSelfDirty) so the logic remains the same but the variable name matches its meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/app/widgetRenderer/damageTracking.ts`:
- Around line 256-258: Rename the local variable markedSelfDirty to a clearer
name (e.g., predicateDirty or isDirtyByPredicate) to reflect that it holds the
result of the isNodeDirty(node) predicate rather than whether node.selfDirty was
set; update its occurrences in the same block (where it is assigned from
isNodeDirty(node), used in the conditional that sets node.selfDirty conditioned
on markSelfDirty, and when computing let dirty = node.dirty || markedSelfDirty)
so the logic remains the same but the variable name matches its meaning.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/core/src/app/widgetRenderer/damageTracking.tspackages/core/src/layout/__tests__/incrementalLayout.phase3.test.tspackages/core/src/layout/__tests__/layout.perf-invariants.test.tspackages/core/src/layout/__tests__/layout.stability-coverage.test.tspackages/core/src/layout/__tests__/layout.stability-signature.test.tspackages/core/src/renderer/__tests__/persistentBlobKeys.test.tspackages/core/src/renderer/__tests__/renderPackets.test.tspackages/core/src/renderer/renderToDrawlist/renderPackets.tspackages/core/src/renderer/renderToDrawlist/renderTree.tspackages/core/src/renderer/renderToDrawlist/widgets/containers.tspackages/core/src/runtime/commit.tspackages/core/src/runtime/renderPacket.ts
💤 Files with no reviewable changes (1)
- packages/core/src/renderer/renderToDrawlist/widgets/containers.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a2c2623a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
Addressed review feedback in commit
Validation re-run:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/renderer/renderToDrawlist/renderTree.ts (1)
296-362: Please attach full-suite verification evidence for this renderer hot path.This path changes packet reuse/rebuild behavior in
renderTree(), so I’d like explicitnode scripts/run-tests.mjsevidence (or a clear baseline of pre-existing failures vs new failures) in the PR thread before merge.As per coding guidelines: “After any code change to commit, reconcile, layout, or renderer files, run the full test suite with
node scripts/run-tests.mjsbefore committing—subtle regressions are common.”
Based on learnings: “Changes to drawlist generation code are a danger zone requiring extra care and full test suite verification.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/renderToDrawlist/renderTree.ts` around lines 296 - 362, Run the full test suite and attach verification evidence for the changed renderer hot path: execute node scripts/run-tests.mjs and include the complete test output (or a clear before/after baseline) in the PR thread to show there are no regressions caused by the packet reuse/rebuild changes around renderTree(), computeRenderPacketKey(), RenderPacketRecorder, renderBasicWidget(), and the node.renderPacket/node.renderPacketKey logic; ensure the report highlights any failing tests, stack traces, and timestamps so reviewers can confirm the drawlist generation changes are safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/renderer/renderToDrawlist/renderPackets.ts`:
- Around line 389-391: The reset() method currently only calls
this.target.reset() but must also clear the recorder-local packet state to avoid
stale data when reusing the recorder; update renderPackets' reset() to also
reset/clear the recorder's ops, resources, maps, and valid state (the fields
used by buildPacket()) so that ops/resources/maps/valid are emptied/reset
alongside this.target; locate the reset() method and add explicit clears for
those recorder-local members referenced by buildPacket() so subsequent
buildPacket() calls start from a clean state.
---
Nitpick comments:
In `@packages/core/src/renderer/renderToDrawlist/renderTree.ts`:
- Around line 296-362: Run the full test suite and attach verification evidence
for the changed renderer hot path: execute node scripts/run-tests.mjs and
include the complete test output (or a clear before/after baseline) in the PR
thread to show there are no regressions caused by the packet reuse/rebuild
changes around renderTree(), computeRenderPacketKey(), RenderPacketRecorder,
renderBasicWidget(), and the node.renderPacket/node.renderPacketKey logic;
ensure the report highlights any failing tests, stack traces, and timestamps so
reviewers can confirm the drawlist generation changes are safe.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/app/widgetRenderer/damageTracking.tspackages/core/src/renderer/renderToDrawlist/renderPackets.tspackages/core/src/renderer/renderToDrawlist/renderTree.tspackages/core/src/renderer/renderToDrawlist/widgets/containers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/renderer/renderToDrawlist/widgets/containers.ts
- packages/core/src/app/widgetRenderer/damageTracking.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/renderer/renderToDrawlist/renderPackets.ts (1)
296-305: Minor inconsistency:addTextRunBlobdoesn't store inresourcesarray.Unlike
addBlobwhich stores bytes in bothblobResourceByIdandresources,addTextRunBlobonly stores intextRunByBlobId. This is intentional since text run segments are stored directly in the op rather than as a resource blob, but this asymmetry could be confusing during future maintenance.Consider adding a brief comment explaining why text runs are handled differently from binary blobs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/renderToDrawlist/renderPackets.ts` around lines 296 - 305, The addTextRunBlob method stores copied text segments only in textRunByBlobId (unlike addBlob which also records bytes in blobResourceById and resources), which can confuse future readers; add a concise inline comment inside addTextRunBlob explaining that text run segments are embedded directly in ops (so they are not stored as binary resources) and reference the contrast with addBlob/blobResourceById/resources to make the asymmetry explicit for maintainers.packages/core/src/renderer/renderToDrawlist/renderTree.ts (1)
311-311: Optional: Consider poolingRenderPacketRecorderinstances.A new
RenderPacketRecorderis allocated for each widget that needs packet recording. While this only occurs when content changes (selfDirty), high-frequency updates (e.g., spinners, progress bars) could benefit from recorder pooling. This is a minor optimization opportunity for a future iteration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/renderToDrawlist/renderTree.ts` at line 311, The code creates a new RenderPacketRecorder per widget (const recorder = new RenderPacketRecorder(builder, rect.x, rect.y)), which can be pooled to reduce allocations for high-frequency updates; implement a simple pooling utility (e.g., RenderPacketRecorderPool with acquire(builder, x, y) and release(recorder)) and replace direct construction in renderTree (where recorder is created) with pool.acquire(...) and ensure recorder.reset/clear is called on release so state isn't leaked, and call pool.release(recorder) after use.packages/core/src/renderer/__tests__/renderPackets.test.ts (1)
90-127: Consider adding a helper comment for the long parameter list.The
renderTreecall has manyundefinedparameters which is acceptable for test scaffolding but could be confusing for future maintainers. Consider adding a brief comment listing which parameters are being intentionally omitted, or extracting default test options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/renderPackets.test.ts` around lines 90 - 127, The renderTree call in renderScene passes many trailing undefineds which is confusing; update renderScene to either add a concise inline comment next to the renderTree invocation listing which parameters are intentionally omitted (by name, matching renderTree’s signature) or extract a small test helper/wrapper (e.g., renderTreeForTest or defaultRenderTreeArgs) that supplies the common default/undefined values so the call becomes renderTree(builder, ..., defaultRenderTreeArgs); reference renderScene and renderTree when making the change so future readers can see which parameters are intentionally left unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/renderer/__tests__/renderPackets.test.ts`:
- Around line 90-127: The renderTree call in renderScene passes many trailing
undefineds which is confusing; update renderScene to either add a concise inline
comment next to the renderTree invocation listing which parameters are
intentionally omitted (by name, matching renderTree’s signature) or extract a
small test helper/wrapper (e.g., renderTreeForTest or defaultRenderTreeArgs)
that supplies the common default/undefined values so the call becomes
renderTree(builder, ..., defaultRenderTreeArgs); reference renderScene and
renderTree when making the change so future readers can see which parameters are
intentionally left unset.
In `@packages/core/src/renderer/renderToDrawlist/renderPackets.ts`:
- Around line 296-305: The addTextRunBlob method stores copied text segments
only in textRunByBlobId (unlike addBlob which also records bytes in
blobResourceById and resources), which can confuse future readers; add a concise
inline comment inside addTextRunBlob explaining that text run segments are
embedded directly in ops (so they are not stored as binary resources) and
reference the contrast with addBlob/blobResourceById/resources to make the
asymmetry explicit for maintainers.
In `@packages/core/src/renderer/renderToDrawlist/renderTree.ts`:
- Line 311: The code creates a new RenderPacketRecorder per widget (const
recorder = new RenderPacketRecorder(builder, rect.x, rect.y)), which can be
pooled to reduce allocations for high-frequency updates; implement a simple
pooling utility (e.g., RenderPacketRecorderPool with acquire(builder, x, y) and
release(recorder)) and replace direct construction in renderTree (where recorder
is created) with pool.acquire(...) and ensure recorder.reset/clear is called on
release so state isn't leaked, and call pool.release(recorder) after use.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/app/widgetRenderer/damageTracking.tspackages/core/src/renderer/__tests__/renderPackets.test.tspackages/core/src/renderer/renderToDrawlist/renderPackets.tspackages/core/src/renderer/renderToDrawlist/renderTree.tspackages/core/src/runtime/commit.ts
This was referenced Feb 26, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
RenderPacketmodel for local-coordinate draw ops (FILL_RECT,DRAW_TEXT_SLICE,DRAW_TEXT_RUN,PUSH_CLIP,POP_CLIP,DRAW_CANVAS,DRAW_IMAGE) plus packet resourcesRuntimeInstancewithrenderPacketKeyandrenderPacketand preserve packet state across reusable instance commitsrenderTree()for cacheable basic widgets so layout-only movement can re-emit retained ops at a new originselfDirtyduring render-time descendant propagation and mark layout-only dirtiness asdirtywithoutselfDirtyto unlock packet reuse on motionValidation
npm run build✅node --test --test-concurrency=1 packages/core/dist/renderer/__tests__/renderPackets.test.js packages/core/dist/__tests__/integration/integration.reflow.test.js packages/core/dist/__tests__/integration/integration.resize.test.js✅npm test❌ (pre-existing failures inpackages/nodeworker integration tests in this environment)node --test --test-concurrency=1 packages/core/dist/renderer/__tests__/render.golden.test.js❌ with 6 pre-existing golden mismatches; reproduced on a pristineorigin/mainworktree (/tmp/rezi-baseline-epic7)Summary by CodeRabbit
New Features
Performance
Bug Fixes
Tests