feat: misc changes for quicker build and deploy times#236
Merged
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:
📝 WalkthroughWalkthroughSwitches backend to a BuildKit multi-stage Docker build, reformats CI docker invocation, expands backend build-context ignores, removes test Dockerfile, trims backend deps, moves frontend build tooling to devDependencies and disables prod sourcemaps, and centralizes frontend test user-event helpers used across many tests. Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions (CI)
participant Build as Docker BuildKit
participant Repo as Repository
participant Registry as GHCR
GHA->>Build: trigger docker/build-push-action@v6 (build-contexts, cache)
Build->>Repo: read `backend/Dockerfile.base` (builder + runtime stages)
Build->>Build: run builder stage (install build deps, uv sync with cache)
Build->>Build: assemble runtime stage (copy venv/uv, install runtime libs)
Build->>Registry: push images (concurrent pushes, PID tracking)
Registry-->>GHA: return push results (success/failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/Dockerfile.base (1)
35-40: Avoidapt-get upgradein the runtime image build step.Line 35 makes builds slower and less reproducible. Prefer refreshed base images plus explicit package installs only.
Suggested diff
-RUN apt-get update && apt-get upgrade -y \ +RUN apt-get update \ && apt-get install -y --no-install-recommends \ curl \ libsnappy1v5 \ liblzma5 \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/Dockerfile.base` around lines 35 - 40, Remove the apt-get upgrade step in the Dockerfile RUN line to avoid slow, non-reproducible builds: change the RUN command that currently contains "apt-get update && apt-get upgrade -y \ ..." to only run "apt-get update && apt-get install -y --no-install-recommends curl libsnappy1v5 liblzma5 && rm -rf /var/lib/apt/lists/*" so the image relies on the refreshed base image plus explicit package installs (preserve --no-install-recommends and the cleanup at the end).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/Dockerfile.base`:
- Around line 35-40: Remove the apt-get upgrade step in the Dockerfile RUN line
to avoid slow, non-reproducible builds: change the RUN command that currently
contains "apt-get update && apt-get upgrade -y \ ..." to only run "apt-get
update && apt-get install -y --no-install-recommends curl libsnappy1v5 liblzma5
&& rm -rf /var/lib/apt/lists/*" so the image relies on the refreshed base image
plus explicit package installs (preserve --no-install-recommends and the cleanup
at the end).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/uv.lockis excluded by!**/*.lockfrontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/stack-tests.ymlbackend/.dockerignorebackend/Dockerfile.basebackend/Dockerfile.testbackend/pyproject.tomldocs/architecture/frontend-build.mddocs/operations/cicd.mdfrontend/package.jsonfrontend/rollup.config.js
💤 Files with no reviewable changes (2)
- backend/Dockerfile.test
- backend/pyproject.toml
There was a problem hiding this comment.
Pull request overview
This pull request optimizes build and deploy times through several changes: disabling sourcemaps in production builds, moving frontend build tools to devDependencies, removing unused backend dependencies, implementing multi-stage Docker builds for the backend base image, and adding GHA layer caching for the backend image. However, there is a critical issue where the async-asgi-testclient dependency was removed but is still actively used in SSE endpoint tests.
Changes:
- Frontend: Disabled sourcemaps in production to reduce bundle size and build time
- Backend: Removed unused dependencies (tiktoken, opentelemetry-instrumentation-asgi, opentelemetry-instrumentation-redis) and the test dependency async-asgi-testclient
- Docker: Implemented multi-stage build for Dockerfile.base to separate build-time and runtime dependencies, removed Dockerfile.test, enhanced .dockerignore
- CI/CD: Added GHA layer caching for backend image builds, updated documentation to reflect 4 images instead of 5
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/rollup.config.js | Conditionally disable sourcemaps in production (!production) |
| frontend/package.json | Move build tools (rollup, plugins, postcss, etc.) from dependencies to devDependencies |
| frontend/package-lock.json | Update dev markers for moved dependencies |
| backend/pyproject.toml | Remove unused dependencies: tiktoken, opentelemetry-instrumentation-asgi, opentelemetry-instrumentation-redis, async-asgi-testclient |
| backend/uv.lock | Reflect dependency removals in lock file |
| backend/Dockerfile.base | Implement multi-stage build with separate builder and runtime stages |
| backend/Dockerfile.test | Remove unused test dockerfile |
| backend/.dockerignore | Add exclusions for tests, docs, configs to reduce build context |
| .github/workflows/stack-tests.yml | Add GHA layer caching for backend image with scope=backend, mode=max |
| docs/operations/cicd.md | Update to 4 images (was 5), document GHA caching strategy |
| docs/architecture/frontend-build.md | Clarify that sourcemaps are only in development builds |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HardMax71
changed the title
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 (7)
frontend/src/routes/__tests__/Settings.test.ts (1)
80-80:⚠️ Potential issue | 🟡 Minor
afterEachis missingcleanup().The
afterEachalready callsvi.unstubAllGlobals(), butcleanup()is absent, leaving rendered components mounted between tests.🛠️ Proposed fix
-import { describe, it, expect, beforeEach, afterEach, 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(() => vi.unstubAllGlobals()); + afterEach(() => { + vi.unstubAllGlobals(); + 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/routes/__tests__/Settings.test.ts` at line 80, The test teardown is incomplete: ensure rendered components are unmounted by calling cleanup() in the afterEach block and reset mocks at test start by calling vi.clearAllMocks() in beforeEach; update the test file to call cleanup() alongside vi.unstubAllGlobals() in the existing afterEach and add a beforeEach that invokes vi.clearAllMocks() so tests don't leak DOM or mock state (refer to afterEach, beforeEach, vi.unstubAllGlobals, cleanup, vi.clearAllMocks).frontend/src/components/editor/__tests__/ResourceLimits.test.ts (1)
1-9:⚠️ Potential issue | 🟡 MinorMissing both
beforeEach(() => vi.clearAllMocks())andafterEach(() => cleanup()).Unlike the other migrated test files,
ResourceLimits.test.tshas nobeforeEachorafterEachhooks at all, violating both halves of the guideline.🛠️ Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; import { user } from '$test/test-utils'; ... describe('ResourceLimits', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + afterEach(() => { + cleanup(); + }); + it('renders nothing when limits is null', ...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/components/editor/__tests__/ResourceLimits.test.ts` around lines 1 - 9, Add the missing test setup/teardown: import cleanup from '@testing-library/svelte' and add a beforeEach hook that calls vi.clearAllMocks() and an afterEach hook that calls cleanup(); place these near the top of ResourceLimits.test.ts so the suite resets mocks and DOM between tests (referencing beforeEach, afterEach, vi.clearAllMocks, and cleanup to locate where to add them).frontend/src/components/editor/__tests__/LanguageSelect.test.ts (1)
1-28: 🛠️ Refactor suggestion | 🟠 MajorAdd
beforeEach/afterEachwithvi.clearAllMocks()andcleanup().This suite has no
beforeEachorafterEach, violating the guideline requiringvi.clearAllMocks()inbeforeEachandcleanup()inafterEach. Without cleanup the DOM accumulates renderedLanguageSelectcomponents across tests. As per coding guidelines, both hooks are mandatory.🛠️ Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen, within, fireEvent, waitFor } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, within, fireEvent, waitFor, cleanup } from '@testing-library/svelte'; import { user } from '$test/test-utils';describe('LanguageSelect', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + async function openMenu() {🤖 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 1 - 28, Add test lifecycle hooks to this test file: import cleanup from '@testing-library/svelte' at the top, then add a beforeEach that calls vi.clearAllMocks() and an afterEach that calls cleanup(); place these hooks at the top-level of the suite (e.g., inside the describe('LanguageSelect', ...) block or file-level) so they run around tests that use renderSelect and openMenu, ensuring mocks are reset and the DOM is cleaned between tests.frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts (1)
25-28: 🛠️ Refactor suggestion | 🟠 MajorAdd
afterEachwithcleanup().
beforeEachclears mocks correctly, but there is noafterEachto unmount rendered components. As per coding guidelines,cleanup()must be called inafterEach.🛠️ Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte';beforeEach(() => { vi.clearAllMocks(); }); + + 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__/EventDetailsModal.test.ts` around lines 25 - 28, The test suite for EventDetailsModal currently only clears mocks in beforeEach but never unmounts rendered components; add an afterEach hook that calls cleanup() to unmount and clean up DOM between tests (e.g., add afterEach(() => { cleanup(); }) alongside the existing beforeEach that calls vi.clearAllMocks()) so rendered components are properly torn down after each test.frontend/src/routes/__tests__/Notifications.test.ts (1)
1-43: 🛠️ Refactor suggestion | 🟠 MajorAdd
afterEachwithcleanup().There is no
afterEachin this suite. Withoutcleanup(), eachrender()call accumulates DOM nodes across tests, causing cross-test bleed. As per coding guidelines,cleanup()must be called inafterEach.🛠️ Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { render, screen, waitFor } from '@testing-library/svelte'; +import { cleanup } from '@testing-library/svelte'; import { createMockNotification, createMockNotifications, user } from '$test/test-utils';beforeEach(() => { vi.clearAllMocks(); ... }); + + afterEach(() => { + cleanup(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/__tests__/Notifications.test.ts` around lines 1 - 43, The test suite is missing cleanup() after each test which causes DOM nodes to accumulate; import cleanup from '@testing-library/svelte' at the top alongside render/screen/waitFor and add an afterEach(() => cleanup()) block (keeping the existing beforeEach and vi.clearAllMocks) so each test's DOM is torn down; reference the test file Notifications.test.ts and its beforeEach and render usages to locate where to add the import and the afterEach call.frontend/src/components/__tests__/NotificationCenter.test.ts (1)
152-172: 🛠️ Refactor suggestion | 🟠 Major
beforeEachshould callvi.clearAllMocks()andafterEachshould callcleanup().Two guideline violations:
beforeEachuses individual.mockReset()calls but does not callvi.clearAllMocks().vi.restoreAllMocks()inafterEachrestores spies, but mock state set up betweenbeforeEachand the test's own mocks won't be reliably cleared withoutvi.clearAllMocks()inbeforeEach.afterEachcallsvi.restoreAllMocks()but never callscleanup(), leaving renderedNotificationCenterinstances mounted in the DOM.As per coding guidelines, both hooks are required.
🛠️ Proposed fix
-import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, cleanup } from '@testing-library/svelte';beforeEach(() => { + vi.clearAllMocks(); mocks.mockAuthStore.isAuthenticated = true; ... }); - afterEach(() => { vi.restoreAllMocks(); }); + afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/NotificationCenter.test.ts` around lines 152 - 172, The test setup should clear all mock state at the start and unmount rendered components at the end: in the beforeEach block (where mocks.mockAuthStore and others are initialized and many .mockReset() calls occur) add a call to vi.clearAllMocks() to ensure mock histories are cleared before each test, and in the afterEach block (which currently calls vi.restoreAllMocks()) add a call to cleanup() to unmount any rendered NotificationCenter instances; keep existing vi.restoreAllMocks() and console spy handling intact.frontend/src/components/__tests__/Header.test.ts (1)
60-79: 🛠️ Refactor suggestion | 🟠 Major
beforeEachshould callvi.clearAllMocks()andafterEachshould callcleanup().Two guideline violations:
beforeEachindividually calls.mockReset()on select mocks but doesn't callvi.clearAllMocks(), which would also reset any spies created elsewhere in the suite.afterEachonly restoreswindow.innerWidthbut never callscleanup(), leaving mountedHeadercomponents in the DOM between tests.As per coding guidelines, both
vi.clearAllMocks()inbeforeEachandcleanup()inafterEachare required.🛠️ Proposed fix
-import { describe, it, expect, beforeEach, afterEach, 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';beforeEach(() => { + vi.clearAllMocks(); setAuth(false); mocks.mockThemeStore.value = 'auto'; - mocks.mockAuthStore.logout.mockReset(); - mocks.mockToggleTheme.mockReset(); - mocks.mockGoto.mockReset(); originalInnerWidth = window.innerWidth; ... }); afterEach(() => { Object.defineProperty(window, 'innerWidth', { writable: true, configurable: true, value: originalInnerWidth }); + cleanup(); });🤖 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 60 - 79, Add vi.clearAllMocks() at the start of the beforeEach block and call cleanup() in afterEach so all mocks/spies and mounted components are fully reset between tests; update the existing beforeEach that currently calls mocks.mockAuthStore.logout.mockReset() and others to also invoke vi.clearAllMocks(), and add cleanup() alongside restoring window.innerWidth in the afterEach that currently only resets originalInnerWidth.
♻️ Duplicate comments (4)
frontend/src/components/admin/events/__tests__/EventFilters.test.ts (1)
1-3: Missingcleanup()inafterEach.Same guideline violation as noted in
ReplayPreviewModal.test.ts— please addcleanup()to anafterEachblock here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/__tests__/EventFilters.test.ts` around lines 1 - 3, The test file EventFilters.test.ts is missing an explicit cleanup call after each test; import cleanup from '@testing-library/svelte' and add an afterEach(() => cleanup()) alongside the existing beforeEach so the DOM is torn down between tests (locate the describe block and the existing beforeEach in EventFilters.test.ts to add the import and afterEach).frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts (1)
1-3: Missingcleanup()inafterEach.Same guideline violation — add
cleanup()inafterEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts` around lines 1 - 3, The test file ReplayProgressBanner.test.ts is missing a cleanup step after each test; import cleanup from '@testing-library/svelte' alongside render and screen and add an afterEach(() => cleanup()) (or afterEach(cleanup)) to ensure DOM is cleared between tests; update the top-level imports and add the afterEach hook in the test file so tests using render/screen/user are properly isolated.frontend/src/components/admin/events/__tests__/EventsTable.test.ts (1)
1-3: Missingcleanup()inafterEach.Same guideline violation — add
cleanup()inafterEach.🤖 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 1 - 3, Add testing-library's cleanup to the test teardown: import cleanup from '@testing-library/svelte' in EventsTable.test.ts and add an afterEach hook that calls cleanup (e.g., afterEach(() => cleanup()) or afterEach(cleanup)) so DOM is reset between tests; ensure the new afterEach is placed with the existing beforeEach/vi setup so it always runs after each test.frontend/src/components/editor/__tests__/OutputPanel.test.ts (1)
1-3: Missingcleanup()inafterEach.Same guideline violation — add
cleanup()inafterEach.🤖 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 1 - 3, Add a call to cleanup() in the test file's afterEach to ensure DOM is torn down between tests: import cleanup from '@testing-library/svelte' (or include it in the existing import from '@testing-library/svelte') and call cleanup() inside the afterEach block in frontend/src/components/editor/__tests__/OutputPanel.test.ts (the file containing the describe/it tests), so the test suite cleans up rendered components between cases.
🧹 Nitpick comments (2)
frontend/src/components/__tests__/NotificationCenter.test.ts (2)
303-308: Redundant localtimerUseralias — useuserWithTimersdirectly.
const timerUser = userWithTimers(lines 304 and 343) simply re-aliases the already-importeduserWithTimers. The local variable adds noise without value; useuserWithTimersdirectly at the call sites.♻️ Proposed refactor
- const timerUser = userWithTimers; render(NotificationCenter); - await timerUser.click(screen.getByRole('button', { name: /Notifications/i })); + await userWithTimers.click(screen.getByRole('button', { name: /Notifications/i })); screen.getByRole('button', { name: /View notification: Test/i }).focus(); - await timerUser.keyboard('{Tab}'); + await userWithTimers.keyboard('{Tab}');Apply the same pattern for the second occurrence at lines 343–345.
Also applies to: 343-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/NotificationCenter.test.ts` around lines 303 - 308, Remove the redundant local alias "timerUser" and use the imported "userWithTimers" directly in the test; specifically delete the declarations `const timerUser = userWithTimers` and replace `timerUser.click` / `timerUser.keyboard` calls with `userWithTimers.click` / `userWithTimers.keyboard` in the NotificationCenter.test (both occurrences around the existing calls to render(NotificationCenter) and keyboard/tab interactions), leaving the rest of the assertions and setup (e.g., setNotifications, render, screen.getByRole) unchanged.
290-299:RegExpfrom variable — negligible risk in tests, but consider string matchers.The static analyser flags lines 293 and 319 for constructing
RegExpfrom a variable (subject/method), which could theoretically enable ReDoS if the input were attacker-controlled. In this test context the values are hardcoded test data, so there is no real security risk. However, replacing the dynamicRegExpwith Testing Library's built-in string matching ({ name: /View notification: .../i }with a static literal, or usingscreen.findByText) removes the warning cleanly and makes intent clearer.Also applies to: 315-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/NotificationCenter.test.ts` around lines 290 - 299, Replace the dynamic RegExp name match in the test with a plain string matcher to avoid constructing a RegExp from the variable; in the test case that uses interactionTestCases (the it.each block that builds subject from method/hasUrl), call screen.findByRole('button', { name: `View notification: ${subject}` }) (and similarly for the other occurrence in the second block using method/subject) instead of new RegExp(...) so the test uses a direct string match; keep the rest of the flow (setNotifications, openDropdown, interactWithButton, and assertions against mocks.mockNotificationStore.markAsRead and mocks.mockGoto) 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/__tests__/test-utils.ts`:
- Around line 25-26: Exporting module-level singletons `user` and
`userWithTimers` causes shared internal keyboard/pointer state across tests;
replace them with factory functions (e.g., `createUser()` and
`createUserWithTimers()`) that call `userEvent.setup()` (and `userEvent.setup({
advanceTimers: vi.advanceTimersByTime })`) so each test can create a fresh
instance in beforeEach; update tests to declare a `let user: UserEventInstance`
and assign `user = createUser()` (or `createUserWithTimers()`) in beforeEach to
avoid cross-test pollution.
In `@frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts`:
- Around line 1-3: Add test cleanup and mock reset scaffolding: import cleanup
from '@testing-library/svelte' and use vi.hoisted() for any top-level mocks, add
beforeEach(() => { vi.clearAllMocks(); }) and afterEach(() => { cleanup(); }) in
this test file (ReplayPreviewModal.test.ts) — apply the same pattern to the
other affected files (EventFilters.test.ts, EventsTable.test.ts,
ReplayProgressBanner.test.ts, OutputPanel.test.ts, ResourceLimits.test.ts,
Settings.test.ts); reference the existing imports (describe, beforeEach, vi,
render) and the test harness utilities (user, screen) to place the new
beforeEach/afterEach blocks alongside them.
In `@frontend/src/components/editor/__tests__/SavedScripts.test.ts`:
- Around line 32-36: In renderAndExpand, after calling renderScripts(...) (which
calls render(SavedScripts,...)) await Svelte's tick before interacting: add
"await tick()" immediately after the renderScripts call and before the
user.click; if tick is not imported in this test file, import it from 'svelte'
(ensure renderAndExpand uses await tick() to let Svelte 5's reactive graph
settle before calling user.click on the "Show Saved Scripts" button).
- Around line 1-5: Add test lifecycle hooks to reset mocks and DOM between
tests: call vi.clearAllMocks() in a beforeEach hook and call cleanup() in an
afterEach hook (import cleanup from `@testing-library/svelte` if not present).
Also ensure the mock for '@lucide/svelte' is hoisted using vi.hoisted() per your
testing guidelines and pair renders that use render(...) and the shared user
singleton with await tick()/waitFor() where needed; reference the existing
top-level vi.mock(...) and the shared user from '$test/test-utils' when making
these additions.
---
Outside diff comments:
In `@frontend/src/components/__tests__/Header.test.ts`:
- Around line 60-79: Add vi.clearAllMocks() at the start of the beforeEach block
and call cleanup() in afterEach so all mocks/spies and mounted components are
fully reset between tests; update the existing beforeEach that currently calls
mocks.mockAuthStore.logout.mockReset() and others to also invoke
vi.clearAllMocks(), and add cleanup() alongside restoring window.innerWidth in
the afterEach that currently only resets originalInnerWidth.
In `@frontend/src/components/__tests__/NotificationCenter.test.ts`:
- Around line 152-172: The test setup should clear all mock state at the start
and unmount rendered components at the end: in the beforeEach block (where
mocks.mockAuthStore and others are initialized and many .mockReset() calls
occur) add a call to vi.clearAllMocks() to ensure mock histories are cleared
before each test, and in the afterEach block (which currently calls
vi.restoreAllMocks()) add a call to cleanup() to unmount any rendered
NotificationCenter instances; keep existing vi.restoreAllMocks() and console spy
handling intact.
In `@frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts`:
- Around line 25-28: The test suite for EventDetailsModal currently only clears
mocks in beforeEach but never unmounts rendered components; add an afterEach
hook that calls cleanup() to unmount and clean up DOM between tests (e.g., add
afterEach(() => { cleanup(); }) alongside the existing beforeEach that calls
vi.clearAllMocks()) so rendered components are properly torn down after each
test.
In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts`:
- Around line 1-28: Add test lifecycle hooks to this test file: import cleanup
from '@testing-library/svelte' at the top, then add a beforeEach that calls
vi.clearAllMocks() and an afterEach that calls cleanup(); place these hooks at
the top-level of the suite (e.g., inside the describe('LanguageSelect', ...)
block or file-level) so they run around tests that use renderSelect and
openMenu, ensuring mocks are reset and the DOM is cleaned between tests.
In `@frontend/src/components/editor/__tests__/ResourceLimits.test.ts`:
- Around line 1-9: Add the missing test setup/teardown: import cleanup from
'@testing-library/svelte' and add a beforeEach hook that calls
vi.clearAllMocks() and an afterEach hook that calls cleanup(); place these near
the top of ResourceLimits.test.ts so the suite resets mocks and DOM between
tests (referencing beforeEach, afterEach, vi.clearAllMocks, and cleanup to
locate where to add them).
In `@frontend/src/routes/__tests__/Notifications.test.ts`:
- Around line 1-43: The test suite is missing cleanup() after each test which
causes DOM nodes to accumulate; import cleanup from '@testing-library/svelte' at
the top alongside render/screen/waitFor and add an afterEach(() => cleanup())
block (keeping the existing beforeEach and vi.clearAllMocks) so each test's DOM
is torn down; reference the test file Notifications.test.ts and its beforeEach
and render usages to locate where to add the import and the afterEach call.
In `@frontend/src/routes/__tests__/Settings.test.ts`:
- Line 80: The test teardown is incomplete: ensure rendered components are
unmounted by calling cleanup() in the afterEach block and reset mocks at test
start by calling vi.clearAllMocks() in beforeEach; update the test file to call
cleanup() alongside vi.unstubAllGlobals() in the existing afterEach and add a
beforeEach that invokes vi.clearAllMocks() so tests don't leak DOM or mock state
(refer to afterEach, beforeEach, vi.unstubAllGlobals, cleanup,
vi.clearAllMocks).
---
Duplicate comments:
In `@frontend/src/components/admin/events/__tests__/EventFilters.test.ts`:
- Around line 1-3: The test file EventFilters.test.ts is missing an explicit
cleanup call after each test; import cleanup from '@testing-library/svelte' and
add an afterEach(() => cleanup()) alongside the existing beforeEach so the DOM
is torn down between tests (locate the describe block and the existing
beforeEach in EventFilters.test.ts to add the import and afterEach).
In `@frontend/src/components/admin/events/__tests__/EventsTable.test.ts`:
- Around line 1-3: Add testing-library's cleanup to the test teardown: import
cleanup from '@testing-library/svelte' in EventsTable.test.ts and add an
afterEach hook that calls cleanup (e.g., afterEach(() => cleanup()) or
afterEach(cleanup)) so DOM is reset between tests; ensure the new afterEach is
placed with the existing beforeEach/vi setup so it always runs after each test.
In `@frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts`:
- Around line 1-3: The test file ReplayProgressBanner.test.ts is missing a
cleanup step after each test; import cleanup from '@testing-library/svelte'
alongside render and screen and add an afterEach(() => cleanup()) (or
afterEach(cleanup)) to ensure DOM is cleared between tests; update the top-level
imports and add the afterEach hook in the test file so tests using
render/screen/user are properly isolated.
In `@frontend/src/components/editor/__tests__/OutputPanel.test.ts`:
- Around line 1-3: Add a call to cleanup() in the test file's afterEach to
ensure DOM is torn down between tests: import cleanup from
'@testing-library/svelte' (or include it in the existing import from
'@testing-library/svelte') and call cleanup() inside the afterEach block in
frontend/src/components/editor/__tests__/OutputPanel.test.ts (the file
containing the describe/it tests), so the test suite cleans up rendered
components between cases.
---
Nitpick comments:
In `@frontend/src/components/__tests__/NotificationCenter.test.ts`:
- Around line 303-308: Remove the redundant local alias "timerUser" and use the
imported "userWithTimers" directly in the test; specifically delete the
declarations `const timerUser = userWithTimers` and replace `timerUser.click` /
`timerUser.keyboard` calls with `userWithTimers.click` /
`userWithTimers.keyboard` in the NotificationCenter.test (both occurrences
around the existing calls to render(NotificationCenter) and keyboard/tab
interactions), leaving the rest of the assertions and setup (e.g.,
setNotifications, render, screen.getByRole) unchanged.
- Around line 290-299: Replace the dynamic RegExp name match in the test with a
plain string matcher to avoid constructing a RegExp from the variable; in the
test case that uses interactionTestCases (the it.each block that builds subject
from method/hasUrl), call screen.findByRole('button', { name: `View
notification: ${subject}` }) (and similarly for the other occurrence in the
second block using method/subject) instead of new RegExp(...) so the test uses a
direct string match; keep the rest of the flow (setNotifications, openDropdown,
interactWithButton, and assertions against
mocks.mockNotificationStore.markAsRead and mocks.mockGoto) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
frontend/src/__tests__/test-utils.tsfrontend/src/components/__tests__/ErrorDisplay.test.tsfrontend/src/components/__tests__/Header.test.tsfrontend/src/components/__tests__/NotificationCenter.test.tsfrontend/src/components/admin/events/__tests__/EventDetailsModal.test.tsfrontend/src/components/admin/events/__tests__/EventFilters.test.tsfrontend/src/components/admin/events/__tests__/EventsTable.test.tsfrontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.tsfrontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.tsfrontend/src/components/editor/__tests__/LanguageSelect.test.tsfrontend/src/components/editor/__tests__/OutputPanel.test.tsfrontend/src/components/editor/__tests__/ResourceLimits.test.tsfrontend/src/components/editor/__tests__/SavedScripts.test.tsfrontend/src/routes/__tests__/Editor.test.tsfrontend/src/routes/__tests__/Login.test.tsfrontend/src/routes/__tests__/Notifications.test.tsfrontend/src/routes/__tests__/Register.test.tsfrontend/src/routes/__tests__/Settings.test.tsfrontend/src/routes/admin/__tests__/AdminEvents.test.tsfrontend/src/routes/admin/__tests__/AdminExecutions.test.tsfrontend/src/routes/admin/__tests__/AdminSagas.test.tsfrontend/src/routes/admin/__tests__/AdminSettings.test.tsfrontend/src/routes/admin/__tests__/AdminUsers.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 23 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts">
<violation number="1" location="frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts:3">
P3: Avoid using a shared `user` instance created at module scope. `userEvent.setup()` should be invoked inside each test or a setup helper so the document/device state is reset per test; importing the global instance risks cross-test state leakage and flakiness.</violation>
</file>
<file name="frontend/src/__tests__/test-utils.ts">
<violation number="1" location="frontend/src/__tests__/test-utils.ts:25">
P2: Avoid exporting a singleton user-event instance from shared utilities. Create a per-test helper that calls userEvent.setup() so tests get a fresh session and input state.</violation>
</file>
<file name="frontend/src/routes/__tests__/Register.test.ts">
<violation number="1" location="frontend/src/routes/__tests__/Register.test.ts:3">
P3: Using the shared `user` singleton from test-utils means the user-event instance (and its input state) is shared across test files. That can cause state leakage between tests and order-dependent failures when files run in the same worker. Prefer creating a fresh `userEvent.setup()` instance per test file or per test (e.g., in `beforeEach`) so input state is isolated.</violation>
</file>
<file name="frontend/src/components/__tests__/Header.test.ts">
<violation number="1" location="frontend/src/components/__tests__/Header.test.ts:3">
P2: The shared `user` instance is created at module scope and reused across all tests. Testing Library recommends calling `userEvent.setup()` inside each test/setup function to avoid shared input state between tests. Consider creating a fresh instance per test (e.g., inside `openUserDropdown`/`openMobileMenu` or each test) instead of importing the singleton.</violation>
</file>
<file name="frontend/src/routes/__tests__/Login.test.ts">
<violation number="1" location="frontend/src/routes/__tests__/Login.test.ts:3">
P2: Avoid importing the shared `user` singleton from test-utils; `userEvent.setup()` creates a stateful instance meant to be created per test/setup. Sharing one instance across test files can leak input state and cause flaky tests.</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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/routes/__tests__/Privacy.test.ts (1)
1-11:⚠️ Potential issue | 🟡 MinorAdd
afterEachwithcleanup()andvi.unstubAllGlobals().Two guideline requirements are missing from the outer scope:
cleanup()is never called — it must be called inafterEachper the project coding standards. Automatic cleanup fires only whenimport '@testing-library/svelte/vitest'is present in the Vitest setup file; relying on that without an explicit call violates the guideline regardless.vi.stubGlobalis called inbeforeEachbutvi.unstubAllGlobals()is never called, leavingwindow.scrollTostubbed after the suite finishes, which could bleed into other test files running in the same worker.🛡️ Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { render, screen } from '@testing-library/svelte'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/svelte'; import Privacy from '$routes/Privacy.svelte'; const mocks = vi.hoisted(() => ({ scrollTo: vi.fn(), })); beforeEach(() => { vi.stubGlobal('scrollTo', mocks.scrollTo); }); +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +});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/__tests__/Privacy.test.ts` around lines 1 - 11, Add an afterEach that calls cleanup() and vi.unstubAllGlobals(), and ensure mocks are reset each run: keep the existing vi.hoisted usage of mocks and the beforeEach that stubs scrollTo, but add a beforeEach call to vi.clearAllMocks() if not present and an afterEach that calls cleanup() and vi.unstubAllGlobals() so the scrollTo stub and other globals are removed and DOM is cleaned up after each test; reference the existing beforeEach, mocks (vi.hoisted), and the test file top-level setup when making the change.
🧹 Nitpick comments (1)
frontend/src/routes/__tests__/Privacy.test.ts (1)
18-23: Callawait tick()afterrender()per the testing guideline.
render(Privacy)is synchronous in@testing-library/sveltev5, soawait renderPrivacy()resolves immediately without flushing Svelte's microtask queue. The guideline requiresawait tick()after render. While Privacy is a static component and this doesn't cause observable failures today, it's an easy fix that keeps the test harness consistent and guards against regressions if the component later gains reactive state.♻️ Proposed refactor
+import { tick } from 'svelte'; ... - function renderPrivacy() { - return render(Privacy); - } + async function renderPrivacy() { + const result = render(Privacy); + await tick(); + return result; + }Based on learnings: "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/routes/__tests__/Privacy.test.ts` around lines 18 - 23, The test calls render(Privacy) via renderPrivacy() but doesn't await Svelte's microtask queue; update the test to call await tick() after rendering to flush reactivity: either add await tick() inside the helper renderPrivacy() (after render(Privacy)) or call await tick() immediately after await renderPrivacy() in the test, and ensure tick is imported from 'svelte' at the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/routes/__tests__/Privacy.test.ts`:
- Around line 1-11: Add an afterEach that calls cleanup() and
vi.unstubAllGlobals(), and ensure mocks are reset each run: keep the existing
vi.hoisted usage of mocks and the beforeEach that stubs scrollTo, but add a
beforeEach call to vi.clearAllMocks() if not present and an afterEach that calls
cleanup() and vi.unstubAllGlobals() so the scrollTo stub and other globals are
removed and DOM is cleaned up after each test; reference the existing
beforeEach, mocks (vi.hoisted), and the test file top-level setup when making
the change.
---
Nitpick comments:
In `@frontend/src/routes/__tests__/Privacy.test.ts`:
- Around line 18-23: The test calls render(Privacy) via renderPrivacy() but
doesn't await Svelte's microtask queue; update the test to call await tick()
after rendering to flush reactivity: either add await tick() inside the helper
renderPrivacy() (after render(Privacy)) or call await tick() immediately after
await renderPrivacy() in the test, and ensure tick is imported from 'svelte' at
the top of the file.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
frontend/src/routes/__tests__/Editor.test.tsfrontend/src/routes/__tests__/Privacy.test.tsfrontend/src/routes/__tests__/Settings.test.tsfrontend/src/routes/admin/__tests__/AdminEvents.test.tsfrontend/src/routes/admin/__tests__/AdminSagas.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/routes/tests/Settings.test.ts
- frontend/src/routes/tests/Editor.test.ts
There was a problem hiding this comment.
2 issues found across 26 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/routes/admin/__tests__/AdminLayout.test.ts">
<violation number="1" location="frontend/src/routes/admin/__tests__/AdminLayout.test.ts:26">
P3: These spies call through to the real `svelte-sonner` toast implementation. Without a mock/no-op implementation, the test will execute real toast side effects (DOM usage) instead of just tracking calls, which can make the suite flaky. Stub the methods (or mock the module) so the spies do not invoke the real toast behavior.</violation>
</file>
<file name="frontend/src/components/__tests__/Header.test.ts">
<violation number="1" location="frontend/src/components/__tests__/Header.test.ts:67">
P2: `vi.spyOn(router, 'goto')` leaves the real router navigation running during tests. Mock the implementation so clicks don’t trigger actual navigation side effects in jsdom.</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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/__tests__/ProtectedRoute.test.ts (2)
28-37:⚠️ Potential issue | 🟡 Minor
vi.clearAllMocks()is missing frombeforeEach.The coding guidelines require
vi.clearAllMocks()in everybeforeEach. Without it, call counts and return-value overrides from prior tests bleed into subsequent ones.waitForInit.mockReset()covers only that one mock; all othervi.fn()instances onmockAuthStore(and the spy itself) keep stale history.🛠️ Proposed fix
beforeEach(() => { + vi.clearAllMocks(); mocks.mockAuthStore.isAuthenticated = null;Based on learnings: "call
cleanup()inafterEachandvi.clearAllMocks()inbeforeEach".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/ProtectedRoute.test.ts` around lines 28 - 37, Add vi.clearAllMocks() at the start of the beforeEach setup to ensure prior test spies/mocks are fully cleared; modify the beforeEach that currently resets mocks.mockAuthStore.waitForInit and spies on router.goto so it begins with vi.clearAllMocks(), then re-create any necessary spies (e.g., vi.spyOn(router, 'goto')) and reset/mock the specific store methods (e.g., mocks.mockAuthStore.waitForInit.mockReset().mockResolvedValue(true)) to avoid stale call counts or return overrides leaking between tests.
74-82:⚠️ Potential issue | 🟡 Minor
cleanup()is missing fromafterEach.The coding guidelines mandate
cleanup()in everyafterEach. Without it, rendered Svelte components accumulate in the document between tests, which can produce false-positive DOM queries or cross-test interference.🛠️ Proposed fix
+import { render, screen, waitFor, cleanup } from '@testing-library/svelte'; ... afterEach(() => { + cleanup(); vi.restoreAllMocks();Based on learnings: "call
cleanup()inafterEachandvi.clearAllMocks()inbeforeEach".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/ProtectedRoute.test.ts` around lines 74 - 82, Tests are missing Svelte DOM cleanup and mock clearing: add a call to cleanup() inside the existing afterEach block (alongside vi.restoreAllMocks() and restoring window.sessionStorage/originalSessionStorage) and add vi.clearAllMocks() to a beforeEach so mocks are cleared before each test; update the afterEach surrounding code that references afterEach, vi.restoreAllMocks, and originalSessionStorage to include cleanup() to prevent DOM leakage between tests.
🧹 Nitpick comments (2)
frontend/vitest.config.ts (1)
18-22: MigrateenvironmentMatchGlobstoprojectsAPI —environmentMatchGlobswas deprecated in Vitest 3 and removed in Vitest 4.0.Use Vitest's
projectsAPI instead, which supports setting differentenvironmentandsetupFilesper project. This resolves the DOM/node setup file conflict by allowing you to define separatesetupFilesfor each environment.Example:
projects: [ { test: { name: 'node', include: ['src/lib/**/*.test.ts', 'src/stores/**/*.test.ts', 'src/utils/**/*.test.ts'], environment: 'node', setupFiles: ['./test/setup.node.ts'], }, }, // Add other projects as needed ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/vitest.config.ts` around lines 18 - 22, Replace the deprecated environmentMatchGlobs usage in vitest.config.ts by defining a projects array that maps the test globs to a named project with environment and setupFiles per project; specifically remove environmentMatchGlobs and add a projects entry (use the symbol projects) that contains a test object with name 'node', include set to the patterns currently in environmentMatchGlobs (e.g., 'src/lib/**/*.test.ts', 'src/stores/**/*.test.ts', 'src/utils/**/*.test.ts'), environment: 'node', and setupFiles pointing to your node setup file (setupFiles: ['./test/setup.node.ts']) so node-specific setup is applied only to that project.frontend/src/components/__tests__/ProtectedRoute.test.ts (1)
127-130: ReplacesetTimeout(resolve, 50)withawait tick()(orwaitFor) per guidelines.Raw
setTimeoutwaits are timing-dependent and can both flake (on slow CI) and waste time (on fast machines). The guideline requiresawait tick()after render for Svelte component updates.🛠️ Proposed fix
+import { tick } from 'svelte'; ... - await new Promise(resolve => setTimeout(resolve, 50)); + await tick();Based on learnings: "use
await tick()after render andwaitFor()for async state".Also applies to: 139-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/ProtectedRoute.test.ts` around lines 127 - 130, Replace the raw setTimeout waits with Svelte/testing-library utilities: import and call await tick() (from 'svelte') immediately after rendering the ProtectedRoute component to wait for Svelte updates instead of using setTimeout(resolve, 50), and for any asynchronous state assertions (e.g., where you expect router.goto calls later) wrap those expectations in waitFor (from '@testing-library/svelte') so the test waits reliably for async changes; update occurrences that reference router.goto and the setTimeout pattern accordingly (including the other similar instance in this test file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/__tests__/Header.test.ts`:
- Around line 49-58: The test helpers openUserDropdown and openMobileMenu render
the Header but miss awaiting tick(), and the test file's lifecycle hooks should
call cleanup() in afterEach and reset mocks with vi.clearAllMocks() in
beforeEach; update openUserDropdown and openMobileMenu to call await tick()
immediately after render(Header), add afterEach(() => cleanup()) (or await
cleanup() if async) and change the manual mock resets in beforeEach to
vi.clearAllMocks() so mocks are fully cleared between tests.
In `@frontend/src/components/__tests__/ProtectedRoute.test.ts`:
- Line 4: Replace the failing vi.spyOn call on the sealed external ESM namespace
by mocking the module at load time: remove vi.spyOn(router, 'goto') and add
vi.mock('@mateothegreat/svelte5-router', { spy: true }) so the spy is created by
the mock; also add vi.clearAllMocks() at the top of the existing beforeEach to
reset mock history before each test, and call cleanup() inside afterEach to
teardown DOM between tests (referencing vi.mock, vi.clearAllMocks, and cleanup).
In `@frontend/src/components/editor/__tests__/LanguageSelect.test.ts`:
- Around line 1-3: Add the missing lifecycle hooks and flush rendering in
openMenu: inside the test suite's describe block, add beforeEach(() => {
vi.clearAllMocks(); }) and afterEach(() => { cleanup(); }) so mocks are reset
and DOM cleaned between tests, and in the openMenu helper (the function that
calls renderSelect) add await tick() immediately after renderSelect() to ensure
DOM updates are flushed before interacting with the menu; reference the describe
block, beforeEach, afterEach, vi.clearAllMocks, cleanup, openMenu, renderSelect,
and tick when applying the changes.
In `@frontend/src/routes/__tests__/Editor.test.ts`:
- Around line 132-136: Add an explicit cleanup step and ensure post-render
microtask flush: in the test teardown modify the afterEach block that currently
calls vi.unstubAllGlobals() to also call cleanup() (so both
vi.unstubAllGlobals() and cleanup() run), and change the render helper function
renderEditor to be async, call render(Editor) inside it, then await tick()
before returning to guarantee Svelte's post-render flush; update any test
callers to await renderEditor() accordingly.
In `@frontend/src/routes/__tests__/Home.test.ts`:
- Line 6: The mock for '@mateothegreat/svelte5-router' should be hoisted using
vi.hoisted(): create the mock functions (e.g., const goto = vi.fn()) inside a
vi.hoisted() call and then return them from the vi.mock() factory so the mock is
hoisted correctly; update the existing vi.mock(...) that returns { route: () =>
{}, goto: vi.fn() } to instead reference the hoisted goto and preserved route
symbol in the factory to follow project conventions.
- Around line 14-16: Make renderHome async and call await tick() immediately
after render(Home) so the Svelte microtask queue is flushed; update any tests
that call renderHome to still await it (e.g., replace synchronous renderHome()
with await renderHome()). Specifically change the renderHome helper (currently
returning render(Home)) to await render(Home) followed by await tick() before
returning the render result so subsequent waitFor/assertions run against flushed
state.
- Around line 1-2: Add missing test teardown/setup: update the vitest import to
include afterEach and ensure `@testing-library/svelte` import includes cleanup;
inside the describe for the Home tests add an afterEach that calls cleanup() and
vi.restoreAllMocks() (or vi.clearAllMocks() per project convention) to unmount
rendered components and restore spies (specifically the vi.spyOn on
meta.updateMetaTags), and keep the existing beforeEach to call
vi.clearAllMocks()/vi.hoisted() as needed so mocks are hoisted and tests do not
leak DOM or spies across specs.
In `@frontend/src/routes/__tests__/Login.test.ts`:
- Around line 46-48: The test helper renderLogin currently returns render(Login)
synchronously; change it to await tick() after calling render(Login) so the
helper becomes async and awaits tick() before returning, and add an afterEach
hook in the test suite that calls cleanup() to ensure DOM teardown between
tests; update references to the renderLogin helper calls to await the async
function where used and import tick and cleanup if not already present.
In `@frontend/src/routes/__tests__/Notifications.test.ts`:
- Around line 38-40: The test helper renderNotifications() does not await
Svelte's reactivity nor do tests call cleanup after each run; update the test
file to add an afterEach(() => cleanup()) and change renderNotifications to
await render(Notifications) followed by await tick() so the component mount
finishes before assertions (referencing renderNotifications, Notifications,
afterEach, cleanup, and tick).
In `@frontend/src/routes/__tests__/Register.test.ts`:
- Around line 35-37: The test lacks proper teardown and a render flush: update
the render helper and add an afterEach cleanup. Make the existing renderRegister
function async (function renderRegister() { ... }) and after calling
render(Register) await tick() to ensure the component is fully rendered before
assertions; also add an afterEach(() => cleanup()) to invoke cleanup() after
each test. Reference the renderRegister helper, the Register component,
afterEach, cleanup(), and tick() when making these edits.
In `@frontend/src/routes/admin/__tests__/AdminEvents.test.ts`:
- Line 12: The import userWithTimers is unused in AdminEvents.test.ts; remove
the userWithTimers import from the imports list to eliminate the unused symbol
warning and potential confusion—locate the import statement that includes
userWithTimers near the top of the test file and delete that identifier (or the
whole import if it becomes empty) while leaving the rest of the test imports and
the vi.useFakeTimers()/vi.advanceTimersByTimeAsync() timer-driven test intact.
In `@frontend/src/routes/admin/__tests__/AdminLayout.test.ts`:
- Around line 1-2: The tests in AdminLayout.test.ts are missing cleanup between
tests; import afterEach from 'vitest' and add an afterEach hook that calls
cleanup() from '@testing-library/svelte' to unmount DOM between tests; locate
the top-level imports (currently importing describe, it, expect, beforeEach, vi)
and add afterEach, and locate the setup block where beforeEach calls
vi.clearAllMocks() and insert an afterEach(() => cleanup()) to prevent DOM
leakage across tests.
- Around line 2-3: The renderLayout test helper currently returns synchronously
and must await Svelte's tick to stabilize async state: import tick from
'svelte', change the renderLayout helper function to be async, and call await
tick() (after render and before returning) so any pending microtasks update the
DOM; update any call sites if needed to await renderLayout. Ensure you modify
the renderLayout helper and its import lines (renderLayout, and the top-level
imports) accordingly.
- Line 21: The router mock is not hoisted and lifecycle cleanup/tick awaits are
missing; change the inline vi.mock call that defines
'@mateothegreat/svelte5-router' (currently using route: () => {}, goto: vi.fn())
to a hoisted mock like the authStore mock pattern, ensure the mocked exports
(route, goto) are declared at top-level hoisted scope, add an afterEach cleanup
block that clears mocks and unmounts (matching the test file's afterEach
pattern), and update renderLayout to await tick() after calling render so the
DOM updates settle before assertions (refer to renderLayout and the vi.mock for
router and authStore, plus afterEach and tick symbols).
In `@frontend/src/routes/admin/__tests__/AdminSettings.test.ts`:
- Around line 72-74: Make renderAdminSettings async and flush the component by
awaiting tick() after render: change function renderAdminSettings() to async,
call const result = render(AdminSettings); await tick(); return result. Import
tick from "svelte" and cleanup from "@testing-library/svelte", and update the
test teardown so afterEach calls cleanup() before vi.unstubAllGlobals() (e.g.,
afterEach(() => { cleanup(); vi.unstubAllGlobals(); })). Ensure the imports and
the async renderAdminSettings helper are used by the tests.
In `@frontend/vitest.config.ts`:
- Line 13: Replace the Vitest worker pool setting to improve jsdom stability:
locate the Vitest config object where pool: 'threads' is set (near the
environment: 'jsdom' setting) and change the value to pool: 'forks' so tests run
in separate processes instead of worker threads; update any related comments to
reflect the rationale for using forks with jsdom.
---
Outside diff comments:
In `@frontend/src/components/__tests__/ProtectedRoute.test.ts`:
- Around line 28-37: Add vi.clearAllMocks() at the start of the beforeEach setup
to ensure prior test spies/mocks are fully cleared; modify the beforeEach that
currently resets mocks.mockAuthStore.waitForInit and spies on router.goto so it
begins with vi.clearAllMocks(), then re-create any necessary spies (e.g.,
vi.spyOn(router, 'goto')) and reset/mock the specific store methods (e.g.,
mocks.mockAuthStore.waitForInit.mockReset().mockResolvedValue(true)) to avoid
stale call counts or return overrides leaking between tests.
- Around line 74-82: Tests are missing Svelte DOM cleanup and mock clearing: add
a call to cleanup() inside the existing afterEach block (alongside
vi.restoreAllMocks() and restoring window.sessionStorage/originalSessionStorage)
and add vi.clearAllMocks() to a beforeEach so mocks are cleared before each
test; update the afterEach surrounding code that references afterEach,
vi.restoreAllMocks, and originalSessionStorage to include cleanup() to prevent
DOM leakage between tests.
---
Nitpick comments:
In `@frontend/src/components/__tests__/ProtectedRoute.test.ts`:
- Around line 127-130: Replace the raw setTimeout waits with
Svelte/testing-library utilities: import and call await tick() (from 'svelte')
immediately after rendering the ProtectedRoute component to wait for Svelte
updates instead of using setTimeout(resolve, 50), and for any asynchronous state
assertions (e.g., where you expect router.goto calls later) wrap those
expectations in waitFor (from '@testing-library/svelte') so the test waits
reliably for async changes; update occurrences that reference router.goto and
the setTimeout pattern accordingly (including the other similar instance in this
test file).
In `@frontend/vitest.config.ts`:
- Around line 18-22: Replace the deprecated environmentMatchGlobs usage in
vitest.config.ts by defining a projects array that maps the test globs to a
named project with environment and setupFiles per project; specifically remove
environmentMatchGlobs and add a projects entry (use the symbol projects) that
contains a test object with name 'node', include set to the patterns currently
in environmentMatchGlobs (e.g., 'src/lib/**/*.test.ts',
'src/stores/**/*.test.ts', 'src/utils/**/*.test.ts'), environment: 'node', and
setupFiles pointing to your node setup file (setupFiles:
['./test/setup.node.ts']) so node-specific setup is applied only to that
project.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
frontend/src/__tests__/test-utils.tsfrontend/src/components/__tests__/Header.test.tsfrontend/src/components/__tests__/ProtectedRoute.test.tsfrontend/src/components/admin/events/__tests__/EventDetailsModal.test.tsfrontend/src/components/admin/events/__tests__/EventsTable.test.tsfrontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.tsfrontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.tsfrontend/src/components/admin/events/__tests__/UserOverviewModal.test.tsfrontend/src/components/editor/__tests__/LanguageSelect.test.tsfrontend/src/components/editor/__tests__/OutputPanel.test.tsfrontend/src/components/editor/__tests__/ResourceLimits.test.tsfrontend/src/components/editor/__tests__/SavedScripts.test.tsfrontend/src/routes/__tests__/Editor.test.tsfrontend/src/routes/__tests__/Home.test.tsfrontend/src/routes/__tests__/Login.test.tsfrontend/src/routes/__tests__/Notifications.test.tsfrontend/src/routes/__tests__/Register.test.tsfrontend/src/routes/__tests__/Settings.test.tsfrontend/src/routes/admin/__tests__/AdminEvents.test.tsfrontend/src/routes/admin/__tests__/AdminExecutions.test.tsfrontend/src/routes/admin/__tests__/AdminLayout.test.tsfrontend/src/routes/admin/__tests__/AdminSagas.test.tsfrontend/src/routes/admin/__tests__/AdminSettings.test.tsfrontend/src/routes/admin/__tests__/AdminUsers.test.tsfrontend/vitest.config.tsfrontend/vitest.setup.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/admin/events/tests/UserOverviewModal.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/components/editor/tests/ResourceLimits.test.ts
- frontend/src/components/editor/tests/OutputPanel.test.ts
- frontend/src/routes/admin/tests/AdminExecutions.test.ts
- frontend/src/routes/admin/tests/AdminSagas.test.ts
There was a problem hiding this comment.
2 issues found across 22 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/lib/admin/stores/sagasStore.svelte.ts">
<violation number="1" location="frontend/src/lib/admin/stores/sagasStore.svelte.ts:25">
P2: Auto-refresh always calls loadSagas, which ignores the execution-specific endpoint used by loadExecutionSagas. After loading a specific execution, the next refresh will fetch a paged list of all sagas and then filter client-side, which can drop the execution’s items if they’re on a different page. Consider refreshing the execution endpoint when executionIdFilter is set.</violation>
</file>
<file name="frontend/src/lib/admin/stores/eventsStore.svelte.ts">
<violation number="1" location="frontend/src/lib/admin/stores/eventsStore.svelte.ts:112">
P2: Starting a new replay while a previous one is still polling will leave the old interval running. Clear any existing interval before creating a new one to avoid duplicate polling and leaks.</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.
1 issue found across 24 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/e2e/test_admin_events_routes.py">
<violation number="1" location="backend/tests/e2e/test_admin_events_routes.py:408">
P3: The SSE test can silently pass even if the stream never emits a `data:` payload, because the loop ends without any assertion. Track whether a status was received and assert it after the loop so missing data fails the test.</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.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/actions/e2e-ready/action.yml">
<violation number="1" location=".github/actions/e2e-ready/action.yml:23">
P2: `kubectl wait` fails immediately when no nodes exist yet, so this can make the k3s init step flaky on slower startups. Consider looping until nodes exist/are ready rather than a single wait call.</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.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/e2e/test_k8s_worker_create_pod.py">
<violation number="1">
P2: Ensure ConfigMap/Pod cleanup runs even when the test fails (e.g., wrap the create/verify block in a try/finally).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…lower-img-sizes # Conflicts: # backend/app/settings.py # backend/tests/e2e/test_k8s_worker_create_pod.py
There was a problem hiding this comment.
4 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/e2e/test_execution_routes.py">
<violation number="1" location="backend/tests/e2e/test_execution_routes.py:63">
P2: `submit_and_wait` can miss completion events because it subscribes to Redis Pub/Sub after execution starts, which introduces race-based test flakiness for fast jobs.</violation>
<violation number="2" location="backend/tests/e2e/test_execution_routes.py:210">
P2: Waiting for `POD_CREATED` via Pub/Sub after submit can miss the event and make cancellation tests flaky.</violation>
</file>
<file name="backend/tests/e2e/conftest.py">
<violation number="1" location="backend/tests/e2e/conftest.py:39">
P1: This per-call Redis pub/sub wait can miss already-published events, making E2E fixtures flaky due to a subscribe-after-produce race.</violation>
<violation number="2" location="backend/tests/e2e/conftest.py:196">
P1: Waiting for RESULT_STORED is not a reliable synchronization point for notification creation because result and notification handlers run in separate consumer groups.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # Wait for saga to start (pod creation command sent) instead of blind sleep | ||
| await event_waiter.wait_for_saga_command(exec_response.execution_id) | ||
| # Wait for pod creation (saga started + command dispatched) instead of blind sleep | ||
| await wait_for_pod_created(redis_client, exec_response.execution_id) |
There was a problem hiding this comment.
P2: Waiting for POD_CREATED via Pub/Sub after submit can miss the event and make cancellation tests flaky.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/e2e/test_execution_routes.py, line 210:
<comment>Waiting for `POD_CREATED` via Pub/Sub after submit can miss the event and make cancellation tests flaky.</comment>
<file context>
@@ -197,16 +198,16 @@ class TestExecutionCancel:
- # Wait for saga to start (pod creation command sent) instead of blind sleep
- await event_waiter.wait_for_saga_command(exec_response.execution_id)
+ # Wait for pod creation (saga started + command dispatched) instead of blind sleep
+ await wait_for_pod_created(redis_client, exec_response.execution_id)
cancel_req = CancelExecutionRequest(reason="Test cancellation")
</file context>
| assert resp.status_code == 200 | ||
| execution = ExecutionResponse.model_validate(resp.json()) | ||
| await waiter.wait_for_result(execution.execution_id, timeout=timeout) | ||
| await wait_for_result(redis_client, execution.execution_id, timeout=timeout) |
There was a problem hiding this comment.
P2: submit_and_wait can miss completion events because it subscribes to Redis Pub/Sub after execution starts, which introduces race-based test flakiness for fast jobs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/e2e/test_execution_routes.py, line 63:
<comment>`submit_and_wait` can miss completion events because it subscribes to Redis Pub/Sub after execution starts, which introduces race-based test flakiness for fast jobs.</comment>
<file context>
@@ -50,16 +51,16 @@
assert resp.status_code == 200
execution = ExecutionResponse.model_validate(resp.json())
- await waiter.wait_for_result(execution.execution_id, timeout=timeout)
+ await wait_for_result(redis_client, execution.execution_id, timeout=timeout)
result_resp = await client.get(f"/api/v1/executions/{execution.execution_id}/result")
assert result_resp.status_code == 200
</file context>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/e2e/test_admin_events_routes.py">
<violation number="1" location="backend/tests/e2e/test_admin_events_routes.py:408">
P2: The replay SSE async generator is no longer closed in this test. Restore explicit `aclose()` (via `try/finally`) to avoid leaking generator state and async teardown flakiness.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+408
to
420
| first_event = await anext(stream) | ||
| status = EventReplayStatusResponse.model_validate( | ||
| json_mod.loads(first_event["data"]) | ||
| ) | ||
|
|
||
| assert status_response.status_code == 200 | ||
| status = EventReplayStatusResponse.model_validate(status_response.json()) | ||
| assert status.session_id == replay_result.session_id | ||
| # After scheduling a replay (dry_run=False), status is SCHEDULED or RUNNING if it started quickly | ||
| assert status.status in (ReplayStatus.SCHEDULED, ReplayStatus.RUNNING) | ||
| assert status.status in ( | ||
| ReplayStatus.SCHEDULED, ReplayStatus.CREATED, | ||
| ReplayStatus.RUNNING, ReplayStatus.COMPLETED, | ||
| ) | ||
| assert status.total_events >= 1 | ||
| assert status.replayed_events >= 0 | ||
| assert status.progress_percentage >= 0.0 | ||
|
|
There was a problem hiding this comment.
P2: The replay SSE async generator is no longer closed in this test. Restore explicit aclose() (via try/finally) to avoid leaking generator state and async teardown flakiness.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/e2e/test_admin_events_routes.py, line 408:
<comment>The replay SSE async generator is no longer closed in this test. Restore explicit `aclose()` (via `try/finally`) to avoid leaking generator state and async teardown flakiness.</comment>
<file context>
@@ -405,21 +405,18 @@ async def test_stream_replay_status_after_replay(
- assert status.progress_percentage >= 0.0
- finally:
- await stream.aclose()
+ first_event = await anext(stream)
+ status = EventReplayStatusResponse.model_validate(
+ json_mod.loads(first_event["data"])
</file context>
Suggested change
| first_event = await anext(stream) | |
| status = EventReplayStatusResponse.model_validate( | |
| json_mod.loads(first_event["data"]) | |
| ) | |
| assert status_response.status_code == 200 | |
| status = EventReplayStatusResponse.model_validate(status_response.json()) | |
| assert status.session_id == replay_result.session_id | |
| # After scheduling a replay (dry_run=False), status is SCHEDULED or RUNNING if it started quickly | |
| assert status.status in (ReplayStatus.SCHEDULED, ReplayStatus.RUNNING) | |
| assert status.status in ( | |
| ReplayStatus.SCHEDULED, ReplayStatus.CREATED, | |
| ReplayStatus.RUNNING, ReplayStatus.COMPLETED, | |
| ) | |
| assert status.total_events >= 1 | |
| assert status.replayed_events >= 0 | |
| assert status.progress_percentage >= 0.0 | |
| try: | |
| first_event = await anext(stream) | |
| status = EventReplayStatusResponse.model_validate( | |
| json_mod.loads(first_event["data"]) | |
| ) | |
| assert status.session_id == replay_result.session_id | |
| assert status.status in ( | |
| ReplayStatus.SCHEDULED, ReplayStatus.CREATED, | |
| ReplayStatus.RUNNING, ReplayStatus.COMPLETED, | |
| ) | |
| assert status.total_events >= 1 | |
| assert status.replayed_events >= 0 | |
| assert status.progress_percentage >= 0.0 | |
| finally: | |
| await stream.aclose() |
|
This was referenced Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Speeds up builds and deploys with Docker layer caching and a slimmer multi‑stage backend; production builds drop source maps. Adds SSE replay status streaming with text/event-stream OpenAPI, admin Svelte stores with auto‑refresh, execution_id filtering for sagas, and faster tests via Redis pub/sub and tighter Kafka/SSE timeouts.
Refactors
Dependencies
Written for commit 162b9a3. Summary will update on new commits.
Summary by CodeRabbit
Chores
Tests
Documentation