EPIC 9B: plumb BLIT_RECT and optimize logs scroll rendering#221
Merged
RtlZeroMemory merged 4 commits intomainfrom Feb 26, 2026
Merged
EPIC 9B: plumb BLIT_RECT and optimize logs scroll rendering#221RtlZeroMemory merged 4 commits intomainfrom
RtlZeroMemory merged 4 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a new blitRect draw command to the DrawlistBuilder API, implements emit/handler paths, updates renderer and renderTree to support damageRect-aware logs console incremental rendering, and updates tests/benchmarks to exercise scroll-blit behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant WR as WidgetRenderer
participant Planner as LogsScrollPlanner
participant Damage as DamageTracker
participant Builder as DrawlistBuilder
App->>WR: commit(updates)
WR->>Planner: buildLogsScrollBlitPlans()
Planner->>Damage: inject scroll blit damage rects
WR->>Damage: track incremental damage
WR->>Builder: incremental render start
Builder->>Builder: clipDamageRectsWithoutMerge()
Builder->>Builder: blitRect(...) // scroll blit ops injected
Builder->>Builder: drawImage()/other ops
WR->>Planner: snapshotLogsScrollState()
WR->>App: return frame
sequenceDiagram
participant Editor as renderEditorWidget
participant Console as Logs Console
participant Damage as damageRect
participant Builder as DrawlistBuilder
Editor->>Console: receive damageRect
Console->>Damage: compute visible row range
alt damageRect present
Console->>Builder: blitRect() for scrolled region
Console->>Builder: drawImage() for glyphs in damageRect
else
Console->>Builder: drawImage() for full content
end
Builder->>Builder: encode commands
Editor->>Builder: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/native/vendor/zireael/src/unicode/zr_grapheme.h (1)
24-25: LGTM! Formatting-only change in vendored file.The whitespace normalization in the struct field declarations is cosmetic and does not affect functionality or the API surface.
Since this is vendored code, consider verifying that the formatting matches the upstream commit (a5d99908b59efe680f97480c4bc0be31d4c3ae22) to ensure consistency with the vendor source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/native/vendor/zireael/src/unicode/zr_grapheme.h` around lines 24 - 25, This change only normalizes whitespace in the vendored struct fields (symbols: len and off) and should be left as-is only if it exactly matches upstream; verify the formatting in zr_grapheme.h matches upstream commit a5d99908b59efe680f97480c4bc0be31d4c3ae22 and, if it does not, revert or adjust the whitespace to match that upstream version so the vendored file remains identical to upstream.packages/native/vendor/zireael/src/core/zr_framebuffer.c (1)
228-235: Minor redundancy:zr_fb_links_reset(dst)is called twice.The explicit
zr_fb_links_reset(dst)at line 229 is redundant becausezr_fb_links_clone_from(dst, src)(line 231) already callszr_fb_links_reset(dst)unconditionally as its first operation (see line 186).Since this is vendored code and the overhead is negligible (two O(1) length resets), this may be intentional for clarity or defensive coding. No action required, just noting for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/native/vendor/zireael/src/core/zr_framebuffer.c` around lines 228 - 235, Remove the redundant zr_fb_links_reset(dst) call before calling zr_fb_links_clone_from(dst, src); zr_fb_links_clone_from already unconditionally resets the destination as its first step, so keep the single reset inside zr_fb_links_clone_from and delete the explicit zr_fb_links_reset(dst) invocation to avoid duplication while preserving behavior.
🤖 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/__tests__/renderer.scrollBlit.benchmark.test.ts`:
- Around line 253-257: The strict assertion comparing partial.timePerFrameMs and
full.timePerFrameMs is flaky; relax it by introducing a small tolerance and
assert that partial.timePerFrameMs is sufficiently less than full.timePerFrameMs
(e.g., partial.timePerFrameMs <= full.timePerFrameMs + margin or
partial.timePerFrameMs < full.timePerFrameMs * (1 - relativeTol)), and include
scenario.name in the error message; update the assertion around
partial.timePerFrameMs / full.timePerFrameMs in
renderer.scrollBlit.benchmark.test.ts to use a margin/relative tolerance
variable (epsilon/margin) instead of a bare `<` comparison.
In `@packages/native/vendor/zireael/include/zr/zr_drawlist.h`:
- Line 2: Update the protocol docs to remove V2 support: edit
docs/protocol/zrdl.md to stop listing "ZRDL v1/v2", remove the V2-specific
opcode documentation (e.g., BLIT_RECT) and any examples or tables that mention
v2; edit docs/protocol/versioning.md to delete or change the
ZR_DRAWLIST_VERSION_V2 symbol and any language saying the engine "accepts v1/v2"
so it only documents v1 compatibility and the current accepted version; ensure
any cross-references or version compatibility rules are adjusted to reflect V2
removal.
---
Nitpick comments:
In `@packages/native/vendor/zireael/src/core/zr_framebuffer.c`:
- Around line 228-235: Remove the redundant zr_fb_links_reset(dst) call before
calling zr_fb_links_clone_from(dst, src); zr_fb_links_clone_from already
unconditionally resets the destination as its first step, so keep the single
reset inside zr_fb_links_clone_from and delete the explicit
zr_fb_links_reset(dst) invocation to avoid duplication while preserving
behavior.
In `@packages/native/vendor/zireael/src/unicode/zr_grapheme.h`:
- Around line 24-25: This change only normalizes whitespace in the vendored
struct fields (symbols: len and off) and should be left as-is only if it exactly
matches upstream; verify the formatting in zr_grapheme.h matches upstream commit
a5d99908b59efe680f97480c4bc0be31d4c3ae22 and, if it does not, revert or adjust
the whitespace to match that upstream version so the vendored file remains
identical to upstream.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
packages/core/src/__tests__/stress/stress.large-trees.test.tspackages/core/src/app/__tests__/partialDrawlistEmission.test.tspackages/core/src/app/__tests__/rawRender.test.tspackages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/app/widgetRenderer.tspackages/core/src/drawlist/builder.tspackages/core/src/drawlist/builderBase.tspackages/core/src/drawlist/types.tspackages/core/src/renderer/__tests__/focusIndicators.test.tspackages/core/src/renderer/__tests__/persistentBlobKeys.test.tspackages/core/src/renderer/__tests__/recipeRendering.test-utils.tspackages/core/src/renderer/__tests__/renderer.border.test.tspackages/core/src/renderer/__tests__/renderer.clip.test.tspackages/core/src/renderer/__tests__/renderer.damage.test.tspackages/core/src/renderer/__tests__/renderer.partial.perf.test.tspackages/core/src/renderer/__tests__/renderer.partial.test.tspackages/core/src/renderer/__tests__/renderer.scrollBlit.benchmark.test.tspackages/core/src/renderer/__tests__/renderer.scrollbar.test.tspackages/core/src/renderer/renderToDrawlist/renderTree.tspackages/core/src/renderer/renderToDrawlist/widgets/editors.tspackages/core/src/testing/renderer.tspackages/core/src/theme/__tests__/theme.switch.test.tspackages/core/src/ui/__tests__/themed.test.tspackages/native/vendor/VENDOR_COMMIT.txtpackages/native/vendor/zireael/include/zr/zr_drawlist.hpackages/native/vendor/zireael/include/zr/zr_metrics.hpackages/native/vendor/zireael/include/zr/zr_version.hpackages/native/vendor/zireael/src/core/zr_config.cpackages/native/vendor/zireael/src/core/zr_cursor.hpackages/native/vendor/zireael/src/core/zr_damage.hpackages/native/vendor/zireael/src/core/zr_drawlist.cpackages/native/vendor/zireael/src/core/zr_engine_present.incpackages/native/vendor/zireael/src/core/zr_event_pack.cpackages/native/vendor/zireael/src/core/zr_event_pack.hpackages/native/vendor/zireael/src/core/zr_framebuffer.cpackages/native/vendor/zireael/src/core/zr_metrics.cpackages/native/vendor/zireael/src/core/zr_placeholder.cpackages/native/vendor/zireael/src/platform/win32/zr_win32_conpty_test.cpackages/native/vendor/zireael/src/platform/win32/zr_win32_conpty_test.hpackages/native/vendor/zireael/src/unicode/zr_grapheme.hpackages/native/vendor/zireael/src/unicode/zr_unicode_data.hpackages/native/vendor/zireael/src/unicode/zr_unicode_pins.hpackages/native/vendor/zireael/src/unicode/zr_utf8.hpackages/native/vendor/zireael/src/unicode/zr_width.hpackages/native/vendor/zireael/src/unicode/zr_wrap.hpackages/native/vendor/zireael/src/util/zr_arena.hpackages/native/vendor/zireael/src/util/zr_ring.hpackages/native/vendor/zireael/src/util/zr_string_builder.hpackages/native/vendor/zireael/src/util/zr_string_view.hpackages/native/vendor/zireael/src/util/zr_vec.h
💤 Files with no reviewable changes (3)
- packages/native/vendor/zireael/src/unicode/zr_utf8.h
- packages/native/vendor/zireael/src/unicode/zr_unicode_pins.h
- packages/native/vendor/zireael/src/util/zr_string_view.h
packages/core/src/renderer/__tests__/renderer.scrollBlit.benchmark.test.ts
Show resolved
Hide resolved
![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: 0f2929a1c1
ℹ️ 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".
RtlZeroMemory
force-pushed
the
epic9b-blit-rect
branch
from
0f2929a to
d01f334
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/renderer/renderToDrawlist/widgets/editors.ts (1)
849-860:⚠️ Potential issue | 🟠 MajorText rendering can corrupt the scrollbar column in damage-only updates.
Line 849 clips text drawing to
contentRect, and Line 932 computes text width fromcontentRect.w. WhenhasScrollbaris true and scrollbar rows are skipped for this damage pass, text writes can land in the scrollbar column and leave stale visuals. Clip/render text againsttextRectinstead.Proposed fix
- builder.pushClip(contentRect.x, contentRect.y, contentRect.w, contentRect.h); - const startIndex = Math.max(0, props.scrollTop); const endIndex = Math.min(filtered.length, startIndex + contentRect.h); const hasScrollbar = contentRect.h > 0 && filtered.length > contentRect.h; const textRect: Rect = { x: contentRect.x, y: contentRect.y, w: hasScrollbar ? clampNonNegative(contentRect.w - 1) : contentRect.w, h: contentRect.h, }; + builder.pushClip(textRect.x, textRect.y, textRect.w, textRect.h); const shouldRenderTextRows = !damageRect || rectIntersects(damageRect, textRect); @@ - const remaining = contentRect.w - (x - contentRect.x); + const remaining = textRect.w - (x - textRect.x);Also applies to: 932-942, 948-986
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/renderToDrawlist/widgets/editors.ts` around lines 849 - 860, The text rendering currently clips to contentRect and computes widths from contentRect.w which allows glyphs to paint into the scrollbar column during damage-only passes; change the clipping and rendering to use the computed textRect instead: replace builder.pushClip(contentRect.x, contentRect.y, contentRect.w, contentRect.h) with a clip using textRect (x,y,w,h) and ensure all text measurement and draw calls that reference contentRect.w or contentRect are switched to use textRect (and respect hasScrollbar) so text never writes into the scrollbar column during damage-only updates; update any places that compute text width or call the text draw routine to read from textRect.w/h and use textRect for clipping.
🧹 Nitpick comments (1)
packages/core/src/renderer/__tests__/renderer.scrollBlit.benchmark.test.ts (1)
77-101: Binary parsing assumes fixed layout — add defensive checks or comments.
parseDrawlistStatsreads specific offsets (16, 20, 24) from the drawlist header. If the protocol format changes, this will silently produce incorrect results.Add version/magic validation or document assumptions
function parseDrawlistStats(drawlist: Uint8Array): Readonly<{ cmdCount: number; blitCount: number }> { if (drawlist.byteLength < HEADER_SIZE) { return Object.freeze({ cmdCount: 0, blitCount: 0 }); } const dv = new DataView(drawlist.buffer, drawlist.byteOffset, drawlist.byteLength); + // Header layout (v1 protocol): + // offset 16: cmdOffset (u32) + // offset 20: cmdBytes (u32) + // offset 24: cmdCount (u32) const cmdOffset = dv.getUint32(16, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/renderer.scrollBlit.benchmark.test.ts` around lines 77 - 101, parseDrawlistStats currently assumes a fixed header layout and blindly reads offsets 16, 20, 24 which can silently break if the protocol changes; update the function to validate the header before parsing by checking for a magic/version field (or explicit minimum HEADER_SIZE) at the start, ensure the DataView reads for cmdOffset/cmdBytes/cmdCount are within bounds (e.g., header region) and fail fast if magic/version mismatches, out-of-range offsets are detected, or HEADER_SIZE is too small, and add a short comment documenting the expected header layout and meaning of OP_BLIT_RECT so future changes must update these checks (refer to parseDrawlistStats, HEADER_SIZE, OP_BLIT_RECT and the offsets 16/20/24).
🤖 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/app/widgetRenderer.ts`:
- Around line 2423-2428: buildLogsScrollBlitPlans uses rects from rectById and
calls builder.blitRect(...) without checking ancestor clipping; update
buildLogsScrollBlitPlans (and the other similar sites around the mentions) to
compute the visible intersection between the geometry.contentRect returned by
resolveLogsConsoleRenderGeometry(...) and the current clip stack (or ancestor
clip rect) before emitting builder.blitRect: if the intersection has
non-positive width/height skip emitting the blit, otherwise pass the
intersection (or wrap the blit with clip begin/end) so blitRect never copies
pixels outside the widget’s visible region. Ensure the same guard is applied at
the other occurrences you noted (the blocks around lines shown) to use
contentRect intersected with ancestor clip.
In `@packages/core/src/renderer/__tests__/renderer.scrollBlit.benchmark.test.ts`:
- Around line 12-13: Replace the hardcoded HEADER_SIZE = 64 with an import of
HEADER_SIZE from the canonical builderBase export while leaving OP_BLIT_RECT =
14 as-is (it cannot be imported from generated writers); update the top of the
test file to import HEADER_SIZE from the builderBase module and use that
constant in place of the literal so the test stays synchronized with the
canonical definition, keeping OP_BLIT_RECT unchanged.
---
Outside diff comments:
In `@packages/core/src/renderer/renderToDrawlist/widgets/editors.ts`:
- Around line 849-860: The text rendering currently clips to contentRect and
computes widths from contentRect.w which allows glyphs to paint into the
scrollbar column during damage-only passes; change the clipping and rendering to
use the computed textRect instead: replace builder.pushClip(contentRect.x,
contentRect.y, contentRect.w, contentRect.h) with a clip using textRect
(x,y,w,h) and ensure all text measurement and draw calls that reference
contentRect.w or contentRect are switched to use textRect (and respect
hasScrollbar) so text never writes into the scrollbar column during damage-only
updates; update any places that compute text width or call the text draw routine
to read from textRect.w/h and use textRect for clipping.
---
Nitpick comments:
In `@packages/core/src/renderer/__tests__/renderer.scrollBlit.benchmark.test.ts`:
- Around line 77-101: parseDrawlistStats currently assumes a fixed header layout
and blindly reads offsets 16, 20, 24 which can silently break if the protocol
changes; update the function to validate the header before parsing by checking
for a magic/version field (or explicit minimum HEADER_SIZE) at the start, ensure
the DataView reads for cmdOffset/cmdBytes/cmdCount are within bounds (e.g.,
header region) and fail fast if magic/version mismatches, out-of-range offsets
are detected, or HEADER_SIZE is too small, and add a short comment documenting
the expected header layout and meaning of OP_BLIT_RECT so future changes must
update these checks (refer to parseDrawlistStats, HEADER_SIZE, OP_BLIT_RECT and
the offsets 16/20/24).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
packages/core/src/__tests__/stress/stress.large-trees.test.tspackages/core/src/app/__tests__/partialDrawlistEmission.test.tspackages/core/src/app/__tests__/rawRender.test.tspackages/core/src/app/__tests__/widgetRenderer.integration.test.tspackages/core/src/app/widgetRenderer.tspackages/core/src/drawlist/builder.tspackages/core/src/drawlist/builderBase.tspackages/core/src/drawlist/types.tspackages/core/src/renderer/__tests__/focusIndicators.test.tspackages/core/src/renderer/__tests__/persistentBlobKeys.test.tspackages/core/src/renderer/__tests__/recipeRendering.test-utils.tspackages/core/src/renderer/__tests__/renderPackets.test.tspackages/core/src/renderer/__tests__/renderer.border.test.tspackages/core/src/renderer/__tests__/renderer.clip.test.tspackages/core/src/renderer/__tests__/renderer.damage.test.tspackages/core/src/renderer/__tests__/renderer.partial.perf.test.tspackages/core/src/renderer/__tests__/renderer.partial.test.tspackages/core/src/renderer/__tests__/renderer.scrollBlit.benchmark.test.tspackages/core/src/renderer/__tests__/renderer.scrollbar.test.tspackages/core/src/renderer/renderToDrawlist/renderPackets.tspackages/core/src/renderer/renderToDrawlist/renderTree.tspackages/core/src/renderer/renderToDrawlist/widgets/editors.tspackages/core/src/testing/renderer.tspackages/core/src/theme/__tests__/theme.switch.test.tspackages/core/src/ui/__tests__/themed.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/core/src/renderer/tests/renderer.damage.test.ts
- packages/core/src/renderer/renderToDrawlist/renderTree.ts
- packages/core/src/renderer/tests/renderer.partial.perf.test.ts
- packages/core/src/app/tests/rawRender.test.ts
- packages/core/src/renderer/tests/recipeRendering.test-utils.ts
- packages/core/src/theme/tests/theme.switch.test.ts
- packages/core/src/tests/stress/stress.large-trees.test.ts
- packages/core/src/renderer/tests/renderer.border.test.ts
packages/core/src/renderer/__tests__/renderer.scrollBlit.benchmark.test.ts
Outdated
Show resolved
Hide resolved
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
This PR implements EPIC 9B end-to-end support for
BLIT_RECTand wires it through Rezi's drawlist pipeline, then applies it tologsConsolescroll rendering to preserve the scrolled region and redraw only exposed strips.1) Vendor bump (Zireael)
include/andsrc/intopackages/native/vendor/zireael.packages/native/vendor/VENDOR_COMMIT.txt->9e9ea6f54e3b83b60d6457b06646d5c488b94156(v1.3.11)@rezi-ui/native).2) Drawlist API + writer plumbing
builder.blitRect(srcX, srcY, w, h, dstX, dstY)3) Renderer integration (real widget)
WidgetRenderer:blitRectfor preserved region.4) Tests
Added/updated tests for golden framebuffer equivalence and blit behavior:
renderer.partial.test.tsrenderer.scrollBlit.benchmark.test.tsUpdated all
DrawlistBuildertest doubles/mocks to implement newblitRectAPI.Updated upstream
renderPacketsbuilder/test implementations to satisfy the new DrawlistBuilder interface after rebasing onto latestmain.Perf Results (logsConsole scroll benchmark)
Measured with benchmark harness logic (viewport 120x40, entries 2400):
BLIT_RECTops emitted in partial mode across these scenarios.Validation Run
npx tsc -b packages/testkit/tsconfig.json --pretty false && npx tsc -b packages/core/tsconfig.json --pretty falsenpm -w @rezi-ui/native run build:nativenode --test --test-concurrency=1 packages/core/dist/renderer/__tests__/renderer.partial.test.js packages/core/dist/renderer/__tests__/renderer.scrollBlit.benchmark.test.jsAll above passed.
Summary by CodeRabbit
New Features
Performance
Tests & Benchmarks