[Grida Canvas] Fix Clipboard IO #375
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
WalkthroughThis set of changes introduces a structured images repository to the document schema, expands the editor API with new image and node creation methods, and refactors clipboard and data transfer handling for more robust, type-safe operations. Clipboard encoding/decoding, SVG/image insertion, and editor state reset flows are updated for consistency and extensibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Editor
participant Clipboard
participant ImageRepo
User->>UI: Paste or drop file/SVG/text
UI->>Editor: Call insertFromFile or insertSVG/text
Editor->>Clipboard: Decode data (async)
alt Image file
Editor->>Editor: createImage(src)
Editor->>ImageRepo: Register image ref
Editor->>Editor: createImageNode(imageRef)
else SVG
Editor->>Editor: createNodeFromSvg(svg)
else Text
Editor->>Editor: createTextNode()
end
Editor->>UI: Insert node into document
Suggested labels
Poem
✨ 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: 8
🔭 Outside diff range comments (1)
editor/grida-canvas-react/provider.tsx (1)
1058-1080:⚠️ Potential issueFix memory leak: revoke object URLs after use.
URL.createObjectURLcreates blob URLs that persist until explicitly revoked, causing memory leaks.const insertImage = useCallback( async ( name: string, file: File, position?: { clientX: number; clientY: number; } ) => { const [x, y] = canvasXY( position ? [position.clientX, position.clientY] : [0, 0] ); // TODO: uploader is not implemented. use uploader configured by user. const url = URL.createObjectURL(file); - const image = await instance.createImage(url); - const node = instance.createImageNode(image); - node.$.position = "absolute"; - node.$.name = name; - node.$.left = x; - node.$.top = y; + try { + const image = await instance.createImage(url); + const node = instance.createImageNode(image); + node.$.position = "absolute"; + node.$.name = name; + node.$.left = x; + node.$.top = y; + } finally { + URL.revokeObjectURL(url); + } }, [instance, canvasXY] );
🧹 Nitpick comments (4)
packages/grida-canvas-io/index.ts (1)
18-44: Consider validating the clipboard payload structure.The encode function should validate that the payload has the expected structure before processing.
export function encode( payload: ClipboardPayload ): Record<string, string | Blob> | null { + if (!payload.payload_id || !Array.isArray(payload.prototypes)) { + return null; + } + const result: Record<string, string | Blob> = {}; if (payload.prototypes.length === 0) { return null; }editor/grida-canvas-react/provider.tsx (1)
1293-1300: Provide user feedback on clipboard write failures.Users should be notified when clipboard operations fail.
if (items) { const clipboardItem = new ClipboardItem(items); - navigator.clipboard.write([clipboardItem]); + navigator.clipboard.write([clipboardItem]).catch((error) => { + toast.error('Failed to copy to clipboard'); + console.error('Clipboard write failed:', error); + }); }editor/grida-canvas/editor.ts (2)
112-121: Consider reordering parameters for better ergonomics.The parameter order
(state, key, force)could be confusing sincekeyis optional but placed beforeforce. Consider moving optional parameters to the end:- public reset( - state: editor.state.IEditorState, - key: string | undefined = undefined, - force: boolean = false - ) { + public reset( + state: editor.state.IEditorState, + force: boolean = false, + key: string | undefined = undefined + ) {
1698-1724: Clever proxy implementation - consider adding documentation.The
$getter provides an elegant way to access and mutate node properties. However, the usage pattern might not be immediately obvious to other developers.Consider adding JSDoc documentation to explain the usage:
+ /** + * Returns a proxy that allows direct property access and mutation. + * @example + * const node = editor.getNodeById<TextNode>(id); + * // Read properties + * const text = node.$.text; + * // Update properties + * node.$.fontSize = 16; + */ get $() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
editor/app/(dev)/canvas/tools/io-svg/page.tsx(1 hunks)editor/components/formfield/grida-canvas/index.tsx(1 hunks)editor/grida-canvas-react/provider.tsx(7 hunks)editor/grida-canvas/editor.ts(7 hunks)editor/grida-canvas/index.ts(3 hunks)editor/grida-canvas/plugins/recorder.ts(1 hunks)editor/grida-canvas/reducers/document.reducer.ts(1 hunks)editor/grida-canvas/reducers/node.reducer.ts(1 hunks)editor/scaffolds/playground-canvas/playground.tsx(3 hunks)editor/scaffolds/sidecontrol/controls/positioning.tsx(6 hunks)editor/scaffolds/sidecontrol/sidecontrol-node-selection.tsx(1 hunks)editor/services/new/index.ts(1 hunks)packages/grida-canvas-io-figma/lib.ts(1 hunks)packages/grida-canvas-io/index.ts(3 hunks)packages/grida-canvas-schema/grida.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
editor/grida-canvas/reducers/document.reducer.ts (1)
editor/grida-canvas/reducers/tools/id.ts (1)
nid(10-15)
editor/scaffolds/sidecontrol/controls/positioning.tsx (1)
packages/grida-canvas-schema/grida.ts (1)
IPositioning(1284-1292)
editor/grida-canvas/index.ts (2)
packages/grida-canvas-schema/grida.ts (6)
ImageRef(283-289)NodeID(779-779)ContainerNode(1517-1530)ImageNode(1446-1457)TextNode(1425-1435)RectangleNode(1695-1711)editor/grida-canvas/editor.ts (1)
NodeProxy(1697-1725)
🔇 Additional comments (25)
editor/scaffolds/sidecontrol/sidecontrol-node-selection.tsx (1)
1234-1234: LGTM! Improved event handling semantics.The change from
onValueChangetoonValueCommitimproves user experience by triggering position updates only when users finalize their input rather than on every keystroke or drag event. This reduces unnecessary updates and provides better performance.editor/scaffolds/sidecontrol/controls/positioning.tsx (2)
37-40: LGTM! Well-executed API change.The prop rename from
onValueChangetoonValueCommitcorrectly reflects the semantic change from continuous updates to commit-based updates. The type definition is properly updated to maintain type safety.
51-112: LGTM! Consistent internal usage updates.All internal callback invocations have been properly updated to use
onValueCommitinstead ofonValueChange. The implementation correctly applies the new commit semantics across all positioning constraint inputs (top, left, right, bottom) and the constraint toggles.packages/grida-canvas-io-figma/lib.ts (1)
237-237: LGTM! Consistent schema extension for image support.The addition of the empty
imagesobject alongsidebitmapscorrectly extends the document structure to support the new image repository pattern. This change aligns with the broader schema updates for image handling.editor/services/new/index.ts (1)
259-259: LGTM! Proper initialization of images repository.The addition of the empty
imagesobject in the initial canvas document data structure correctly aligns with the updated schema. This ensures new canvas documents are initialized with the images repository support.editor/grida-canvas/plugins/recorder.ts (1)
95-95: LGTM! Correct adaptation to updated reset method signature.The addition of
undefinedas the second parameter correctly adapts to the newresetmethod signature that includes an optionalkeyparameter. The existing behavior is preserved while conforming to the updated API.editor/app/(dev)/canvas/tools/io-svg/page.tsx (1)
96-101: LGTM! Simplified state reset logic.The replacement of action dispatching with a direct
instance.resetcall simplifies the code and improves clarity. The editor state initialization witheditor.state.initis correct and the approach is more direct than the previous dispatch-based method.editor/grida-canvas/reducers/document.reducer.ts (1)
192-196: LGTM! Improved ID generation logic for prototype insertion.The new conditional ID generation logic is well-designed. It preserves the caller-provided
idfor the root node while generating fresh IDs for children, preventing ID conflicts and offering better control over node creation.editor/components/formfield/grida-canvas/index.tsx (1)
73-79: LGTM! Simplified editor state reset.The refactor from dispatch-based reset to direct
instance.reset()call improves code clarity and aligns with the broader simplification of editor state management across the codebase.editor/scaffolds/playground-canvas/playground.tsx (3)
216-222: LGTM! Consistent with editor state management refactor.The change from dispatch-based reset to direct method call is consistent with the broader refactoring effort to simplify editor state management.
248-254: LGTM! Consistent reset pattern.Another instance of the consistent refactoring from dispatch-based to direct method calls for editor state reset.
744-746: LGTM! Centralized SVG processing through editor API.The change to use
editor.createNodeFromSvg(svg)centralizes SVG processing within the editor instance, removing the need for directiosvgmodule usage. This improves consistency and encapsulation of SVG node creation logic.editor/grida-canvas/reducers/node.reducer.ts (1)
55-79: LGTM! Added positioning property handlers to support enhanced node management.The new positioning property handlers (
position,left,top,right,bottom) follow the established pattern and extend the node reducer's capabilities to support the enhanced positioning features. The simple direct assignment approach is appropriate for these properties.editor/grida-canvas/index.ts (3)
632-632: LGTM: Adding images repository initializationThe addition of an empty
imagesobject follows the same pattern as the existingbitmapsinitialization and aligns with the schema updates to support structured image management.
2058-2064: LGTM: Well-documented async image creation methodThe
createImagemethod is properly documented and typed, returning aPromise<ImageRef>which is appropriate for asynchronous image loading operations.
2100-2114: LGTM: Consistent node creation API expansionThe new node creation methods follow a consistent pattern:
- Proper return type constraints using
NodeProxy<T>- Logical parameter types for each node type
- Clear method naming that reflects their purpose
These additions effectively expand the editor's programmatic node creation capabilities.
packages/grida-canvas-schema/grida.ts (3)
281-299: LGTM! Well-structured image reference schema.The
ImageReftype andIImagesRepositoryinterface follow the established pattern and include all necessary metadata for managing image references.
425-427: Consistent extension of the document interface.The document definition properly extends both repositories following the existing pattern.
956-956: Proper initialization of the images property.The empty object initialization is consistent with the bitmaps property pattern.
packages/grida-canvas-io/index.ts (1)
373-373: Good defensive programming with the images fallback.Adding the fallback for the images property ensures backward compatibility.
editor/grida-canvas/editor.ts (5)
152-159: Good addition of the reduce method.The implementation correctly uses Immer for immutable state updates and properly notifies listeners.
447-452: LGTM!Clean implementation of the deleteNode method.
498-513: Well-structured text node creation.Good defaults with empty text and auto dimensions.
515-533: Appropriate defaults for rectangle creation.The 100x100 dimensions and black fill are reasonable defaults.
400-402: Good improvement to type safety.The generic type parameter allows callers to specify the expected node type, improving type safety throughout the codebase.
| export function filetype( | ||
| file: File | ||
| ): [true, ValidFileType] | [false, string] { | ||
| const type = file.type || file.name.split(".").pop() || file.name; | ||
| if (type === "image/svg+xml") { | ||
| return [true, "image/svg+xml" as const]; | ||
| } else if (type === "image/png") { | ||
| return [true, "image/png" as const]; | ||
| } else if (type === "image/jpeg") { | ||
| return [true, "image/jpeg" as const]; | ||
| } else if (type === "image/gif") { | ||
| return [true, "image/gif" as const]; | ||
| } else { | ||
| return [false, type]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve file type detection logic.
The fallback logic when file.type is missing could return the full filename as a type, which would be incorrect.
- const type = file.type || file.name.split(".").pop() || file.name;
+ const type = file.type || (file.name.includes('.') ? file.name.split(".").pop() : '');
+ if (!type) {
+ return [false, 'unknown'];
+ }📝 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.
| export function filetype( | |
| file: File | |
| ): [true, ValidFileType] | [false, string] { | |
| const type = file.type || file.name.split(".").pop() || file.name; | |
| if (type === "image/svg+xml") { | |
| return [true, "image/svg+xml" as const]; | |
| } else if (type === "image/png") { | |
| return [true, "image/png" as const]; | |
| } else if (type === "image/jpeg") { | |
| return [true, "image/jpeg" as const]; | |
| } else if (type === "image/gif") { | |
| return [true, "image/gif" as const]; | |
| } else { | |
| return [false, type]; | |
| } | |
| } | |
| export function filetype( | |
| file: File | |
| ): [true, ValidFileType] | [false, string] { | |
| const type = file.type || (file.name.includes('.') ? file.name.split('.').pop() : ''); | |
| if (!type) { | |
| return [false, 'unknown']; | |
| } | |
| if (type === "image/svg+xml") { | |
| return [true, "image/svg+xml" as const]; | |
| } else if (type === "image/png") { | |
| return [true, "image/png" as const]; | |
| } else if (type === "image/jpeg") { | |
| return [true, "image/jpeg" as const]; | |
| } else if (type === "image/gif") { | |
| return [true, "image/gif" as const]; | |
| } else { | |
| return [false, type]; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/grida-canvas-io/index.ts around lines 120 to 135, the filetype
function falls back to using the full filename as the type if file.type is
missing, which is incorrect. Update the fallback logic to extract only the file
extension from the filename (e.g., the substring after the last dot) instead of
using the entire filename. This ensures the type detection is based on the file
extension rather than the full filename.
| const item = event.clipboardData.items[i]; | ||
| const payload = await io.clipboard.decode(item); | ||
| items.push(payload); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid silent error catching - log or report errors.
Empty catch blocks hide errors and make debugging difficult.
- } catch {}
+ } catch (error) {
+ console.error('Failed to decode clipboard item:', error);
+ }Also applies to: 1218-1218
🤖 Prompt for AI Agents
In editor/grida-canvas-react/provider.tsx at lines 1165 to 1169 and also line
1218, the catch blocks are empty which silently swallow errors and hinder
debugging. Modify these catch blocks to log the caught errors using
console.error or an appropriate logging mechanism to ensure errors are visible
and can be addressed.
| const node = instance.createTextNode(); | ||
| node.$.name = text; | ||
| node.$.text = text; | ||
| node.$.left = x; | ||
| node.$.top = y; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for node creation methods.
The new node creation methods might fail, but there's no error handling.
const insertText = useCallback(
(
text: string,
position?: {
clientX: number;
clientY: number;
}
) => {
const [x, y] = canvasXY(
position ? [position.clientX, position.clientY] : [0, 0]
);
- const node = instance.createTextNode();
- node.$.name = text;
- node.$.text = text;
- node.$.left = x;
- node.$.top = y;
+ try {
+ const node = instance.createTextNode();
+ if (!node) {
+ throw new Error('Failed to create text node');
+ }
+ node.$.name = text;
+ node.$.text = text;
+ node.$.left = x;
+ node.$.top = y;
+ } catch (error) {
+ toast.error('Failed to insert text');
+ console.error(error);
+ }
},
[instance, canvasXY]
);📝 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 node = instance.createTextNode(); | |
| node.$.name = text; | |
| node.$.text = text; | |
| node.$.left = x; | |
| node.$.top = y; | |
| }, | |
| const insertText = useCallback( | |
| ( | |
| text: string, | |
| position?: { | |
| clientX: number; | |
| clientY: number; | |
| } | |
| ) => { | |
| const [x, y] = canvasXY( | |
| position ? [position.clientX, position.clientY] : [0, 0] | |
| ); | |
| try { | |
| const node = instance.createTextNode(); | |
| if (!node) { | |
| throw new Error('Failed to create text node'); | |
| } | |
| node.$.name = text; | |
| node.$.text = text; | |
| node.$.left = x; | |
| node.$.top = y; | |
| } catch (error) { | |
| toast.error('Failed to insert text'); | |
| console.error(error); | |
| } | |
| }, | |
| [instance, canvasXY] | |
| ); |
🤖 Prompt for AI Agents
In editor/grida-canvas-react/provider.tsx around lines 1048 to 1053, the code
creates a text node without any error handling, which could cause unhandled
exceptions if node creation fails. Wrap the node creation and property
assignments in a try-catch block to catch any errors, and handle them
appropriately, such as logging the error or returning a fallback value, to
ensure the application remains stable.
| } | ||
| } catch (e) { | ||
| // | ||
| reportError(e); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Import or define the reportError function.
The reportError function is used but not imported, which will cause a runtime error.
🏁 Script executed:
#!/bin/bash
# Check if reportError is defined or imported elsewhere
rg -A 5 "reportError" --type ts --type tsxLength of output: 70
Let’s broaden the search across all script and markup file types to be sure:
🏁 Script executed:
#!/bin/bash
# Search for any occurrence or definition of reportError in TS, TSX, JS, and JSX files
rg -Hn --glob "*.{ts,tsx,js,jsx}" -A5 "reportError" .Length of output: 7563
Define or import reportError in provider.tsx
The call to reportError(e) on line 1303 of editor/grida-canvas-react/provider.tsx has no matching definition or import anywhere in the codebase, which will lead to a runtime error.
Please add the appropriate import or define reportError before using it:
• File: editor/grida-canvas-react/provider.tsx
Line 1303:
- reportError(e);
+ reportError(e); // ← ensure `reportError` is imported or definedCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In editor/grida-canvas-react/provider.tsx at line 1303, the function reportError
is called but not imported or defined, causing a runtime error. To fix this,
locate the correct module that exports reportError and add an import statement
for it at the top of provider.tsx, or define the reportError function locally if
it does not exist elsewhere. Ensure the import or definition is placed before
line 1303 so the function is available when called.
| public async createNodeFromSvg( | ||
| svg: string | ||
| ): Promise<NodeProxy<grida.program.nodes.ContainerNode>> { | ||
| const id = this.__createNodeId(); | ||
| const optimized = iosvg.v0.optimize(svg).data; | ||
| let result = await iosvg.v0.convert(optimized, { | ||
| name: "svg", | ||
| currentColor: { r: 0, g: 0, b: 0, a: 1 }, | ||
| }); | ||
| if (result) { | ||
| result = result as grida.program.nodes.i.IPositioning & | ||
| grida.program.nodes.i.IFixedDimension; | ||
|
|
||
| this.insert({ | ||
| id: id, | ||
| prototype: result, | ||
| }); | ||
|
|
||
| return this.getNodeById<grida.program.nodes.ContainerNode>(id); | ||
| } else { | ||
| throw new Error("Failed to convert SVG"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing currentColor and improving type safety.
The hardcoded black color might not be appropriate in all contexts, and the type assertion could be safer.
- public async createNodeFromSvg(
- svg: string
- ): Promise<NodeProxy<grida.program.nodes.ContainerNode>> {
+ public async createNodeFromSvg(
+ svg: string,
+ options: {
+ currentColor?: cg.RGBA;
+ } = {}
+ ): Promise<NodeProxy<grida.program.nodes.ContainerNode>> {
const id = this.__createNodeId();
const optimized = iosvg.v0.optimize(svg).data;
let result = await iosvg.v0.convert(optimized, {
name: "svg",
- currentColor: { r: 0, g: 0, b: 0, a: 1 },
+ currentColor: options.currentColor ?? { r: 0, g: 0, b: 0, a: 1 },
});
if (result) {
- result = result as grida.program.nodes.i.IPositioning &
- grida.program.nodes.i.IFixedDimension;
+ // Ensure result has required properties
+ if (!('x' in result) || !('y' in result) || !('width' in result) || !('height' in result)) {
+ throw new Error("SVG conversion result missing required positioning/dimension properties");
+ }🤖 Prompt for AI Agents
In editor/grida-canvas/editor.ts around lines 454 to 476, the currentColor
parameter is hardcoded to black, which limits flexibility, and the type
assertion on the result is unsafe. Modify the createNodeFromSvg method to accept
currentColor as an optional parameter with a proper type, defaulting to black if
not provided. Replace the unsafe type assertion with a more robust type check or
safer casting to improve type safety.
| async createImage( | ||
| src: string | ||
| ): Promise<Readonly<grida.program.document.ImageRef>> { | ||
| const res = await fetch(src); | ||
| const blob = await res.blob(); | ||
| const bytes = await blob.arrayBuffer(); | ||
| const type = blob.type; | ||
|
|
||
| const { width, height } = await new Promise<{ | ||
| width: number; | ||
| height: number; | ||
| }>((resolve, reject) => { | ||
| const img = new Image(); | ||
| img.onload = () => resolve({ width: img.width, height: img.height }); | ||
| img.onerror = reject; | ||
| img.src = src; | ||
| }); | ||
|
|
||
| const ref: grida.program.document.ImageRef = { | ||
| url: src, | ||
| width, | ||
| height, | ||
| bytes: bytes.byteLength, | ||
| type: type as "image/png" | "image/jpeg" | "image/webp" | "image/gif", | ||
| }; | ||
|
|
||
| this.reduce((state) => { | ||
| state.document.images[src] = ref; | ||
| return state; | ||
| }); | ||
|
|
||
| return ref; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize image fetching and add error handling.
The current implementation has several issues:
- The image is fetched twice (once with
fetch, once withnew Image()) - No error handling for network failures or invalid images
- Type casting at line 255 might not be safe
async createImage(
src: string
): Promise<Readonly<grida.program.document.ImageRef>> {
- const res = await fetch(src);
- const blob = await res.blob();
- const bytes = await blob.arrayBuffer();
- const type = blob.type;
-
- const { width, height } = await new Promise<{
- width: number;
- height: number;
- }>((resolve, reject) => {
- const img = new Image();
- img.onload = () => resolve({ width: img.width, height: img.height });
- img.onerror = reject;
- img.src = src;
- });
-
- const ref: grida.program.document.ImageRef = {
- url: src,
- width,
- height,
- bytes: bytes.byteLength,
- type: type as "image/png" | "image/jpeg" | "image/webp" | "image/gif",
- };
+ try {
+ const res = await fetch(src);
+ if (!res.ok) {
+ throw new Error(`Failed to fetch image: ${res.statusText}`);
+ }
+
+ const blob = await res.blob();
+ const type = blob.type;
+
+ // Validate image type
+ const validTypes = ["image/png", "image/jpeg", "image/webp", "image/gif"];
+ if (!validTypes.includes(type)) {
+ throw new Error(`Unsupported image type: ${type}`);
+ }
+
+ // Create object URL to avoid re-fetching
+ const objectUrl = URL.createObjectURL(blob);
+
+ try {
+ const { width, height } = await new Promise<{
+ width: number;
+ height: number;
+ }>((resolve, reject) => {
+ const img = new Image();
+ img.onload = () => resolve({ width: img.width, height: img.height });
+ img.onerror = () => reject(new Error("Failed to load image"));
+ img.src = objectUrl;
+ });
+
+ const ref: grida.program.document.ImageRef = {
+ url: src,
+ width,
+ height,
+ bytes: blob.size,
+ type: type as "image/png" | "image/jpeg" | "image/webp" | "image/gif",
+ };
+
+ this.reduce((state) => {
+ state.document.images[src] = ref;
+ return state;
+ });
+
+ return ref;
+ } finally {
+ URL.revokeObjectURL(objectUrl);
+ }
+ } catch (error) {
+ throw new Error(`Failed to create image: ${error.message}`);
+ }📝 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.
| async createImage( | |
| src: string | |
| ): Promise<Readonly<grida.program.document.ImageRef>> { | |
| const res = await fetch(src); | |
| const blob = await res.blob(); | |
| const bytes = await blob.arrayBuffer(); | |
| const type = blob.type; | |
| const { width, height } = await new Promise<{ | |
| width: number; | |
| height: number; | |
| }>((resolve, reject) => { | |
| const img = new Image(); | |
| img.onload = () => resolve({ width: img.width, height: img.height }); | |
| img.onerror = reject; | |
| img.src = src; | |
| }); | |
| const ref: grida.program.document.ImageRef = { | |
| url: src, | |
| width, | |
| height, | |
| bytes: bytes.byteLength, | |
| type: type as "image/png" | "image/jpeg" | "image/webp" | "image/gif", | |
| }; | |
| this.reduce((state) => { | |
| state.document.images[src] = ref; | |
| return state; | |
| }); | |
| return ref; | |
| } | |
| async createImage( | |
| src: string | |
| ): Promise<Readonly<grida.program.document.ImageRef>> { | |
| try { | |
| const res = await fetch(src); | |
| if (!res.ok) { | |
| throw new Error(`Failed to fetch image: ${res.statusText}`); | |
| } | |
| const blob = await res.blob(); | |
| const type = blob.type; | |
| // Validate image type | |
| const validTypes = ["image/png", "image/jpeg", "image/webp", "image/gif"]; | |
| if (!validTypes.includes(type)) { | |
| throw new Error(`Unsupported image type: ${type}`); | |
| } | |
| // Create object URL to avoid re-fetching | |
| const objectUrl = URL.createObjectURL(blob); | |
| try { | |
| const { width, height } = await new Promise<{ | |
| width: number; | |
| height: number; | |
| }>((resolve, reject) => { | |
| const img = new Image(); | |
| img.onload = () => resolve({ width: img.width, height: img.height }); | |
| img.onerror = () => reject(new Error("Failed to load image")); | |
| img.src = objectUrl; | |
| }); | |
| const ref: grida.program.document.ImageRef = { | |
| url: src, | |
| width, | |
| height, | |
| bytes: blob.size, | |
| type: type as "image/png" | "image/jpeg" | "image/webp" | "image/gif", | |
| }; | |
| this.reduce((state) => { | |
| state.document.images[src] = ref; | |
| return state; | |
| }); | |
| return ref; | |
| } finally { | |
| URL.revokeObjectURL(objectUrl); | |
| } | |
| } catch (error: any) { | |
| throw new Error(`Failed to create image: ${error.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In editor/grida-canvas/editor.ts lines 232 to 264, the createImage function
fetches the image twice and lacks error handling. Refactor to fetch the image
once using fetch, convert to a blob, and create an object URL for the Image
element to load and get dimensions. Add try-catch blocks to handle network
errors and image loading failures gracefully. Replace unsafe type casting by
validating the blob type against allowed image MIME types before assigning it to
the ref object.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
editor/grida-canvas/editor.ts (2)
232-264: This appears to be a duplicate of a previous review.The image fetching logic has the same issues previously identified: double fetching, lack of error handling, and unsafe type casting.
454-476: This appears to be a duplicate of a previous review.The hardcoded currentColor and unsafe type assertion issues were previously flagged.
packages/grida-canvas-io/index.ts (1)
120-135: This appears to be a duplicate of a previous review.The file type detection logic issues were previously identified.
🧹 Nitpick comments (4)
editor/grida-canvas/editor.ts (3)
497-512: Consider making text content configurable.The text node is created with an empty string, which may not be ideal for all use cases. Consider adding an optional parameter for initial text content.
- public createTextNode(): NodeProxy<grida.program.nodes.TextNode> { + public createTextNode(initialText: string = ""): NodeProxy<grida.program.nodes.TextNode> { const id = this.__createNodeId(); this.dispatch({ type: "insert", id: id, prototype: { type: "text", _$id: id, - text: "", + text: initialText, width: "auto", height: "auto", }, }); return this.getNodeById(id); }
514-532: Consider making rectangle properties configurable.The rectangle is created with hardcoded dimensions and fill color. Consider making these configurable for better flexibility.
- public createRectangleNode(): NodeProxy<grida.program.nodes.RectangleNode> { + public createRectangleNode(options: { + width?: number; + height?: number; + fill?: cg.Paint; + } = {}): NodeProxy<grida.program.nodes.RectangleNode> { const id = this.__createNodeId(); this.dispatch({ type: "insert", id: id, prototype: { type: "rectangle", _$id: id, - width: 100, - height: 100, + width: options.width ?? 100, + height: options.height ?? 100, - fill: { - type: "solid", - color: { r: 0, g: 0, b: 0, a: 1 }, - }, + fill: options.fill ?? { + type: "solid", + color: { r: 0, g: 0, b: 0, a: 1 }, + }, }, }); return this.getNodeById(id); }
1702-1723: Improve error handling in the Proxy setter.The current implementation silently returns false for unknown properties, which may make debugging difficult. Consider logging warnings or providing more specific error information.
get $() { // @ts-expect-error - this is a workaround to allow the proxy to be used as a node return new Proxy(this, { get: (target, prop: string) => { return (target.editor.getNodeSnapshotById(target.node_id) as T)[ prop as keyof T ]; }, set: (target, prop: string, value) => { try { target.editor.dispatch({ type: "node/change/*", node_id: target.node_id, [prop]: value, }); return true; } catch (e) { + if (target.editor.debug) { + console.warn(`Failed to set property ${prop} on node ${target.node_id}:`, e); + } return false; // unknown prop } }, }) as T; }packages/grida-canvas-io/index.ts (1)
46-61: Improve text extraction logic.The current implementation concatenates all text nodes with newlines, which may not preserve the original document structure. Consider adding context about the text node hierarchy.
export function encodeClipboardText( payload: ClipboardPayload ): string | null { let __text_plain = ""; for (const p of payload.prototypes) { if (p.type === "text") { - __text_plain += p.text + "\n"; + // Preserve some context about text structure + const text = p.text?.trim(); + if (text) { + __text_plain += text + "\n"; + } } } if (__text_plain.trim().length > 0) { return __text_plain; } return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
editor/grida-canvas-react/provider.tsx(7 hunks)editor/grida-canvas/editor.ts(7 hunks)packages/grida-canvas-io/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/grida-canvas-react/provider.tsx
🔇 Additional comments (1)
packages/grida-canvas-io/index.ts (1)
383-383: LGTM! Good defensive programming.Ensuring the
imagesproperty is always present prevents potential runtime errors when accessing the document structure.
| export function decode( | ||
| item: DataTransferItem, | ||
| config: { | ||
| noEmptyText: boolean; | ||
| } = { noEmptyText: true } | ||
| ): Promise<DecodedItem | null> { | ||
| return new Promise((resolve, reject) => { | ||
| if (item.kind === "file") { | ||
| const file = item.getAsFile(); | ||
| if (file) { | ||
| const [valid, type] = filetype(file); | ||
|
|
||
| if (valid) { | ||
| return resolve({ type: type, file }); | ||
| } else { | ||
| return reject(new Error(`Unsupported file type: ${type}`)); | ||
| } | ||
| } else { | ||
| return reject(new Error("File is not a valid file")); | ||
| } | ||
| } else if (item.kind === "string" && item.type === "text/plain") { | ||
| item.getAsString((data) => { | ||
| if (config.noEmptyText && data.trim().length === 0) { | ||
| return resolve(null); | ||
| } | ||
| return resolve({ type: "text", text: data }); | ||
| }); | ||
| } else if (item.kind === "string" && item.type === "text/html") { | ||
| item.getAsString((html) => { | ||
| const data = io.clipboard.decodeClipboardHtml(html); | ||
| if (data) { | ||
| return resolve({ type: "clipboard", clipboard: data }); | ||
| } else { | ||
| return reject(new Error("Unknown HTML payload")); | ||
| } | ||
| }); | ||
| } else { | ||
| return resolve(null); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout protection for string decoding.
The getAsString calls could potentially hang if the clipboard data is corrupted. Consider adding timeout protection.
export function decode(
item: DataTransferItem,
config: {
noEmptyText: boolean;
+ timeout?: number;
- } = { noEmptyText: true }
+ } = { noEmptyText: true, timeout: 5000 }
): Promise<DecodedItem | null> {
return new Promise((resolve, reject) => {
+ const timeoutId = setTimeout(() => {
+ reject(new Error('Clipboard decode timeout'));
+ }, config.timeout);
+
+ const cleanup = () => clearTimeout(timeoutId);
+
if (item.kind === "file") {
const file = item.getAsFile();
if (file) {
const [valid, type] = filetype(file);
if (valid) {
+ cleanup();
return resolve({ type: type, file });
} else {
+ cleanup();
return reject(new Error(`Unsupported file type: ${type}`));
}
} else {
+ cleanup();
return reject(new Error("File is not a valid file"));
}
} else if (item.kind === "string" && item.type === "text/plain") {
item.getAsString((data) => {
+ cleanup();
if (config.noEmptyText && data.trim().length === 0) {
return resolve(null);
}
return resolve({ type: "text", text: data });
});
} else if (item.kind === "string" && item.type === "text/html") {
item.getAsString((html) => {
+ cleanup();
const data = io.clipboard.decodeClipboardHtml(html);
if (data) {
return resolve({ type: "clipboard", clipboard: data });
} else {
return reject(new Error("Unknown HTML payload"));
}
});
} else {
+ cleanup();
return resolve(null);
}
});
}📝 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.
| export function decode( | |
| item: DataTransferItem, | |
| config: { | |
| noEmptyText: boolean; | |
| } = { noEmptyText: true } | |
| ): Promise<DecodedItem | null> { | |
| return new Promise((resolve, reject) => { | |
| if (item.kind === "file") { | |
| const file = item.getAsFile(); | |
| if (file) { | |
| const [valid, type] = filetype(file); | |
| if (valid) { | |
| return resolve({ type: type, file }); | |
| } else { | |
| return reject(new Error(`Unsupported file type: ${type}`)); | |
| } | |
| } else { | |
| return reject(new Error("File is not a valid file")); | |
| } | |
| } else if (item.kind === "string" && item.type === "text/plain") { | |
| item.getAsString((data) => { | |
| if (config.noEmptyText && data.trim().length === 0) { | |
| return resolve(null); | |
| } | |
| return resolve({ type: "text", text: data }); | |
| }); | |
| } else if (item.kind === "string" && item.type === "text/html") { | |
| item.getAsString((html) => { | |
| const data = io.clipboard.decodeClipboardHtml(html); | |
| if (data) { | |
| return resolve({ type: "clipboard", clipboard: data }); | |
| } else { | |
| return reject(new Error("Unknown HTML payload")); | |
| } | |
| }); | |
| } else { | |
| return resolve(null); | |
| } | |
| }); | |
| } | |
| export function decode( | |
| item: DataTransferItem, | |
| config: { | |
| noEmptyText: boolean; | |
| timeout?: number; | |
| } = { noEmptyText: true, timeout: 5000 } | |
| ): Promise<DecodedItem | null> { | |
| return new Promise((resolve, reject) => { | |
| const timeoutId = setTimeout(() => { | |
| reject(new Error('Clipboard decode timeout')); | |
| }, config.timeout); | |
| const cleanup = () => clearTimeout(timeoutId); | |
| if (item.kind === "file") { | |
| const file = item.getAsFile(); | |
| if (file) { | |
| const [valid, type] = filetype(file); | |
| if (valid) { | |
| cleanup(); | |
| return resolve({ type: type, file }); | |
| } else { | |
| cleanup(); | |
| return reject(new Error(`Unsupported file type: ${type}`)); | |
| } | |
| } else { | |
| cleanup(); | |
| return reject(new Error("File is not a valid file")); | |
| } | |
| } else if (item.kind === "string" && item.type === "text/plain") { | |
| item.getAsString((data) => { | |
| cleanup(); | |
| if (config.noEmptyText && data.trim().length === 0) { | |
| return resolve(null); | |
| } | |
| return resolve({ type: "text", text: data }); | |
| }); | |
| } else if (item.kind === "string" && item.type === "text/html") { | |
| item.getAsString((html) => { | |
| cleanup(); | |
| const data = io.clipboard.decodeClipboardHtml(html); | |
| if (data) { | |
| return resolve({ type: "clipboard", clipboard: data }); | |
| } else { | |
| return reject(new Error("Unknown HTML payload")); | |
| } | |
| }); | |
| } else { | |
| cleanup(); | |
| return resolve(null); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/grida-canvas-io/index.ts around lines 186 to 226, the decode
function uses getAsString callbacks for string data without timeout protection,
which may cause the promise to hang indefinitely if the clipboard data is
corrupted. To fix this, wrap the getAsString calls in a timeout mechanism that
rejects or resolves the promise if the callback does not complete within a
reasonable time (e.g., a few seconds), ensuring the decode function always
settles.
| export function encode( | ||
| payload: ClipboardPayload | ||
| ): Record<string, string | Blob> | null { | ||
| const result: Record<string, string | Blob> = {}; | ||
|
|
||
| if (payload.prototypes.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| // text/html (grida) | ||
| const __main_html = encodeClipboardHtml(payload); | ||
| const utf8Html = new TextEncoder().encode(__main_html); | ||
| result["text/html"] = new Blob([utf8Html], { | ||
| type: "text/html;charset=utf-8", | ||
| }); | ||
|
|
||
| // text/plain (universal) | ||
| const __text_plain = encodeClipboardText(payload); | ||
| if (__text_plain) { | ||
| const utf8 = new TextEncoder().encode(__text_plain); | ||
| result["text/plain"] = new Blob([utf8], { | ||
| type: "text/plain;charset=utf-8", | ||
| }); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the encode function.
The function should validate the payload structure to prevent runtime errors when accessing properties.
export function encode(
payload: ClipboardPayload
): Record<string, string | Blob> | null {
+ if (!payload || typeof payload !== 'object' || !Array.isArray(payload.prototypes)) {
+ throw new Error('Invalid clipboard payload structure');
+ }
+
const result: Record<string, string | Blob> = {};
if (payload.prototypes.length === 0) {
return null;
}📝 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.
| export function encode( | |
| payload: ClipboardPayload | |
| ): Record<string, string | Blob> | null { | |
| const result: Record<string, string | Blob> = {}; | |
| if (payload.prototypes.length === 0) { | |
| return null; | |
| } | |
| // text/html (grida) | |
| const __main_html = encodeClipboardHtml(payload); | |
| const utf8Html = new TextEncoder().encode(__main_html); | |
| result["text/html"] = new Blob([utf8Html], { | |
| type: "text/html;charset=utf-8", | |
| }); | |
| // text/plain (universal) | |
| const __text_plain = encodeClipboardText(payload); | |
| if (__text_plain) { | |
| const utf8 = new TextEncoder().encode(__text_plain); | |
| result["text/plain"] = new Blob([utf8], { | |
| type: "text/plain;charset=utf-8", | |
| }); | |
| } | |
| return result; | |
| } | |
| export function encode( | |
| payload: ClipboardPayload | |
| ): Record<string, string | Blob> | null { | |
| if (!payload || typeof payload !== 'object' || !Array.isArray(payload.prototypes)) { | |
| throw new Error('Invalid clipboard payload structure'); | |
| } | |
| const result: Record<string, string | Blob> = {}; | |
| if (payload.prototypes.length === 0) { | |
| return null; | |
| } | |
| // text/html (grida) | |
| const __main_html = encodeClipboardHtml(payload); | |
| const utf8Html = new TextEncoder().encode(__main_html); | |
| result["text/html"] = new Blob([utf8Html], { | |
| type: "text/html;charset=utf-8", | |
| }); | |
| // text/plain (universal) | |
| const __text_plain = encodeClipboardText(payload); | |
| if (__text_plain) { | |
| const utf8 = new TextEncoder().encode(__text_plain); | |
| result["text/plain"] = new Blob([utf8], { | |
| type: "text/plain;charset=utf-8", | |
| }); | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In packages/grida-canvas-io/index.ts around lines 18 to 44, the encode function
lacks input validation for the payload parameter, which may cause runtime errors
if the payload structure is incorrect. Add validation at the start of the
function to check that payload is an object and that payload.prototypes is an
array before accessing its length. If validation fails, return null or handle
the error gracefully to prevent runtime exceptions.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation