feat: unified layout loading pipeline with LoadDialog (#1348)#1351
Merged
feat: unified layout loading pipeline with LoadDialog (#1348)#1351
Conversation
- Unified API and File loading logic into load-pipeline.ts - Created LoadDialog component for choosing between server and local layouts - Added Zod runtime schema validation for Persist API layout lists - Implemented archive extraction guardrails (size, entry count, ratio) for security - Updated App and StartScreen to use the new unified loading flow
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new LoadDialog UI and a unified load pipeline; switches App and StartScreen to open the dialog (or fallback to file import); adds archive guardrails, Zod validation for persistence responses, a Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant LoadDialog
participant PersistenceAPI
participant FileSystem as File System
participant Archive
participant LoadPipeline
participant LayoutStore
User->>App: Trigger "Load" (menu / UI)
App->>LoadDialog: Open dialog (if API available) / or call LoadPipeline.loadFromFile() fallback
alt Load from API (via dialog)
User->>LoadDialog: Request list / select saved layout
LoadDialog->>PersistenceAPI: listSavedLayouts / loadSavedLayout(uuid)
PersistenceAPI-->>LoadDialog: layout + images
LoadDialog->>LoadPipeline: loadFromApi(uuid)
LoadPipeline->>LayoutStore: finalizeLayoutLoad(layout, images)
else Import from File (via dialog)
User->>LoadDialog: Click "Import from file"
LoadDialog->>FileSystem: Open file picker
FileSystem-->>LoadDialog: .zip file
LoadDialog->>Archive: extractFolderArchive(file) (guardrails)
Archive-->>LoadDialog: layout + images
LoadDialog->>LoadPipeline: loadFromFile(file)
LoadPipeline->>LayoutStore: finalizeLayoutLoad(layout, images)
else Fallback (API not available)
App->>LoadPipeline: loadFromFile()
LoadPipeline->>FileSystem: open file picker
FileSystem-->>LoadPipeline: .zip file (or cancel)
LoadPipeline->>Archive: extractFolderArchive(file)
Archive-->>LoadPipeline: layout + images
LoadPipeline->>LayoutStore: finalizeLayoutLoad(layout, images)
end
LoadDialog->>App: Close dialog on success
App-->>User: Layout rendered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:XXL
Contributor
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
src/lib/components/LoadDialog.svelte
Outdated
Comment on lines
38
to
44
| onMount(async () => { | ||
| if (apiActive) { | ||
| await loadLayouts(); | ||
| } else { | ||
| loading = false; | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: The saved layouts list in the Load dialog is only fetched once in onMount using the initial apiActive value; if the persistence API becomes available after app startup (the normal case, since availability is determined asynchronously in initializePersistence), apiActive flips to true but loadLayouts() is never called again, so users see an empty "No layouts saved on server yet." state even when layouts exist until they manually hit Retry. [logic error]
Severity Level: Major ⚠️
- ⚠️ Load dialog shows empty server layouts list initially.
- ⚠️ Users must click Retry before seeing saved layouts.
- ⚠️ Confusing UX when switching layouts via toolbar Load.
Suggested change
| onMount(async () => { | |
| if (apiActive) { | |
| await loadLayouts(); | |
| } else { | |
| loading = false; | |
| } | |
| }); | |
| onMount(async () => { | |
| if (apiActive) { | |
| await loadLayouts(); | |
| } else { | |
| loading = false; | |
| } | |
| }); | |
| $effect(() => { | |
| // When API becomes available after mount and we haven't loaded yet, | |
| // automatically fetch the layouts list once. | |
| if (apiActive && !loading && layouts.length === 0 && !error) { | |
| loading = true; | |
| loadLayouts(); | |
| } | |
| }); |
Steps of Reproduction ✅
1. Start the app so `App.svelte` mounts and runs its first `onMount` block
(`src/App.svelte:264-389`), which calls `initializePersistence()`
(`src/lib/stores/persistence.svelte.ts:74-106`) asynchronously; at this point
`apiAvailable` is `null` (`line 41`), so `isApiAvailable()` (`line 50`) returns `false`.
2. During the same initial mount, `LoadDialog.svelte` is also mounted via `<LoadDialog />`
in `App.svelte` (`src/App.svelte:1766`), and its `onMount` handler
(`src/lib/components/LoadDialog.svelte:38-44`) runs while `isApiAvailable()` is still
`false`, so `apiActive` is `false` and the code sets `loading = false` without calling
`loadLayouts()`.
3. After the persistence health check finishes, `initializePersistence()` sets
`apiAvailable` to `true` (`src/lib/stores/persistence.svelte.ts:90-99`), so
`isApiAvailable()` now returns `true`; later, when the user clicks "Load" in the toolbar,
`handleLoad()` in `App.svelte` (`src/App.svelte:704-710`) sees `isApiAvailable()` as
`true` and opens the Load dialog via `dialogStore.open("load")`.
4. When the Load dialog becomes visible, its internal state is `apiActive === true`,
`loading === false`, `layouts === []`, `error === null` (because `loadLayouts()` was never
called after API became available), so the `{#if}` chain in `LoadDialog.svelte`
(`src/lib/components/LoadDialog.svelte:148-161`) takes the `{:else if !layouts ||
layouts.length === 0}` branch and renders "No layouts saved on server yet." even though
`listSavedLayouts()` (used by `loadLayouts()`) would return actual saved layouts until the
user manually clicks the "Retry" button to trigger `loadLayouts()` once.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/components/LoadDialog.svelte
**Line:** 38:44
**Comment:**
*Logic Error: The saved layouts list in the Load dialog is only fetched once in `onMount` using the initial `apiActive` value; if the persistence API becomes available after app startup (the normal case, since availability is determined asynchronously in `initializePersistence`), `apiActive` flips to true but `loadLayouts()` is never called again, so users see an empty "No layouts saved on server yet." state even when layouts exist until they manually hit Retry.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
src/lib/utils/load-pipeline.ts
Outdated
Comment on lines
27
to
47
| const layoutStore = getLayoutStore(); | ||
| const imageStore = getImageStore(); | ||
| const toastStore = getToastStore(); | ||
| const selectionStore = getSelectionStore(); | ||
| const canvasStore = getCanvasStore(); | ||
|
|
||
| // Clear and restore images if provided | ||
| if (images) { | ||
| imageStore.clearAllImages(); | ||
| for (const [deviceSlug, deviceImages] of images) { | ||
| if (deviceImages.front) { | ||
| imageStore.setDeviceImage(deviceSlug, "front", deviceImages.front); | ||
| } | ||
| if (deviceImages.rear) { | ||
| imageStore.setDeviceImage(deviceSlug, "rear", deviceImages.rear); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Always ensure bundled images are loaded | ||
| imageStore.loadBundledImages(); |
Contributor
There was a problem hiding this comment.
Suggestion: When loading a layout from a ZIP archive, bundled device images are loaded after custom images and overwrite them, so user-uploaded overrides (including placement-level overrides for built‑in device types) will be silently lost on load; bundled images should be the fallback base, with custom images applied last to take precedence. [logic error]
Severity Level: Critical 🚨
- ❌ Importing ZIP loses custom images for built-in device types.
- ⚠️ Start screen "Import File" fails to restore overrides.
- ⚠️ Toolbar and keyboard load shortcuts drop customized visuals.
Suggested change
| const layoutStore = getLayoutStore(); | |
| const imageStore = getImageStore(); | |
| const toastStore = getToastStore(); | |
| const selectionStore = getSelectionStore(); | |
| const canvasStore = getCanvasStore(); | |
| // Clear and restore images if provided | |
| if (images) { | |
| imageStore.clearAllImages(); | |
| for (const [deviceSlug, deviceImages] of images) { | |
| if (deviceImages.front) { | |
| imageStore.setDeviceImage(deviceSlug, "front", deviceImages.front); | |
| } | |
| if (deviceImages.rear) { | |
| imageStore.setDeviceImage(deviceSlug, "rear", deviceImages.rear); | |
| } | |
| } | |
| } | |
| // Always ensure bundled images are loaded | |
| imageStore.loadBundledImages(); | |
| const layoutStore = getLayoutStore(); | |
| const imageStore = getImageStore(); | |
| const toastStore = getToastStore(); | |
| const selectionStore = getSelectionStore(); | |
| const canvasStore = getCanvasStore(); | |
| // Clear and restore images if provided | |
| if (images) { | |
| imageStore.clearAllImages(); | |
| // Load bundled images first as defaults | |
| imageStore.loadBundledImages(); | |
| // Then overlay any custom images from the archive so they take precedence | |
| for (const [deviceSlug, deviceImages] of images) { | |
| if (deviceImages.front) { | |
| imageStore.setDeviceImage(deviceSlug, "front", deviceImages.front); | |
| } | |
| if (deviceImages.rear) { | |
| imageStore.setDeviceImage(deviceSlug, "rear", deviceImages.rear); | |
| } | |
| } | |
| } else { | |
| // Ensure bundled images are loaded when no custom images are provided | |
| imageStore.loadBundledImages(); | |
| } |
Steps of Reproduction ✅
1. In a test or runtime session, obtain the image store via `getImageStore()` from
`src/lib/stores/images.svelte.ts:206-211`. Note that bundled device slugs come from
`getBundledImageSlugs()` used inside `loadBundledImages()` at
`src/lib/stores/images.svelte.ts:108-131`.
2. Create a layout that uses at least one built-in device type (a slug returned by
`getBundledImageSlugs()`), then set a custom image for that slug using
`imageStore.setDeviceImage(bundledSlug, "front", customImage)` as defined in
`src/lib/stores/images.svelte.ts:34-40`. This mimics a user overriding the default image
for a built-in device type.
3. Save the layout as a folder-based archive using the existing save flow in
`src/App.svelte:671-681`, which calls `imageStore.getUserImages()` and
`downloadArchive()`. `downloadArchive()` delegates to `createFolderArchive()` in
`src/lib/utils/archive.ts:222-314`, which writes those custom images into the ZIP (see
device-type branch at `archive.ts:263-309`).
4. Import the saved archive through any load entrypoint that calls `loadFromFile()` in
`src/lib/utils/load-pipeline.ts:97-107` (e.g., toolbar "Load" handler `handleLoad()` at
`src/App.svelte:704-709`, StartScreen `handleImportFile()` at
`src/lib/components/StartScreen.svelte:128-133`, or LoadDialog `handleImportFile()` at
`src/lib/components/LoadDialog.svelte:101-106`). `loadFromFile()` calls
`extractFolderArchive()` (which rebuilds the `images: ImageStoreMap`) and then
`finalizeLayoutLoad(layout, images, failedImages.length)` at
`src/lib/utils/load-pipeline.ts:104-106`. Inside `finalizeLayoutLoad()`
(`load-pipeline.ts:22-74`), the current implementation first applies the custom images
(`setDeviceImage()` loop at lines 34-43) and then calls `imageStore.loadBundledImages()`
at line 47. Because `loadBundledImages()` overwrites entries for bundled slugs, the
bundled defaults replace the just-restored custom images for those same slugs, so after
import the layout shows default graphics instead of the user's saved overrides.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/utils/load-pipeline.ts
**Line:** 27:47
**Comment:**
*Logic Error: When loading a layout from a ZIP archive, bundled device images are loaded after custom images and overwrite them, so user-uploaded overrides (including placement-level overrides for built‑in device types) will be silently lost on load; bundled images should be the fallback base, with custom images applied last to take precedence.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/LoadDialog.svelte`:
- Line 55: Replace the direct console.error call in LoadDialog.svelte with the
namespaced debug logger: import the layoutDebug from '$lib/utils/debug' (using
the prescribed import pattern) and call layoutDebug with a descriptive message
and the error object (e.g., include the error via formatting/placeholders)
instead of console.error("Failed to load layouts:", e); update the code path
that catches layout-loading errors to use layoutDebug so logs follow the
project's debug namespace conventions.
- Around line 38-44: The current onMount block only calls loadLayouts once and
misses later changes to apiActive or dialog reopen; update LoadDialog.svelte to
also react to changes in apiActive and the dialog-open state (e.g., a
prop/variable like open or show) by invoking loadLayouts whenever apiActive
becomes true or the dialog is reopened, and ensure loading is set false on
failures; specifically, add a reactive watcher/statement that watches apiActive
and the dialog-open flag and calls loadLayouts() (and sets loading
appropriately) when apiActive transitions to true or when the dialog is shown
again so the list is refreshed.
In `@src/lib/utils/archive.ts`:
- Around line 331-337: The empty-blob guard currently runs after calling
JSZip.loadAsync, which can throw on zero-byte uploads; move the check for
blob.size === 0 to run before calling getJSZip()/JSZip.loadAsync so you throw
the "Archive file is empty (0 bytes)." error deterministically; specifically, in
the function where getJSZip() and JSZip.loadAsync(blob) are used, check
blob.size first and throw if zero, then call getJSZip() and
JSZip.loadAsync(blob) only when blob is non-empty.
- Around line 347-357: The code is incorrectly reading JSZip internals via
file._data.uncompressedSize which is not part of the public API; update
extractFolderArchive, extractNewFormatZip, and extractOldFormatZip to
asynchronously obtain each ZipObject's uncompressed bytes via the public
file.async(...) API (e.g., "uint8array" or "nodebuffer") and use the resulting
byte length to compute totalUncompressedSize, compression ratios, and
per-file/YAML size checks before parsing; replace all usages of
file._data?.uncompressedSize with await file.async(...) size measurements (and
reuse the loaded bytes where possible to avoid double-decompression) to ensure
guardrails are reliable.
In `@src/lib/utils/load-pipeline.ts`:
- Around line 33-44: Always clear previously set custom images before finalizing
any layout load: call imageStore.clearAllImages() unconditionally before
invoking finalizeLayoutLoad(layout), then only apply new images when the images
parameter is present by iterating and calling
imageStore.setDeviceImage(deviceSlug, "front"|"rear", ...). Update the block
that currently guards clearAllImages() behind if (images) and the analogous code
path used before finalizeLayoutLoad(layout) so that previous overrides cannot
leak into API-loaded layouts.
- Line 109: Replace the console.error call in load-pipeline.ts with the project
namespaced debug logger: add the import `import { layoutDebug } from
'$lib/utils/debug'` if missing, then call layoutDebug with the same message and
the error (e.g., include the error object as a structured parameter) instead of
console.error so the failure to load layout is logged via the debug namespace
and can be filtered at runtime.
In `@src/lib/utils/persistence-api.ts`:
- Around line 205-214: The JSON parsing (await response.json()) is outside the
try/catch so a SyntaxError can escape; move the response.json() call into the
same try block used for validation in listSavedLayouts (i.e., call const rawData
= await response.json() inside the try, then run
LayoutListResponseSchema.parse(rawData)), and in the catch catch any error and
rethrow a PersistenceError("Invalid response from API server") after logging
(log("listSavedLayouts: validation failed %O", error)) so all parse/validation
failures are handled consistently.
In `@src/tests/archive-guardrails.test.ts`:
- Around line 5-58: Add a regression test that validates extractFolderArchive
rejects a zero-byte archive: create an empty Blob (size 0) or a JSZip-generated
empty blob with its size mocked to 0 and add an it(...) in the "Archive
Guardrails" suite that awaits
expect(extractFolderArchive(zeroByteBlob)).rejects.toThrow(...) checking the
same guardrail error path used for invalid archives (e.g., "Archive too
large"/generic "invalid archive" message used by extractFolderArchive) so the
zero-length edge case is covered alongside the existing
size/entry/compression/YAML tests.
In `@src/tests/load-pipeline.test.ts`:
- Around line 42-45: The requestAnimationFrame stub created with
vi.stubGlobal("requestAnimationFrame", ...) is not undone and can leak into
other tests; fix it by adding a teardown that restores the global after each
test (e.g. add an afterEach or afterAll hook that calls
vi.unstubGlobal("requestAnimationFrame") or vi.unstubAllGlobals()) so the
stubbed "requestAnimationFrame" is removed after tests run.
In `@src/tests/LoadDialog.test.ts`:
- Around line 33-137: Add a new test that simulates API initially unavailable
then becoming available and verifies listSavedLayouts is fetched: mock
persistenceStore.isApiAvailable to return false, open the dialog via
dialogStore.open("load") and render LoadDialog, assert
persistenceApi.listSavedLayouts has not been called, then change the mock
(persistenceStore.isApiAvailable) to return true, trigger the dialog refresh by
reopening or calling the same open/refresh path used in the component (e.g.,
dialogStore.open("load") or whatever LoadDialog listens to), await the component
update, and assert persistenceApi.listSavedLayouts was called and the layout UI
appears; reference persistenceStore.isApiAvailable, dialogStore.open,
persistenceApi.listSavedLayouts, and LoadDialog to locate relevant code.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/App.sveltesrc/lib/components/LoadDialog.sveltesrc/lib/components/StartScreen.sveltesrc/lib/stores/dialogs.svelte.tssrc/lib/utils/archive.tssrc/lib/utils/load-pipeline.tssrc/lib/utils/persistence-api.tssrc/tests/App.cleanupPrompt.test.tssrc/tests/App.startScreen.test.tssrc/tests/LoadDialog.test.tssrc/tests/archive-guardrails.test.tssrc/tests/load-pipeline.test.ts
- Replace private JSZip _data.uncompressedSize with public async API - Move empty blob check before JSZip.loadAsync to avoid confusing errors - Fix image load ordering: always clear → bundled → custom overlay - Replace onMount with $effect for reactive API fetch in LoadDialog - Move response.json() inside try/catch in persistence-api.ts - Replace console.error/warn with debug loggers - Fix requestAnimationFrame stub leak in load-pipeline tests - Add zero-byte archive guardrail test - Add delayed API availability test for LoadDialog Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@CodeAnt-AI review |
Contributor
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Collaborator
Author
|
@CodeRabbit-ai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
codeant-ai
bot
added
size:XXL
Comment on lines
197
to
208
| <button | ||
| class="delete-btn" | ||
| onclick={(e) => { | ||
| e.stopPropagation(); | ||
| handleDeleteLayout(item); | ||
| }} | ||
| disabled={deletingId === item.id} | ||
| aria-label={`Delete layout ${item.name}`} | ||
| title="Delete layout" | ||
| > | ||
| <IconTrash size={16} /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Suggestion: When the delete button has keyboard focus, pressing Enter or Space will both trigger the delete action (via the button's default click behavior) and bubble a keydown event up to the parent .layout-item, which interprets Enter/Space as a request to open the layout; this means keyboard users will unintentionally open a layout while trying to delete it. Add a keydown handler on the delete button that prevents propagation and handles Enter/Space itself, matching the behavior already used on the StartScreen delete button. [logic error]
Severity Level: Major ⚠️
- ⚠️ Keyboard users cannot delete from Load dialog reliably.
- ⚠️ Delete button activates wrong action (opens layout instead).
- ⚠️ Inconsistent behavior vs StartScreen delete interaction.
Suggested change
| <button | |
| class="delete-btn" | |
| onclick={(e) => { | |
| e.stopPropagation(); | |
| handleDeleteLayout(item); | |
| }} | |
| disabled={deletingId === item.id} | |
| aria-label={`Delete layout ${item.name}`} | |
| title="Delete layout" | |
| > | |
| <IconTrash size={16} /> | |
| </button> | |
| <button | |
| class="delete-btn" | |
| onclick={(e) => { | |
| e.stopPropagation(); | |
| handleDeleteLayout(item); | |
| }} | |
| onkeydown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| handleDeleteLayout(item); | |
| } | |
| }} | |
| disabled={deletingId === item.id} | |
| aria-label={`Delete layout ${item.name}`} | |
| title="Delete layout" | |
| > | |
| <IconTrash size={16} /> | |
| </button> |
Steps of Reproduction ✅
1. Open the main application so that the `LoadDialog` component (imported in
`/workspace/Rackula/src/App.svelte:39` and rendered at
`/workspace/Rackula/src/App.svelte:1766` as `<LoadDialog />`) can be shown via the dialogs
store.
2. Trigger the "Load Layout" dialog so the saved-layout list from
`src/lib/components/LoadDialog.svelte` is visible; each layout row is rendered by the
`.layout-item` container with `onkeydown={(e) => { if (e.key === "Enter" || e.key === " ")
{ e.preventDefault(); handleOpenLayout(item); } }}` (see lines 171–179 of the new hunk),
which opens the layout when Enter/Space is pressed anywhere within the row.
3. Tab keyboard focus onto a layout's delete button inside that row, which is rendered as
`<button class="delete-btn" ...>` with only an `onclick` handler that stops click
propagation and calls `handleDeleteLayout(item)` but has no `onkeydown` (lines 197–208 in
`src/lib/components/LoadDialog.svelte`).
4. Press Enter or Space: the `keydown` event originates on the delete button, bubbles to
the parent `.layout-item` `onkeydown` handler (because nothing on the button stops
propagation), `handleOpenLayout(item)` executes and opens the layout, while the delete
action is not triggered via keyboard; this reproduces the bug where keyboard activation of
the delete button opens the layout instead of deleting it. This differs from
`src/lib/components/StartScreen.svelte:240–252`, where the delete button includes its own
`onkeydown` handler that prevents propagation and calls `handleDeleteLayout(item)`,
confirming the intended pattern.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/components/LoadDialog.svelte
**Line:** 197:208
**Comment:**
*Logic Error: When the delete button has keyboard focus, pressing Enter or Space will both trigger the delete action (via the button's default click behavior) and bubble a keydown event up to the parent `.layout-item`, which interprets Enter/Space as a request to open the layout; this means keyboard users will unintentionally open a layout while trying to delete it. Add a keydown handler on the delete button that prevents propagation and handles Enter/Space itself, matching the behavior already used on the StartScreen delete button.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/LoadDialog.svelte`:
- Around line 166-209: The outer interactive container currently uses
role="button" and nests a real <button>, which breaks accessibility; change the
outer clickable "layout" element into a real button element (e.g., use a <button
type="button"> with the existing classes, aria-disabled bound to !item.valid,
and replace the custom onkeydown Enter/Space handling with native button
behavior) and move the delete control out to be a sibling (keep the delete
button using handleDeleteLayout and e.stopPropagation() so it doesn't trigger
handleOpenLayout); preserve the deletingId, handleOpenLayout,
handleDeleteLayout, item.valid, item.id, and the existing aria-label/title and
disabled bindings on the delete button.
In `@src/lib/utils/archive.ts`:
- Around line 341-353: The loop that sums uncompressed sizes (using entries,
zip.files and file.async) should short-circuit as soon as totalUncompressedSize
exceeds LIMITS.MAX_TOTAL_UNCOMPRESSED_BYTES to avoid decompressing remaining
files; modify the loop in archive.ts where totalUncompressedSize is accumulated
to check after each bytes.byteLength addition (or before continuing) and
immediately throw the existing Error when the cap is crossed, so you stop
calling file.async on further entries and exit early.
In `@src/tests/load-pipeline.test.ts`:
- Around line 90-134: Replace the invalid TypeScript casts "as vi.Mock" with the
official Mock type from Vitest: import { Mock } from 'vitest' at the top of the
test file and change the casts to "as Mock" for the mocked calls (e.g.,
persistenceApi.loadSavedLayout, fileUtils.openFilePicker,
archive.extractFolderArchive and the other mocked references in the file);
update all five occurrences so the mocks compile under strict TypeScript.
In `@src/tests/LoadDialog.test.ts`:
- Around line 38-163: Replace the non-idiomatic type assertions "as vi.Mock"
with the exported Vitest type "as Mock" throughout the test file (affecting
usages like persistenceStore.isApiAvailable, persistenceApi.listSavedLayouts,
loadPipeline.loadFromApi, loadPipeline.loadFromFile, etc.), and add a top-level
import { Mock } from "vitest" so the type is available; update all ~10
occurrences to use "as Mock" to match Vitest 4.x typings.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/lib/components/LoadDialog.sveltesrc/lib/utils/archive.tssrc/lib/utils/load-pipeline.tssrc/lib/utils/persistence-api.tssrc/tests/LoadDialog.test.tssrc/tests/archive-guardrails.test.tssrc/tests/load-pipeline.test.ts
Comment on lines
166
to
209
| <div | ||
| class="layout-item" | ||
| class:invalid={!item.valid} | ||
| class:deleting={deletingId === item.id} | ||
| role="button" | ||
| tabindex={item.valid ? 0 : -1} | ||
| aria-disabled={!item.valid} | ||
| onclick={() => handleOpenLayout(item)} | ||
| onkeydown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| handleOpenLayout(item); | ||
| } | ||
| }} | ||
| > | ||
| <div class="layout-info"> | ||
| <span class="layout-name"> | ||
| {item.name} | ||
| {#if !item.valid} | ||
| <span class="error-badge" title="Corrupted file">!</span> | ||
| {/if} | ||
| </span> | ||
| <span class="layout-meta"> | ||
| {#if item.valid} | ||
| {formatCounts(item)} - {formatDate(item.updatedAt)} | ||
| {:else} | ||
| <span class="error-text">File corrupted</span> - | ||
| {formatDate(item.updatedAt)} | ||
| {/if} | ||
| </span> | ||
| </div> | ||
| <button | ||
| class="delete-btn" | ||
| onclick={(e) => { | ||
| e.stopPropagation(); | ||
| handleDeleteLayout(item); | ||
| }} | ||
| disabled={deletingId === item.id} | ||
| aria-label={`Delete layout ${item.name}`} | ||
| title="Delete layout" | ||
| > | ||
| <IconTrash size={16} /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
nesting button element inside role="button" WCAG WAI-ARIA accessibility
💡 Result:
Nesting a native interactive control (like a <button>) inside an element with role="button" is not conforming and is a common accessibility/UX failure.
- ARIA-in-HTML rule (allowed descendants): An element with
role="button"must follow the same descendant restrictions as a native<button>—it must not contain interactive descendants (or descendants withtabindex). The spec gives<div role="button"><button>…</button></div>as an example of non-conforming descendants. [1] - HTML content model: A native
<button>’s permitted content is “phrasing content” and must not include interactive content, reinforcing the “no interactive-inside-interactive” rule. [2] - Best practice: Prefer a single native control (usually a real
<button>) instead of addingrole="button"to a non-button element. [3]
What to do instead
- Make only one element the actual control (e.g., one
<button>for the whole clickable area), and keep inner elements non-interactive; or - If you need multiple actions inside a “card,” make the outer container non-interactive and keep separate inner buttons/links (don’t make the container
role="button"/tabindex="0").
Sources: ARIA in HTML (W3C) [1], MDN <button> element reference [2], W3C “Notes on ARIA Use in HTML” [3].
🌐 Web query:
interactive element inside role="button" WAI-ARIA anti-pattern
💡 Result:
Putting an interactive element (e.g., <a>, <button>, <input>, anything focusable/clickable) inside an element with role="button" is a WAI‑ARIA/HTML anti‑pattern because:
role="button"has “presentational children” behavior: browsers/AT effectively treat descendants asrole="presentation", so the nested control’s semantics (role/name) may be suppressed in the accessibility tree. The inner control may no longer be exposed/announced as a real link/button/etc. [1] [2]- If a descendant remains focusable (e.g., it has
tabindex="0"or is inherently focusable like<a href>), focus can land on something that has no corresponding accessible node, breaking “Name, Role, Value” expectations and leading to confusing/incorrect SR output. [3] - For native
<button>, HTML conformance rules also prohibit interactive content descendants (and descendants withtabindex) for similar reasons (nested controls lead to unpredictable behavior). [4]
Preferred fix: don’t nest interactive controls. Make one element the button, and keep any other controls as separate sibling elements (or redesign so only one action is interactive). [3] [4]
Sources:
[1] MDN – ARIA button role (“All descendants are presentational”) https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role
[2] W3C WAI‑ARIA Authoring Practices 1.1 – roles whose descendants are automatically presentational (includes button) https://www.w3.org/TR/2017/NOTE-wai-aria-practices-1.1-20171214/
[3] W3C ACT rule – presentational-children roles + focusable descendants cause missing name/role in accessibility tree https://www.w3.org/WAI/standards-guidelines/act/rules/307n5z/
[4] W3C HTML reference – nesting constraints for button (e.g., button not descendant of button; interactive nesting constraints) https://w3c.github.io/html-reference/spec.html
🌐 Web query:
WCAG guidelines multiple interactive controls same row layout
💡 Result:
WCAG doesn’t prohibit placing multiple interactive controls in the same row (e.g., a horizontal button group/toolbar). What matters is that the row still meets the relevant success criteria:
- Adequate hit area / spacing between adjacent controls (especially in dense rows): Meet SC 2.5.8 Target Size (Minimum) (AA, WCAG 2.2) by making each target at least 24×24 CSS px, or ensuring sufficient spacing so small targets aren’t easy to tap/click by mistake. [1]
- Keyboard tab order matches a logical/expected sequence: Ensure SC 2.4.3 Focus Order (A)—focus should move through the controls in an order that preserves meaning and usability (typically left-to-right, top-to-bottom to match the UI). [2]
- Programmatic order/reading order remains meaningful (don’t “fake” the row with CSS reordering): If sequence affects meaning, meet SC 1.3.2 Meaningful Sequence (A) by keeping DOM order meaningful (be careful with CSS
order, reversing flex rows, absolute positioning, etc.). [3] - Clear labels/names for each control (especially icon-only buttons in a row): Meet SC 4.1.2 Name, Role, Value (A) (use native controls where possible; ensure each control has a programmatic name via text/
aria-label/aria-labelledby). [4] - If the row is a related set of form controls, group them semantically (not just visually): Use
fieldset/legendto convey relationships for grouped options (technique H71, supports SC 1.3.1 Info and Relationships (A)). [5]
Rule of thumb: A “same-row” layout is fine if (1) targets aren’t too small/close together, (2) focus/reading order is predictable, and (3) relationships and labels are conveyed in code—not only by visual proximity.
Sources: [1] [2] [3] [4] [5]
Avoid nesting interactive controls inside role="button".
A role="button" container with a nested <button> violates WCAG and WAI-ARIA conformance: the inner button's semantics may be suppressed in the accessibility tree, and nested focusable elements can break keyboard navigation and screen reader announcements.
Refactor into separate sibling controls: convert the outer container to a clickable button (or remove role="button" entirely), and keep the delete button as an independent sibling. This maintains accessibility and keyboard behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/LoadDialog.svelte` around lines 166 - 209, The outer
interactive container currently uses role="button" and nests a real <button>,
which breaks accessibility; change the outer clickable "layout" element into a
real button element (e.g., use a <button type="button"> with the existing
classes, aria-disabled bound to !item.valid, and replace the custom onkeydown
Enter/Space handling with native button behavior) and move the delete control
out to be a sibling (keep the delete button using handleDeleteLayout and
e.stopPropagation() so it doesn't trigger handleOpenLayout); preserve the
deletingId, handleOpenLayout, handleDeleteLayout, item.valid, item.id, and the
existing aria-label/title and disabled bindings on the delete button.
src/tests/LoadDialog.test.ts
Outdated
Comment on lines
38
to
163
| (persistenceStore.isApiAvailable as vi.Mock).mockReturnValue(true); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| dialogStore.close(); | ||
| }); | ||
|
|
||
| it("shows loading state while fetching layouts", async () => { | ||
| let resolveLayouts: (value: unknown) => void; | ||
| const pendingPromise = new Promise((resolve) => { | ||
| resolveLayouts = resolve; | ||
| }); | ||
| (persistenceApi.listSavedLayouts as vi.Mock).mockReturnValue(pendingPromise); | ||
|
|
||
| dialogStore.open("load"); | ||
| render(LoadDialog); | ||
| await tick(); | ||
|
|
||
| // Initial state should show loading | ||
| expect(screen.getByText(/Loading saved layouts/i)).toBeInTheDocument(); | ||
| expect(screen.getByTestId("spinner-loader")).toBeInTheDocument(); | ||
|
|
||
| // Resolve promise and loading should disappear | ||
| resolveLayouts!([]); | ||
| await waitFor(() => { | ||
| expect(screen.queryByText(/Loading saved layouts/i)).not.toBeInTheDocument(); | ||
| }); | ||
| }); | ||
|
|
||
| it("lists saved layouts from the API when available", async () => { | ||
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | ||
| { | ||
| id: "uuid-1", | ||
| name: "Test Layout 1", | ||
| version: "1.0", | ||
| updatedAt: new Date().toISOString(), | ||
| rackCount: 1, | ||
| deviceCount: 5, | ||
| valid: true, | ||
| }, | ||
| ]; | ||
| (persistenceApi.listSavedLayouts as vi.Mock).mockResolvedValue(mockLayouts); | ||
|
|
||
| dialogStore.open("load"); | ||
| render(LoadDialog); | ||
|
|
||
| expect(await screen.findByText("Test Layout 1")).toBeInTheDocument(); | ||
| expect(screen.getByText(/1 rack, 5 devices/i)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("calls loadFromApi and closes when a layout is clicked", async () => { | ||
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | ||
| { | ||
| id: "uuid-1", | ||
| name: "Test Layout 1", | ||
| version: "1.0", | ||
| updatedAt: new Date().toISOString(), | ||
| rackCount: 1, | ||
| deviceCount: 5, | ||
| valid: true, | ||
| }, | ||
| ]; | ||
| (persistenceApi.listSavedLayouts as vi.Mock).mockResolvedValue(mockLayouts); | ||
| (loadPipeline.loadFromApi as vi.Mock).mockResolvedValue(true); | ||
|
|
||
| dialogStore.open("load"); | ||
| render(LoadDialog); | ||
|
|
||
| const layoutItem = await screen.findByText("Test Layout 1"); | ||
| await fireEvent.click(layoutItem); | ||
|
|
||
| expect(loadPipeline.loadFromApi).toHaveBeenCalledWith("uuid-1"); | ||
| expect(dialogStore.isOpen("load")).toBe(false); | ||
| }); | ||
|
|
||
| it("calls loadFromFile and closes when import button is clicked", async () => { | ||
| (loadPipeline.loadFromFile as vi.Mock).mockResolvedValue(true); | ||
|
|
||
| dialogStore.open("load"); | ||
| render(LoadDialog); | ||
|
|
||
| const importBtn = screen.getByText(/Import from local file/i); | ||
| await fireEvent.click(importBtn); | ||
|
|
||
| expect(loadPipeline.loadFromFile).toHaveBeenCalled(); | ||
| expect(dialogStore.isOpen("load")).toBe(false); | ||
| }); | ||
|
|
||
| it("fetches layouts when API becomes available after initial render", async () => { | ||
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | ||
| { | ||
| id: "uuid-delayed", | ||
| name: "Delayed Layout", | ||
| version: "1.0", | ||
| updatedAt: new Date().toISOString(), | ||
| rackCount: 2, | ||
| deviceCount: 10, | ||
| valid: true, | ||
| }, | ||
| ]; | ||
| // Start with API unavailable | ||
| (persistenceStore.isApiAvailable as vi.Mock).mockReturnValue(false); | ||
| (persistenceApi.listSavedLayouts as vi.Mock).mockResolvedValue(mockLayouts); | ||
|
|
||
| dialogStore.open("load"); | ||
| render(LoadDialog); | ||
| await tick(); | ||
|
|
||
| // API not available yet — listSavedLayouts should not be called | ||
| expect(persistenceApi.listSavedLayouts).not.toHaveBeenCalled(); | ||
|
|
||
| // Simulate API becoming available | ||
| (persistenceStore.isApiAvailable as vi.Mock).mockReturnValue(true); | ||
| // Trigger reactivity by re-rendering (the $effect watches apiActive which is $derived) | ||
| // In real app, the persistence store would trigger this. In test, we need to | ||
| // re-render since mocked isApiAvailable isn't reactive. | ||
| dialogStore.close(); | ||
| dialogStore.open("load"); | ||
| render(LoadDialog); | ||
|
|
||
| expect(await screen.findByText("Delayed Layout")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("shows error state when API fails to list layouts", async () => { | ||
| (persistenceApi.listSavedLayouts as vi.Mock).mockRejectedValue( | ||
| new persistenceApi.PersistenceError("Server error"), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vitest 4.x Mock type TypeScript strict mode vi.Mock vs Mock imported
💡 Result:
In Vitest 4.x, the “mock function” type you want in TypeScript strict mode is the exported type from vitest (e.g. Mock, MockInstance, etc.), not something like vi.Mock.
1) vi is a runtime helper; the mock types are exported from vitest
vi.fn() returns a typed mock function, and Vitest’s API docs show the mock methods returning Mock<T> (a type). For example, mockImplementation(fn: T): Mock<T>. (vitest.dev)
So for typing, do this:
import { vi, type Mock } from 'vitest'
type FetchUser = (id: string) => Promise<{ id: string }>
const fetchUser: Mock<FetchUser> = vi.fn()2) Prefer vi.mocked(...) to “convert” an imported function to a typed mock
When you mock/spy module exports, the import’s static type often stays as the original function type. Vitest’s guide shows using vi.mocked(exampleObject.answer) to get a properly-typed mocked handle you can call mock methods on. (vitest.dev)
import * as api from './api'
import { vi } from 'vitest'
vi.mock('./api', { spy: true })
vi.mocked(api.getUser).mockResolvedValue({ id: '1' })3) vi.Mock vs Mock (imported)
If you see examples using vi.Mock, that’s either:
- mixing up value namespace (
vi) with type exports, or - relying on global/namespace typing that’s brittle under strict TS settings.
In Vitest 4.x, the stable/idiomatic approach is: import mock-related types from vitest (like Mock) and use vi.mocked() to get strict, correct typing. (vitest.dev)
Citations:
- 1: https://vitest.dev/api/mock
- 2: https://vitest.dev/guide/mocking/modules.html?utm_source=openai
- 3: https://vitest.dev/api/mock
🏁 Script executed:
# Check if the test file exists and examine its imports and usage
fd LoadDialog.test.ts --type fRepository: RackulaLives/Rackula
Length of output: 92
🏁 Script executed:
# Read the test file to see imports and current vi.Mock usage
head -40 src/tests/LoadDialog.test.tsRepository: RackulaLives/Rackula
Length of output: 1342
🏁 Script executed:
# Check for tsconfig.json to confirm strict mode
find . -name "tsconfig.json" -o -name "tsconfig.test.json" | head -5Repository: RackulaLives/Rackula
Length of output: 99
🏁 Script executed:
# Check tsconfig.json for strict mode
cat tsconfig.json | head -20Repository: RackulaLives/Rackula
Length of output: 572
🏁 Script executed:
# Count all vi.Mock usages in LoadDialog.test.ts
rg "as vi\.Mock" src/tests/LoadDialog.test.ts -nRepository: RackulaLives/Rackula
Length of output: 845
🏁 Script executed:
# Get full line count to verify scope
wc -l src/tests/LoadDialog.test.tsRepository: RackulaLives/Rackula
Length of output: 96
Use Vitest's exported Mock type instead of vi.Mock.
Vitest 4.x exports Mock and MockInstance types directly from the vitest package. Using as vi.Mock is not the idiomatic pattern and doesn't align with Vitest's official API. There are 10 instances of this pattern in the file.
💡 Proposed fix
-import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
+import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from "vitest";
...
- (persistenceStore.isApiAvailable as vi.Mock).mockReturnValue(true);
+ (persistenceStore.isApiAvailable as Mock).mockReturnValue(true);
- (persistenceApi.listSavedLayouts as vi.Mock).mockReturnValue(pendingPromise);
+ (persistenceApi.listSavedLayouts as Mock).mockReturnValue(pendingPromise);Replace all 10 instances of as vi.Mock with as Mock throughout the file.
📝 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.
Suggested change
| (persistenceStore.isApiAvailable as vi.Mock).mockReturnValue(true); | |
| }); | |
| afterEach(() => { | |
| dialogStore.close(); | |
| }); | |
| it("shows loading state while fetching layouts", async () => { | |
| let resolveLayouts: (value: unknown) => void; | |
| const pendingPromise = new Promise((resolve) => { | |
| resolveLayouts = resolve; | |
| }); | |
| (persistenceApi.listSavedLayouts as vi.Mock).mockReturnValue(pendingPromise); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| await tick(); | |
| // Initial state should show loading | |
| expect(screen.getByText(/Loading saved layouts/i)).toBeInTheDocument(); | |
| expect(screen.getByTestId("spinner-loader")).toBeInTheDocument(); | |
| // Resolve promise and loading should disappear | |
| resolveLayouts!([]); | |
| await waitFor(() => { | |
| expect(screen.queryByText(/Loading saved layouts/i)).not.toBeInTheDocument(); | |
| }); | |
| }); | |
| it("lists saved layouts from the API when available", async () => { | |
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | |
| { | |
| id: "uuid-1", | |
| name: "Test Layout 1", | |
| version: "1.0", | |
| updatedAt: new Date().toISOString(), | |
| rackCount: 1, | |
| deviceCount: 5, | |
| valid: true, | |
| }, | |
| ]; | |
| (persistenceApi.listSavedLayouts as vi.Mock).mockResolvedValue(mockLayouts); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| expect(await screen.findByText("Test Layout 1")).toBeInTheDocument(); | |
| expect(screen.getByText(/1 rack, 5 devices/i)).toBeInTheDocument(); | |
| }); | |
| it("calls loadFromApi and closes when a layout is clicked", async () => { | |
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | |
| { | |
| id: "uuid-1", | |
| name: "Test Layout 1", | |
| version: "1.0", | |
| updatedAt: new Date().toISOString(), | |
| rackCount: 1, | |
| deviceCount: 5, | |
| valid: true, | |
| }, | |
| ]; | |
| (persistenceApi.listSavedLayouts as vi.Mock).mockResolvedValue(mockLayouts); | |
| (loadPipeline.loadFromApi as vi.Mock).mockResolvedValue(true); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| const layoutItem = await screen.findByText("Test Layout 1"); | |
| await fireEvent.click(layoutItem); | |
| expect(loadPipeline.loadFromApi).toHaveBeenCalledWith("uuid-1"); | |
| expect(dialogStore.isOpen("load")).toBe(false); | |
| }); | |
| it("calls loadFromFile and closes when import button is clicked", async () => { | |
| (loadPipeline.loadFromFile as vi.Mock).mockResolvedValue(true); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| const importBtn = screen.getByText(/Import from local file/i); | |
| await fireEvent.click(importBtn); | |
| expect(loadPipeline.loadFromFile).toHaveBeenCalled(); | |
| expect(dialogStore.isOpen("load")).toBe(false); | |
| }); | |
| it("fetches layouts when API becomes available after initial render", async () => { | |
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | |
| { | |
| id: "uuid-delayed", | |
| name: "Delayed Layout", | |
| version: "1.0", | |
| updatedAt: new Date().toISOString(), | |
| rackCount: 2, | |
| deviceCount: 10, | |
| valid: true, | |
| }, | |
| ]; | |
| // Start with API unavailable | |
| (persistenceStore.isApiAvailable as vi.Mock).mockReturnValue(false); | |
| (persistenceApi.listSavedLayouts as vi.Mock).mockResolvedValue(mockLayouts); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| await tick(); | |
| // API not available yet — listSavedLayouts should not be called | |
| expect(persistenceApi.listSavedLayouts).not.toHaveBeenCalled(); | |
| // Simulate API becoming available | |
| (persistenceStore.isApiAvailable as vi.Mock).mockReturnValue(true); | |
| // Trigger reactivity by re-rendering (the $effect watches apiActive which is $derived) | |
| // In real app, the persistence store would trigger this. In test, we need to | |
| // re-render since mocked isApiAvailable isn't reactive. | |
| dialogStore.close(); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| expect(await screen.findByText("Delayed Layout")).toBeInTheDocument(); | |
| }); | |
| it("shows error state when API fails to list layouts", async () => { | |
| (persistenceApi.listSavedLayouts as vi.Mock).mockRejectedValue( | |
| new persistenceApi.PersistenceError("Server error"), | |
| import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from "vitest"; | |
| import { render, screen, fireEvent, waitFor } from "@testing-library/svelte"; | |
| import { tick } from "svelte"; | |
| import LoadDialog from "$lib/components/dialogs/LoadDialog.svelte"; | |
| import * as persistenceApi from "$lib/api/persistence"; | |
| import * as loadPipeline from "$lib/pipeline/loadPipeline"; | |
| import { dialogStore } from "$lib/stores/dialog"; | |
| import { persistenceStore } from "$lib/stores/persistence"; | |
| // ... other test setup code ... | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| (persistenceStore.isApiAvailable as Mock).mockReturnValue(true); | |
| }); | |
| afterEach(() => { | |
| dialogStore.close(); | |
| }); | |
| it("shows loading state while fetching layouts", async () => { | |
| let resolveLayouts: (value: unknown) => void; | |
| const pendingPromise = new Promise((resolve) => { | |
| resolveLayouts = resolve; | |
| }); | |
| (persistenceApi.listSavedLayouts as Mock).mockReturnValue(pendingPromise); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| await tick(); | |
| // Initial state should show loading | |
| expect(screen.getByText(/Loading saved layouts/i)).toBeInTheDocument(); | |
| expect(screen.getByTestId("spinner-loader")).toBeInTheDocument(); | |
| // Resolve promise and loading should disappear | |
| resolveLayouts!([]); | |
| await waitFor(() => { | |
| expect(screen.queryByText(/Loading saved layouts/i)).not.toBeInTheDocument(); | |
| }); | |
| }); | |
| it("lists saved layouts from the API when available", async () => { | |
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | |
| { | |
| id: "uuid-1", | |
| name: "Test Layout 1", | |
| version: "1.0", | |
| updatedAt: new Date().toISOString(), | |
| rackCount: 1, | |
| deviceCount: 5, | |
| valid: true, | |
| }, | |
| ]; | |
| (persistenceApi.listSavedLayouts as Mock).mockResolvedValue(mockLayouts); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| expect(await screen.findByText("Test Layout 1")).toBeInTheDocument(); | |
| expect(screen.getByText(/1 rack, 5 devices/i)).toBeInTheDocument(); | |
| }); | |
| it("calls loadFromApi and closes when a layout is clicked", async () => { | |
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | |
| { | |
| id: "uuid-1", | |
| name: "Test Layout 1", | |
| version: "1.0", | |
| updatedAt: new Date().toISOString(), | |
| rackCount: 1, | |
| deviceCount: 5, | |
| valid: true, | |
| }, | |
| ]; | |
| (persistenceApi.listSavedLayouts as Mock).mockResolvedValue(mockLayouts); | |
| (loadPipeline.loadFromApi as Mock).mockResolvedValue(true); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| const layoutItem = await screen.findByText("Test Layout 1"); | |
| await fireEvent.click(layoutItem); | |
| expect(loadPipeline.loadFromApi).toHaveBeenCalledWith("uuid-1"); | |
| expect(dialogStore.isOpen("load")).toBe(false); | |
| }); | |
| it("calls loadFromFile and closes when import button is clicked", async () => { | |
| (loadPipeline.loadFromFile as Mock).mockResolvedValue(true); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| const importBtn = screen.getByText(/Import from local file/i); | |
| await fireEvent.click(importBtn); | |
| expect(loadPipeline.loadFromFile).toHaveBeenCalled(); | |
| expect(dialogStore.isOpen("load")).toBe(false); | |
| }); | |
| it("fetches layouts when API becomes available after initial render", async () => { | |
| const mockLayouts: persistenceApi.SavedLayoutItem[] = [ | |
| { | |
| id: "uuid-delayed", | |
| name: "Delayed Layout", | |
| version: "1.0", | |
| updatedAt: new Date().toISOString(), | |
| rackCount: 2, | |
| deviceCount: 10, | |
| valid: true, | |
| }, | |
| ]; | |
| // Start with API unavailable | |
| (persistenceStore.isApiAvailable as Mock).mockReturnValue(false); | |
| (persistenceApi.listSavedLayouts as Mock).mockResolvedValue(mockLayouts); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| await tick(); | |
| // API not available yet — listSavedLayouts should not be called | |
| expect(persistenceApi.listSavedLayouts).not.toHaveBeenCalled(); | |
| // Simulate API becoming available | |
| (persistenceStore.isApiAvailable as Mock).mockReturnValue(true); | |
| // Trigger reactivity by re-rendering (the $effect watches apiActive which is $derived) | |
| // In real app, the persistence store would trigger this. In test, we need to | |
| // re-render since mocked isApiAvailable isn't reactive. | |
| dialogStore.close(); | |
| dialogStore.open("load"); | |
| render(LoadDialog); | |
| expect(await screen.findByText("Delayed Layout")).toBeInTheDocument(); | |
| }); | |
| it("shows error state when API fails to list layouts", async () => { | |
| (persistenceApi.listSavedLayouts as Mock).mockRejectedValue( | |
| new persistenceApi.PersistenceError("Server error"), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/LoadDialog.test.ts` around lines 38 - 163, Replace the
non-idiomatic type assertions "as vi.Mock" with the exported Vitest type "as
Mock" throughout the test file (affecting usages like
persistenceStore.isApiAvailable, persistenceApi.listSavedLayouts,
loadPipeline.loadFromApi, loadPipeline.loadFromFile, etc.), and add a top-level
import { Mock } from "vitest" so the type is available; update all ~10
occurrences to use "as Mock" to match Vitest 4.x typings.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/tests/LoadDialog.test.ts (1)
38-50:⚠️ Potential issue | 🟠 MajorReplace
vi.Mockcasts with Vitest’s typed mock APIs.Line 38 and related lines rely on
as vi.Mock; this is brittle under strict typing and should be replaced withtype Mock(orvi.mocked(...)) across the file.💡 Proposed fix
-import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from "vitest"; ... - (persistenceStore.isApiAvailable as vi.Mock).mockReturnValue(true); + (persistenceStore.isApiAvailable as Mock).mockReturnValue(true); ... - (persistenceApi.listSavedLayouts as vi.Mock).mockReturnValue(pendingPromise); + (persistenceApi.listSavedLayouts as Mock).mockReturnValue(pendingPromise);Apply the same replacement for all
as vi.Mockoccurrences in this file.#!/bin/bash # Verify all remaining non-idiomatic Vitest casts in this file rg -n 'as vi\.Mock' src/tests/LoadDialog.test.tsAlso applies to: 79-102, 139-140, 150-150, 162-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/LoadDialog.test.ts` around lines 38 - 50, Replace all brittle "as vi.Mock" casts in this test file with Vitest's typed mock helpers: use vi.mocked(...) or the Mock<T> type. For each mocked function such as persistenceStore.isApiAvailable and persistenceApi.listSavedLayouts (and other occurrences around lines mentioned), swap (fn as vi.Mock) to either vi.mocked(fn) when asserting call behavior or declare the mock with Mock<typeof moduleFn> / Mock<FnSignature> and use that typed mock for mockReturnValue / mockResolvedValue. Make sure to update every occurrence (including the other blocks noted) so all mockReturnValue / mockResolvedValue calls use the typed mock helper instead of "as vi.Mock".src/lib/utils/archive.ts (1)
341-353:⚠️ Potential issue | 🟠 MajorShort-circuit the uncompressed-size loop once the cap is exceeded.
At Line 341 onward, the code keeps decompressing entries even after crossing
MAX_TOTAL_UNCOMPRESSED_BYTES, which increases CPU work on hostile archives.💡 Proposed fix
let totalUncompressedSize = 0; for (const name of entries) { const file = zip.files[name]; if (!file || file.dir) continue; const bytes = await file.async("uint8array"); totalUncompressedSize += bytes.byteLength; + if (totalUncompressedSize > LIMITS.MAX_TOTAL_UNCOMPRESSED_BYTES) { + throw new Error( + `Archive uncompressed size is too large (${Math.round(totalUncompressedSize / 1024 / 1024)}MB).`, + ); + } } - - if (totalUncompressedSize > LIMITS.MAX_TOTAL_UNCOMPRESSED_BYTES) { - throw new Error( - `Archive uncompressed size is too large (${Math.round(totalUncompressedSize / 1024 / 1024)}MB).`, - ); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/archive.ts` around lines 341 - 353, The loop that sums totalUncompressedSize is decompressing every entry even after exceeding LIMITS.MAX_TOTAL_UNCOMPRESSED_BYTES; change the loop that iterates over entries (using zip.files[name] and file.async(...)) to check after adding each file's byteLength whether totalUncompressedSize > LIMITS.MAX_TOTAL_UNCOMPRESSED_BYTES and if so immediately stop further processing (break out and throw the same error) to short-circuit work and prevent decompressing remaining entries.src/tests/load-pipeline.test.ts (1)
90-90:⚠️ Potential issue | 🟠 MajorUse Vitest
Mocktyping instead ofvi.Mockcasts.The
as vi.Mockassertions at Line 90 and related lines should be migrated totype Mock(orvi.mocked) to keep strict TypeScript compatibility.💡 Proposed fix
-import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from "vitest"; ... - (persistenceApi.loadSavedLayout as vi.Mock).mockResolvedValue(layout); + (persistenceApi.loadSavedLayout as Mock).mockResolvedValue(layout);Repeat for every
as vi.Mockin this file.#!/bin/bash # Verify all remaining non-idiomatic Vitest casts in this file rg -n 'as vi\.Mock' src/tests/load-pipeline.test.tsAlso applies to: 100-100, 118-119, 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/load-pipeline.test.ts` at line 90, Replace the non-idiomatic TypeScript casts using "as vi.Mock" in this test file with proper Vitest typings: use the Mock generic type or vi.mocked helpers (for example replace "(persistenceApi.loadSavedLayout as vi.Mock).mockResolvedValue(layout)" by typing the mocked function via Mock or using vi.mocked(persistenceApi.loadSavedLayout).mockResolvedValue) and apply the same change to all occurrences (e.g., persistenceApi.loadSavedLayout, persistenceApi.deleteLayout, persistenceApi.saveLayout, and other mocks referenced around lines 100, 118–119, 134). Ensure imports/types for Mock or vi.mocked are available and update each mock invocation to use the typed mock methods instead of the "as vi.Mock" cast.src/lib/components/LoadDialog.svelte (1)
166-209:⚠️ Potential issue | 🟠 MajorRemove nested interactive controls in layout rows.
Line 170 (
role="button"container) wrapping Line 197 (<button>) creates an interactive-in-interactive accessibility conflict and can produce incorrect keyboard behavior.💡 Proposed fix (structure)
-<div class="layout-item" role="button" tabindex={...} onclick={...} onkeydown={...}> - <div class="layout-info">...</div> - <button class="delete-btn" ...>...</button> -</div> +<div class="layout-item-row"> + <button + type="button" + class="layout-item" + class:invalid={!item.valid} + class:deleting={deletingId === item.id} + aria-disabled={!item.valid} + onclick={() => handleOpenLayout(item)} + > + <div class="layout-info">...</div> + </button> + <button class="delete-btn" ...>...</button> +</div>Keep the delete action as a sibling control (not a descendant of the open-layout control).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/LoadDialog.svelte` around lines 166 - 209, The layout row in LoadDialog.svelte currently nests the delete <button> inside the div that has role="button" and onclick calling handleOpenLayout(item), causing an interactive-in-interactive accessibility conflict; fix this by moving the delete control out of the role="button" container so the delete button becomes a sibling element (preserving class="delete-btn", disabled={deletingId === item.id}, aria-label and title) and keep its onclick handler that calls e.stopPropagation() and handleDeleteLayout(item); ensure the clickable row still uses role="button", tabindex logic and the onkeydown handler that calls handleOpenLayout(item) remain on the row container so opening and deletion are distinct accessible controls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/components/LoadDialog.svelte`:
- Around line 166-209: The layout row in LoadDialog.svelte currently nests the
delete <button> inside the div that has role="button" and onclick calling
handleOpenLayout(item), causing an interactive-in-interactive accessibility
conflict; fix this by moving the delete control out of the role="button"
container so the delete button becomes a sibling element (preserving
class="delete-btn", disabled={deletingId === item.id}, aria-label and title) and
keep its onclick handler that calls e.stopPropagation() and
handleDeleteLayout(item); ensure the clickable row still uses role="button",
tabindex logic and the onkeydown handler that calls handleOpenLayout(item)
remain on the row container so opening and deletion are distinct accessible
controls.
In `@src/lib/utils/archive.ts`:
- Around line 341-353: The loop that sums totalUncompressedSize is decompressing
every entry even after exceeding LIMITS.MAX_TOTAL_UNCOMPRESSED_BYTES; change the
loop that iterates over entries (using zip.files[name] and file.async(...)) to
check after adding each file's byteLength whether totalUncompressedSize >
LIMITS.MAX_TOTAL_UNCOMPRESSED_BYTES and if so immediately stop further
processing (break out and throw the same error) to short-circuit work and
prevent decompressing remaining entries.
In `@src/tests/load-pipeline.test.ts`:
- Line 90: Replace the non-idiomatic TypeScript casts using "as vi.Mock" in this
test file with proper Vitest typings: use the Mock generic type or vi.mocked
helpers (for example replace "(persistenceApi.loadSavedLayout as
vi.Mock).mockResolvedValue(layout)" by typing the mocked function via Mock or
using vi.mocked(persistenceApi.loadSavedLayout).mockResolvedValue) and apply the
same change to all occurrences (e.g., persistenceApi.loadSavedLayout,
persistenceApi.deleteLayout, persistenceApi.saveLayout, and other mocks
referenced around lines 100, 118–119, 134). Ensure imports/types for Mock or
vi.mocked are available and update each mock invocation to use the typed mock
methods instead of the "as vi.Mock" cast.
In `@src/tests/LoadDialog.test.ts`:
- Around line 38-50: Replace all brittle "as vi.Mock" casts in this test file
with Vitest's typed mock helpers: use vi.mocked(...) or the Mock<T> type. For
each mocked function such as persistenceStore.isApiAvailable and
persistenceApi.listSavedLayouts (and other occurrences around lines mentioned),
swap (fn as vi.Mock) to either vi.mocked(fn) when asserting call behavior or
declare the mock with Mock<typeof moduleFn> / Mock<FnSignature> and use that
typed mock for mockReturnValue / mockResolvedValue. Make sure to update every
occurrence (including the other blocks noted) so all mockReturnValue /
mockResolvedValue calls use the typed mock helper instead of "as vi.Mock".
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/lib/components/LoadDialog.sveltesrc/lib/utils/archive.tssrc/lib/utils/load-pipeline.tssrc/lib/utils/persistence-api.tssrc/tests/LoadDialog.test.tssrc/tests/archive-guardrails.test.tssrc/tests/load-pipeline.test.ts
- Restructure LoadDialog layout rows to fix interactive-in-interactive a11y violation (button nested inside role="button") - Early break in archive decompression loop when size limit exceeded - Replace non-idiomatic `as vi.Mock` casts with `vi.mocked()` in load-pipeline and LoadDialog test files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
Contributor
Sequence DiagramShows the unified success-path when a user chooses to load a layout from the server or import a local .zip. The LoadDialog delegates to the centralized load-pipeline which obtains layout+images (from API or archive), updates stores (images + layout) and shows the final toast. sequenceDiagram
participant User
participant LoadDialog
participant LoadPipeline
participant BackendOrArchive as "Persist API / Archive"
participant Stores as "Layout & Image Stores"
participant Toast
User->>LoadDialog: Open dialog & choose "Saved" or "Import"
alt Saved on server
LoadDialog->>LoadPipeline: loadFromApi(uuid)
else Local file
LoadDialog->>LoadPipeline: loadFromFile(file?)
end
LoadPipeline->>BackendOrArchive: Fetch layout (+ images) (API or extract ZIP)
BackendOrArchive-->>LoadPipeline: layout + images (or error)
LoadPipeline->>Stores: Clear/reload bundled images → overlay custom images → load layout
LoadPipeline->>Toast: Show success or warning (failed images)
Generated by CodeAnt AI |
Contributor
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/LoadDialog.svelte`:
- Around line 109-118: The formatDate function should be made locale-aware for
time format preference; change formatDate to use toLocaleString (or
Intl.DateTimeFormat) and explicitly pass the user's hour12 preference obtained
from Intl.DateTimeFormat().resolvedOptions().hour12 so the formatted string
respects 12/24-hour settings across browsers—update the function formatDate to
compute const hour12 = new Intl.DateTimeFormat().resolvedOptions().hour12 and
include hour12 in the options passed to toLocaleString/Intl.DateTimeFormat while
keeping the existing year/month/day/hour/minute fields.
In `@src/tests/load-pipeline.test.ts`:
- Around line 23-31: The mock PersistenceError in the vi.mock block lacks the
optional statusCode parameter present in the real class; update the mock
PersistenceError constructor to accept an optional statusCode?: number and
assign it to this.statusCode (while still calling super(message) and setting
this.name = "PersistenceError") so the mock API matches the real
PersistenceError used elsewhere (also verify the loadSavedLayout mock remains
unchanged).
In `@src/tests/LoadDialog.test.ts`:
- Around line 126-159: The test currently calls render(LoadDialog) twice which
can leave multiple component instances in the DOM; change the second render to
use the original render result (e.g., const { rerender, unmount } =
render(LoadDialog)) and either call rerender(LoadDialog) after toggling
dialogStore or call unmount() before calling render again, while keeping the
same mocked persistenceStore.isApiAvailable toggle and awaiting tick() as
needed; update references in this test to use rerender/unmount instead of a
second render and keep assertions against persistenceApi.listSavedLayouts and
screen.findByText("Delayed Layout").
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/LoadDialog.sveltesrc/lib/utils/archive.tssrc/tests/LoadDialog.test.tssrc/tests/load-pipeline.test.ts
- Make formatDate locale-aware by passing hour12 from Intl.DateTimeFormat - Add missing statusCode parameter to mock PersistenceError in both test files - Fix double render in LoadDialog delayed API test by unmounting first instance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/LoadDialog.svelte`:
- Around line 408-410: The CSS rule :global(.spinner) is unused in
LoadDialog.svelte because the component uses the .spinner-loader class; remove
the dead :global(.spinner) block or replace it with :global(.spinner-loader) so
the animation applies to the actual element, updating the style rule to match
the .spinner-loader class used in the component.
In `@src/tests/load-pipeline.test.ts`:
- Around line 115-143: Add a failing-extraction test for loadFromFile: mock
fileUtils.openFilePicker to resolve a File, mock archive.extractFolderArchive to
reject with an Error (e.g., "Invalid archive format"), call loadFromFile, assert
it returns false, and assert toastStore.toasts includes an error toast (use
expect.objectContaining({ type: "error" })); reference loadFromFile,
fileUtils.openFilePicker, archive.extractFolderArchive, and toastStore.toasts
when locating where to add the new test.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/LoadDialog.sveltesrc/tests/LoadDialog.test.tssrc/tests/load-pipeline.test.ts
- Remove dead :global(.spinner) CSS rule (component uses .spinner-loader) - Add missing test for loadFromFile extraction failure error path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/LoadDialog.svelte`:
- Around line 16-56: The debug call in loadLayouts uses persistenceDebug.api but
the project standard requires the layout-specific debugger; update the import at
the top of LoadDialog.svelte to import { layoutDebug } from "$lib/utils/debug"
instead of persistenceDebug, and replace the usage in loadLayouts (and any other
occurrences in this file) from persistenceDebug.api(...) to layoutDebug.api(...)
so the logging follows the established namespace pattern.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
This PR unifies the layout loading flow between server-persisted (Persist API) and local file (.zip) imports.
Key Changes:
load-pipeline.tsfor consistent image restoration and state management.App.svelteandStartScreen.svelte.Closes #1348
CodeAnt-AI Description
Unify layout loading with a Load dialog and secure ZIP import
What Changed
Impact
✅ Clearer archive rejection errors✅ Fewer silent API failures when listing saved layouts✅ Consistent layout restore behavior and messaging across import sources💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.