Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds layered-image and GIF support across upload, preview, rendering, and UI: new LayeredImageData types and hook changes, GIF encoding/decoding pipeline in renderer with gifenc, upload validation expanded (including GIFs and stricter path/mime/dimension checks), and UI components updated to render layered compositions and handle IPFS fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ArtworkPreview / ImageGrid
participant Hook as useArtworkPreview
participant API as /api/renderer/stack-images
User->>UI: open preview / request generation
UI->>Hook: request generatedImages
Hook-->>UI: LayeredImageData[]
alt LayeredImageData.type == 'single'
UI->>UI: render single <img> from url
else LayeredImageData.type == 'layered'
UI->>UI: overlay layers (absolute positioned) in order
end
opt server composition needed (no local files)
Hook->>API: GET stack-images?images=...
API->>API: detect formats, compose (GIF frames or WebP)
API-->>Hook: image buffer / URL
Hook-->>UI: LayeredImageData (single URL)
end
sequenceDiagram
participant Uploader as User
participant Hook as useArtworkUpload
participant FS as File Input
Uploader->>FS: select files/folders
FS-->>Hook: FileList
Hook->>Hook: validate path structure, enforce 3 segments
Hook->>Hook: validate MIME uniformity (allow image/gif)
Hook->>Hook: validate dimensions/aspect, enforce max traits
Hook-->>Uploader: errors or CollectionFile[] ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
packages/hooks/src/useArtworkPreview.ts (3)
283-285: Same here: encode query param values correctlyReplace encodeURI with encodeURIComponent when building the renderer URL.
- for (const image of imageLayerStack) { - url.searchParams.append('images', encodeURI(image)) - } + for (const image of imageLayerStack) { + url.searchParams.append('images', encodeURIComponent(image)) + }
53-64: Prevent blob URL leaks across generationsBlob URLs created for layered previews (especially with local files) are pushed to usedBlobUrls but aren’t revoked when generating a new preview, causing memory to grow. Introduce a helper to revoke and clear previously used blob URLs.
const usedBlobUrls = React.useRef<string[]>([]) const isMountedRef = React.useRef(false) useEffect(() => { isMountedRef.current = true return () => { isMountedRef.current = false // Cleanup blob URLs on unmount usedBlobUrls.current.forEach((url) => URL.revokeObjectURL(url)) } }, []) + + // Revoke previously used blob URLs (e.g., before generating new previews) + const revokeUsedBlobUrls = React.useCallback(() => { + if (!usedBlobUrls.current.length) return + usedBlobUrls.current.forEach((url) => URL.revokeObjectURL(url)) + usedBlobUrls.current = [] + }, [])
232-238: Call blob cleanup before generating a new previewThis ensures blob URLs from the previous preview are released before allocating new ones in the layered GIF path.
const generateStackedImage = React.useCallback( async (e?: BaseSyntheticEvent) => { try { if (e) e.stopPropagation() + // Free blob URLs from the previous preview before generating a new one + revokeUsedBlobUrls() if (!canvas.current) throw new Error('No canvas') if (!layers.length) throw new Error('No layers')
🧹 Nitpick comments (10)
apps/web/src/components/SingleImageUpload/SingleImageUpload.tsx (1)
113-123: Add input accept and align multiple to single-file behaviorThe UI allows selecting multiple files but only the first one is processed. Also, advertising accepted types in the file picker improves UX and reduces errors.
Apply:
<input className={defaultUploadStyle} id="file-upload" data-testid="file-upload" name="file" type="file" - multiple={true} + multiple={false} + accept={acceptableMIME.join(',')} onChange={(event) => { handleFileUpload(event.currentTarget.files) }} />apps/web/src/modules/auction/components/AuctionImage.tsx (2)
8-15: Fix typo in props interface nameMinor naming nit: AucitonImageProps -> AuctionImageProps.
-interface AucitonImageProps { +interface AuctionImageProps { image?: string name?: string isLoading?: boolean } -export const AuctionImage = ({ image, name }: AucitonImageProps) => { +export const AuctionImage = ({ image, name }: AuctionImageProps) => {
26-29: Alt text: provide a non-empty defaultUsing an empty string suppresses screen readers. Prefer a meaningful default when name is absent.
- alt={name || ''} + alt={name ?? 'Auction image'}apps/web/src/components/Artwork/ArtworkPreview.tsx (3)
1-1: Import fallback image utilities for resilient renderingTo handle IPFS/data/blob URLs consistently (and gain gateway fallbacks), use FallbackNextImage + getFetchableUrls in the preview UI.
-import { LayeredImageData, OrderedTraits } from '@buildeross/hooks/useArtworkPreview' +import { LayeredImageData, OrderedTraits } from '@buildeross/hooks/useArtworkPreview' +import { getFetchableUrls } from '@buildeross/ipfs-service' +import { FallbackNextImage } from 'src/components/FallbackImage'
33-38: Use FallbackNextImage for single preview URLThis aligns with the rest of the app’s image handling and supports gateway and local blob/data URIs seamlessly.
- <img - height={'100%'} - width={'100%'} - src={generatedImages[0].url} - alt="preview" - /> + <FallbackNextImage + srcList={getFetchableUrls(generatedImages[0].url)} + alt="preview" + fill + style={{ objectFit: 'contain' }} + />
41-58: Use FallbackNextImage for layered preview itemsEnsures each layer benefits from gateway fallbacks and robust URL handling.
- {generatedImages[0].layers?.map((layer, index) => ( - <img - key={index} - src={layer.url} - alt={layer.trait} - style={{ - position: 'absolute', - top: 0, - left: 0, - width: '100%', - height: '100%', - objectFit: 'contain', - zIndex: index, - }} - /> - ))} + {generatedImages[0].layers?.map((layer, index) => ( + <FallbackNextImage + key={index} + srcList={getFetchableUrls(layer.url)} + alt={layer.trait} + fill + style={{ objectFit: 'contain', zIndex: index }} + /> + ))}packages/hooks/src/useArtworkUpload.ts (2)
158-170: Improve error messaging for incorrect folder structure.The error messages could be more specific about the expected folder structure format to help users understand what's required.
Apply this diff to improve error messages:
if (paths.length !== 3 || !paths) { if (paths.length > 3) { setUploadArtworkError({ - directory: `file or folder naming incorrect. must not include back slashes.`, + directory: `Folder nesting too deep. Expected format: collection/trait/property.ext, but found ${paths.length} levels.`, }) return } setUploadArtworkError({ - directory: `folder structure is incorrect. download the demo folder to compare.`, + directory: `Incorrect folder structure. Expected format: collection/trait/property.ext, but found ${paths.length} levels. Download the demo folder for reference.`, }) return }
234-263: Consider extracting image validation logic into a separate function.The image size validation logic within the reducer is complex and could benefit from being extracted into a separate helper function for better maintainability and testability.
Consider extracting the image validation logic:
const validateImageDimensions = ( file: File, onError: (error: ArtworkUploadError) => void, onComplete?: () => void ) => { const fr = new FileReader() fr.readAsDataURL(file) fr.onload = () => { const img = new Image() img.src = fr.result?.toString() || '' img.onload = function () { const height = img.height const width = img.width const min = 600 if ((height < min || width < min) && file.type !== 'image/svg+xml') { onError({ dimensions: `we recommend images of min, 600px width x height, your images are width: ${width} x ${height} px`, }) return } if (height !== width) { onError({ dimensions: `images must be of equal height and width, your images are width: ${width} x ${height} px`, }) return } onComplete?.() } } }Then use it in the reducer:
- /* get image size */ - const fr = new FileReader() - fr.readAsDataURL(cv) - const getImageSize = (fr: FileReader, count: number) => { - // ... existing logic - } - fr.onload = () => getImageSize(fr, filesArray.indexOf(cv)) + validateImageDimensions( + cv, + setUploadArtworkError, + filesArray.indexOf(cv) === filesArray.length - 1 ? () => setIsProcessing(false) : undefined + )apps/web/src/pages/api/renderer/stack-images.ts (2)
147-156: Optimize memory usage when creating combined RGB buffer.The current approach creates a large combined buffer for all frames which could cause memory issues with many frames.
Consider processing frames in batches for the global palette:
- // Create global palette from all frames - const combinedRgb = new Uint8Array( - allRgbFrames.reduce((sum, frame) => sum + frame.length, 0) - ) - let offset = 0 - for (const frame of allRgbFrames) { - combinedRgb.set(frame, offset) - offset += frame.length - } - - const globalPalette = quantize(combinedRgb, 256) + // Create global palette by sampling frames + const sampleSize = Math.min(5, allRgbFrames.length) + const sampledFrames = allRgbFrames.filter((_, i) => i % Math.ceil(allRgbFrames.length / sampleSize) === 0) + + const combinedRgb = new Uint8Array( + sampledFrames.reduce((sum, frame) => sum + frame.length, 0) + ) + let offset = 0 + for (const frame of sampledFrames) { + combinedRgb.set(frame, offset) + offset += frame.length + } + + const globalPalette = quantize(combinedRgb, 256)
290-312: Add retry logic with exponential backoff for image fetching.The current implementation tries each URL sequentially but doesn't implement retry logic for transient failures.
Consider adding retry logic:
const getImageData = async (imageUrl: string): Promise<Buffer> => { const urls = getFetchableUrls(imageUrl) if (!urls?.length) throw new Error('Invalid IPFS url: ' + imageUrl) + const fetchWithRetry = async (url: string, retries = 2): Promise<Buffer> => { + for (let attempt = 0; attempt <= retries; attempt++) { + try { + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), REQUEST_TIMEOUT) + + const res = await fetch(url, { signal: controller.signal }) + clearTimeout(timeoutId) + + if (!res.ok) throw new Error(`HTTP ${res.status}`) + + const arrayBuffer = await res.arrayBuffer() + return Buffer.from(arrayBuffer) + } catch (err) { + if (attempt === retries) { + throw err + } + // Exponential backoff + await new Promise(resolve => setTimeout(resolve, Math.pow(2, attempt) * 1000)) + } + } + throw new Error('Failed after retries') + } + for (const url of urls) { try { - const controller = new AbortController() - const timeoutId = setTimeout(() => controller.abort(), REQUEST_TIMEOUT) - - const res = await fetch(url, { signal: controller.signal }) - clearTimeout(timeoutId) - - if (!res.ok) throw new Error(`HTTP ${res.status}`) - - const arrayBuffer = await res.arrayBuffer() - return Buffer.from(arrayBuffer) + return await fetchWithRetry(url) } catch (err) { console.warn(`Failed to fetch from ${url}: ${(err as Error).message}`) } } throw new Error('Failed to fetch image from all fetchable URLs') }
📜 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 (9)
apps/web/package.json(1 hunks)apps/web/src/components/Artwork/ArtworkPreview.tsx(3 hunks)apps/web/src/components/Artwork/Playground/ImageGrid.tsx(1 hunks)apps/web/src/components/SingleImageUpload/SingleImageUpload.tsx(1 hunks)apps/web/src/modules/auction/components/AuctionImage.tsx(3 hunks)apps/web/src/pages/api/renderer/stack-images.ts(3 hunks)apps/web/src/types/gifenc.d.ts(1 hunks)packages/hooks/src/useArtworkPreview.ts(8 hunks)packages/hooks/src/useArtworkUpload.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/web/src/components/Artwork/Playground/ImageGrid.tsx (2)
packages/hooks/src/useArtworkPreview.ts (1)
LayeredImageData(14-22)apps/web/src/styles/Artwork.css.ts (2)
imageGridWrapperStyle(181-195)previewGeneratedImageStyle(129-140)
packages/hooks/src/useArtworkPreview.ts (1)
packages/constants/src/rendererBase.ts (1)
RENDERER_BASE(1-1)
apps/web/src/components/Artwork/ArtworkPreview.tsx (1)
packages/hooks/src/useArtworkPreview.ts (1)
LayeredImageData(14-22)
apps/web/src/modules/auction/components/AuctionImage.tsx (2)
packages/ipfs-service/src/gateway.ts (1)
getFetchableUrls(28-55)apps/web/src/components/FallbackImage.tsx (1)
FallbackNextLegacyImage(131-144)
packages/hooks/src/useArtworkUpload.ts (1)
packages/utils/src/sanitize.ts (1)
sanitizeFileName(6-7)
🔇 Additional comments (7)
apps/web/package.json (1)
44-45: No issues found with gifenc dependency usageAll checks passed:
- gifenc is only imported in apps/web/src/pages/api/renderer/stack-images.ts (an API route using NextApiRequest/NextApiResponse, not an Edge runtime).
- Type definitions exist at apps/web/src/types/gifenc.d.ts.
- Package version is correctly set to ^1.0.3 in apps/web/package.json.
No further action needed.
apps/web/src/components/SingleImageUpload/SingleImageUpload.tsx (1)
29-35: LGTM: GIF MIME type addedExpanding acceptableMIME to include image/gif is correct and matches the PR goal.
apps/web/src/modules/auction/components/AuctionImage.tsx (1)
15-15: LGTM: Gateway-aware src list with FallbackNextLegacyImageDeriving srcList via getFetchableUrls integrates well with the fallback image component and removes the need for local onError handling.
apps/web/src/components/Artwork/Playground/ImageGrid.tsx (1)
1-1: LGTM! Import aligns with the new layered image data structure.The import correctly references the
LayeredImageDatatype from the hooks package, which supports both single and layered image rendering.packages/hooks/src/useArtworkUpload.ts (1)
76-82: LGTM! Well-structured type for collection file data.The
CollectionFiletype is appropriately exported and provides a clear structure for representing uploaded artwork files with their collection context.apps/web/src/pages/api/renderer/stack-images.ts (1)
11-36: LGTM! Robust image format detection implementation.The format detection logic correctly identifies GIF, PNG, and SVG formats using appropriate magic bytes and string patterns.
apps/web/src/types/gifenc.d.ts (1)
1-21: LGTM! Well-structured TypeScript declarations for gifenc.The type declarations correctly define the GIF encoder interface and functions needed for the GIF encoding functionality.
dan13ram
force-pushed
the
feat/support-animated-gifs
branch
from
13ed836 to
8db6e26
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
packages/hooks/src/useArtworkUpload.ts (1)
104-121: Guard against missing trait/property to avoid pushing invalid ImageProps.If upload.trait is not found in artwork (index === -1) or the property name doesn’t exist (childIndex === -1), this code can push entries with undefined trait/name into images. Short-circuit safely and fall back to upload.name for the label.
Apply this diff:
- const index = artwork?.map((e: ArtworkType) => e.trait).indexOf(upload.trait) - const childIndex = artwork[index]?.properties.indexOf(upload.name) - const childName = artwork[index]?.properties[childIndex] + const index = artwork?.map((e: ArtworkType) => e.trait).indexOf(upload.trait) + if (index < 0) { + // Trait not found in artwork schema; skip this entry + return acc + } + const childIndex = artwork[index]?.properties.indexOf(upload.name) ?? -1 + const childName = + childIndex >= 0 ? artwork[index]?.properties[childIndex] : upload.name
♻️ Duplicate comments (2)
apps/web/src/pages/api/renderer/stack-images.ts (1)
98-115: Thanks for addressing memory concerns previously raisedThe frame cap and memory estimation guard address the prior OOM risk in animated composition.
packages/hooks/src/useArtworkUpload.ts (1)
226-231: Resolved: Correct previous-element bounds check for MIME type consistency.The prior off-by-one/precedence issue was fixed by checking
index > 0then comparing tofilesArray[index - 1]. This correctly enforces batch homogeneity.
🧹 Nitpick comments (11)
apps/web/src/pages/api/renderer/stack-images.ts (6)
21-27: Tighten PNG signature detection to reduce false positivesUse the full 8-byte PNG signature instead of checking only the first 4 bytes.
- if ( - buffer.length >= 8 && - buffer[0] === 0x89 && - buffer[1] === 0x50 && - buffer[2] === 0x4e && - buffer[3] === 0x47 - ) { + if ( + buffer.length >= 8 && + buffer[0] === 0x89 && // 137 + buffer[1] === 0x50 && // P + buffer[2] === 0x4e && // N + buffer[3] === 0x47 && // G + buffer[4] === 0x0d && + buffer[5] === 0x0a && + buffer[6] === 0x1a && + buffer[7] === 0x0a + ) {
101-114: Memory guard added — good safeguard (one tweak to the estimate)The added frame-count cap and memory estimate are great. The estimate currently multiplies by the number of layers, but the big allocation at this stage is per composited frame (post-layering). Remove imageData.length to avoid overly conservative rejections.
- const estimatedMemoryMB = - (maxFrames * imageData.length * SVG_DEFAULT_SIZE * SVG_DEFAULT_SIZE * 4) / - (1024 * 1024) + const estimatedMemoryMB = + (maxFrames * SVG_DEFAULT_SIZE * SVG_DEFAULT_SIZE * 4) / (1024 * 1024)
225-242: Optional: Use contain+transparent background for static (non-GIF) inputs to avoid croppingWith fit: 'inside', overlays larger than base may get cropped during composite. Using fit: 'contain' ensures a consistent 1080x1080 canvas with transparent padding, avoiding unexpected crops.
- imageData.map(async (buffer) => - sharp(buffer) - .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { fit: 'inside' }) - .toBuffer() - ) + imageData.map(async (buffer) => + sharp(buffer) + .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { + fit: 'contain', + background: { r: 0, g: 0, b: 0, alpha: 0 }, + }) + .png() + .toBuffer() + )
273-287: HEAD handler performs a network fetch; consider a cheaper detection pathHEAD often precedes GET for cache validation. Fetching the first image to detect type adds latency and load. Consider:
- Infer from query (e.g., file extension) when trustworthy.
- Cache detection per URL (short TTL) or make HEAD skip detection and default to image/webp; let GET return the real content-type.
Would you like me to add a small in-memory LRU cache for “format by URL” to avoid repeated fetches on hot paths?
174-181: Optional: Preserve original frame delays (if available)Currently, all frames use a fixed 100ms delay. If you want smoother/accurate playback, read per-frame delays from the animated source (sharp().metadata().delay) and compute blended delays when stacking multiple animated inputs (e.g., use LCM/GCD approach across layers).
I can wire up delay extraction and normalization if you confirm which layer should drive timing when multiple animations are present.
299-304: Set Content-Length header for better client caching/streamingAfter computing buffer, add Content-Length to help intermediaries and some clients.
- res.setHeader('Content-Type', contentType) - res.send(buffer) + res.setHeader('Content-Type', contentType) + res.setHeader('Content-Length', String(buffer.length)) + res.send(buffer)apps/web/src/components/Artwork/Playground/ImageGrid.tsx (1)
21-21: Consider using a more stable key for layer rendering.Using array indices as React keys can cause issues during re-renders if the layers array is reordered. Consider using a unique identifier if available.
If layers have unique identifiers, use them as keys:
- key={layerIndex} + key={layer.trait || layerIndex}This would provide more stable keys while still falling back to index when trait is not unique.
apps/web/src/modules/create-proposal/components/TransactionForm/AddArtwork/AddArtwork.tsx (1)
258-275: Extract dimension validation to reduce code duplication.The dimension validation logic could be extracted into a helper function to improve maintainability.
Consider extracting the dimension comparison logic:
const validateDimensions = ( uploaded: { width: number; height: number }, existing: { width: number; height: number } ): string | null => { if (uploaded.width !== existing.width || uploaded.height !== existing.height) { return `Uploaded artwork dimensions (${uploaded.width}x${uploaded.height}) do not match existing artwork dimensions (${existing.width}x${existing.height})` } return null }Then use it in the effect:
- if ( - uploadedDimensions.width !== existingDimensions.width || - uploadedDimensions.height !== existingDimensions.height - ) { - setDimensionMismatch( - `Uploaded artwork dimensions (${uploadedDimensions.width}x${uploadedDimensions.height}) do not match existing artwork dimensions (${existingDimensions.width}x${existingDimensions.height})` - ) - } else { - setDimensionMismatch(null) - } + const mismatch = validateDimensions(uploadedDimensions, existingDimensions) + setDimensionMismatch(mismatch)packages/hooks/src/useArtworkUpload.ts (3)
186-194: Minor: simplify and make property push safer.
if (!!traits)is always true. Also,filter(...)[0]is less clear thanfind.Apply this diff:
- if (!!traits) { - traits - .filter((trait) => trait.trait === currentTrait)[0] - ?.properties?.push(currentProperty) - } + const traitEntry = traits.find((t) => t.trait === currentTrait) + traitEntry?.properties.push(currentProperty)
286-296: Avoid shadowing state variable name for clarity.Inside the effect,
const filesArray = ...shadows thefilesArraystate variable. Rename the local variable to avoid confusion.Apply this diff:
- if (isProcessing === false && !!fileInfo?.fileArray) { - const filesArray = fileInfo.fileArray.reduce((acc: File[], cv: CollectionFile) => { + if (isProcessing === false && !!fileInfo?.fileArray) { + const selectedFiles = fileInfo.fileArray.reduce((acc: File[], cv: CollectionFile) => { acc.push(cv.file) return acc }, []) - setFilesArray(filesArray) + setFilesArray(selectedFiles) }
199-210: Clarify naming error message to match checks.The checks target “:” and periods in names, not forward slashes. Adjust the message to reduce confusion.
Apply this diff:
- setUploadArtworkError({ - directory: `file or folder naming incorrect. must not include forward slashes or periods.`, - }) + setUploadArtworkError({ + directory: + 'File/folder naming incorrect. Traits/folders must not contain periods. Files must not contain ":" and must have exactly one "." before the extension.', + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/web/src/components/Artwork/Playground/ImageGrid.tsx(1 hunks)apps/web/src/modules/create-dao/components/Artwork/ArtworkForm.tsx(1 hunks)apps/web/src/modules/create-proposal/components/ArtworkUpload/ArtworkUpload.tsx(1 hunks)apps/web/src/modules/create-proposal/components/TransactionForm/AddArtwork/AddArtwork.tsx(6 hunks)apps/web/src/modules/create-proposal/components/TransactionForm/AddArtwork/AddArtworkForm.tsx(5 hunks)apps/web/src/modules/create-proposal/components/TransactionForm/ReplaceArtwork/ReplaceArtworkForm.tsx(2 hunks)apps/web/src/pages/api/renderer/stack-images.ts(3 hunks)packages/hooks/src/useArtworkPreview.ts(8 hunks)packages/hooks/src/useArtworkUpload.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/modules/create-dao/components/Artwork/ArtworkForm.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hooks/src/useArtworkPreview.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/src/components/Artwork/Playground/ImageGrid.tsx (2)
packages/hooks/src/useArtworkPreview.ts (1)
LayeredImageData(14-22)apps/web/src/styles/Artwork.css.ts (2)
imageGridWrapperStyle(181-195)previewGeneratedImageStyle(129-140)
packages/hooks/src/useArtworkUpload.ts (2)
packages/utils/src/sanitize.ts (1)
sanitizeFileName(6-7)apps/web/src/components/MediaPreview/Image.tsx (1)
Image(8-24)
🔇 Additional comments (9)
apps/web/src/modules/create-proposal/components/TransactionForm/ReplaceArtwork/ReplaceArtworkForm.tsx (3)
89-89: LGTM! Support for GIF format is correctly added.The helper text now includes GIF in the list of supported image types, aligning with the broader PR changes for animated GIF support.
101-101: UI alignment improvements look good.The text alignment change from center to left provides better readability for error messages, consistent with standard form validation patterns.
Also applies to: 110-110
103-104: Spacing adjustments improve visual hierarchy.The updated margins (mt="x4" mb="x2" for the first error, mt="x2" mb="x4" for the second) create better visual separation between error messages and surrounding content.
Also applies to: 112-113
apps/web/src/modules/create-proposal/components/ArtworkUpload/ArtworkUpload.tsx (1)
225-225: Confirm necessity of dualsetOrderedLayers([])callsBoth calls clear the same state in quick succession—one on file selection, one before merging new traits. Please verify whether you need both:
- In
handleUpload(around line 214): resets layers immediately when files are chosen.- In the main
useEffect(at line 225): clears layers right before setting up artwork and merging traits.If the immediate reset in
handleUploadis sufficient, consider removing the effect’s reset to avoid back-to-back updates. Otherwise, retain both for explicit separation of concerns.apps/web/src/components/Artwork/Playground/ImageGrid.tsx (1)
14-37: Layered image rendering implementation looks solid.The component correctly:
- Handles both single and layered image types based on the
typefield- Includes defensive checks for
layersexistence and non-empty array- Properly positions layers with absolute positioning and z-index
The implementation addresses the previous review comment about defensive checks.
apps/web/src/modules/create-proposal/components/TransactionForm/AddArtwork/AddArtworkForm.tsx (3)
35-36: New validation props enhance error feedback.The addition of
fileExtensionMismatchanddimensionMismatchprops provides clear, specific validation feedback to users about file compatibility issues.
65-92: Improved requirements documentation with clear categories.The restructured requirements section with labeled categories (Adding new traits, File consistency, etc.) provides much clearer guidance to users. The formatting with bold labels enhances readability.
155-164: Validation error messages are properly displayed.The new error messages for file extension and dimension mismatches are correctly rendered with appropriate styling and spacing, providing clear feedback to users.
packages/hooks/src/useArtworkUpload.ts (1)
76-81: New CollectionFile type is a solid foundation for layered assets.Clear, minimal, and future-proof. This metadata wrapper around File enables consistent downstream processing.
| const getFileExtensionFromUri = (uri: string): string => { | ||
| const parts = uri.split('.') | ||
| return parts.length > 1 ? `.${parts[parts.length - 1]}` : '' | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling file extensions with query parameters.
The getFileExtensionFromUri function might fail if URIs contain query parameters or fragments. Consider a more robust implementation.
const getFileExtensionFromUri = (uri: string): string => {
- const parts = uri.split('.')
- return parts.length > 1 ? `.${parts[parts.length - 1]}` : ''
+ // Remove query parameters and fragments
+ const cleanUri = uri.split('?')[0].split('#')[0]
+ const parts = cleanUri.split('.')
+ return parts.length > 1 ? `.${parts[parts.length - 1].toLowerCase()}` : ''
}📝 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 getFileExtensionFromUri = (uri: string): string => { | |
| const parts = uri.split('.') | |
| return parts.length > 1 ? `.${parts[parts.length - 1]}` : '' | |
| } | |
| const getFileExtensionFromUri = (uri: string): string => { | |
| // Remove query parameters and fragments | |
| const cleanUri = uri.split('?')[0].split('#')[0] | |
| const parts = cleanUri.split('.') | |
| return parts.length > 1 ? `.${parts[parts.length - 1].toLowerCase()}` : '' | |
| } |
| const getImageDimensionsFromFile = ( | ||
| file: File | ||
| ): Promise<{ width: number; height: number }> => { | ||
| return new Promise((resolve, reject) => { | ||
| if (file.type === 'image/svg+xml') { | ||
| // SVG files don't need dimension validation | ||
| resolve({ width: 0, height: 0 }) | ||
| return | ||
| } | ||
|
|
||
| const fr = new FileReader() | ||
| fr.readAsDataURL(file) | ||
| fr.onload = () => { | ||
| const img = new Image() | ||
| img.onload = () => { | ||
| resolve({ width: img.width, height: img.height }) | ||
| } | ||
| img.onerror = () => { | ||
| reject(new Error(`Failed to load image from file: ${file.name}`)) | ||
| } | ||
| img.src = fr.result?.toString() || '' | ||
| } | ||
| fr.onerror = () => { | ||
| reject(new Error(`Failed to read file: ${file.name}`)) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for unsupported image formats.
The getImageDimensionsFromFile function should validate the file type before processing to avoid attempting to read unsupported formats.
const getImageDimensionsFromFile = (
file: File
): Promise<{ width: number; height: number }> => {
return new Promise((resolve, reject) => {
+ // Check if file type is supported
+ const supportedTypes = ['image/png', 'image/gif', 'image/jpeg', 'image/svg+xml']
+ if (!supportedTypes.includes(file.type)) {
+ reject(new Error(`Unsupported file type: ${file.type}`))
+ return
+ }
+
if (file.type === 'image/svg+xml') {
// SVG files don't need dimension validation
resolve({ width: 0, height: 0 })
return
}📝 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 getImageDimensionsFromFile = ( | |
| file: File | |
| ): Promise<{ width: number; height: number }> => { | |
| return new Promise((resolve, reject) => { | |
| if (file.type === 'image/svg+xml') { | |
| // SVG files don't need dimension validation | |
| resolve({ width: 0, height: 0 }) | |
| return | |
| } | |
| const fr = new FileReader() | |
| fr.readAsDataURL(file) | |
| fr.onload = () => { | |
| const img = new Image() | |
| img.onload = () => { | |
| resolve({ width: img.width, height: img.height }) | |
| } | |
| img.onerror = () => { | |
| reject(new Error(`Failed to load image from file: ${file.name}`)) | |
| } | |
| img.src = fr.result?.toString() || '' | |
| } | |
| fr.onerror = () => { | |
| reject(new Error(`Failed to read file: ${file.name}`)) | |
| } | |
| }) | |
| } | |
| const getImageDimensionsFromFile = ( | |
| file: File | |
| ): Promise<{ width: number; height: number }> => { | |
| return new Promise((resolve, reject) => { | |
| // Check if file type is supported | |
| const supportedTypes = ['image/png', 'image/gif', 'image/jpeg', 'image/svg+xml'] | |
| if (!supportedTypes.includes(file.type)) { | |
| reject(new Error(`Unsupported file type: ${file.type}`)) | |
| return | |
| } | |
| if (file.type === 'image/svg+xml') { | |
| // SVG files don't need dimension validation | |
| resolve({ width: 0, height: 0 }) | |
| return | |
| } | |
| const fr = new FileReader() | |
| fr.readAsDataURL(file) | |
| fr.onload = () => { | |
| const img = new Image() | |
| img.onload = () => { | |
| resolve({ width: img.width, height: img.height }) | |
| } | |
| img.onerror = () => { | |
| reject(new Error(`Failed to load image from file: ${file.name}`)) | |
| } | |
| img.src = fr.result?.toString() || '' | |
| } | |
| fr.onerror = () => { | |
| reject(new Error(`Failed to read file: ${file.name}`)) | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
In
apps/web/src/modules/create-proposal/components/TransactionForm/AddArtwork/AddArtwork.tsx
around lines 39 to 65, the function reads any file without validating its MIME
type first; add a pre-check that only image files are processed (e.g., allow
file.type === 'image/svg+xml' or file.type.startsWith('image/')), and
immediately reject the Promise with a clear error for unsupported MIME types;
keep the existing SVG shortcut and the FileReader/image flow for supported
images, and ensure all early exits call resolve/reject so the Promise never
hangs.
| const fetchableUrls = getFetchableUrls(firstUri) | ||
| if (fetchableUrls && fetchableUrls.length > 0) { | ||
| getImageDimensions(fetchableUrls[0]) | ||
| .then((dimensions) => { | ||
| setExistingDimensions(dimensions) | ||
| }) | ||
| .catch((error) => { | ||
| console.error('Failed to get existing artwork dimensions:', error) | ||
| setExistingDimensions(null) | ||
| }) | ||
| } else { | ||
| console.error('Failed to get fetchable URLs for IPFS URI:', firstUri) | ||
| setExistingDimensions(null) | ||
| } | ||
| }, [properties]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout for image dimension fetching.
The getImageDimensions call could hang indefinitely if the image fails to load from IPFS. Consider adding a timeout.
const fetchableUrls = getFetchableUrls(firstUri)
if (fetchableUrls && fetchableUrls.length > 0) {
- getImageDimensions(fetchableUrls[0])
+ // Add timeout for dimension fetching
+ Promise.race([
+ getImageDimensions(fetchableUrls[0]),
+ new Promise((_, reject) =>
+ setTimeout(() => reject(new Error('Timeout fetching dimensions')), 10000)
+ )
+ ])
.then((dimensions) => {
setExistingDimensions(dimensions)
})
.catch((error) => {
console.error('Failed to get existing artwork dimensions:', error)
setExistingDimensions(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.
| const fetchableUrls = getFetchableUrls(firstUri) | |
| if (fetchableUrls && fetchableUrls.length > 0) { | |
| getImageDimensions(fetchableUrls[0]) | |
| .then((dimensions) => { | |
| setExistingDimensions(dimensions) | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to get existing artwork dimensions:', error) | |
| setExistingDimensions(null) | |
| }) | |
| } else { | |
| console.error('Failed to get fetchable URLs for IPFS URI:', firstUri) | |
| setExistingDimensions(null) | |
| } | |
| }, [properties]) | |
| const fetchableUrls = getFetchableUrls(firstUri) | |
| if (fetchableUrls && fetchableUrls.length > 0) { | |
| // Add timeout for dimension fetching | |
| Promise.race([ | |
| getImageDimensions(fetchableUrls[0]), | |
| new Promise((_, reject) => | |
| setTimeout(() => reject(new Error('Timeout fetching dimensions')), 10000) | |
| ) | |
| ]) | |
| .then((dimensions) => { | |
| setExistingDimensions(dimensions) | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to get existing artwork dimensions:', error) | |
| setExistingDimensions(null) | |
| }) | |
| } else { | |
| console.error('Failed to get fetchable URLs for IPFS URI:', firstUri) | |
| setExistingDimensions(null) | |
| } | |
| }, [properties]) |
| const framePng = await sharp(imageData[i], { page: frameIndex }) | ||
| .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { fit: 'inside' }) | ||
| .png() | ||
| .toBuffer() | ||
|
|
There was a problem hiding this comment.
Fix: Ensure uniform 1080x1080 canvas and correct GIF frame extraction
Current resize uses fit: 'inside', which produces variable frame dimensions. Later, GIFEncoder() is called with fixed SVG_DEFAULT_SIZE width/height, leading to mismatches. Also, extract frames with animated: true for reliability.
- const framePng = await sharp(imageData[i], { page: frameIndex })
- .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { fit: 'inside' })
- .png()
- .toBuffer()
+ const framePng = await sharp(imageData[i], { animated: true })
+ .extractFrame(frameIndex)
+ .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, {
+ fit: 'contain',
+ background: { r: 0, g: 0, b: 0, alpha: 0 },
+ })
+ .png()
+ .toBuffer()- const staticPng = await sharp(imageData[i])
- .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { fit: 'inside' })
- .png()
- .toBuffer()
+ const staticPng = await sharp(imageData[i])
+ .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, {
+ fit: 'contain',
+ background: { r: 0, g: 0, b: 0, alpha: 0 },
+ })
+ .png()
+ .toBuffer()Also applies to: 88-92
| // Create GIF encoder | ||
| const encoder = GIFEncoder() | ||
|
|
||
| // Process all frames to get RGB data | ||
| const allRgbFrames: Uint8Array[] = [] | ||
|
|
||
| for (const frame of compositedFrames) { | ||
| const rgbData = await sharp(frame).raw().toBuffer() | ||
| allRgbFrames.push(rgbData) | ||
| } | ||
|
|
||
| // Create global palette from all frames | ||
| const combinedRgb = new Uint8Array( | ||
| allRgbFrames.reduce((sum, frame) => sum + frame.length, 0) | ||
| ) | ||
| let offset = 0 | ||
| for (const frame of allRgbFrames) { | ||
| combinedRgb.set(frame, offset) | ||
| offset += frame.length | ||
| } | ||
|
|
||
| const globalPalette = quantize(combinedRgb, 256) | ||
|
|
||
| // Encode all frames | ||
| for (const rgbFrame of allRgbFrames) { | ||
| const indexedData = applyPalette(rgbFrame, globalPalette) | ||
|
|
||
| encoder.writeFrame(indexedData, SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { | ||
| palette: globalPalette, | ||
| delay: 100, // 100ms = 10fps | ||
| dispose: 2, // clear to background | ||
| }) | ||
| } | ||
|
|
||
| encoder.finish() | ||
| const result = Buffer.from(encoder.bytesView()) | ||
|
|
There was a problem hiding this comment.
Fix: Pass correct width/height to GIFEncoder, ensure RGBA, and set loop count
Encoder currently uses fixed SVG_DEFAULT_SIZE regardless of the actual frame size, which can corrupt output. Also add encoder.setRepeat(0) for infinite looping and ensure RGBA input to quantize/applyPalette.
- // Create GIF encoder
- const encoder = GIFEncoder()
+ // Create GIF encoder
+ const encoder = GIFEncoder()
+ // Loop forever
+ encoder.setRepeat(0)
- // Process all frames to get RGB data
- const allRgbFrames: Uint8Array[] = []
+ // Process all frames to get RGBA data and dimensions
+ const allRgbFrames: { data: Uint8Array; width: number; height: number }[] = []
- for (const frame of compositedFrames) {
- const rgbData = await sharp(frame).raw().toBuffer()
- allRgbFrames.push(rgbData)
- }
+ for (const frame of compositedFrames) {
+ const { data, info } = await sharp(frame)
+ .ensureAlpha()
+ .raw()
+ .toBuffer({ resolveWithObject: true })
+ // Avoid copying; wrap Buffer into a Uint8Array view
+ const view = new Uint8Array(data.buffer, data.byteOffset, data.byteLength)
+ allRgbFrames.push({ data: view, width: info.width, height: info.height })
+ }
// Create global palette from all frames
- const combinedRgb = new Uint8Array(
- allRgbFrames.reduce((sum, frame) => sum + frame.length, 0)
- )
+ const combinedRgb = new Uint8Array(
+ allRgbFrames.reduce((sum, f) => sum + f.data.length, 0)
+ )
let offset = 0
- for (const frame of allRgbFrames) {
- combinedRgb.set(frame, offset)
- offset += frame.length
+ for (const f of allRgbFrames) {
+ combinedRgb.set(f.data, offset)
+ offset += f.data.length
}
const globalPalette = quantize(combinedRgb, 256)
// Encode all frames
- for (const rgbFrame of allRgbFrames) {
- const indexedData = applyPalette(rgbFrame, globalPalette)
-
- encoder.writeFrame(indexedData, SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, {
+ for (const f of allRgbFrames) {
+ const indexedData = applyPalette(f.data, globalPalette)
+ encoder.writeFrame(indexedData, f.width, f.height, {
palette: globalPalette,
delay: 100, // 100ms = 10fps
dispose: 2, // clear to background
})
}
encoder.finish()
const result = Buffer.from(encoder.bytesView())📝 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.
| // Create GIF encoder | |
| const encoder = GIFEncoder() | |
| // Process all frames to get RGB data | |
| const allRgbFrames: Uint8Array[] = [] | |
| for (const frame of compositedFrames) { | |
| const rgbData = await sharp(frame).raw().toBuffer() | |
| allRgbFrames.push(rgbData) | |
| } | |
| // Create global palette from all frames | |
| const combinedRgb = new Uint8Array( | |
| allRgbFrames.reduce((sum, frame) => sum + frame.length, 0) | |
| ) | |
| let offset = 0 | |
| for (const frame of allRgbFrames) { | |
| combinedRgb.set(frame, offset) | |
| offset += frame.length | |
| } | |
| const globalPalette = quantize(combinedRgb, 256) | |
| // Encode all frames | |
| for (const rgbFrame of allRgbFrames) { | |
| const indexedData = applyPalette(rgbFrame, globalPalette) | |
| encoder.writeFrame(indexedData, SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { | |
| palette: globalPalette, | |
| delay: 100, // 100ms = 10fps | |
| dispose: 2, // clear to background | |
| }) | |
| } | |
| encoder.finish() | |
| const result = Buffer.from(encoder.bytesView()) | |
| // Create GIF encoder | |
| const encoder = GIFEncoder() | |
| // Loop forever | |
| encoder.setRepeat(0) | |
| // Process all frames to get RGBA data and dimensions | |
| const allRgbFrames: { data: Uint8Array; width: number; height: number }[] = [] | |
| for (const frame of compositedFrames) { | |
| const { data, info } = await sharp(frame) | |
| .ensureAlpha() | |
| .raw() | |
| .toBuffer({ resolveWithObject: true }) | |
| // Avoid copying; wrap Buffer into a Uint8Array view | |
| const view = new Uint8Array(data.buffer, data.byteOffset, data.byteLength) | |
| allRgbFrames.push({ data: view, width: info.width, height: info.height }) | |
| } | |
| // Create global palette from all frames | |
| const combinedRgb = new Uint8Array( | |
| allRgbFrames.reduce((sum, f) => sum + f.data.length, 0) | |
| ) | |
| let offset = 0 | |
| for (const f of allRgbFrames) { | |
| combinedRgb.set(f.data, offset) | |
| offset += f.data.length | |
| } | |
| const globalPalette = quantize(combinedRgb, 256) | |
| // Encode all frames | |
| for (const f of allRgbFrames) { | |
| const indexedData = applyPalette(f.data, globalPalette) | |
| encoder.writeFrame(indexedData, f.width, f.height, { | |
| palette: globalPalette, | |
| delay: 100, // 100ms = 10fps | |
| dispose: 2, // clear to background | |
| }) | |
| } | |
| encoder.finish() | |
| const result = Buffer.from(encoder.bytesView()) |
🤖 Prompt for AI Agents
In apps/web/src/pages/api/renderer/stack-images.ts around lines 150-186, fix the
GIF encoding by creating the GIFEncoder with the actual frame width/height,
ensuring frames are RGBA before quantization, and setting the loop count to
infinite: obtain each frame's width/height (e.g., from sharp(frame).metadata()
or a known decoded size) and pass those values to GIFEncoder(width, height);
when reading pixels use sharp(frame).ensureAlpha().raw() so
quantize/applyPalette receive RGBA data; build the combinedRgb from RGBA buffers
accordingly; call encoder.setRepeat(0) before writing frames; and use the actual
width/height when calling encoder.writeFrame and when validating indexed frame
sizes.
| const resizedImages = await Promise.all( | ||
| imageData.map(async (buffer) => | ||
| sharp(buffer) | ||
| .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { fit: 'inside' }) | ||
| .gif() | ||
| .toBuffer() | ||
| ) | ||
| ) | ||
|
|
||
| const compositeParams = resizedImages.slice(1).map((buffer) => ({ | ||
| input: buffer, | ||
| gravity: 'center' as const, | ||
| })) | ||
|
|
||
| const result = await sharp(resizedImages[0]) | ||
| .composite(compositeParams) | ||
| .gif() | ||
| .toBuffer() | ||
| return { buffer: result, contentType: 'image/gif' } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid double GIF encoding for static GIFs; composite as PNG then encode once
Each layer is first encoded to GIF, then decoded for composite, then encoded again to GIF, degrading quality and wasting CPU. Keep layers as PNG (RGBA) during resize/composite and GIF-encode only at the end.
- const resizedImages = await Promise.all(
- imageData.map(async (buffer) =>
- sharp(buffer)
- .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { fit: 'inside' })
- .gif()
- .toBuffer()
- )
- )
+ const resizedImages = await Promise.all(
+ imageData.map(async (buffer) =>
+ sharp(buffer)
+ .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, {
+ fit: 'contain',
+ background: { r: 0, g: 0, b: 0, alpha: 0 },
+ })
+ .png()
+ .toBuffer()
+ )
+ )
const compositeParams = resizedImages.slice(1).map((buffer) => ({
input: buffer,
gravity: 'center' as const,
}))
const result = await sharp(resizedImages[0])
.composite(compositeParams)
.gif()
.toBuffer()📝 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 resizedImages = await Promise.all( | |
| imageData.map(async (buffer) => | |
| sharp(buffer) | |
| .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { fit: 'inside' }) | |
| .gif() | |
| .toBuffer() | |
| ) | |
| ) | |
| const compositeParams = resizedImages.slice(1).map((buffer) => ({ | |
| input: buffer, | |
| gravity: 'center' as const, | |
| })) | |
| const result = await sharp(resizedImages[0]) | |
| .composite(compositeParams) | |
| .gif() | |
| .toBuffer() | |
| return { buffer: result, contentType: 'image/gif' } | |
| const resizedImages = await Promise.all( | |
| imageData.map(async (buffer) => | |
| sharp(buffer) | |
| .resize(SVG_DEFAULT_SIZE, SVG_DEFAULT_SIZE, { | |
| fit: 'contain', | |
| background: { r: 0, g: 0, b: 0, alpha: 0 }, | |
| }) | |
| .png() | |
| .toBuffer() | |
| ) | |
| ) | |
| const compositeParams = resizedImages.slice(1).map((buffer) => ({ | |
| input: buffer, | |
| gravity: 'center' as const, | |
| })) | |
| const result = await sharp(resizedImages[0]) | |
| .composite(compositeParams) | |
| .gif() | |
| .toBuffer() | |
| return { buffer: result, contentType: 'image/gif' } |
| setIsProcessing(true) | ||
| const filesArray = Array.from(files).filter((file) => file.name !== '.DS_Store') | ||
| const acceptableMIME = ['image/png', 'image/svg+xml'] | ||
| const acceptableMIME = ['image/png', 'image/svg+xml', 'image/gif'] |
There was a problem hiding this comment.
JPEG support missing from acceptable MIME types (likely regression).
You’ve added GIF support, but JPEG is omitted. This will reject valid JPEG uploads. Consider also allowing image/webp and image/apng for broader coverage.
Apply this diff:
- const acceptableMIME = ['image/png', 'image/svg+xml', 'image/gif']
+ const acceptableMIME = [
+ 'image/png',
+ 'image/svg+xml',
+ 'image/gif',
+ 'image/jpeg',
+ 'image/jpg',
+ 'image/webp',
+ 'image/apng',
+ ]📝 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 acceptableMIME = ['image/png', 'image/svg+xml', 'image/gif'] | |
| - const acceptableMIME = ['image/png', 'image/svg+xml', 'image/gif'] | |
| + const acceptableMIME = [ | |
| + 'image/png', | |
| + 'image/svg+xml', | |
| + 'image/gif', | |
| + 'image/jpeg', | |
| + 'image/jpg', | |
| + 'image/webp', | |
| + 'image/apng', | |
| + ] |
🤖 Prompt for AI Agents
In packages/hooks/src/useArtworkUpload.ts around line 145, the acceptableMIME
array omits JPEG (and broader modern image formats), which will reject valid
JPEG uploads; update the array to include 'image/jpeg' and 'image/jpg' and also
add 'image/webp' and 'image/apng' for broader coverage, ensuring no duplicates
and preserving existing entries ('image/png', 'image/svg+xml', 'image/gif');
adjust any associated validation/tests if they rely on the old list.
| if (paths.length !== 3 || !paths) { | ||
| if (paths.length > 3) { | ||
| setUploadArtworkError({ | ||
| directory: `file or folder naming incorrect. must not include back slashes.`, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if (paths.length !== 3 || !paths) { | ||
| if (paths.length > 3) { | ||
| setUploadArtworkError({ | ||
| directory: `file or folder naming incorrect. must not include back slashes.`, | ||
| directory: `folder structure is incorrect. download the demo folder to compare.`, | ||
| }) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Reducer early-returns break accumulation and can crash subsequent iterations.
Bare return inside reduce returns undefined as the accumulator, which will cause acc.push(...) later to throw. Always return acc to short-circuit while preserving the accumulator. Also, || !paths is dead code (paths is always truthy after split).
Apply this diff:
- if (paths.length !== 3 || !paths) {
+ if (paths.length !== 3) {
if (paths.length > 3) {
setUploadArtworkError({
- directory: `file or folder naming incorrect. must not include back slashes.`,
+ directory: `Folder structure is too deep. Expected: <collection>/<trait>/<property.ext>`,
})
- return
+ return acc
}
setUploadArtworkError({
- directory: `folder structure is incorrect. download the demo folder to compare.`,
+ directory: `Folder structure is incorrect. Download the demo folder to compare.`,
})
- return
+ return acc
}
// forward slashes seem to be converted to `:`
// check for both folder and file name
if (
cv.name.includes(':') ||
paths[2]?.includes(':') ||
cv.name.split('.').length !== 2 ||
paths[1].split('.').length !== 1
) {
setUploadArtworkError({
- directory: `file or folder naming incorrect. must not include forward slashes or periods.`,
+ directory: `File/folder naming incorrect. Traits must not contain periods. Files must not contain ":" and must have exactly one "." before the extension.`,
})
- return
+ return acc
}
if (cv.type.length && !acceptableMIME.includes(cv.type)) {
setUploadArtworkError({
mime: `${cv.type} is an unsupported file type - file: ${cv.name}`,
})
- return
+ return acc
}
if (traits.length > 10) {
setUploadArtworkError({
maxTraits: `Maximum of 10 traits per collection. Your upload includes ${traits.length} traits.`,
})
- return
+ return acc
}
if (index > 0 && filesArray[index - 1].type !== cv.type) {
setUploadArtworkError({
mime: `All file types must be the same.`,
})
- return
+ return acc
}Also applies to: 206-231
🤖 Prompt for AI Agents
In packages/hooks/src/useArtworkUpload.ts around lines 158-171 (and likewise
206-231), the reduce callback uses bare return statements which return undefined
as the accumulator and can crash later when acc.push(...) is called; also the
condition `|| !paths` is dead because paths is always truthy after split. Fix by
removing the `|| !paths` check, and in each early-exit branch inside the reducer
replace bare `return` with `return acc` so the accumulator is preserved; keep
the error-setting logic as-is (or refactor to set errors before returning acc)
and ensure all code paths in the reducer return the accumulator.
| /* get image size */ | ||
| const fr = new FileReader() | ||
| fr.readAsDataURL(cv) | ||
| const getImageSize = (fr: FileReader, count: number) => { | ||
| let img = new Image() | ||
| img.src = fr.result?.toString() || '' | ||
| img.onload = function () { | ||
| let height = img.height | ||
| let width = img.width | ||
| let min = 600 | ||
|
|
||
| if ((height < min || width < min) && cv.type !== 'image/svg+xml') { | ||
| setUploadArtworkError({ | ||
| dimensions: `we recommend images of min, 600px width x height, your images are width: ${width} x ${height} px`, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if (height !== width) { | ||
| setUploadArtworkError({ | ||
| dimensions: `images must be of equal height and width, your images are width: ${width} x ${height} px`, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| if (count === filesArray?.length - 1) { | ||
| setIsProcessing(false) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| fr.onload = () => getImageSize(fr, filesArray.indexOf(cv)) | ||
| fr.onload = () => getImageSize(fr, filesArray.indexOf(cv)) | ||
|
|
There was a problem hiding this comment.
Ensure isProcessing resets even when the last file fails validation; also avoid O(n) index lookup and set onload before read.
If the last file triggers a validation error, the early return prevents setIsProcessing(false) and the hook can remain stuck. Also, use the reducer’s index instead of filesArray.indexOf(cv) and attach onload before starting the read.
Apply this diff:
- /* get image size */
- const fr = new FileReader()
- fr.readAsDataURL(cv)
- const getImageSize = (fr: FileReader, count: number) => {
+ /* get image size */
+ const fr = new FileReader()
+ const getImageSize = (fr: FileReader, count: number) => {
let img = new Image()
img.src = fr.result?.toString() || ''
img.onload = function () {
+ const isLast = count === filesArray.length - 1
let height = img.height
let width = img.width
let min = 600
if ((height < min || width < min) && cv.type !== 'image/svg+xml') {
setUploadArtworkError({
- dimensions: `we recommend images of min, 600px width x height, your images are width: ${width} x ${height} px`,
+ dimensions: `We recommend a minimum of 600px width and height. Your image is ${width}x${height}px.`,
})
- return
+ if (isLast) setIsProcessing(false)
+ return
}
if (height !== width) {
setUploadArtworkError({
- dimensions: `images must be of equal height and width, your images are width: ${width} x ${height} px`,
+ dimensions: `Images must be square. Your image is ${width}x${height}px.`,
})
- return
+ if (isLast) setIsProcessing(false)
+ return
}
- if (count === filesArray?.length - 1) {
+ if (isLast) {
setIsProcessing(false)
}
}
}
- fr.onload = () => getImageSize(fr, filesArray.indexOf(cv))
+ fr.onload = () => getImageSize(fr, index)
+ fr.readAsDataURL(cv)📝 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.
| /* get image size */ | |
| const fr = new FileReader() | |
| fr.readAsDataURL(cv) | |
| const getImageSize = (fr: FileReader, count: number) => { | |
| let img = new Image() | |
| img.src = fr.result?.toString() || '' | |
| img.onload = function () { | |
| let height = img.height | |
| let width = img.width | |
| let min = 600 | |
| if ((height < min || width < min) && cv.type !== 'image/svg+xml') { | |
| setUploadArtworkError({ | |
| dimensions: `we recommend images of min, 600px width x height, your images are width: ${width} x ${height} px`, | |
| }) | |
| return | |
| } | |
| if (height !== width) { | |
| setUploadArtworkError({ | |
| dimensions: `images must be of equal height and width, your images are width: ${width} x ${height} px`, | |
| }) | |
| return | |
| } | |
| if (count === filesArray?.length - 1) { | |
| setIsProcessing(false) | |
| } | |
| } | |
| } | |
| } | |
| fr.onload = () => getImageSize(fr, filesArray.indexOf(cv)) | |
| fr.onload = () => getImageSize(fr, filesArray.indexOf(cv)) | |
| /* get image size */ | |
| const fr = new FileReader() | |
| const getImageSize = (fr: FileReader, count: number) => { | |
| let img = new Image() | |
| img.src = fr.result?.toString() || '' | |
| img.onload = function () { | |
| const isLast = count === filesArray.length - 1 | |
| let height = img.height | |
| let width = img.width | |
| let min = 600 | |
| if ((height < min || width < min) && cv.type !== 'image/svg+xml') { | |
| setUploadArtworkError({ | |
| dimensions: `We recommend a minimum of 600px width and height. Your image is ${width}x${height}px.`, | |
| }) | |
| if (isLast) setIsProcessing(false) | |
| return | |
| } | |
| if (height !== width) { | |
| setUploadArtworkError({ | |
| dimensions: `Images must be square. Your image is ${width}x${height}px.`, | |
| }) | |
| if (isLast) setIsProcessing(false) | |
| return | |
| } | |
| if (isLast) { | |
| setIsProcessing(false) | |
| } | |
| } | |
| } | |
| fr.onload = () => getImageSize(fr, index) | |
| fr.readAsDataURL(cv) |
🤖 Prompt for AI Agents
In packages/hooks/src/useArtworkUpload.ts around lines 233 to 264, the
image-validation logic attaches FileReader.onload after calling readAsDataURL,
uses filesArray.indexOf(cv) (O(n)) and returns early on validation failures
without resetting setIsProcessing(false); change to attach fr.onload before
fr.readAsDataURL, accept the current file index from the reducer and pass it
into getImageSize instead of filesArray.indexOf(cv), and ensure every validation
early-return calls setIsProcessing(false) (or move the finalization to a
finally-style path) so processing is cleared even when the last file fails
validation.
dan13ram
force-pushed
the
feat/support-animated-gifs
branch
from
8db6e26 to
3d138cc
Compare
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores