Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds Prettier formatting and production-build verification to frontend CI and pre-commit checks; introduces Prettier config and scripts; raises svelte button rule to error; many frontend components and tests were reformatted, with targeted functional changes to auth initialization, modal/key handling, notification stream lifecycle, accessibility, and Settings tab UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as App.svelte
participant Auth as Auth Store
participant Header as Header Component
participant Notif as NotificationCenter
User->>App: Load page
activate App
App->>Auth: initAuth onMount
activate Auth
Auth-->>App: authState
deactivate Auth
App->>Header: render(authState)
Header-->>User: show nav/menu
User->>Header: click Logout
Header->>Auth: logout()
activate Auth
Auth-->>Header: cleared
deactivate Auth
Header->>App: navigate('/login')
App-->>User: redirect to /login
deactivate App
sequenceDiagram
participant Auth as Auth Store
participant Notif as NotificationCenter
participant API as Backend (notifications)
participant UI as Notification UI
Auth->>Notif: auth changed (logged in)
Notif->>API: fetch initial notifications (limit=20)
API-->>Notif: initial list
Notif->>API: open SSE stream (connect)
API-->>Notif: SSE events (streaming)
Notif->>UI: render notifications / badge count
User->>UI: mark notification read (Enter/Space)
UI->>Notif: markAsRead -> API
Notif->>API: mark read
API-->>Notif: ack
Auth->>Notif: auth changed (logged out)
Notif->>API: disconnect SSE
Notif->>UI: clear notifications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 146 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
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/components/Modal.svelte">
<violation number="1" location="frontend/src/components/Modal.svelte:36">
P2: This adds a second Escape listener while the window Escape listener already exists, which can invoke `onClose` twice for a single keypress.</violation>
</file>
<file name="frontend/src/components/admin/events/EventsTable.svelte">
<violation number="1" location="frontend/src/components/admin/events/EventsTable.svelte:39">
P2: Parent row keyboard handler captures Space/Enter from focused child buttons, so pressing Space on an inner action can trigger `onViewDetails` instead of only the child control.</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.
Pull request overview
This PR standardizes frontend formatting/styling, adds missing unit tests, and tightens CI quality gates (coverage, type-checking, formatting, and build verification).
Changes:
- Introduces Prettier formatting (config + scripts + pre-commit + CI) and applies broad formatting cleanups across TS/Svelte/test files.
- Adds/extends unit tests for editor/admin/notifications utilities and components; enforces Vitest coverage thresholds.
- Improves UI consistency with new shared component classes (e.g., empty states/dividers) and minor UX/accessibility tweaks.
Reviewed changes
Copilot reviewed 135 out of 146 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/docker-cleanup.sh | Removes the Docker cleanup helper script. |
| frontend/vitest.config.ts | Adds global coverage thresholds for Vitest. |
| frontend/src/utils/meta.ts | Reflows page meta strings for consistent formatting. |
| frontend/src/styles/components.css | Adds shared utility classes (.divider, .empty-state, .mobile-card). |
| frontend/src/stores/userSettings.svelte.ts | Formats exported store helpers into multi-line functions. |
| frontend/src/stores/theme.svelte.ts | Formats dynamic imports/catch blocks and exported helpers. |
| frontend/src/stores/notificationStore.svelte.ts | Formatting + minor style consistency in API calls and array ops. |
| frontend/src/stores/auth.svelte.ts | Formatting + catch block expanded for clarity. |
| frontend/src/stores/tests/userSettings.test.ts | Reformats parameterized tests and imports for readability. |
| frontend/src/stores/tests/notificationStore.test.ts | Formatting updates to test structures and callbacks. |
| frontend/src/stores/tests/auth.test.ts | Formatting updates; simplifies some expectations. |
| frontend/src/routes/admin/tests/AdminUsers.test.ts | Formatting updates; adjusts assertions where native required prevents submit. |
| frontend/src/routes/admin/tests/AdminSettings.test.ts | Formatting updates for mocks and async setup. |
| frontend/src/routes/admin/tests/AdminSagas.test.ts | Formatting updates to expectations and removed blank lines. |
| frontend/src/routes/admin/tests/AdminLayout.test.ts | Formatting updates for array callbacks. |
| frontend/src/routes/admin/tests/AdminExecutions.test.ts | Formatting updates and helper signature simplification. |
| frontend/src/routes/admin/tests/AdminEvents.test.ts | Formatting updates; updates “View details” accessible name expectations. |
| frontend/src/routes/admin/AdminSagas.svelte | Import formatting + unwrapOr call formatting + minor handler style change. |
| frontend/src/routes/admin/AdminLayout.svelte | Formats long class attributes into multi-line blocks. |
| frontend/src/routes/admin/AdminExecutions.svelte | Refactors markup to shared table/form classes and adds empty-state usage. |
| frontend/src/routes/tests/Settings.test.ts | Formatting updates to mocks and test data objects. |
| frontend/src/routes/tests/Register.test.ts | Formatting updates to assertions and async flows. |
| frontend/src/routes/tests/Privacy.test.ts | Formatting updates for callbacks/indentation. |
| frontend/src/routes/tests/Notifications.test.ts | Formatting updates; consolidates some waitFor blocks. |
| frontend/src/routes/tests/Login.test.ts | Formatting updates to async mocks and expectations. |
| frontend/src/routes/tests/Home.test.ts | Formatting updates to parameterized test cases. |
| frontend/src/routes/tests/Editor.test.ts | Formatting updates; minor expression parenthesization. |
| frontend/src/routes/Register.svelte | Applies single-quote and formatting conventions; expands markup for readability. |
| frontend/src/routes/Notifications.svelte | Formatting updates + keyboard handling tweak to support Space key. |
| frontend/src/routes/Login.svelte | Applies single-quote and formatting conventions; expands markup for readability. |
| frontend/src/routes/Home.svelte | Reformats feature config and markup blocks for consistency. |
| frontend/src/main.ts | Fixes object literal formatting with trailing commas. |
| frontend/src/lib/notifications/stream.svelte.ts | Import formatting + expands for await loop into block. |
| frontend/src/lib/notifications/tests/stream.test.ts | Formatting updates for helpers and getter. |
| frontend/src/lib/formatters.ts | Expands Intl options object formatting + adds trailing commas. |
| frontend/src/lib/editor/execution.svelte.ts | Formatting updates; expands getters and ternary formatting. |
| frontend/src/lib/editor/tests/languages.test.ts | Condenses it.each into a single call formatting style. |
| frontend/src/lib/editor/tests/execution.test.ts | Formatting updates to parameterized tests and SSE helpers. |
| frontend/src/lib/api-interceptors.ts | Formatting updates; expands mapped status message + joins pipeline. |
| frontend/src/lib/admin/stores/sagasStore.svelte.ts | Import formatting + expands unwrapOr/API call formatting + filter formatting. |
| frontend/src/lib/admin/stores/executionsStore.svelte.ts | Expands unwrapOr/API call formatting + interval cleanup formatting. |
| frontend/src/lib/admin/stores/eventsStore.svelte.ts | Expands unwrapOr/API call formatting + replay/percent calc formatting. |
| frontend/src/lib/admin/stores/tests/sagasStore.test.ts | Formatting updates to inline arrays. |
| frontend/src/lib/admin/stores/tests/executionsStore.test.ts | Formatting updates to mock module exports. |
| frontend/src/lib/admin/stores/tests/eventsStore.test.ts | Formatting updates; expands JSON.stringify calls and expectations. |
| frontend/src/lib/admin/sagas/sagaStates.ts | Adds trailing commas for object entries. |
| frontend/src/lib/admin/sagas/tests/sagaStates.test.ts | Import formatting + parameterized test formatting. |
| frontend/src/lib/admin/rate-limits/rateLimits.ts | Adds trailing commas for object/array literals. |
| frontend/src/lib/admin/rate-limits/tests/rateLimits.test.ts | Import formatting + expected object formatting. |
| frontend/src/lib/admin/pagination.svelte.ts | Adds trailing commas + expands getters/setters for readability. |
| frontend/src/lib/admin/events/eventTypes.ts | Adds trailing comma to EVENT_TYPES list. |
| frontend/src/lib/admin/events/tests/eventTypes.test.ts | Formatting updates to expected arrays and parameterized tests. |
| frontend/src/lib/admin/constants.ts | Adds trailing commas to exported constants. |
| frontend/src/lib/admin/tests/pagination.test.ts | Removes stray blank line at end. |
| frontend/src/lib/admin/tests/constants.test.ts | Import formatting + removes stray blank line at end. |
| frontend/src/lib/tests/user-settings.test.ts | Formatting updates for mocks and expectations. |
| frontend/src/lib/tests/formatters.test.ts | Formats Intl option objects and consolidates expectation formatting. |
| frontend/src/lib/tests/api-interceptors.test.ts | Parameterized test formatting + adds parentheses for casts. |
| frontend/src/components/editor/tests/ScriptActions.test.ts | Adds new unit tests for ScriptActions component. |
| frontend/src/components/editor/tests/SavedScripts.test.ts | Formats inline script objects into multi-line literals. |
| frontend/src/components/editor/tests/ResourceLimits.test.ts | Removes stray blank line. |
| frontend/src/components/editor/tests/OutputPanel.test.ts | Formats test cases with nested objects. |
| frontend/src/components/editor/tests/LanguageSelect.test.ts | Removes stray blank line + formats expectations. |
| frontend/src/components/editor/tests/EditorToolbar.test.ts | Adds new unit tests for EditorToolbar component. |
| frontend/src/components/editor/tests/CodeMirrorEditor.test.ts | Formats hoisted mocks and mock classes/options. |
| frontend/src/components/editor/ScriptActions.svelte | Formats button markup blocks consistently. |
| frontend/src/components/editor/SavedScripts.svelte | Formats toggle/list markup; minor layout changes. |
| frontend/src/components/editor/ResourceLimits.svelte | Formats popover/button markup and handler expressions. |
| frontend/src/components/editor/LanguageSelect.svelte | Formats menu markup and handlers; minor expression parenthesization. |
| frontend/src/components/editor/EditorToolbar.svelte | Formats toolbar markup and attributes across lines. |
| frontend/src/components/editor/CodeMirrorEditor.svelte | Switches to single quotes in CodeMirror theme selectors + formats dispatch calls. |
| frontend/src/components/admin/users/tests/UserFormModal.test.ts | Updates test to satisfy new required fields on submit. |
| frontend/src/components/admin/users/tests/UserFilters.test.ts | Formats advancedFilters setup and option mapping. |
| frontend/src/components/admin/users/tests/RateLimitsModal.test.ts | Formats default rules mock data. |
| frontend/src/components/admin/users/tests/DeleteUserModal.test.ts | Removes stray blank line. |
| frontend/src/components/admin/users/UsersTable.svelte | Uses shared empty-state + adds spinner and improves table header semantics. |
| frontend/src/components/admin/users/UserFormModal.svelte | Adds required constraints + formatting updates. |
| frontend/src/components/admin/users/UserFilters.svelte | Formats bindables and expands markup blocks. |
| frontend/src/components/admin/users/DeleteUserModal.svelte | Formatting updates + expands delete button markup. |
| frontend/src/components/admin/sagas/tests/SagaStatsCards.test.ts | Formats hoisted mock component. |
| frontend/src/components/admin/sagas/tests/SagaFilters.test.ts | Formats option mapping in assertions. |
| frontend/src/components/admin/sagas/tests/SagaDetailsModal.test.ts | Formats formatter mocks. |
| frontend/src/components/admin/sagas/SagasTable.svelte | Uses shared empty-state + improves table header semantics. |
| frontend/src/components/admin/sagas/SagaStatsCards.svelte | Callback formatting for filters. |
| frontend/src/components/admin/sagas/SagaFilters.svelte | Minor option attribute shorthand change + trailing comma cleanup. |
| frontend/src/components/admin/sagas/SagaDetailsModal.svelte | Formats more blocks; minor markup layout changes. |
| frontend/src/components/admin/events/tests/UserOverviewModal.test.ts | Formats helper signature and removes stray blank line. |
| frontend/src/components/admin/events/tests/ReplayProgressBanner.test.ts | Formats nested session objects in tests. |
| frontend/src/components/admin/events/tests/ReplayPreviewModal.test.ts | Formats preview fixtures and removes stray blank line. |
| frontend/src/components/admin/events/tests/EventsTable.test.ts | Updates role name expectation for “View details for event”. |
| frontend/src/components/admin/events/tests/EventStatsCards.test.ts | Formats parameterized tests and nested objects. |
| frontend/src/components/admin/events/tests/EventFilters.test.ts | Formats option mapping in assertions. |
| frontend/src/components/admin/events/tests/EventDetailsModal.test.ts | Formats helper signature and removes stray blank line. |
| frontend/src/components/admin/events/UserOverviewModal.svelte | Formats profile/stats blocks for readability. |
| frontend/src/components/admin/events/ReplayProgressBanner.svelte | Formats markup and conditional rendering blocks. |
| frontend/src/components/admin/events/ReplayPreviewModal.svelte | Tightens copy + formats warning/footer buttons. |
| frontend/src/components/admin/events/EventStatsCards.svelte | Formats conditional class expression across lines. |
| frontend/src/components/admin/events/EventFilters.svelte | Formats markup and handlers; condenses button labels. |
| frontend/src/components/admin/events/EventDetailsModal.svelte | Formats table cells and JSON preview; formats footer buttons. |
| frontend/src/components/admin/tests/FilterPanel.test.ts | Formats parameterized tests. |
| frontend/src/components/admin/tests/AutoRefreshControl.test.ts | Formats option mapping in assertion. |
| frontend/src/components/admin/FilterPanel.svelte | Adds trailing commas + sets aria-expanded; formats buttons. |
| frontend/src/components/admin/AutoRefreshControl.svelte | Adds trailing commas + formats select/button markup. |
| frontend/src/components/tests/Pagination.test.ts | Formats parameterized tests and arrays. |
| frontend/src/components/tests/NotificationCenter.test.ts | Formats hoisted getters, helpers, and parameterized tests. |
| frontend/src/components/tests/ModalWrapper.svelte | Condenses prop destructuring for test wrapper. |
| frontend/src/components/tests/Header.test.ts | Formats mocks/helpers and compresses long assertions. |
| frontend/src/components/tests/Footer.test.ts | Formats external link filtering and assertions. |
| frontend/src/components/tests/EventTypeIcon.test.ts | Fixes formatting/indentation in comparison assertion. |
| frontend/src/components/tests/ErrorDisplay.test.ts | Fixes object literal formatting with trailing comma. |
| frontend/src/components/Spinner.svelte | Formats prop destructuring; compacts SVG shape markup. |
| frontend/src/components/Pagination.svelte | Adds aria-label for page size select; minor layout formatting. |
| frontend/src/components/NotificationCenter.svelte | Uses shared Spinner component; formats async chaining and handlers. |
| frontend/src/components/Modal.svelte | Adds Escape-key close handler on the backdrop/dialog container. |
| frontend/src/components/Footer.svelte | Formats markup; keeps external link rel attributes explicit. |
| frontend/src/components/EventTypeIcon.svelte | Normalizes iconMap keys; uses valid identifier keys for snake_case types. |
| frontend/src/components/ErrorDisplay.svelte | Formats derived expressions and markup blocks for consistency. |
| frontend/src/tests/test-utils.ts | Refactors factories/arrays formatting and improves readability. |
| frontend/src/App.svelte | Normalizes quotes and route definitions; formatting cleanup. |
| frontend/package.json | Adds Prettier scripts/deps; makes check fail on warnings. |
| frontend/eslint.config.js | Makes svelte/button-has-type an error (stricter linting). |
| frontend/.prettierrc | Adds Prettier configuration for the frontend (Svelte plugin). |
| frontend/.prettierignore | Ignores build artifacts and generated API client files. |
| .pre-commit-config.yaml | Adds Prettier check hook; aligns svelte-check with CI --fail-on-warnings. |
| .github/workflows/frontend-ci.yml | Adds Prettier check + production build verification to CI. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts (1)
1-2:⚠️ Potential issue | 🟠 MajorAdd
afterEach(cleanup)to prevent cross-test DOM leakage.
beforeEachclears mocks, but DOM cleanup is missing, which violates the test standard and can cause state bleed across tests.Suggested patch
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; @@ describe('ReplayProgressBanner', () => { beforeEach(() => { vi.clearAllMocks(); }); + + afterEach(() => { + cleanup(); + });As per coding guidelines: "Call
cleanup()in VitestafterEach,vi.clearAllMocks()inbeforeEach".Also applies to: 35-37
🤖 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 1 - 2, Tests are missing DOM cleanup between runs; import cleanup from '@testing-library/svelte' and add afterEach(cleanup) alongside the existing beforeEach setup (which should call vi.clearAllMocks()) so the DOM is torn down after each test and prevents cross-test leakage; reference the test file's setup where beforeEach and vi are used and add afterEach(cleanup) there, and ensure the import list (render, screen, ...) includes cleanup.frontend/src/lib/__tests__/user-settings.test.ts (1)
3-33:⚠️ Potential issue | 🟠 MajorHoist test mocks with
vi.hoisted()beforevi.mock()declarations.Mock functions and objects are currently declared as top-level constants, then used in
vi.mock()calls. Move all mock declarations into avi.hoisted(() => ...)block to ensure they are properly hoisted and available to the mock factory functions.Suggested pattern
-const mockGetUserSettings = vi.fn(); -const mockUpdateUserSettings = vi.fn(); +const { mockGetUserSettings, mockUpdateUserSettings, mockSetUserSettings, mockSetTheme, mockAuthStore } = + vi.hoisted(() => ({ + mockGetUserSettings: vi.fn(), + mockUpdateUserSettings: vi.fn(), + mockSetUserSettings: vi.fn(), + mockSetTheme: vi.fn(), + mockAuthStore: { isAuthenticated: true as boolean | null }, + })); ... -const mockSetUserSettings = vi.fn(); ... -const mockSetTheme = vi.fn(); ... -const mockAuthStore = { - isAuthenticated: true as boolean | null, -};As per coding guidelines:
frontend/src/**/__tests__/**/*.test.{ts,tsx}must hoist mocks withvi.hoisted()beforevi.mock()declarations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/__tests__/user-settings.test.ts` around lines 3 - 33, The top-level mock variables (mockGetUserSettings, mockUpdateUserSettings, mockSetUserSettings, mockSetTheme, mockAuthStore) are not hoisted and thus may be undefined inside vi.mock factories; wrap all mock declarations in a vi.hoisted(() => { ... }) block so they are created before any vi.mock() calls, then update the existing vi.mock factories (the ones referencing getUserSettingsApiV1UserSettingsGet, updateUserSettingsApiV1UserSettingsPut, setUserSettings, setTheme, and authStore) to use those hoisted mocks (leave factory bodies the same), ensuring you keep mock names unchanged so references remain valid.frontend/src/components/__tests__/Pagination.test.ts (1)
1-3:⚠️ Potential issue | 🟡 MinorAdd
cleanup()inafterEachto complete test isolation setup.The file currently clears mocks in
beforeEach(line 20) but lacks DOM cleanup between tests. AddafterEachto the vitest import,cleanupto the@testing-library/svelteimport, and implement theafterEachhook as shown:Proposed fix
-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 { render, screen, cleanup } from '@testing-library/svelte'; @@ describe('Pagination', () => { 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/components/__tests__/Pagination.test.ts` around lines 1 - 3, Add afterEach to the vitest import and import cleanup from '@testing-library/svelte', then implement an afterEach hook that calls cleanup() (and optionally vi.clearAllMocks() if you prefer clearing mocks there) to ensure DOM is reset between tests; reference the existing beforeEach and use the afterEach, cleanup, and vi.clearAllMocks symbols when making the change.frontend/src/routes/Settings.svelte (1)
91-96:⚠️ Potential issue | 🟠 MajorGuard can race auth initialization before settings load.
This mount-time check reads
authStore.isAuthenticatedimmediately without awaiting initialization. If auth verification is in progress, settings can be skipped for an eventually authenticated user.🔒 Suggested fix
- onMount(() => { - if (!authStore.isAuthenticated) { - return; - } - loadSettings(); - }); + onMount(() => { + void (async () => { + await authStore.waitForInit(); + if (!authStore.isAuthenticated) return; + await loadSettings(); + })(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Settings.svelte` around lines 91 - 96, The onMount check reads authStore.isAuthenticated immediately and can race auth initialization; change the onMount handler to wait for auth initialization or reactively subscribe to authStore and call loadSettings once authStore.isAuthenticated becomes true (or when an authStore.isInitialized flag flips), instead of skipping loadSettings on the immediate check—update the onMount block, the authStore access, and ensure loadSettings is invoked when the store updates (e.g., via authStore.subscribe or an awaitable init method) so authenticated users always load settings.frontend/src/components/__tests__/Spinner.test.ts (1)
1-3:⚠️ Potential issue | 🟡 MinorAdd required Vitest lifecycle hooks for test cleanup and mock management.
The suite is missing the standardized
beforeEachandafterEachhooks required by project test conventions. Multiplerender()calls throughout the tests (lines 10, 26, 33, 47, 52, 59, 69, 80, 89, 97) need proper cleanup between test runs.🔧 Suggested patch
-import { describe, it, expect } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; import Spinner from '$components/Spinner.svelte'; describe('Spinner', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + const getSpinner = () => screen.getByRole('status');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/Spinner.test.ts` around lines 1 - 3, Add Vitest lifecycle hooks to ensure test isolation: import beforeEach and afterEach from 'vitest' (and import cleanup from '@testing-library/svelte' if not already) in Spinner.test.ts, then add a beforeEach that calls vi.restoreAllMocks() and vi.clearAllMocks() and an afterEach that calls cleanup() to ensure every render() of Spinner is cleaned up between tests; reference the existing render/screen/Spinner usages so hooks run for all tests in this file.frontend/src/components/editor/OutputPanel.svelte (1)
10-18:⚠️ Potential issue | 🟡 MinorUse
interface Propsfor$props()instead of an inline object shape.Lines 10–18 use an inline object type annotation, which violates the project's convention of extracting prop types to a named interface.
Proposed fix
+ interface Props { + phase: ExecutionPhase; + result: ExecutionResult | null; + error: string | null; + } + let { phase, result, error, - }: { - phase: ExecutionPhase; - result: ExecutionResult | null; - error: string | null; - } = $props(); + }: Props = $props();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/OutputPanel.svelte` around lines 10 - 18, Extract the inline prop type into a named interface called Props and use that for the $props() annotation: create interface Props { phase: ExecutionPhase; result: ExecutionResult | null; error: string | null; } and then change the destructuring from the inline annotation to use Props (i.e., let { phase, result, error }: Props = $props()). Update any imports/exports if needed and ensure ExecutionPhase and ExecutionResult types remain referenced correctly.frontend/src/components/admin/events/ReplayProgressBanner.svelte (1)
18-23:⚠️ Potential issue | 🟠 MajorAdd an explicit accessible name to the icon-only close button.
The button is icon-only;
titleis not a reliable accessible name. Addaria-labelso screen readers consistently announce the action.Suggested patch
<button type="button" onclick={onClose} + aria-label="Close replay progress banner" class="absolute top-2 right-2 p-1 hover:bg-blue-100 dark:hover:bg-blue-800 rounded-lg transition-colors" title="Close" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/ReplayProgressBanner.svelte` around lines 18 - 23, The icon-only close button in the ReplayProgressBanner.svelte component relies on title for accessibility; add an explicit accessible name by adding an aria-label (e.g., aria-label="Close" or a localized equivalent) to the <button> that uses onclick={onClose}, so screen readers consistently announce the action instead of relying on title alone.frontend/src/routes/admin/AdminUsers.svelte (1)
151-189:⚠️ Potential issue | 🟠 MajorEnsure
savingUseris reset on all failure paths.At Line 160,
savingUseris set totrue, but it’s only reset at Line 185. If any awaited call throws before that point, the modal can stay stuck in a loading/disabled state.Suggested fix
async function saveUser(): Promise<void> { @@ - savingUser = true; - - let result; - - if (editingUser) { - const updateData: UserUpdate = { - username: userForm.username, - email: userForm.email || null, - role: userForm.role, - is_active: userForm.is_active, - }; - if (userForm.password) updateData.password = userForm.password; - result = await updateUserApiV1AdminUsersUserIdPut({ - path: { user_id: editingUser.user_id }, - body: updateData, - }); - } else { - result = await createUserApiV1AdminUsersPost({ - body: { - username: userForm.username, - email: userForm.email, - password: userForm.password, - role: userForm.role, - is_active: userForm.is_active, - }, - }); - } - - savingUser = false; - - unwrap(result); - showUserModal = false; - await loadUsers(); + savingUser = true; + try { + let result; + + if (editingUser) { + const updateData: UserUpdate = { + username: userForm.username, + email: userForm.email || null, + role: userForm.role, + is_active: userForm.is_active, + }; + if (userForm.password) updateData.password = userForm.password; + result = await updateUserApiV1AdminUsersUserIdPut({ + path: { user_id: editingUser.user_id }, + body: updateData, + }); + } else { + result = await createUserApiV1AdminUsersPost({ + body: { + username: userForm.username, + email: userForm.email, + password: userForm.password, + role: userForm.role, + is_active: userForm.is_active, + }, + }); + } + + unwrap(result); + showUserModal = false; + await loadUsers(); + } finally { + savingUser = false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/AdminUsers.svelte` around lines 151 - 189, The saveUser function sets savingUser = true but only clears it after awaited calls succeed, so any thrown error leaves the UI stuck; wrap the API/unwrap/loadUsers flow in a try/catch/finally inside saveUser (or at minimum use a finally block) to ensure savingUser is set to false regardless of failures, keep showUserModal=false and loadUsers/unwrap inside the successful path (try) and handle or rethrow errors in catch, and reference saveUser, savingUser, updateUserApiV1AdminUsersUserIdPut, createUserApiV1AdminUsersPost, unwrap, showUserModal, and loadUsers to locate where to add the try/catch/finally.frontend/src/components/admin/sagas/__tests__/SagaFilters.test.ts (1)
1-2:⚠️ Potential issue | 🟠 MajorAdd
cleanup()inafterEachfor this test suite.The test file renders DOM elements via
renderFilters()in multiple tests but lacks cleanup teardown. Whilevi.clearAllMocks()is correctly placed inbeforeEach, the missingafterEachcleanup can cause test pollution.Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; @@ describe('SagaFilters', () => { 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/components/admin/sagas/__tests__/SagaFilters.test.ts` around lines 1 - 2, Add a teardown step to this test suite by calling cleanup() in an afterEach block so DOM rendered by renderFilters() is unmounted between tests; keep the existing vi.clearAllMocks() in beforeEach and add afterEach(() => cleanup()) (or the equivalent cleanup import) near the top of the file to prevent test pollution from leftover DOM nodes and listeners.frontend/src/components/admin/__tests__/AutoRefreshControl.test.ts (1)
1-2:⚠️ Potential issue | 🟠 MajorAdd
cleanup()inafterEachfor consistent DOM teardown.
beforeEachclearing is present (line 30), butcleanup()teardown is missing.Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; @@ describe('AutoRefreshControl', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());As per coding guidelines: Call
cleanup()in VitestafterEach,vi.clearAllMocks()inbeforeEach.🤖 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 1 - 2, Import cleanup from '@testing-library/svelte' and add an afterEach hook that calls cleanup() to ensure consistent DOM teardown; locate the existing beforeEach and ensure it calls vi.clearAllMocks() (use vi.clearAllMocks() inside beforeEach) so test mocks are reset, and reference the symbols import { beforeEach, afterEach, vi } and the cleanup function to guide placement.frontend/src/components/admin/sagas/__tests__/SagaStatsCards.test.ts (1)
1-2:⚠️ Potential issue | 🟡 MinorAdd
afterEach(cleanup)to complete test lifecycle hygiene.This suite clears mocks in
beforeEachbut does not clean up rendered DOM. Tests callingrender()andrerender()require cleanup between runs to prevent DOM leakage.Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; @@ describe('SagaStatsCards', () => { beforeEach(() => vi.clearAllMocks()); + afterEach(() => cleanup());Per coding guidelines:
Call cleanup() in Vitest afterEach, 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/sagas/__tests__/SagaStatsCards.test.ts` around lines 1 - 2, The test suite is missing DOM cleanup between tests; import cleanup from '@testing-library/svelte' and add afterEach(cleanup) alongside the existing beforeEach(() => vi.clearAllMocks()) to ensure render()/rerender() calls don't leak DOM between tests by invoking cleanup after each test.frontend/src/routes/__tests__/Notifications.test.ts (1)
1-2:⚠️ Potential issue | 🟡 MinorAdd
afterEach(cleanup)to prevent cross-test DOM leakage.The suite resets mocks in
beforeEach, but does not clean rendered DOM between tests. Multiplerender()calls across test cases can leave DOM artifacts.Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte'; @@ describe('Notifications', () => { beforeEach(() => { vi.clearAllMocks(); @@ }); + afterEach(() => cleanup());Per coding guidelines:
Call cleanup() in Vitest afterEach, vi.clearAllMocks() in beforeEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/__tests__/Notifications.test.ts` around lines 1 - 2, Add a test-level cleanup step to avoid DOM leakage: call cleanup() in an afterEach hook (afterEach(cleanup)) so every render from render() is removed between tests, and ensure mocks are cleared in the beforeEach by calling vi.clearAllMocks() (or add it if missing) alongside existing setup; locate the test suite using describe/it/beforeEach imports in Notifications.test.ts and add these lifecycle hooks.
🟡 Minor comments (23)
frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts-28-31 (1)
28-31:⚠️ Potential issue | 🟡 MinorMake the render helper async and await
tick()after render.The helper returns before Svelte microtasks flush. Per test conventions in this repo, it should
await tick()and callers should await the helper.Based on learnings: "Applies to frontend/src//tests//*.test.{ts,tsx} : Use
await tick()after Svelte render,waitFor()for async state in Vitest".🤖 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 28 - 31, The render helper renderBanner currently returns immediately after calling render(ReplayProgressBanner...), which means Svelte microtasks may not have flushed; make renderBanner async, call render as before, then await tick() before returning (preserving and returning the onClose vi.fn and render result) so callers can await the helper and rely on flushed Svelte state.frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts-35-43 (1)
35-43:⚠️ Potential issue | 🟡 MinorSynchronize post-render assertions with
await tick().Project test rules require
await tick()after Svelte render. Consider makingrenderModalasync and awaiting it in tests to reduce timing flakes.Suggested patch
import { user } from '$test/test-utils'; +import { tick } from 'svelte'; @@ -function renderModal(overrides: Partial<{ preview: ReplayPreview | null; open: boolean }> = {}) { +async function renderModal(overrides: Partial<{ preview: ReplayPreview | null; open: boolean }> = {}) { @@ - return { ...result, onClose, onConfirm }; + await tick(); + return { ...result, onClose, onConfirm }; } @@ - it('renders nothing when closed', () => { - renderModal({ open: false }); + it('renders nothing when closed', async () => { + await renderModal({ open: false }); @@ - it('renders nothing visible when preview is null', () => { - renderModal({ preview: null }); + it('renders nothing visible when preview is null', async () => { + await renderModal({ preview: null });Based on learnings: “Use
await tick()after Svelte render,waitFor()for async state in Vitest.”Also applies to: 50-117
🤖 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 35 - 43, The tests are missing the required await tick() after Svelte render which causes timing flakes; make renderModal async, call render(ReplayPreviewModal, ...) then await tick(), and update tests to await renderModal(...) so post-render assertions are synchronized; reference the renderModal helper, the ReplayPreviewModal component, and the onClose/onConfirm mock props when making these changes.frontend/src/components/__tests__/Modal.test.ts-7-9 (1)
7-9:⚠️ Potential issue | 🟡 MinorMissing
cleanup()call inafterEachhook.The file includes
vi.clearAllMocks()inbeforeEachbut lacks a correspondingafterEachhook withcleanup(). As per coding guidelines, Vitest tests must callcleanup()inafterEachto prevent test pollution and memory leaks.🧹 Proposed fix to add cleanup
-import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { render, screen, fireEvent } from '@testing-library/svelte'; +import { cleanup } from '@testing-library/svelte'; import { user } from '$test/test-utils'; import ModalWrapper from './ModalWrapper.svelte'; describe('Modal', () => { beforeEach(() => { vi.clearAllMocks(); }); + + afterEach(() => { + cleanup(); + });Based on learnings: "Call
cleanup()in VitestafterEach,vi.clearAllMocks()inbeforeEach"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/Modal.test.ts` around lines 7 - 9, Add an afterEach hook that calls cleanup() to avoid test pollution; currently only beforeEach runs vi.clearAllMocks(), so add afterEach(() => cleanup()) alongside the existing beforeEach that invokes vi.clearAllMocks() in Modal.test.ts so DOM and side effects are cleaned between tests.frontend/src/components/Modal.svelte-29-37 (1)
29-37:⚠️ Potential issue | 🟡 MinorAvoid double-calling
onCloseon Escape.Line 29 and Line 36 both handle Escape, so a single key press can invoke
onClose()twice via bubbling + window listener. Keep only one Escape handler (prefer the window one) or stop propagation in the inner handler.Suggested fix
<svelte:window onkeydown={(e) => open && e.key === 'Escape' && onClose()} /> {`#if` open} <div class="modal-backdrop" transition:fade={{ duration: 150 }} onclick={handleBackdropClick} - onkeydown={(e) => e.key === 'Escape' && onClose()} role="dialog" aria-modal="true" aria-labelledby="modal-title" tabindex="-1" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Modal.svelte` around lines 29 - 37, The Escape key is handled twice: by the svelte:window onkeydown handler and by the modal div's onkeydown, causing onClose() to run twice; fix by removing the inner onkeydown={(e) => e.key === 'Escape' && onClose()} from the modal div (or alternatively make the inner handler call e.stopPropagation() before invoking onClose), ensuring only the svelte:window onkeydown triggers onClose; update references in Modal.svelte where svelte:window onkeydown, the modal div role="dialog" element, and handleBackdropClick/onClose are used.frontend/src/lib/admin/stores/sagasStore.svelte.ts-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorSplit mixed value/type import on Line 1.
This mixes a value import and a type-only import in one statement. Please split it into separate imports.
Suggested fix
-import { listSagasApiV1SagasGet, type SagaStatusResponse } from '$lib/api'; +import { listSagasApiV1SagasGet } from '$lib/api'; +import type { SagaStatusResponse } from '$lib/api';As per coding guidelines:
Use import type for type-only imports — never mix type and value imports in single import statement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/admin/stores/sagasStore.svelte.ts` at line 1, The import currently mixes a value and a type: split the statement so the runtime import for the API function (listSagasApiV1SagasGet) stays in a normal import and the type-only import (SagaStatusResponse) uses an "import type" declaration; update the top of the file to have one import for listSagasApiV1SagasGet and a separate "import type { SagaStatusResponse }" import so the type-only symbol is not imported as a runtime value.frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts-6-13 (1)
6-13:⚠️ Potential issue | 🟡 Minor
cancelledis missing from explicit “all expected states” coverage.The suite claims full-state coverage but skips explicit
cancelledassertions in these checks. That can let regressions slip through.Suggested patch
it('has all expected states', () => { expect(SAGA_STATES.created).toBeDefined(); expect(SAGA_STATES.running).toBeDefined(); expect(SAGA_STATES.compensating).toBeDefined(); expect(SAGA_STATES.completed).toBeDefined(); expect(SAGA_STATES.failed).toBeDefined(); expect(SAGA_STATES.timeout).toBeDefined(); + expect(SAGA_STATES.cancelled).toBeDefined(); }); it('has correct labels', () => { expect(SAGA_STATES.created.label).toBe('Created'); expect(SAGA_STATES.running.label).toBe('Running'); + expect(SAGA_STATES.compensating.label).toBe('Compensating'); expect(SAGA_STATES.completed.label).toBe('Completed'); expect(SAGA_STATES.failed.label).toBe('Failed'); + expect(SAGA_STATES.timeout.label).toBe('Timeout'); + expect(SAGA_STATES.cancelled.label).toBe('Cancelled'); });Also applies to: 24-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts` around lines 6 - 13, The test "has all expected states" is missing an explicit assertion for SAGA_STATES.cancelled; update the test(s) that enumerate expected states (the block referencing SAGA_STATES.created/running/compensating/completed/failed/timeout) to also assert expect(SAGA_STATES.cancelled).toBeDefined(); and apply the same addition to the other similar assertion block later in the file (the second occurrence around the other test covering states) so the suite explicitly covers the cancelled state.frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorAdd standard Vitest isolation hooks for this test file.
Please add
beforeEach(() => vi.clearAllMocks())andafterEach(() => cleanup())to keep test isolation consistent as this suite evolves.As per coding guidelines, "Call `cleanup()` in Vitest `afterEach`, `vi.clearAllMocks()` in `beforeEach`."Suggested patch
-import { describe, it, expect } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { cleanup } from '@testing-library/svelte'; import { SAGA_STATES, getSagaStateInfo } from '$lib/admin/sagas/sagaStates'; describe('sagaStates', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + describe('SAGA_STATES', () => {Also applies to: 4-5
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts` around lines 1 - 3, Add Vitest isolation hooks to this test file: import beforeEach, afterEach, and vi from 'vitest' (e.g. import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest') and import cleanup from '@testing-library/svelte'; then add beforeEach(() => vi.clearAllMocks()) and afterEach(() => cleanup()) at the top-level of the test file (the one importing SAGA_STATES and getSagaStateInfo) so mocks and DOM are reset between tests.frontend/src/routes/__tests__/Settings.test.ts-79-83 (1)
79-83:⚠️ Potential issue | 🟡 MinorAdd explicit
cleanup()and post-rendertick()in test scaffolding.Line 79 currently unstubs globals only. Add DOM cleanup in
afterEachand awaittick()after render to ensure proper test isolation and Svelte reactivity synchronization.🔧 Proposed patch
-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte'; +import { tick } from 'svelte'; @@ - afterEach(() => vi.unstubAllGlobals()); + afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + }); @@ - function renderSettings() { - return render(Settings); + async function renderSettings() { + const utils = render(Settings); + await tick(); + return utils; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/__tests__/Settings.test.ts` around lines 79 - 83, The test teardown only calls vi.unstubAllGlobals() and the render helper returns render(Settings) without awaiting Svelte microtasks; update the afterEach block to also call cleanup() to unmount DOM between tests and change renderSettings() to await render(Settings) followed by awaiting tick() so Svelte reactive updates settle; reference the existing afterEach, vi.unstubAllGlobals(), renderSettings(), and the Settings component when making these edits.frontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.ts-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorAdd the required Vitest isolation hooks in this test file.
This suite is missing the required
beforeEach/afterEachpattern for mock reset and cleanup as per frontend test standards.🔧 Suggested patch
-import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { cleanup } from '@testing-library/svelte'; import type { EndpointGroup } from '$lib/api'; import { getGroupColor, detectGroupFromEndpoint, createEmptyRule } from '$lib/admin/rate-limits/rateLimits'; const EXPECTED_GROUPS: EndpointGroup[] = ['execution', 'admin', 'sse', 'websocket', 'auth', 'api', 'public']; describe('rateLimits', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + describe('getGroupColor', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.ts` around lines 1 - 4, Add Vitest isolation hooks to reset and restore mocks and cleanup between tests: import beforeEach and afterEach (and vi if not already available) from 'vitest' alongside describe/it/expect, then add beforeEach(() => { vi.resetAllMocks(); vi.clearAllMocks(); }) and afterEach(() => { vi.restoreAllMocks(); }); if the test suite uses DOM testing-library utilities, also call cleanup() from '@testing-library/svelte' in afterEach. Apply these changes near the top of the file where describe/it/expect are imported — this will ensure tests exercising getGroupColor, detectGroupFromEndpoint, and createEmptyRule run in isolation.frontend/src/lib/formatters.ts-59-78 (1)
59-78:⚠️ Potential issue | 🟡 MinorGuard
formatDurationagainst non-finite inputs.
formatDuration(NaN)andformatDuration(Infinity)currently produce invalid output (e.g.,"NaNh","Infinityh") instead of returning'N/A'. The issue occurs becauseNaN < 0andInfinity < 0both evaluate tofalse, bypassing the guard checks.🛠️ Proposed fix
export function formatDuration(seconds: number | null | undefined): string { - if (seconds === null || seconds === undefined) return 'N/A'; - if (seconds < 0) return 'N/A'; + if (seconds === null || seconds === undefined) return 'N/A'; + if (!Number.isFinite(seconds) || seconds < 0) return 'N/A'; if (seconds < 1) { return `${Math.round(seconds * 1000)}ms`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/formatters.ts` around lines 59 - 78, formatDuration currently returns strange strings for non-finite inputs (NaN, Infinity); add an early guard that treats any non-finite numeric input as invalid by checking Number.isFinite(seconds) (after the null/undefined check) and returning 'N/A' when it fails, then proceed with existing logic; update the function around the initial guards that include the null/undefined and negative checks to include this finite-number check so NaN/Infinity don't flow into the later formatting branches.frontend/src/lib/formatters.ts-133-137 (1)
133-137:⚠️ Potential issue | 🟡 MinorHandle
truncateboundary values for very smallmaxLength.The current implementation fails for
maxLength ≤ 2: it produces output exceeding the requested max length. For example,truncate('hello world', 2)returns a 13-character string instead of 2 characters. This occurs becausestr.slice(0, maxLength - 3)produces problematic results whenmaxLength < 3. While current usage in the codebase specifiesmaxLength = 12, the function should handle these edge cases gracefully.🛠️ Proposed fix
export function truncate(str: string | null | undefined, maxLength = 50): string { - if (!str) return ''; + if (!str || maxLength <= 0) return ''; if (str.length <= maxLength) return str; + if (maxLength <= 3) return '.'.repeat(maxLength); return `${str.slice(0, maxLength - 3)}...`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/formatters.ts` around lines 133 - 137, The truncate function breaks for maxLength ≤ 2 because maxLength - 3 becomes negative; change the logic in truncate(str, maxLength) so it always returns a string no longer than maxLength: if str is falsy return '', if str.length <= maxLength return str, else if maxLength <= 3 return str.slice(0, maxLength) (no ellipsis), otherwise return `${str.slice(0, maxLength - 3)}...`. Update the behavior in the truncate function to use these branches so the output length is never larger than maxLength.frontend/src/components/editor/__tests__/SavedScripts.test.ts-21-34 (1)
21-34:⚠️ Potential issue | 🟡 MinorAdd standard test lifecycle hooks and await post-render tick.
This test file lacks
beforeEach/afterEachhygiene hooks and does not awaittick()after rendering, allowing mock state and DOM residue to leak between tests.Add imports for
afterEach,cleanup, andtick:import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { render, screen, cleanup } from '@testing-library/svelte'; +import { tick } from 'svelte';Make
renderScriptsasync and awaittick()post-render:-function renderScripts(scripts: SavedScriptResponse[] = []) { +async function renderScripts(scripts: SavedScriptResponse[] = []) { const onload = vi.fn(); const ondelete = vi.fn(); const onrefresh = vi.fn(); const result = render(SavedScripts, { props: { scripts, onload, ondelete, onrefresh } }); + await tick(); return { ...result, onload, ondelete, onrefresh }; }Add lifecycle hooks to the describe block and await
renderScripts:describe('SavedScripts', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + afterEach(() => { + cleanup(); + }); + async function renderAndExpand(scripts: SavedScriptResponse[] = []) { - const result = renderScripts(scripts); + const result = await renderScripts(scripts); await user.click(screen.getByRole('button', { name: /Show Saved Scripts/i })); return result; }Update all direct
renderScripts()calls throughout the describe block toawait renderScripts().🤖 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 21 - 34, The tests leak DOM/mocks because renderScripts and callers don't await the post-render tick and there's no test cleanup; import afterEach, cleanup and tick from your test utils, make renderScripts async and await tick() after calling render(...), add lifecycle hooks inside the describe block (use afterEach(() => cleanup()) and optionally beforeEach to reset mocks if needed), and update all renderScripts() and renderAndExpand() calls to await renderScripts(...) so the DOM settles before interacting with it.frontend/src/components/admin/events/__tests__/EventFilters.test.ts-9-21 (1)
9-21:⚠️ Potential issue | 🟡 MinorAlign test lifecycle and render timing with repo test conventions.
This test file is missing
cleanup()in anafterEachhook and does not await a post-rendertick(), both required by the frontend test guidelines. TherenderFiltershelper must be async and awaittick()after rendering; all test call sites must await the helper.🛠️ Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; +import { tick } from 'svelte'; import { user } from '$test/test-utils'; import { EVENT_TYPES } from '$lib/admin/events/eventTypes'; import type { EventFilter } from '$lib/api'; import EventFilters from '$components/admin/events/EventFilters.svelte'; -function renderFilters(overrides: Partial<{ onApply: () => void; onClear: () => void; filters: EventFilter }> = {}) { +async function renderFilters( + overrides: Partial<{ onApply: () => void; onClear: () => void; filters: EventFilter }> = {}, +) { const onApply = overrides.onApply ?? vi.fn(); const onClear = overrides.onClear ?? vi.fn(); const filters: EventFilter = overrides.filters ?? {}; const result = render(EventFilters, { props: { filters, onApply, onClear } }); + await tick(); return { ...result, onApply, onClear }; } describe('EventFilters', () => { beforeEach(() => { vi.clearAllMocks(); }); + afterEach(() => { + cleanup(); + });Update all test call sites to
await renderFilters()(lines 23, 38, 44, 51, 57, 63).🤖 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 9 - 21, Tests don't follow repo test lifecycle: make renderFilters async, await render and then await tick() inside it (function name: renderFilters) and update all tests that call it to use await renderFilters(); also add an afterEach hook that calls cleanup() (and vi.clearAllMocks() can remain in beforeEach) so the DOM is torn down between tests and post-render updates are awaited per guidelines.frontend/src/components/__tests__/Header.test.ts-78-103 (1)
78-103:⚠️ Potential issue | 🟡 MinorAdd required test isolation hooks (
vi.clearAllMocksandcleanup).
beforeEach/afterEachcurrently miss the repository-required cleanup pattern, which can leak mock and DOM state across tests.Proposed fix
-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte'; @@ beforeEach(() => { + vi.clearAllMocks(); setAuth(false); mocks.mockThemeStore.value = 'auto'; @@ afterEach(() => { + cleanup(); Object.defineProperty(window, 'innerWidth', { writable: true, configurable: true, value: originalInnerWidth }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/Header.test.ts` around lines 78 - 103, The tests lack required isolation: add vi.clearAllMocks() at the start of beforeEach and call cleanup() in afterEach to reset mocked state and DOM between tests; specifically update the beforeEach block (where setAuth, mocks.mockThemeStore, vi.spyOn(router, 'goto'), and matchMedia are configured) to invoke vi.clearAllMocks() before other setup, and update the afterEach block that restores window.innerWidth to also call cleanup() to unmount DOM and listeners.frontend/src/components/editor/__tests__/OutputPanel.test.ts-35-39 (1)
35-39:⚠️ Potential issue | 🟡 MinorAdd
afterEach(cleanup)for rendered component tests.The test suite renders the component in 14+ test cases across multiple nested describe blocks. DOM cleanup is missing, which can cause test isolation issues. While
vi.clearAllMocks()is correctly placed inbeforeEach, the corresponding DOM cleanup inafterEachis required.Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; @@ beforeEach(() => { vi.clearAllMocks(); vi.spyOn(toast, 'success'); vi.spyOn(toast, 'error'); }); + + afterEach(() => { + cleanup(); + });🤖 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 35 - 39, Add DOM cleanup after each test in this suite by calling afterEach(cleanup) alongside the existing beforeEach; import cleanup from '@testing-library/react' at the top if not present, then add afterEach(cleanup) in the same scope as the existing beforeEach block (where vi.clearAllMocks() and vi.spyOn(toast, 'success'|'error') are set) to ensure rendered components are unmounted between tests.frontend/src/components/__tests__/Header.test.ts-66-74 (1)
66-74:⚠️ Potential issue | 🟡 MinorAdd
await tick()after render in interaction helpers.These helpers call
render(Header)immediately followed by user interaction without allowing Svelte effects to settle. Addawait tick()after eachrender()call to ensure component state is ready before interaction.Proposed fix
-import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { tick } from 'svelte'; import { render, screen, waitFor } from '@testing-library/svelte'; const openUserDropdown = async () => { render(Header); + await tick(); await user.click(screen.getByRole('button', { name: 'User menu' })); }; const openMobileMenu = async () => { render(Header); + await tick(); await user.click(screen.getByRole('button', { name: 'Open menu' })); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/Header.test.ts` around lines 66 - 74, Both helpers call render(Header) then interact immediately; add "await tick()" right after each render(Header) in openUserDropdown and openMobileMenu so Svelte's microtask queue settles before user.click, and ensure tick is imported from 'svelte' at the top of the test file if not already present.frontend/src/components/editor/__tests__/CodeMirrorEditor.test.ts-108-112 (1)
108-112:⚠️ Potential issue | 🟡 MinorAdd
afterEach(cleanup)to prevent DOM leakage between tests.This suite renders components via
renderEditor()and clears mocks inbeforeEach, but it missing testing-library cleanup. The test file usesrender()multiple times (lines 114, 122, 134, 146, 157, 166, 173) which mounts components to the DOM and requires cleanup after each test.Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, waitFor } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, waitFor, cleanup } from '@testing-library/svelte'; @@ beforeEach(() => { vi.clearAllMocks(); document.documentElement.classList.remove('dark'); }); + + afterEach(() => { + cleanup(); + });🤖 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 108 - 112, The test suite is leaking DOM between tests because it never calls testing-library's cleanup; import cleanup from '@testing-library/react' and add an afterEach(cleanup) call in this file (alongside the existing beforeEach that calls vi.clearAllMocks and removes the 'dark' class) so every render created by renderEditor()/render() is unmounted and the DOM is reset between tests (add the import and the afterEach invocation near the top of the file where beforeEach is defined).frontend/src/routes/admin/AdminSagas.svelte-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorSplit type-only import from value import.
Line 3 mixes a type and value in a single import statement. Type-only imports must be separated.
Suggested patch
-import { getSagaStatusApiV1SagasSagaIdGet, type SagaStatusResponse } from '$lib/api'; +import { getSagaStatusApiV1SagasSagaIdGet } from '$lib/api'; +import type { SagaStatusResponse } from '$lib/api';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/AdminSagas.svelte` at line 3, Split the mixed import so value and type are separate: keep the runtime function getSagaStatusApiV1SagasSagaIdGet in a regular import and move SagaStatusResponse into a type-only import using the "import type" form; update the import lines that reference getSagaStatusApiV1SagasSagaIdGet and SagaStatusResponse in AdminSagas.svelte accordingly so the value import and the type-only import are distinct.frontend/src/components/admin/users/__tests__/UserFilters.test.ts-29-29 (1)
29-29:⚠️ Potential issue | 🟡 MinorAdd
afterEach(cleanup)to complete lifecycle hygiene.This test suite clears mocks in
beforeEachbut is missing the correspondingafterEach(cleanup)call. Since the file renders a Svelte component multiple times across tests, cleanup must be called after each test to avoid DOM state leakage.Add the import:
import { cleanup } from '@testing-library/svelte';and add the hook:afterEach(cleanup);after thebeforeEachcall on line 29.🤖 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` at line 29, The test suite currently calls vi.clearAllMocks() in beforeEach but never performs DOM cleanup; add an import for cleanup from '@testing-library/svelte' and register afterEach(cleanup) alongside the existing beforeEach to ensure Svelte components are unmounted between tests; update the top-level imports to include cleanup and add afterEach(cleanup); next to the existing beforeEach(() => vi.clearAllMocks()) so the test lifecycle runs cleanup after each test.frontend/src/routes/__tests__/Register.test.ts-25-37 (1)
25-37:⚠️ Potential issue | 🟡 MinorAdd
cleanup()inafterEachand makerenderRegister()async withawait tick().Frontend test lifecycle requires cleanup after each test and explicit tick handling for Svelte renders.
Proposed patch
-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte'; +import { tick } from 'svelte'; @@ describe('Register', () => { beforeEach(() => { vi.clearAllMocks(); mocks.registerApiV1AuthRegisterPost.mockResolvedValue({ data: {}, error: undefined }); vi.spyOn(toast, 'success'); vi.spyOn(toast, 'error'); vi.spyOn(toast, 'warning'); vi.spyOn(toast, 'info'); vi.spyOn(meta, 'updateMetaTags'); }); + + afterEach(() => cleanup()); - function renderRegister() { - return render(Register); + async function renderRegister() { + const view = render(Register); + await tick(); + return view; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/__tests__/Register.test.ts` around lines 25 - 37, Tests are missing teardown and proper async render handling: add an afterEach that calls cleanup() and vi.clearAllMocks() (or similar) to restore DOM between tests, and change renderRegister() to be async returning await render(Register) followed by await tick() so Svelte updates settle; update references to the existing beforeEach setup (mocks.registerApiV1AuthRegisterPost, vi.spyOn(...) and meta.updateMetaTags) to remain unchanged, only adding the new afterEach and making the renderRegister helper async with await tick() after render.frontend/src/components/admin/users/__tests__/UsersTable.test.ts-21-21 (1)
21-21:⚠️ Potential issue | 🟡 MinorAdd
afterEach(cleanup)for test isolation.
beforeEachis correctly configured, but this suite should also cleanup mounted DOM after each test to ensure proper test isolation.Proposed patch
-import { render, screen } from '@testing-library/svelte'; +import { render, screen, cleanup } from '@testing-library/svelte'; @@ describe('UsersTable', () => { 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/components/admin/users/__tests__/UsersTable.test.ts` at line 21, Add an afterEach(cleanup) call to this test suite to unmount DOM between tests: locate the test file containing beforeEach(() => vi.clearAllMocks()) in UsersTable.test.ts and add afterEach(() => cleanup()) (or afterEach(cleanup) depending on your test utils import) near the top-level describe so the DOM is cleaned up after each test; ensure cleanup is imported from the testing library if not already.frontend/src/routes/__tests__/Editor.test.ts-136-136 (1)
136-136:⚠️ Potential issue | 🟡 MinorImport and call
cleanup()inafterEachto prevent cross-test DOM leakage.Add
cleanupto the import from@testing-library/svelteand call it in theafterEachhook before unstubbing globals.Proposed patch
-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte'; @@ -afterEach(() => vi.unstubAllGlobals()); +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/__tests__/Editor.test.ts` at line 136, The test teardown currently only calls vi.unstubAllGlobals() in afterEach; also import cleanup from '@testing-library/svelte' and call cleanup() as the first step inside afterEach (before vi.unstubAllGlobals()) to unmount Svelte components and prevent DOM leakage between tests; update the import list to include cleanup and modify the afterEach hook that references vi.unstubAllGlobals() to invoke cleanup() first.frontend/src/components/admin/sagas/__tests__/SagaStatsCards.test.ts-69-79 (1)
69-79:⚠️ Potential issue | 🟡 MinorWait for DOM update after
rerenderto avoid flaky assertions.The expectation runs immediately after
rerender(Line 78). UsewaitFor(ortick) to synchronize with async DOM updates.Proposed fix
-import { render, screen } from '@testing-library/svelte'; +import { render, screen, waitFor } from '@testing-library/svelte'; @@ - it('updates counts when sagas change', () => { + it('updates counts when sagas change', async () => { const { rerender } = renderCards([createMockSaga({ state: 'running' })]); expect(screen.getByText('1')).toBeInTheDocument(); - rerender({ + await rerender({ sagas: [ createMockSaga({ saga_id: 's1', state: 'completed' }), createMockSaga({ saga_id: 's2', state: 'completed' }), ], }); - expect(screen.getByText('2')).toBeInTheDocument(); + await waitFor(() => { + expect(screen.getByText('2')).toBeInTheDocument(); + }); });Based on learnings:
Use await tick() after Svelte render, waitFor() for async state in Vitest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/sagas/__tests__/SagaStatsCards.test.ts` around lines 69 - 79, The test "updates counts when sagas change" performs assertions immediately after calling rerender which can be flaky; update the test to wait for the DOM to reflect the new sagas by awaiting an async synchronization helper (e.g., use waitFor from `@testing-library/svelte/`@testing-library/react or await tick() if using Svelte's tick) after calling rerender({ sagas: [...] }) before asserting screen.getByText('2'), keeping the existing helpers renderCards and createMockSaga and the same assertions but wrapped in an async test function that awaits the DOM update.
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
Adds Prettier formatting and stricter checks to the frontend, and fills in missing unit tests while polishing UI and accessibility. This improves CI reliability, code consistency, and test coverage.
Refactors
Dependencies
Written for commit f178f2e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Code Quality