Surface widget failures where developers look - #43
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change expands CLI validation and diagnostics, adds transactional rendering and structured runtime failure handling, improves host and reload resilience, and adds tests for budgets, canvas constraints, callback failures, image handling, and recovery behavior. ChangesCLI validation and development diagnostics
Runtime rendering and callback failures
Host and reload resilience
Project guidance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Widget as Widget callback
participant Reconciler
participant NativeBridge
participant Tree
Widget->>Reconciler: throw callback error
Reconciler->>NativeBridge: reportError(scope, details)
Reconciler->>NativeBridge: abortBatch()
NativeBridge->>Tree: restore transaction snapshot
Tree-->>NativeBridge: restored committed tree
NativeBridge-->>Reconciler: failure surface state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR makes widget failures transactional and surfaces actionable diagnostics across development, validation, runtime, and host boundaries.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| sdk/src/reconciler.ts | Makes render generations transactional and applies named failure boundaries to effects, cleanup, events, timers, canvas work, storage, and provider callbacks. |
| runtime/src/tree.zig | Adds batch rollback, explicit budget diagnostics, canvas ancestry enforcement, and deterministic runtime error surfaces. |
| runtime/src/js_engine.zig | Tracks unhandled QuickJS promise rejections and propagates render failure state through runtime execution. |
| runtime/src/main.zig | Expands runtime error propagation, image validation, renderer lifecycle handling, and developer-facing diagnostics. |
| cli/src/index.ts | Adds recursive development watching, stale and presentation-health reporting, status reconciliation warnings, and broader static widget validation. |
| host/src/supervisor.zig | Improves shared-renderer launch, exit, and status-publication failure handling. |
| host/src/macos_host.zig | Hardens macOS callback and renderer failure handling. |
| host/src/windows_host.zig | Hardens Windows callback and renderer failure handling. |
| runtime/src/widget_log.zig | Improves widget-log durability, truncation safety, and fallback diagnostics. |
| sdk/test/reconciler.test.mjs | Extends coverage for transactional aborts, callback scopes, error reporting, and hot-swap state transfer. |
Sequence Diagram
sequenceDiagram
participant Widget
participant SDK
participant Runtime
participant Host
participant Developer
Widget->>SDK: Render generation
SDK->>Runtime: beginBatch
SDK->>Runtime: Apply tree operations
alt Generation succeeds
SDK->>Runtime: endBatch
Runtime->>Host: Present committed frame
else Generation or callback fails
SDK->>Runtime: abortBatch and reportError
Runtime->>Runtime: Preserve last committed tree
Runtime->>Host: Display deterministic error surface
Runtime-->>Developer: Emit named message and stack
end
Developer->>Runtime: Submit hot-swap candidate
alt First render succeeds
Runtime->>Host: Activate candidate
else First render fails
Runtime-->>Developer: Report failed hot swap
Runtime->>Host: Keep previous bundle running
end
Reviews (5): Last reviewed commit: "Document Weaver's performance and design..." | Re-trigger Greptile
|
Addressed the CI release-audit ratchet in 395a262; the native dependency SHA is now both pinned and audited.\n\n@greptileai |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
host/src/windows_host.zig (1)
767-783: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExpose renderer failure even when a GPU sidecar already exists.
A widget that previously wrote
gpuretains that sidecar after the shared renderer exits, so this condition leaves its status reason empty despiterenderer_failure_reasonbeing active. Apply the shared failure to running GPU widgets with no widget-specific reason regardless ofbackend.Proposed fix
- else if (slot.wants_gpu and backend.len == 1 and backend[0] == '-' and self.renderer_failure_reason_len > 0) + else if (slot.platform.process != null and slot.wants_gpu and self.renderer_failure_reason_len > 0) self.renderer_failure_reason[0..self.renderer_failure_reason_len]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@host/src/windows_host.zig` around lines 767 - 783, Update the reason selection in the entries construction to use renderer_failure_reason for running GPU widgets without a widget-specific slot reason, regardless of the backend value. Remove the backend.len/backend[0] == '-' restriction from the fallback while preserving slot.reason() precedence and the existing empty-string fallback.runtime/src/widget_log.zig (2)
66-91: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
write_failednever clears, so a transient log write failure permanently replaces the widget UI.A momentarily full disk or a locked file latches
write_failedfor the process lifetime, andview()shows theWidgetLogUnavailablepanel forever even after writes resume. Every other degraded path added in this PR latches and recovers (dispatchProviderFramesandnoteProjectionFailureinruntime/src/main.zig). Clear the flag on a successful write for consistency.🔧 Proposed fix
fn writeLine(line: []const u8) void { - writeLineFallible(line) catch |err| noteFailure(err); + writeLineFallible(line) catch |err| return noteFailure(err); + if (write_failed.swap(false, .acq_rel)) { + fallback_reported.store(false, .release); + std.debug.print("weaver widget log recovered; path={s}\n", .{path_buffer[0..path_len]}); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/widget_log.zig` around lines 66 - 91, Update the successful-write path in writeLine or writeLineFallible to clear the write_failed flag after writeLineFallible completes without error. Preserve noteFailure(err) for failed writes, and ensure subsequent successful writes allow view() to stop showing WidgetLogUnavailable.
14-26: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEager
createFilefailure bypasses the newWidgetLogUnavailablesurface.
initnow hard-fails oncreateFile, andmain()calls it withtry(Line 1198 ofruntime/src/main.zig), so the exact conditions this PR addedfailed()for — unwritable log directory, no disk space — now kill the process before the window exists, instead of rendering theWidgetLogUnavailablepanel added inview(). Route the eager-create failure throughnoteFailureand let startup continue.🔧 Proposed fix
- var file = try std.Io.Dir.cwd().createFile(runtime_io, path, .{ .read = true, .truncate = false }); - file.close(runtime_io); write_failed.store(false, .release); fallback_reported.store(false, .release); + if (std.Io.Dir.cwd().createFile(runtime_io, path, .{ .read = true, .truncate = false })) |file| { + var opened = file; + opened.close(runtime_io); + } else |err| { + noteFailure(err); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/widget_log.zig` around lines 14 - 26, Update init to handle createFile failures through noteFailure instead of propagating the error, allowing startup to continue and failed() to expose WidgetLogUnavailable in view(). Preserve successful file creation and cleanup behavior, and keep initialization of the path, I/O, and failure state intact.
🧹 Nitpick comments (6)
runtime/src/geometry.zig (1)
37-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood error-propagation change; consider covering it with a
Store.loadtest.Distinguishing
FileNotFound(→null) from parse failures (→error.InvalidGeometryRecord) while propagating other I/O errors is a sound improvement, and matches howmain.zigalready logs and degrades gracefully on anyloaderror. However, the existing tests only exerciseparse/formatdirectly (lines 87-92); there's no test hittingStore.loaditself with a malformed on-disk record to confirm it now surfacesInvalidGeometryRecordrather than silently returningnull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/geometry.zig` around lines 37 - 44, Add a test for Store.load that writes a malformed geometry record to a temporary path, invokes load, and asserts it returns error.InvalidGeometryRecord rather than null. Keep the existing parse and format tests unchanged, and use the established Store construction and test temporary-resource cleanup patterns.runtime/src/tree.zig (1)
277-304: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIterate nodes by pointer to avoid copying every
Node.
for (self.nodes)copies eachNode(inline text/source/font buffers) per iteration.canvasAncestorViolationruns on everyendBatch, so this is measurable for free savings.♻️ Suggested change
- for (self.nodes) |node_value| if (node_value.alive) { + for (&self.nodes) |*node_value| if (node_value.alive) { count += 1; }; @@ - for (self.nodes, 0..) |node_value, index| { + for (&self.nodes, 0..) |*node_value, index| {As per coding guidelines, "Optimize for very low memory and CPU usage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/tree.zig` around lines 277 - 304, Update nodeCount and canvasAncestorViolation to iterate self.nodes by pointer (using mutable or const pointers as appropriate) instead of copying each Node value, while preserving the existing alive, kind, and ancestor checks and returned results.Source: Coding guidelines
cli/src/index.ts (1)
483-499: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueStop the presentation watcher once it has reported.
After
presentationFailureReportedis set, the interval keeps waking every second for the rest of the dev session only to return at Line 484. Clearing it on the reporting paths keeps dev idle cost at zero.♻️ Clear the interval after the terminal report
presentationFailureReported = true; const state = status ? `${status.state}${status.reason ? `: ${status.reason}` : ""}` : "absent from host status"; process.stderr.write(`weaver dev ERROR: "${project.config.name}" has not presented a frame after 10 seconds (${state}); check weaver logs ${JSON.stringify(project.config.name)}\n`); + clearInterval(presentationWatch); } catch (error) { presentationFailureReported = true; process.stderr.write(`weaver dev ERROR: presentation health could not be read after 10 seconds: ${errorMessage(error)}\n`); + clearInterval(presentationWatch); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/index.ts` around lines 483 - 499, Update the presentationWatch callback so it calls clearInterval(presentationWatch) immediately after setting presentationFailureReported and emitting each terminal failure report, covering both the missing/non-presented status path and the readStatus catch path. Preserve the existing early return for healthy presentation and ensure the watcher stops after reporting.runtime/src/main.zig (2)
1455-1501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHand-rolled PNG/GIF/BMP/JPEG header parsing just to build an error label.
The offsets and bounds here check out, but this is ~50 lines of bespoke format sniffing that duplicates knowledge the decoder already had when it returned
ImageTooLarge. Every added format or malformed-header edge case becomes Weaver's problem for a cosmetic string. Prefer surfacing the dimensions from the decoder/SDK error path (or the manifest/weaver checkvalidation, which per the PR already validates decoded RGBA sizes) and drop this parser.As per coding guidelines: "Prefer obvious, straightforward solutions over clever implementations; actively suggest simpler or more obvious alternatives when appropriate."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/main.zig` around lines 1455 - 1501, Remove the bespoke encoded-image header parser, including encodedImageDimensions and its format-specific logic. Instead, obtain the actual image dimensions from the decoder/SDK error path or the existing manifest/weaver check validation when constructing the ImageTooLarge error label, while preserving the current validation behavior and avoiding duplicate format parsing.Source: Coding guidelines
1418-1449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the image-budget strings from named constants.
max_image_rgba_bytes=262144is duplicated, andasked for 17should come frommax_images + 1instead of hardcoded values so the failure text stays aligned with the runtime limits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/main.zig` around lines 1418 - 1449, Update the ImageTooLarge and ImageRegistryFull handling around the failure-label construction to derive all limit text from the existing named runtime constants. Replace duplicated 262144 values with the image RGBA-byte limit constant, and calculate the ImageRegistryFull requested count as max_images + 1 instead of hardcoding 17; use these derived values consistently in labels and log messages.runtime/src/js_engine.zig (1)
315-325: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the unnecessary
JS_DupValuepairJS_ToCStringLen2only reads the value, sorenderedcan be passed directly and the extra refcount churn dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/js_engine.zig` around lines 315 - 325, Update valueDetails to pass value directly to JS_ToCStringLen2, removing the rendered duplicate and its corresponding JS_FreeValue cleanup while preserving the existing formatting, copying, and error-handling behavior.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/index.ts`:
- Around line 1631-1637: Update the unknown-bundled-font diagnostic in the
class-attribute handling branch to pass the current visited sourceFile to
locationMessage instead of project.sourceFile, while preserving the existing
classAttribute location and message behavior.
In `@runtime/src/bridge.zig`:
- Around line 268-288: Update endBatch so canvasAncestorViolation() is evaluated
only on the outermost batch commit, after the batch depth is decremented by the
appropriate endBatch flow. Preserve the existing violation-specific fail
messages, and avoid rejecting transient nested-batch state before the batch
fully closes.
In `@runtime/src/js_engine.zig`:
- Around line 288-300: Update the truncation logic in the unhandled rejection
display around valueDetails and tree.showError so byte limits are rounded down
to a valid UTF-8 codepoint boundary before slicing or copying. Ensure both the
details truncation and first_line extraction never pass partial UTF-8 sequences
to the renderer, while preserving the existing 150-byte limit and fallback
behavior.
In `@runtime/src/main.zig`:
- Around line 1384-1396: Update buildNodeForTest to accept a caller-owned Model
pointer instead of constructing a local Model from retained_tree and fonts, then
pass that model to buildNode. Update every caller to construct one persistent
Model before invoking buildNodeForTest, using the existing retained tree and
fonts storage, so the returned WidgetUi.Node borrows from storage that remains
alive through finalize and avoids copying the large Tree onto the stack.
- Around line 123-137: Move startup image registration out of initEffects and
invoke it after loadLocalImages and seedImageStates complete, so image states
exist before registerImageBytes and failure handling run. Preserve the existing
registration, recordImageFailure, registered, and clearImageFailure behavior
while changing only the initialization order.
In `@runtime/src/tree.zig`:
- Around line 230-238: Update Tree batch snapshot handling in beginBatch and its
corresponding endBatch cleanup to reuse a parent-owned snapshot allocated during
initialization, copying the current tree into that existing storage for each
outermost batch instead of calling allocator.create/destroy per render. Preserve
the existing batch_depth and rollback behavior while eliminating per-batch
allocation and deallocation churn.
In `@runtime/src/widget_log.zig`:
- Around line 34-43: Update the log-writing flow around writer.print(format,
args) so fixed-buffer exhaustion truncates the current message and still writes
the buffered content, rather than calling noteFailure. Preserve noteFailure for
genuine write failures from operations such as writeTimestamp, prefix writes,
and the final output write, and ensure oversized lines do not latch write_failed
or replace the widget.
---
Outside diff comments:
In `@host/src/windows_host.zig`:
- Around line 767-783: Update the reason selection in the entries construction
to use renderer_failure_reason for running GPU widgets without a widget-specific
slot reason, regardless of the backend value. Remove the backend.len/backend[0]
== '-' restriction from the fallback while preserving slot.reason() precedence
and the existing empty-string fallback.
In `@runtime/src/widget_log.zig`:
- Around line 66-91: Update the successful-write path in writeLine or
writeLineFallible to clear the write_failed flag after writeLineFallible
completes without error. Preserve noteFailure(err) for failed writes, and ensure
subsequent successful writes allow view() to stop showing WidgetLogUnavailable.
- Around line 14-26: Update init to handle createFile failures through
noteFailure instead of propagating the error, allowing startup to continue and
failed() to expose WidgetLogUnavailable in view(). Preserve successful file
creation and cleanup behavior, and keep initialization of the path, I/O, and
failure state intact.
---
Nitpick comments:
In `@cli/src/index.ts`:
- Around line 483-499: Update the presentationWatch callback so it calls
clearInterval(presentationWatch) immediately after setting
presentationFailureReported and emitting each terminal failure report, covering
both the missing/non-presented status path and the readStatus catch path.
Preserve the existing early return for healthy presentation and ensure the
watcher stops after reporting.
In `@runtime/src/geometry.zig`:
- Around line 37-44: Add a test for Store.load that writes a malformed geometry
record to a temporary path, invokes load, and asserts it returns
error.InvalidGeometryRecord rather than null. Keep the existing parse and format
tests unchanged, and use the established Store construction and test
temporary-resource cleanup patterns.
In `@runtime/src/js_engine.zig`:
- Around line 315-325: Update valueDetails to pass value directly to
JS_ToCStringLen2, removing the rendered duplicate and its corresponding
JS_FreeValue cleanup while preserving the existing formatting, copying, and
error-handling behavior.
In `@runtime/src/main.zig`:
- Around line 1455-1501: Remove the bespoke encoded-image header parser,
including encodedImageDimensions and its format-specific logic. Instead, obtain
the actual image dimensions from the decoder/SDK error path or the existing
manifest/weaver check validation when constructing the ImageTooLarge error
label, while preserving the current validation behavior and avoiding duplicate
format parsing.
- Around line 1418-1449: Update the ImageTooLarge and ImageRegistryFull handling
around the failure-label construction to derive all limit text from the existing
named runtime constants. Replace duplicated 262144 values with the image
RGBA-byte limit constant, and calculate the ImageRegistryFull requested count as
max_images + 1 instead of hardcoding 17; use these derived values consistently
in labels and log messages.
In `@runtime/src/tree.zig`:
- Around line 277-304: Update nodeCount and canvasAncestorViolation to iterate
self.nodes by pointer (using mutable or const pointers as appropriate) instead
of copying each Node value, while preserving the existing alive, kind, and
ancestor checks and returned results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce22106d-56fc-4635-89bb-ad17c346588a
📒 Files selected for processing (25)
cli/src/host-tools.tscli/src/index.tscli/src/origin.tscli/test/canvas-size.test.mjscli/test/image-budget.test.mjscli/test/lowered-budget.test.mjshost/src/macos_host.zighost/src/supervisor.zighost/src/windows_host.zigruntime/native-sdkruntime/src/bridge.zigruntime/src/dev_reload.zigruntime/src/geometry.zigruntime/src/image_paths.zigruntime/src/js_engine.zigruntime/src/main.zigruntime/src/network.zigruntime/src/provider_macos.zigruntime/src/tree.zigruntime/src/widget_log.zigsdk/src/class-compiler.tssdk/src/native.d.tssdk/src/reconciler.tssdk/test/reconciler.test.mjstest/all.test.mjs
| fn initEffects(model: *Model, effects: *Effects) void { | ||
| for (model.images[0..model.image_count]) |image| { | ||
| _ = effects.registerImageBytes(image.id, image.bytes) catch |err| { | ||
| std.log.err("widget image {d} failed to decode/register: {s}", .{ image.id, @errorName(err) }); | ||
| if (findImageState(model, @intCast(image.id))) |state| { | ||
| recordImageFailure(state, err, image.bytes, "initial image decode/register"); | ||
| } else { | ||
| std.log.err("initial image decode/register failed: image={d}, cause={s}", .{ image.id, @errorName(err) }); | ||
| } | ||
| continue; | ||
| }; | ||
| if (findImageState(model, @intCast(image.id))) |state| state.registered = true; | ||
| if (findImageState(model, @intCast(image.id))) |state| { | ||
| state.registered = true; | ||
| clearImageFailure(state); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where init_fx is invoked in the Native SDK app wrapper.
rg -nP --type=zig -C4 '\binit_fx\b'Repository: SunkenInTime/weaver
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== main.zig references ==\n'
rg -n --hidden --glob 'runtime/src/main.zig' 'initEffects|init_fx|seedImageStates|loadLocalImages|synchronizeImages|findImageState|registered|recordImageFailure|clearImageFailure' runtime/src/main.zig
printf '\n== main.zig around relevant lines ==\n'
sed -n '100,170p' runtime/src/main.zig
printf '\n== create / startup flow ==\n'
sed -n '1260,1365p' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 10612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate WidgetApp.create and init_fx wiring ==\n'
rg -n --hidden --glob 'runtime/src/main.zig' 'WidgetApp\.create|init_fx\s*=|init_fx\b' runtime/src/main.zig
printf '\n== locate synchronizeImages and image seeding ==\n'
rg -n --hidden --glob 'runtime/src/main.zig' 'synchronizeImages|seedImageStates|loadLocalImages' runtime/src/main.zig
printf '\n== locate initEffects call sites ==\n'
rg -n --hidden --glob 'runtime/src/main.zig' '\binitEffects\s*\(' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 900
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== runtime/src/main.zig outline ==\n'
ast-grep outline runtime/src/main.zig --view expanded | sed -n '1,220p'
printf '\n== exact references ==\n'
rg -n --hidden 'initEffects|WidgetApp\.create|loadLocalImages|seedImageStates|synchronizeImages|findImageState|initFx|init_fx' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 1311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search entire repo for init_fx / initFx ==\n'
rg -n --hidden '\binit[_A]?fx\b|\binitFx\b' .
printf '\n== search entire repo for WidgetApp.create wiring ==\n'
rg -n --hidden 'WidgetApp\.create|create\(' runtime src . | sed -n '1,200p'Repository: SunkenInTime/weaver
Length of output: 10022
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1140,1188p' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 2355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1068,1125p' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 2626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== synchronizeImages ==\n'
sed -n '1149,1198p' runtime/src/main.zig
printf '\n== image state mutation helpers ==\n'
sed -n '1068,1125p' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 5066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Model definition and defaults ==\n'
sed -n '1,120p' runtime/src/main.zig
printf '\n== loadLocalImages and seedImageStates ==\n'
sed -n '977,1038p' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 8019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== image_tree_generation and image_epoch writes ==\n'
rg -n --hidden 'image_tree_generation|image_epoch|registered = true|clearImageFailure|recordImageFailure|seedImageStates|synchronizeImages|synchronizeImageNode|removeImageState|addImageState' runtime/src/main.zig
printf '\n== around any image generation writes ==\n'
sed -n '1038,1168p' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 8146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '360,405p' runtime/src/main.zigRepository: SunkenInTime/weaver
Length of output: 2321
Move startup image registration after seeding init_fx runs before loadLocalImages/seedImageStates, and seedImageStates marks the tree as current, so the initial registerImageBytes/failure-recording branch in initEffects never handles startup images.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@runtime/src/main.zig` around lines 123 - 137, Move startup image registration
out of initEffects and invoke it after loadLocalImages and seedImageStates
complete, so image states exist before registerImageBytes and failure handling
run. Preserve the existing registration, recordImageFailure, registered, and
clearImageFailure behavior while changing only the initialization order.
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cli/src/index.ts (2)
454-463: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate
activeManifestonly after reload succeeds.
activeManifestis assigned beforesignalHost/signalDevReloadcompletes. If signaling fails, the next rebuild can treat the manifest as unchanged and take the in-place reload path, leaving host configuration stale.Proposed fix
if (configChanged) { - activeManifest = next.manifest; signalHost("--signal-reload"); + activeManifest = next.manifest; } else { await signalDevReload(directory); + activeManifest = next.manifest; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/index.ts` around lines 454 - 463, Move the activeManifest assignment in the rebuild flow to occur only after both signalHost and signalDevReload complete successfully. Preserve the existing failure handling so a signaling error leaves activeManifest pointing to the last successfully applied manifest, forcing the next rebuild to use the correct reload path.
1659-1671: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winEnforce the image-size limit before reading the file.
readFileSync(canonical)loads the entire asset before checking the 1 MiB limit, allowing an arbitrarily large local file to consume excessive memory duringweaver check. CheckstatSync(canonical).sizefirst or read onlyMAX_IMAGE_STREAM_BYTES + 1bytes.As per coding guidelines, optimize for very low memory and CPU usage.
Proposed fix
+ const encodedSize = statSync(canonical).size; + if (encodedSize > 1024 * 1024) { + return `ImageStreamTooLarge: ${JSON.stringify(source)} is ${encodedSize} encoded bytes; max_image_stream_bytes=1048576`; + } bytes = readFileSync(canonical); - if (bytes.length > 1024 * 1024) { - return `ImageStreamTooLarge: ...`; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/index.ts` around lines 1659 - 1671, The asset validation flow should enforce the 1 MiB limit before loading file contents. In the block containing canonical path resolution and readFileSync, inspect statSync(canonical).size first and return the existing ImageStreamTooLarge error when it exceeds MAX_IMAGE_STREAM_BYTES; otherwise read the file, preserving the current unreadable and root-boundary handling.Source: Coding guidelines
runtime/src/main.zig (1)
1076-1120: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
constforstateherestateis only used to mutate its pointee, so this binding should beconst;vartriggers Zig’s “local variable is never mutated” error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/main.zig` around lines 1076 - 1120, Change the `state` binding in `synchronizeImageNode` from `var` to `const`, preserving all mutations to the pointed-to image state through the existing pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cli/src/index.ts`:
- Around line 454-463: Move the activeManifest assignment in the rebuild flow to
occur only after both signalHost and signalDevReload complete successfully.
Preserve the existing failure handling so a signaling error leaves
activeManifest pointing to the last successfully applied manifest, forcing the
next rebuild to use the correct reload path.
- Around line 1659-1671: The asset validation flow should enforce the 1 MiB
limit before loading file contents. In the block containing canonical path
resolution and readFileSync, inspect statSync(canonical).size first and return
the existing ImageStreamTooLarge error when it exceeds MAX_IMAGE_STREAM_BYTES;
otherwise read the file, preserving the current unreadable and root-boundary
handling.
In `@runtime/src/main.zig`:
- Around line 1076-1120: Change the `state` binding in `synchronizeImageNode`
from `var` to `const`, preserving all mutations to the pointed-to image state
through the existing pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 946feed5-828a-4af5-8bc2-cbcdbbebf195
📒 Files selected for processing (8)
cli/src/index.tshost/src/windows_host.zigruntime/src/bridge.zigruntime/src/geometry.zigruntime/src/js_engine.zigruntime/src/main.zigruntime/src/tree.zigruntime/src/widget_log.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- runtime/src/js_engine.zig
|
@coderabbitai review |
✅ Action performedReview finished.
|
- Expand agent guidance with project context and shared terminology - Clarify measurement-backed limits, visible failures, and obvious solutions
Native SDK branch macos-memory-shared-renderer-prep (600d6cf6) carries the autorelease-pool, analytic-rounded-clip, and Metal tiled-image memory work; the tiled-image change still needs live verification. The briefs scope the two follow-up passes: the error-propagation seams (partially landed via #43) and the receipt sweep over every numeric limit. The shared-renderer experiment plan lives in the handoff doc accompanying this work.
1 participant
What changed
This is the repository-wide error-propagation pass for every place a widget failure can be born, swallowed, or presented.
max_nodes=128, asked for 129.weaver checkvalidation for retained-tree budgets, decoded-RGBA image size, and canvas clipping/opacity ancestry.Why
The highest-impact reproduction exceeded the retained node budget, passed
weaver check, produced no useful log, committed a half-built tree, and displayed uninitialized GPU memory. The bridge already generated many useful errors, but asynchronous SDK scheduling, generic process exits, stale status surfaces, and log-only fallbacks discarded their meaning before it reached the developer.The governing rule is: a failure must surface where the developer is looking—
weaver check, the dev stream, the per-widget log, or the widget itself—and it must name the cause or budget.Developer impact
The canonical over-budget widget now:
weaver checkwith the budget, limit, ask, and headroom;node capacity exhausted: max_nodes=128, asked for 129with a stack;Hot-swap failures leave the old bundle running. Dead platform callbacks close their windows instead of retaining stale surface contents.
Native dependency
Depends on SunkenInTime/native#17 for the three-platform callback-name and stale-window fix. The submodule pointer is pinned to that PR's commit.
Validation
npm test— 76/76 passednpm run typecheck— passedzig build testinruntime— passedzig build testinhost— passedgit diff --check— clean in both repositoriesThe standalone native test suite reported 1,821 passed / 17 skipped. Its overall build remains environment-limited on this Windows checkout by missing WebView2 loader DLL fixtures and an unavailable runtime-core connection check.
Summary by CodeRabbit
weaver devwith stale rebuild tracking, periodic reminders, recovery (“caught up”) messaging, recursive widget-directory watching, and “presentation health” frame checks.weaver checkwith broader widget-budget limits, improved canvas ancestor validation (clip vs opacity), and stricter local image asset/budget validation with clearer naming.