feat: move assets, rewrite logic from rust to zig #11
Conversation
WalkthroughThis change replaces the Rust-based WebAssembly module and its integration with a new implementation in Zig. All Rust source files, build scripts, and related dependencies are removed. New Zig modules are introduced for texture, line, and interaction logic. The build system and loader are updated to support Zig, and TypeScript interfaces are provided for Zig modules. Integration and test code are refactored to use the new Zig-based API. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ZigModule
participant WebGPU
participant Loader
Browser->>Loader: Import Zig module via loader
Loader->>ZigModule: Compile & instantiate Zig/WASM
Browser->>ZigModule: init_state(width, height)
Browser->>ZigModule: add_texture(id, points, texture_index)
Browser->>ZigModule: connectWebGPUPrograms({draw_texture, ...})
Browser->>ZigModule: on_pointer_move(x, y)
ZigModule->>WebGPU: Call draw_texture/draw_triangle via function pointers
Browser->>ZigModule: canvas_render()
ZigModule->>WebGPU: Render all textures and borders
Browser->>ZigModule: picks_render()
ZigModule->>WebGPU: Render picking textures
Browser->>ZigModule: on_pointer_click(), on_pointer_down(), etc.
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code Graph Analysis (1)src/WebGPU/pointer.ts (1)
🔇 Additional comments (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
src/logic/texture.zig (1)
42-60:⚠️ Potential issueCritical: Memory leak in pick data allocation.
Same memory leak issue as
get_vertex_data. The allocated memory is never freed.Additionally, fix the incorrect comment on line 55:
-vertex_data[i * 7 + 6] = @floatFromInt(self.id); // v-coordinate +vertex_data[i * 7 + 6] = @floatFromInt(self.id); // texture ID for picking
🧹 Nitpick comments (10)
README.md (1)
7-10: Fix bare URL formatting for better markdown compliance.The URL should be properly formatted as a markdown link to improve readability and comply with markdown best practices.
Apply this diff to format the URL properly:
-# Generating WebGPU Binding Groups Layout and analyzing memory of structs - -https://webgpufundamentals.org/webgpu/lessons/resources/wgsl-offset-computer.html# +# Generating WebGPU Binding Groups Layout and analyzing memory of structs + +[WebGPU WGSL Offset Computer](https://webgpufundamentals.org/webgpu/lessons/resources/wgsl-offset-computer.html#)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Bare URL used
null(MD034, no-bare-urls)
integration-tests/index.ts (1)
4-4: Consider starting asset ID from a more conventional value.Starting the asset ID at 1000 seems arbitrary. Consider whether starting from 1 or 0 would be more appropriate for the test scenarios, unless there's a specific reason to use 1000 (e.g., avoiding conflicts with existing assets).
If there's no specific requirement for starting at 1000, consider:
-let new_asset_id = 1000 +let new_asset_id = 1src/index.ts (1)
46-48: Consider fixing typo in constant name.The constant
ASSET_ID_TRESHOLDhas a typo - should beASSET_ID_THRESHOLD.If this constant is defined in the Zig module, consider renaming it there as well to maintain consistency.
webpack.config.js (1)
53-56: Remove commented code instead of leaving it in the codebase.Since you're migrating from Rust to Zig, the commented WASM inline rule is no longer needed and should be removed entirely for cleaner code.
- // { - // test: /\.wasm$/, - // type: "asset/inline", - // },integration-tests/tests/creator.spec.ts (1)
47-50: Verify the mouse drag sequence implementation.The drag sequence (down → move → up) is correct, but ensure that the Zig implementation properly handles these events in sequence. The hardcoded coordinates (300, 200) should ideally be calculated relative to the viewport dimensions for better maintainability.
- await page.mouse.move(300, 200) + await page.mouse.move(center.x - 200, center.y - 300)src/WebGPU/pointer.ts (2)
7-7: Remove the commented parameter from the function signature.Since the state management has moved to Zig, the commented
stateparameter should be removed entirely rather than left as a comment.-export default function initMouseController(canvas: HTMLCanvasElement, /*state: State*/) { +export default function initMouseController(canvas: HTMLCanvasElement) {
35-37: Unused event parameter in mouseup handler.The
mouseupevent handler receives an event parameterebut doesn't use it. This is correct based on the Zig function signature, but the parameter can be removed for clarity.- canvas.addEventListener('mouseup', e => { + canvas.addEventListener('mouseup', () => { on_pointer_up() })zigar-loader.cjs (1)
1-3: Fix typos in the comment.There are typos that should be corrected for better code documentation.
-// created in common JS because original zigar-laoder is in commonJS +// created in common JS because original zigar-loader is in commonJS // https://github.com/chung-leong/zigar/blob/main/zigar-loader/dist/index.js -// Do not rewrite it to anytign else, oterwise updatign this file with zigar-loader will be problematic +// Do not rewrite it to anything else, otherwise updating this file with zigar-loader will be problematicsrc/logic/texture.zig (1)
5-5: Fix typo in constant name.The constant name has a typo.
-const SHADER_TRIANGLE_INDICIES = [_]usize{ +const SHADER_TRIANGLE_INDICES = [_]usize{Remember to update all references to this constant in lines 28 and 47.
src/logic/index.d.ts (1)
13-13: Fix typo in parameter name.The parameter name
textre_indexshould betexture_index.- export const add_texture: (id: number, points: PointUV[], textre_index: number) => void + export const add_texture: (id: number, points: PointUV[], texture_index: number) => void
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
crate/Cargo.lockis excluded by!**/*.lockintegration-tests/tests/creator.spec.ts-snapshots/after-hover-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/after-move-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/after-selection-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/after-upload-chromium.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
ADRs/replace Rust with Zig(1 hunks)README.md(1 hunks)build-wasm.mjs(0 hunks)crate/Cargo.toml(0 hunks)crate/LICENSE(0 hunks)crate/benches/bench.rs(0 hunks)crate/glue_code.js(0 hunks)crate/src/lib.rs(0 hunks)crate/src/line.rs(0 hunks)crate/src/texture.rs(0 hunks)crate/src/types.rs(0 hunks)integration-tests/index.ts(2 hunks)integration-tests/template.html(0 hunks)integration-tests/tests/creator.spec.ts(3 hunks)package.json(2 hunks)playwright.config.ts(2 hunks)src/WebGPU/pick.ts(4 hunks)src/WebGPU/pointer.ts(1 hunks)src/index.ts(3 hunks)src/logic/index.d.ts(1 hunks)src/logic/index.zig(1 hunks)src/logic/line.zig(1 hunks)src/logic/texture.zig(1 hunks)src/logic/types.zig(1 hunks)src/run.ts(1 hunks)webpack.config.js(2 hunks)zigar-loader.cjs(1 hunks)
💤 Files with no reviewable changes (10)
- crate/LICENSE
- integration-tests/template.html
- crate/benches/bench.rs
- build-wasm.mjs
- crate/src/line.rs
- crate/glue_code.js
- crate/src/types.rs
- crate/Cargo.toml
- crate/src/texture.rs
- crate/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/WebGPU/pointer.ts (1)
src/logic/index.d.ts (4)
on_pointer_move(20-20)on_pointer_click(17-17)on_pointer_down(18-18)on_pointer_up(19-19)
src/WebGPU/pick.ts (2)
src/WebGPU/pointer.ts (1)
pointer(4-4)src/logic/index.d.ts (1)
on_update_pick(16-16)
🪛 markdownlint-cli2 (0.17.2)
README.md
9-9: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (20)
playwright.config.ts (2)
3-29: Excellent documentation addition for Playwright usage.The comprehensive comment block provides clear guidance for various Playwright commands and use cases. This will help developers quickly understand how to run tests, update snapshots, and debug issues.
106-106: Good typo fix improves comment clarity.The correction from "liek" to "like" improves the readability of the comment.
package.json (2)
51-52: Dependency changes support Zig integration.The addition of
zigar-loaderand removal ofwasm-packcorrectly reflects the migration from Rust to Zig. The zigar-loader will handle Zig compilation in the webpack build process.
23-24:✅ Verification successful
Build script changes align with Rust to Zig migration.
The removal of the Rust WebAssembly build step (
node build-wasm.mjs) and the simplified start script are consistent with migrating from Rust to Zig. However, ensure that the build process still functions correctly without the WASM build step.Verify that the build process works correctly after removing the Rust WASM build step:
🏁 Script executed:
#!/bin/bash # Description: Verify build process functionality after Rust->Zig migration # Check if zigar-loader is properly configured in webpack echo "Checking webpack configuration for zigar-loader..." if [ -f "webpack.config.js" ]; then echo "Found webpack.config.js" rg -A 10 -B 5 "zigar-loader" webpack.config.js || echo "zigar-loader not found in webpack config" else echo "webpack.config.js not found" fi # Check if Zig files exist in expected locations echo "Checking for Zig source files..." fd -e zig . src/ # Verify zigar-loader.cjs exists echo "Checking for zigar-loader.cjs..." ls -la zigar-loader.cjs 2>/dev/null || echo "zigar-loader.cjs not found"Length of output: 969
Zigar-loader setup confirmed; build configuration is correct
Verified that:
- webpack.config.js includes the
.zigrule usingzigar-loader.cjs- Zig source files exist under
src/logic/*.zigzigar-loader.cjsis present at project rootAll necessary pieces for the Rust→Zig migration are in place. Build scripts align with these changes—no further adjustments required.
integration-tests/index.ts (2)
18-20: Dynamic asset ID generation improves test robustness.The switch from a fixed asset ID to an incrementing variable is a good improvement that prevents ID conflicts when multiple images are loaded during testing. The automatic increment ensures unique IDs for each asset.
1-25:✅ Verification successful
Verify that removed position update functionality is intentionally deprecated.
The removal of image position update logic suggests a change in the interaction model. Ensure this aligns with the new Zig-based implementation and that the functionality is now handled differently (e.g., direct pointer interaction).
Check if the removed update functionality has been replaced by direct interaction in the new Zig implementation:
🏁 Script executed:
#!/bin/bash # Description: Verify interaction model changes in the Zig implementation # Check for pointer/interaction handling in the new Zig logic echo "Checking Zig implementation for interaction handling..." fd -e zig . src/logic/ -x cat {} \; | rg -A 5 -B 5 "pointer|interaction|update" # Check if template.html still has update button echo "Checking integration test template for update button..." if [ -f "integration-tests/template.html" ]; then rg -A 3 -B 3 "button|update" integration-tests/template.html || echo "No update button found in template" fi # Check creator.spec.ts for interaction patterns echo "Checking test spec for interaction patterns..." if [ -f "integration-tests/tests/creator.spec.ts" ]; then rg -A 5 -B 5 "click|drag|pointer" integration-tests/tests/creator.spec.ts || echo "No interaction patterns found" fiLength of output: 3664
Image position updates are now handled via direct pointer events
Verified that the Zig implementation uses on_pointer_down/move/up and update_coords to drive asset movements, and the integration tests exercise mouse.down/move to validate drag-based updates. The deprecated manual update API has been intentionally removed in favor of this direct interaction model—no further changes needed.src/logic/types.zig (1)
1-11: LGTM! Clean and appropriate struct definitions.The Point and PointUV structs are well-defined for graphics use cases. Using f32 for coordinates is appropriate, and the public const declarations follow Zig conventions.
src/logic/line.zig (1)
12-15: Verify line extension logic.The line is being extended by
half_widthin both directions along the parallel axis. This creates a longer line than the distance between start and end points. Is this the intended behavior?src/index.ts (2)
7-7: LGTM! Clean integration with Zig module.The import and integration with the Zig module is well-implemented.
57-66: LGTM! Proper texture addition with Zig integration.The texture addition logic correctly integrates with the new Zig
add_texturefunction, maintaining the same coordinate calculation and texture indexing.webpack.config.js (1)
58-67: LGTM! The Zig loader configuration is well-structured.The loader is correctly configured with:
- Proper exclusion of node_modules
- Environment-based optimization levels (Debug for dev, ReleaseFast for prod)
- Conditional WASM embedding based on the build mode
integration-tests/tests/creator.spec.ts (1)
7-8: Good improvements to test reliability.Setting fixed viewport dimensions and using them consistently throughout the test ensures reproducible test results across different environments.
Also applies to: 25-25, 38-39
zigar-loader.cjs (1)
72-91: LGTM! Well-implemented WASM loading strategies.The dual loading approach correctly handles both browser (fetch) and Node.js (fs) environments, with proper async handling and module imports.
src/logic/texture.zig (1)
62-66: Clean implementation of coordinate update.Good use of Zig's iterator syntax for in-place updates.
src/run.ts (2)
11-18: Function signature properly updated for Zig migration.The removal of
stateandassetsListparameters aligns with the new Zig-based architecture where state is managed internally.
26-46:✅ Verification successful
Verify the vertex_data structure from Zig.
The implementation looks correct, but ensure that the
vertex_dataparameter from Zig has the expectedtypedArrayproperty.Also verify the TypeScript declarations match:
🏁 Script executed:
#!/bin/bash # Check how vertex data is passed from Zig to JS ast-grep --pattern 'draw_texture' src/logic/index.zigLength of output: 53
🏁 Script executed:
#!/bin/bash # Check the TypeScript declarations for these functions cat src/logic/index.d.ts | grep -A 3 -E "(draw_texture|draw_triangle|pick_texture)"Length of output: 380
I couldn’t locate the Zig-side definitions or the
ZigF32Arrayshape yet—let’s search for where that type is declared and wheredraw_texturelives in the Zig sources:
🏁 Script executed:
#!/bin/bash # Find the TS declaration of ZigF32Array to confirm it exposes `.typedArray` grep -R "ZigF32Array" -n src # List all Zig source files in the repo find . -type f -name "*.zig" # Search every Zig file for the draw_texture binding grep -R "draw_texture" -n .Length of output: 862
Verified
vertex_data.typedArrayExists and Matches ZigF32Array– In
src/logic/index.d.ts,type ZigF32Array = { typedArray: Float32Array }.
– Insrc/logic/index.zig, the importeddraw_texturesignature is*const fn([]const f32, u32) void, matching the unwrappedFloat32Array.No further changes required.
src/WebGPU/pick.ts (3)
51-56: Clarify if depth testing is needed for picking.The depth attachment is commented out. This could cause issues with overlapping objects where the wrong object might be picked.
Is depth testing intentionally disabled? If objects can overlap in your scene, you'll need depth testing to pick the frontmost object correctly.
86-102: Good refactoring of matrix creation.The method now properly encapsulates the pick matrix computation with cleaner parameters.
104-112: Clean async pick implementation.Good use of the
isPreviousPickDoneflag to prevent concurrent pick operations. The integration with Zig viaon_update_pickis well structured.src/logic/index.zig (1)
73-73:✅ Verification successful
Verify ASSET_ID_TRESHOLD constant usage.
The constant is compared with
>=which includes the threshold value itself. Ensure this is the intended behavior.
🏁 Script executed:
#!/bin/bash # Description: Check all usages of ASSET_ID_TRESHOLD to verify consistent comparison logic # Search for all occurrences of ASSET_ID_TRESHOLD rg -A 2 -B 2 "ASSET_ID_TRESHOLD"Length of output: 1482
ASSET_ID_TRESHOLD inclusive comparisons are correct.
Across both Zig and TypeScript,
>= ASSET_ID_TRESHOLDconsistently treats the threshold value itself as valid (TS throws only whenid < ASSET_ID_TRESHOLD). No change required.
| pub fn get_vertex_data(start: PointUV, end: PointUV, width: f32, color: [4]f32) []f32 { | ||
| const vertex_data = std.heap.page_allocator.alloc(f32, 6 * 8) catch unreachable; |
There was a problem hiding this comment.
Critical: Fix memory leak and dangerous error handling.
Two serious issues here:
- Memory leak: The allocated memory is never freed, causing a memory leak with each function call.
- Dangerous error handling: Using
catch unreachablewill panic the program if allocation fails.
Consider this approach to fix both issues:
- pub fn get_vertex_data(start: PointUV, end: PointUV, width: f32, color: [4]f32) []f32 {
- const vertex_data = std.heap.page_allocator.alloc(f32, 6 * 8) catch unreachable;
+ pub fn get_vertex_data(allocator: std.mem.Allocator, start: PointUV, end: PointUV, width: f32, color: [4]f32) ![]f32 {
+ const vertex_data = try allocator.alloc(f32, 6 * 8);This requires the caller to manage memory and properly handle allocation errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get_vertex_data(start: PointUV, end: PointUV, width: f32, color: [4]f32) []f32 { | |
| const vertex_data = std.heap.page_allocator.alloc(f32, 6 * 8) catch unreachable; | |
| pub fn get_vertex_data(allocator: std.mem.Allocator, start: PointUV, end: PointUV, width: f32, color: [4]f32) ![]f32 { | |
| const vertex_data = try allocator.alloc(f32, 6 * 8); |
🤖 Prompt for AI Agents
In src/logic/line.zig around lines 5 to 6, the function allocates memory with
std.heap.page_allocator.alloc but never frees it, causing a memory leak, and
uses catch unreachable which panics on allocation failure. To fix this, change
the function to return an error union that includes the allocated buffer, so the
caller can handle allocation failures properly and is responsible for freeing
the memory. Remove catch unreachable and propagate allocation errors instead.
| const points = [_]struct { x: f32, y: f32 }{ | ||
| .{ .x = ax - half_width * @cos(angle), .y = ay - half_width * @sin(angle) }, | ||
| .{ .x = ax + half_width * @cos(angle), .y = ay + half_width * @sin(angle) }, | ||
| .{ .x = bx + half_width * @cos(angle), .y = by + half_width * @sin(angle) }, | ||
| .{ .x = bx - half_width * @cos(angle), .y = by - half_width * @sin(angle) }, | ||
| .{ .x = ax - half_width * @cos(angle), .y = ay - half_width * @sin(angle) }, | ||
| .{ .x = bx + half_width * @cos(angle), .y = by + half_width * @sin(angle) }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for robustness.
Consider adding validation for edge cases that could cause mathematical issues:
+ // Validate inputs
+ if (width <= 0) return error.InvalidWidth;
+ if (start.x == end.x and start.y == end.y) return error.ZeroLengthLine;
+
const points = [_]struct { x: f32, y: f32 }{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const points = [_]struct { x: f32, y: f32 }{ | |
| .{ .x = ax - half_width * @cos(angle), .y = ay - half_width * @sin(angle) }, | |
| .{ .x = ax + half_width * @cos(angle), .y = ay + half_width * @sin(angle) }, | |
| .{ .x = bx + half_width * @cos(angle), .y = by + half_width * @sin(angle) }, | |
| .{ .x = bx - half_width * @cos(angle), .y = by - half_width * @sin(angle) }, | |
| .{ .x = ax - half_width * @cos(angle), .y = ay - half_width * @sin(angle) }, | |
| .{ .x = bx + half_width * @cos(angle), .y = by + half_width * @sin(angle) }, | |
| }; | |
| // Validate inputs | |
| if (width <= 0) return error.InvalidWidth; | |
| if (start.x == end.x and start.y == end.y) return error.ZeroLengthLine; | |
| const points = [_]struct { x: f32, y: f32 }{ | |
| .{ .x = ax - half_width * @cos(angle), .y = ay - half_width * @sin(angle) }, | |
| .{ .x = ax + half_width * @cos(angle), .y = ay + half_width * @sin(angle) }, | |
| .{ .x = bx + half_width * @cos(angle), .y = by + half_width * @sin(angle) }, | |
| .{ .x = bx - half_width * @cos(angle), .y = by - half_width * @sin(angle) }, | |
| .{ .x = ax - half_width * @cos(angle), .y = ay - half_width * @sin(angle) }, | |
| .{ .x = bx + half_width * @cos(angle), .y = by + half_width * @sin(angle) }, | |
| }; |
🤖 Prompt for AI Agents
In src/logic/line.zig around lines 18 to 25, the calculation of points uses
trigonometric functions without validating inputs, which could lead to
mathematical errors or unexpected behavior. Add input validation to ensure
parameters like angle and half_width are within valid ranges and not NaN or
infinite. This will prevent potential runtime errors and improve robustness.
ADRs/replace Rust with Zig
Outdated
| Fight with Rust syntax/specification takes more efford than writign application code itself. | ||
|
|
||
| - we cannot get pointer/read only struct if we cannot justify lifetime of that struct is logner then where the palce where reference is used | ||
| - We have Point(x,y) and PointUV(x,y,u,v) it takes a lot of code to implement traits to make later create fucntion that takes any of those two points. Also traits can be onyl done with methods, no commo nfield or getters, so we have to call point.x() which makes it super unredable during compelx calculations | ||
| - zig has produced smaller and more performant code, both in debugging and production modes. In production build: | ||
|
|
||
| Rust: | ||
| 173 kB of JS & embedded WASM as base64 | ||
| 103 kB of wasm | ||
|
|
||
| Wasm | ||
| 166 kB of JS \* embedded WASM as base64 | ||
| 44 kB rust(coupled to super fast, not with smallest size optimiser) | ||
|
|
||
| - with zig we can also reuse C libaries. C environment is significantly bigger than rust. | ||
| - complication process takes in Rust like 4-5s secodns while is Zig around 1s | ||
| - Zig seems to produce faster performing WASM, although zigar JS glue code seems to be slowing down more than wasm-pack JS glue code. In both cases the result is not far from each other. | ||
|
|
||
| Downsides: | ||
|
|
||
| - Zig is a new, prone ot large changes language/environment | ||
| - Compiler is helpful but not that super friendly helpful as in Rust |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve document quality and professionalism.
The ADR contains valuable technical insights but has numerous spelling, grammar, and formatting issues that detract from its credibility.
Key issues to address:
- Line 1: "efford" → "effort", "writign" → "writing"
- Line 3: "logner then" → "longer than", "palce" → "place"
- Line 4: "fucntion" → "function", "onyl" → "only", "commo nfield" → "common field"
- Line 12: "Wasm" should be "Zig" (based on context)
- Line 16: "complication" → "compilation", "secodns" → "seconds", "is" → "in"
- Line 21: "ot" → "to"
Consider restructuring with:
- Clear sections (Context, Decision, Consequences)
- Proper formatting for technical comparisons
- Substantiation for performance claims with benchmarks
🤖 Prompt for AI Agents
In ADRs/replace Rust with Zig from lines 1 to 22, fix multiple spelling and
grammar errors such as "efford" to "effort," "writign" to "writing," "logner
then" to "longer than," "palce" to "place," "fucntion" to "function," "onyl" to
"only," "commo nfield" to "common field," "Wasm" to "Zig" on line 12,
"complication" to "compilation," "secodns" to "seconds," "is" to "in," and "ot"
to "to." Additionally, restructure the document into clear sections like
Context, Decision, and Consequences, format technical comparisons properly, and
support performance claims with benchmarks to improve professionalism and
clarity.
|
|
||
| const state = State.new(300, 300) | ||
|
|
||
| init_state(canvas.clientWidth, canvas.clientHeight) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify canvas dimension handling.
Ensure the Zig init_state function properly handles dynamic canvas resizing, as the previous state management might have included resize logic.
🏁 Script executed:
#!/bin/bash
# Search for canvas resize handling in Zig modules
ast-grep --pattern 'init_state($_, $_) {
$$$
}'Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for init_state definition in Zig source files
rg "fn init_state" -g '*.zig' -nLength of output: 107
🏁 Script executed:
#!/bin/bash
# Show init_state implementation with context to inspect resize handling
rg -n -C5 "pub fn init_state" src/logic/index.zigLength of output: 401
🏁 Script executed:
#!/bin/bash
# Show the first 100 lines of the TypeScript entrypoint to inspect init_state usage
echo "----- src/index.ts (lines 1-100) -----"
sed -n '1,100p' src/index.ts || echo "Failed to read src/index.ts"
# Check for dynamic canvas resize handlers in the TS code
echo "----- Searching for window resize listeners -----"
rg -n "addEventListener.*resize" src/index.ts || echo "No addEventListener('resize') found"
rg -n "onresize" src/index.ts || echo "No window.onresize found"Length of output: 3365
Add dynamic canvas resize handling
The init_state function in Zig only sets the dimensions once and isn’t re-invoked on resize. The canvasSizeObserver callback is currently a no-op, so resizing the canvas won’t update the Zig state. To keep your rendering state in sync, re-call init_state (or a dedicated resize API) inside that observer.
• File: src/index.ts
– Locate the canvasSizeObserver call:
ts canvasSizeObserver(canvas, device, () => { // state.needsRefresh = true })
– Update it to reinitialize dimensions:
diff canvasSizeObserver(canvas, device, () => { - // state.needsRefresh = true + init_state(canvas.clientWidth, canvas.clientHeight); + // TODO: trigger a refresh flag in Zig if needed })
• If simply resetting width/height isn’t enough, consider exposing a Zig function (e.g. set_needs_refresh()) to mark the state dirty after a resize.
This ensures your Zig state.width/height always reflect the actual canvas size.
🤖 Prompt for AI Agents
In src/index.ts at line 21, the call to init_state sets canvas dimensions only
once and does not handle dynamic resizing. Locate the canvasSizeObserver call
and update its callback to re-call init_state with the current
canvas.clientWidth and canvas.clientHeight to update the Zig state on resize. If
needed, also expose and call a Zig function to mark the state as needing refresh
after resizing to keep rendering in sync.
src/WebGPU/pointer.ts
Outdated
| canvas.addEventListener('click', e => { | ||
| on_pointer_click() | ||
| }) |
There was a problem hiding this comment.
The click handler doesn't update pointer coordinates.
Unlike mousedown, the click handler doesn't call updatePointer(e) before invoking on_pointer_click(). This could lead to stale pointer coordinates if the mouse moved between the last mousemove event and the click.
canvas.addEventListener('click', e => {
+ updatePointer(e)
on_pointer_click()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| canvas.addEventListener('click', e => { | |
| on_pointer_click() | |
| }) | |
| canvas.addEventListener('click', e => { | |
| updatePointer(e) | |
| on_pointer_click() | |
| }) |
🤖 Prompt for AI Agents
In src/WebGPU/pointer.ts around lines 26 to 28, the click event handler calls
on_pointer_click() without updating the pointer coordinates, which can cause
stale data if the mouse moved before the click. Modify the click handler to call
updatePointer(e) before on_pointer_click() to ensure the pointer coordinates are
current when handling the click event.
| draw_triangle: *const fn ([]const f32) void, | ||
| pick_texture: *const fn ([]const f32, u32) void, | ||
| }; | ||
| var web_gpu_programs: *const WebGpuPrograms = undefined; |
There was a problem hiding this comment.
Initialize web_gpu_programs to prevent undefined behavior.
The web_gpu_programs pointer is initially undefined. If any rendering functions are called before connectWebGPUPrograms, the program will crash.
Consider initializing with a default no-op implementation or adding null checks:
-var web_gpu_programs: *const WebGpuPrograms = undefined;
+var web_gpu_programs: ?*const WebGpuPrograms = null;Then update the usage to check for null:
if (web_gpu_programs) |programs| {
programs.draw_texture(vertex_data, asset.value_ptr.texture_id);
}Also applies to: 15-19
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 12 and 15-19, the pointer variable
web_gpu_programs is initialized as undefined, which can cause crashes if
accessed before proper assignment. To fix this, initialize web_gpu_programs to
null or a safe default no-op implementation to avoid undefined behavior.
Additionally, update all usages of web_gpu_programs to include null checks
before dereferencing, ensuring the program only calls methods on it when it is
valid.
src/logic/index.zig
Outdated
| state.ongoing_action = .none; | ||
| } | ||
|
|
||
| pub fn on_pointer_move(x: f32, y: f32) !void { |
There was a problem hiding this comment.
Fix error handling in on_pointer_move.
The function signature indicates it can return an error (!void) but it never actually returns any errors. Additionally, line 92 uses .? which could panic.
-pub fn on_pointer_move(x: f32, y: f32) !void {
+pub fn on_pointer_move(x: f32, y: f32) void {
switch (state.ongoing_action) {
.move => {
const offset = Types.Point{
.x = x - state.last_pointer_coords.x,
.y = y - state.last_pointer_coords.y,
};
state.last_pointer_coords = Types.Point{ .x = x, .y = y };
- const asset_ptr: *Texture = state.assets.getPtr(state.active_asset_id).?;
+ if (state.assets.getPtr(state.active_asset_id)) |asset_ptr| {
var new_points: [4]Types.PointUV = undefined;
for (asset_ptr.points, 0..) |point, i| {
new_points[i] = Types.PointUV{
.x = point.x + offset.x,
.y = point.y + offset.y,
.u = point.u,
.v = point.v,
};
}
asset_ptr.update_coords(new_points);
+ }
},
.none => {},
}
}Also applies to: 92-92
🤖 Prompt for AI Agents
In src/logic/index.zig at line 83, the function on_pointer_move is declared to
return an error (!void) but does not actually return any errors, and line 92
uses the error propagation operator (?.) which can cause a panic. To fix this,
either update the function signature to return void if no errors are expected or
properly handle and return errors within the function. Also, replace the use of
?. on line 92 with proper error handling to avoid panics, such as using try or
explicit error checks.
| pub fn add_texture(id: u32, points: [4]Types.PointUV, texture_index: u32) void { | ||
| state.assets.put(id, Texture.new(id, points, texture_index)) catch unreachable; | ||
| } |
There was a problem hiding this comment.
Improve error handling in add_texture.
Using catch unreachable will crash the program if allocation fails. Consider proper error handling or at least logging.
-pub fn add_texture(id: u32, points: [4]Types.PointUV, texture_index: u32) void {
- state.assets.put(id, Texture.new(id, points, texture_index)) catch unreachable;
+pub fn add_texture(id: u32, points: [4]Types.PointUV, texture_index: u32) !void {
+ try state.assets.put(id, Texture.new(id, points, texture_index));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn add_texture(id: u32, points: [4]Types.PointUV, texture_index: u32) void { | |
| state.assets.put(id, Texture.new(id, points, texture_index)) catch unreachable; | |
| } | |
| pub fn add_texture(id: u32, points: [4]Types.PointUV, texture_index: u32) !void { | |
| try state.assets.put(id, Texture.new(id, points, texture_index)); | |
| } |
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 53 to 55, the add_texture function currently
uses `catch unreachable` which causes the program to crash on allocation
failure. Replace this with proper error handling by either returning an error
from the function or logging the failure before handling it gracefully. Modify
the function signature if needed to propagate errors or ensure the error is
logged and handled without crashing.
| pub fn update_points(id: u32, points: [4]Types.PointUV) void { | ||
| var asset_ptr: *Texture = state.assets.getPtr(id).?; | ||
| asset_ptr.update_coords(points); | ||
| } |
There was a problem hiding this comment.
Add null safety check for asset lookup.
Using .? will panic if the ID doesn't exist in the hashmap. Add a proper check first.
pub fn update_points(id: u32, points: [4]Types.PointUV) void {
- var asset_ptr: *Texture = state.assets.getPtr(id).?;
- asset_ptr.update_coords(points);
+ if (state.assets.getPtr(id)) |asset_ptr| {
+ asset_ptr.update_coords(points);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn update_points(id: u32, points: [4]Types.PointUV) void { | |
| var asset_ptr: *Texture = state.assets.getPtr(id).?; | |
| asset_ptr.update_coords(points); | |
| } | |
| pub fn update_points(id: u32, points: [4]Types.PointUV) void { | |
| if (state.assets.getPtr(id)) |asset_ptr| { | |
| asset_ptr.update_coords(points); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 57 to 60, the use of `.?` for asset lookup
will cause a panic if the ID is not found. Replace this with a null safety check
by first retrieving the pointer with `getPtr(id)` and verifying it is not null
before calling `update_coords(points)`. Handle the case where the asset is not
found gracefully without panicking.
| fn get_border() []f32 { | ||
| var vertex_data = std.ArrayList(f32).init(std.heap.page_allocator); | ||
| // TODO: free memory, defer list.deinit(); | ||
| const red = [_]f32{ 1.0, 0.0, 0.0, 1.0 }; | ||
| if (state.assets.get(state.hovered_asset_id)) |asset| { | ||
| for (asset.points, 0..) |point, i| { | ||
| const next_point = if (i == 3) asset.points[0] else asset.points[i + 1]; | ||
| const new_verticies = Line.get_vertex_data( | ||
| point, | ||
| next_point, | ||
| 20.0, | ||
| red, | ||
| ); | ||
| // std.debug.print("new_verticies: {any}\n", .{new_verticies}); | ||
| vertex_data.appendSlice(new_verticies) catch unreachable; | ||
| } | ||
| } | ||
|
|
||
| const green = [_]f32{ 0.0, 1.0, 0.0, 1.0 }; | ||
| if (state.assets.get(state.active_asset_id)) |asset| { | ||
| for (asset.points, 0..) |point, i| { | ||
| const next_point = if (i == 3) asset.points[0] else asset.points[i + 1]; | ||
|
|
||
| vertex_data.appendSlice(Line.get_vertex_data( | ||
| point, | ||
| next_point, | ||
| 20.0, | ||
| green, | ||
| )) catch unreachable; | ||
| } | ||
| } | ||
|
|
||
| return vertex_data.items; | ||
| } |
There was a problem hiding this comment.
Fix memory leak in get_border function.
The ArrayList is allocated but never freed, causing a memory leak on every render. This is acknowledged in the TODO comment but needs to be addressed.
Consider one of these solutions:
Option 1: Return owned ArrayList and free in caller
-fn get_border() []f32 {
+fn get_border() std.ArrayList(f32) {
var vertex_data = std.ArrayList(f32).init(std.heap.page_allocator);
- // TODO: free memory, defer list.deinit();
// ... rest of function ...
- return vertex_data.items;
+ return vertex_data;
}Then in canvas_render:
- const border_verticies = get_border();
+ var border_list = get_border();
+ defer border_list.deinit();
+ const border_verticies = border_list.items;Option 2: Use a pre-allocated buffer or arena allocator
+var border_arena = std.heap.ArenaAllocator.init(std.heap.page_allocator);
+
fn get_border() []f32 {
- var vertex_data = std.ArrayList(f32).init(std.heap.page_allocator);
+ _ = border_arena.reset(.retain_capacity);
+ var vertex_data = std.ArrayList(f32).init(border_arena.allocator());Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 110 to 143, the get_border function creates
an ArrayList that is never freed, causing a memory leak. To fix this, either
modify get_border to return the owned ArrayList so the caller can defer its
deinit and free the memory properly, or refactor the function to use a
pre-allocated buffer or arena allocator that manages memory lifecycle outside
this function. Ensure that whichever approach you choose, the allocated memory
is properly released after use to prevent leaks.
|
🎉 This PR is included in version 0.1.0-next.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Documentation