feat: generate asset ids in zig, add sourcemaps to JS #16
Conversation
WalkthroughThis change removes explicit asset ID management from the TypeScript and Zig interfaces, shifting asset identification to internal logic. It updates asset-related type definitions, callbacks, and function signatures accordingly. The ESLint configuration is refactored and simplified. Additional improvements include enhanced cleanup routines, error handling in GPU picking, and always enabling production source maps. Changes
Sequence Diagram(s)sequenceDiagram
participant TS as TypeScript (src/index.ts)
participant Zig as Zig Logic (index.zig)
participant GPU as WebGPU
participant UI as User Interface
UI->>TS: User adds image
TS->>Zig: addImage(img, points?)
Zig->>Zig: Assigns internal asset ID, stores asset
Zig->>TS: on_asset_update_callback([AssetZig])
TS->>UI: Updates asset list (no explicit IDs)
UI->>TS: Calls destroy()
TS->>Zig: destroy_state()
TS->>GPU: Unconfigure context, destroy device
Possibly related PRs
Suggested labels
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 selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ 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: 5
🔭 Outside diff range comments (1)
src/logic/texture.zig (1)
28-33: 🛠️ Refactor suggestionAllocated vertex buffers are never freed
Both
get_vertex_dataandget_vertex_pick_datausestd.heap.page_allocator.allocwithout a correspondingfree.
This is inside a hot path and can quickly exhaust memory when assets are updated frequently.Add a de-allocator or switch to a caller-supplied buffer.
🧹 Nitpick comments (10)
jestWgslTransformer.js (1)
1-5: Unusedoptionsparameter – please remove or prefix with_
optionsis accepted but never referenced. This will trigger theno-unused-varsrule once ESLint is wired up.
Two quick fixes:-export const process = (sourceText, sourcePath, options) => { +export const process = (sourceText, sourcePath /* options _unused_ */) => {or
-export const process = (sourceText, sourcePath, options) => { +export const process = (sourceText, sourcePath) => {eslint.config.js (1)
9-14: Stylistic plugin is declared but not enabled
@stylistic/jsis added toplugins, but none of its rules are turned on.
If you intended to leverage the preset, add something like:rules: { ...stylisticJs.configs.custom, // or whichever stylistic preset you need }Otherwise the plugin inclusion is dead weight.
src/logic/texture.zig (1)
10-13: Struct name leaks implementation detail
AssetZigbakes the language name into the public API. If the struct is meant to cross the JS boundary, consider a neutral name (AssetorSerializedAsset) to avoid later churn if the backend changes again.integration-tests/index.ts (1)
55-55:creator.addImageis async-unsafe
URL.createObjectURLis revoked only when the page unloads.
Consider revoking after the texture is uploaded to free memory:img.onload = () => { creator.addImage(img).finally(() => URL.revokeObjectURL(img.src)) }src/run.ts (3)
34-35: Avoid sentinel0→ initialiserafIdlazily
requestAnimationFramenever returns0, but passing0tocancelAnimationFrameis still a (no-op) call.
Initialising toundefined(ornull) removes the redundant cancel and makes types clearer.- let rafId = 0 + let rafId: number | undefined
55-58: Duplicate frame-scheduling can be trimmed
requestAnimationFrame(draw)is invoked both at the end ofdraw(line 55) and once more immediately after the function definition (line 58).
That’s correct for an rAF loop, but the latter call can race with a very fast first loop and momentarily schedule two frames.
Instead, start the loop once and havedrawreschedule itself:- rafId = requestAnimationFrame(draw) + rafId = requestAnimationFrame(draw) ... - rafId = requestAnimationFrame(draw) + draw(performance.now() as DOMHighResTimeStamp) // kickoff without double-queueing
60-62: Expose a no-op guard in the cleanup closureIf
stopCreatoris called twice we’ll attempt to cancel the same frame ID twice.
A quick guard prevents accidental double invocations:- return () => { - cancelAnimationFrame(rafId) - } + return () => { + if (rafId !== undefined) { + cancelAnimationFrame(rafId) + rafId = undefined + } + }src/logic/index.d.ts (1)
34-34: Typo:destory_state→destroy_stateThe misspelling propagates to every TS consumer and the Zig export.
Aligning the name now avoids permanent API baggage.- export const destory_state: VoidFunction + export const destroy_state: VoidFunctionsrc/WebGPU/pick.ts (1)
108-116: Minor catch-block polish & typoSpelling and spacing nit; also
console.debug(err)could aid sporadic GPU-device loss debugging without polluting normal logs.- /* ignorign errors when map fails because device was destroyed(so buffer too and was unmapped before mapAsync completed)*/ + // ignoring errors when map fails because the device/buffer was destroyed + // console.debug('pick mapAsync failed', err)src/index.ts (1)
58-69: Callback array copy allocates every frame
connect_on_asset_update_callbackmay fire every pick frame; recreating and spreading arrays/object literals inside the callback can become GC-heavy.Consider re-using typed arrays / objects or passing references and letting the consumer copy only when necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
eslint.config copy.mjs(0 hunks)eslint.config.js(1 hunks)eslint.config.mjs(0 hunks)integration-tests/index.ts(1 hunks)jestWgslTransformer.js(1 hunks)package.json(1 hunks)src/WebGPU/pick.ts(4 hunks)src/index.ts(3 hunks)src/logic/index.d.ts(1 hunks)src/logic/index.zig(6 hunks)src/logic/texture.zig(2 hunks)src/run.ts(4 hunks)webpack.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- eslint.config copy.mjs
- eslint.config.mjs
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/run.ts (2)
src/WebGPU/pick.ts (1)
PickManager(7-119)src/logic/index.d.ts (1)
connect_web_gpu_programs(24-29)
src/WebGPU/pick.ts (1)
src/logic/index.d.ts (1)
on_update_pick(19-19)
src/index.ts (4)
src/logic/index.d.ts (3)
connect_on_asset_update_callback(30-30)add_texture(16-16)destory_state(34-34)src/WebGPU/getTexture/index.ts (1)
createTextureFromSource(106-117)src/utils/getDefaultPoints.ts (1)
getDefaultPoints(3-19)src/run.ts (1)
runCreator(11-63)
🔇 Additional comments (7)
eslint.config.js (1)
7-15:⚠️ Potential issueSecond argument should be TypeScript-ESLint’s recommended preset, not ESLint’s
The call currently passes
eslint.configs.recommendedtwice, so TypeScript-specific rules never get applied.-export default tseslint.config(eslint.configs.recommended, eslint.configs.recommended, { +export default tseslint.config( + eslint.configs.recommended, // base JS rules + tseslint.configs.recommended, // TypeScript rules + { + rules: { + 'no-multiple-empty-lines': 'error', + semi: ['error', 'never'], + }, + plugins: { '@stylistic/js': stylisticJs }, + }, +)Likely an incorrect or invalid review comment.
src/logic/texture.zig (1)
72-78: Missingidfield can break picking / hit-testing
serialize()omits.id, yetget_vertex_pick_datastill writesself.idinto the vertex buffer (line 60).
If the JS side relies on the serialized payload for hit-testing, dropping the id will desynchronise the two paths.pub fn serialize(self: Texture) AssetZig { return AssetZig{ .points = self.points, .texture_id = self.texture_id, + // .id = self.id, // ← consider including or document why it's safe to drop }; }webpack.config.js (1)
18-18: Considerhidden-source-mapinstead ofsource-mapfor production
'source-map'exposes the full original source in the published bundle.
If you only need stack-traces without revealing code, switch to'hidden-source-map'(or'nosources-source-map') to avoid leaking IP while keeping debuggability.- devtool: isProd ? 'source-map' : 'eval-source-map', + devtool: isProd ? 'hidden-source-map' : 'eval-source-map',src/logic/index.d.ts (1)
16-18:update_pointsstill exposesidalthoughadd_textureno longer doesWith asset IDs now generated internally, external code has no stable handle to pass to
update_points(id, …).
Either:
- Expose the generated ID from
add_texture, or- Change
update_pointsto identify assets differently (e.g., by pointer or implicit “last”).Please verify the new contract; otherwise updates will be impossible.
src/index.ts (1)
71-79:newTextureIdrace on slow image loads
newTextureIdis derived fromtextures.lengthatimg.onloadtime.
If multiple large images load out-of-order, IDs will mismatch the order of originalassets[], potentially breaking deterministic re-loads.Capture the index synchronously:
- const newTextureId = textures.length + const newTextureId = textures.push({ + url: img.src, + texture: createTextureFromSource(device, img, { flipY: true }), + }) - 1 - textures.push({ … })Push first, then use the returned index.
src/logic/index.zig (2)
18-22: Renaming follows Zig conventions – looks good.Switching to
snake_casefor exported functions is consistent with idiomatic Zig style and avoids mixed-case exports on the TS side.
24-27: Callback signature update acknowledged, but verify all call-sites.The callback now transmits
[]AssetZig. Double-check that every caller in the TS layer (and tests) has been updated; mismatched function pointers will SIGILL at runtime.
| var next_asset_id: u32 = ASSET_ID_TRESHOLD; | ||
| pub fn add_texture(points: [4]Types.PointUV, texture_id: u32) void { | ||
| state.assets.put(next_asset_id, Texture.new(next_asset_id, points, texture_id)) catch unreachable; | ||
| next_asset_id +%= 1; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Un-checked wrapping add risks ID reuse after overflow.
next_asset_id +%= 1 silently wraps on u32 overflow, eventually colliding with existing assets after 4 294 967 295 inserts (or sooner if IDs are recycled).
Prefer checked addition and fail explicitly, or switch to u64.
- next_asset_id +%= 1;
+ next_asset_id += 1;
+ if (next_asset_id == 0) { // overflowed back to 0
+ @panic("asset id overflow");
+ }📝 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.
| var next_asset_id: u32 = ASSET_ID_TRESHOLD; | |
| pub fn add_texture(points: [4]Types.PointUV, texture_id: u32) void { | |
| state.assets.put(next_asset_id, Texture.new(next_asset_id, points, texture_id)) catch unreachable; | |
| next_asset_id +%= 1; | |
| } | |
| var next_asset_id: u32 = ASSET_ID_TRESHOLD; | |
| pub fn add_texture(points: [4]Types.PointUV, texture_id: u32) void { | |
| state.assets.put(next_asset_id, Texture.new(next_asset_id, points, texture_id)) catch unreachable; | |
| next_asset_id += 1; | |
| if (next_asset_id == 0) { // overflowed back to 0 | |
| @panic("asset id overflow"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 63 to 67, the increment of next_asset_id
uses wrapping addition which can silently overflow and cause ID reuse. Replace
the wrapping addition with a checked addition that detects overflow and
explicitly fails or handles the error. Alternatively, change the type of
next_asset_id to u64 to reduce overflow risk. Ensure the code prevents silent ID
collisions by handling the increment safely.
src/logic/index.zig
Outdated
| pub fn destory_state() void { | ||
| state.assets.deinit(); | ||
| next_asset_id = ASSET_ID_TRESHOLD; | ||
| web_gpu_programs = undefined; | ||
| on_asset_update_cb = undefined; | ||
| // state itself is not destoyed as it will be reinitalized before usage | ||
| // and has no reference to memory to free | ||
| } |
There was a problem hiding this comment.
Typo and incomplete cleanup in destory_state.
- Function name is misspelled – should be
destroy_state. - Only the hashmap is de-initialised; any heap allocations owned by
Textureinstances (vertex buffers, etc.) are not released. - Outstanding result buffers from the previous comment remain leaked.
-pub fn destory_state() void {
- state.assets.deinit();
+pub fn destroy_state() void {
+ var it = state.assets.valueIterator();
+ while (it.next()) |tex| {
+ tex.deinit(); // assuming Texture owns heap memory
+ }
+ state.assets.deinit();Add corresponding rename on the TS side.
📝 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 destory_state() void { | |
| state.assets.deinit(); | |
| next_asset_id = ASSET_ID_TRESHOLD; | |
| web_gpu_programs = undefined; | |
| on_asset_update_cb = undefined; | |
| // state itself is not destoyed as it will be reinitalized before usage | |
| // and has no reference to memory to free | |
| } | |
| pub fn destroy_state() void { | |
| var it = state.assets.valueIterator(); | |
| while (it.next()) |tex| { | |
| tex.deinit(); // assuming Texture owns heap memory | |
| } | |
| state.assets.deinit(); | |
| next_asset_id = ASSET_ID_TRESHOLD; | |
| web_gpu_programs = undefined; | |
| on_asset_update_cb = undefined; | |
| // state itself is not destoyed as it will be reinitalized before usage | |
| // and has no reference to memory to free | |
| } |
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 223 to 230, rename the function from
destory_state to destroy_state to fix the typo. Extend the cleanup logic to
properly release all heap allocations owned by Texture instances, such as vertex
buffers, to prevent memory leaks. Also, ensure any outstanding result buffers
are freed as indicated by previous comments. Finally, update the corresponding
function name on the TypeScript side to maintain consistency.
| var result = std.heap.page_allocator.alloc(AssetZig, state.assets.count()) catch unreachable; | ||
| var iterator = state.assets.iterator(); | ||
| var i: usize = 0; | ||
| while (iterator.next()) |entry| { | ||
| result[i] = entry.value_ptr.*; | ||
| result[i] = entry.value_ptr.serialize(); | ||
| i += 1; | ||
| } |
There was a problem hiding this comment.
Leaking allocator memory every pointer-up.
std.heap.page_allocator.alloc is never freed; result lives past the function scope with no matching dealloc, causing an unbounded leak during interactive sessions.
If the JS side copies the slice immediately, free it right after the callback:
on_asset_update_cb(result);
-}
+ std.heap.page_allocator.free(result);
+}Otherwise, expose a free_asset_update_buffer callback for the consumer to invoke.
📝 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.
| var result = std.heap.page_allocator.alloc(AssetZig, state.assets.count()) catch unreachable; | |
| var iterator = state.assets.iterator(); | |
| var i: usize = 0; | |
| while (iterator.next()) |entry| { | |
| result[i] = entry.value_ptr.*; | |
| result[i] = entry.value_ptr.serialize(); | |
| i += 1; | |
| } | |
| on_asset_update_cb(result); | |
| std.heap.page_allocator.free(result); | |
| } |
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 99 to 105, the memory allocated with
std.heap.page_allocator.alloc for 'result' is never freed, causing a memory
leak. To fix this, either free the allocated memory immediately after the JS
side copies the slice if it does so synchronously, or provide a dedicated
'free_asset_update_buffer' function that the consumer can call to deallocate the
memory when it is no longer needed.
|
🎉 This PR is included in version 0.1.0-next.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit