fix: E2E test suite recovery — 58 failures → 0#1508
Open
Conversation
Group 1 - Replace Rack Flow: - Rewrite single-rack.spec.ts → multi-rack.spec.ts for multi-rack mode - Fix rack-configuration.spec.ts: 2-step wizard (name→next→height→create) - Fix view-reset.spec.ts: remove replace dialog from new rack flow - Fix keyboard.spec.ts: help dialog uses sr-only title, wizard dialog - Fix device-metadata.spec.ts: remove replace dialog from cross-rack test Group 2 - Mobile (partial): - Fix android-chrome.spec.ts: addInitScript for sessionStorage before nav - Fix ios-safari.spec.ts: same addInitScript timing fix Group 3 - Starter Library: - Remove hardcoded device count assertion (toHaveCount(26) → toBeGreaterThan(0)) - Remove hardcoded colour assertion (Steel Blue test) - Fix search count assertion to use toBeGreaterThan(0) Group 5a - Export: - Fix legend checkbox: bits-ui renders button, not input — use label click Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…loading Recovers E2E test suite from 44 failures to 3 remaining. Key fixes: - Mobile: .tap() → .click(), mobileDragDeviceToRack() helper for bottom sheet - Rack config: height 24→12 for 10" racks, scoped dual-view assertions - Keyboard: updated delete assertions for multi-rack mode - Starter library: strict mode fixes for Cable Manager, Brush Panel - Device images: btn-add-device → btn-create-custom-device - Toast: .first() for strict mode in persistence test - iOS FAB: viewport 1024→1280px to clear breakpoint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Final phase of E2E recovery (58 → 0 failures across two sessions). Bug fixes (app code): - Fix session restore race condition: persistence $effect cleared localStorage before App.onMount could restore session data. Added hasEverHadRack guard to only clear session after a rack has existed in the current page lifecycle. - Increase file picker cancel timeout from 300ms to 1000ms to prevent false cancels in slow/headless environments. E2E test fixes: - Toast strict mode: add .first()/.last() to success toast locators across carlton-migration, persistence, device-name, duplicate-device-ids, and archive-format specs (two success toasts can overlap after file load) - Device metadata cross-rack: switch to Devices tab after clickNewRack, add rackIndex option to dragDeviceToRack helper for multi-rack targeting - Custom device: keep as .skip() — needs dragDeviceByName helper and Svelte number input binding workaround (two separate blockers documented) - Un-fixme archive-format load ZIP and device-name save/load tests 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 · |
Contributor
📝 WalkthroughWalkthroughThis pull request updates E2E tests and helpers for multi-rack support, adds mobile-specific test helpers and gesture changes, tightens locator targeting, migrates many flows from a replace-dialog to a new rack wizard, removes the single-rack test suite, adjusts file-picker debounce and session-storage persistence logic, and increases a test timeout. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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:L
Contributor
Sequence DiagramThis PR fixes a startup race where persistence effects could clear stored session data before the app restored it. The new guard defers clearing until a rack has existed at least once, allowing reliable restore and then normal save clear behavior. sequenceDiagram
participant Browser
participant App
participant Persistence
participant LocalStorage
Browser->>App: Open application
App->>Persistence: Initialize persistence effects
Persistence->>Persistence: Initial state has no rack
Persistence-->>LocalStorage: Skip clear on first empty run
App->>LocalStorage: Restore saved session
LocalStorage-->>App: Return saved layout
App->>Persistence: Layout now has rack
Persistence->>LocalStorage: Save session after debounce
opt Rack removed later
Persistence->>LocalStorage: Clear session after rack previously existed
end
Generated by CodeAnt AI |
Contributor
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
src/tests/file.test.ts
Outdated
| // Should correctly detect cancel | ||
| expect(result).toBeNull(); | ||
| }, 1000); | ||
| }, 2000); |
Contributor
There was a problem hiding this comment.
Suggestion: This test now has a hard 2s timeout, but the code under test intentionally waits ~1s for cancel detection and can take longer under loaded CI/headless workers. The reduced per-test timeout can reintroduce flaky failures (timeout before assertion) even when behavior is correct. Remove the custom timeout (or raise it to match project defaults) so slow environments do not fail spuriously. [logic error]
Severity Level: Major ⚠️
- ⚠️ Unit test can fail on slower CI workers.
- ⚠️ Flaky test failures reduce suite trustworthiness.
- ⚠️ Production file-picker behavior remains unaffected.
Suggested change
| }, 2000); | |
| }); |
Steps of Reproduction ✅
1. Run unit tests via `npm run test:run` (script at `package.json:19`; also used in CI at
`.github/workflows/test.yml:41`).
2. Vitest includes this file (`vitest.config.ts:66`) with default `testTimeout: 10000`
(`vitest.config.ts:69`), but this specific test overrides timeout to 2000ms at
`src/tests/file.test.ts:127`.
3. The test `returns null on cancel even with rapid interaction`
(`src/tests/file.test.ts:116-127`) dispatches `focus` (`:119`) and waits for
`openFilePicker()` (`:123`), where cancel resolution is intentionally delayed 1000ms
(`src/lib/utils/file.ts:52-58`).
4. Under slower/loaded workers (project uses forked workers at `vitest.config.ts:74-75`;
flaky detection reruns tests repeatedly at `.github/workflows/code-health.yml:81`), extra
event-loop delay can push total runtime beyond 2s, causing timeout failure before
assertion at `src/tests/file.test.ts:126`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tests/file.test.ts
**Line:** 127:127
**Comment:**
*Logic Error: This test now has a hard 2s timeout, but the code under test intentionally waits ~1s for cancel detection and can take longer under loaded CI/headless workers. The reduced per-test timeout can reintroduce flaky failures (timeout before assertion) even when behavior is correct. Remove the custom timeout (or raise it to match project defaults) so slow environments do not fail spuriously.
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/android-chrome.spec.ts`:
- Around line 432-435: The fixed 2s sleep after page.reload is flaky; replace
the page.waitForTimeout(2000) call with a deterministic post-reload wait by
waiting for a concrete condition: use page.waitForLoadState('networkidle')
immediately after page.reload and then wait for a stable DOM signal (e.g., await
page.waitForSelector('<app-ready-or-main-element>') or a specific element/text
that indicates the app/session restore completed) instead of a timeout; update
the test where page.reload and page.waitForTimeout are used to rely on
page.waitForLoadState and a suitable page.waitForSelector or
page.waitForResponse for the known restoration indicator.
In `@e2e/device-metadata.spec.ts`:
- Around line 310-312: The test currently mutates rack 0 and then uses
page.locator(locators.rack.device).first() and selectDevice(page, 0), which both
pick the first DOM device (still rack 0) so it doesn't prove cross-rack
persistence; update the flow to explicitly scope selection and assertions to
rackIndex 1 after calling dragDeviceToRack(page, { rackIndex: 1 }) by selecting
the device within the rack-1 container (use the rack locator for index 1 and
assert its device is visible and has expected metadata), then switch back to
rackIndex 0 (use selectDevice or the rack-0 container) and re-assert
TEST_METADATA is still present on rack 0; make these changes around usages of
dragDeviceToRack, selectDevice, locators.rack.device and the TEST_METADATA
assertions so the test verifies both racks independently.
In `@e2e/helpers/device-actions.ts`:
- Around line 16-18: Update the JSDoc for the exported helper function
dragDeviceToRack to document the new options.rackIndex parameter: describe its
type (number, optional), default behavior when omitted, and exact targeting
semantics (e.g., zero-based rack index used to choose which rack receives the
drop and how it interacts with deviceIndex and yOffsetPercent). Locate the
comment block immediately above the dragDeviceToRack declaration and add a
sentence or `@param` entry for rackIndex clarifying that it selects the
destination rack (zero-based) and what happens if the index is out of range or
unspecified.
- Around line 32-35: The code currently falls back to the first rack SVG when
the requested rack is missing, hiding selection errors; in the function that
selects a rack (using rackSvgs and rackIndex) stop silently redirecting by
validating that rackSvgs[rackIndex] exists and, if not, throw or return a clear
error (or fail the test) instead of using document.querySelector(".rack-svg");
update the place that assigns rack (the rack variable assignment) to perform
this explicit check and surface an informative message including the invalid
rackIndex so tests fail visibly when an out-of-range rack is requested.
In `@e2e/keyboard.spec.ts`:
- Around line 91-100: The test uses a generic '[role="dialog"]' selector which
can match multiple overlapping dialogs (e.g., NewRackWizard and
ConfirmReplaceDialog) and cause flakes; update the test that opens the new rack
wizard (clickNewRack) to assert against the specific dialog locator used across
tests — e.g., use locators.dialog.root (or scope the selector to the
NewRackWizard root) for both the visible and not-to-be-visible assertions so the
checks target the intended dialog instance only.
In `@e2e/multi-rack.spec.ts`:
- Around line 58-73: The test "max rack limit shows toast warning" is flaky/slow
because it sequentially creates 9 racks via UI and asserts against a
non-specific toast selector; update the test to set an explicit timeout (use
test.setTimeout or test(..., { timeout: ... })) to accommodate CI, ensure the
wizard is fully closed before checking for toasts by waiting for "#rack-name" to
be hidden (e.g., await page.locator("#rack-name").waitFor({ state: 'hidden' })),
and make the toast assertion deterministic by either adding a warning-specific
selector to locators (e.g., locators.toast.warning) and asserting that, or
explicitly assert that locators.toast.success is not visible before checking the
warning toast after calling clickNewRack/completeWizardWithClicks.
In `@e2e/starter-library.spec.ts`:
- Around line 234-235: Update the inline comment above the locator declaration
to use current model names: replace "Brush Cable Manager" with "Brush Panel" (or
simply "Cable Manager" as appropriate) so it reads something like "Should show
cable management items (Cable Manager 1U, 2U, and Brush Panel)"; edit the
comment text that precedes the const results =
page.locator(locators.device.paletteItem) line to reflect this wording change.
- Line 262: Remove the changelog-style inline note comment "// Hardcoded colour
assertion removed — violates testing rules (no hardcoded color values)" from the
test body; locate the comment inside the e2e/starter-library.spec.ts test case
(the spec where color assertions are made) and delete that historical rationale
so the test contains only assertions and setup, keeping PR discussion in the
pull request description instead of in-line comments.
- Around line 224-227: The current assertions using
page.locator('.device-palette-item:has-text("Switch")').count().toBeGreaterThan(0)
are too weak; replace them with explicit content checks that the returned items
include the expected device names (e.g., "KVM Switch", "24‑Port Switch",
"48‑Port Switch"). Locate the occurrences using the
'.device-palette-item:has-text("Switch")' locator (and the similar locators at
the other flagged spots) and assert either the exact list via
locator.allTextContents() equality or that the array contains each expected
string, or assert each expected text appears at a specific index with
locator.nth(index).textContent() to ensure the search returns the intended
devices rather than merely any result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57394fe3-1080-4ee7-a6d1-cc2b7409789a
📒 Files selected for processing (22)
e2e/android-chrome.spec.tse2e/archive-format.spec.tse2e/carlton-migration.spec.tse2e/custom-device.spec.tse2e/device-images.spec.tse2e/device-metadata.spec.tse2e/device-name.spec.tse2e/duplicate-device-ids.spec.tse2e/export.spec.tse2e/helpers/device-actions.tse2e/helpers/mobile-navigation.tse2e/ios-safari.spec.tse2e/keyboard.spec.tse2e/multi-rack.spec.tse2e/persistence.spec.tse2e/rack-configuration.spec.tse2e/single-rack.spec.tse2e/starter-library.spec.tse2e/view-reset.spec.tssrc/lib/utils/file.tssrc/lib/utils/persistence-manager.svelte.tssrc/tests/file.test.ts
💤 Files with no reviewable changes (1)
- e2e/single-rack.spec.ts
Comment on lines
91
to
+100
| test("Escape closes dialogs", async ({ page }) => { | ||
| // Open new rack dialog (this shows replace dialog) | ||
| // Open new rack wizard dialog | ||
| await clickNewRack(page); | ||
| await expect(page.locator(locators.dialog.root)).toBeVisible(); | ||
| await expect(page.locator('[role="dialog"]')).toBeVisible(); | ||
|
|
||
| // Press Escape | ||
| await page.keyboard.press("Escape"); | ||
|
|
||
| // Dialog should close | ||
| await expect(page.locator(locators.dialog.root)).not.toBeVisible(); | ||
| await expect(page.locator('[role="dialog"]')).not.toBeVisible(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential ambiguity with [role="dialog"] selector.
Per src/lib/components/Dialog.svelte:44-68, the bits-ui Dialog.Content component adds role="dialog" to all dialogs. If multiple dialogs can coexist (e.g., ConfirmReplaceDialog over NewRackWizard), this selector may match multiple elements, causing flaky assertions.
Consider using locators.dialog.root for consistency with other tests, or scope the selector if disambiguation is needed.
💡 Optional: Use the established locator for consistency
// Open new rack wizard dialog
await clickNewRack(page);
- await expect(page.locator('[role="dialog"]')).toBeVisible();
+ await expect(page.locator(locators.dialog.root)).toBeVisible();
// Press Escape
await page.keyboard.press("Escape");
// Dialog should close
- await expect(page.locator('[role="dialog"]')).not.toBeVisible();
+ await expect(page.locator(locators.dialog.root)).not.toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/keyboard.spec.ts` around lines 91 - 100, The test uses a generic
'[role="dialog"]' selector which can match multiple overlapping dialogs (e.g.,
NewRackWizard and ConfirmReplaceDialog) and cause flakes; update the test that
opens the new rack wizard (clickNewRack) to assert against the specific dialog
locator used across tests — e.g., use locators.dialog.root (or scope the
selector to the NewRackWizard root) for both the visible and not-to-be-visible
assertions so the checks target the intended dialog instance only.
Comment on lines
+58
to
+73
| test("max rack limit shows toast warning", async ({ page }) => { | ||
| // Create 9 more racks (already have 1 from share link) to hit the limit of 10 | ||
| for (let i = 2; i <= 10; i++) { | ||
| await clickNewRack(page); | ||
| await completeWizardWithClicks(page, { name: `Rack ${i}` }); | ||
| } | ||
|
|
||
| // Attempt to create 11th rack — should show toast warning | ||
| await clickNewRack(page); | ||
|
|
||
| // Wizard should NOT open | ||
| await expect(page.locator("#rack-name")).not.toBeVisible(); | ||
|
|
||
| // Toast warning should appear | ||
| await expect(page.locator(locators.toast.root)).toBeVisible(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for warning/error toast selectors in locators
rg -n "toast" e2e/helpers/locators.tsRepository: RackulaLives/Rackula
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Get complete toast locators definition
cat -n e2e/helpers/locators.ts | sed -n '71,85p'Repository: RackulaLives/Rackula
Length of output: 520
🏁 Script executed:
#!/bin/bash
# Check if there are warning-specific toast selectors
rg -A 10 "toast:" e2e/helpers/locators.tsRepository: RackulaLives/Rackula
Length of output: 294
🏁 Script executed:
#!/bin/bash
# Check the complete multi-rack.spec.ts test to understand setup and any existing configurations
cat -n e2e/multi-rack.spec.ts | sed -n '1,80p'Repository: RackulaLives/Rackula
Length of output: 3384
Consider test execution time and toast selector specificity for max rack limit test.
Creating 9 racks sequentially (lines 60-63) will accumulate time in CI environments. Each iteration involves opening the wizard, filling inputs, and waiting for creation.
Additionally, line 72 uses locators.toast.root which matches any toast element (.toast). Since the codebase only defines toast.root and toast.success selectors with no warning-specific selector, a success toast from the 10th rack creation could still be visible when the 11th attempt triggers the warning, causing the assertion to pass prematurely.
Consider:
- Add an explicit timeout for this test to accommodate sequential operations
- Either add a warning-specific toast selector to the locators, or wait explicitly for the wizard to close before checking for the toast
💡 Optional: Add timeout and clarify toast assertion
test("max rack limit shows toast warning", async ({ page }) => {
+ test.setTimeout(60000);
// Create 9 more racks (already have 1 from share link) to hit the limit of 10
for (let i = 2; i <= 10; i++) {
await clickNewRack(page);
await completeWizardWithClicks(page, { name: `Rack ${i}` });
}
// Attempt to create 11th rack — should show toast warning
await clickNewRack(page);
// Wizard should NOT open
await expect(page.locator("#rack-name")).not.toBeVisible();
- // Toast warning should appear
+ // Toast warning should appear (consider adding warning-specific selector to locators)
await expect(page.locator(locators.toast.root)).toBeVisible();
});📝 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
| test("max rack limit shows toast warning", async ({ page }) => { | |
| // Create 9 more racks (already have 1 from share link) to hit the limit of 10 | |
| for (let i = 2; i <= 10; i++) { | |
| await clickNewRack(page); | |
| await completeWizardWithClicks(page, { name: `Rack ${i}` }); | |
| } | |
| // Attempt to create 11th rack — should show toast warning | |
| await clickNewRack(page); | |
| // Wizard should NOT open | |
| await expect(page.locator("#rack-name")).not.toBeVisible(); | |
| // Toast warning should appear | |
| await expect(page.locator(locators.toast.root)).toBeVisible(); | |
| }); | |
| test("max rack limit shows toast warning", async ({ page }) => { | |
| test.setTimeout(60000); | |
| // Create 9 more racks (already have 1 from share link) to hit the limit of 10 | |
| for (let i = 2; i <= 10; i++) { | |
| await clickNewRack(page); | |
| await completeWizardWithClicks(page, { name: `Rack ${i}` }); | |
| } | |
| // Attempt to create 11th rack — should show toast warning | |
| await clickNewRack(page); | |
| // Wizard should NOT open | |
| await expect(page.locator("#rack-name")).not.toBeVisible(); | |
| // Toast warning should appear (consider adding warning-specific selector to locators) | |
| await expect(page.locator(locators.toast.root)).toBeVisible(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/multi-rack.spec.ts` around lines 58 - 73, The test "max rack limit shows
toast warning" is flaky/slow because it sequentially creates 9 racks via UI and
asserts against a non-specific toast selector; update the test to set an
explicit timeout (use test.setTimeout or test(..., { timeout: ... })) to
accommodate CI, ensure the wizard is fully closed before checking for toasts by
waiting for "#rack-name" to be hidden (e.g., await
page.locator("#rack-name").waitFor({ state: 'hidden' })), and make the toast
assertion deterministic by either adding a warning-specific selector to locators
(e.g., locators.toast.warning) and asserting that, or explicitly assert that
locators.toast.success is not visible before checking the warning toast after
calling clickNewRack/completeWizardWithClicks.
e2e/starter-library.spec.ts
Outdated
Comment on lines
+224
to
+227
| // Should show switch items (KVM Switch, 24-Port Switch, 48-Port Switch) | ||
| expect( | ||
| await page.locator('.device-palette-item:has-text("Switch")').count(), | ||
| ).toBeGreaterThan(0); |
Contributor
There was a problem hiding this comment.
Search-result assertions are now too weak and can hide regressions.
These checks only prove “something is visible,” not that the search returned the intended device(s). Please assert expected result content per query.
Proposed assertion tightening
@@
- // Should show switch items (KVM Switch, 24-Port Switch, 48-Port Switch)
- expect(
- await page.locator('.device-palette-item:has-text("Switch")').count(),
- ).toBeGreaterThan(0);
+ // Should show switch items
+ await expect(
+ page.getByRole("listitem", { name: /Switch/i }).first(),
+ ).toBeVisible();
@@
- // Should show cable management items (Cable Manager 1U, 2U, and Brush Cable Manager)
- const results = page.locator(locators.device.paletteItem);
- expect(await results.count()).toBeGreaterThan(0);
+ // Should show cable manager items
+ await expect(
+ page.getByRole("listitem", { name: /Cable Manager/i }).first(),
+ ).toBeVisible();
@@
- // Should show brush panel
- await expect(
- page.locator(locators.device.paletteItem).first(),
- ).toBeVisible();
+ // Should show brush panel
+ await expect(
+ page.getByRole("listitem", {
+ name: "Brush Panel, 1U, cable-management",
+ exact: true,
+ }),
+ ).toBeVisible();Also applies to: 234-237, 244-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/starter-library.spec.ts` around lines 224 - 227, The current assertions
using
page.locator('.device-palette-item:has-text("Switch")').count().toBeGreaterThan(0)
are too weak; replace them with explicit content checks that the returned items
include the expected device names (e.g., "KVM Switch", "24‑Port Switch",
"48‑Port Switch"). Locate the occurrences using the
'.device-palette-item:has-text("Switch")' locator (and the similar locators at
the other flagged spots) and assert either the exact list via
locator.allTextContents() equality or that the array contains each expected
string, or assert each expected text appears at a specific index with
locator.nth(index).textContent() to ensure the search returns the intended
devices rather than merely any result.
- Replace silent rack fallback with explicit error on invalid rackIndex - Add JSDoc for rackIndex parameter in dragDeviceToRack - Scope cross-rack test assertions to correct rack container and verify original rack metadata persists - Strengthen search assertions with specific device name checks - Replace fixed 2s sleep with networkidle + DOM element wait - Increase file test cancel timeout from 2s to 3s - Fix stale comment and remove changelog-style inline comment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/android-chrome.spec.ts (1)
363-382:⚠️ Potential issue | 🟠 MajorClose the bottom sheet before the long-press sequence.
mobileDragDeviceToRack()leaves the sheet/backdrop open. Unlike the tap test, this flow immediately sends pointer events to the rack and then only asserts “no menu”, so it can pass without ever pressing the device.🛠️ Proposed fix
test("long-press does not trigger Android context menu", async ({ page }) => { // Open bottom sheet to expose palette items on mobile await mobileDragDeviceToRack(page); + await page.keyboard.press("Escape"); + await expect(page.locator(locators.mobile.bottomSheet)).not.toBeVisible({ + timeout: 2000, + }); const rackDevice = page.locator(locators.rack.device).first(); await expect(rackDevice).toBeVisible({ timeout: 5000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/android-chrome.spec.ts` around lines 363 - 382, The test calls mobileDragDeviceToRack() which leaves the bottom sheet/backdrop open and then performs the long-press; close the sheet/backdrop before starting the mouse long-press so pointer events actually target the rack. After mobileDragDeviceToRack(page) and before acquiring rackDevice/boundingBox, add a step to close the sheet—e.g. click the sheet backdrop locator or press Escape (use the existing backdrop or sheet close selector in your test harness), then await the backdrop to be hidden; keep references to mobileDragDeviceToRack, rackDevice (locators.rack.device) and contextMenu ([role="menu"]) so the subsequent long-press and expect(contextMenu).not.toBeVisible() validate the correct behavior.
♻️ Duplicate comments (2)
e2e/starter-library.spec.ts (2)
237-241:⚠️ Potential issue | 🟠 MajorCable/brush search checks are non-discriminative and can mask regressions.
At Line 241 and Line 250, assertions only prove that some item is visible. They do not prove the query filtered to cable/brush results. Please assert specific expected results and one unrelated item is hidden.
Suggested tightening
test("can search for cable management devices", async ({ page }) => { const searchInput = page.locator('[data-testid="search-devices"]'); await searchInput.fill("Cable Manager"); - // Should show cable management items (Cable Manager) - const results = page.locator(locators.device.paletteItem); - expect(await results.count()).toBeGreaterThan(0); + await expect( + page.getByRole("listitem", { + name: "Cable Manager, 1U, cable-management", + exact: true, + }), + ).toBeVisible(); + await expect( + page.getByRole("listitem", { + name: "Cable Manager, 2U, cable-management", + exact: true, + }), + ).toBeVisible(); + await expect( + page.getByRole("listitem", { name: "Server, 1U, server", exact: true }), + ).not.toBeVisible(); }); test("can search for brush panel", async ({ page }) => { const searchInput = page.locator('[data-testid="search-devices"]'); await searchInput.fill("Brush Panel"); // Should show brush panel - await expect( - page.locator(locators.device.paletteItem).first(), - ).toBeVisible(); + await expect( + page.getByRole("listitem", { + name: "Brush Panel, 1U, cable-management", + exact: true, + }), + ).toBeVisible(); + await expect( + page.getByRole("listitem", { name: "Server, 1U, server", exact: true }), + ).not.toBeVisible(); });Also applies to: 246-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/starter-library.spec.ts` around lines 237 - 241, The current test fills searchInput with "Cable Manager" and only asserts that page.locator(locators.device.paletteItem) has a count > 0; change the assertions to verify specific expected items appear and at least one unrelated item is not shown: after searchInput.fill("Cable Manager") assert that the expected palette entry text(s) (e.g., the cable/brush palette item labels) are present via the same locator or a more specific locator and that an unrelated item (e.g., a known non-cable palette label) is not visible/has count 0; apply the same tightening to the similar block that fills "Brush" (lines referencing searchInput.fill("Brush") / page.locator(locators.device.paletteItem)) so the test validates both positive and negative outcomes instead of only a non-zero count.
224-233:⚠️ Potential issue | 🟠 MajorSearch assertion is still permissive and misses an expected switch variant.
This block can still pass when search filtering is broken (full list visible), and it does not assert the
"48-Port"switch mentioned in the comment. Please assert expected switch entries explicitly and include at least one negative assertion for a non-switch item.Suggested tightening
// Should show switch items (KVM Switch, 24-Port Switch, 48-Port Switch) - const results = page.locator(locators.device.paletteItem); - expect(await results.count()).toBeGreaterThan(0); - - const names = await results.allTextContents(); - const matchingNames = names.map((n) => n.trim()).filter((n) => /switch/i.test(n)); - expect(matchingNames.length).toBeGreaterThan(0); - expect(matchingNames.some((n) => /KVM Switch/i.test(n))).toBe(true); - expect(matchingNames.some((n) => /24-Port/i.test(n))).toBe(true); + await expect( + page.getByRole("listitem", { name: "KVM Switch, 1U, kvm", exact: true }), + ).toBeVisible(); + await expect( + page.getByRole("listitem", { + name: "Switch (24-Port), 1U, network", + exact: true, + }), + ).toBeVisible(); + await expect( + page.getByRole("listitem", { + name: "Switch (48-Port), 1U, network", + exact: true, + }), + ).toBeVisible(); + await expect( + page.getByRole("listitem", { name: "Server, 1U, server", exact: true }), + ).not.toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/starter-library.spec.ts` around lines 224 - 233, The current test that gathers items via page.locator(locators.device.paletteItem) and builds names/matchingNames is too permissive; update the assertions to explicitly check for the exact expected switch variants (include checks that matchingNames contains "KVM Switch", "24-Port Switch", and "48-Port Switch") and add at least one negative assertion that a known non-switch item (e.g., "Server" or "Firewall") is not present in matchingNames; keep using the same variables (results, names, matchingNames) and the existing locator to locate the elements but tighten the regex/contains checks to assert the three explicit switch strings and one negative case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/device-metadata.spec.ts`:
- Around line 303-315: After creating the second rack, wait for the second rack
container to be mounted before calling dragDeviceToRack; specifically, after
completeWizardWithClicks and before clicking the Devices tab / calling
dragDeviceToRack({ rackIndex: 1 }), add an explicit wait for the second
".rack-front" (use page.locator(".rack-front").nth(1) or the same locator used
later) to be visible/attached so the test won't attempt to target rackIndex 1
before it exists.
---
Outside diff comments:
In `@e2e/android-chrome.spec.ts`:
- Around line 363-382: The test calls mobileDragDeviceToRack() which leaves the
bottom sheet/backdrop open and then performs the long-press; close the
sheet/backdrop before starting the mouse long-press so pointer events actually
target the rack. After mobileDragDeviceToRack(page) and before acquiring
rackDevice/boundingBox, add a step to close the sheet—e.g. click the sheet
backdrop locator or press Escape (use the existing backdrop or sheet close
selector in your test harness), then await the backdrop to be hidden; keep
references to mobileDragDeviceToRack, rackDevice (locators.rack.device) and
contextMenu ([role="menu"]) so the subsequent long-press and
expect(contextMenu).not.toBeVisible() validate the correct behavior.
---
Duplicate comments:
In `@e2e/starter-library.spec.ts`:
- Around line 237-241: The current test fills searchInput with "Cable Manager"
and only asserts that page.locator(locators.device.paletteItem) has a count > 0;
change the assertions to verify specific expected items appear and at least one
unrelated item is not shown: after searchInput.fill("Cable Manager") assert that
the expected palette entry text(s) (e.g., the cable/brush palette item labels)
are present via the same locator or a more specific locator and that an
unrelated item (e.g., a known non-cable palette label) is not visible/has count
0; apply the same tightening to the similar block that fills "Brush" (lines
referencing searchInput.fill("Brush") /
page.locator(locators.device.paletteItem)) so the test validates both positive
and negative outcomes instead of only a non-zero count.
- Around line 224-233: The current test that gathers items via
page.locator(locators.device.paletteItem) and builds names/matchingNames is too
permissive; update the assertions to explicitly check for the exact expected
switch variants (include checks that matchingNames contains "KVM Switch",
"24-Port Switch", and "48-Port Switch") and add at least one negative assertion
that a known non-switch item (e.g., "Server" or "Firewall") is not present in
matchingNames; keep using the same variables (results, names, matchingNames) and
the existing locator to locate the elements but tighten the regex/contains
checks to assert the three explicit switch strings and one negative case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bca2a081-7c56-42b6-b916-8e6200d9c0df
📒 Files selected for processing (5)
e2e/android-chrome.spec.tse2e/device-metadata.spec.tse2e/helpers/device-actions.tse2e/starter-library.spec.tssrc/tests/file.test.ts
Comment on lines
+303
to
+315
| // Create a second rack (multi-rack mode — wizard opens directly) | ||
| await clickNewRack(page); | ||
| await page.click('[data-testid="btn-replace-rack"]'); | ||
| await completeWizardWithClicks(page, { name: "Second Rack", height: 24 }); | ||
|
|
||
| // Add a device to the second rack | ||
| await dragDeviceToRack(page); | ||
| await expect(page.locator(locators.rack.device).first()).toBeVisible(); | ||
| // Switch back to Devices tab (clickNewRack switches to Racks tab) | ||
| await page.getByTestId("sidebar-tab-devices").click(); | ||
|
|
||
| // Add a device to the second rack (rackIndex 1) | ||
| await dragDeviceToRack(page, { rackIndex: 1 }); | ||
|
|
||
| // Scope assertions to the second rack container | ||
| const secondRack = page.locator(".rack-front").nth(1); | ||
| await expect(secondRack.locator(locators.rack.device).first()).toBeVisible(); |
Contributor
There was a problem hiding this comment.
Wait for rack 2 before calling dragDeviceToRack({ rackIndex: 1 }).
completeWizardWithClicks only waits on the first rack container, which is already present before this flow starts. On slower runs this can switch back to Devices and try to target rack 2 before the second .rack-front is mounted.
🛠️ Proposed fix
await clickNewRack(page);
await completeWizardWithClicks(page, { name: "Second Rack", height: 24 });
+ const rackFronts = page.locator(".rack-front");
+ await expect(rackFronts).toHaveCount(2);
+
// Switch back to Devices tab (clickNewRack switches to Racks tab)
await page.getByTestId("sidebar-tab-devices").click();
// Add a device to the second rack (rackIndex 1)
await dragDeviceToRack(page, { rackIndex: 1 });
// Scope assertions to the second rack container
- const secondRack = page.locator(".rack-front").nth(1);
+ const secondRack = rackFronts.nth(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/device-metadata.spec.ts` around lines 303 - 315, After creating the
second rack, wait for the second rack container to be mounted before calling
dragDeviceToRack; specifically, after completeWizardWithClicks and before
clicking the Devices tab / calling dragDeviceToRack({ rackIndex: 1 }), add an
explicit wait for the second ".rack-front" (use
page.locator(".rack-front").nth(1) or the same locator used later) to be
visible/attached so the test won't attempt to target rackIndex 1 before it
exists.
Contributor
There was a problem hiding this comment.
@ggfevans, here's the fix for the race condition:
- Add
await expect(rackFronts).toHaveCount(2)aftercompleteWizardWithClicksso the test waits for the second.rack-frontto be mounted before switching tabs and dragging - Hoist the
rackFrontslocator so it can be reused forsecondRack
🧠 Learnings used
Learnt from: ggfevans
Repo: RackulaLives/Rackula PR: 1414
File: e2e/device-metadata.spec.ts:110-111
Timestamp: 2026-03-08T11:42:39.285Z
Learning: In `e2e/device-metadata.spec.ts`, `setDeviceColour()` is only called in the "Save Serialization" test. After the call, the test proceeds to `deselectDevice()` and download/YAML assertions. `getDeviceMetadata()` is NOT called after `setDeviceColour()`. Additionally, `colour_override` is not currently serialized to YAML (tracked separately), so colour is not verified in the YAML assertions of that test.
Learnt from: CR
Repo: RackulaLives/Rackula PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T03:55:06.866Z
Learning: Wait for CodeRabbit approval before merging PRs; use `coderabbit --prompt-only --type uncommitted` or `--type committed` to review changes locally before pushing
Learnt from: ggfevans
Repo: RackulaLives/Rackula PR: 1351
File: src/lib/components/LoadDialog.svelte:16-56
Timestamp: 2026-02-28T01:41:47.732Z
Learning: Guideline: Use the correct logging namespaces. The persistenceDebug.api logger maps to the rackula:persistence:api namespace and should be used for persistence API operations (e.g., listSavedLayouts, deleteSavedLayout). The layoutDebug namespace (rackula:layout:*) is for layout state and device operations, not API calls. In files like src/lib/components/LoadDialog.svelte, ensure API-related logging uses rackula:persistence:api, while layout-related events use rackula:layout:*. This separation improves log clarity and traceability.
Failed to handle agent chat message. Please try again.
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:L
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
Changes
App bug fixes
$effectininitPersistenceEffectscleared localStorage beforeApp.onMountcould restore session data. AddedhasEverHadRackguard.openFilePickerto prevent false cancels in slow/headless environments.E2E test fixes (across 3 commits)
single-rack.spec.tswithmulti-rack.spec.ts.tap()→.click()mobileDragDeviceToRack()helper.first()/.last()to ambiguous locators.first()on success toast across 5 spec filesrackIndexoption indragDeviceToRackbtn-add-device→btn-create-custom-device7 Skipped tests (documented)
bind:valueon number inputs + palette orderinghasTouch)Test plan
npx playwright test— 0 failures, 164 passed, 7 skipped (2 consecutive runs)npm run test:run— 1949 unit tests passednpm run lint— clean🤖 Generated with Claude Code
CodeAnt-AI Description
Prevent saved work from disappearing on reload and reduce false file-picker cancels
What Changed
Impact
✅ Fewer lost sessions after refresh✅ Fewer false file-picker cancels✅ More reliable file import and export flows💡 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.