feat: add YAML editor panel with conflict detection#1324
Merged
Conversation
Extract the YAML editor feature from dev/yamllama-ding-dong into a clean implementation. Adds LayoutYamlPanel component with read-only/edit modes, debounced Zod schema validation, and conflict detection when the layout changes externally during editing. - Wire into dialog store, File menu, Toolbar, mobile bottom sheet - Remove deprecated slot_width/slot_position from YAML serialisation - Update yaml-roundtrip test for removed fields Closes #1323 Implements #1176 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add analytics tracking to handleViewYaml toolbar handler - Fix redundant min(100%, 100%) CSS rule in LayoutYamlPanel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 YAML editor feature: new LayoutYamlPanel component, UI wiring to open it from File menu/toolbar/mobile sheet (dialog + bottom sheet), App.svelte handlers to open/close/apply YAML edits, dialog/sheet type updates, tests for the editor, and removal of deprecated slot_width/slot_position from YAML utilities. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant FileMenu as FileMenu
participant App as App.svelte
participant Dialog as Dialog/Sheet
participant Editor as LayoutYamlPanel
participant Store as LayoutStore
User->>FileMenu: Click "View YAML"
FileMenu->>App: onviewyaml()
App->>Dialog: open yamlEditor (dialog or sheet) with current layout
Dialog->>Editor: render baseline YAML
User->>Editor: Edit YAML
Editor->>Editor: Debounced parse & Zod validation
alt Valid YAML
Editor->>User: Enable Apply
else Invalid YAML
Editor->>User: Show validation errors
end
alt Concurrent change detected
Editor->>User: Prompt "Reload latest" or "Apply anyway"
User->>Editor: Choose action
end
User->>Editor: Click Apply
Editor->>App: onapply(updatedLayout)
App->>Store: loadLayout(updatedLayout)
App->>App: requestAnimationFrame -> handleFitAll()
App->>Dialog: close yamlEditor
App->>User: Show success toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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:XL
Contributor
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/LayoutYamlPanel.svelte`:
- Around line 62-65: The effect currently bails out or drops sync results if
`isEditing` flips true during the async `syncFromLayout` call, causing a
hydration race that leaves `yamlText` empty when the user quickly enters edit
mode; modify the `$effect` that calls `syncFromLayout(layout, {
allowWhileEditing: false })` so it snapshots `isEditing` (e.g. `const
editingAtStart = isEditing`) before awaiting the async call and only ignores the
result if `editingAtStart` was true (not if `isEditing` changed while the
promise was in flight); keep the `isApplying` and `open` guards, and apply the
same snapshotting fix where `syncFromLayout` is used elsewhere (the other blocks
around the same area).
- Around line 189-200: The commitParsedLayout function currently clears conflict
state and pendingLayout in the finally block even when onapply or syncFromLayout
throws; change it to catch errors from await onapply?.(nextLayout) and await
syncFromLayout(nextLayout), set isApplying = false in a finally, but only clear
showConflictPrompt, latestYamlAtConflict, pendingLayout and set isEditing =
false when the apply/sync succeeds; on failure keep those conflict/pending flags
intact, surface the error to the user (e.g., set/emit an error or call an
existing notification handler) and rethrow or return so callers can react.
Ensure you update references to commitParsedLayout, onapply, syncFromLayout,
isApplying, isEditing, showConflictPrompt, latestYamlAtConflict, and
pendingLayout when implementing this flow.
- Around line 203-223: handleApply can continue after async awaits even if the
user cancelled/closed the panel; add an intent guard to abort stale flows:
introduce a component-level numeric token (e.g. applyIntentId) that you
increment at the start of handleApply, capture it in a local const (e.g. const
intent = applyIntentId) and after each await (after parseLayoutYaml and
serializeLayoutToYaml) check if intent !== applyIntentId and return early if so;
apply the same guard before setting
pendingLayout/latestYamlAtConflict/showConflictPrompt and before calling
commitParsedLayout(parsedLayout).
In `@src/tests/LayoutYamlPanel.test.ts`:
- Around line 92-144: The test replaces yamlUtils.serializeLayoutToYaml with
vi.spyOn but never restores it, risking test pollution; fix by restoring the spy
after the test (either call the spy's mockRestore() or invoke
vi.restoreAllMocks()) — e.g., capture the spy returned from vi.spyOn(yamlUtils,
"serializeLayoutToYaml") and call spy.mockRestore() at the end of the it block
or add an afterEach(() => vi.restoreAllMocks()) in the describe that contains
this test so serializeLayoutToYaml is returned to its original implementation
for subsequent tests.
In `@src/tests/yaml-roundtrip.test.ts`:
- Around line 41-44: The test currently uses exact-length assertions
(expect(restored.racks).toHaveLength(1) and
expect(restored.device_types).toHaveLength(1)) which violates the ESLint rule;
update the assertions in yaml-roundtrip.test.ts around parseLayoutYaml to assert
existence/non-emptiness instead (e.g., assert Array.isArray(restored.racks) and
restored.racks.length > 0 and similarly for restored.device_types, or use
expect(restored.racks).toBeDefined() plus
expect(restored.racks.length).toBeGreaterThan(0)) so the round-trip validates
presence without using exact length checks.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/App.sveltesrc/lib/components/FileMenu.sveltesrc/lib/components/LayoutYamlPanel.sveltesrc/lib/components/MobileFileSheet.sveltesrc/lib/components/Toolbar.sveltesrc/lib/stores/dialogs.svelte.tssrc/lib/utils/yaml.tssrc/tests/LayoutYamlPanel.test.tssrc/tests/yaml-roundtrip.test.ts
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
928
to
936
| function handleYamlApply(nextLayout: Layout) { | ||
| layoutStore.loadLayout(nextLayout); | ||
| layoutStore.markDirty(); | ||
| selectionStore.clearSelection(); | ||
| toastStore.showToast("YAML applied", "success"); | ||
|
|
||
| requestAnimationFrame(() => { | ||
| handleFitAll(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: After successfully applying valid YAML, the layout is updated and the canvas re-fitted but the YAML dialog/sheet remains open, which contradicts the stated behavior in the PR test plan ("Apply valid YAML — layout updates, editor closes") and forces users to dismiss the editor manually instead of closing it automatically in both desktop and mobile flows. [logic error]
Severity Level: Major ⚠️
- Critical: LayoutYamlPanel dialog or sheet remains open post-apply.
- Warning: YAML editor UX diverges from documented test plan.
Suggested change
| function handleYamlApply(nextLayout: Layout) { | |
| layoutStore.loadLayout(nextLayout); | |
| layoutStore.markDirty(); | |
| selectionStore.clearSelection(); | |
| toastStore.showToast("YAML applied", "success"); | |
| requestAnimationFrame(() => { | |
| handleFitAll(); | |
| }); | |
| function handleYamlApply(nextLayout: Layout) { | |
| layoutStore.loadLayout(nextLayout); | |
| layoutStore.markDirty(); | |
| selectionStore.clearSelection(); | |
| toastStore.showToast("YAML applied", "success"); | |
| if (viewportStore.isMobile) { | |
| dialogStore.closeSheet(); | |
| } else { | |
| dialogStore.close(); | |
| } | |
| requestAnimationFrame(() => { | |
| handleFitAll(); | |
| }); | |
| } |
Steps of Reproduction ✅
1. Open the app and ensure a layout is present (create or load one) so the toolbar is
active; this uses `App.svelte` lines 1567-1583 where `<Toolbar ...
onviewyaml={handleOpenYamlEditor} />` is wired.
2. On desktop, click the File/toolbar action that triggers "View YAML"; this calls
`handleOpenYamlEditor` in `src/App.svelte:911-918`, which opens the YAML editor dialog by
calling `dialogStore.open("yamlEditor")`, causing the `<Dialog>` with `<LayoutYamlPanel
... onapply={handleYamlApply} />` at `App.svelte:1781-1792` to render. On mobile, the same
action from `MobileFileSheet` at `src/App.svelte:1830-1837` opens the YAML editor bottom
sheet at `App.svelte:1842-1852`.
3. In the YAML editor UI (`LayoutYamlPanel.svelte`), click "Edit YAML", enter valid YAML,
and click "Apply YAML"; this triggers `handleApply` at
`src/lib/components/LayoutYamlPanel.svelte:203-223`, which parses the YAML and then calls
`await commitParsedLayout(parsedLayout)`, and `commitParsedLayout` at lines 189-200
invokes the `onapply` callback provided by the parent.
4. The `onapply` callback is `handleYamlApply` in `src/App.svelte:928-937`, which loads
the new layout, marks it dirty, clears selection, shows the "YAML applied" toast, and
refits the canvas via `requestAnimationFrame`, but never calls `dialogStore.close()` or
`dialogStore.closeSheet()`, so `yamlEditorDialogOpen` / `yamlEditorSheetOpen` (derived at
`App.svelte:145-162`) remain true and the YAML editor dialog or bottom sheet stays open
after applying, contrary to the PR test plan step "Apply valid YAML — layout updates,
editor closes".Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/App.svelte
**Line:** 928:936
**Comment:**
*Logic Error: After successfully applying valid YAML, the layout is updated and the canvas re-fitted but the YAML dialog/sheet remains open, which contradicts the stated behavior in the PR test plan ("Apply valid YAML — layout updates, editor closes") and forces users to dismiss the editor manually instead of closing it automatically in both desktop and mobile flows.
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.
Comment on lines
+62
to
+65
| $effect(() => { | ||
| if (!open || isEditing || isApplying) return; | ||
| void syncFromLayout(layout, { allowWhileEditing: false }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: Closing the YAML editor while in edit mode does not reset its internal editing state, so reopening it shows stale in-progress edits instead of a fresh snapshot of the current layout; add an effect that resets editing-related state when open becomes false so each new open starts from the latest layout. [logic error]
Severity Level: Major ⚠️
- ⚠️ YAML editor reopens showing stale unsaved YAML content.
- ⚠️ Applying YAML can overwrite newer layout modifications.
- ⚠️ Conflict detection baseline may not match current layout.
Suggested change
| $effect(() => { | |
| if (!open || isEditing || isApplying) return; | |
| void syncFromLayout(layout, { allowWhileEditing: false }); | |
| }); | |
| $effect(() => { | |
| if (!open || isEditing || isApplying) return; | |
| void syncFromLayout(layout, { allowWhileEditing: false }); | |
| }); | |
| $effect(() => { | |
| if (!open) { | |
| // Reset editing session when the panel is closed so reopening starts fresh | |
| isEditing = false; | |
| showConflictPrompt = false; | |
| pendingLayout = null; | |
| latestYamlAtConflict = null; | |
| syntaxError = null; | |
| schemaError = null; | |
| isValidating = false; | |
| } | |
| }); |
Steps of Reproduction ✅
1. Start the app with the PR code and open the YAML editor via the File > View YAML menu
or toolbar "View YAML" action, which calls `handleOpenYamlEditor()` in
`src/App.svelte:911-918` and opens the `<Dialog>` containing `<LayoutYamlPanel
open={yamlEditorDialogOpen} ...>` at `src/App.svelte:1781-1791` (desktop) or the
`<BottomSheet>` with `<LayoutYamlPanel open={yamlEditorSheetOpen} ...>` at
`src/App.svelte:1842-1852` (mobile), setting the `open` prop to `true`.
2. In `LayoutYamlPanel.svelte`, the first `$effect` at `lines 62-65` runs because `open
=== true`, `isEditing === false`, and `isApplying === false`, invoking
`syncFromLayout(layout, { allowWhileEditing: false })` which sets `baselineYaml` and
`yamlText` to the current layout and clears validation state.
3. Click the "Edit YAML" button in the panel header (the button wired to
`onclick={handleEditModeToggle}` at `LayoutYamlPanel.svelte:264-270`), which sets
`isEditing = true` in `handleEditModeToggle()` at `LayoutYamlPanel.svelte:160-168`, then
type arbitrary changes into the `<textarea data-testid="yaml-textarea">` at
`LayoutYamlPanel.svelte:310-317` without clicking "Cancel edits" or "Apply YAML", leaving
unsaved in-progress edits in `yamlText` and `isEditing === true`.
4. Close the YAML editor without using "Cancel edits": on desktop, close the dialog via
its close control or ESC so `handleYamlEditorClose()` in `src/App.svelte:920-922` calls
`dialogStore.close()`, making `yamlEditorDialogOpen` false; on mobile, close the sheet so
`handleYamlEditorSheetClose()` at `src/App.svelte:924-926` calls
`dialogStore.closeSheet()`, making `yamlEditorSheetOpen` false. In both cases,
`LayoutYamlPanel`'s `open` prop becomes `false`, but there is no `$effect` that resets
`isEditing`, `yamlText`, or other editing state on close, so those variables retain their
previous values.
5. Reopen the YAML editor again via the same menu/button so `open` becomes `true` once
more for `LayoutYamlPanel`. The first `$effect` at `LayoutYamlPanel.svelte:62-65` now sees
`open === true` but `isEditing === true` (carried over from the prior session) and
immediately returns without calling `syncFromLayout`, so `baselineYaml`, `yamlText`, and
validation flags are not refreshed from the latest `layout`. The textarea shows the stale
unsaved edits from the prior session instead of a fresh snapshot of `layout`, and if
`layoutStore.layout` has changed in the meantime (e.g., racks/devices modified through the
main UI in `App.svelte`), pressing "Apply YAML" will parse and apply this stale YAML in
`handleApply()` at `LayoutYamlPanel.svelte:205-223`, potentially overwriting newer layout
changes or producing misleading conflict prompts.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/components/LayoutYamlPanel.svelte
**Line:** 62:65
**Comment:**
*Logic Error: Closing the YAML editor while in edit mode does not reset its internal editing state, so reopening it shows stale in-progress edits instead of a fresh snapshot of the current layout; add an effect that resets editing-related state when `open` becomes false so each new open starts from the latest layout.
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.
| }: Props = $props(); | ||
|
|
||
| function handleAction(action?: () => void) { | ||
| action?.(); |
Contributor
There was a problem hiding this comment.
Suggestion: On mobile, tapping the "View YAML" action briefly opens the YAML sheet and then immediately closes it because handleAction opens the new sheet and then calls onclose, which closes whatever sheet is currently open (now the YAML editor), so the user never sees the YAML editor. [logic error]
Severity Level: Critical 🚨
- ❌ Mobile users cannot open Layout YAML editor sheet.
- ⚠️ New YAML editor feature unusable from mobile File menu.
Contributor
|
CodeAnt AI finished reviewing your PR. |
Snapshot isEditing before await in syncFromLayout to prevent hydration race, preserve conflict state on apply failure, add intent guard to handleApply against stale data, restore spied mocks in tests, and replace exact-length assertions in roundtrip test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add isApplying check and applyIntentId bump to prevent rapid double-clicks on "Apply anyway" from triggering concurrent commits. 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/LayoutYamlPanel.svelte`:
- Around line 239-248: The reload flow in handleReloadLatest may complete with
stale state if user actions occur while await serializeLayoutToYaml(layout) is
in flight; add an intent guard by capturing a local intent/token or snapshot of
relevant state (e.g., latestYamlAtConflict and/or pendingLayout) before the
await and compare it immediately after the await to ensure the user still
intends this reload; if the token/state changed, abort without mutating
baselineYaml, yamlText, showConflictPrompt, latestYamlAtConflict, pendingLayout,
or isEditing.
- Around line 1-23: Add debug logging using the debug package with namespace
"rackula:layout:state": import and instantiate the debugger at top of
LayoutYamlPanel.svelte, then insert debug calls at key operations — inside
syncFromLayout (e.g., "syncing from layout, runId=%d" with runId), inside
validateYamlText (e.g., "validation complete, hasErrors=%o" with
syntaxError/schemaError state), and inside handleApply (e.g., "apply started,
intent=%d" and "conflict detected, baselineYaml differs from latestYaml") so the
async flows (sync, validation, conflict detection) emit namespace-filtered
diagnostics; use the existing function/variable names syncFromLayout,
validateYamlText, handleApply, runId, syntaxError, schemaError, baselineYaml and
latestYaml to place the messages.
Comment on lines
1
to
23
| <script lang="ts"> | ||
| import type { ZodIssue } from "zod"; | ||
| import type { Layout } from "$lib/types"; | ||
| import { LayoutSchema } from "$lib/schemas"; | ||
| import { buildYamlFilename } from "$lib/utils/folder-structure"; | ||
| import { | ||
| parseLayoutYaml, | ||
| parseYaml, | ||
| serializeLayoutToYaml, | ||
| } from "$lib/utils/yaml"; | ||
| import { getToastStore } from "$lib/stores/toast.svelte"; | ||
| import { IconCopy, IconDownload } from "./icons"; | ||
| import { ICON_SIZE } from "$lib/constants/sizing"; | ||
|
|
||
| interface Props { | ||
| open: boolean; | ||
| layout: Layout; | ||
| onapply?: (layout: Layout) => void | Promise<void>; | ||
| } | ||
|
|
||
| let { open, layout, onapply }: Props = $props(); | ||
|
|
||
| const toastStore = getToastStore(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add debug logging for YAML editor operations.
Per coding guidelines, use the debug npm package with namespace filtering for debug logging. This component handles complex async flows (sync, validation, conflict detection) where debug logging would aid troubleshooting.
Proposed addition
<script lang="ts">
import type { ZodIssue } from "zod";
import type { Layout } from "$lib/types";
import { LayoutSchema } from "$lib/schemas";
import { buildYamlFilename } from "$lib/utils/folder-structure";
import {
parseLayoutYaml,
parseYaml,
serializeLayoutToYaml,
} from "$lib/utils/yaml";
import { getToastStore } from "$lib/stores/toast.svelte";
import { IconCopy, IconDownload } from "./icons";
import { ICON_SIZE } from "$lib/constants/sizing";
+ import { layoutDebug } from "$lib/utils/debug";
+
+ const debug = layoutDebug.extend("yaml-panel");Then use in key operations:
// In syncFromLayout
debug("syncing from layout, runId=%d", runId);
// In validateYamlText
debug("validation complete, hasErrors=%o", { syntaxError, schemaError });
// In handleApply
debug("apply started, intent=%d", intent);
debug("conflict detected, baselineYaml differs from latestYaml");As per coding guidelines: "Use the debug npm package with namespace filtering for debug logging... Use established namespaces: rackula:layout:state..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/LayoutYamlPanel.svelte` around lines 1 - 23, Add debug
logging using the debug package with namespace "rackula:layout:state": import
and instantiate the debugger at top of LayoutYamlPanel.svelte, then insert debug
calls at key operations — inside syncFromLayout (e.g., "syncing from layout,
runId=%d" with runId), inside validateYamlText (e.g., "validation complete,
hasErrors=%o" with syntaxError/schemaError state), and inside handleApply (e.g.,
"apply started, intent=%d" and "conflict detected, baselineYaml differs from
latestYaml") so the async flows (sync, validation, conflict detection) emit
namespace-filtered diagnostics; use the existing function/variable names
syncFromLayout, validateYamlText, handleApply, runId, syntaxError, schemaError,
baselineYaml and latestYaml to place the messages.
Comment on lines
239
to
248
| async function handleReloadLatest(): Promise<void> { | ||
| const latest = | ||
| latestYamlAtConflict ?? (await serializeLayoutToYaml(layout)); | ||
| baselineYaml = latest; | ||
| yamlText = latest; | ||
| showConflictPrompt = false; | ||
| latestYamlAtConflict = null; | ||
| pendingLayout = null; | ||
| isEditing = true; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding an intent guard after the async await.
If the user rapidly triggers other actions while serializeLayoutToYaml is in flight at line 241, the reload could complete with stale assumptions. This is low-risk since it's an explicit user action, but for consistency with handleApply, an intent guard would be more robust.
Optional enhancement
async function handleReloadLatest(): Promise<void> {
+ const intent = ++applyIntentId;
const latest =
latestYamlAtConflict ?? (await serializeLayoutToYaml(layout));
+ if (intent !== applyIntentId) return;
baselineYaml = latest;
yamlText = latest;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/LayoutYamlPanel.svelte` around lines 239 - 248, The reload
flow in handleReloadLatest may complete with stale state if user actions occur
while await serializeLayoutToYaml(layout) is in flight; add an intent guard by
capturing a local intent/token or snapshot of relevant state (e.g.,
latestYamlAtConflict and/or pendingLayout) before the await and compare it
immediately after the await to ensure the user still intends this reload; if the
token/state changed, abort without mutating baselineYaml, yamlText,
showConflictPrompt, latestYamlAtConflict, pendingLayout, or isEditing.
- Add handleFitAll to YAML editor close handlers for consistency with other dialog close patterns (share, help, view sheet) - Add hasRacks prop to MobileFileSheet and disable Share/View YAML buttons when no racks exist, matching FileMenu behaviour - Add clarifying comment on conflict test mock strategy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tests/MobileFileSheet.test.ts (1)
87-96: 🧹 Nitpick | 🔵 TrivialAdd tests for the new
View YAMLaction path.This update covers Share with
hasRacks, but it doesn’t add assertions for the newly introducedView YAMLbehavior (enabled/disabled state and callback+close flow). Please add that user-facing coverage.As per coding guidelines, “Test only high-value behavior … user-facing behavior … error paths and edge cases.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/MobileFileSheet.test.ts` around lines 87 - 96, Add user-facing tests for the new "View YAML" action: after calling renderSheet({ hasRacks: true }), assert the "View YAML" button is enabled (getByRole("button", { name: "View YAML" })), simulate a click with fireEvent.click and assert the onViewYAML (or onViewYaml) callback is called once and onClose is called once while onLoad/onSave/onExport/onShare remain unchanged; also add a separate test using renderSheet({ hasRacks: false }) to assert the "View YAML" button is disabled and clicking it does not call onViewYAML or onClose. Ensure you reference the existing helpers and mocks (renderSheet, onViewYAML/onViewYaml, onClose, onLoad, onSave, onExport, onShare) when adding assertions.
🤖 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/MobileFileSheet.svelte`:
- Around line 97-103: The View YAML button currently calls
handleAction(onviewyaml) which triggers onviewyaml then the generic onclose
path, causing the YAML sheet to open then immediately close; fix by ensuring the
View YAML action does not invoke the generic close: either (preferred) change
the button to call onviewyaml directly from MobileFileSheet.svelte (bypassing
handleAction) or update handleAction to accept a skipClose flag (e.g.,
handleAction(action, { skipClose: true })) and, when that flag is set or when
action === onviewyaml, avoid calling onclose; update references to handleAction,
onviewyaml and onclose accordingly.
In `@src/tests/LayoutYamlPanel.test.ts`:
- Around line 33-37: Replace direct DOM access of the textarea value in the
tests with testing-library matchers: locate the element via
screen.getByTestId("yaml-textarea") and use the jest-dom matcher to assert its
value (e.g., toHaveValue with expect.stringContaining("name: Baseline Layout"))
inside the existing waitFor. Apply the same change to the other two occurrences
(the assertions referenced around lines 75-79 and 126-130) so tests use
toHaveValue/assertions from jest-dom instead of casting and reading .value
directly.
---
Outside diff comments:
In `@src/tests/MobileFileSheet.test.ts`:
- Around line 87-96: Add user-facing tests for the new "View YAML" action: after
calling renderSheet({ hasRacks: true }), assert the "View YAML" button is
enabled (getByRole("button", { name: "View YAML" })), simulate a click with
fireEvent.click and assert the onViewYAML (or onViewYaml) callback is called
once and onClose is called once while onLoad/onSave/onExport/onShare remain
unchanged; also add a separate test using renderSheet({ hasRacks: false }) to
assert the "View YAML" button is disabled and clicking it does not call
onViewYAML or onClose. Ensure you reference the existing helpers and mocks
(renderSheet, onViewYAML/onViewYaml, onClose, onLoad, onSave, onExport, onShare)
when adding assertions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/App.sveltesrc/lib/components/MobileFileSheet.sveltesrc/tests/LayoutYamlPanel.test.tssrc/tests/MobileFileSheet.test.ts
Comment on lines
97
to
103
| <button | ||
| type="button" | ||
| class="file-action" | ||
| disabled={!hasRacks} | ||
| onclick={() => handleAction(onviewyaml)} | ||
| aria-label="View YAML" | ||
| > |
There was a problem hiding this comment.
View YAML can close the YAML sheet immediately on mobile.
handleAction runs onviewyaml before onclose; in the mobile path that can open the YAML sheet and then immediately close it via the generic close callback.
🐛 Localized fix for `View YAML` handoff
<button
type="button"
class="file-action"
disabled={!hasRacks}
- onclick={() => handleAction(onviewyaml)}
+ onclick={() => {
+ onclose?.();
+ onviewyaml?.();
+ }}
aria-label="View YAML"
>📝 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
| <button | |
| type="button" | |
| class="file-action" | |
| disabled={!hasRacks} | |
| onclick={() => handleAction(onviewyaml)} | |
| aria-label="View YAML" | |
| > | |
| <button | |
| type="button" | |
| class="file-action" | |
| disabled={!hasRacks} | |
| onclick={() => { | |
| onclose?.(); | |
| onviewyaml?.(); | |
| }} | |
| aria-label="View YAML" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/MobileFileSheet.svelte` around lines 97 - 103, The View
YAML button currently calls handleAction(onviewyaml) which triggers onviewyaml
then the generic onclose path, causing the YAML sheet to open then immediately
close; fix by ensuring the View YAML action does not invoke the generic close:
either (preferred) change the button to call onviewyaml directly from
MobileFileSheet.svelte (bypassing handleAction) or update handleAction to accept
a skipClose flag (e.g., handleAction(action, { skipClose: true })) and, when
that flag is set or when action === onviewyaml, avoid calling onclose; update
references to handleAction, onviewyaml and onclose accordingly.
Comment on lines
33
to
37
| await waitFor(() => { | ||
| expect( | ||
| (screen.getByTestId("yaml-textarea") as HTMLTextAreaElement).value, | ||
| ).toContain("name: Baseline Layout"); | ||
| }); |
There was a problem hiding this comment.
Avoid direct DOM node .value access in tests.
These assertions should use testing-library matchers instead of casting and reading textarea .value directly.
♻️ Suggested pattern (apply to all three occurrences)
await waitFor(() => {
- expect(
- (screen.getByTestId("yaml-textarea") as HTMLTextAreaElement).value,
- ).toContain("name: Baseline Layout");
+ expect(screen.getByTestId("yaml-textarea")).toHaveValue(
+ expect.stringContaining("name: Baseline Layout"),
+ );
});As per coding guidelines, “ESLint hard-blocks … DOM node access in tests …”.
Also applies to: 75-79, 126-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/LayoutYamlPanel.test.ts` around lines 33 - 37, Replace direct DOM
access of the textarea value in the tests with testing-library matchers: locate
the element via screen.getByTestId("yaml-textarea") and use the jest-dom matcher
to assert its value (e.g., toHaveValue with expect.stringContaining("name:
Baseline Layout")) inside the existing waitFor. Apply the same change to the
other two occurrences (the assertions referenced around lines 75-79 and 126-130)
so tests use toHaveValue/assertions from jest-dom instead of casting and reading
.value directly.
…heet Cover the new hasRacks-gated behaviour: View YAML calls handler when enabled, Share and View YAML buttons are disabled when no racks exist. 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/tests/MobileFileSheet.test.ts`:
- Around line 20-29: The test helper renderSheet currently types its overrides
as Record<string, unknown>, which is too permissive; change the overrides
parameter to use Partial<ComponentProps<typeof MobileFileSheet>> so TypeScript
will validate prop names and types against the component; import ComponentProps
from 'react' (or React) if not already imported and update the renderSheet
signature and any related usages to use the new typed overrides while preserving
the spreading of onload/onsave/... into props.
src/tests/MobileFileSheet.test.ts
Outdated
Comment on lines
20
to
29
| function renderSheet(overrides: Record<string, unknown> = {}) { | ||
| return render(MobileFileSheet, { | ||
| props: { | ||
| onload: onLoad, | ||
| onsave: onSave, | ||
| onexport: onExport, | ||
| onshare: onShare, | ||
| onclose: onClose, | ||
| ...overrides, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MobileFileSheet.test.ts" -o -name "MobileFileSheet.svelte" | head -20Repository: RackulaLives/Rackula
Length of output: 143
🏁 Script executed:
fd -t f "MobileFileSheet"Repository: RackulaLives/Rackula
Length of output: 139
🏁 Script executed:
cat -n ./src/tests/MobileFileSheet.test.ts | head -120Repository: RackulaLives/Rackula
Length of output: 4408
🏁 Script executed:
cat -n ./src/lib/components/MobileFileSheet.svelteRepository: RackulaLives/Rackula
Length of output: 5194
🏁 Script executed:
rg "ComponentProps" --type ts --type tsx | head -20Repository: RackulaLives/Rackula
Length of output: 91
🏁 Script executed:
cat -n ./tsconfig.json | head -30Repository: RackulaLives/Rackula
Length of output: 981
🏁 Script executed:
rg "ComponentProps" --type ts | head -20Repository: RackulaLives/Rackula
Length of output: 46
🏁 Script executed:
rg "from.*svelte" --type ts | grep -i import | head -15Repository: RackulaLives/Rackula
Length of output: 1324
🏁 Script executed:
cat package.json | grep -A 5 '"svelte"'Repository: RackulaLives/Rackula
Length of output: 226
🏁 Script executed:
rg "ComponentProps" . --type tsRepository: RackulaLives/Rackula
Length of output: 46
🌐 Web query:
Svelte 5 ComponentProps type extract component props from .svelte file
💡 Result:
Use Svelte’s built-in ComponentProps utility type with the component’s type (typeof), e.g.:
import Comp from './Comp.svelte';
import type { ComponentProps } from 'svelte';
type CompProps = ComponentProps<typeof Comp>;If you want to avoid a runtime import, you can do it purely as a type:
import type { ComponentProps } from 'svelte';
type CompProps = ComponentProps<typeof import('./Comp.svelte').default>;This works in Svelte 5 and gives you the props type inferred from the .svelte component. [1][2]
Sources: [1] Stack Overflow answer showing ComponentProps<typeof Comp> usage, [2] Svelte’s published type definitions include ComponentProps.
🏁 Script executed:
rg "type.*ComponentProps" src/tests/ --type tsRepository: RackulaLives/Rackula
Length of output: 46
🏁 Script executed:
cat -n ./src/tests/MobileFileSheet.test.ts | grep -A 5 -B 5 "renderSheet"Repository: RackulaLives/Rackula
Length of output: 3229
Tighten renderSheet override typing to component props
Record<string, unknown> is too loose for strict TypeScript and won't catch invalid prop keys in tests. Use Partial<ComponentProps<typeof MobileFileSheet>> instead.
Proposed diff
+import type { ComponentProps } from "svelte";
import { describe, it, expect, vi, beforeEach } from "vitest";
import { render, screen, fireEvent } from "@testing-library/svelte";
import MobileFileSheet from "$lib/components/MobileFileSheet.svelte";
@@
- function renderSheet(overrides: Record<string, unknown> = {}) {
+ function renderSheet(
+ overrides: Partial<ComponentProps<typeof MobileFileSheet>> = {},
+ ) {
return render(MobileFileSheet, {
props: {
onload: onLoad,
onsave: onSave,
onexport: onExport,
onshare: onShare,
onclose: onClose,
...overrides,
},
});
}The behavioral tests validating user-facing actions, conditional disabled states, and handler callbacks are well-designed and comply with testing guidelines.
📝 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
| function renderSheet(overrides: Record<string, unknown> = {}) { | |
| return render(MobileFileSheet, { | |
| props: { | |
| onload: onLoad, | |
| onsave: onSave, | |
| onexport: onExport, | |
| onshare: onShare, | |
| onclose: onClose, | |
| ...overrides, | |
| }, | |
| function renderSheet( | |
| overrides: Partial<ComponentProps<typeof MobileFileSheet>> = {}, | |
| ) { | |
| return render(MobileFileSheet, { | |
| props: { | |
| onload: onLoad, | |
| onsave: onSave, | |
| onexport: onExport, | |
| onshare: onShare, | |
| onclose: onClose, | |
| ...overrides, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/MobileFileSheet.test.ts` around lines 20 - 29, The test helper
renderSheet currently types its overrides as Record<string, unknown>, which is
too permissive; change the overrides parameter to use
Partial<ComponentProps<typeof MobileFileSheet>> so TypeScript will validate prop
names and types against the component; import ComponentProps from 'react' (or
React) if not already imported and update the renderSheet signature and any
related usages to use the new typed overrides while preserving the spreading of
onload/onsave/... into props.
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:XL
Comment on lines
126
to
145
| async function syncFromLayout( | ||
| sourceLayout: Layout, | ||
| options: { allowWhileEditing: boolean } = { allowWhileEditing: true }, | ||
| ): Promise<void> { | ||
| const runId = ++syncRun; | ||
| debug("syncing from layout, runId=%d", runId); | ||
| const editingAtStart = isEditing; | ||
| const serialized = await serializeLayoutToYaml(sourceLayout); | ||
| if (runId !== syncRun) return; | ||
| if (!options.allowWhileEditing && editingAtStart) return; | ||
|
|
||
| baselineYaml = serialized; | ||
| yamlText = serialized; | ||
| syntaxError = null; | ||
| schemaError = null; | ||
| isValidating = false; | ||
| showConflictPrompt = false; | ||
| latestYamlAtConflict = null; | ||
| pendingLayout = null; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: The use of editingAtStart in syncFromLayout only checks whether editing was active when the sync started, so if the user switches into edit mode while serializeLayoutToYaml is still in flight, the late-arriving sync will still overwrite yamlText and baselineYaml with the serialized layout, clobbering any edits just made; instead, the post-await guard should check the current isEditing state so in-flight syncs are discarded once editing has begun. [race condition]
Severity Level: Major ⚠️
- ❌ Initial YAML edits can be silently discarded on open.
- ⚠️ Users may distrust Layout YAML editor reliability.
- ⚠️ Race affects File menu "View YAML" workflow.
Suggested change
| async function syncFromLayout( | |
| sourceLayout: Layout, | |
| options: { allowWhileEditing: boolean } = { allowWhileEditing: true }, | |
| ): Promise<void> { | |
| const runId = ++syncRun; | |
| debug("syncing from layout, runId=%d", runId); | |
| const editingAtStart = isEditing; | |
| const serialized = await serializeLayoutToYaml(sourceLayout); | |
| if (runId !== syncRun) return; | |
| if (!options.allowWhileEditing && editingAtStart) return; | |
| baselineYaml = serialized; | |
| yamlText = serialized; | |
| syntaxError = null; | |
| schemaError = null; | |
| isValidating = false; | |
| showConflictPrompt = false; | |
| latestYamlAtConflict = null; | |
| pendingLayout = null; | |
| } | |
| async function syncFromLayout( | |
| sourceLayout: Layout, | |
| options: { allowWhileEditing: boolean } = { allowWhileEditing: true }, | |
| ): Promise<void> { | |
| const runId = ++syncRun; | |
| debug("syncing from layout, runId=%d", runId); | |
| const serialized = await serializeLayoutToYaml(sourceLayout); | |
| if (runId !== syncRun) return; | |
| if (!options.allowWhileEditing && isEditing) return; | |
| baselineYaml = serialized; | |
| yamlText = serialized; | |
| syntaxError = null; | |
| schemaError = null; | |
| isValidating = false; | |
| showConflictPrompt = false; | |
| latestYamlAtConflict = null; | |
| pendingLayout = null; | |
| } |
Steps of Reproduction ✅
1. Open the app and trigger the Layout YAML panel by clicking "View YAML" from the File
menu (`src/lib/components/FileMenu.svelte:98` label) which shows `<LayoutYamlPanel>` in
`src/App.svelte:1795`.
2. When the panel opens, the `$effect` at
`src/lib/components/LayoutYamlPanel.svelte:65-68` runs with `open === true`, `isEditing
=== false`, `isApplying === false`, and calls `syncFromLayout(layout, { allowWhileEditing:
false })` to load YAML asynchronously.
3. Before `serializeLayoutToYaml(layout)` in `syncFromLayout` resolves
(`LayoutYamlPanel.svelte:133`), click "Edit YAML" (button at
`LayoutYamlPanel.svelte:305-311`), which sets `isEditing = true` via
`handleEditModeToggle()` (`LayoutYamlPanel.svelte:189-197`), then immediately type some
changes into the textarea bound to `yamlText` (`LayoutYamlPanel.svelte:351-358`).
4. When the in-flight `serializeLayoutToYaml` promise resolves, `syncRun` still matches
`runId` (`LayoutYamlPanel.svelte:130-135`) and `editingAtStart` is still `false`, so the
guard `if (!options.allowWhileEditing && editingAtStart) return;` does not fire; as a
result, `baselineYaml` and `yamlText` are overwritten with `serialized`
(`LayoutYamlPanel.svelte:137-138`), silently discarding the user edits just made while
`isEditing` is now `true`. Changing the guard to check `isEditing` (current state)
prevents this overwrite once editing has begun.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/components/LayoutYamlPanel.svelte
**Line:** 126:145
**Comment:**
*Race Condition: The use of `editingAtStart` in `syncFromLayout` only checks whether editing was active when the sync started, so if the user switches into edit mode while `serializeLayoutToYaml` is still in flight, the late-arriving sync will still overwrite `yamlText` and `baselineYaml` with the serialized layout, clobbering any edits just made; instead, the post-`await` guard should check the current `isEditing` state so in-flight syncs are discarded once editing has begun.
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 Incremental review completed. |
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
Summary
dev/yamllama-ding-donginto a clean branch without unrelated auth removal, infra cleanup, or E2E rewritesLayoutYamlPanelcomponent with read-only/edit modes, debounced Zod schema validation, and conflict detection when layout changes externally during editingslot_widthandslot_positionfrom YAML serialisation (NetBox 3.8+)syncFromLayout, preserves conflict state on apply failure, adds intent guards to prevent stale/duplicate applieshasRacksguard to MobileFileSheet Share and View YAML buttons (parity with FileMenu)handleFitAll()to YAML editor close handlers for consistency with other dialog patternsCloses #1323
Implements #1176
Test plan
npm run lint— no errorsnpm run test:run— all 1880 tests pass (87 files)npm run build— production build succeeds🤖 Generated with Claude Code
CodeAnt-AI Description
Add editable Layout YAML panel with validation, conflict detection, and UI integrations
What Changed
Impact
✅ Clearer YAML validation and errors✅ Fewer accidental overwrites from concurrent edits✅ Consistent mobile and desktop access to YAML💡 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.