day309 Grida Canvas - UX / Photos Library#467
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a unified Library UI that combines Icons and Unsplash-backed Photos browsing/insert/drag behavior, new UI primitives (Pills, SearchInput, LoadingIndicator), typed data-transfer payloads for images/SVGs, server-side Unsplash fetch utilities, and updates to floating-window demos and drop handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Library as Library UI
participant Photos as PhotosBrowser
participant API as lib-photos-actions (server)
participant DataTransfer as datatransfer
participant Editor as Canvas / use-data-transfer
User->>Library: Open Photos tab
Library->>Photos: mount with onInsert/onDragStart
Photos->>API: fetchPhotosAction(mode/search/random)
API->>Unsplash: HTTP request
Unsplash-->>API: Response
API-->>Photos: normalized PhotoActionState
Photos->>Photos: render virtualized grid
User->>Photos: Drag photo
Photos->>DataTransfer: encode({type:"image",name,src,width,height})
User->>Editor: Drop payload
Editor->>DataTransfer: decode(payload)
Editor->>Editor: create rect node + image fill (size, transforms)
Editor-->>User: show toast / insertion result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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. Comment |
…and Photos, including drag-and-drop functionality and search capabilities.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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)
581-673: Fixondropreliability: deps, decode failure handling, and fetch non-2xx handling.This path can currently throw (decode) and can report success on failed HTTP responses; also
instanceis used but not in the deps array.const ondrop = useCallback( (event: React.DragEvent<HTMLDivElement>) => { event.preventDefault(); const knwondata = event.dataTransfer.getData("x-grida-data-transfer"); if (knwondata) { - const data = datatransfer.decode(knwondata); + let data: datatransfer.DataTransferPayload; + try { + data = datatransfer.decode(knwondata); + } catch { + toast.error("Invalid drag payload"); + return; + } switch (data.type) { - case "svg": + case "svg": { const { name, src } = data; - const task = fetch(src, { + const task = fetch(src, { cache: "no-store", - }).then((res) => - res.text().then((text) => { - insertSVG(name, text, event); - }) - ); + }).then(async (res) => { + if (!res.ok) throw new Error(`Failed to fetch SVG (${res.status})`); + const text = await res.text(); + insertSVG(name, text, event); + }); toast.promise(task, { loading: "Loading...", success: "Inserted", error: "Failed to insert SVG", }); break; + } case "image": { const { name, src, width, height } = data; const task = (async () => { const imageRef = await instance.createImageAsync(src); const [x, y] = instance.camera.clientPointToCanvasPoint([ event.clientX, event.clientY, ]); const node = instance.commands.createRectangleNode(); node.$.position = "absolute"; node.$.name = name || "Photo"; node.$.left = x; node.$.top = y; node.$.width = width || imageRef.width; node.$.height = height || imageRef.height; node.$.fill_paints = [ { type: "image", src: imageRef.url, fit: "cover", transform: cmath.transform.identity, filters: cg.def.IMAGE_FILTERS, blend_mode: cg.def.BLENDMODE, opacity: 1, active: true, } satisfies cg.ImagePaint, ]; })(); toast.promise(task, { loading: "Loading image...", success: "Image inserted", error: "Failed to insert image", }); break; } // ... } // ... }, - [insertFromFile, insertSVG] + [insertFromFile, insertSVG, instance] );(If you want this even safer: validate
srcscheme/host before fetching/creating images.)
🧹 Nitpick comments (12)
editor/package.json (1)
221-221: Pinunsplash-jsmore tightly (and confirm key handling stays server-side).
^7.0.20can float; consider pinning (or at least~7.0.20) if you want to reduce surprise API changes.Also please verify any Unsplash access key usage is server-only (e.g., Route Handler / Server Action) and not embedded into client bundles.
- "unsplash-js": "^7.0.20", + "unsplash-js": "~7.0.20",editor/grida-canvas-hosted/library/components/loading-indicator.tsx (1)
12-22: Looks good; consider hiding from a11y tree whenloadingis false.If
Progressannounces itself, you may wantaria-hidden={!loading}(or conditional render) instead of opacity-only hiding.editor/grida-canvas-hosted/library/components/search-input.tsx (1)
19-39: Usecn()for className composition instead of template string.import React from "react"; +import { cn } from "@/components/lib/utils"; // ... export function SearchInput({ // ... }: SearchInputProps) { return ( - <InputGroup className={`h-7 ${className || ""}`}> + <InputGroup className={cn("h-7", className)}> <InputGroupAddon align="inline-start" className="ps-2"> <SearchIcon className="size-3" /> </InputGroupAddon>editor/grida-canvas-hosted/library/library.tsx (2)
15-39: LGTM! Clean TabButton component.The TabButton component is well-structured with appropriate accessibility and styling. The
valueprop is defined but unused - consider removing it if it's not needed for future identification purposes.function TabButton({ - value, active, onClick, children, }: { - value: string; active: boolean; onClick: () => void; children: React.ReactNode; }) {
88-105: Silent failure when no image URL is available for drag.When
imageUrlis falsy, the drag operation silently fails without any data transfer. Consider logging a warning or providing fallback behavior to help debug issues.const handlePhotoDragStart = useCallback( (photo: PhotoAsset, event: React.DragEvent<HTMLElement>) => { const imageUrl = photo.urls.regular || photo.urls.full || photo.urls.raw; if (imageUrl) { event.dataTransfer.setData( datatransfer.key, datatransfer.encode({ type: "image", name: photo.alt || "Photo", src: imageUrl, width: photo.width, height: photo.height, }) ); + } else { + console.warn("Photo drag started but no valid URL available", photo.id); } }, [] );editor/grida-canvas-hosted/library/components/pills.tsx (2)
26-53: Consider adding type="button" for explicit button behavior.While not strictly required, explicitly setting
type="button"prevents accidental form submissions if this component is ever used inside a form.<button + type="button" data-has-thumbnail={!!thumbnail} className={cn(
40-49: Add error handling for Image component.The component is properly set up as a Client Component (
'use client'), which allows using theonErrorhandler. Since thumbnail URLs may fail to load, consider adding anonErrorhandler to display a fallback state or gracefully handle load failures. Example approach: track fallback state usinguseStateto avoid infinite retry loops.editor/grida-canvas-hosted/library/lib-icons.ts (2)
77-131: Consider stricter typing for API response normalization.The
normalizeIconsfunction usesany[]for the raw list parameter. While this is pragmatic for handling varied API responses, consider defining a loose input type to document expected fields and improve IDE support.+type RawIconItem = { + download?: string; + vendor?: string; + host?: string; + family?: string; + name?: string; + title?: string; + id?: string; + variant?: string; + style?: string; + size?: number; + properties?: { + style?: string; + variant?: string; + size?: number; + }; + tags?: string[]; + keywords?: string[]; +}; const normalizeIcons = ( - rawList: any[], + rawList: RawIconItem[], allowedVendors?: Set<IconVendorId> ): IconsBrowserItem[] => {
168-202: Consider adding request timeout for icon fetching.The
fetchIconsfunction usescache: "no-store"which is appropriate for fresh data, but there's no timeout handling. Long-running requests could block the UI.export async function fetchIcons({ vendor, variants, allowedVendors, + signal, }: { vendor?: IconVendorId | null; variants?: IconVariantFilters; allowedVendors?: Iterable<IconVendorId>; + signal?: AbortSignal; }): Promise<IconsBrowserItem[]> { const params = new URLSearchParams(); // ... - const res = await fetch(url, { cache: "no-store" }); + const res = await fetch(url, { cache: "no-store", signal });This allows callers to implement timeout behavior via AbortController.
editor/grida-canvas-hosted/library/icons-browser.tsx (2)
82-99: Consider memoizingallowedVendorsset.The
allowedVendorsset is recreated on everyfetchIconscall. While the impact is minimal, it could be memoized outside the callback since it only depends onvendors.+ const allowedVendors = useMemo( + () => new Set(vendors.map((v) => v.vendor)), + [vendors] + ); + const fetchIcons = useCallback(async () => { if (!vendorsLoaded) return; setLoading(true); setError(null); try { - const allowedVendors = new Set(vendors.map((v) => v.vendor)); const list = await fetchIconsFromApi({ vendor: filters.vendor, variants: filters.variants, allowedVendors, }); setIcons(list); } catch (e) { setError(e instanceof Error ? e.message : "Failed to load icons"); } finally { setLoading(false); } - }, [filters.vendor, filters.variants, vendorsLoaded, vendors]); + }, [filters.vendor, filters.variants, vendorsLoaded, allowedVendors]);
367-373:handleInsertcorrectly handles asynconInsert.The callback properly awaits the
onInsertpromise. However, there's no loading feedback or error handling during insertion. Consider adding try-catch if insertion errors should be surfaced to the user.editor/grida-canvas-hosted/library/lib-photos-actions.ts (1)
186-198: Pagination metadata may be misleading after filtering.The
totalandtotalPagesreflect API totals before filtering premium photos. AfterfilterFreePhotos, the actualresults.lengthcould be significantly less thanperPage, and the true total of free photos is unknown. Consider adding a comment or documenting this limitation for consumers.The current comment on lines 186-187 acknowledges this, which is good. Consumers should be aware that pagination may appear inconsistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
editor/app/(dev)/ui/floating-window/page.tsx(3 hunks)editor/components/logos/unsplash.tsx(1 hunks)editor/grida-canvas-hosted/library/components/loading-indicator.tsx(1 hunks)editor/grida-canvas-hosted/library/components/pills.tsx(1 hunks)editor/grida-canvas-hosted/library/components/search-input.tsx(1 hunks)editor/grida-canvas-hosted/library/icons-browser.tsx(10 hunks)editor/grida-canvas-hosted/library/lib-icons.ts(1 hunks)editor/grida-canvas-hosted/library/lib-photos-actions.ts(1 hunks)editor/grida-canvas-hosted/library/library.tsx(1 hunks)editor/grida-canvas-hosted/library/photos-browser.tsx(1 hunks)editor/grida-canvas-hosted/playground/playground.tsx(6 hunks)editor/grida-canvas-react/use-data-transfer.ts(3 hunks)editor/grida-canvas/data-transfer.ts(1 hunks)editor/next.config.ts(1 hunks)editor/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
editor/grida-canvas-hosted/library/components/loading-indicator.tsxeditor/grida-canvas/data-transfer.tseditor/grida-canvas-hosted/library/library.tsxeditor/next.config.tseditor/grida-canvas-react/use-data-transfer.tseditor/grida-canvas-hosted/library/components/search-input.tsxeditor/app/(dev)/ui/floating-window/page.tsxeditor/components/logos/unsplash.tsxeditor/grida-canvas-hosted/library/components/pills.tsxeditor/grida-canvas-hosted/library/photos-browser.tsxeditor/grida-canvas-hosted/library/lib-photos-actions.tseditor/grida-canvas-hosted/library/lib-icons.tseditor/grida-canvas-hosted/library/icons-browser.tsxeditor/grida-canvas-hosted/playground/playground.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React.js 19 for web UI development
Files:
editor/grida-canvas-hosted/library/components/loading-indicator.tsxeditor/grida-canvas-hosted/library/library.tsxeditor/grida-canvas-hosted/library/components/search-input.tsxeditor/app/(dev)/ui/floating-window/page.tsxeditor/components/logos/unsplash.tsxeditor/grida-canvas-hosted/library/components/pills.tsxeditor/grida-canvas-hosted/library/photos-browser.tsxeditor/grida-canvas-hosted/library/icons-browser.tsxeditor/grida-canvas-hosted/playground/playground.tsx
**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Tailwind CSS 4 for styling
Files:
editor/grida-canvas-hosted/library/components/loading-indicator.tsxeditor/grida-canvas/data-transfer.tseditor/grida-canvas-hosted/library/library.tsxeditor/next.config.tseditor/grida-canvas-react/use-data-transfer.tseditor/grida-canvas-hosted/library/components/search-input.tsxeditor/app/(dev)/ui/floating-window/page.tsxeditor/components/logos/unsplash.tsxeditor/grida-canvas-hosted/library/components/pills.tsxeditor/grida-canvas-hosted/library/photos-browser.tsxeditor/grida-canvas-hosted/library/lib-photos-actions.tseditor/grida-canvas-hosted/library/lib-icons.tseditor/grida-canvas-hosted/library/icons-browser.tsxeditor/grida-canvas-hosted/playground/playground.tsx
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Shadcn UI for reusable UI components
Files:
editor/grida-canvas-hosted/library/components/loading-indicator.tsxeditor/grida-canvas-hosted/library/components/search-input.tsxeditor/components/logos/unsplash.tsxeditor/grida-canvas-hosted/library/components/pills.tsx
{editor,apps}/**/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Next.js 15 as the web framework
Files:
editor/app/(dev)/ui/floating-window/page.tsx
🧠 Learnings (9)
📚 Learning: 2025-12-01T00:21:48.564Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Applies to **/components/**/*.{ts,tsx} : Use Shadcn UI for reusable UI components
Applied to files:
editor/grida-canvas-hosted/library/components/loading-indicator.tsxeditor/grida-canvas-hosted/library/components/search-input.tsxeditor/grida-canvas-hosted/library/icons-browser.tsxeditor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Applied to files:
editor/grida-canvas/data-transfer.tseditor/grida-canvas-react/use-data-transfer.tseditor/grida-canvas-hosted/library/components/pills.tsxeditor/grida-canvas-hosted/library/lib-icons.tseditor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:21:48.564Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Applies to editor/lib/**/*.{ts,tsx} : Keep /lib modules in the /editor directory strictly designed, non-opinionated, reusable, and stable for promotion to /packages directory
Applied to files:
editor/grida-canvas-hosted/library/library.tsxeditor/grida-canvas-hosted/library/lib-icons.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Cache ImageData and dimensions in refs (imageDataRef, sizeRef) for efficient exports
Applied to files:
editor/grida-canvas-react/use-data-transfer.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use React hooks for state management (imageSrc, shape, grid, maxRadius, gamma, jitter, opacity, color, customShapeImage, imageDataRef, sizeRef)
Applied to files:
editor/grida-canvas-react/use-data-transfer.tseditor/app/(dev)/ui/floating-window/page.tsxeditor/grida-canvas-hosted/library/components/pills.tsxeditor/grida-canvas-hosted/library/photos-browser.tsxeditor/grida-canvas-hosted/library/icons-browser.tsxeditor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Custom images used as halftone shapes should be loaded as HTMLImageElement for efficient canvas rendering and preserve original image colors in as-is mode
Applied to files:
editor/grida-canvas-react/use-data-transfer.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : When adding new shape types, update the Shape type union, add cases in drawShape() function, add cases in shapeToSVG() function, and add SelectItem in UI
Applied to files:
editor/app/(dev)/ui/floating-window/page.tsxeditor/grida-canvas-hosted/library/components/pills.tsx
📚 Learning: 2025-12-01T00:21:48.564Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Applies to **/*.{ts,tsx} : Use Lucide or Radix Icons for icons
Applied to files:
editor/grida-canvas-hosted/library/lib-icons.tseditor/grida-canvas-hosted/library/icons-browser.tsxeditor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:21:48.564Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Applies to **/*.{tsx,jsx} : Use React.js 19 for web UI development
Applied to files:
editor/grida-canvas-hosted/library/icons-browser.tsx
🧬 Code graph analysis (4)
editor/grida-canvas-hosted/library/components/search-input.tsx (1)
editor/components/ui/input-group.tsx (3)
InputGroup(164-164)InputGroupAddon(165-165)InputGroupInput(168-168)
editor/app/(dev)/ui/floating-window/page.tsx (4)
editor/grida-canvas-hosted/library/icons-browser.tsx (2)
IconsBrowserItem(48-48)IconsBrowser(341-528)editor/grida-canvas-hosted/library/photos-browser.tsx (1)
PhotosBrowser(417-417)editor/components/floating-window/floating-window.tsx (3)
FloatingWindowTrigger(83-85)FloatingWindowBody(74-81)FloatingWindowRoot(47-59)editor/components/floating-window/primitives.tsx (3)
FloatingWindowTrigger(425-464)FloatingWindowBody(514-530)FloatingWindowRoot(153-356)
editor/grida-canvas-hosted/library/photos-browser.tsx (4)
editor/grida-canvas-hosted/library/lib-photos-actions.ts (4)
PhotoAsset(17-41)PhotoTopic(52-58)fetchPhotosAction(264-338)fetchPhotoTopics(245-262)editor/grida-canvas-hosted/library/components/loading-indicator.tsx (1)
LoadingIndicator(12-24)editor/grida-canvas-hosted/library/components/search-input.tsx (1)
SearchInput(19-41)editor/grida-canvas-hosted/library/components/pills.tsx (2)
PillsList(61-72)Pill(17-54)
editor/grida-canvas-hosted/playground/playground.tsx (2)
editor/components/floating-window/primitives.tsx (4)
useFloatingWindowControls(394-416)FloatingWindowClose(473-512)FloatingWindowBody(514-530)FloatingWindowTrigger(425-464)editor/grida-canvas-hosted/library/library.tsx (1)
Library(47-188)
🔇 Additional comments (33)
editor/app/(dev)/ui/floating-window/page.tsx (1)
76-218: Library panes + new Photos floating window integration looks clean.The
useMemopanes keep the render blocks readable, and the new window is wired consistently with the existing ones.editor/next.config.ts (1)
47-51: No action needed. Next.js 15.3.6 fully supports glob-style wildcard patterns inremotePatterns.hostname. The pattern"*.unsplash.com"correctly matches single-level subdomains (e.g.,images.unsplash.com,plus.unsplash.com) and will not silently break image loading.editor/grida-canvas-hosted/library/library.tsx (2)
107-152: Good error handling pattern in photo insertion.The async IIFE pattern with try-catch and toast.promise provides proper error handling and user feedback. The re-throwing pattern correctly preserves error information.
154-187: Well-structured tabbed UI implementation.The component correctly handles tab state persistence, conditional rendering, and prop passing to child browsers. The flex layout with
min-h-0properly handles overflow in flexbox contexts.editor/grida-canvas-hosted/library/components/pills.tsx (1)
61-71: Good use of gradient overlays for scroll indication.The implementation with
pointer-events-noneand gradient overlays provides a clean visual cue for scrollable content. The hidden ScrollBar is appropriate for this horizontal pills design.editor/grida-canvas-hosted/playground/playground.tsx (4)
38-38: Good icon choice from Radix Icons.Using
Cross1Iconfrom@radix-ui/react-iconsaligns with the coding guidelines that specify using Lucide or Radix Icons.
368-370: Clean rename from icons-specific to library-generic controls.The rename from
iconsWindowControlstolibraryWindowControlsproperly reflects the broadened scope of the unified Library UI.
481-508: Well-integrated floating window for Library UI.The FloatingWindowRoot is properly configured with the new "library" windowId, appropriate dimensions, and correctly renders the Library component. The close button implementation with Cross1Icon and sr-only label provides good accessibility.
592-606: Consistent prop type update in SidebarLeft.The prop rename from
iconsWindowControlstolibraryWindowControlsis correctly reflected in both the destructured props and the TypeScript type definition.editor/grida-canvas-hosted/library/photos-browser.tsx (8)
101-159: Good pagination and random mode handling.The
loadMorefunction correctly differentiates between random mode (no pagination) and search/topic modes (with pagination). The guard conditions at the start prevent duplicate requests.
161-182: Infinite scroll with IntersectionObserver implemented correctly.The effect properly cleans up the observer on unmount and correctly handles the intersection callback with appropriate guards. The
rootMargin: "200px"provides good preloading behavior.
184-191: Initial load effect is well-guarded.Using
hasInitialLoadRefto ensure single initial load is a good pattern. The eslint-disable comment is appropriate here since we explicitly want mount-only behavior.
337-343: Virtualizer configuration looks correct.Using
lanes: 2for a two-column masonry-style layout withoverscan: 6provides good scrolling performance. TheestimateSizecallback properly calculates heights based on aspect ratios.
345-356: ResizeObserver cleanup is properly handled.The effect correctly observes container width changes and disconnects on cleanup.
596-602: Consider adding unique keys for skeleton items.The skeleton keys use index which is fine for static placeholders, but the template literal could be simplified.
654-690: PhotoCard handles drag and click correctly.The component properly supports both drag-and-drop and click-to-insert workflows. The figcaption with gradient overlay provides good UX for author attribution.
620-641: Unsplash attribution is incomplete and does not meet API requirements.The implementation provides a link to Unsplash but is missing required attribution elements. Per Unsplash API guidelines, proper attribution must include:
- Photographer name and link to their profile
- Link to Unsplash with UTM parameters (
?utm_source=your_app_name&utm_medium=referral)- Text like "Photo by [Name] on Unsplash"
The current code only links to the Unsplash homepage without photographer attribution or UTM tracking, which does not satisfy API integration requirements.
Likely an incorrect or invalid review comment.
editor/grida-canvas-hosted/library/lib-icons.ts (4)
1-7: Well-defined IconsBrowserItem type.The type provides a clean interface for icon items with appropriate optional fields. This aligns with learnings about keeping lib modules reusable and stable.
9-12: Good use of branded string union with escape hatch.The
(string & {})pattern allows type-safe known values while permitting arbitrary strings when needed. This is a useful pattern for extensible category systems.
157-166: Good caching strategy for vendor data.Using
cache: "force-cache"for vendor metadata is appropriate since this data changes infrequently. The error handling with status code in the message aids debugging.
204-209: Clean utility for default variant generation.The
getDefaultVariantsfunction provides a straightforward way to initialize variant filters with the ANY_VARIANT sentinel value.editor/grida-canvas-hosted/library/icons-browser.tsx (4)
32-48: Imports and exports look well-organized.The refactored imports properly separate concerns: UI components from internal library, icons from lucide-react (CheckIcon), and type/utility exports from
./lib-icons. The re-export ofIconsBrowserItemmaintains backward compatibility for consumers.
410-432: Clean migration to new UI primitives.The refactored search input and pills-based vendor selection using
SearchInput,Pill, andPillsListcomponents follows a consistent pattern and improves maintainability. The active state binding (active={selectedVendor === vendor.vendor}) is correct.
484-523: Virtualized grid rendering is well-implemented.The row-based virtualization correctly:
- Calculates row items from
filteredIconsusing column count- Handles empty cells with placeholder divs
- Uses stable keys (
icon.idandempty-${virtualRow.key}-${idx})- Passes
onDragStartthrough to eachIconGridCell
289-311: Line range correction: the IconGridCell component spans lines 289-339, not 289-311.The component correctly delegates drag handling to the parent via the
onDragStartcallback. The parent handler inlibrary.tsx(lines 74-79) properly callsevent.dataTransfer.setData()with encoded icon data, so the drag operation has the required transfer payload.editor/grida-canvas-hosted/library/lib-photos-actions.ts (8)
1-4: Server action directive is correctly placed.The
"use server"directive at the top marks this as a Next.js Server Actions module. The unsplash-js import is appropriate for server-side usage.
6-15: Type extension for premium/plus flags is appropriate.The
UnsplashPhototype correctly extends the base type to include theplusandpremiumflags that the Unsplash API returns but aren't in the library's types. The JSDoc comments clearly explain the filtering purpose.
89-105: Client caching works but consider edge cases.The module-level
clientCacheis effective for reducing client instantiation. However, in serverless environments, the cache may persist across invocations within the same instance. This is generally acceptable for an API client, but note that if environment variables change at runtime, the cached client won't reflect those changes.
107-130: Normalization handles edge cases well.The
normalizePhotofunction properly handles:
- Missing
alt_descriptionwith a fallback chain- Type narrowing for
premium/plusfields using"in"operator- Profile image resolution fallback (small → medium → large)
139-156: Random photos implementation handles API response variance correctly.The function correctly handles the Unsplash API behavior where
getRandomreturns either a single photo object or an array depending on thecountparameter. TheArray.isArraycheck at line 153 is essential.
202-243: Search implementation is correct and mirrors topic photos pattern.The query validation, API call, normalization, and result construction follow the established pattern. The same pagination caveat (filtered results vs. API totals) applies here as well.
279-290: Mode fallback logic may produce unexpected behavior.The condition
mode === "random" || (!safeQuery && mode !== "topic")means:
- If
mode === "search"butqueryis empty, it falls back to random mode silently.This might be intentional UX, but consider whether an empty search query should return an error instead of random photos, to avoid confusing the caller.
326-337: Error handling is robust and produces consistent state.The catch block returns a well-structured error state with:
status: "error"- Preserved
modefor context- Empty results array
- Sanitized error message
This ensures consumers always receive a valid
PhotoActionStateshape.
| const handleInsertIcon = useCallback( | ||
| async (icon: IconsBrowserItem) => { | ||
| const task = fetch(icon.download, { cache: "no-store" }) | ||
| .then((res) => { | ||
| if (!res.ok) { | ||
| throw new Error("Failed to fetch icon"); | ||
| } | ||
| return res.text(); | ||
| }) | ||
| .then((svg) => instance.commands.createNodeFromSvg(svg)) | ||
| .then((node) => { | ||
| node.$.name = icon.name || node.$.name; | ||
| }); | ||
|
|
||
| toast.promise(task, { | ||
| loading: "Loading icon...", | ||
| success: "Icon inserted", | ||
| error: "Failed to insert icon", | ||
| }); | ||
| }, | ||
| [instance.commands] | ||
| ); |
There was a problem hiding this comment.
Unhandled promise rejection risk in icon insertion flow.
The task promise chain doesn't propagate errors correctly to toast.promise. If createNodeFromSvg returns a node and the subsequent .then block throws, the error won't be caught. Additionally, the callback accesses node.$.name without null-checking the node.
const handleInsertIcon = useCallback(
async (icon: IconsBrowserItem) => {
const task = fetch(icon.download, { cache: "no-store" })
.then((res) => {
if (!res.ok) {
throw new Error("Failed to fetch icon");
}
return res.text();
})
.then((svg) => instance.commands.createNodeFromSvg(svg))
.then((node) => {
+ if (!node) {
+ throw new Error("Failed to create node from SVG");
+ }
node.$.name = icon.name || node.$.name;
});
toast.promise(task, {
loading: "Loading icon...",
success: "Icon inserted",
error: "Failed to insert icon",
});
},
[instance.commands]
);📝 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 handleInsertIcon = useCallback( | |
| async (icon: IconsBrowserItem) => { | |
| const task = fetch(icon.download, { cache: "no-store" }) | |
| .then((res) => { | |
| if (!res.ok) { | |
| throw new Error("Failed to fetch icon"); | |
| } | |
| return res.text(); | |
| }) | |
| .then((svg) => instance.commands.createNodeFromSvg(svg)) | |
| .then((node) => { | |
| node.$.name = icon.name || node.$.name; | |
| }); | |
| toast.promise(task, { | |
| loading: "Loading icon...", | |
| success: "Icon inserted", | |
| error: "Failed to insert icon", | |
| }); | |
| }, | |
| [instance.commands] | |
| ); | |
| const handleInsertIcon = useCallback( | |
| async (icon: IconsBrowserItem) => { | |
| const task = fetch(icon.download, { cache: "no-store" }) | |
| .then((res) => { | |
| if (!res.ok) { | |
| throw new Error("Failed to fetch icon"); | |
| } | |
| return res.text(); | |
| }) | |
| .then((svg) => instance.commands.createNodeFromSvg(svg)) | |
| .then((node) => { | |
| if (!node) { | |
| throw new Error("Failed to create node from SVG"); | |
| } | |
| node.$.name = icon.name || node.$.name; | |
| }); | |
| toast.promise(task, { | |
| loading: "Loading icon...", | |
| success: "Icon inserted", | |
| error: "Failed to insert icon", | |
| }); | |
| }, | |
| [instance.commands] | |
| ); |
🤖 Prompt for AI Agents
In editor/grida-canvas-hosted/library/library.tsx around lines 51 to 72, the
promise chain for inserting an icon can swallow errors and accesses node.$.name
without checking node; change the implementation so the entire async flow
returns a single promise (or rewrite as async/await) that propagates any thrown
errors to toast.promise, and add a null/undefined check before setting
node.$.name (e.g., if (!node) throw new Error(...) or guard the assignment).
Ensure any synchronous operations inside .then return values or throw to be
caught by toast.promise so errors surface correctly.
| function usePhotos(perPage: number) { | ||
| const [query, setQuery] = useState(""); | ||
| const debouncedQuery = useDebounce(query, SEARCH_INPUT_DEBOUNCE_MS); | ||
| const [photos, setPhotos] = useState<PhotoAsset[]>([]); | ||
| const [topics, setTopics] = useState<PhotoTopic[]>([]); | ||
| const [topicsError, setTopicsError] = useState<string | null>(null); | ||
| const [topicsLoading, setTopicsLoading] = useState(true); | ||
| const [selectedTopic, setSelectedTopic] = useState<string | null>(null); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [isPending, startTransition] = useTransition(); | ||
| const hasSearchedRef = useRef(false); | ||
|
|
||
| // Pagination state | ||
| const [currentPage, setCurrentPage] = useState(1); | ||
| const [hasMore, setHasMore] = useState(true); | ||
| const [isLoadingMore, setIsLoadingMore] = useState(false); | ||
| const [currentMode, setCurrentMode] = useState<PhotoMode>("random"); | ||
| const [currentQuery, setCurrentQuery] = useState(""); | ||
| const [currentTopicSlug, setCurrentTopicSlug] = useState<string | null>(null); | ||
| const [totalPages, setTotalPages] = useState(1); | ||
|
|
||
| // Scroll detection for infinite scrolling | ||
| const sentinelRef = useRef<HTMLDivElement | null>(null); | ||
| const scrollRef = useRef<HTMLDivElement | null>(null); | ||
| const hasInitialLoadRef = useRef(false); | ||
|
|
||
| const loadInitial = useCallback( | ||
| (params: { mode: PhotoMode; topicSlug?: string; searchQuery?: string }) => { | ||
| // Reset scroll to top when switching topics/queries | ||
| if (scrollRef.current) { | ||
| scrollRef.current.scrollTop = 0; | ||
| } | ||
|
|
||
| const queryToUse = params.searchQuery ?? query; | ||
|
|
||
| startTransition(async () => { | ||
| setError(null); | ||
| setCurrentPage(1); | ||
| setHasMore(true); | ||
| setCurrentMode(params.mode); | ||
| setCurrentQuery(queryToUse); | ||
| setCurrentTopicSlug(params.topicSlug ?? null); | ||
|
|
||
| const next = await fetchPhotosAction({ | ||
| mode: params.mode, | ||
| query: queryToUse, | ||
| perPage, | ||
| topicSlug: params.topicSlug, | ||
| page: 1, | ||
| }); | ||
| if (next.status === "error") { | ||
| setPhotos([]); | ||
| setError(next.message ?? "Failed to load photos"); | ||
| setHasMore(false); | ||
| setTotalPages(0); | ||
| return; | ||
| } | ||
| setPhotos(next.results); | ||
| setTotalPages(next.totalPages ?? 1); | ||
| setHasMore((next.totalPages ?? 1) > 1); | ||
| }); | ||
| }, | ||
| [perPage, query] | ||
| ); |
There was a problem hiding this comment.
Complex hook with potential stale closure issue.
The loadInitial callback depends on query but also receives searchQuery as a parameter. When params.searchQuery is undefined, it falls back to query from closure, which may be stale in certain call sequences. Consider making the search query parameter required or removing the closure dependency.
const loadInitial = useCallback(
- (params: { mode: PhotoMode; topicSlug?: string; searchQuery?: string }) => {
+ (params: { mode: PhotoMode; topicSlug?: string; searchQuery: string }) => {
// Reset scroll to top when switching topics/queries
if (scrollRef.current) {
scrollRef.current.scrollTop = 0;
}
- const queryToUse = params.searchQuery ?? query;
+ const queryToUse = params.searchQuery;
startTransition(async () => {
// ...
});
},
- [perPage, query]
+ [perPage]
);Then update call sites to always pass the query explicitly.
📝 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.
| function usePhotos(perPage: number) { | |
| const [query, setQuery] = useState(""); | |
| const debouncedQuery = useDebounce(query, SEARCH_INPUT_DEBOUNCE_MS); | |
| const [photos, setPhotos] = useState<PhotoAsset[]>([]); | |
| const [topics, setTopics] = useState<PhotoTopic[]>([]); | |
| const [topicsError, setTopicsError] = useState<string | null>(null); | |
| const [topicsLoading, setTopicsLoading] = useState(true); | |
| const [selectedTopic, setSelectedTopic] = useState<string | null>(null); | |
| const [error, setError] = useState<string | null>(null); | |
| const [isPending, startTransition] = useTransition(); | |
| const hasSearchedRef = useRef(false); | |
| // Pagination state | |
| const [currentPage, setCurrentPage] = useState(1); | |
| const [hasMore, setHasMore] = useState(true); | |
| const [isLoadingMore, setIsLoadingMore] = useState(false); | |
| const [currentMode, setCurrentMode] = useState<PhotoMode>("random"); | |
| const [currentQuery, setCurrentQuery] = useState(""); | |
| const [currentTopicSlug, setCurrentTopicSlug] = useState<string | null>(null); | |
| const [totalPages, setTotalPages] = useState(1); | |
| // Scroll detection for infinite scrolling | |
| const sentinelRef = useRef<HTMLDivElement | null>(null); | |
| const scrollRef = useRef<HTMLDivElement | null>(null); | |
| const hasInitialLoadRef = useRef(false); | |
| const loadInitial = useCallback( | |
| (params: { mode: PhotoMode; topicSlug?: string; searchQuery?: string }) => { | |
| // Reset scroll to top when switching topics/queries | |
| if (scrollRef.current) { | |
| scrollRef.current.scrollTop = 0; | |
| } | |
| const queryToUse = params.searchQuery ?? query; | |
| startTransition(async () => { | |
| setError(null); | |
| setCurrentPage(1); | |
| setHasMore(true); | |
| setCurrentMode(params.mode); | |
| setCurrentQuery(queryToUse); | |
| setCurrentTopicSlug(params.topicSlug ?? null); | |
| const next = await fetchPhotosAction({ | |
| mode: params.mode, | |
| query: queryToUse, | |
| perPage, | |
| topicSlug: params.topicSlug, | |
| page: 1, | |
| }); | |
| if (next.status === "error") { | |
| setPhotos([]); | |
| setError(next.message ?? "Failed to load photos"); | |
| setHasMore(false); | |
| setTotalPages(0); | |
| return; | |
| } | |
| setPhotos(next.results); | |
| setTotalPages(next.totalPages ?? 1); | |
| setHasMore((next.totalPages ?? 1) > 1); | |
| }); | |
| }, | |
| [perPage, query] | |
| ); | |
| function usePhotos(perPage: number) { | |
| const [query, setQuery] = useState(""); | |
| const debouncedQuery = useDebounce(query, SEARCH_INPUT_DEBOUNCE_MS); | |
| const [photos, setPhotos] = useState<PhotoAsset[]>([]); | |
| const [topics, setTopics] = useState<PhotoTopic[]>([]); | |
| const [topicsError, setTopicsError] = useState<string | null>(null); | |
| const [topicsLoading, setTopicsLoading] = useState(true); | |
| const [selectedTopic, setSelectedTopic] = useState<string | null>(null); | |
| const [error, setError] = useState<string | null>(null); | |
| const [isPending, startTransition] = useTransition(); | |
| const hasSearchedRef = useRef(false); | |
| // Pagination state | |
| const [currentPage, setCurrentPage] = useState(1); | |
| const [hasMore, setHasMore] = useState(true); | |
| const [isLoadingMore, setIsLoadingMore] = useState(false); | |
| const [currentMode, setCurrentMode] = useState<PhotoMode>("random"); | |
| const [currentQuery, setCurrentQuery] = useState(""); | |
| const [currentTopicSlug, setCurrentTopicSlug] = useState<string | null>(null); | |
| const [totalPages, setTotalPages] = useState(1); | |
| // Scroll detection for infinite scrolling | |
| const sentinelRef = useRef<HTMLDivElement | null>(null); | |
| const scrollRef = useRef<HTMLDivElement | null>(null); | |
| const hasInitialLoadRef = useRef(false); | |
| const loadInitial = useCallback( | |
| (params: { mode: PhotoMode; topicSlug?: string; searchQuery: string }) => { | |
| // Reset scroll to top when switching topics/queries | |
| if (scrollRef.current) { | |
| scrollRef.current.scrollTop = 0; | |
| } | |
| const queryToUse = params.searchQuery; | |
| startTransition(async () => { | |
| setError(null); | |
| setCurrentPage(1); | |
| setHasMore(true); | |
| setCurrentMode(params.mode); | |
| setCurrentQuery(queryToUse); | |
| setCurrentTopicSlug(params.topicSlug ?? null); | |
| const next = await fetchPhotosAction({ | |
| mode: params.mode, | |
| query: queryToUse, | |
| perPage, | |
| topicSlug: params.topicSlug, | |
| page: 1, | |
| }); | |
| if (next.status === "error") { | |
| setPhotos([]); | |
| setError(next.message ?? "Failed to load photos"); | |
| setHasMore(false); | |
| setTotalPages(0); | |
| return; | |
| } | |
| setPhotos(next.results); | |
| setTotalPages(next.totalPages ?? 1); | |
| setHasMore((next.totalPages ?? 1) > 1); | |
| }); | |
| }, | |
| [perPage] | |
| ); |
🤖 Prompt for AI Agents
In editor/grida-canvas-hosted/library/photos-browser.tsx around lines 36 to 99,
loadInitial currently falls back to the outer `query` closure when
params.searchQuery is undefined, which can produce stale values; make the search
query explicit by changing loadInitial signature to require a searchQuery
(remove fallback to closure) and update all call sites to pass the intended
query string (e.g., always pass debouncedQuery or current input) so loadInitial
no longer depends on the potentially stale `query` state; ensure tests and
usages set searchQuery when switching topics/modes and update the types
accordingly.
| export type DataTransferPayload = | ||
| | { | ||
| type: "svg"; | ||
| name: string; | ||
| src: string; | ||
| } | ||
| | { | ||
| type: "image"; | ||
| name: string; | ||
| src: string; | ||
| width?: number; | ||
| height?: number; | ||
| }; | ||
|
|
||
| export const key = "x-grida-data-transfer"; | ||
| export function encode(data: any) { | ||
| export function encode(data: DataTransferPayload) { | ||
| const txt = JSON.stringify(data); | ||
| return txt; | ||
| } | ||
| export function decode(data: string): DataTransferPayload { | ||
| return JSON.parse(data) as DataTransferPayload; | ||
| } |
There was a problem hiding this comment.
Harden decode() against invalid/malicious payloads (don’t trust JSON.parse + cast).
At minimum, catch parse errors and validate the discriminant + required fields before returning.
export namespace datatransfer {
+ function isRecord(v: unknown): v is Record<string, unknown> {
+ return typeof v === "object" && v !== null;
+ }
+
export function encode(data: DataTransferPayload) {
const txt = JSON.stringify(data);
return txt;
}
export function decode(data: string): DataTransferPayload {
- return JSON.parse(data) as DataTransferPayload;
+ const parsed: unknown = JSON.parse(data);
+ if (!isRecord(parsed) || typeof parsed.type !== "string") {
+ throw new Error("Invalid data-transfer payload");
+ }
+ if (parsed.type === "svg") {
+ if (typeof parsed.name !== "string" || typeof parsed.src !== "string") {
+ throw new Error("Invalid svg payload");
+ }
+ return parsed as DataTransferPayload;
+ }
+ if (parsed.type === "image") {
+ if (typeof parsed.name !== "string" || typeof parsed.src !== "string") {
+ throw new Error("Invalid image payload");
+ }
+ return parsed as DataTransferPayload;
+ }
+ throw new Error("Unknown payload type");
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In editor/grida-canvas/data-transfer.ts around lines 2–23, decode() currently
trusts JSON.parse and a cast; wrap the parse in a try/catch to handle malformed
JSON, then perform runtime validation on the resulting object: ensure it is a
plain object, validate discriminant "type" is either "svg" or "image", check
required fields (name and src) are non-empty strings, and for "image" validate
optional width/height are numbers if present. If validation fails, throw a clear
error (or return a safe default) rather than returning a blind cast; implement
these checks in a small type-guard helper used by decode().
1 participant
day-309-grida-canvas-ux-photos-browser.mp4
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.