Conversation
|
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 many new and expanded frontend unit tests, standardizes test import paths to alias resolution, centralizes shared test utilities and mocks, adjusts Vitest setup, removes explicit Svelte Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/editor/__tests__/LanguageSelect.test.ts (2)
21-26:⚠️ Potential issue | 🟡 MinorMissing
beforeEachandafterEachhooks per coding guidelines.The test file should include
vi.clearAllMocks()inbeforeEachandcleanup()inafterEachto ensure proper test isolation.Proposed fix
+import { render, screen, within, fireEvent, waitFor, cleanup } from '@testing-library/svelte'; -import { render, screen, within, fireEvent, waitFor } from '@testing-library/svelte';describe('LanguageSelect', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + async function openMenu() {As per coding guidelines: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts` around lines 21 - 26, Tests in the LanguageSelect suite are missing lifecycle hooks; inside the describe('LanguageSelect', ...) block add a beforeEach that calls vi.clearAllMocks() and an afterEach that calls cleanup() (ensure cleanup is imported from `@testing-library/react` and vi is available via Vitest), so place these hooks alongside the existing helper openMenu() to guarantee test isolation and mock reset between tests.
6-6:⚠️ Potential issue | 🟡 MinorUse path alias instead of relative import.
Per coding guidelines, use path aliases (
$lib,$components,$stores, etc.) for all imports instead of relative imports.Proposed fix
-import LanguageSelect from '../LanguageSelect.svelte'; +import LanguageSelect from '$components/editor/LanguageSelect.svelte';As per coding guidelines: "Use path aliases ($lib, $components, $stores, $routes, $utils) for all imports; never use relative imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts` at line 6, The test imports LanguageSelect via a relative path; update the import to use the project path alias (e.g., $components or the correct alias for your components folder) instead of '../LanguageSelect.svelte' so the test imports LanguageSelect from the aliased module (replace the import statement that references LanguageSelect in LanguageSelect.test.ts to use the $components/... alias).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/__tests__/EventTypeIcon.test.ts`:
- Around line 1-8: The test suite is missing DOM cleanup after each spec; add an
afterEach hook that calls cleanup() to remove rendered DOM and prevent leakage
between multiple render() calls (lines with render at 33, 42, 47, 50, 64); keep
the existing beforeEach with vi.clearAllMocks(), and ensure cleanup() is
imported from '@testing-library/svelte' at the top so the afterEach(() =>
cleanup()) runs after each test.
In `@frontend/src/components/__tests__/Modal.test.ts`:
- Around line 1-9: Update the test setup: change the component import to use the
path alias by replacing the relative import of ModalWrapper.svelte with the
alias $components/__tests__/ModalWrapper.svelte (reference: ModalWrapper import)
and add an afterEach(cleanup) call alongside the existing beforeEach that calls
vi.clearAllMocks() so the DOM is cleaned between tests (references: beforeEach,
vi.clearAllMocks, afterEach, cleanup).
In `@frontend/src/components/__tests__/ModalWrapper.svelte`:
- Line 2: Replace the relative import of Modal in the ModalWrapper.svelte test
with the $components path alias: update the import statement that references
Modal (symbol: Modal) in ModalWrapper.svelte to import from
"$components/Modal.svelte" instead of a relative path so it adheres to the
frontend path-alias guideline.
In `@frontend/src/components/__tests__/Pagination.test.ts`:
- Around line 19-22: The test suite for Pagination is missing an afterEach
cleanup call; update the test file so that afterEach invokes cleanup() to
unmount components between tests, and ensure cleanup is imported from
'@testing-library/react' at the top of the file; keep the existing beforeEach
vi.clearAllMocks() and add afterEach(() => cleanup()) alongside it to satisfy
the guideline referenced in the describe('Pagination', ...) block.
In `@frontend/src/components/admin/__tests__/ActionButtons.test.ts`:
- Around line 1-21: Update the ActionButtons import to use the path alias by
replacing the relative import of ActionButtons.svelte with
'$components/admin/ActionButtons.svelte', add an import for cleanup from
'@testing-library/svelte', and call afterEach(cleanup) inside the describe block
(use the existing beforeEach/vi.clearAllMocks pattern) so rendered components
are torn down between tests; ensure the symbols referenced are the ActionButtons
import, cleanup, and afterEach.
In `@frontend/src/components/admin/__tests__/FilterPanel.test.ts`:
- Around line 1-9: Add an afterEach cleanup hook and switch the relative Svelte
import to the project path alias: import cleanup from the testing library and
call afterEach(cleanup); replace the current relative import of
FilterPanelWrapper.svelte with the path-alias version used across the repo (keep
the same component symbol FilterPanelWrapper) so the file uses
afterEach(cleanup) and the aliased import for FilterPanelWrapper.
In `@frontend/src/components/admin/__tests__/FilterPanelWrapper.svelte`:
- Line 2: Replace the relative import of FilterPanel in
FilterPanelWrapper.svelte with the project path-alias: change the import
statement that currently references '../FilterPanel.svelte' to use the
$components alias (e.g., import FilterPanel from '$components/...'), ensuring it
points to the same FilterPanel component location so the file follows the
frontend import policy.
In `@frontend/src/components/admin/__tests__/StatsCard.test.ts`:
- Around line 1-11: The test is missing teardown and uses a relative import; add
an afterEach(cleanup) call to the test file to ensure DOM cleanup after each
test and change the StatsCard import to use the path alias
($components/admin/StatsCard.svelte) instead of the relative path; update the
top imports to include cleanup from '@testing-library/svelte' and ensure the
existing describe/beforeEach block remains unchanged while the StatsCard import
target is replaced accordingly.
In `@frontend/src/components/admin/__tests__/StatusBadge.test.ts`:
- Around line 1-8: Replace the relative import of StatusBadge in the test with
the path alias import "$components/admin/StatusBadge.svelte" and add a test
teardown hook by importing cleanup from '@testing-library/svelte' and calling
cleanup() inside an afterEach block; update the imports at the top (replace
'../StatusBadge.svelte' with the alias and add cleanup) and add afterEach(() =>
cleanup()) below the existing beforeEach to ensure DOM is cleaned between tests.
---
Outside diff comments:
In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts`:
- Around line 21-26: Tests in the LanguageSelect suite are missing lifecycle
hooks; inside the describe('LanguageSelect', ...) block add a beforeEach that
calls vi.clearAllMocks() and an afterEach that calls cleanup() (ensure cleanup
is imported from `@testing-library/react` and vi is available via Vitest), so
place these hooks alongside the existing helper openMenu() to guarantee test
isolation and mock reset between tests.
- Line 6: The test imports LanguageSelect via a relative path; update the import
to use the project path alias (e.g., $components or the correct alias for your
components folder) instead of '../LanguageSelect.svelte' so the test imports
LanguageSelect from the aliased module (replace the import statement that
references LanguageSelect in LanguageSelect.test.ts to use the $components/...
alias).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/src/components/__tests__/EventTypeIcon.test.tsfrontend/src/components/__tests__/Modal.test.tsfrontend/src/components/__tests__/ModalWrapper.sveltefrontend/src/components/__tests__/Pagination.test.tsfrontend/src/components/admin/__tests__/ActionButtons.test.tsfrontend/src/components/admin/__tests__/FilterPanel.test.tsfrontend/src/components/admin/__tests__/FilterPanelWrapper.sveltefrontend/src/components/admin/__tests__/StatsCard.test.tsfrontend/src/components/admin/__tests__/StatusBadge.test.tsfrontend/src/components/editor/__tests__/LanguageSelect.test.ts
There was a problem hiding this comment.
Pull request overview
Adds additional frontend unit coverage for several Svelte components, primarily around keyboard interaction behavior and common UI rendering/styling states.
Changes:
- Extend
LanguageSelectunit tests to cover Space key activation and deeper menu/submenu keyboard navigation. - Add new unit tests for admin UI components (
StatusBadge,StatsCard,FilterPanel,ActionButtons). - Add new unit tests for shared UI components (
Pagination,Modal,EventTypeIcon) plus small Svelte wrappers to support modal/panel testing.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/editor/tests/LanguageSelect.test.ts | Adds Space key coverage and comprehensive menu/submenu keyboard navigation tests. |
| frontend/src/components/admin/tests/StatusBadge.test.ts | New rendering/class/suffix coverage for StatusBadge. |
| frontend/src/components/admin/tests/StatsCard.test.ts | New coverage for label/value rendering, sublabel, icon, compact mode, and color props. |
| frontend/src/components/admin/tests/FilterPanelWrapper.svelte | Test wrapper enabling bind:open and slot content for FilterPanel. |
| frontend/src/components/admin/tests/FilterPanel.test.ts | New coverage for toggle button behavior, badge visibility, title, and action buttons. |
| frontend/src/components/admin/tests/ActionButtons.test.ts | New coverage for variants, colors, disabled behavior, titles, and action count. |
| frontend/src/components/tests/Pagination.test.ts | New coverage for page info text, navigation, disabled states, page size selector, and visibility rules. |
| frontend/src/components/tests/ModalWrapper.svelte | Test wrapper to provide modal body and optional footer snippet. |
| frontend/src/components/tests/Modal.test.ts | New coverage for open/closed rendering, a11y attributes, close interactions, sizing, and footer rendering. |
| frontend/src/components/tests/EventTypeIcon.test.ts | New coverage for known/unknown event type icons and size prop behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…instead of relative paths)
…instead of relative paths)
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 (4)
frontend/src/components/editor/__tests__/OutputPanel.test.ts (1)
34-39:⚠️ Potential issue | 🟡 MinorMissing
cleanup()inafterEachhook.Add
afterEachwithcleanup()to properly unmount components between tests.Proposed fix
+import { cleanup } from '@testing-library/svelte'; + describe('OutputPanel', () => { beforeEach(() => { vi.clearAllMocks(); vi.spyOn(toast, 'success'); vi.spyOn(toast, 'error'); }); + + afterEach(() => { + cleanup(); + });As per coding guidelines: "Frontend tests must... call cleanup() in afterEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/__tests__/OutputPanel.test.ts` around lines 34 - 39, The tests for OutputPanel are missing an afterEach hook that calls cleanup(), so add an afterEach(() => { cleanup(); }) alongside the existing beforeEach to ensure components are unmounted between tests; update the test file where describe('OutputPanel') contains beforeEach (and the vi.clearAllMocks / vi.spyOn calls) to include afterEach calling cleanup() (import cleanup from `@testing-library/react` if not already imported).frontend/src/components/admin/events/__tests__/EventsTable.test.ts (1)
21-24:⚠️ Potential issue | 🟡 MinorMissing
cleanup()inafterEachhook.Add
afterEachwithcleanup()to properly unmount components between tests.Proposed fix
+import { cleanup } from '@testing-library/svelte'; + describe('EventsTable', () => { beforeEach(() => { vi.clearAllMocks(); }); + + afterEach(() => { + cleanup(); + });As per coding guidelines: "Frontend tests must... call cleanup() in afterEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/__tests__/EventsTable.test.ts` around lines 21 - 24, Tests for the EventsTable suite are missing an afterEach that unmounts components; add an afterEach hook calling cleanup() (from `@testing-library/react`) to run after each test alongside the existing beforeEach that calls vi.clearAllMocks(), ensuring components rendered in the describe('EventsTable') block are properly unmounted between tests and preventing cross-test leakage.frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts (1)
41-44:⚠️ Potential issue | 🟡 MinorMissing
cleanup()inafterEachhook.Add
afterEachwithcleanup()to properly unmount components between tests.Proposed fix
+import { cleanup } from '@testing-library/svelte'; + describe('ReplayPreviewModal', () => { beforeEach(() => { vi.clearAllMocks(); }); + + afterEach(() => { + cleanup(); + });As per coding guidelines: "Frontend tests must... call cleanup() in afterEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts` around lines 41 - 44, The test suite for ReplayPreviewModal is missing an afterEach teardown to unmount components; add an afterEach hook that calls cleanup() (from `@testing-library/react`) after the existing beforeEach vi.clearAllMocks() to ensure components are unmounted between tests and prevent cross-test state leakage in ReplayPreviewModal.test.ts.frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts (1)
21-24:⚠️ Potential issue | 🟡 MinorMissing
cleanup()inafterEachhook.Per coding guidelines, frontend tests should call
cleanup()inafterEachto properly unmount components between tests. This prevents potential test pollution.Proposed fix
+import { cleanup } from '@testing-library/svelte'; + describe('EventDetailsModal', () => { beforeEach(() => { vi.clearAllMocks(); }); + + afterEach(() => { + cleanup(); + });As per coding guidelines: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts` around lines 21 - 24, The test suite for EventDetailsModal is missing an afterEach cleanup call; add an afterEach hook that calls cleanup() (imported from `@testing-library/react` if not already) so components are unmounted between tests, and keep the existing vi.clearAllMocks() in beforeEach; update the top of EventDetailsModal.test.ts to ensure cleanup is imported and add afterEach(() => cleanup()) alongside the existing beforeEach.
♻️ Duplicate comments (1)
frontend/src/components/admin/__tests__/StatsCard.test.ts (1)
1-2:⚠️ Potential issue | 🟠 MajorAdd
afterEach(cleanup)to complete test teardown.
beforeEach(vi.clearAllMocks())is present, but teardown cleanup is missing, so DOM can leak between tests.🔧 Suggested patch
-import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { cleanup, render, screen } from '@testing-library/svelte'; @@ describe('StatsCard', () => { beforeEach(() => { vi.clearAllMocks(); }); + + afterEach(() => { + cleanup(); + });As per coding guidelines
frontend/src/**/__tests__/**/*.test.ts: “call cleanup() in afterEach and vi.clearAllMocks() in beforeEach.”Also applies to: 8-12
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/__tests__/StatsCard.test.ts` around lines 1 - 2, Add test teardown by importing cleanup from '@testing-library/svelte' and calling afterEach(cleanup) in the test file; keep the existing beforeEach(vi.clearAllMocks()) as-is so the file contains both beforeEach(vi.clearAllMocks()) and afterEach(cleanup) to prevent DOM leaks between tests (update frontend/src/components/admin/__tests__/StatsCard.test.ts and any other tests flagged similarly).
🧹 Nitpick comments (8)
frontend/src/components/admin/__tests__/StatsCard.test.ts (1)
28-31: Prefer user-visible assertions over paragraph-count checks.
querySelectorAll('p')-count assertions are fragile and can fail on harmless markup changes. Prefer role/text-based checks tied to behavior.Also applies to: 39-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/__tests__/StatsCard.test.ts` around lines 28 - 31, Replace fragile paragraph-count assertions in the StatsCard tests with user-facing text/role queries: update the test "does not render sublabel when absent" to render StatsCard with props { label: 'Total', value: 42 } and assert the sublabel text is not present using something like queryByText('<expected sublabel text>') or queryByRole if the sublabel has a role, and update the complementary test (the one that renders a sublabel) to assert presence via getByText/getByRole for the exact sublabel string; locate the tests referencing StatsCard and the two it(...) blocks and swap the container.querySelectorAll('p') checks for text/role-based assertions to make them resilient to markup changes.frontend/src/components/editor/__tests__/ResourceLimits.test.ts (1)
17-21: Missing test lifecycle hooks (beforeEach/afterEach).Add
cleanup()inafterEachto properly unmount components between tests.Proposed fix
+import { cleanup } from '@testing-library/svelte'; +import { afterEach } from 'vitest'; + describe('ResourceLimits', () => { + afterEach(() => { + cleanup(); + }); + it('renders nothing when limits is null', () => {As per coding guidelines: "Frontend tests must... call cleanup() in afterEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/__tests__/ResourceLimits.test.ts` around lines 17 - 21, The test 'ResourceLimits' is missing teardown hooks; add an afterEach hook that calls cleanup() to unmount components between tests (e.g., add afterEach(() => cleanup()) in the test file testing the ResourceLimits component) and ensure any required imports (cleanup from '@testing-library/svelte') are present so render(ResourceLimits, ...) is cleaned up after each spec.frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts (1)
11-15: Missing test lifecycle hooks (beforeEach/afterEach).Per coding guidelines, frontend tests should include
vi.clearAllMocks()inbeforeEachandcleanup()inafterEach. While this test suite doesn't use mocks, adding these hooks ensures consistency and prevents potential issues if mocks are added later.Proposed fix
+import { cleanup } from '@testing-library/svelte'; +import { beforeEach, afterEach, vi } from 'vitest'; + describe('EventStatsCards', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + it('renders nothing when stats is null', () => {As per coding guidelines: "Frontend tests must... call cleanup() in afterEach and vi.clearAllMocks() in beforeEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts` around lines 11 - 15, Add test lifecycle hooks to the EventStatsCards suite: inside the describe('EventStatsCards') block add a beforeEach that calls vi.clearAllMocks() and an afterEach that calls cleanup() so the suite always clears mocks and cleans DOM between tests; reference the existing describe block and the test helper/render function (renderCards) when adding these hooks.frontend/src/components/editor/__tests__/SavedScripts.test.ts (1)
30-35: Missing test lifecycle hooks.The test suite is missing
beforeEachwithvi.clearAllMocks()andafterEachwithcleanup()as required by the testing guidelines.♻️ Proposed fix
+import { cleanup } from '@testing-library/svelte'; + describe('SavedScripts', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + async function renderAndExpand(scripts: SavedScriptResponse[] = []) {Based on learnings: "Frontend tests must... call cleanup() in afterEach and vi.clearAllMocks() in beforeEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/__tests__/SavedScripts.test.ts` around lines 30 - 35, The SavedScripts test suite is missing test lifecycle hooks; add a beforeEach that calls vi.clearAllMocks() and an afterEach that calls cleanup() inside the describe('SavedScripts', ...) block so mocks are reset before each test and DOM is cleaned after each test; place these hooks alongside the existing renderAndExpand helper so they apply to all tests in this suite.frontend/src/components/editor/__tests__/CodeMirrorEditor.test.ts (1)
86-90: Missingcleanup()inafterEachhook.Per the test guidelines, frontend tests should call
cleanup()inafterEachto ensure proper DOM cleanup between tests, especially withisolate: falsein the Vitest config.♻️ Proposed fix
+import { cleanup } from '@testing-library/svelte'; + describe('CodeMirrorEditor', () => { beforeEach(() => { vi.clearAllMocks(); document.documentElement.classList.remove('dark'); }); + + afterEach(() => { + cleanup(); + });Based on learnings: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/__tests__/CodeMirrorEditor.test.ts` around lines 86 - 90, Add a teardown call to the test file so DOM is cleaned between tests: in the CodeMirrorEditor test suite add an afterEach hook that calls cleanup() (alongside existing beforeEach which already calls vi.clearAllMocks() and document.documentElement.classList.remove('dark')); ensure you import or reference cleanup from the testing-library utilities used by the project and place the call in afterEach to satisfy the isolate:false Vitest config.frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts (1)
35-38: Missingcleanup()inafterEachhook.Add an
afterEachhook to callcleanup()for proper DOM cleanup between tests.♻️ Proposed fix
+import { cleanup } from '@testing-library/svelte'; + describe('ReplayProgressBanner', () => { beforeEach(() => { vi.clearAllMocks(); }); + + afterEach(() => { + cleanup(); + });Based on learnings: "Frontend tests must... call cleanup() in afterEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts` around lines 35 - 38, The test suite for ReplayProgressBanner currently only clears mocks in beforeEach; add an afterEach hook that calls cleanup() to ensure DOM is torn down between tests, and import cleanup from `@testing-library/react` if not already imported; update the describe('ReplayProgressBanner') block (where beforeEach calls vi.clearAllMocks()) to include afterEach(() => cleanup()) so each test's DOM is properly cleaned up.frontend/src/components/admin/events/__tests__/EventFilters.test.ts (1)
17-20: Missingcleanup()inafterEachhook.Add an
afterEachhook to callcleanup()for proper DOM cleanup between tests.♻️ Proposed fix
+import { cleanup } from '@testing-library/svelte'; + describe('EventFilters', () => { beforeEach(() => { vi.clearAllMocks(); }); + + afterEach(() => { + cleanup(); + });Based on learnings: "Frontend tests must... call cleanup() in afterEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/__tests__/EventFilters.test.ts` around lines 17 - 20, Add an afterEach hook to this test suite to call cleanup() so the DOM is torn down between tests: inside the describe('EventFilters') block (next to the existing beforeEach that calls vi.clearAllMocks()), add afterEach(() => { cleanup(); }) and ensure cleanup is imported from the testing library; this will avoid DOM state leakage across tests.frontend/src/components/editor/__tests__/LanguageSelect.test.ts (1)
21-26: Add test lifecycle hooks for cleanup and mock reset.Per coding guidelines, frontend tests should call
cleanup()inafterEachandvi.clearAllMocks()inbeforeEachto ensure proper isolation between test runs.♻️ Suggested lifecycle hooks
+import { cleanup } from '@testing-library/svelte'; + describe('LanguageSelect', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + async function openMenu() {As per coding guidelines: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts` around lines 21 - 26, Tests in LanguageSelect.test do not include the required lifecycle hooks which can cause cross-test contamination; add a beforeEach that calls vi.clearAllMocks() and an afterEach that calls cleanup() (import cleanup from `@testing-library/react` if not already) at the top-level of the describe block containing openMenu/renderSelect so mocks are reset and DOM is cleaned between each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/vitest.setup.ts`:
- Around line 101-106: The forEach callbacks in beforeEach currently use concise
arrow bodies that return the boolean result of delete
(localStorageStore/sessionStorageStore), which triggers a lint error; update the
callbacks to not return a value—either switch to a for...of loop over
Object.keys(localStorageStore) and Object.keys(sessionStorageStore) and call
delete inside the loop, or keep forEach but change the arrow to a block body
(e.g., forEach(key => { delete localStorageStore[key]; }) and similarly for
sessionStorageStore) so the delete expression is not returned; leave the call to
cleanup() unchanged.
---
Outside diff comments:
In `@frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts`:
- Around line 21-24: The test suite for EventDetailsModal is missing an
afterEach cleanup call; add an afterEach hook that calls cleanup() (imported
from `@testing-library/react` if not already) so components are unmounted between
tests, and keep the existing vi.clearAllMocks() in beforeEach; update the top of
EventDetailsModal.test.ts to ensure cleanup is imported and add afterEach(() =>
cleanup()) alongside the existing beforeEach.
In `@frontend/src/components/admin/events/__tests__/EventsTable.test.ts`:
- Around line 21-24: Tests for the EventsTable suite are missing an afterEach
that unmounts components; add an afterEach hook calling cleanup() (from
`@testing-library/react`) to run after each test alongside the existing beforeEach
that calls vi.clearAllMocks(), ensuring components rendered in the
describe('EventsTable') block are properly unmounted between tests and
preventing cross-test leakage.
In `@frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts`:
- Around line 41-44: The test suite for ReplayPreviewModal is missing an
afterEach teardown to unmount components; add an afterEach hook that calls
cleanup() (from `@testing-library/react`) after the existing beforeEach
vi.clearAllMocks() to ensure components are unmounted between tests and prevent
cross-test state leakage in ReplayPreviewModal.test.ts.
In `@frontend/src/components/editor/__tests__/OutputPanel.test.ts`:
- Around line 34-39: The tests for OutputPanel are missing an afterEach hook
that calls cleanup(), so add an afterEach(() => { cleanup(); }) alongside the
existing beforeEach to ensure components are unmounted between tests; update the
test file where describe('OutputPanel') contains beforeEach (and the
vi.clearAllMocks / vi.spyOn calls) to include afterEach calling cleanup()
(import cleanup from `@testing-library/react` if not already imported).
---
Duplicate comments:
In `@frontend/src/components/admin/__tests__/StatsCard.test.ts`:
- Around line 1-2: Add test teardown by importing cleanup from
'@testing-library/svelte' and calling afterEach(cleanup) in the test file; keep
the existing beforeEach(vi.clearAllMocks()) as-is so the file contains both
beforeEach(vi.clearAllMocks()) and afterEach(cleanup) to prevent DOM leaks
between tests (update frontend/src/components/admin/__tests__/StatsCard.test.ts
and any other tests flagged similarly).
---
Nitpick comments:
In `@frontend/src/components/admin/__tests__/StatsCard.test.ts`:
- Around line 28-31: Replace fragile paragraph-count assertions in the StatsCard
tests with user-facing text/role queries: update the test "does not render
sublabel when absent" to render StatsCard with props { label: 'Total', value: 42
} and assert the sublabel text is not present using something like
queryByText('<expected sublabel text>') or queryByRole if the sublabel has a
role, and update the complementary test (the one that renders a sublabel) to
assert presence via getByText/getByRole for the exact sublabel string; locate
the tests referencing StatsCard and the two it(...) blocks and swap the
container.querySelectorAll('p') checks for text/role-based assertions to make
them resilient to markup changes.
In `@frontend/src/components/admin/events/__tests__/EventFilters.test.ts`:
- Around line 17-20: Add an afterEach hook to this test suite to call cleanup()
so the DOM is torn down between tests: inside the describe('EventFilters') block
(next to the existing beforeEach that calls vi.clearAllMocks()), add
afterEach(() => { cleanup(); }) and ensure cleanup is imported from the testing
library; this will avoid DOM state leakage across tests.
In `@frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts`:
- Around line 11-15: Add test lifecycle hooks to the EventStatsCards suite:
inside the describe('EventStatsCards') block add a beforeEach that calls
vi.clearAllMocks() and an afterEach that calls cleanup() so the suite always
clears mocks and cleans DOM between tests; reference the existing describe block
and the test helper/render function (renderCards) when adding these hooks.
In `@frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts`:
- Around line 35-38: The test suite for ReplayProgressBanner currently only
clears mocks in beforeEach; add an afterEach hook that calls cleanup() to ensure
DOM is torn down between tests, and import cleanup from `@testing-library/react`
if not already imported; update the describe('ReplayProgressBanner') block
(where beforeEach calls vi.clearAllMocks()) to include afterEach(() =>
cleanup()) so each test's DOM is properly cleaned up.
In `@frontend/src/components/editor/__tests__/CodeMirrorEditor.test.ts`:
- Around line 86-90: Add a teardown call to the test file so DOM is cleaned
between tests: in the CodeMirrorEditor test suite add an afterEach hook that
calls cleanup() (alongside existing beforeEach which already calls
vi.clearAllMocks() and document.documentElement.classList.remove('dark'));
ensure you import or reference cleanup from the testing-library utilities used
by the project and place the call in afterEach to satisfy the isolate:false
Vitest config.
In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts`:
- Around line 21-26: Tests in LanguageSelect.test do not include the required
lifecycle hooks which can cause cross-test contamination; add a beforeEach that
calls vi.clearAllMocks() and an afterEach that calls cleanup() (import cleanup
from `@testing-library/react` if not already) at the top-level of the describe
block containing openMenu/renderSelect so mocks are reset and DOM is cleaned
between each test.
In `@frontend/src/components/editor/__tests__/ResourceLimits.test.ts`:
- Around line 17-21: The test 'ResourceLimits' is missing teardown hooks; add an
afterEach hook that calls cleanup() to unmount components between tests (e.g.,
add afterEach(() => cleanup()) in the test file testing the ResourceLimits
component) and ensure any required imports (cleanup from
'@testing-library/svelte') are present so render(ResourceLimits, ...) is cleaned
up after each spec.
In `@frontend/src/components/editor/__tests__/SavedScripts.test.ts`:
- Around line 30-35: The SavedScripts test suite is missing test lifecycle
hooks; add a beforeEach that calls vi.clearAllMocks() and an afterEach that
calls cleanup() inside the describe('SavedScripts', ...) block so mocks are
reset before each test and DOM is cleaned after each test; place these hooks
alongside the existing renderAndExpand helper so they apply to all tests in this
suite.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
frontend/src/__tests__/test-utils.tsfrontend/src/components/__tests__/ModalWrapper.sveltefrontend/src/components/admin/__tests__/ActionButtons.test.tsfrontend/src/components/admin/__tests__/FilterPanelWrapper.sveltefrontend/src/components/admin/__tests__/StatsCard.test.tsfrontend/src/components/admin/__tests__/StatusBadge.test.tsfrontend/src/components/admin/events/__tests__/EventDetailsModal.test.tsfrontend/src/components/admin/events/__tests__/EventFilters.test.tsfrontend/src/components/admin/events/__tests__/EventStatsCards.test.tsfrontend/src/components/admin/events/__tests__/EventsTable.test.tsfrontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.tsfrontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.tsfrontend/src/components/admin/events/__tests__/UserOverviewModal.test.tsfrontend/src/components/editor/__tests__/CodeMirrorEditor.test.tsfrontend/src/components/editor/__tests__/LanguageSelect.test.tsfrontend/src/components/editor/__tests__/OutputPanel.test.tsfrontend/src/components/editor/__tests__/ResourceLimits.test.tsfrontend/src/components/editor/__tests__/SavedScripts.test.tsfrontend/src/lib/__tests__/api-interceptors.test.tsfrontend/src/lib/__tests__/formatters.test.tsfrontend/src/routes/admin/__tests__/AdminEvents.test.tsfrontend/src/routes/admin/__tests__/AdminExecutions.test.tsfrontend/src/routes/admin/__tests__/AdminSagas.test.tsfrontend/src/routes/admin/__tests__/AdminUsers.test.tsfrontend/vitest.config.tsfrontend/vitest.setup.ts
💤 Files with no reviewable changes (4)
- frontend/src/routes/admin/tests/AdminEvents.test.ts
- frontend/src/routes/admin/tests/AdminSagas.test.ts
- frontend/src/routes/admin/tests/AdminUsers.test.ts
- frontend/src/routes/admin/tests/AdminExecutions.test.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/admin/events/tests/UserOverviewModal.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/admin/tests/ActionButtons.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (7)
frontend/src/routes/admin/__tests__/AdminExecutions.test.ts (3)
12-16: Use path alias for vi.mock.The mock uses a relative path
'../../../lib/api'instead of the path alias'$lib/api'. This violates the coding guideline requiring path aliases for all imports.As per coding guidelines: "Use path aliases ($lib, $components, $stores, $routes, $utils) for all imports; never use relative imports."
♻️ Suggested fix
-vi.mock('../../../lib/api', () => ({ +vi.mock('$lib/api', () => ({ listExecutionsApiV1AdminExecutionsGet: (...args: unknown[]) => mocks.listExecutionsApiV1AdminExecutionsGet(...args),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/__tests__/AdminExecutions.test.ts` around lines 12 - 16, The test mock uses a relative import in vi.mock instead of the required path alias; update the module specifier in the vi.mock call to use the $lib alias (e.g., replace '../../../lib/api' with '$lib/api') so the mocked exports (listExecutionsApiV1AdminExecutionsGet, updatePriorityApiV1AdminExecutionsExecutionIdPriorityPut, getQueueStatusApiV1AdminExecutionsQueueGet) are wired via the correct alias import.
27-30: Use path alias for vi.mock.Same issue - use
'$lib/api-interceptors'instead of relative path.♻️ Suggested fix
-vi.mock('../../../lib/api-interceptors', async (importOriginal) => { +vi.mock('$lib/api-interceptors', async (importOriginal) => { const actual = await importOriginal() as Record<string, unknown>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/__tests__/AdminExecutions.test.ts` around lines 27 - 30, Update the vi.mock call to use the project path alias instead of the relative path: replace the string '../../../lib/api-interceptors' in the vi.mock invocation with '$lib/api-interceptors' while keeping the async importOriginal wrapper and the spread of the actual module (the vi.mock call and its importOriginal usage should remain otherwise unchanged).
57-72: Consider adding afterEach with cleanup().The test file renders components but doesn't have an
afterEachblock withcleanup(). While@testing-library/svelteauto-cleans in most setups, the coding guidelines explicitly require callingcleanup()inafterEach.As per coding guidelines: "Frontend tests must... call cleanup() in afterEach."
♻️ Suggested fix
import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/svelte'; +import { cleanup, render, screen, waitFor } from '@testing-library/svelte'; ... describe('AdminExecutions', () => { beforeEach(() => { vi.clearAllMocks(); ... }); + + afterEach(() => { + cleanup(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/__tests__/AdminExecutions.test.ts` around lines 57 - 72, Add an afterEach that calls cleanup() for the AdminExecutions test suite: inside the describe('AdminExecutions') block add afterEach(() => cleanup()); and ensure cleanup is imported from '@testing-library/svelte' (and keep existing beforeEach mocks setup such as mocks.listExecutionsApiV1AdminExecutionsGet / mocks.getQueueStatusApiV1AdminExecutionsQueueGet / mocks.updatePriorityApiV1AdminExecutionsExecutionIdPriorityPut intact); this satisfies the guideline requiring cleanup() in afterEach.frontend/src/routes/admin/__tests__/AdminSagas.test.ts (3)
11-14: Use path alias for vi.mock.The mock uses a relative path
'../../../lib/api'instead of the path alias'$lib/api'.As per coding guidelines: "Use path aliases ($lib, $components, $stores, $routes, $utils) for all imports; never use relative imports."
♻️ Suggested fix
-vi.mock('../../../lib/api', () => ({ +vi.mock('$lib/api', () => ({ listSagasApiV1SagasGet: (...args: unknown[]) => mocks.listSagasApiV1SagasGet(...args),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/__tests__/AdminSagas.test.ts` around lines 11 - 14, Replace the relative import path in the test mock with the project path alias: update the vi.mock call so it targets '$lib/api' instead of '../../../lib/api', keeping the mock factory intact (i.e., preserve the mocked functions listSagasApiV1SagasGet and getSagaStatusApiV1SagasSagaIdGet delegating to mocks.listSagasApiV1SagasGet and mocks.getSagaStatusApiV1SagasSagaIdGet).
35-40: Consider adding afterEach with cleanup().The test file renders components but doesn't have an
afterEachblock withcleanup().As per coding guidelines: "Frontend tests must... call cleanup() in afterEach."
♻️ Suggested fix
import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/svelte'; +import { cleanup, render, screen, waitFor } from '@testing-library/svelte'; ... describe('AdminSagas', () => { beforeEach(() => { vi.clearAllMocks(); ... }); + + afterEach(() => { + cleanup(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/__tests__/AdminSagas.test.ts` around lines 35 - 40, Add an afterEach hook that calls cleanup() to ensure DOM is unmounted between tests; inside the same describe('AdminSagas', ...) block add afterEach(() => cleanup()); ensure cleanup is imported from '@testing-library/react' at the top of the test file and keep it alongside the existing beforeEach setup that mocks listSagasApiV1SagasGet and getSagaStatusApiV1SagasSagaIdGet.
16-16: Use path alias for vi.mock.Same issue - use
'$lib/api-interceptors'instead of relative path.♻️ Suggested fix
-vi.mock('../../../lib/api-interceptors'); +vi.mock('$lib/api-interceptors');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/__tests__/AdminSagas.test.ts` at line 16, Update the vi.mock call to use the project path alias instead of a relative path: replace the string passed to vi.mock in the AdminSagas.test.ts test (the vi.mock(...) invocation) with '$lib/api-interceptors' so the module uses the path alias; ensure there are no other tests in the file still importing the relative '../../../lib/api-interceptors'.frontend/src/stores/__tests__/notificationStore.test.ts (1)
39-49: Consider aligning mock reset pattern with coding guidelines.The guideline specifies
vi.clearAllMocks()inbeforeEach. Currently, you're usingmockReset()per-mock inbeforeEachandvi.clearAllMocks()inafterEach. While functionally correct (mockReset is actually more thorough), this is inverted from the standard pattern.As per coding guidelines: "Frontend tests must... call cleanup() in afterEach and vi.clearAllMocks() in beforeEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/__tests__/notificationStore.test.ts` around lines 39 - 49, Move vi.clearAllMocks() into beforeEach and remove the individual mockReset() calls (mockGetNotifications.mockReset, mockMarkRead.mockReset, mockMarkAllRead.mockReset, mockDeleteNotification.mockReset) so the top of beforeEach uses vi.clearAllMocks() (you can keep vi.resetModules() there as well); then ensure afterEach calls cleanup() (and remove vi.clearAllMocks() from afterEach). Update references in tests around beforeEach/afterEach and the mocks so they rely on vi.clearAllMocks() and cleanup() per the guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/routes/admin/__tests__/AdminExecutions.test.ts`:
- Around line 12-16: The test mock uses a relative import in vi.mock instead of
the required path alias; update the module specifier in the vi.mock call to use
the $lib alias (e.g., replace '../../../lib/api' with '$lib/api') so the mocked
exports (listExecutionsApiV1AdminExecutionsGet,
updatePriorityApiV1AdminExecutionsExecutionIdPriorityPut,
getQueueStatusApiV1AdminExecutionsQueueGet) are wired via the correct alias
import.
- Around line 27-30: Update the vi.mock call to use the project path alias
instead of the relative path: replace the string '../../../lib/api-interceptors'
in the vi.mock invocation with '$lib/api-interceptors' while keeping the async
importOriginal wrapper and the spread of the actual module (the vi.mock call and
its importOriginal usage should remain otherwise unchanged).
- Around line 57-72: Add an afterEach that calls cleanup() for the
AdminExecutions test suite: inside the describe('AdminExecutions') block add
afterEach(() => cleanup()); and ensure cleanup is imported from
'@testing-library/svelte' (and keep existing beforeEach mocks setup such as
mocks.listExecutionsApiV1AdminExecutionsGet /
mocks.getQueueStatusApiV1AdminExecutionsQueueGet /
mocks.updatePriorityApiV1AdminExecutionsExecutionIdPriorityPut intact); this
satisfies the guideline requiring cleanup() in afterEach.
In `@frontend/src/routes/admin/__tests__/AdminSagas.test.ts`:
- Around line 11-14: Replace the relative import path in the test mock with the
project path alias: update the vi.mock call so it targets '$lib/api' instead of
'../../../lib/api', keeping the mock factory intact (i.e., preserve the mocked
functions listSagasApiV1SagasGet and getSagaStatusApiV1SagasSagaIdGet delegating
to mocks.listSagasApiV1SagasGet and mocks.getSagaStatusApiV1SagasSagaIdGet).
- Around line 35-40: Add an afterEach hook that calls cleanup() to ensure DOM is
unmounted between tests; inside the same describe('AdminSagas', ...) block add
afterEach(() => cleanup()); ensure cleanup is imported from
'@testing-library/react' at the top of the test file and keep it alongside the
existing beforeEach setup that mocks listSagasApiV1SagasGet and
getSagaStatusApiV1SagasSagaIdGet.
- Line 16: Update the vi.mock call to use the project path alias instead of a
relative path: replace the string passed to vi.mock in the AdminSagas.test.ts
test (the vi.mock(...) invocation) with '$lib/api-interceptors' so the module
uses the path alias; ensure there are no other tests in the file still importing
the relative '../../../lib/api-interceptors'.
In `@frontend/src/stores/__tests__/notificationStore.test.ts`:
- Around line 39-49: Move vi.clearAllMocks() into beforeEach and remove the
individual mockReset() calls (mockGetNotifications.mockReset,
mockMarkRead.mockReset, mockMarkAllRead.mockReset,
mockDeleteNotification.mockReset) so the top of beforeEach uses
vi.clearAllMocks() (you can keep vi.resetModules() there as well); then ensure
afterEach calls cleanup() (and remove vi.clearAllMocks() from afterEach). Update
references in tests around beforeEach/afterEach and the mocks so they rely on
vi.clearAllMocks() and cleanup() per the guideline.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/__tests__/test-utils.tsfrontend/src/lib/admin/stores/__tests__/eventsStore.test.tsfrontend/src/lib/admin/stores/__tests__/executionsStore.test.tsfrontend/src/lib/admin/stores/__tests__/sagasStore.test.tsfrontend/src/routes/admin/__tests__/AdminExecutions.test.tsfrontend/src/routes/admin/__tests__/AdminSagas.test.tsfrontend/src/routes/admin/__tests__/AdminUsers.test.tsfrontend/src/stores/__tests__/notificationStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/admin/tests/AdminUsers.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/routes/__tests__/Editor.test.ts">
<violation number="1" location="frontend/src/routes/__tests__/Editor.test.ts:409">
P2: `document.createElement` is spied with a mocked implementation but never restored, so the mock can leak into later tests. Restore the spy at the end of the test (or in `afterEach`) to avoid cross-test pollution.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (11)
frontend/src/components/admin/__tests__/AutoRefreshControl.test.ts (1)
32-35: Consider addingawait tick()after render for consistency.The coding guidelines recommend using
await tick()after render. While synchronous renders may work without it, addingtick()ensures component initialization is complete and maintains consistency with the project's test patterns.♻️ Example adjustment
+import { tick } from 'svelte'; it('renders auto-refresh checkbox', async () => { renderControl(); + await tick(); expect(screen.getByText('Auto-refresh')).toBeInTheDocument(); });Apply similarly to other synchronous test cases. Alternatively, you could have
renderControlreturn a promise and awaittick()internally.As per coding guidelines: "use await tick() after render and waitFor() for async state".
Also applies to: 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/__tests__/AutoRefreshControl.test.ts` around lines 32 - 35, Add an explicit await tick() after rendering in the failing test(s): call renderControl() then await tick() before asserting; update the test at the it block containing 'renders auto-refresh checkbox' (and the similar test around the other mentioned case) to ensure component initialization completes — alternatively make renderControl return a Promise that awaits tick() internally and await that; reference functions: renderControl and tick.frontend/src/lib/__tests__/api-interceptors.test.ts (1)
105-106: Consider adding defensive assertions before accessing mock calls.Using non-null assertions (
!) onmock.calls[0]assumes the interceptors were registered. If registration fails silently, these would cause confusing errors.Suggested improvement
- errorInterceptor = mockErrorUse.mock.calls[0]![0]; - requestInterceptor = mockRequestUse.mock.calls[0]![0]; + expect(mockErrorUse.mock.calls[0]).toBeDefined(); + expect(mockRequestUse.mock.calls[0]).toBeDefined(); + errorInterceptor = mockErrorUse.mock.calls[0][0]; + requestInterceptor = mockRequestUse.mock.calls[0][0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/__tests__/api-interceptors.test.ts` around lines 105 - 106, Before dereferencing mock.calls[0] for mockErrorUse and mockRequestUse, add defensive assertions that those mocks were invoked (e.g., expect(mockErrorUse).toHaveBeenCalled() / expect(mockErrorUse.mock.calls.length).toBeGreaterThan(0) and the same for mockRequestUse) so errorInterceptor and requestInterceptor assignments (errorInterceptor = mockErrorUse.mock.calls[0][0]; requestInterceptor = mockRequestUse.mock.calls[0][0];) are safe and you can remove the non-null assertions.frontend/src/components/admin/users/__tests__/UserFormModal.test.ts (2)
28-29: AddafterEachwithcleanup()for proper test isolation.The test suite has
beforeEachbut is missing the correspondingafterEachwithcleanup()to ensure DOM cleanup between tests.♻️ Suggested refactor
+import { render, screen, cleanup } from '@testing-library/svelte'; ... describe('UserFormModal', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());As per coding guidelines: "Frontend tests must... call cleanup() in afterEach".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/UserFormModal.test.ts` around lines 28 - 29, The test suite for UserFormModal currently only calls vi.clearAllMocks() in beforeEach and lacks DOM cleanup between tests; add an afterEach that calls cleanup() (from `@testing-library/react`) to ensure proper test isolation and prevent leaked DOM, i.e., add afterEach(() => { cleanup(); }) alongside the existing beforeEach( () => vi.clearAllMocks() ) in the UserFormModal test file.
6-9: Consider hoisting the mock withvi.hoisted().Per coding guidelines, mocks should be hoisted to ensure consistent behavior across test runs, especially when mocks depend on imported utilities.
♻️ Suggested refactor
+const mockComponents = vi.hoisted(() => ({ + createMockNamedComponents: (obj: Record<string, string>) => obj, +})); + -vi.mock('$components/Spinner.svelte', async () => { - const utils = await import('$test/test-utils'); - return { default: utils.createMockNamedComponents({ default: '<span data-testid="spinner">...</span>' }).default }; -}); +vi.mock('$components/Spinner.svelte', () => ({ + default: { render: () => ({ html: '<span data-testid="spinner">...</span>' }) }, +}));As per coding guidelines: "Frontend tests must hoist mocks using vi.hoisted()".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/UserFormModal.test.ts` around lines 6 - 9, Hoist the Spinner mock by replacing the current vi.mock(...) with vi.hoisted(() => vi.mock(...)) so the mock is registered before module imports; specifically, wrap the mock for '$components/Spinner.svelte' in vi.hoisted and inside the hoisted callback import the test utils and return the same mock shape (using utils.createMockNamedComponents(...).default) so the mocked Spinner remains deterministic across runs in UserFormModal.test.ts.frontend/src/components/admin/users/__tests__/UserFilters.test.ts (1)
23-24: AddafterEachwithcleanup()for proper test isolation.♻️ Suggested refactor
+import { render, screen, cleanup } from '@testing-library/svelte'; ... describe('UserFilters', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());As per coding guidelines: "Frontend tests must... call cleanup() in afterEach".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/UserFilters.test.ts` around lines 23 - 24, The test suite for UserFilters currently only calls vi.clearAllMocks() in beforeEach; add an afterEach hook that calls cleanup() to ensure DOM and test state are torn down between tests. Locate the describe('UserFilters', ...) block (where beforeEach(() => vi.clearAllMocks()) is defined) and add afterEach(() => cleanup()) alongside it so each test run properly unmounts components and avoids cross-test leakage.frontend/src/components/admin/users/__tests__/UsersTable.test.ts (2)
20-21: AddafterEachwithcleanup()for proper test isolation.♻️ Suggested refactor
+import { render, screen, cleanup } from '@testing-library/svelte'; ... describe('UsersTable', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());As per coding guidelines: "Frontend tests must... call cleanup() in afterEach".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/UsersTable.test.ts` around lines 20 - 21, The UsersTable test suite only calls vi.clearAllMocks in beforeEach; add an afterEach that calls cleanup() to ensure DOM and side-effect isolation between tests. Update the describe('UsersTable', ...) block to include afterEach(() => cleanup()) (importing cleanup from `@testing-library/react` if not already) alongside the existing beforeEach(() => vi.clearAllMocks()) so each test run unmounts rendered components and resets the DOM.
6-8: Consider hoisting the mock withvi.hoisted().♻️ Suggested refactor
+const mockFormatters = vi.hoisted(() => ({ + formatTimestamp: vi.fn((ts: string) => `formatted:${ts}`), +})); + -vi.mock('$lib/formatters', () => ({ - formatTimestamp: vi.fn((ts: string) => `formatted:${ts}`), -})); +vi.mock('$lib/formatters', () => mockFormatters);As per coding guidelines: "Frontend tests must hoist mocks using vi.hoisted()".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/UsersTable.test.ts` around lines 6 - 8, Hoist the mock by wrapping the existing vi.mock call in vi.hoisted so the formatter stub runs before module imports: replace the current vi.mock('$lib/formatters', ...) usage in UsersTable.test.ts with a hoisted mock using vi.hoisted(() => vi.mock('$lib/formatters', { formatTimestamp: vi.fn((ts: string) => `formatted:${ts}`) }))); keep the mocked symbol name formatTimestamp and ensure this hoisted declaration sits at the top of the test file so it executes before imports.frontend/src/components/admin/users/__tests__/DeleteUserModal.test.ts (2)
6-9: Consider hoisting the mock withvi.hoisted().♻️ Suggested refactor
+vi.mock('$components/Spinner.svelte', () => ({ + default: { render: () => ({ html: '<span data-testid="spinner">...</span>' }) }, +}));As per coding guidelines: "Frontend tests must hoist mocks using vi.hoisted()".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/DeleteUserModal.test.ts` around lines 6 - 9, In DeleteUserModal.test.ts the module mock for '$components/Spinner.svelte' is not hoisted; wrap the mock registration in vi.hoisted so it runs before module imports—i.e., call vi.hoisted(() => vi.mock('$components/Spinner.svelte', async () => { const utils = await import('$test/test-utils'); return { default: utils.createMockNamedComponents({ default: '<span data-testid=\"spinner\">...</span>' }).default }; })); keeping the same factory body and references to createMockNamedComponents.
30-31: AddafterEachwithcleanup()for proper test isolation.♻️ Suggested refactor
+import { render, screen, cleanup } from '@testing-library/svelte'; ... describe('DeleteUserModal', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());As per coding guidelines: "Frontend tests must... call cleanup() in afterEach".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/DeleteUserModal.test.ts` around lines 30 - 31, The tests for DeleteUserModal currently only call vi.clearAllMocks() in beforeEach; add an afterEach that calls cleanup() to ensure proper DOM/test isolation (and keep or move vi.clearAllMocks() there if you prefer). Ensure you import cleanup from '@testing-library/react' at the top if it isn't already, and add afterEach(() => cleanup()) within the describe('DeleteUserModal') block so DOM nodes are removed between tests.frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts (2)
61-62: AddafterEachwithcleanup()for proper test isolation.♻️ Suggested refactor
+import { render, screen, waitFor, cleanup } from '@testing-library/svelte'; ... describe('RateLimitsModal', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());As per coding guidelines: "Frontend tests must... call cleanup() in afterEach".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts` around lines 61 - 62, Add an afterEach that calls cleanup() to ensure proper DOM/test isolation for the RateLimitsModal tests: import cleanup from '@testing-library/react' (if not already imported) and add afterEach(() => cleanup()) in the describe block (alongside the existing beforeEach(() => vi.clearAllMocks())). Ensure the cleanup() call is invoked in the test teardown so DOM is unmounted between tests.
6-30: Consider hoisting mocks withvi.hoisted().Multiple mocks are defined without hoisting. Consider consolidating them using
vi.hoisted()for consistency with coding guidelines.♻️ Suggested refactor pattern
+const mocks = vi.hoisted(() => ({ + getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet: vi.fn().mockResolvedValue({ + data: [ + { endpoint_pattern: '/api/v1/*', requests: 100, window_seconds: 60, group: 'general', algorithm: 'sliding_window', enabled: true }, + ], + }), + getGroupColor: vi.fn(() => 'badge-neutral'), + detectGroupFromEndpoint: vi.fn(() => 'general'), + createEmptyRule: vi.fn(() => ({ + endpoint_pattern: '', + requests: 10, + window_seconds: 60, + group: 'general', + algorithm: 'sliding_window', + enabled: true, + })), +})); + vi.mock('$components/Spinner.svelte', ...); -vi.mock('$lib/api', () => ({ - getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet: vi.fn().mockResolvedValue(...), -})); +vi.mock('$lib/api', () => ({ + getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet: mocks.getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet, +})); // Apply similar pattern to other mocksAs per coding guidelines: "Frontend tests must hoist mocks using vi.hoisted()".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts` around lines 6 - 30, Hoist the test mocks by replacing the inline vi.mock calls with a single vi.hoisted() block that returns the same mocked modules; specifically, move the mocking of '$components/Spinner.svelte', '$lib/api' (mocking getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet), and '$lib/admin/rate-limits' (mocking getGroupColor, detectGroupFromEndpoint, createEmptyRule) into a hoisted vi.hoisted() setup so they run before module imports and follow the project's frontend testing guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/admin/__tests__/AutoRefreshControl.test.ts`:
- Around line 29-30: Tests for AutoRefreshControl are missing an afterEach
cleanup call; update the test file to import cleanup from `@testing-library/react`
alongside the existing imports (refer to the import statement on line 2) and add
afterEach(() => cleanup()) after the existing beforeEach(() =>
vi.clearAllMocks()) so components are unmounted between tests; keep
vi.clearAllMocks() in beforeEach and ensure cleanup is called in afterEach.
- Around line 6-9: The mock for Spinner.svelte should be hoisted using
vi.hoisted() instead of an async factory to avoid timing issues; replace the
current vi.mock(...) async pattern with a hoisted factory that calls
vi.hoisted(() => { const utils = require('$test/test-utils'); return { default:
utils.createMockNamedComponents({ default: '<span
data-testid=\"spinner\">...</span>' }).default }; }); so the mock is initialized
before module imports — update the vi.mock call for '$components/Spinner.svelte'
to use vi.hoisted and require the test-utils synchronously.
In `@frontend/src/components/admin/sagas/__tests__/SagaDetailsModal.test.ts`:
- Around line 18-31: Update the test file to follow guidelines: replace any
vi.mock calls with vi.hoisted() for hoisted mocks, add afterEach(() =>
cleanup()) to tear down the DOM, change the render helper renderModal to be
async and await tick() before returning, and wrap all DOM assertion queries in
waitFor(...) to stabilize async checks (references: renderModal, cleanup, tick,
waitFor, and use vi.hoisted for mocks).
- Around line 1-2: Tests render DOM via render() from `@testing-library/svelte`
but never call cleanup between tests; add an import for cleanup from
'@testing-library/svelte' and register afterEach(cleanup) (or afterEach(() =>
cleanup())) alongside the existing beforeEach/vi.clearAllMocks call so the DOM
rendered by render() in SagaDetailsModal.test is removed between tests and
prevents leakage and flakiness.
- Around line 6-17: The mocks for formatTimestamp/formatDurationBetween and
getSagaStateInfo are defined inline in vi.mock; hoist them using vi.hoisted() so
the mock functions are created before module imports and then return those
hoisted references from the mock factories (referencing formatTimestamp,
formatDurationBetween, getSagaStateInfo symbols), and add an afterEach hook that
calls cleanup() from `@testing-library/svelte` to explicitly unmount between
tests; ensure the test file imports cleanup and replaces the inline vi.mock
function bodies with references to the hoisted mocks.
In `@frontend/src/components/admin/sagas/__tests__/SagaFilters.test.ts`:
- Around line 1-3: The test file is missing cleanup() in an afterEach hook and
should call vi.clearAllMocks() in beforeEach; update the import line to include
cleanup from '@testing-library/svelte' and afterEach from 'vitest', add
afterEach(() => cleanup()) and ensure beforeEach(() => { vi.clearAllMocks(); /*
keep existing setup */ }) so tests conform to the repo standard and clear DOM
and mocks between tests; locate imports near the top and the test setup hooks
around describe/it blocks (file: SagaFilters.test.ts, symbols: cleanup,
afterEach, beforeEach, vi.clearAllMocks).
- Around line 6-16: The test mock should be hoisted and test lifecycle/async
handling updated: replace the inline mock object in vi.mock with a hoisted value
via vi.hoisted(...) and reference that hoisted value in vi.mock; import and call
cleanup() inside afterEach(() => cleanup()) to ensure DOM teardown; update the
renderFilters() helper to await render(...) then await tick() so DOM updates
settle before assertions; and wrap the async expectation(s) in waitFor(...)
(from `@testing-library/svelte`) so the test waits for state changes instead of
asserting immediately.
- Line 2: Make the test helper renderFilters async and, after calling render(),
await tick() so the DOM settles; replace vi.mock('$lib/admin/sagas', ...) with a
hoisted mock via vi.hoisted() to ensure proper isolation; add an afterEach hook
that calls cleanup() and ensure beforeEach calls vi.clearAllMocks(); and update
all callsites of renderFilters (the tests that call it at the indicated
locations) to use await renderFilters() so tests wait for the settled DOM and
mocked module behavior.
In `@frontend/src/components/admin/sagas/__tests__/SagasTable.test.ts`:
- Around line 24-31: Change renderTable to an async function and await the
Svelte tick after rendering SagasTable: import tick from 'svelte' and waitFor
from '@testing-library/svelte', then inside renderTable await tick() before
returning. Update tests that call renderTable() (lines ~102–116) to await
renderTable() and replace direct expect assertions that rely on asynchronous DOM
updates with await waitFor(() => /* assertion */). Ensure references to the
helper (renderTable), the component (SagasTable), and the imported helpers
(tick, waitFor) are used as described.
- Around line 6-22: Replace the top-level mocks to use vi.hoisted() so they run
before module import (hoist the mocks for formatTimestamp/formatDurationBetween
from '$lib/formatters', getSagaStateInfo from '$lib/admin/sagas', and the
Spinner mock that imports '$test/test-utils'); add cleanup() to the test file's
afterEach hook to tear down DOM between tests; and update the renderTable helper
to call await tick() after rendering so Svelte updates are flushed (references:
formatTimestamp, formatDurationBetween, getSagaStateInfo, Spinner mock,
renderTable, and afterEach cleanup).
- Around line 33-35: Add a test cleanup to avoid DOM leakage: import cleanup
from '@testing-library/react' (or the relevant testing-library package used in
the repo) at the top of
frontend/src/components/admin/sagas/__tests__/SagasTable.test.ts and add
afterEach(() => cleanup()) (or afterEach(cleanup)) alongside the existing
beforeEach(() => vi.clearAllMocks()); ensure the import and the afterEach hook
are added so tests run cleanup between cases.
In `@frontend/src/components/admin/sagas/__tests__/SagaStatsCards.test.ts`:
- Around line 28-30: Update the test suite to be async-safe: change the helper
renderCards function to async (renderCards) and await tick() after rendering
SagaStatsCards; make each test async and wrap assertions inside await
waitFor(...) instead of immediate asserts; add an afterEach hook that calls
cleanup(); and import tick from "svelte" and waitFor and cleanup from
"@testing-library/svelte" at the top of the file so tests use proper async
rendering and cleanup.
- Around line 33-34: Add an afterEach cleanup call to ensure DOM/test isolation:
import cleanup from '@testing-library/react' if not already imported and add
afterEach(() => cleanup()) alongside the existing beforeEach(() =>
vi.clearAllMocks()) in the SagaStatsCards.test.ts test suite so each test cleans
up rendered DOM between runs.
In `@frontend/src/lib/__tests__/api-interceptors.test.ts`:
- Around line 79-81: In the test file's beforeEach block, prepend a call to
vi.clearAllMocks() so mock call history is reset before each test; i.e., inside
the existing beforeEach(...) (the one that currently calls vi.resetModules()),
add vi.clearAllMocks() as the first statement to ensure previous mock
invocations don't leak between tests.
In `@frontend/src/routes/__tests__/Editor.test.ts`:
- Around line 400-427: The test "creates download with correct filename and
extension" currently checks URL lifecycle and click but doesn't assert the
anchor's download attribute; update the test to capture the created anchor when
mocking document.createElement (inside the mockImplementation for tag === 'a')
and assert its download attribute equals the expected filename+extension (e.g.,
'my_script.py') after typing into the 'Script Name' field and triggering the
download; keep the existing mocks (mockClick, mockCreateObjectURL,
mockRevokeObjectURL) and add a final expect on the captured anchor.download to
verify filename and extension.
- Around line 409-427: The test leaks spies because only vi.unstubAllGlobals()
is called in afterEach, so restore all mocks/spies by invoking the
testing-library cleanup function in afterEach before calling
vi.unstubAllGlobals(); specifically ensure you add a call to cleanup() alongside
the existing vi.unstubAllGlobals() so spies created with vi.spyOn (e.g., the
document.createElement spy) and other mocks are fully restored between tests
rather than just clearing call history.
- Around line 329-332: The helper createFileInput is brittle because it calls
renderEditor synchronously and casts the queried node to HTMLInputElement
without a runtime check; update createFileInput to await the async
renderEditor(), query for an input and assert its type (or throw) before
returning it, and replace the direct type cast with a safe typed return. Also
wrap all assertions that follow async UI events in waitFor (imported from
`@testing-library/react`) — specifically update the first two file upload tests
and both example script loading tests to await waitFor(...) around
post-click/post-upload expectations so DOM updates are observed reliably.
---
Nitpick comments:
In `@frontend/src/components/admin/__tests__/AutoRefreshControl.test.ts`:
- Around line 32-35: Add an explicit await tick() after rendering in the failing
test(s): call renderControl() then await tick() before asserting; update the
test at the it block containing 'renders auto-refresh checkbox' (and the similar
test around the other mentioned case) to ensure component initialization
completes — alternatively make renderControl return a Promise that awaits tick()
internally and await that; reference functions: renderControl and tick.
In `@frontend/src/components/admin/users/__tests__/DeleteUserModal.test.ts`:
- Around line 6-9: In DeleteUserModal.test.ts the module mock for
'$components/Spinner.svelte' is not hoisted; wrap the mock registration in
vi.hoisted so it runs before module imports—i.e., call vi.hoisted(() =>
vi.mock('$components/Spinner.svelte', async () => { const utils = await
import('$test/test-utils'); return { default: utils.createMockNamedComponents({
default: '<span data-testid=\"spinner\">...</span>' }).default }; })); keeping
the same factory body and references to createMockNamedComponents.
- Around line 30-31: The tests for DeleteUserModal currently only call
vi.clearAllMocks() in beforeEach; add an afterEach that calls cleanup() to
ensure proper DOM/test isolation (and keep or move vi.clearAllMocks() there if
you prefer). Ensure you import cleanup from '@testing-library/react' at the top
if it isn't already, and add afterEach(() => cleanup()) within the
describe('DeleteUserModal') block so DOM nodes are removed between tests.
In `@frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts`:
- Around line 61-62: Add an afterEach that calls cleanup() to ensure proper
DOM/test isolation for the RateLimitsModal tests: import cleanup from
'@testing-library/react' (if not already imported) and add afterEach(() =>
cleanup()) in the describe block (alongside the existing beforeEach(() =>
vi.clearAllMocks())). Ensure the cleanup() call is invoked in the test teardown
so DOM is unmounted between tests.
- Around line 6-30: Hoist the test mocks by replacing the inline vi.mock calls
with a single vi.hoisted() block that returns the same mocked modules;
specifically, move the mocking of '$components/Spinner.svelte', '$lib/api'
(mocking getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet), and
'$lib/admin/rate-limits' (mocking getGroupColor, detectGroupFromEndpoint,
createEmptyRule) into a hoisted vi.hoisted() setup so they run before module
imports and follow the project's frontend testing guideline.
In `@frontend/src/components/admin/users/__tests__/UserFilters.test.ts`:
- Around line 23-24: The test suite for UserFilters currently only calls
vi.clearAllMocks() in beforeEach; add an afterEach hook that calls cleanup() to
ensure DOM and test state are torn down between tests. Locate the
describe('UserFilters', ...) block (where beforeEach(() => vi.clearAllMocks())
is defined) and add afterEach(() => cleanup()) alongside it so each test run
properly unmounts components and avoids cross-test leakage.
In `@frontend/src/components/admin/users/__tests__/UserFormModal.test.ts`:
- Around line 28-29: The test suite for UserFormModal currently only calls
vi.clearAllMocks() in beforeEach and lacks DOM cleanup between tests; add an
afterEach that calls cleanup() (from `@testing-library/react`) to ensure proper
test isolation and prevent leaked DOM, i.e., add afterEach(() => { cleanup(); })
alongside the existing beforeEach( () => vi.clearAllMocks() ) in the
UserFormModal test file.
- Around line 6-9: Hoist the Spinner mock by replacing the current vi.mock(...)
with vi.hoisted(() => vi.mock(...)) so the mock is registered before module
imports; specifically, wrap the mock for '$components/Spinner.svelte' in
vi.hoisted and inside the hoisted callback import the test utils and return the
same mock shape (using utils.createMockNamedComponents(...).default) so the
mocked Spinner remains deterministic across runs in UserFormModal.test.ts.
In `@frontend/src/components/admin/users/__tests__/UsersTable.test.ts`:
- Around line 20-21: The UsersTable test suite only calls vi.clearAllMocks in
beforeEach; add an afterEach that calls cleanup() to ensure DOM and side-effect
isolation between tests. Update the describe('UsersTable', ...) block to include
afterEach(() => cleanup()) (importing cleanup from `@testing-library/react` if not
already) alongside the existing beforeEach(() => vi.clearAllMocks()) so each
test run unmounts rendered components and resets the DOM.
- Around line 6-8: Hoist the mock by wrapping the existing vi.mock call in
vi.hoisted so the formatter stub runs before module imports: replace the current
vi.mock('$lib/formatters', ...) usage in UsersTable.test.ts with a hoisted mock
using vi.hoisted(() => vi.mock('$lib/formatters', { formatTimestamp: vi.fn((ts:
string) => `formatted:${ts}`) }))); keep the mocked symbol name formatTimestamp
and ensure this hoisted declaration sits at the top of the test file so it
executes before imports.
In `@frontend/src/lib/__tests__/api-interceptors.test.ts`:
- Around line 105-106: Before dereferencing mock.calls[0] for mockErrorUse and
mockRequestUse, add defensive assertions that those mocks were invoked (e.g.,
expect(mockErrorUse).toHaveBeenCalled() /
expect(mockErrorUse.mock.calls.length).toBeGreaterThan(0) and the same for
mockRequestUse) so errorInterceptor and requestInterceptor assignments
(errorInterceptor = mockErrorUse.mock.calls[0][0]; requestInterceptor =
mockRequestUse.mock.calls[0][0];) are safe and you can remove the non-null
assertions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/src/components/admin/__tests__/AutoRefreshControl.test.tsfrontend/src/components/admin/sagas/__tests__/SagaDetailsModal.test.tsfrontend/src/components/admin/sagas/__tests__/SagaFilters.test.tsfrontend/src/components/admin/sagas/__tests__/SagaStatsCards.test.tsfrontend/src/components/admin/sagas/__tests__/SagasTable.test.tsfrontend/src/components/admin/users/__tests__/DeleteUserModal.test.tsfrontend/src/components/admin/users/__tests__/RateLimitsModal.test.tsfrontend/src/components/admin/users/__tests__/UserFilters.test.tsfrontend/src/components/admin/users/__tests__/UserFormModal.test.tsfrontend/src/components/admin/users/__tests__/UsersTable.test.tsfrontend/src/lib/__tests__/api-interceptors.test.tsfrontend/src/routes/__tests__/Editor.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/components/admin/users/__tests__/UserFilters.test.ts (1)
1-5: Missingcleanup()inafterEach.Per coding guidelines, frontend tests should call
cleanup()inafterEachto properly unmount components between tests and prevent state leakage.♻️ Proposed fix
import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { cleanup, render, screen } from '@testing-library/svelte'; import { proxy } from 'svelte/internal/client'; import { user } from '$test/test-utils'; import UserFilters from '$components/admin/users/UserFilters.svelte';describe('UserFilters', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());Based on learnings: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/UserFilters.test.ts` around lines 1 - 5, Add proper test teardown and mock hoisting: import cleanup from '@testing-library/svelte' at the top of UserFilters.test.ts, call vi.clearAllMocks() inside the existing beforeEach, and add an afterEach(() => cleanup()) to unmount components between tests; if any mocks are declared in the file ensure they are hoisted with vi.hoisted() so they run before imports (refer to vi.hoisted in the test file and use cleanup(), beforeEach, afterEach, vi.clearAllMocks()).frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts (2)
1-10: Missingvi.hoisted()for mocks andcleanup()inafterEach.Per coding guidelines, frontend tests must hoist mocks using
vi.hoisted()and callcleanup()inafterEach. The current implementation uses inlinevi.mock()calls without hoisting, which can lead to timing issues with ES module mocking.♻️ Proposed fix to comply with test guidelines
import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/svelte'; +import { cleanup, render, screen, waitFor } from '@testing-library/svelte'; import { proxy } from 'svelte/internal/client'; import { user, createMockUser } from '$test/test-utils'; import RateLimitsModal from '$components/admin/users/RateLimitsModal.svelte'; +const mocks = vi.hoisted(() => ({ + getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet: vi.fn().mockResolvedValue({ + data: [ + { endpoint_pattern: '/api/v1/*', requests: 100, window_seconds: 60, group: 'general', algorithm: 'sliding_window', enabled: true }, + ], + }), + getGroupColor: vi.fn(() => 'badge-neutral'), + detectGroupFromEndpoint: vi.fn(() => 'general'), + createEmptyRule: vi.fn(() => ({ + endpoint_pattern: '', + requests: 10, + window_seconds: 60, + group: 'general', + algorithm: 'sliding_window', + enabled: true, + })), +})); + vi.mock('$components/Spinner.svelte', async () => { const utils = await import('$test/test-utils'); return { default: utils.createMockNamedComponents({ default: '<span data-testid="spinner">...</span>' }).default }; }); -vi.mock('$lib/api', () => ({ - getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet: vi.fn().mockResolvedValue({ - data: [ - { endpoint_pattern: '/api/v1/*', requests: 100, window_seconds: 60, group: 'general', algorithm: 'sliding_window', enabled: true }, - ], - }), -})); +vi.mock('$lib/api', () => ({ + getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet: mocks.getDefaultRateLimitRulesApiV1AdminRateLimitsDefaultsGet, +})); -vi.mock('$lib/admin/rate-limits', () => ({ - getGroupColor: vi.fn(() => 'badge-neutral'), - detectGroupFromEndpoint: vi.fn(() => 'general'), - createEmptyRule: vi.fn(() => ({ - endpoint_pattern: '', - requests: 10, - window_seconds: 60, - group: 'general', - algorithm: 'sliding_window', - enabled: true, - })), -})); +vi.mock('$lib/admin/rate-limits', () => ({ + getGroupColor: mocks.getGroupColor, + detectGroupFromEndpoint: mocks.detectGroupFromEndpoint, + createEmptyRule: mocks.createEmptyRule, +}));And add
afterEach:describe('RateLimitsModal', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());Based on learnings: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts` around lines 1 - 10, The test is missing hoisted mocks and proper test lifecycle cleanup: replace the inline vi.mock(...) for Spinner with a hoisted mock using vi.hoisted(() => ...) referencing the existing vi.mock call for '$components/Spinner.svelte' (so the mocked Spinner is hoisted before imports), add a beforeEach that calls vi.clearAllMocks() and any necessary setup, and add an afterEach that imports and calls cleanup() from '@testing-library/svelte' to unmount components after each test; update the top of RateLimitsModal.test.ts to use vi.hoisted for mocks and add the beforeEach/afterEach hooks to ensure isolation.
3-3: Replace internalproxyAPI withwritablestore or plain object (if component handles reactivity correctly).The
proxyimport fromsvelte/internal/clientis an undocumented internal API that may break on Svelte updates. For reactive test data in Svelte 5, use awritablestore instead, which is the documented pattern. Alternatively, if the component's prop interface uses$props()correctly, a plain object should work without additional reactivity wrapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts` at line 3, The test imports the undocumented internal symbol `proxy` (from svelte/internal/client); replace its usage with a documented reactive primitive or plain object: update the test in RateLimitsModal.test.ts to create reactive test data using Svelte's `writable` store (import { writable } from 'svelte/store') and pass the store value or subscribe as needed, or simply use a plain JS object if the component accepts regular props via its $props() interface; remove the `proxy` import and refactor any references to `proxy(...)` to either `writable(...)` or the plain object while ensuring the test updates use set/update on the writable when simulating reactivity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts`:
- Around line 70-74: The test named "shows spinner when loading" uses
renderModal({ loading: true, config: null }) but only asserts that "Quick
Settings" is not in the document; update the test to directly assert the spinner
is visible (or change the test name to match the current assertion). Locate the
test in RateLimitsModal.test.ts and modify the "shows spinner when loading" spec
to call renderModal({ loading: true, config: null }) and then assert the spinner
element is present (e.g., by test id, role="status", or visible loading text)
instead of only checking for "Quick Settings"; keep or remove the "Quick
Settings" negative assertion as needed. Ensure you reference the renderModal
helper and the existing "Quick Settings" assertion so the new expectation
targets the actual spinner element.
---
Nitpick comments:
In `@frontend/src/components/admin/users/__tests__/RateLimitsModal.test.ts`:
- Around line 1-10: The test is missing hoisted mocks and proper test lifecycle
cleanup: replace the inline vi.mock(...) for Spinner with a hoisted mock using
vi.hoisted(() => ...) referencing the existing vi.mock call for
'$components/Spinner.svelte' (so the mocked Spinner is hoisted before imports),
add a beforeEach that calls vi.clearAllMocks() and any necessary setup, and add
an afterEach that imports and calls cleanup() from '@testing-library/svelte' to
unmount components after each test; update the top of RateLimitsModal.test.ts to
use vi.hoisted for mocks and add the beforeEach/afterEach hooks to ensure
isolation.
- Line 3: The test imports the undocumented internal symbol `proxy` (from
svelte/internal/client); replace its usage with a documented reactive primitive
or plain object: update the test in RateLimitsModal.test.ts to create reactive
test data using Svelte's `writable` store (import { writable } from
'svelte/store') and pass the store value or subscribe as needed, or simply use a
plain JS object if the component accepts regular props via its $props()
interface; remove the `proxy` import and refactor any references to `proxy(...)`
to either `writable(...)` or the plain object while ensuring the test updates
use set/update on the writable when simulating reactivity.
In `@frontend/src/components/admin/users/__tests__/UserFilters.test.ts`:
- Around line 1-5: Add proper test teardown and mock hoisting: import cleanup
from '@testing-library/svelte' at the top of UserFilters.test.ts, call
vi.clearAllMocks() inside the existing beforeEach, and add an afterEach(() =>
cleanup()) to unmount components between tests; if any mocks are declared in the
file ensure they are hoisted with vi.hoisted() so they run before imports (refer
to vi.hoisted in the test file and use cleanup(), beforeEach, afterEach,
vi.clearAllMocks()).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/components/admin/users/__tests__/RateLimitsModal.test.tsfrontend/src/components/admin/users/__tests__/UserFilters.test.tsfrontend/src/components/admin/users/__tests__/UserFormModal.test.tsfrontend/src/lib/admin/stores/__tests__/executionsStore.test.tsfrontend/src/svelte-internal-client.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/admin/users/tests/UserFormModal.test.ts
- frontend/src/lib/admin/stores/tests/executionsStore.test.ts
|
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
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.
Summary by cubic
Add unit tests for Modal, Pagination, EventTypeIcon, ActionButtons, FilterPanel, StatsCard/StatusBadge, AutoRefreshControl, saga/user views (tables/modals), api-interceptors (initialize), editor file upload, and expanded LanguageSelect keyboard navigation.
Extract shared mocks into test-utils (incl. Spinner/admin helpers), switch tests to $ aliases, and simplify setup with global fake timers, no userEvent delays, disabled pointer checks, and a jsdom-only env; no runtime changes.
Written for commit bc82be3. Summary will update on new commits.
Summary by CodeRabbit