fix: center dropped image and svg insertions#700
Open
k08200 wants to merge 5 commits intogridaco:mainfrom
Open
fix: center dropped image and svg insertions#700k08200 wants to merge 5 commits intogridaco:mainfrom
k08200 wants to merge 5 commits intogridaco:mainfrom
Conversation
|
@k08200 is attempting to deploy a commit to the Grida Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a client->canvas coordinate utility and tests, extends the data-transfer hook to compute canvas drop points and center inserted images/SVGs using resolved dimensions, and makes insertion reducer placement optional via a new action field. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client
participant Hook as use-data-transfer
participant Xform as clientPointToCanvasPoint
participant Loader as Image/Metadata Resolver
participant Doc as Document/Canvas
User->>Hook: drop at clientPosition (file/metadata)
Hook->>Xform: clientPointToCanvasPoint(clientPosition)
Xform-->>Hook: canvasPosition
Hook->>Loader: resolve width/height (if needed)
Loader-->>Hook: size {width,height}
Hook->>Hook: compute half-offsets (ignore invalid)
Hook->>Doc: insertImageRectangle(position = canvasPosition - halfOffsets, size)
Doc-->>Hook: nodeCreated/selected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
editor/grida-canvas-react/__tests__/data-transfer-position.test.ts (1)
4-24: ⚡ Quick winAdd a regression that exercises the real drop flow.
These tests prove the helper math, but they will not catch wiring mistakes in
editor/grida-canvas-react/use-data-transfer.tssuch as using the wrong event coordinates or choosing the wrong dimensions for inline image drops. Please add either a hook-level regression or the matching../test/manual case for image/SVG drop under zoom/pan. Based on learnings: When checking UX bugs or changing interaction behavior ingrida-canvas*, check../test/for related manual test cases and add new ones when appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-react/__tests__/data-transfer-position.test.ts` around lines 4 - 24, Add a hook-level regression (or a new manual case in ../test/) that exercises the real drop flow through use-data-transfer.ts rather than only unit-testing getCenteredCanvasInsertionPoint: simulate an actual drop of an inline image/SVG under non-identity zoom/pan, dispatch real DragEvent/DropEvent objects carrying a file/dataTransfer and set clientX/clientY (and pageX/pageY if needed), ensure the component/hook receives the event and converts coordinates via clientPointToCanvasPoint, and assert the created element uses the converted canvas position and the measured (or fallback) dimensions; this will catch wiring mistakes like using wrong event coords or wrong dimension sources in use-data-transfer.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@editor/grida-canvas-react/use-data-transfer.ts`:
- Around line 604-620: The width/height fallbacks currently use nullish
coalescing so invalid numeric values like 0, NaN or Infinity from the payload
can become the node size; when computing imageWidth/imageHeight before calling
getCenteredCanvasInsertionPoint and creating the node, validate the payload
values with a positive-finite guard (e.g., Number.isFinite(value) && value > 0)
and only use payload width/height if they pass, otherwise fall back to
imageRef.width/imageRef.height; keep the rest of the flow
(getCenteredCanvasInsertionPoint, instance.commands.createRectangleNode, and
setting layout_* properties) unchanged.
---
Nitpick comments:
In `@editor/grida-canvas-react/__tests__/data-transfer-position.test.ts`:
- Around line 4-24: Add a hook-level regression (or a new manual case in
../test/) that exercises the real drop flow through use-data-transfer.ts rather
than only unit-testing getCenteredCanvasInsertionPoint: simulate an actual drop
of an inline image/SVG under non-identity zoom/pan, dispatch real
DragEvent/DropEvent objects carrying a file/dataTransfer and set clientX/clientY
(and pageX/pageY if needed), ensure the component/hook receives the event and
converts coordinates via clientPointToCanvasPoint, and assert the created
element uses the converted canvas position and the measured (or fallback)
dimensions; this will catch wiring mistakes like using wrong event coords or
wrong dimension sources in use-data-transfer.ts.
🪄 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
Run ID: bb7ff988-5e6f-4c4a-85f2-7659e2e846c3
📒 Files selected for processing (3)
editor/grida-canvas-react/__tests__/data-transfer-position.test.tseditor/grida-canvas-react/data-transfer-position.tseditor/grida-canvas-react/use-data-transfer.ts
Contributor
Author
|
Behavior verification update: I ran a temporary browser harness against the actual |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
editor/grida-canvas-react/use-data-transfer.ts (1)
650-651:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate dropped image dimensions before applying them to node size.
Line 650 and Line 651 still allow
0,NaN, orInfinityfrom the transfer payload via??, which can create invalid/invisible nodes at Line 665 and Line 666. Use a positive-finite guard before reusing payload dimensions.Proposed fix
- const imageWidth = width ?? imageRef.width; - const imageHeight = height ?? imageRef.height; + const imageWidth = + Number.isFinite(width) && width > 0 ? width : imageRef.width; + const imageHeight = + Number.isFinite(height) && height > 0 + ? height + : imageRef.height;Also applies to: 665-666
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-react/use-data-transfer.ts` around lines 650 - 651, The image dimension assignment (imageWidth = width ?? imageRef.width; imageHeight = height ?? imageRef.height) can accept 0/NaN/Infinity from the transfer payload; change these to guard for positive finite numbers before using payload values (e.g. use Number.isFinite(width) && width > 0 ? width : imageRef.width, and similarly for height) and apply the same positive-finite guard where node size is set later (the code around setting node width/height at the lines referenced). This ensures imageWidth/imageHeight and subsequent node size assignments only use valid positive finite values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@editor/grida-canvas-react/use-data-transfer.ts`:
- Around line 650-651: The image dimension assignment (imageWidth = width ??
imageRef.width; imageHeight = height ?? imageRef.height) can accept
0/NaN/Infinity from the transfer payload; change these to guard for positive
finite numbers before using payload values (e.g. use Number.isFinite(width) &&
width > 0 ? width : imageRef.width, and similarly for height) and apply the same
positive-finite guard where node size is set later (the code around setting node
width/height at the lines referenced). This ensures imageWidth/imageHeight and
subsequent node size assignments only use valid positive finite values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3ab3dd3-481a-47b3-bc51-b571d8484f7e
📒 Files selected for processing (1)
editor/grida-canvas-react/use-data-transfer.ts
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 (1)
editor/grida-canvas-react/use-data-transfer.ts (1)
659-664:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait
insertSVGand reject non-OK fetches in the SVG drop task.At Line 659–Line 664, the promise chain resolves even if
insertSVGlater fails, sotoast.promisecan report success prematurely. Also, non-2xx responses are not treated as errors.Proposed fix
- const task = fetch(src, { - cache: "no-store", - }).then((res) => - res.text().then((text) => { - insertSVG(name, text, dropPosition); - }) - ); + const task = fetch(src, { cache: "no-store" }).then(async (res) => { + if (!res.ok) { + throw new Error(`Failed to fetch SVG (${res.status})`); + } + const text = await res.text(); + await insertSVG(name, text, dropPosition); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-react/use-data-transfer.ts` around lines 659 - 664, The task promise currently treats any fetch response as success and doesn't await insertSVG, causing toast.promise to resolve prematurely; update the fetch chain used to create the task (the const task = fetch(src, { cache: "no-store" }) ... ) to check res.ok and throw on non-2xx responses, await res.text(), then await insertSVG(name, text, dropPosition) and propagate any thrown errors so the task rejects on fetch or insert failures.
♻️ Duplicate comments (1)
editor/grida-canvas-react/use-data-transfer.ts (1)
677-678:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate payload dimensions before using them as inserted image size.
At Line 677 and Line 678, nullish fallback still accepts
0,NaN, andInfinity, which can produce invalid rectangle dimensions (even though centering logic guards half-size math).Proposed fix
- const imageWidth = width ?? imageRef.width; - const imageHeight = height ?? imageRef.height; + const imageWidth = + Number.isFinite(width) && width > 0 ? width : imageRef.width; + const imageHeight = + Number.isFinite(height) && height > 0 + ? height + : imageRef.height;Also applies to: 679-690
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-react/use-data-transfer.ts` around lines 677 - 678, The code currently assigns imageWidth = width ?? imageRef.width and imageHeight = height ?? imageRef.height which still allows 0, NaN, or Infinity to be used; update the insertion-dimension logic (the variables imageWidth/imageHeight and the same logic used in the block handling payload dimensions around lines 679-690) to validate and sanitize the incoming width/height: accept the payload value only if Number.isFinite(value) and value > 0, otherwise fall back to imageRef.width/imageRef.height (or a safe positive default), and ensure the centering/rect math uses these sanitized values to avoid invalid rectangle sizes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@editor/grida-canvas-react/use-data-transfer.ts`:
- Around line 659-664: The task promise currently treats any fetch response as
success and doesn't await insertSVG, causing toast.promise to resolve
prematurely; update the fetch chain used to create the task (the const task =
fetch(src, { cache: "no-store" }) ... ) to check res.ok and throw on non-2xx
responses, await res.text(), then await insertSVG(name, text, dropPosition) and
propagate any thrown errors so the task rejects on fetch or insert failures.
---
Duplicate comments:
In `@editor/grida-canvas-react/use-data-transfer.ts`:
- Around line 677-678: The code currently assigns imageWidth = width ??
imageRef.width and imageHeight = height ?? imageRef.height which still allows 0,
NaN, or Infinity to be used; update the insertion-dimension logic (the variables
imageWidth/imageHeight and the same logic used in the block handling payload
dimensions around lines 679-690) to validate and sanitize the incoming
width/height: accept the payload value only if Number.isFinite(value) and value
> 0, otherwise fall back to imageRef.width/imageRef.height (or a safe positive
default), and ensure the centering/rect math uses these sanitized values to
avoid invalid rectangle sizes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b103a02-f7d4-4357-8e2f-a258a888a26e
📒 Files selected for processing (1)
editor/grida-canvas-react/use-data-transfer.ts
Contributor
Author
2026-05-01.12.30.51.movGOOD!!!!!! |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
editor/grida-canvas-react/use-data-transfer.ts (1)
665-681:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate payload dimensions before reusing them.
width ?? imageRef.widthandheight ?? imageRef.heightstill allow0,NaN, orInfinityto flow through, so a malformed drop payload can create a zero-sized image and center it incorrectly.Suggested fix
- const imageWidth = width ?? imageRef.width; - const imageHeight = height ?? imageRef.height; + const imageWidth = + Number.isFinite(width) && width > 0 ? width : imageRef.width; + const imageHeight = + Number.isFinite(height) && height > 0 + ? height + : imageRef.height;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-react/use-data-transfer.ts` around lines 665 - 681, The width/height fallbacks allow 0, NaN, or Infinity through; before computing imageWidth/imageHeight in the async task, validate the incoming data.width and data.height and the imageRef.width/imageRef.height using finite and positive checks (e.g., Number.isFinite(...) && > 0) so that imageWidth = valid(data.width) ? data.width : valid(imageRef.width) ? imageRef.width : a sane default, and likewise for imageHeight; then pass those validated values into centerCanvasPoint and insertImageRectangle (referencing createImageAsync, centerCanvasPoint, and insertImageRectangle to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@editor/grida-canvas-react/use-data-transfer.ts`:
- Around line 665-681: The width/height fallbacks allow 0, NaN, or Infinity
through; before computing imageWidth/imageHeight in the async task, validate the
incoming data.width and data.height and the imageRef.width/imageRef.height using
finite and positive checks (e.g., Number.isFinite(...) && > 0) so that
imageWidth = valid(data.width) ? data.width : valid(imageRef.width) ?
imageRef.width : a sane default, and likewise for imageHeight; then pass those
validated values into centerCanvasPoint and insertImageRectangle (referencing
createImageAsync, centerCanvasPoint, and insertImageRectangle to locate the
code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a3141ad-d0a0-4957-a353-d02531a95114
📒 Files selected for processing (4)
editor/grida-canvas-react/use-data-transfer.tseditor/grida-canvas/__tests__/headless/node-properties.test.tseditor/grida-canvas/action.tseditor/grida-canvas/reducers/document.reducer.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Note: pnpm --dir editor typecheck currently fails on unrelated missing static image modules under app/(www), app/(site), and components/stripe.
Closes #366
Summary by CodeRabbit
New Features
Bug Fixes
Tests