Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
WalkthroughAdds an Icon Library Generator: backend Convex schema, queries/mutations/actions with server-side SVG validation and upload handling; frontend listing/editor routes and components for upload, sketchy previews, reordering, deletion, and export; SVG conversion/validation libs; seeding and test scripts; E2E scenarios and minor config/dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as Frontend UI
participant Convex as Convex Backend
participant Storage as File Storage
User->>Frontend: Select SVG files & click "Upload"
Frontend->>Frontend: Validate files (size/type)
Frontend->>Convex: Call generateUploadUrl()
Convex-->>Frontend: Return upload URL
Frontend->>Storage: Upload SVG to URL
Storage-->>Frontend: Return storageId
Frontend->>Convex: Call addIcon(libraryId, storageId, originalName)
Convex->>Convex: Read storage object, validate SVG, compute hash, create icon record
Convex-->>Frontend: Return fileName
Frontend->>Frontend: Refresh icon grid / show toast
sequenceDiagram
participant User as User
participant Frontend as Frontend UI
participant Storage as File Storage
participant Browser as Browser Download
User->>Frontend: Click "Export .excalidrawlib"
Frontend->>Storage: Fetch each icon SVG (via resolved URLs)
Storage-->>Frontend: Return SVG text
Frontend->>Frontend: Convert SVGs -> Excalidraw elements (svgToExcalidraw + styleSettings)
Frontend->>Frontend: Build library JSON, create Blob
Frontend->>Browser: Trigger download (anchor click)
Browser-->>User: Save .excalidrawlib file
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@apps/web/src/app/library-generator/`[id]/page.tsx:
- Around line 79-106: The handleSave function is missing bowing, randomize, and
pencilFilter when building the styleSettings payload for updateLibrary; update
the payload in handleSave (the object passed to updateLibrary in handleSave) to
include styleSettings.bowing, styleSettings.randomize, and
styleSettings.pencilFilter (pulling them from the local styleSettings state) and
remove/adjust the forced unknown cast so the payload matches the backend
StyleSettings shape or explicitly type the object to Parameters<typeof
updateLibrary>[0]["styleSettings"] to avoid dropping these properties.
- Around line 51-67: The effect in useEffect reads dbSettings but overwrites
bowing, randomize, and pencilFilter with hardcoded values; update the
setStyleSettings call to pull bowing, randomize, and pencilFilter from
dbSettings (casting/transforming as needed) and only fall back to the current
defaults when those keys are undefined, so setStyleSettings uses
dbSettings.fillStyle, dbSettings.roughness, dbSettings.bowing,
dbSettings.randomize, and dbSettings.pencilFilter (with appropriate type
coercion to match StyleSettings) instead of fixed values.
In `@apps/web/src/components/icon-library/library-card.tsx`:
- Around line 38-49: The preview images in LibraryCard are rendered with an
empty alt which is inaccessible; update the previewUrls.map callback to include
the index (e.g., previewUrls.map((url, idx) => ...)) and set the Image alt prop
to a meaningful string such as `${libraryName} preview ${idx + 1}` (use the
component prop or variable that holds the library name) so each Image has
descriptive alt text; modify the JSX in library-card.tsx where Image is used to
pass that computed alt.
In `@apps/web/src/lib/icon-library/svg-to-excalidraw.ts`:
- Around line 140-241: The function svgToExcalidrawElements creates a DOMParser
unconditionally which will throw in SSR; add an explicit guard at the top of
svgToExcalidrawElements that checks typeof window !== "undefined" (or typeof
document !== "undefined") before instantiating DOMParser and, if not present,
throw a clear error like "svgToExcalidrawElements must be called in a browser
environment"; this ensures the DOMParser creation and subsequent
parser.parseFromString calls are only attempted on the client and fail fast with
a clear message when run server-side.
In `@packages/backend/convex/iconLibraries.ts`:
- Around line 232-243: The deleteIcon mutation currently deletes the DB record
then calls ctx.storage.delete which can orphan storage if the storage delete
fails; update deleteIcon (use the existing handler, ctx.db.get, ctx.db.delete,
ctx.storage.delete identifiers) to delete the storage first or wrap the sequence
in try/catch so that you only call ctx.db.delete after ctx.storage.delete
succeeds, and if you must delete the DB first add rollback logic to re-create or
restore the DB record on storage failure and log the error; ensure errors from
ctx.storage.delete are propagated or handled so orphaned storage cannot occur.
- Around line 6-14: DEFAULT_STYLE_SETTINGS in iconLibraries.ts currently defines
strokeColor, backgroundColor, strokeWidth, strokeStyle, fillStyle, roughness,
and opacity which does not match the frontend StyleSettings in
svg-to-excalidraw.ts (which expects fillStyle, roughness, bowing, randomize,
pencilFilter). Fix by updating the backend schema and DEFAULT_STYLE_SETTINGS to
include the frontend fields (add bowing, randomize, pencilFilter with sensible
defaults and keep fillStyle/roughness) or implement a clear transformation
layer: create a converter function (e.g., mapDbStyleToFrontendStyle and
mapFrontendStyleToDbStyle) used wherever library settings are read/written to
convert between DEFAULT_STYLE_SETTINGS and the frontend StyleSettings, and add a
migration to persist missing fields if needed; reference DEFAULT_STYLE_SETTINGS
and the frontend StyleSettings type in svg-to-excalidraw.ts when making the
change.
In `@packages/backend/convex/iconLibrariesActions.ts`:
- Around line 52-81: The current flow creates and validates the SVG then calls
ctx.runMutation(internal.iconLibraries.addIconRecord) but if that mutation
throws the uploaded blob (storageId) is left orphaned; wrap the call to
ctx.runMutation(internal.iconLibraries.addIconRecord, {...}) in a try/catch, and
in the catch await ctx.storage.delete(storageId) before rethrowing the error so
any failure (e.g., invalid libraryId) cleans up the uploaded file; keep all
preceding logic (ctx.storage.get, validateSvgText, hashText, sanitizeFileStem)
intact and ensure the deletion happens only on mutation failure.
In `@packages/backend/convex/schema.ts`:
- Around line 4-21: The styleSettings schema is missing three fields the UI
sends; add bowing: v.number(), randomize: v.boolean(), and pencilFilter:
v.string() to the styleSettings object in schema.ts (alongside strokeColor,
backgroundColor, strokeWidth, strokeStyle, fillStyle, roughness, opacity), and
mirror these same fields and types in the validator used by the update mutation
in iconLibraries.ts so incoming updates are accepted and not dropped.
In `@scripts/test-svg-conversion.ts`:
- Around line 25-33: The test's styleSettings object doesn't match the
StyleSettings interface used by svgToExcalidrawElements; update the test to
construct a StyleSettings-typed object (use StyleSettings as the type for the
mock) and include the missing required properties (add bowing, randomize,
pencilFilter with appropriate values) and remove or replace any non-interface
fields (e.g., replace opacity with the correct alpha/opacity field if defined in
StyleSettings); ensure the variable referenced by the mock uses the
StyleSettings type so the compiler enforces the shape when calling
svgToExcalidrawElements.
🧹 Nitpick comments (15)
packages/backend/convex/iconLibrariesActions.ts (1)
6-40: Consider reusing the shared SVG validation utilities to avoid drift.
Ifpackages/backend/lib/icon-library/svg-validate.tsis the canonical validator, importingMAX_SVG_BYTESandvalidateSvgTexthere will keep backend validation consistent.scripts/test-svg-conversion.ts (1)
74-83: Hardcoded Playwright path is fragile.The path assumes Playwright is installed in
packages/backend/node_modules. This may break if:
- Dependencies are hoisted to the root
node_modules- The package manager changes (e.g., pnpm with different linking)
- Running from a different working directory
Consider using
require.resolve('playwright')or importing directly if Playwright is a dev dependency of the script's package.♻️ Suggested fix
-const playwrightPath = join( - repoRoot, - "packages", - "backend", - "node_modules", - "playwright", - "index.js" -); -const playwright = await import(pathToFileURL(playwrightPath).href); -const { chromium } = playwright as typeof import("playwright"); +// If playwright is a direct dependency: +import { chromium } from "playwright"; + +// Or if dynamic resolution is needed: +// const { chromium } = await import("playwright");packages/backend/convex/iconLibraries.ts (2)
24-39: Unbounded loop in getUniqueSlug.The
while (true)loop could theoretically run indefinitely if slug collisions persist (e.g., due to a bug or malicious input). Consider adding a maximum attempt limit.♻️ Suggested fix
const getUniqueSlug = async (ctx: { db: DatabaseReader }, base: string) => { let slug = base; let attempt = 1; + const maxAttempts = 100; - while (true) { + while (attempt <= maxAttempts) { const existing = await ctx.db .query("iconLibraries") .withIndex("by_slug", (q) => q.eq("slug", slug)) .unique(); if (!existing) { return slug; } attempt += 1; slug = `${base}-${attempt}`; } + throw new Error("Unable to generate unique slug after maximum attempts."); };
41-71: N+1 query pattern in list().For each library, the handler queries all icons and then fetches storage URLs for up to 9 icons. With many libraries, this creates O(n) database queries + O(n*9) storage calls.
For now this is acceptable if the library count is small, but consider pagination or denormalizing
iconCountonto the library document if performance becomes an issue.apps/web/src/app/library-generator/page.tsx (1)
76-93: Consider adding Enter key support for library creation.Users may expect to press Enter in the input field to create a library. Currently, only the button click triggers creation.
♻️ Suggested enhancement
- <div className="flex flex-col gap-3 sm:flex-row sm:items-center"> + <form + className="flex flex-col gap-3 sm:flex-row sm:items-center" + onSubmit={(e) => { + e.preventDefault(); + handleCreate(); + }} + > <Input onChange={(event) => setLibraryName(event.target.value)} placeholder="Library name" value={libraryName} /> <Button disabled={isCreating} - onClick={handleCreate} size="sm" - type="button" + type="submit" > {isCreating ? "Creating…" : "Create"} </Button> - </div> + </form>apps/web/src/components/icon-library/style-controls.tsx (2)
31-40: Consider adding ARIA attributes for slider accessibility.Native range inputs benefit from ARIA attributes to improve screen reader experience.
♻️ Suggested enhancement
<input + aria-valuemax={10} + aria-valuemin={0} + aria-valuenow={value.roughness} className="h-2 w-full cursor-pointer appearance-none rounded-lg bg-muted" id="roughness" max="10" min="0" onChange={(e) => update("roughness", Number(e.target.value))} step="0.5" type="range" value={value.roughness} />
64-80: Native select may not match design system.The select uses inline styles while other form elements use shadcn/ui components. Consider using a consistent
Selectcomponent from the UI library if one exists, or document the intentional choice.apps/web/src/components/icon-library/icon-grid.tsx (1)
117-141: Consider improving accessibility for the selection button.The selection button uses a custom checkbox pattern. While the
aria-labelis good, consider addingaria-pressedto indicate the toggle state for better screen reader support.♻️ Suggested improvement
<button aria-label={isSelected ? "Deselect icon" : "Select icon"} + aria-pressed={isSelected} className="absolute top-1 left-1 flex h-5 w-5 items-center justify-center rounded border bg-background/80 transition-colors hover:bg-background" onClick={() => toggleSelect(icon.id)} type="button" >apps/web/src/components/icon-library/svg-uploader.tsx (2)
16-26: Add error handling for the async upload callback.The
handleChangefunction is async and callsonUploadwithout a try-catch. IfonUploadthrows, the input won't be reset. Per coding guidelines, async code should handle errors appropriately.♻️ Suggested improvement
const handleChange = async (event: React.ChangeEvent<HTMLInputElement>) => { if (!event.target.files || event.target.files.length === 0) { return; } - await onUpload(event.target.files); - - if (inputRef.current) { - inputRef.current.value = ""; + try { + await onUpload(event.target.files); + } finally { + if (inputRef.current) { + inputRef.current.value = ""; + } } };
43-50: Add accessible label to the hidden file input.The hidden file input lacks an accessible name. While the button triggers it, screen readers may still announce the input without context. Add
aria-labelor associate it with a label. Per coding guidelines, add labels for form inputs.♻️ Suggested improvement
<input accept="image/svg+xml" + aria-label="Upload SVG files" className="hidden" multiple onChange={handleChange} ref={inputRef} type="file" />apps/web/src/components/icon-library/sketchy-icon-preview.tsx (2)
48-82: Consider using async/await for cleaner fetch logic.The coding guidelines prefer
async/awaitsyntax over promise chains for better readability. However, the current implementation with thecancelledflag is functionally correct.♻️ Suggested refactor using async/await
useEffect(() => { if (!svgUrl) { setSvgText(null); setError(null); return; } let cancelled = false; - setIsLoading(true); - setError(null); - - fetch(svgUrl) - .then((res) => { - if (!res.ok) { - throw new Error(`Failed to fetch SVG: ${res.status}`); - } - return res.text(); - }) - .then((text) => { - if (!cancelled) { - setSvgText(text); - setIsLoading(false); - } - }) - .catch((err) => { - if (!cancelled) { - setError(err instanceof Error ? err.message : "Failed to fetch SVG"); - setIsLoading(false); - } - }); + + const fetchSvg = async () => { + setIsLoading(true); + setError(null); + + try { + const res = await fetch(svgUrl); + if (!res.ok) { + throw new Error(`Failed to fetch SVG: ${res.status}`); + } + const text = await res.text(); + if (!cancelled) { + setSvgText(text); + } + } catch (err) { + if (!cancelled) { + setError(err instanceof Error ? err.message : "Failed to fetch SVG"); + } + } finally { + if (!cancelled) { + setIsLoading(false); + } + } + }; + + fetchSvg(); return () => { cancelled = true; }; }, [svgUrl]);
136-153: Loading and error indicators may overlap with the rendered sketch.The loading/error spans and the container div are siblings, so when the sketch renders successfully, previous loading text could briefly appear alongside it. Consider conditionally hiding the container or clearing states more explicitly.
♻️ Suggested improvement
return ( <div className="relative flex aspect-square items-center justify-center rounded border bg-muted/30"> {isLoading && ( <span className="text-[10px] text-muted-foreground">Loading...</span> )} - {error && ( + {error && !isLoading && ( <span className="text-[10px] text-destructive" title={error}> Error </span> )} <div aria-label={`Sketchy preview of ${name}`} - className="flex h-full w-full items-center justify-center [&>svg]:max-h-full [&>svg]:max-w-full" + className={`flex h-full w-full items-center justify-center [&>svg]:max-h-full [&>svg]:max-w-full ${isLoading || error ? "hidden" : ""}`} ref={containerRef} role="img" /> </div> );apps/web/src/app/library-generator/[id]/page.tsx (2)
144-160: Sequential uploads may be slow for multiple files.Files are uploaded one at a time using
for...ofwithawait. For many files, consider usingPromise.allSettledfor parallel uploads with individual error handling, or limit concurrency with a batching approach.♻️ Example parallel upload with error collection
const handleUpload = async (files: FileList) => { if (isUploading) { return; } setIsUploading(true); try { - for (const file of Array.from(files)) { - await uploadSvgFile(file); - } + const results = await Promise.allSettled( + Array.from(files).map((file) => uploadSvgFile(file)) + ); + const failures = results.filter((r) => r.status === "rejected"); + if (failures.length > 0) { + toast.error(`${failures.length} file(s) failed to upload.`); + } } catch (error) { const message = error instanceof Error ? error.message : "Upload failed."; toast.error(message); } finally { setIsUploading(false); } };Note: This requires
uploadSvgFileto throw on failure rather than just showing a toast. You may need to refactor error handling accordingly.
162-175: Sequential icon deletion may be slow for bulk operations.Similar to uploads, icons are deleted sequentially. Consider parallel deletion for better UX when deleting many icons.
apps/web/src/components/icon-library/export-button.tsx (1)
59-77: Consider parallel fetching for better export performance.Icons are fetched sequentially, which can be slow for large libraries. Parallel fetching would improve UX.
♻️ Suggested parallel approach
- for (const icon of icons) { - if (!icon.url) { - throw new Error(`Missing icon URL for ${icon.name}.`); - } - const response = await fetch(icon.url); - if (!response.ok) { - throw new Error(`Failed to load ${icon.name}.`); - } - const svgText = await response.text(); - const elements = svgToExcalidrawElements(svgText, styleSettings); - - libraryItems.push({ - id: randomId(), - status: "published", - created: Date.now(), - name: icon.name, - elements, - }); - } + const results = await Promise.all( + icons.map(async (icon) => { + if (!icon.url) { + throw new Error(`Missing icon URL for ${icon.name}.`); + } + const response = await fetch(icon.url); + if (!response.ok) { + throw new Error(`Failed to load ${icon.name}.`); + } + const svgText = await response.text(); + const elements = svgToExcalidrawElements(svgText, styleSettings); + + return { + id: randomId(), + status: "published" as const, + created: Date.now(), + name: icon.name, + elements, + }; + }) + ); + libraryItems.push(...results);
| useEffect(() => { | ||
| if (data?.library) { | ||
| setLibraryName(data.library.name); | ||
| const dbSettings = data.library.styleSettings as unknown as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
| setStyleSettings({ | ||
| fillStyle: (dbSettings.fillStyle || | ||
| "hachure") as StyleSettings["fillStyle"], | ||
| roughness: (dbSettings.roughness as number) ?? 1, | ||
| bowing: 1, | ||
| randomize: true, | ||
| pencilFilter: false, | ||
| }); | ||
| } | ||
| }, [data?.library]); |
There was a problem hiding this comment.
Database style settings are partially ignored.
The effect hardcodes bowing: 1, randomize: true, and pencilFilter: false instead of reading them from dbSettings. This means user-saved settings for these properties will be lost on page load.
🐛 Proposed fix
useEffect(() => {
if (data?.library) {
setLibraryName(data.library.name);
const dbSettings = data.library.styleSettings as unknown as Record<
string,
unknown
>;
setStyleSettings({
fillStyle: (dbSettings.fillStyle ||
"hachure") as StyleSettings["fillStyle"],
roughness: (dbSettings.roughness as number) ?? 1,
- bowing: 1,
- randomize: true,
- pencilFilter: false,
+ bowing: (dbSettings.bowing as number) ?? 1,
+ randomize: (dbSettings.randomize as boolean) ?? true,
+ pencilFilter: (dbSettings.pencilFilter as boolean) ?? false,
});
}
}, [data?.library]);📝 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.
| useEffect(() => { | |
| if (data?.library) { | |
| setLibraryName(data.library.name); | |
| const dbSettings = data.library.styleSettings as unknown as Record< | |
| string, | |
| unknown | |
| >; | |
| setStyleSettings({ | |
| fillStyle: (dbSettings.fillStyle || | |
| "hachure") as StyleSettings["fillStyle"], | |
| roughness: (dbSettings.roughness as number) ?? 1, | |
| bowing: 1, | |
| randomize: true, | |
| pencilFilter: false, | |
| }); | |
| } | |
| }, [data?.library]); | |
| useEffect(() => { | |
| if (data?.library) { | |
| setLibraryName(data.library.name); | |
| const dbSettings = data.library.styleSettings as unknown as Record< | |
| string, | |
| unknown | |
| >; | |
| setStyleSettings({ | |
| fillStyle: (dbSettings.fillStyle || | |
| "hachure") as StyleSettings["fillStyle"], | |
| roughness: (dbSettings.roughness as number) ?? 1, | |
| bowing: (dbSettings.bowing as number) ?? 1, | |
| randomize: (dbSettings.randomize as boolean) ?? true, | |
| pencilFilter: (dbSettings.pencilFilter as boolean) ?? false, | |
| }); | |
| } | |
| }, [data?.library]); |
🤖 Prompt for AI Agents
In `@apps/web/src/app/library-generator/`[id]/page.tsx around lines 51 - 67, The
effect in useEffect reads dbSettings but overwrites bowing, randomize, and
pencilFilter with hardcoded values; update the setStyleSettings call to pull
bowing, randomize, and pencilFilter from dbSettings (casting/transforming as
needed) and only fall back to the current defaults when those keys are
undefined, so setStyleSettings uses dbSettings.fillStyle, dbSettings.roughness,
dbSettings.bowing, dbSettings.randomize, and dbSettings.pencilFilter (with
appropriate type coercion to match StyleSettings) instead of fixed values.
| const handleSave = async () => { | ||
| if (isSaving) { | ||
| return; | ||
| } | ||
| setIsSaving(true); | ||
| try { | ||
| await updateLibrary({ | ||
| id: libraryId, | ||
| name: libraryName.trim() || "Untitled Library", | ||
| styleSettings: { | ||
| strokeColor: "#000000", | ||
| backgroundColor: "transparent", | ||
| strokeWidth: 2, | ||
| strokeStyle: "solid" as const, | ||
| fillStyle: styleSettings.fillStyle, | ||
| roughness: styleSettings.roughness, | ||
| opacity: 100, | ||
| } as unknown as Parameters<typeof updateLibrary>[0]["styleSettings"], | ||
| }); | ||
| toast.success("Library updated."); | ||
| } catch (error) { | ||
| const message = | ||
| error instanceof Error ? error.message : "Failed to save library."; | ||
| toast.error(message); | ||
| } finally { | ||
| setIsSaving(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Style settings bowing, randomize, and pencilFilter are not persisted.
The handleSave function sends style settings to updateLibrary, but the payload doesn't include bowing, randomize, or pencilFilter. These settings will be lost on save and reload.
🐛 Ensure all style settings are persisted
The payload in handleSave (lines 88-96) appears to map to a different schema format. Verify that the backend schema supports the full StyleSettings interface or update accordingly. The current implementation drops bowing, randomize, and pencilFilter.
🤖 Prompt for AI Agents
In `@apps/web/src/app/library-generator/`[id]/page.tsx around lines 79 - 106, The
handleSave function is missing bowing, randomize, and pencilFilter when building
the styleSettings payload for updateLibrary; update the payload in handleSave
(the object passed to updateLibrary in handleSave) to include
styleSettings.bowing, styleSettings.randomize, and styleSettings.pencilFilter
(pulling them from the local styleSettings state) and remove/adjust the forced
unknown cast so the payload matches the backend StyleSettings shape or
explicitly type the object to Parameters<typeof
updateLibrary>[0]["styleSettings"] to avoid dropping these properties.
| : previewUrls.map((url) => ( | ||
| <div | ||
| className="relative flex aspect-square items-center justify-center rounded border bg-muted/30" | ||
| key={url} | ||
| > | ||
| <Image | ||
| alt="" | ||
| className="object-contain" | ||
| fill | ||
| sizes="96px" | ||
| src={url} | ||
| unoptimized |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/components/icon-library/library-card.tsx | head -60Repository: anand-testcompare/sketchi
Length of output: 2110
Provide meaningful alt text for preview images.
These preview images convey information about the library's icons; empty alt text makes them inaccessible to screen readers. Use the library name and index to create descriptive alt text.
♿ Suggested fix
- : previewUrls.map((url) => (
+ : previewUrls.map((url, index) => (
<div
className="relative flex aspect-square items-center justify-center rounded border bg-muted/30"
key={url}
>
<Image
- alt=""
+ alt={`${name} icon preview ${index + 1}`}
className="object-contain"
fill
sizes="96px"
src={url}
unoptimized📝 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.
| : previewUrls.map((url) => ( | |
| <div | |
| className="relative flex aspect-square items-center justify-center rounded border bg-muted/30" | |
| key={url} | |
| > | |
| <Image | |
| alt="" | |
| className="object-contain" | |
| fill | |
| sizes="96px" | |
| src={url} | |
| unoptimized | |
| : previewUrls.map((url, index) => ( | |
| <div | |
| className="relative flex aspect-square items-center justify-center rounded border bg-muted/30" | |
| key={url} | |
| > | |
| <Image | |
| alt={`${name} icon preview ${index + 1}`} | |
| className="object-contain" | |
| fill | |
| sizes="96px" | |
| src={url} | |
| unoptimized | |
| /> | |
| </div> | |
| )) |
🤖 Prompt for AI Agents
In `@apps/web/src/components/icon-library/library-card.tsx` around lines 38 - 49,
The preview images in LibraryCard are rendered with an empty alt which is
inaccessible; update the previewUrls.map callback to include the index (e.g.,
previewUrls.map((url, idx) => ...)) and set the Image alt prop to a meaningful
string such as `${libraryName} preview ${idx + 1}` (use the component prop or
variable that holds the library name) so each Image has descriptive alt text;
modify the JSX in library-card.tsx where Image is used to pass that computed
alt.
| export const svgToExcalidrawElements = ( | ||
| svgText: string, | ||
| styleSettings: StyleSettings | ||
| ) => { | ||
| const parser = new DOMParser(); | ||
| const doc = parser.parseFromString(svgText, "image/svg+xml"); | ||
| const parsedSvg = doc.querySelector("svg") as SVGSVGElement | null; | ||
| const parseError = doc.querySelector("parsererror"); | ||
|
|
||
| if (!parsedSvg || parseError) { | ||
| throw new Error("Unable to parse SVG."); | ||
| } | ||
|
|
||
| let container: HTMLDivElement | null = null; | ||
| let svg = parsedSvg; | ||
|
|
||
| if (typeof document !== "undefined" && document.body) { | ||
| container = document.createElement("div"); | ||
| container.style.cssText = | ||
| "position:absolute;left:-99999px;top:-99999px;width:0;height:0;overflow:hidden;visibility:hidden;"; | ||
| container.appendChild(document.importNode(parsedSvg, true)); | ||
| document.body.appendChild(container); | ||
| const attached = container.querySelector("svg") as SVGSVGElement | null; | ||
| if (attached) { | ||
| svg = attached; | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| const geometryElements = Array.from( | ||
| svg.querySelectorAll("path,rect,circle,ellipse,line,polyline,polygon") | ||
| ).filter((element) => "getTotalLength" in element) as SVGGeometryElement[]; | ||
|
|
||
| const pathPoints = geometryElements | ||
| .map((element) => sampleGeometryPoints(element, svg)) | ||
| .filter((points) => points.length > 1); | ||
|
|
||
| if (pathPoints.length === 0) { | ||
| throw new Error("SVG has no usable geometry."); | ||
| } | ||
|
|
||
| const flattened = pathPoints.flat(); | ||
| const bounds = computeBounds(flattened); | ||
| const width = bounds.maxX - bounds.minX; | ||
| const height = bounds.maxY - bounds.minY; | ||
| const scale = | ||
| width > 0 || height > 0 ? TARGET_SIZE / Math.max(width, height) : 1; | ||
|
|
||
| const groupId = randomId(); | ||
| const updated = Date.now(); | ||
|
|
||
| return pathPoints.map((points) => { | ||
| const scaledPoints = points.map((point) => ({ | ||
| x: (point.x - bounds.minX) * scale, | ||
| y: (point.y - bounds.minY) * scale, | ||
| })); | ||
|
|
||
| const localBounds = computeBounds(scaledPoints); | ||
| const elementX = localBounds.minX; | ||
| const elementY = localBounds.minY; | ||
| const elementPoints = scaledPoints.map( | ||
| (point) => | ||
| [point.x - localBounds.minX, point.y - localBounds.minY] as [ | ||
| number, | ||
| number, | ||
| ] | ||
| ); | ||
|
|
||
| return { | ||
| type: "freedraw", | ||
| version: 1, | ||
| versionNonce: randomInt(), | ||
| isDeleted: false, | ||
| id: randomId(), | ||
| fillStyle: styleSettings.fillStyle, | ||
| strokeWidth: 2, | ||
| strokeStyle: "solid", | ||
| roughness: styleSettings.roughness, | ||
| opacity: 100, | ||
| angle: 0, | ||
| x: elementX, | ||
| y: elementY, | ||
| strokeColor: "#000000", | ||
| backgroundColor: "transparent", | ||
| width: localBounds.maxX - localBounds.minX, | ||
| height: localBounds.maxY - localBounds.minY, | ||
| seed: randomInt(), | ||
| groupIds: [groupId], | ||
| strokeSharpness: "sharp", | ||
| boundElements: [], | ||
| updated, | ||
| link: null, | ||
| points: elementPoints, | ||
| lastCommittedPoint: null, | ||
| simulatePressure: true, | ||
| pressures: [], | ||
| } satisfies ExcalidrawElement; | ||
| }); | ||
| } finally { | ||
| container?.remove(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "svgToExcalidrawElements" --type ts --type tsx -B 3 -A 3Repository: anand-testcompare/sketchi
Length of output: 96
🏁 Script executed:
rg -n "svgToExcalidrawElements" -t ts -B 3 -A 3Repository: anand-testcompare/sketchi
Length of output: 3362
🏁 Script executed:
head -n 80 apps/web/src/components/icon-library/export-button.tsxRepository: anand-testcompare/sketchi
Length of output: 2046
🏁 Script executed:
head -n 1 apps/web/src/components/icon-library/export-button.tsxRepository: anand-testcompare/sketchi
Length of output: 102
🏁 Script executed:
cat apps/web/package.json | grep -A 5 '"next"'Repository: anand-testcompare/sketchi
Length of output: 220
🏁 Script executed:
rg -n "from.*svg-to-excalidraw" --type ts -B 2Repository: anand-testcompare/sketchi
Length of output: 1630
🏁 Script executed:
head -n 20 apps/web/src/app/library-generator/[id]/page.tsxRepository: anand-testcompare/sketchi
Length of output: 897
🏁 Script executed:
head -n 20 scripts/test-svg-conversion.tsRepository: anand-testcompare/sketchi
Length of output: 522
Add explicit DOMParser check for SSR safety.
All current call sites are client-side only (export-button.tsx uses React hooks, [id]/page.tsx has "use client"), but the function itself lacks a defensive guard. It checks typeof document !== "undefined" but instantiates DOMParser without verification. Adding an explicit guard ensures the API fails fast with a clear message if called in an SSR context.
export const svgToExcalidrawElements = (
svgText: string,
styleSettings: StyleSettings
) => {
+ if (typeof DOMParser === "undefined") {
+ throw new Error("svgToExcalidrawElements requires a browser environment.");
+ }
const parser = new DOMParser();🤖 Prompt for AI Agents
In `@apps/web/src/lib/icon-library/svg-to-excalidraw.ts` around lines 140 - 241,
The function svgToExcalidrawElements creates a DOMParser unconditionally which
will throw in SSR; add an explicit guard at the top of svgToExcalidrawElements
that checks typeof window !== "undefined" (or typeof document !== "undefined")
before instantiating DOMParser and, if not present, throw a clear error like
"svgToExcalidrawElements must be called in a browser environment"; this ensures
the DOMParser creation and subsequent parser.parseFromString calls are only
attempted on the client and fail fast with a clear message when run server-side.
| const DEFAULT_STYLE_SETTINGS = { | ||
| strokeColor: "#1f2937", | ||
| backgroundColor: "transparent", | ||
| strokeWidth: 1, | ||
| strokeStyle: "solid", | ||
| fillStyle: "solid", | ||
| roughness: 0, | ||
| opacity: 100, | ||
| } as const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check backend DEFAULT_STYLE_SETTINGS
echo "=== Backend DEFAULT_STYLE_SETTINGS ==="
cat -n packages/backend/convex/iconLibraries.ts | head -20
echo -e "\n=== Backend schema definition (full iconLibraries.ts) ==="
wc -l packages/backend/convex/iconLibraries.tsRepository: anand-testcompare/sketchi
Length of output: 829
🏁 Script executed:
#!/bin/bash
# Search for StyleSettings in frontend
echo "=== Frontend StyleSettings interface ==="
fd svg-to-excalidraw packages/apps/web/src --type f
rg "interface StyleSettings|type StyleSettings" packages/apps/web/srcRepository: anand-testcompare/sketchi
Length of output: 297
🏁 Script executed:
#!/bin/bash
# More comprehensive search for StyleSettings definitions
echo "=== All StyleSettings definitions in codebase ==="
rg "StyleSettings\s*[:=]\s*{|interface\s+StyleSettings|type\s+StyleSettings" --type ts --type tsxRepository: anand-testcompare/sketchi
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Find the correct structure
echo "=== Repository structure ==="
git ls-files | head -30
echo -e "\n=== Looking for web app ==="
fd "svg-to-excalidraw" --type fRepository: anand-testcompare/sketchi
Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Search for StyleSettings definitions with corrected regex
echo "=== Frontend StyleSettings ==="
rg "StyleSettings" --type ts --type tsx -A 8Repository: anand-testcompare/sketchi
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Read the svg-to-excalidraw.ts file to find StyleSettings
echo "=== Frontend StyleSettings in svg-to-excalidraw.ts ==="
cat -n apps/web/src/lib/icon-library/svg-to-excalidraw.ts | grep -A 20 "StyleSettings"Repository: anand-testcompare/sketchi
Length of output: 1425
🏁 Script executed:
#!/bin/bash
# Search for StyleSettings in all files
echo "=== All StyleSettings references ==="
rg "StyleSettings" -A 8 -B 2Repository: anand-testcompare/sketchi
Length of output: 14633
Schema mismatch: Backend and frontend StyleSettings are incompatible.
The backend DEFAULT_STYLE_SETTINGS stores strokeColor, backgroundColor, strokeWidth, strokeStyle, fillStyle, roughness, and opacity. The frontend StyleSettings interface (in svg-to-excalidraw.ts) expects fillStyle, roughness, bowing, randomize, and pencilFilter.
Only fillStyle and roughness overlap. When the frontend loads library settings from the database, it extracts only these two properties and hardcodes the rest (bowing: 1, randomize: true, pencilFilter: false), meaning user preferences for bowing, randomize, and pencilFilter are never persisted. Conversely, the backend stores stroke properties that the frontend never uses.
Align the backend schema with the frontend's actual needs, or provide a migration/transformation layer to handle the schema difference.
🤖 Prompt for AI Agents
In `@packages/backend/convex/iconLibraries.ts` around lines 6 - 14,
DEFAULT_STYLE_SETTINGS in iconLibraries.ts currently defines strokeColor,
backgroundColor, strokeWidth, strokeStyle, fillStyle, roughness, and opacity
which does not match the frontend StyleSettings in svg-to-excalidraw.ts (which
expects fillStyle, roughness, bowing, randomize, pencilFilter). Fix by updating
the backend schema and DEFAULT_STYLE_SETTINGS to include the frontend fields
(add bowing, randomize, pencilFilter with sensible defaults and keep
fillStyle/roughness) or implement a clear transformation layer: create a
converter function (e.g., mapDbStyleToFrontendStyle and
mapFrontendStyleToDbStyle) used wherever library settings are read/written to
convert between DEFAULT_STYLE_SETTINGS and the frontend StyleSettings, and add a
migration to persist missing fields if needed; reference DEFAULT_STYLE_SETTINGS
and the frontend StyleSettings type in svg-to-excalidraw.ts when making the
change.
| export const deleteIcon = mutation({ | ||
| args: { iconId: v.id("iconItems") }, | ||
| handler: async (ctx, { iconId }) => { | ||
| const icon = await ctx.db.get(iconId); | ||
| if (!icon) { | ||
| throw new Error("Icon not found."); | ||
| } | ||
|
|
||
| await ctx.db.delete(iconId); | ||
| await ctx.storage.delete(icon.storageId); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Storage deletion order may leave orphaned storage objects.
If ctx.storage.delete(icon.storageId) fails after the database record is deleted, the storage object becomes orphaned. Consider deleting storage first, or wrapping in a try-catch with cleanup logic.
♻️ Suggested reorder
handler: async (ctx, { iconId }) => {
const icon = await ctx.db.get(iconId);
if (!icon) {
throw new Error("Icon not found.");
}
- await ctx.db.delete(iconId);
await ctx.storage.delete(icon.storageId);
+ await ctx.db.delete(iconId);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const deleteIcon = mutation({ | |
| args: { iconId: v.id("iconItems") }, | |
| handler: async (ctx, { iconId }) => { | |
| const icon = await ctx.db.get(iconId); | |
| if (!icon) { | |
| throw new Error("Icon not found."); | |
| } | |
| await ctx.db.delete(iconId); | |
| await ctx.storage.delete(icon.storageId); | |
| }, | |
| }); | |
| export const deleteIcon = mutation({ | |
| args: { iconId: v.id("iconItems") }, | |
| handler: async (ctx, { iconId }) => { | |
| const icon = await ctx.db.get(iconId); | |
| if (!icon) { | |
| throw new Error("Icon not found."); | |
| } | |
| await ctx.storage.delete(icon.storageId); | |
| await ctx.db.delete(iconId); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@packages/backend/convex/iconLibraries.ts` around lines 232 - 243, The
deleteIcon mutation currently deletes the DB record then calls
ctx.storage.delete which can orphan storage if the storage delete fails; update
deleteIcon (use the existing handler, ctx.db.get, ctx.db.delete,
ctx.storage.delete identifiers) to delete the storage first or wrap the sequence
in try/catch so that you only call ctx.db.delete after ctx.storage.delete
succeeds, and if you must delete the DB first add rollback logic to re-create or
restore the DB record on storage failure and log the error; ensure errors from
ctx.storage.delete are propagated or handled so orphaned storage cannot occur.
| const file = await ctx.storage.get(storageId); | ||
| if (!file) { | ||
| throw new Error("Uploaded SVG not found."); | ||
| } | ||
|
|
||
| if (file.size > MAX_SVG_BYTES) { | ||
| await ctx.storage.delete(storageId); | ||
| throw new Error("SVG exceeds 256KB limit."); | ||
| } | ||
|
|
||
| const svgText = await file.text(); | ||
| try { | ||
| validateSvgText(svgText); | ||
| } catch (error) { | ||
| await ctx.storage.delete(storageId); | ||
| throw error; | ||
| } | ||
|
|
||
| const contentHash = (await hashText(svgText)).slice(0, 12); | ||
| const fileStem = sanitizeFileStem(originalName); | ||
| const fileName = `${libraryId}-${fileStem}-${contentHash}.svg`; | ||
|
|
||
| return ctx.runMutation(internal.iconLibraries.addIconRecord, { | ||
| libraryId, | ||
| storageId, | ||
| originalName, | ||
| contentHash, | ||
| fileName, | ||
| byteSize: file.size, | ||
| }); |
There was a problem hiding this comment.
Clean up uploaded storage if addIconRecord fails.
If ctx.runMutation throws (e.g., invalid libraryId), the uploaded blob is left orphaned. Wrap the mutation in a try/catch and delete on failure to avoid storage leaks.
🧹 Proposed fix
- return ctx.runMutation(internal.iconLibraries.addIconRecord, {
- libraryId,
- storageId,
- originalName,
- contentHash,
- fileName,
- byteSize: file.size,
- });
+ try {
+ return await ctx.runMutation(internal.iconLibraries.addIconRecord, {
+ libraryId,
+ storageId,
+ originalName,
+ contentHash,
+ fileName,
+ byteSize: file.size,
+ });
+ } catch (error) {
+ await ctx.storage.delete(storageId);
+ throw error;
+ }🤖 Prompt for AI Agents
In `@packages/backend/convex/iconLibrariesActions.ts` around lines 52 - 81, The
current flow creates and validates the SVG then calls
ctx.runMutation(internal.iconLibraries.addIconRecord) but if that mutation
throws the uploaded blob (storageId) is left orphaned; wrap the call to
ctx.runMutation(internal.iconLibraries.addIconRecord, {...}) in a try/catch, and
in the catch await ctx.storage.delete(storageId) before rethrowing the error so
any failure (e.g., invalid libraryId) cleans up the uploaded file; keep all
preceding logic (ctx.storage.get, validateSvgText, hashText, sanitizeFileStem)
intact and ensure the deletion happens only on mutation failure.
| const styleSettings = v.object({ | ||
| strokeColor: v.string(), | ||
| backgroundColor: v.string(), | ||
| strokeWidth: v.number(), | ||
| strokeStyle: v.union( | ||
| v.literal("solid"), | ||
| v.literal("dashed"), | ||
| v.literal("dotted") | ||
| ), | ||
| fillStyle: v.union( | ||
| v.literal("solid"), | ||
| v.literal("hachure"), | ||
| v.literal("cross-hatch"), | ||
| v.literal("zigzag") | ||
| ), | ||
| roughness: v.number(), | ||
| opacity: v.number(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/backend && find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) | head -20Repository: anand-testcompare/sketchi
Length of output: 685
🏁 Script executed:
rg -n "styleSettings" --type ts --type tsx -B 2 -A 5Repository: anand-testcompare/sketchi
Length of output: 96
🏁 Script executed:
rg -n "bowing|randomize|pencilFilter" --type ts --type tsx --type js --type jsx -B 2 -A 2Repository: anand-testcompare/sketchi
Length of output: 96
🏁 Script executed:
# Search for where styleSettings is created or populated in UI code
rg -n "strokeColor|backgroundColor|strokeWidth|fillStyle|roughness|opacity" --type ts --type tsx -l | head -20Repository: anand-testcompare/sketchi
Length of output: 96
🏁 Script executed:
rg -n "styleSettings" --type-add ts:*.ts --type-add tsx:*.tsx -B 2 -A 5 | head -100Repository: anand-testcompare/sketchi
Length of output: 7355
🏁 Script executed:
rg "bowing|randomize|pencilFilter" | head -50Repository: anand-testcompare/sketchi
Length of output: 2450
🏁 Script executed:
# Find the schema.ts file and examine its context
find . -name "schema.ts" -type fRepository: anand-testcompare/sketchi
Length of output: 104
🏁 Script executed:
# Search for where style objects are created in the codebase
rg "strokeColor.*backgroundColor|backgroundColor.*strokeColor" -B 3 -A 3 | head -80Repository: anand-testcompare/sketchi
Length of output: 4723
🏁 Script executed:
cat -n packages/backend/convex/schema.ts | head -40Repository: anand-testcompare/sketchi
Length of output: 1362
🏁 Script executed:
# Check where styleSettings is used in database operations
rg -n "styleSettings" packages/backend/convex/ -A 3 -B 1Repository: anand-testcompare/sketchi
Length of output: 2279
🏁 Script executed:
rg -n "DEFAULT_STYLE_SETTINGS" packages/backend/convex/ -B 5 -A 10Repository: anand-testcompare/sketchi
Length of output: 2214
🏁 Script executed:
# Check the complete styleSettings structure in iconLibraries.ts update mutation
sed -n '148,168p' packages/backend/convex/iconLibraries.tsRepository: anand-testcompare/sketchi
Length of output: 635
Add bowing, randomize, and pencilFilter to styleSettings schema.
The UI sends these three fields in style controls, but the schema only accepts strokeColor, backgroundColor, strokeWidth, strokeStyle, fillStyle, roughness, and opacity. Without these fields in the schema validation, updates will fail or drop data. Update both the styleSettings object in schema.ts and the corresponding validator in iconLibraries.ts update mutation.
🤖 Prompt for AI Agents
In `@packages/backend/convex/schema.ts` around lines 4 - 21, The styleSettings
schema is missing three fields the UI sends; add bowing: v.number(), randomize:
v.boolean(), and pencilFilter: v.string() to the styleSettings object in
schema.ts (alongside strokeColor, backgroundColor, strokeWidth, strokeStyle,
fillStyle, roughness, opacity), and mirror these same fields and types in the
validator used by the update mutation in iconLibraries.ts so incoming updates
are accepted and not dropped.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/web/src/app/library-generator/`[id]/page.tsx:
- Around line 263-269: The IconGrid is marked busy only by isUploading, so save
operations can still allow deletes/moves; update the isBusy prop passed to
IconGrid to include the save state (e.g., change isBusy={isUploading} to
isBusy={isUploading || isSaving} or whatever save-state variable you have),
ensure the save state variable (isSaving, isSavingLibrary, etc.) is set around
the save routine (the function that performs the library save), and ensure
handlers like handleDeleteSelected and handleMove respect/early-return when
isBusy is true so UI actions are blocked during saves.
- Around line 136-141: The code calls response.json() and then addIcon inside
handleUpload (page.tsx) which can throw a JSON parsing error that isn't
file-specific; wrap the response.json() call in its own try/catch (or validate
response text before parsing) and on failure throw or log a new Error that
includes the file.name and response.status/text so the outer catch can surface a
file-specific message; ensure you still reference payload.storageId (used by
addIcon) only after successful parse and rethrow or propagate a descriptive
error from the JSON parse block so handlers know which file failed.
- Around line 219-222: The Input component rendering the library name (value
bound to libraryName and onChange calling setLibraryName) lacks an accessible
label; update the JSX where Input is used to add either a visible <label> tied
to the input (for example, label text "Library name" with htmlFor matching the
Input's id) or provide an aria-label/aria-labelledby attribute on the Input
(ensure you set a unique id prop and reference it if using a label), so screen
readers can identify the field.
In `@apps/web/src/components/header.tsx`:
- Around line 10-16: The Home Link currently only contains an sr-only span so
sighted users see an empty box; update the Link in header.tsx (the Link element
with aria-label="Home" and className="flex h-7 w-7 ...") to include a visible
Home icon (either import and render the existing HomeIcon component or inline an
SVG) placed alongside the sr-only span, mark the icon aria-hidden="true", style
it to fit the container (match h-4 w-4 or similar) and keep the sr-only text for
screen readers so the visual and accessible labels are preserved.
🧹 Nitpick comments (3)
apps/web/src/components/mode-toggle.tsx (1)
9-13: Handle potentialundefinedstate ofresolvedThemeduring hydration.
resolvedThemecan beundefinedbefore the theme is resolved on the client (e.g., during SSR or initial hydration). If the user clicks the toggle before hydration completes, the comparisonresolvedTheme === "dark"will befalse, always setting the theme to"light"regardless of the user's actual preference.Consider guarding against this edge case:
Suggested fix
const toggleTheme = () => { + if (!resolvedTheme) return; setTheme(resolvedTheme === "dark" ? "light" : "dark"); };apps/web/src/components/icon-library/icon-grid.tsx (1)
157-170: Consider using lucide-reactCheckicon for consistency.The codebase already imports icons from
lucide-react(e.g.,Minus,Plus). Using an inline SVG for the checkmark creates inconsistency.♻️ Suggested refactor
+import { Check, Minus, Plus } from "lucide-react"; -import { Minus, Plus } from "lucide-react";Then replace the inline SVG:
{isSelected && ( - <svg - aria-hidden="true" - className="h-3 w-3 text-primary" - fill="none" - stroke="currentColor" - strokeWidth={3} - viewBox="0 0 24 24" - > - <path - d="M5 13l4 4L19 7" - strokeLinecap="round" - strokeLinejoin="round" - /> - </svg> + <Check aria-hidden="true" className="h-3 w-3 text-primary" /> )}apps/web/src/app/library-generator/[id]/page.tsx (1)
162-175: Sequential deletion stops on first error, leaving partial state.If one
deleteIconcall fails, subsequent icons won't be deleted, but the UI will have already removed them from selection. Consider usingPromise.allSettledto attempt all deletions and report combined results.♻️ Proposed improvement
const handleDeleteSelected = async (ids: string[]) => { try { - for (const id of ids) { - await deleteIcon({ iconId: id as Id<"iconItems"> }); - } - toast.success( - ids.length === 1 ? "Icon deleted." : `${ids.length} icons deleted.` - ); + const results = await Promise.allSettled( + ids.map((id) => deleteIcon({ iconId: id as Id<"iconItems"> })) + ); + const failures = results.filter((r) => r.status === "rejected"); + if (failures.length === 0) { + toast.success( + ids.length === 1 ? "Icon deleted." : `${ids.length} icons deleted.` + ); + } else if (failures.length < ids.length) { + toast.warning(`Deleted ${ids.length - failures.length} of ${ids.length} icons.`); + } else { + toast.error("Failed to delete icons."); + } } catch (error) { const message = error instanceof Error ? error.message : "Failed to delete icons."; toast.error(message); } };
| const payload = (await response.json()) as { storageId: string }; | ||
| await addIcon({ | ||
| libraryId, | ||
| storageId: payload.storageId as Id<"_storage">, | ||
| originalName: file.name, | ||
| }); |
There was a problem hiding this comment.
JSON parsing can throw on malformed responses.
If the upload succeeds (response.ok) but returns malformed JSON, response.json() will throw an unhandled error. The outer try-catch in handleUpload will catch it, but the error message won't be file-specific.
🐛 Suggested improvement
+ let payload: { storageId: string };
+ try {
+ payload = (await response.json()) as { storageId: string };
+ } catch {
+ toast.error(`Invalid response for ${file.name}.`);
+ return;
+ }
- const payload = (await response.json()) as { storageId: string };
await addIcon({
libraryId,
- storageId: payload.storageId as Id<"_storage">,
+ storageId: payload.storageId as Id<"_storage">,
originalName: file.name,
});📝 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 payload = (await response.json()) as { storageId: string }; | |
| await addIcon({ | |
| libraryId, | |
| storageId: payload.storageId as Id<"_storage">, | |
| originalName: file.name, | |
| }); | |
| let payload: { storageId: string }; | |
| try { | |
| payload = (await response.json()) as { storageId: string }; | |
| } catch { | |
| toast.error(`Invalid response for ${file.name}.`); | |
| return; | |
| } | |
| await addIcon({ | |
| libraryId, | |
| storageId: payload.storageId as Id<"_storage">, | |
| originalName: file.name, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/web/src/app/library-generator/`[id]/page.tsx around lines 136 - 141, The
code calls response.json() and then addIcon inside handleUpload (page.tsx) which
can throw a JSON parsing error that isn't file-specific; wrap the
response.json() call in its own try/catch (or validate response text before
parsing) and on failure throw or log a new Error that includes the file.name and
response.status/text so the outer catch can surface a file-specific message;
ensure you still reference payload.storageId (used by addIcon) only after
successful parse and rethrow or propagate a descriptive error from the JSON
parse block so handlers know which file failed.
| <Input | ||
| onChange={(event) => setLibraryName(event.target.value)} | ||
| value={libraryName} | ||
| /> |
There was a problem hiding this comment.
Input lacks accessible label.
The library name input has no associated <label> or aria-label, making it inaccessible to screen reader users. As per coding guidelines, form inputs should have proper labels.
♿ Suggested fix
+ <label className="sr-only" htmlFor="library-name">
+ Library name
+ </label>
<Input
+ id="library-name"
onChange={(event) => setLibraryName(event.target.value)}
+ placeholder="Library name"
value={libraryName}
/>Or use aria-label:
<Input
+ aria-label="Library name"
onChange={(event) => setLibraryName(event.target.value)}
+ placeholder="Library name"
value={libraryName}
/>📝 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.
| <Input | |
| onChange={(event) => setLibraryName(event.target.value)} | |
| value={libraryName} | |
| /> | |
| <label className="sr-only" htmlFor="library-name"> | |
| Library name | |
| </label> | |
| <Input | |
| id="library-name" | |
| onChange={(event) => setLibraryName(event.target.value)} | |
| placeholder="Library name" | |
| value={libraryName} | |
| /> |
🤖 Prompt for AI Agents
In `@apps/web/src/app/library-generator/`[id]/page.tsx around lines 219 - 222, The
Input component rendering the library name (value bound to libraryName and
onChange calling setLibraryName) lacks an accessible label; update the JSX where
Input is used to add either a visible <label> tied to the input (for example,
label text "Library name" with htmlFor matching the Input's id) or provide an
aria-label/aria-labelledby attribute on the Input (ensure you set a unique id
prop and reference it if using a label), so screen readers can identify the
field.
| <IconGrid | ||
| icons={icons} | ||
| isBusy={isUploading} | ||
| onDeleteSelected={handleDeleteSelected} | ||
| onMove={handleMove} | ||
| styleSettings={styleSettings} | ||
| /> |
There was a problem hiding this comment.
isBusy doesn't account for save operations.
The isBusy prop only reflects isUploading, but users can still delete or move icons while the library is saving. This could lead to inconsistent state if a save is in progress.
🐛 Suggested fix
<IconGrid
icons={icons}
- isBusy={isUploading}
+ isBusy={isUploading || isSaving}
onDeleteSelected={handleDeleteSelected}
onMove={handleMove}
styleSettings={styleSettings}
/>📝 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.
| <IconGrid | |
| icons={icons} | |
| isBusy={isUploading} | |
| onDeleteSelected={handleDeleteSelected} | |
| onMove={handleMove} | |
| styleSettings={styleSettings} | |
| /> | |
| <IconGrid | |
| icons={icons} | |
| isBusy={isUploading || isSaving} | |
| onDeleteSelected={handleDeleteSelected} | |
| onMove={handleMove} | |
| styleSettings={styleSettings} | |
| /> |
🤖 Prompt for AI Agents
In `@apps/web/src/app/library-generator/`[id]/page.tsx around lines 263 - 269, The
IconGrid is marked busy only by isUploading, so save operations can still allow
deletes/moves; update the isBusy prop passed to IconGrid to include the save
state (e.g., change isBusy={isUploading} to isBusy={isUploading || isSaving} or
whatever save-state variable you have), ensure the save state variable
(isSaving, isSavingLibrary, etc.) is set around the save routine (the function
that performs the library save), and ensure handlers like handleDeleteSelected
and handleMove respect/early-return when isBusy is true so UI actions are
blocked during saves.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/app/page.tsx`:
- Around line 27-32: The Link anchor is wrapping a Button causing nested
interactive elements (Link + Button) which breaks accessibility; update the
usage in page.tsx to render the Button as the Link target by using the Button's
render prop (or Base UI composition) instead of wrapping—i.e., call Button with
render={(props) => <Link {...props}><Wand2 />Open generator</Link>} or
equivalent so the Button becomes the anchor; ensure your custom Link component
forwards refs and spreads received props onto its underlying DOM element to
satisfy Base UI's render prop requirements.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/components/icon-library/sketchy-icon-preview.tsx`:
- Around line 55-82: The fetch logic leaves previous SVG content visible on new
requests or on errors; update the effect to clear stale previews by calling
setSvgText(null or empty string) when starting a new fetch (before
fetch(svgUrl)) and also in the catch block before setError, and ensure the
cleanup/cancelled logic still prevents setting state after unmount; reference
setSvgText, setIsLoading, setError, cancelled, fetch(svgUrl), and svgUrl when
making these changes.
🧹 Nitpick comments (3)
apps/web/src/app/page.tsx (2)
21-21: Add responsive breakpoints for the grid layout.
grid-cols-3without responsive modifiers will render three cramped columns on mobile devices. Consider using responsive breakpoints for better mobile experience.♻️ Suggested fix
- <div className="grid grid-cols-3 gap-4"> + <div className="grid grid-cols-1 gap-4 md:grid-cols-3">
29-29: Addaria-hiddento decorative icon.The Wand2 icon is decorative since there's accompanying "Open" text. Hide it from screen readers to avoid redundant announcements.
♻️ Suggested fix
- <Wand2 /> + <Wand2 aria-hidden="true" />apps/web/src/components/icon-library/sketchy-icon-preview.tsx (1)
48-82: Prefer async/await (and abort) for the SVG fetch effect.The current promise chain is harder to read and makes cancellation handling awkward. Converting to
async/awaitwith anAbortControllerimproves readability and avoids late state updates after unmount.As per coding guidelines, prefer async/await over promise chains.♻️ Suggested refactor
useEffect(() => { if (!svgUrl) { setSvgText(null); setError(null); return; } - let cancelled = false; - setIsLoading(true); - setError(null); - - fetch(svgUrl) - .then((res) => { - if (!res.ok) { - throw new Error(`Failed to fetch SVG: ${res.status}`); - } - return res.text(); - }) - .then((text) => { - if (!cancelled) { - setSvgText(text); - setIsLoading(false); - } - }) - .catch((err) => { - if (!cancelled) { - setError(err instanceof Error ? err.message : "Failed to fetch SVG"); - setIsLoading(false); - } - }); - - return () => { - cancelled = true; - }; + const controller = new AbortController(); + let cancelled = false; + + const loadSvg = async () => { + try { + setIsLoading(true); + setError(null); + const res = await fetch(svgUrl, { signal: controller.signal }); + if (!res.ok) { + throw new Error(`Failed to fetch SVG: ${res.status}`); + } + const text = await res.text(); + if (!cancelled) { + setSvgText(text); + } + } catch (err) { + if (!cancelled && (err as DOMException)?.name !== "AbortError") { + setError(err instanceof Error ? err.message : "Failed to fetch SVG"); + } + } finally { + if (!cancelled) { + setIsLoading(false); + } + } + }; + + void loadSvg(); + + return () => { + cancelled = true; + controller.abort(); + }; }, [svgUrl]);
| let cancelled = false; | ||
| setIsLoading(true); | ||
| setError(null); | ||
|
|
||
| fetch(svgUrl) | ||
| .then((res) => { | ||
| if (!res.ok) { | ||
| throw new Error(`Failed to fetch SVG: ${res.status}`); | ||
| } | ||
| return res.text(); | ||
| }) | ||
| .then((text) => { | ||
| if (!cancelled) { | ||
| setSvgText(text); | ||
| setIsLoading(false); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| if (!cancelled) { | ||
| setError(err instanceof Error ? err.message : "Failed to fetch SVG"); | ||
| setIsLoading(false); | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [svgUrl]); |
There was a problem hiding this comment.
Clear stale previews on fetch/parse failure.
When a fetch or parse fails, svgText remains the previous value and the container contents are left intact, so users can see a stale icon behind the error state. Clear the container and reset svgText on new requests and on errors to avoid misleading previews.
🔧 Proposed fix
useEffect(() => {
if (!svgUrl) {
setSvgText(null);
setError(null);
return;
}
let cancelled = false;
setIsLoading(true);
setError(null);
+ setSvgText(null);
+ if (containerRef.current) {
+ containerRef.current.replaceChildren();
+ }
fetch(svgUrl)
.then((res) => {
if (!res.ok) {
throw new Error(`Failed to fetch SVG: ${res.status}`);
}
return res.text();
})
.then((text) => {
if (!cancelled) {
setSvgText(text);
setIsLoading(false);
}
})
.catch((err) => {
if (!cancelled) {
setError(err instanceof Error ? err.message : "Failed to fetch SVG");
+ setSvgText(null);
setIsLoading(false);
}
});📝 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.
| let cancelled = false; | |
| setIsLoading(true); | |
| setError(null); | |
| fetch(svgUrl) | |
| .then((res) => { | |
| if (!res.ok) { | |
| throw new Error(`Failed to fetch SVG: ${res.status}`); | |
| } | |
| return res.text(); | |
| }) | |
| .then((text) => { | |
| if (!cancelled) { | |
| setSvgText(text); | |
| setIsLoading(false); | |
| } | |
| }) | |
| .catch((err) => { | |
| if (!cancelled) { | |
| setError(err instanceof Error ? err.message : "Failed to fetch SVG"); | |
| setIsLoading(false); | |
| } | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [svgUrl]); | |
| let cancelled = false; | |
| setIsLoading(true); | |
| setError(null); | |
| setSvgText(null); | |
| if (containerRef.current) { | |
| containerRef.current.replaceChildren(); | |
| } | |
| fetch(svgUrl) | |
| .then((res) => { | |
| if (!res.ok) { | |
| throw new Error(`Failed to fetch SVG: ${res.status}`); | |
| } | |
| return res.text(); | |
| }) | |
| .then((text) => { | |
| if (!cancelled) { | |
| setSvgText(text); | |
| setIsLoading(false); | |
| } | |
| }) | |
| .catch((err) => { | |
| if (!cancelled) { | |
| setError(err instanceof Error ? err.message : "Failed to fetch SVG"); | |
| setSvgText(null); | |
| setIsLoading(false); | |
| } | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [svgUrl]); |
🤖 Prompt for AI Agents
In `@apps/web/src/components/icon-library/sketchy-icon-preview.tsx` around lines
55 - 82, The fetch logic leaves previous SVG content visible on new requests or
on errors; update the effect to clear stale previews by calling setSvgText(null
or empty string) when starting a new fetch (before fetch(svgUrl)) and also in
the catch block before setError, and ensure the cleanup/cancelled logic still
prevents setting state after unmount; reference setSvgText, setIsLoading,
setError, cancelled, fetch(svgUrl), and svgUrl when making these changes.
- Add ZIP download support for bulk SVG export - Fix Excalidraw export schema (add missing properties, remove deprecated strokeSharpness) - Add text labels to Excalidraw exports (grouped with icon, hand-drawn font) - Show labels in preview with Caveat font - Default fillStyle to hachure, showLabel to true - Improve icon grid layout for few icons (auto-fit) - Add hover animation to home page card - Style Open button with hand-drawn font
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/web/src/components/icon-library/export-button.tsx`:
- Around line 127-141: The ZIP export currently uses sanitizeFileName and calls
zip.file(`${safeName}.svg`, ...) which lets JSZip overwrite entries when
different icons produce the same sanitized name; fix by deduplicating filenames
before calling zip.file: maintain a Set or Map of used names inside the export
loop, generate a candidate like `${safeName}${suffix}` (e.g., append "-1", "-2",
incrementing until unused), use that uniqueName for
zip.file(`${uniqueName}.svg`, svgText), and ensure the dedupe logic is applied
for every icon processed (references: sanitizeFileName, icons array, zip.file,
JSZip).
In `@apps/web/src/lib/icon-library/svg-to-excalidraw.ts`:
- Around line 174-201: The export flow calls svgToExcalidrawElements with
untrusted SVG and then that function appends the parsed SVG into the DOM,
enabling inline scripts/event handlers; fix by validating the raw SVG before
conversion: in export-button.tsx import the existing validateSvgText (from
svg-validate), call validateSvgText(svgText) right after fetching the SVG and
before invoking svgToExcalidrawElements, and handle validation failures (throw
or surface a user-friendly error) so only validated SVGs reach
svgToExcalidrawElements.
In `@packages/backend/convex/iconLibraries.ts`:
- Around line 245-266: The reorderIcons mutation currently validates and patches
each icon inside Promise.all which can leave partial updates if one validation
fails; change reorderIcons to first fetch and validate all orderedIds using
ctx.db.get (ensure each exists and icon.libraryId === libraryId) before
performing any ctx.db.patch calls, then apply the patches (either in a single
transaction if supported or sequentially) updating sortOrder and updatedAt;
reference the reorderIcons handler, orderedIds, ctx.db.get, and ctx.db.patch
when making the change.
🧹 Nitpick comments (4)
apps/web/src/components/icon-library/icon-grid.tsx (1)
31-37: Extract icon sizing constants instead of magic numbers.
Keeps zoom behavior consistent and easier to tune. As per coding guidelines, please extract descriptive constants.♻️ Suggested refactor
+const DEFAULT_ICON_SIZE = 80; +const ICON_SIZE_STEP = 20; +const MIN_ICON_SIZE = 40; +const MAX_ICON_SIZE = 200; + export default function IconGrid({ icons, onDeleteSelected, onMove, isBusy, styleSettings, }: IconGridProps) { const [isEditMode, setIsEditMode] = useState(false); const [selectedIds, setSelectedIds] = useState<Set<string>>(new Set()); - const [iconSize, setIconSize] = useState(80); + const [iconSize, setIconSize] = useState(DEFAULT_ICON_SIZE); - const handleZoomIn = () => setIconSize((prev) => Math.min(prev + 20, 200)); - const handleZoomOut = () => setIconSize((prev) => Math.max(prev - 20, 40)); + const handleZoomIn = () => + setIconSize((prev) => Math.min(prev + ICON_SIZE_STEP, MAX_ICON_SIZE)); + const handleZoomOut = () => + setIconSize((prev) => Math.max(prev - ICON_SIZE_STEP, MIN_ICON_SIZE));apps/web/src/app/library-generator/[id]/page.tsx (1)
166-179: Sequential deletion may leave partial state on failure.The loop deletes icons one-by-one. If one deletion fails mid-way, remaining icons won't be processed and no rollback occurs. Consider collecting errors and reporting them at the end, or using
Promise.allSettledto handle partial failures gracefully.♻️ Suggested approach with error aggregation
const handleDeleteSelected = async (ids: string[]) => { + const errors: string[] = []; try { - for (const id of ids) { - await deleteIcon({ iconId: id as Id<"iconItems"> }); - } + const results = await Promise.allSettled( + ids.map((id) => deleteIcon({ iconId: id as Id<"iconItems"> })) + ); + for (const result of results) { + if (result.status === "rejected") { + errors.push(result.reason instanceof Error ? result.reason.message : "Unknown error"); + } + } + if (errors.length > 0 && errors.length < ids.length) { + toast.warning(`Deleted ${ids.length - errors.length} icons. ${errors.length} failed.`); + return; + } + if (errors.length === ids.length) { + toast.error("Failed to delete icons."); + return; + } toast.success( ids.length === 1 ? "Icon deleted." : `${ids.length} icons deleted.` ); } catch (error) { const message = error instanceof Error ? error.message : "Failed to delete icons."; toast.error(message); } };packages/backend/convex/iconLibraries.ts (2)
24-39: Unbounded loop ingetUniqueSluglacks a safety limit.While unlikely in practice, this
while (true)loop has no upper bound. If there's a database issue or unexpected state, this could loop indefinitely.♻️ Add a safety limit
const getUniqueSlug = async (ctx: { db: DatabaseReader }, base: string) => { let slug = base; let attempt = 1; + const MAX_ATTEMPTS = 1000; - while (true) { + while (attempt <= MAX_ATTEMPTS) { const existing = await ctx.db .query("iconLibraries") .withIndex("by_slug", (q) => q.eq("slug", slug)) .unique(); if (!existing) { return slug; } attempt += 1; slug = `${base}-${attempt}`; } + throw new Error(`Could not generate unique slug after ${MAX_ATTEMPTS} attempts.`); };
85-88: Inconsistent index usage ingetBySlug.This query uses
by_libraryindex without ordering, whilelistandgetuseby_library_orderwith explicit ordering. If icon order matters for display consistency, consider using the same approach.♻️ Use consistent index and ordering
const icons = await ctx.db .query("iconItems") - .withIndex("by_library", (q) => q.eq("libraryId", library._id)) + .withIndex("by_library_order", (q) => q.eq("libraryId", library._id)) + .order("asc") .collect();
| try { | ||
| const zip = new JSZip(); | ||
|
|
||
| for (const icon of icons) { | ||
| if (!icon.url) { | ||
| throw new Error(`Missing icon URL for ${icon.name}.`); | ||
| } | ||
| const response = await fetch(icon.url); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to load ${icon.name}.`); | ||
| } | ||
| const svgText = await response.text(); | ||
| const safeName = sanitizeFileName(icon.name); | ||
| zip.file(`${safeName}.svg`, svgText); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the sanitizeFileName function
rg "sanitizeFileName" -A 5 -B 2Repository: anand-testcompare/sketchi
Length of output: 2492
🏁 Script executed:
# Check the export-button.tsx file to understand context
head -n 150 apps/web/src/components/icon-library/export-button.tsx | tail -n 50Repository: anand-testcompare/sketchi
Length of output: 1526
🏁 Script executed:
# Look for icon type definitions and data patterns
rg "interface.*[Ii]con|type.*[Ii]con" --max-count 10 -A 3Repository: anand-testcompare/sketchi
Length of output: 7052
🌐 Web query:
JSZip file() method duplicate filename behavior overwrites
💡 Result:
In JSZip, calling zip.file(name, data) with a filename that already exists updates/replaces (overwrites) the existing entry—JSZip treats files as a map keyed by filename, so you can’t keep multiple entries with the exact same path/name in a single JSZip instance. This is explicitly described as “Add (or update) a file”. [1], [2]
If you need to avoid overwriting, you must choose unique names yourself (e.g., add a suffix like file (2).txt), or check first with zip.file(name) (returns the existing entry or null) and rename accordingly. [1], [3]
Sources: [1] JSZip file(name, data) docs (stuk.github.io), [2] JSZip API (files uses name as key) (stuk.github.io), [3] JSZip file(name) getter docs (stuk.github.io)
Citations:
- 1: https://stuk.github.io/jszip/documentation/api_jszip/file_data.html?utm_source=openai
- 2: https://stuk.github.io/jszip/documentation/api_jszip.html?utm_source=openai
- 3: https://stuk.github.io/jszip/documentation/api_jszip/file_name.html?utm_source=openai
ZIP export silently loses icons with colliding sanitized names.
sanitizeFileName can map different input names to the same value (e.g., "icon-1", "icon_1", "icon@1" all become "icon-1"). JSZip silently overwrites entries with duplicate filenames, causing data loss. Deduplicate with a suffix to preserve all icons.
🛠️ Suggested fix (dedupe filenames)
try {
const zip = new JSZip();
+ const nameCounts = new Map<string, number>();
for (const icon of icons) {
if (!icon.url) {
throw new Error(`Missing icon URL for ${icon.name}.`);
}
const response = await fetch(icon.url);
if (!response.ok) {
throw new Error(`Failed to load ${icon.name}.`);
}
const svgText = await response.text();
const safeName = sanitizeFileName(icon.name);
- zip.file(`${safeName}.svg`, svgText);
+ const nextCount = (nameCounts.get(safeName) ?? 0) + 1;
+ nameCounts.set(safeName, nextCount);
+ const uniqueName =
+ nextCount > 1 ? `${safeName}-${nextCount}` : safeName;
+ zip.file(`${uniqueName}.svg`, svgText);
}📝 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.
| try { | |
| const zip = new JSZip(); | |
| for (const icon of icons) { | |
| if (!icon.url) { | |
| throw new Error(`Missing icon URL for ${icon.name}.`); | |
| } | |
| const response = await fetch(icon.url); | |
| if (!response.ok) { | |
| throw new Error(`Failed to load ${icon.name}.`); | |
| } | |
| const svgText = await response.text(); | |
| const safeName = sanitizeFileName(icon.name); | |
| zip.file(`${safeName}.svg`, svgText); | |
| } | |
| try { | |
| const zip = new JSZip(); | |
| const nameCounts = new Map<string, number>(); | |
| for (const icon of icons) { | |
| if (!icon.url) { | |
| throw new Error(`Missing icon URL for ${icon.name}.`); | |
| } | |
| const response = await fetch(icon.url); | |
| if (!response.ok) { | |
| throw new Error(`Failed to load ${icon.name}.`); | |
| } | |
| const svgText = await response.text(); | |
| const safeName = sanitizeFileName(icon.name); | |
| const nextCount = (nameCounts.get(safeName) ?? 0) + 1; | |
| nameCounts.set(safeName, nextCount); | |
| const uniqueName = | |
| nextCount > 1 ? `${safeName}-${nextCount}` : safeName; | |
| zip.file(`${uniqueName}.svg`, svgText); | |
| } |
🤖 Prompt for AI Agents
In `@apps/web/src/components/icon-library/export-button.tsx` around lines 127 -
141, The ZIP export currently uses sanitizeFileName and calls
zip.file(`${safeName}.svg`, ...) which lets JSZip overwrite entries when
different icons produce the same sanitized name; fix by deduplicating filenames
before calling zip.file: maintain a Set or Map of used names inside the export
loop, generate a candidate like `${safeName}${suffix}` (e.g., append "-1", "-2",
incrementing until unused), use that uniqueName for
zip.file(`${uniqueName}.svg`, svgText), and ensure the dedupe logic is applied
for every icon processed (references: sanitizeFileName, icons array, zip.file,
JSZip).
| export const svgToExcalidrawElements = ( | ||
| svgText: string, | ||
| styleSettings: StyleSettings, | ||
| iconName?: string | ||
| ): ExcalidrawElement[] => { | ||
| const parser = new DOMParser(); | ||
| const doc = parser.parseFromString(svgText, "image/svg+xml"); | ||
| const parsedSvg = doc.querySelector("svg") as SVGSVGElement | null; | ||
| const parseError = doc.querySelector("parsererror"); | ||
|
|
||
| if (!parsedSvg || parseError) { | ||
| throw new Error("Unable to parse SVG."); | ||
| } | ||
|
|
||
| let container: HTMLDivElement | null = null; | ||
| let svg = parsedSvg; | ||
|
|
||
| if (typeof document !== "undefined" && document.body) { | ||
| container = document.createElement("div"); | ||
| container.style.cssText = | ||
| "position:absolute;left:-99999px;top:-99999px;width:0;height:0;overflow:hidden;visibility:hidden;"; | ||
| container.appendChild(document.importNode(parsedSvg, true)); | ||
| document.body.appendChild(container); | ||
| const attached = container.querySelector("svg") as SVGSVGElement | null; | ||
| if (attached) { | ||
| svg = attached; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for validateSvgText function and related validation
rg -n "validateSvgText|sanitize.*[Ss]vg|svg.*sanitize" --type ts --type tsxRepository: anand-testcompare/sketchi
Length of output: 96
🏁 Script executed:
# Search for svgToExcalidrawElements call sites to understand usage context
rg -n "svgToExcalidrawElements" --type ts --type tsx -B 2 -A 2Repository: anand-testcompare/sketchi
Length of output: 96
🏁 Script executed:
# Check for any existing sanitization or DOMPurify usage
rg -n "DOMPurify|sanitize|xss" --type ts --type tsxRepository: anand-testcompare/sketchi
Length of output: 96
🏁 Script executed:
# Examine the svg-to-excalidraw.ts file more closely
cat -n apps/web/src/lib/icon-library/svg-to-excalidraw.ts | head -50Repository: anand-testcompare/sketchi
Length of output: 1501
🏁 Script executed:
# Read the specific lines from the file
sed -n '174,201p' apps/web/src/lib/icon-library/svg-to-excalidraw.tsRepository: anand-testcompare/sketchi
Length of output: 1064
🏁 Script executed:
# Search for validateSvgText without type specification
rg -n "validateSvgText|svg-validate"Repository: anand-testcompare/sketchi
Length of output: 599
🏁 Script executed:
# Search for svgToExcalidrawElements call sites
rg -n "svgToExcalidrawElements" -B 3 -A 3Repository: anand-testcompare/sketchi
Length of output: 3379
🏁 Script executed:
# Check imports and validation at top of file
head -30 apps/web/src/lib/icon-library/svg-to-excalidraw.tsRepository: anand-testcompare/sketchi
Length of output: 725
🏁 Script executed:
# Search for sanitization/XSS prevention patterns
rg -n "DOMPurify|sanitize|security|xss" -i | head -30Repository: anand-testcompare/sketchi
Length of output: 4831
🏁 Script executed:
# Check the svg-validate.ts file to understand what validation is being done
cat apps/web/src/lib/icon-library/svg-validate.tsRepository: anand-testcompare/sketchi
Length of output: 902
🏁 Script executed:
# Get more context on the export-button.tsx call to svgToExcalidrawElements
sed -n '65,85p' apps/web/src/components/icon-library/export-button.tsxRepository: anand-testcompare/sketchi
Length of output: 683
Add SVG validation before DOM insertion in export-button to prevent XSS.
In export-button.tsx (line 74), svgToExcalidrawElements is called with untrusted SVG fetched from icon.url without validation. The function then appends the SVG to the DOM (line 189–190 in svg-to-excalidraw.ts), which allows inline scripts and event handlers to execute. A validateSvgText function already exists (svg-validate.ts) and is used in other call paths (library-generator, backend) but is missing here.
Import and call validateSvgText(svgText) before passing to svgToExcalidrawElements in export-button.tsx:
Proposed fix
+import { validateSvgText } from "@/lib/icon-library/svg-validate";
import {
type StyleSettings,
svgToExcalidrawElements,
} from "@/lib/icon-library/svg-to-excalidraw"; const svgText = await response.text();
+ validateSvgText(svgText);
const elements = svgToExcalidrawElements(🤖 Prompt for AI Agents
In `@apps/web/src/lib/icon-library/svg-to-excalidraw.ts` around lines 174 - 201,
The export flow calls svgToExcalidrawElements with untrusted SVG and then that
function appends the parsed SVG into the DOM, enabling inline scripts/event
handlers; fix by validating the raw SVG before conversion: in export-button.tsx
import the existing validateSvgText (from svg-validate), call
validateSvgText(svgText) right after fetching the SVG and before invoking
svgToExcalidrawElements, and handle validation failures (throw or surface a
user-friendly error) so only validated SVGs reach svgToExcalidrawElements.
| export const reorderIcons = mutation({ | ||
| args: { | ||
| libraryId: v.id("iconLibraries"), | ||
| orderedIds: v.array(v.id("iconItems")), | ||
| }, | ||
| handler: async (ctx, { libraryId, orderedIds }) => { | ||
| const now = Date.now(); | ||
|
|
||
| await Promise.all( | ||
| orderedIds.map(async (iconId, index) => { | ||
| const icon = await ctx.db.get(iconId); | ||
| if (!icon || icon.libraryId !== libraryId) { | ||
| throw new Error("Invalid icon reorder request."); | ||
| } | ||
| await ctx.db.patch(iconId, { | ||
| sortOrder: index, | ||
| updatedAt: now, | ||
| }); | ||
| }) | ||
| ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Race condition in reorderIcons may cause partial updates.
Using Promise.all with validation inside each iteration means if one icon fails validation (e.g., wrong library), other patches may have already been applied. This leaves the database in an inconsistent state.
🐛 Validate all icons first, then apply patches
export const reorderIcons = mutation({
args: {
libraryId: v.id("iconLibraries"),
orderedIds: v.array(v.id("iconItems")),
},
handler: async (ctx, { libraryId, orderedIds }) => {
const now = Date.now();
+ // Validate all icons belong to the library before making changes
+ const icons = await Promise.all(
+ orderedIds.map(async (iconId) => {
+ const icon = await ctx.db.get(iconId);
+ if (!icon || icon.libraryId !== libraryId) {
+ throw new Error("Invalid icon reorder request.");
+ }
+ return icon;
+ })
+ );
+
+ // Apply all patches after validation passes
await Promise.all(
- orderedIds.map(async (iconId, index) => {
- const icon = await ctx.db.get(iconId);
- if (!icon || icon.libraryId !== libraryId) {
- throw new Error("Invalid icon reorder request.");
- }
- await ctx.db.patch(iconId, {
+ orderedIds.map(async (iconId, index) =>
+ ctx.db.patch(iconId, {
sortOrder: index,
updatedAt: now,
- });
- })
+ })
+ )
);
},
});🤖 Prompt for AI Agents
In `@packages/backend/convex/iconLibraries.ts` around lines 245 - 266, The
reorderIcons mutation currently validates and patches each icon inside
Promise.all which can leave partial updates if one validation fails; change
reorderIcons to first fetch and validate all orderedIds using ctx.db.get (ensure
each exists and icon.libraryId === libraryId) before performing any ctx.db.patch
calls, then apply the patches (either in a single transaction if supported or
sequentially) updating sortOrder and updatedAt; reference the reorderIcons
handler, orderedIds, ctx.db.get, and ctx.db.patch when making the change.
…w-findings-from-pr-19 fix: CodeRabbit review findings from PR #19
1 participant
Summary by CodeRabbit
New Features
Backend
UX Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.