fix: snapshots on init #31
Conversation
WalkthroughThis update refactors asset and texture loading logic, modifies asset ID handling, and adjusts API signatures for asset management in both TypeScript and Zig code. It introduces new types, improves asynchronous handling of assets, updates integration tests and HTML templates, adds a loading texture fallback, and enhances pointer event handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Creator
participant Logic (Zig)
participant GPU
User->>UI: Selects images via #add-image or #start-project-from-images
UI->>Creator: Calls addImage(url) or resetAssets([{url}, ...])
Creator->>GPU: addTexture(url) (async image load)
GPU-->>Creator: Texture ready (with dimensions)
Creator->>Logic (Zig): add_asset(maybe_id, points, texture_id) or reset_assets([...], with_snapshot)
Logic (Zig)->>Creator: on_asset_update_cb([...])
Creator->>UI: Updates asset state/history
Estimated code review effort4 (60–120 minutes) 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! ✨ 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes snapshot handling on initialization by implementing asynchronous asset loading with fallback textures to prevent race conditions during initialization.
- Refactored parameter signatures to use primitive types instead of DOM elements for better separation of concerns
- Implemented asynchronous texture loading with fallback "loading" textures to prevent rendering issues
- Added proper snapshot control during asset initialization to avoid unwanted history entries
Reviewed Changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/getDefaultPoints.ts | Refactored to accept primitive width/height parameters instead of DOM elements |
| src/run.ts | Added loading texture fallback for unloaded textures |
| src/logic/index.zig | Updated asset management to handle optional callbacks and snapshot control |
| src/loadingTexture.ts | New file providing fallback texture for assets during loading |
| src/index.ts | Refactored to handle asynchronous asset loading with proper timing control |
| src/WebGPU/pointer.ts | Added mouse leave handling and pointer state management |
| src/WebGPU/pick.ts | Added clarifying comment about pointer state timing |
| playwright.config.ts | Simplified http-server command |
| package.json | Added http-server dependency |
| integration-tests/ | Updated tests to support new API and added initial assets test |
Comments suppressed due to low confidence (1)
src/logic/index.zig:81
- [nitpick] The parameter 'maybe_id' is ambiguous. Consider renaming to 'id_or_zero' or 'provided_id' to clarify that 0 means generate a new ID.
pub fn add_asset(maybe_id: u32, points: [4]Types.PointUV, texture_id: u32) void {
![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)
src/WebGPU/pick.ts (1)
115-116: Good documentation of the design issue.The comment accurately describes the synchronization problem where pointer coordinates are updated immediately while pick event processing is deferred. Consider addressing this architectural concern in a future refactor to ensure consistent event ordering.
src/index.ts (1)
172-177: Fix typos in the comment.- const textureId = addTexture(asset.url, (width, height) => { - // we wait to add iamge once poitns are known because otherwise - // if we add img first with "Default" point value and update - // it later ocne texture is loaded, we will get history snapshot with - // that "default" points + const textureId = addTexture(asset.url, (width, height) => { + // we wait to add image once points are known because otherwise + // if we add img first with "Default" point value and update + // it later once texture is loaded, we will get history snapshot with + // that "default" points
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
integration-tests/another-image-sample.jpgis excluded by!**/*.jpgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
integration-tests/index.ts(2 hunks)integration-tests/init.ts(1 hunks)integration-tests/template.html(1 hunks)integration-tests/tests/history.spec.ts(1 hunks)integration-tests/tests/initial-assets.spec.ts(1 hunks)package.json(1 hunks)playwright.config.ts(1 hunks)src/WebGPU/pick.ts(1 hunks)src/WebGPU/pointer.ts(2 hunks)src/index.ts(3 hunks)src/loadingTexture.ts(1 hunks)src/logic/index.d.ts(1 hunks)src/logic/index.zig(8 hunks)src/run.ts(2 hunks)src/utils/getDefaultPoints.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/WebGPU/pointer.ts (1)
src/logic/index.d.ts (1)
on_pointer_leave(30-30)
integration-tests/index.ts (1)
src/index.ts (1)
SerializedOutputAsset(28-33)
src/run.ts (3)
src/loadingTexture.ts (1)
getLoadingTexture(1-32)src/logic/index.d.ts (1)
connect_web_gpu_programs(32-38)src/WebGPU/programs/initPrograms.ts (4)
drawTexture(18-18)drawMSDF(21-21)drawTriangle(12-12)pickTexture(19-19)
src/utils/getDefaultPoints.ts (1)
src/utils/clamp.ts (1)
clamp(1-3)
🔇 Additional comments (24)
package.json (1)
41-41: http-server version ^14.1.1 is up-to-dateThe
http-serverdevDependency at version^14.1.1matches the latest stable release (14.1.1) and aligns with our Playwright test setup. Approved.src/loadingTexture.ts (3)
1-6: LGTM! Clean function signature and well-defined constants.The function signature is clear and the color constants provide good readability for the texture pattern definition.
8-16: Creative and readable texture data definition.The approach of using color arrays and
.flat()to create the texture data is both readable and maintainable. The yellow "F" pattern will serve as a good visual indicator for loading textures.
18-31: Proper GPU texture creation and data upload.The texture creation uses appropriate format (
rgba8unorm), correct usage flags, and proper data upload with correct layout parameters. The implementation follows WebGPU best practices.integration-tests/init.ts (1)
68-68: Good improvement in selector specificity.Changing from the generic
input[type="file"]to the specific#add-imageID selector improves test reliability and aligns with the updated HTML template structure.integration-tests/tests/history.spec.ts (1)
12-13: Good removal of non-null assertion operators.Removing the
!operators allows Playwright's built-in error handling to work properly, making the tests more robust and idiomatic.integration-tests/template.html (1)
33-34: Excellent UI and accessibility improvements.The changes provide several benefits:
- Proper labels improve accessibility and user experience
- Semantic separation between single image addition and bulk project initialization
- Clear IDs that align with test selectors
- Multiple file support for project initialization
These changes enhance both usability and testability.
playwright.config.ts (1)
114-114: LGTM! Dependency management improvement.Removing
npxaligns with addinghttp-serveras a direct dev dependency, which is a cleaner approach than relying on npx for package execution.src/run.ts (5)
14-14: Excellent defensive programming pattern.Adding the loading texture import enables robust fallback handling for missing textures during async loading.
34-34: Good initialization strategy.Initializing the loading texture once and reusing it across all rendering calls is efficient and provides consistent fallback behavior.
37-43: Robust texture fallback implementation.The nullish coalescing operator ensures
draw_texturealways receives a valid GPU texture, preventing runtime errors when textures haven't loaded yet.
44-51: Consistent fallback pattern for MSDF rendering.The same robust fallback pattern applied to
draw_msdfensures consistent behavior across all texture rendering operations.
53-60: Proper fallback for picking operations.Applying the loading texture fallback to
pick_textureensures picking operations work correctly even when assets are still loading, maintaining UI responsiveness.integration-tests/tests/initial-assets.spec.ts (1)
8-27: Test image files verified
- Confirmed that both
integration-tests/image-sample.pngandintegration-tests/another-image-sample.jpgare present.Integration test is approved as-is.
src/WebGPU/pointer.ts (4)
1-6: Good import organization.Adding
on_pointer_leaveimport expands the pointer event handling capabilities for better canvas boundary management.
8-8: Excellent sentinel value choice.Using
-1asOUTSIDE_CANVASis much better than0since(0,0)is a valid canvas coordinate. This clearly distinguishes between a valid position and "outside canvas" state.
25-26: Proper initialization with sentinel values.Initializing pointer coordinates to
OUTSIDE_CANVAScorrectly represents the initial state where the pointer hasn't entered the canvas yet.
34-50: Robust mouseleave event handling.The implementation correctly:
- Follows the established deferred processing pattern for event queue consistency
- Sets coordinates to the sentinel value when leaving canvas
- Calls the appropriate logic callback
- Maintains proper event ordering with other pointer operations
This ensures reliable pointer state management when the cursor exits the canvas area.
integration-tests/index.ts (1)
43-62: LGTM! Good handling of file inputs.The implementation correctly:
- Uses specific IDs for file inputs instead of generic selectors
- Resets input values to allow re-uploading the same file
- Properly creates object URLs from selected files
src/utils/getDefaultPoints.ts (1)
3-42: Good refactoring to decouple from DOM elements.The change from DOM elements to numeric parameters improves:
- Testability (easier to test with numeric values)
- Reusability (can be used in non-DOM contexts)
- Performance (avoids DOM property access)
src/logic/index.zig (3)
28-31: Good defensive programming with optional callback.Making the callback optional and checking for null before invocation prevents potential crashes.
75-85: Well-implemented ID generation logic.The ID generation function correctly uses wrapping addition to handle overflow, and the conditional ID assignment in
add_assetproperly handles both provided and generated IDs.
318-338: Clever callback suppression during reset.The temporary nullification of the callback during reset prevents unwanted notifications, and the conditional notification based on
with_snapshotprovides fine-grained control over update behavior.src/index.ts (1)
159-192: Well-implemented async asset reset with proper error handling.The use of
Promise.allSettledensures that one failed asset doesn't block others from loading, and filtering for fulfilled results provides resilience against individual failures.
| function addTexture(url: string, callback?: (width: number, height: number) => void): number { | ||
| loadingTextures++ | ||
| updateProcessing() | ||
|
|
||
| const textureId = textures.length | ||
| textures.push({ url }) | ||
|
|
||
| const img = new Image() | ||
| img.src = url | ||
|
|
||
| img.onload = () => { | ||
| textures[textureId].texture = createTextureFromSource(device, img, { flipY: true }) | ||
| callback?.(img.width, img.height) | ||
|
|
||
| loadingTextures-- | ||
| updateProcessing() | ||
| } | ||
|
|
||
| img.onerror = () => { | ||
| console.error(`Failed to load image from ${url}`) | ||
|
|
||
| loadingTextures-- | ||
| updateProcessing() | ||
| } | ||
|
|
||
| return textureId | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling in texture loading.
The current implementation has potential issues:
- Failed textures remain in the array without a texture property, which could cause issues when rendering
- Errors are only logged to console without notifying the caller
Consider adding error state tracking or removing failed textures from the array.
🤖 Prompt for AI Agents
In src/index.ts between lines 88 and 114, the addTexture function currently adds
a texture object to the textures array before loading the image, but if the
image fails to load, the texture property is never set and the failed texture
remains in the array, which may cause rendering issues. To fix this, modify the
img.onerror handler to either remove the failed texture entry from the textures
array or mark it with an error state. Additionally, enhance error handling by
invoking the callback with an error indication or adding an optional error
callback parameter so the caller is notified of the failure instead of just
logging to the console.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/index.ts (1)
88-114: Potential issue: Incomplete error handling in texture loading.The current implementation has the same issues identified in previous reviews:
- Failed textures remain in the array without a texture property
- Only console logging without proper error propagation to callers
Consider enhancing error handling:
img.onerror = () => { console.error(`Failed to load image from ${url}`) + // Mark the texture as failed or remove it from the array + textures[textureId] = { url, texture: null } // or similar error state loadingTextures-- updateProcessing() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/WebGPU/pick.ts(1 hunks)src/index.ts(3 hunks)src/logic/index.d.ts(1 hunks)src/logic/index.zig(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/WebGPU/pick.ts
- src/logic/index.d.ts
🔇 Additional comments (12)
src/logic/index.zig (8)
28-28: Good: Making callback pointer nullable improves robustness.The change from a non-null pointer to an optional pointer allows for safer callback handling and prevents potential issues when the callback is not set.
75-79: Good: Proper ID generation with overflow handling.The
generate_id()function correctly uses wrapping addition (+%=) to handle potential overflow, which is appropriate for ID generation.
115-147: Good: Improved null safety in callback handling.The early return pattern when the callback is null (
const cb = on_asset_update_cb orelse return;) is a good defensive programming practice that prevents potential crashes.
192-196: Good: New pointer leave handler for better UX.The
on_pointer_leave()function properly resets the interaction state when the pointer leaves the canvas, which should improve user experience by preventing stuck states.
318-338: Good: Conditional snapshot behavior aligns with PR objective.The addition of the
with_snapshotparameter and conditional call tonotify_about_assets_update()directly addresses the PR objective of fixing snapshots on initialization. This prevents unnecessary snapshot creation during asset reset.
345-345: Good: Proper state cleanup in destroy_state.Resetting
active_asset_idto 0 indestroy_state()ensures clean state management and prevents potential issues with stale asset references.
392-392: Good: Test updated to match new signature.The test correctly passes
falsefor the newwith_snapshotparameter, maintaining the original test behavior while adapting to the new API.
81-85: Alladd_assetcalls updated to the new signature; no further action required.
- src/index.ts:
add_asset(0 /* auto-generate ID */, points, textureId)- src/logic/index.zig:
add_asset(asset.id, asset.points, asset.texture_id);src/index.ts (4)
21-33: Good: Clear separation of input and output asset types.The distinction between
SerializedInputAsset(for loading) andSerializedOutputAsset(for callbacks) provides better type safety and clearer API contracts. The optional fields in the input type allow for flexible asset loading scenarios.
36-37: Good: Simplified API signature using URLs.The change from
HTMLImageElementtostring(URL) inaddImageand the addition of optionalwithSnapshotparameter inresetAssetsmake the API more flexible and align with the snapshot fix objective.
133-138: Good: Improved addImage implementation with async texture loading.The refactored
addImagefunction properly uses the newaddTexturehelper and generates default points based on actual image dimensions, which should provide better initial asset placement.
140-152: Good: Consistent texture loading for icons.The icon loading has been properly refactored to use the new
addTexturefunction with normalized coordinates calculation, maintaining consistency with the new texture loading approach.
| const resetAssets: CreatorAPI['resetAssets'] = async (assets, withSnapshot = false) => { | ||
| const results = await Promise.allSettled( | ||
| assets.map<Promise<AssetZig>>( | ||
| (asset) => | ||
| new Promise((resolve, reject) => { | ||
| if (asset.points) { | ||
| return resolve({ | ||
| points: asset.points, | ||
| texture_id: asset.textureId || addTexture(asset.url), | ||
| id: asset.id || 0, | ||
| }) | ||
| } | ||
|
|
||
| const textureId = addTexture(asset.url, (width, height) => { | ||
| // we wait to add image once points are known because otherwise | ||
| // if we add img first with "Default" point value and update | ||
| // it later once texture is loaded, we will get history snapshot with | ||
| // that "default" points | ||
| return resolve({ | ||
| points: getDefaultPoints(width, height, canvas.clientWidth, canvas.clientHeight), | ||
| texture_id: textureId, // if there is no points, then for sure there is no asset.textureId | ||
| id: 0, | ||
| }) | ||
| }) | ||
| }) | ||
| ) | ||
| ) | ||
|
|
||
| const zigAssets = results | ||
| .filter((result) => result.status === 'fulfilled') | ||
| .map((result) => result.value) | ||
|
|
||
| reset_assets(zigAssets, withSnapshot) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue: Complex async logic with limited error handling.
While the Promise.allSettled approach is good for handling partial failures, there are potential concerns:
- No error logging for rejected promises
- Silent filtering of failed assets may confuse users
- Potential race condition if
resetAssetsis called multiple times rapidly
Consider adding error handling and user feedback:
const results = await Promise.allSettled(
assets.map<Promise<AssetZig>>(
(asset) =>
new Promise((resolve, reject) => {
if (asset.points) {
return resolve({
points: asset.points,
texture_id: asset.textureId || addTexture(asset.url),
id: asset.id || 0,
})
}
const textureId = addTexture(asset.url, (width, height) => {
+ try {
return resolve({
points: getDefaultPoints(width, height, canvas.clientWidth, canvas.clientHeight),
texture_id: textureId,
id: 0,
})
+ } catch (error) {
+ reject(new Error(`Failed to generate points for ${asset.url}: ${error.message}`))
+ }
})
})
)
)
+// Log failed assets for debugging
+const failedAssets = results.filter(result => result.status === 'rejected')
+if (failedAssets.length > 0) {
+ console.warn(`Failed to load ${failedAssets.length} assets:`, failedAssets.map(r => r.reason))
+}📝 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 resetAssets: CreatorAPI['resetAssets'] = async (assets, withSnapshot = false) => { | |
| const results = await Promise.allSettled( | |
| assets.map<Promise<AssetZig>>( | |
| (asset) => | |
| new Promise((resolve, reject) => { | |
| if (asset.points) { | |
| return resolve({ | |
| points: asset.points, | |
| texture_id: asset.textureId || addTexture(asset.url), | |
| id: asset.id || 0, | |
| }) | |
| } | |
| const textureId = addTexture(asset.url, (width, height) => { | |
| // we wait to add image once points are known because otherwise | |
| // if we add img first with "Default" point value and update | |
| // it later once texture is loaded, we will get history snapshot with | |
| // that "default" points | |
| return resolve({ | |
| points: getDefaultPoints(width, height, canvas.clientWidth, canvas.clientHeight), | |
| texture_id: textureId, // if there is no points, then for sure there is no asset.textureId | |
| id: 0, | |
| }) | |
| }) | |
| }) | |
| ) | |
| ) | |
| const zigAssets = results | |
| .filter((result) => result.status === 'fulfilled') | |
| .map((result) => result.value) | |
| reset_assets(zigAssets, withSnapshot) | |
| } | |
| const resetAssets: CreatorAPI['resetAssets'] = async (assets, withSnapshot = false) => { | |
| const results = await Promise.allSettled( | |
| assets.map<Promise<AssetZig>>( | |
| (asset) => | |
| new Promise((resolve, reject) => { | |
| if (asset.points) { | |
| return resolve({ | |
| points: asset.points, | |
| texture_id: asset.textureId || addTexture(asset.url), | |
| id: asset.id || 0, | |
| }) | |
| } | |
| const textureId = addTexture(asset.url, (width, height) => { | |
| try { | |
| return resolve({ | |
| points: getDefaultPoints(width, height, canvas.clientWidth, canvas.clientHeight), | |
| texture_id: textureId, | |
| id: 0, | |
| }) | |
| } catch (error) { | |
| reject(new Error(`Failed to generate points for ${asset.url}: ${error.message}`)) | |
| } | |
| }) | |
| }) | |
| ) | |
| ) | |
| // Log failed assets for debugging | |
| const failedAssets = results.filter(result => result.status === 'rejected') | |
| if (failedAssets.length > 0) { | |
| console.warn(`Failed to load ${failedAssets.length} assets:`, failedAssets.map(r => r.reason)) | |
| } | |
| const zigAssets = results | |
| .filter((result) => result.status === 'fulfilled') | |
| .map((result) => result.value) | |
| reset_assets(zigAssets, withSnapshot) | |
| } |
🤖 Prompt for AI Agents
In src/index.ts around lines 159 to 192, the async logic inside resetAssets uses
Promise.allSettled but does not handle or log errors from rejected promises, and
silently filters out failed assets which can confuse users. To fix this, add
error logging for any rejected promises after Promise.allSettled resolves, and
consider notifying the user about assets that failed to process. Additionally,
implement a mechanism to prevent race conditions if resetAssets is called
multiple times rapidly, such as debouncing calls or tracking ongoing executions
to avoid overlapping resets.
|
🎉 This PR is included in version 0.1.0-next.12 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores
Documentation