Conversation
also updated tests
📝 WalkthroughWalkthroughConsolidates per-value Svelte stores into singleton store objects, replaces auth-init with an authStore, removes SSE health/shutdown API/types and narrows several generated API types, updates API interceptors and streaming behavior, and adds/rewrites many unit and e2e tests and test utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (UI)
participant Auth as authStore
participant API as Backend API
participant Settings as userSettingsStore
UI->>Auth: authStore.initialize() / authStore.login()
Auth->>API: POST /auth/login or GET /auth/verify
API-->>Auth: token / profile
Auth->>Auth: persist state (sessionStorage)
Auth->>Settings: load user settings (if authenticated)
Settings-->>Auth: settings data
Auth-->>UI: update authStore.{isAuthenticated,username,userRole,csrfToken}
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 40 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/Header.svelte">
<violation number="1" location="frontend/src/components/Header.svelte:133">
P2: Avoid calling `toUpperCase()` on an undefined result. The optional chaining only guards `charAt`, so when username is null this expression throws before `??` runs.</violation>
</file>
<file name="frontend/src/App.svelte">
<violation number="1" location="frontend/src/App.svelte:9">
P3: Remove the unused themeStore import to avoid dead code and lint warnings.</violation>
</file>
<file name="frontend/src/lib/api-interceptors.ts">
<violation number="1" location="frontend/src/lib/api-interceptors.ts:41">
P2: Directly mutating authStore fields here bypasses AuthStore’s internal auth cache reset, so `verifyAuth()` can keep returning a cached `true` after a 401. Consider adding a public reset method on AuthStore (or exposing a dedicated clear function) that also invalidates `#authCache`, and call that instead of manually assigning fields.</violation>
</file>
<file name="frontend/src/stores/auth.svelte.ts">
<violation number="1" location="frontend/src/stores/auth.svelte.ts:74">
P2: During token verification, `userId` and `userEmail` are overwritten to `null` in sessionStorage before `fetchUserProfile()` attempts to repopulate them. If the profile fetch fails (which only logs a warning), these values are permanently lost from storage until the next successful profile fetch. This causes data inconsistency after page refresh.
Preserve existing values instead of overwriting with null.</violation>
</file>
<file name="frontend/src/stores/__tests__/theme.test.ts">
<violation number="1" location="frontend/src/stores/__tests__/theme.test.ts:8">
P2: The mock specifier doesn’t match the dynamic import in the store (`$stores/auth.svelte`), so the mock won’t apply and tests may load the real auth module. Mock the same specifier used in the store to ensure isolation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/Notifications.svelte (1)
26-29:⚠️ Potential issue | 🟡 MinorMissing error handling around
notificationStore.load(100)—loadingcan get stuck.If
notificationStore.load(100)throws,loadingremainstrueindefinitely, leaving the user stuck on the spinner. Wrap in try/catch or use a finally block.Proposed fix
// Load data immediately using shared store loading = true; - await notificationStore.load(100); - loading = false; + try { + await notificationStore.load(100); + } catch (err) { + console.error('Failed to load notifications:', err); + toast.error('Failed to load notifications'); + } finally { + loading = false; + }
🤖 Fix all issues with AI agents
In `@frontend/src/lib/api-interceptors.ts`:
- Around line 40-48: clearAuthState() duplicates AuthStore's private clearing
logic; expose that logic via a public method on AuthStore (e.g., add a public
clearAuth() or clearAuthPublic() that internally calls the existing private
`#clearAuth` and also removes sessionStorage('authState') if the private method
doesn't) and update api-interceptors.ts to call authStore.clearAuth() instead of
its local clearAuthState(); remove the duplicate clearAuthState() function so
there is a single source of truth (refer to AuthStore#clearAuth(), authStore,
and clearAuthState()).
In `@frontend/src/routes/Editor.svelte`:
- Around line 154-163: The early return after handling a 404 in the update block
prevents loadSavedScripts() from running and so the sidebar never refreshes;
modify the branch inside the updateSavedScriptApiV1ScriptsScriptIdPut error
handling so that after creating the fallback script via
createSavedScriptApiV1ScriptsPost (and setting currentScriptId from the returned
data) you still call await loadSavedScripts() (and then call toast.success as
before) instead of returning immediately; ensure you keep the existing behavior
for non-404 errors (i.e., still return) and update only the 404 fallback flow in
the function that contains updateSavedScriptApiV1ScriptsScriptIdPut,
createSavedScriptApiV1ScriptsPost, currentScriptId, and loadSavedScripts.
- Around line 56-57: selectedLang and selectedVersion are not being persisted to
localStorage like script/scriptName/currentScriptId; update the editor state
persistence to save selectedLang and selectedVersion and restore them on load.
Modify the same initialization/restore logic that handles script, scriptName,
and currentScriptId to read persisted values for selectedLang and
selectedVersion (falling back to 'python' and '3.11' if absent) and add
corresponding effects so changes to selectedLang and selectedVersion are written
to localStorage whenever they change; reference the selectedLang and
selectedVersion variables and the existing persistence effects that handle
script/scriptName/currentScriptId to implement this consistently.
In `@frontend/src/stores/notificationStore.svelte.ts`:
- Around line 15-37: The load method can throw before it resets this.loading;
wrap the await getNotificationsApiV1NotificationsGet call (and subsequent
processing) in try/catch/finally inside the load function so any thrown
exception sets this.loading = false and sets this.error (and returns an empty
array) instead of leaving loading true; specifically, catch exceptions from
getNotificationsApiV1NotificationsGet, set this.error to a readable message,
ensure this.notifications is not left inconsistent, and use finally to always
set this.loading = false.
In `@frontend/src/stores/theme.svelte.ts`:
- Around line 7-10: The getSystemTheme function and the matchMedia usage later
are accessing .matches (and .addEventListener) without ensuring matchMedia
returned a non-null MediaQueryList; update getSystemTheme to store the result of
window.matchMedia('(prefers-color-scheme: dark)') into a local variable, check
that it is truthy before reading .matches (fall back to 'light' if falsy), and
apply the same guard where matchMedia is used to addEventListener (check the
MediaQueryList exists before calling .addEventListener or use
addListener/removeListener fallbacks), referencing getSystemTheme and the
matchMedia usage near the event listener code to locate and fix both places.
🧹 Nitpick comments (10)
frontend/src/lib/__tests__/user-settings.test.ts (1)
24-24: Nit:boolean | nulltype annotation is wider than needed.
isAuthenticatedis used as a boolean (true/false) in all tests. The| nullin the type cast appears unnecessary.Suggested fix
- isAuthenticated: true as boolean | null, + isAuthenticated: true as boolean,frontend/src/stores/__tests__/userSettings.test.ts (1)
4-11:DEFAULTSduplicatesDEFAULT_EDITOR_SETTINGSfrom the store — intentional but worth a note.This is a common and acceptable test pattern (assert against known literals rather than importing from SUT). Just be aware that if defaults change in the store, these tests need a manual update.
frontend/src/components/ProtectedRoute.svelte (1)
28-28: Minor inconsistency in null handling forisAuthenticated.Line 28 uses
authStore.isAuthenticated ?? false(guarding againstnull), but the$effecton Line 49 checks!authStore.isAuthenticatedwithout nullish coalescing. IfisAuthenticatedcan benull, both paths should handle it consistently. Currently this is benign since!nullistrueand triggers redirect — same as!false— but the inconsistency is worth noting.Also applies to: 48-53
frontend/src/stores/notificationStore.svelte.ts (1)
43-68: Same missing error handling inmarkAsRead,markAllAsRead, anddelete.These async methods also lack try/catch. If the API client throws, the error goes unhandled and the caller receives a rejected promise rather than
false. Consider wrapping in try/catch for consistency with the return-type contract (boolean).frontend/src/App.svelte (1)
9-9:themeStoreis imported but not referenced in this file.If the import is needed purely for its side effect (applying the theme on module initialization), consider adding a comment to make this intent explicit — otherwise it may be removed by a future cleanup or linter.
- import { themeStore } from '$stores/theme.svelte'; + import { themeStore } from '$stores/theme.svelte'; // side-effect: applies persisted theme on loadfrontend/src/lib/auth-init.ts (2)
98-105:_setAuthStoresunconditionally setsisAuthenticated = true.This ignores the
authData.isAuthenticatedfield from the persisted data. Currently safe because this method is only called from_handlePersistedAuthwhich already implies we believe we have a valid session, but it's a subtle coupling. If the persisted data hadisAuthenticated: falsefor some reason, this would silently override it.Consider:
- authStore.isAuthenticated = true; + authStore.isAuthenticated = authData.isAuthenticated;
127-136:_getPersistedAuthdoes not validate the parsed JSON shape.
JSON.parsemay succeed on data that doesn't conform toPersistedAuth(e.g., missing fields). This would propagateundefinedvalues into_setAuthStores. Consider adding a minimal structural check (e.g., verifyusernameandtimestampexist) before returning.Example guard
private static _getPersistedAuth(): PersistedAuth | null { try { const authData = sessionStorage.getItem('authState'); if (!authData) return null; - return JSON.parse(authData); + const parsed = JSON.parse(authData); + if (!parsed || typeof parsed.timestamp !== 'number') return null; + return parsed as PersistedAuth; } catch (e) { console.error('[AuthInit] Failed to parse persisted auth:', e); return null; } }frontend/src/routes/Notifications.svelte (1)
146-146: Conditional is sound but slightly redundant.
notificationStore.unreadCount > 0already impliesnotifications.length > 0(you can't have unread notifications without notifications). Not a functional issue — just a minor observation.frontend/src/routes/Editor.svelte (2)
89-101: Reading and writingauthenticatedinside the same$effectcauses a needless re-run.
authenticatedis a$statevariable that is both read (line 92, to capture the previous value) and written (line 93) inside this effect. Svelte 5 tracks it as a dependency, so every write schedules another effect execution. It stabilizes on the second pass (no infinite loop), but the double-run is wasteful and the pattern is fragile—any future change that adds side-effects here could cause subtle bugs.A cleaner approach is to track the previous auth value in a plain (untracked) variable:
Proposed fix
- // React to auth changes - $effect(() => { - const isAuth = authStore.isAuthenticated ?? false; - const wasAuthenticated = authenticated; - authenticated = isAuth; - if (!wasAuthenticated && authenticated) { - loadSavedScripts(); - } else if (wasAuthenticated && !authenticated) { - savedScripts = []; - currentScriptId = null; - scriptName = ''; - } - }); + // React to auth changes + let prevAuth = false; // plain variable – not tracked by $effect + $effect(() => { + const isAuth = authStore.isAuthenticated ?? false; + authenticated = isAuth; // still keep the $state for template use + if (!prevAuth && isAuth) { + loadSavedScripts(); + } else if (prevAuth && !isAuth) { + savedScripts = []; + currentScriptId = null; + scriptName = ''; + } + prevAuth = isAuth; + });
71-71: Extract inline defaults for readability.This single-line derived expression is dense and hard to scan at a glance.
Suggested refactor
- let editorSettings = $derived({ ...{ theme: 'auto', font_size: 14, tab_size: 4, use_tabs: false, word_wrap: true, show_line_numbers: true }, ...userSettingsStore.editorSettings }); + const defaultEditorSettings = { + theme: 'auto', + font_size: 14, + tab_size: 4, + use_tabs: false, + word_wrap: true, + show_line_numbers: true, + }; + let editorSettings = $derived({ ...defaultEditorSettings, ...userSettingsStore.editorSettings });
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@frontend/src/routes/Editor.svelte`:
- Around line 80-88: The effect checking for script-name changes is firing on
savedScripts refreshes (triggered by loadSavedScripts after saveScript) and
nullifying currentScriptId unexpectedly; fix it by gating the check to run only
when the name was edited by the user: add a boolean flag (e.g.,
nameEditedByUser) that is set when the user changes scriptName (input handler)
and cleared after handling the change or after loadSavedScripts updates, then
modify the $effect to only perform the savedScripts.find and toast.info logic
when nameEditedByUser is true; ensure saveScript/loadSavedScripts clear or
preserve the flag appropriately so API refreshes don't trigger the toast.
In `@frontend/src/stores/auth.svelte.ts`:
- Line 95: The assignment to this.userId uses data.user_id which can be
undefined and violates the declared string | null type; change the assignment to
coalesce undefined to null (use data.user_id ?? null) similar to how data.email
is handled, ensuring this.userId always becomes a string or null; update the
assignment in the auth store where this.userId is set (look for the statement
"this.userId = data.user_id") so it uses the defensive coalescing.
- Around line 140-157: During verify success, persistAuthState is overwriting
userId/userEmail with null and then trying to restore them asynchronously via
fetchUserProfile, causing brief null flicker and permanent loss on fetch
failure; change the logic around the persistAuthState call in the verification
path so it preserves existing userId/userEmail (use the current
this.userId/this.userEmail or read the existing persisted values) instead of
writing null, and only update persistent userId/userEmail after a successful
fetchUserProfile (or fallback to leaving previous values if fetch fails); update
the block that sets
this.isAuthenticated/this.username/this.userRole/this.csrfToken and the
persistAuthState call accordingly so fetchUserProfile only mutates persisted
userId/userEmail on success.
In `@frontend/src/stores/notificationStore.svelte.ts`:
- Around line 48-73: Wrap the API calls in markAsRead, markAllAsRead, and delete
in try/catch (mirroring load()) so thrown errors (e.g., network failures) are
caught and cause the method to return false instead of bubbling an exception;
specifically, update async methods markAsRead(notificationId: string),
markAllAsRead(), and delete(notificationId: string) to perform the await inside
a try block and return false from the catch (optionally logging the error), and
keep the existing post-success state updates to this.notifications in the try
block.
🧹 Nitpick comments (3)
frontend/src/stores/auth.svelte.ts (1)
61-90: Persist object duplicates the in-memory state assignments.Lines 67-72 update instance fields and lines 74-81 build an almost identical object for
persistAuthState. This pattern repeats inverifyAuth(lines 140-151). Consider extracting a helper to keep these in sync:Sketch
+ private updateAndPersist(fields: Partial<AuthState>) { + if (fields.isAuthenticated !== undefined) this.isAuthenticated = fields.isAuthenticated; + if (fields.username !== undefined) this.username = fields.username; + if (fields.userRole !== undefined) this.userRole = fields.userRole; + if (fields.csrfToken !== undefined) this.csrfToken = fields.csrfToken; + if (fields.userId !== undefined) this.userId = fields.userId; + if (fields.userEmail !== undefined) this.userEmail = fields.userEmail; + persistAuthState(fields); + }frontend/src/lib/__tests__/auth-init.test.ts (1)
76-97:clearAuthre-implementation aftermockResetis correct but easy to drift.The
clearAuthbehavior is duplicated: once in the initial declaration (lines 15-22) and again inbeforeEach(lines 87-94). If the realclearAuthbehavior changes, only one copy might get updated. Consider extracting the mock implementation into a named function:Sketch
+function mockClearAuthImpl() { + mockAuthStore.isAuthenticated = false; + mockAuthStore.username = null; + mockAuthStore.userId = null; + mockAuthStore.userRole = null; + mockAuthStore.userEmail = null; + mockAuthStore.csrfToken = null; +} + const mockAuthStore = { ... - clearAuth: vi.fn(() => { - mockAuthStore.isAuthenticated = false; - ... - }), + clearAuth: vi.fn(mockClearAuthImpl), }; // In beforeEach: - mockAuthStore.clearAuth.mockReset().mockImplementation(() => { - mockAuthStore.isAuthenticated = false; - ... - }); + mockAuthStore.clearAuth.mockReset().mockImplementation(mockClearAuthImpl);frontend/src/routes/Editor.svelte (1)
59-62: localStorage writes on every keystroke forscript.The
$effecton line 60 fires on every character change in the editor, callingJSON.stringify(script)andlocalStorage.setItemsynchronously. For large scripts this could introduce input latency. Consider debouncing the persistence, for example:Sketch
// Debounced persistence for script content let saveTimer: ReturnType<typeof setTimeout>; $effect(() => { const value = script; clearTimeout(saveTimer); saveTimer = setTimeout(() => localStorage.setItem('script', JSON.stringify(value)), 300); });
…se errors are caought by api interceptors
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 (2)
frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts (1)
84-90:⚠️ Potential issue | 🟡 MinorDuplicate test: lines 84–86 and 88–90 are identical.
Both
"returns 0 for empty steps"and"returns 0 for empty steps array"callgetSagaProgressPercentage([], 'execution_saga')and asserttoBe(0). One should be removed.Proposed fix — remove the duplicate
it('returns 0 for empty steps', () => { expect(getSagaProgressPercentage([], 'execution_saga')).toBe(0); }); - it('returns 0 for empty steps array', () => { - expect(getSagaProgressPercentage([], 'execution_saga')).toBe(0); - }); - it('calculates correct percentage for execution_saga', () => {frontend/src/routes/Notifications.svelte (1)
17-21:⚠️ Potential issue | 🟡 Minor
loadingstaystrueforever ifnotificationStore.load()throws.There's no
try/catch/finallyaround the await, so ifloadrejects, line 20 never executes. Same pattern inapplyFilters(lines 57-64).Proposed fix
onMount(async () => { loading = true; - await notificationStore.load(100); - loading = false; + try { + await notificationStore.load(100); + } finally { + loading = false; + } });Apply the same
try/finallywrapper inapplyFilters.
🤖 Fix all issues with AI agents
In `@frontend/src/routes/Editor.svelte`:
- Around line 90-101: The $effect reacting to the derived authenticated value
calls loadSavedScripts() as a fire-and-forget, which can produce unhandled
promise rejections; update the effect (the block using $effect, the reactive
read of authenticated and the prevAuth variable) to await or explicitly handle
errors from loadSavedScripts()—for example call loadSavedScripts().catch(err =>
process or log the error) or wrap it in an IIFE async function and try/catch—so
any rejection from loadSavedScripts() is caught and handled rather than left
unhandled.
In `@frontend/src/routes/Settings.svelte`:
- Around line 129-137: saveSettings crashes because savedSnapshot can be an
empty string when loadSettings failed; before calling JSON.parse on
savedSnapshot in saveSettings, guard against empty/invalid snapshot and fall
back to an empty object (or safely parse with try/catch). Update the logic in
saveSettings to check savedSnapshot (the savedSnapshot variable) and either use
a default {} for original or safely parse inside a try/catch to avoid
JSON.parse('') throwing, keeping the rest of the comparison against formData
unchanged.
🧹 Nitpick comments (6)
frontend/src/lib/admin/autoRefresh.svelte.ts (1)
82-93: Minor:onDestroy(cleanup)is redundant with the$effectcleanup.The cleanup function returned from
$effect(line 92) already runs when the component is destroyed, so theonDestroy(cleanup)on line 84 results instop()being called twice on teardown. It's harmless but unnecessary whenautoCleanupis true and an$effectis always active.frontend/src/lib/admin/sagas/sagaStates.ts (1)
59-61: Removing the fallback is safe at compile-time but fragile at runtime.Since
SAGA_STATESis typedRecord<SagaState, …>, TypeScript guarantees all keys exist. However, if the backend ever returns a new/unexpected state string that hasn't been added to theSagaStateunion yet, this will returnundefinedand callers will throw on.label,.color, etc. Consider a defensive fallback or a null-check at call sites.This is low-risk given the TS contract, so flagging as optional.
Optional: add a runtime guard
export function getSagaStateInfo(state: SagaState): SagaStateConfig { - return SAGA_STATES[state]; + return SAGA_STATES[state] ?? { label: state, color: 'badge-neutral', bgColor: 'bg-neutral-50 dark:bg-neutral-900/20', icon: CircleX }; }frontend/src/stores/__tests__/notificationStore.test.ts (1)
16-36: Mock reimplementsgetErrorMessagelogic — consider importing the real function instead.This mock duplicates the full error-extraction logic of the real
getErrorMessage. If the implementation evolves, this mock will silently diverge. Since this is a notification store test (not an api-interceptors test), consider letting the realgetErrorMessagerun through, or at minimum simplify to a stub like(err, fallback) => String(err?.detail?.[0]?.msg ?? err?.detail ?? err?.message ?? fallback ?? 'An error occurred').frontend/src/routes/Settings.svelte (1)
95-127: Duplicated settings-to-formData mapping — consider extracting a helper.The same object shape construction (lines 106-123) is repeated in
saveSettings(lines 158-175) andrestoreSettingscallsloadSettingswhich repeats it again. A small helper liketoFormData(data)would reduce the duplication.frontend/src/routes/Editor.svelte (1)
103-124:onMountdoesn't guard against API errors for limits/examples fetches.
getK8sResourceLimitsApiV1K8sLimitsGetandgetExampleScriptsApiV1ExampleScriptsGetare called without error handling. If either fails (network issue, 500, etc.), the destructureddatawill beundefinedand the subsequent code will silently proceed with empty runtimes — which is acceptable degradation — but the lack of any user feedback could be confusing (no runtimes shown, no error toast).Given the PR objective of removing try/catch in favor of API interceptors, this is likely intentional. Just confirming you're happy with the silent fallback.
frontend/src/stores/__tests__/auth.test.ts (1)
348-357:fetchUserProfilerejection test doesn't assert the error value.
rejects.toBeDefined()only confirms the promise rejects — it doesn't validate what it rejects with. Consider usingrejects.toThrow()or matching on a specific message to prevent regressions where the error type/message changes silently.Proposed tightening
- await expect(authStore.fetchUserProfile()).rejects.toBeDefined(); + await expect(authStore.fetchUserProfile()).rejects.toThrow();
There was a problem hiding this comment.
4 issues found across 32 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.
<file name="frontend/src/routes/admin/AdminSettings.svelte">
<violation number="1" location="frontend/src/routes/admin/AdminSettings.svelte:39">
P2: loadSettings now silently ignores API errors, leaving users without feedback when settings fail to load. Restore an error toast (or similar reporting) so failures are visible.</violation>
</file>
<file name="frontend/src/components/editor/LanguageSelect.svelte">
<violation number="1" location="frontend/src/components/editor/LanguageSelect.svelte:75">
P2: The non-null assertion on `currentVersions[focusedVersionIndex]!` can pass `undefined` when the hovered language has no versions, leading to invalid selection. Guard for a defined version before calling `selectVersion`.</violation>
</file>
<file name="frontend/src/lib/api-interceptors.ts">
<violation number="1" location="frontend/src/lib/api-interceptors.ts:100">
P2: Avoid assuming response/request are always defined in the error interceptor; this can throw on network errors before error handling runs. Restore the defensive optional checks.</violation>
</file>
<file name="frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts">
<violation number="1" location="frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts:88">
P3: This test duplicates the existing "returns 0 for empty steps" case directly above, adding no coverage and increasing maintenance. Remove the duplicate or merge the assertions into a single 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.
7 issues found across 12 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.
<file name="frontend/e2e/settings.spec.ts">
<violation number="1" location="frontend/e2e/settings.spec.ts:167">
P2: This conditional makes the restore test a no-op when the button isn’t visible, so regressions won’t fail the test. Since the test creates history, assert the Restore button is visible and proceed with the restore flow.</violation>
</file>
<file name="frontend/src/routes/__tests__/Notifications.test.ts">
<violation number="1" location="frontend/src/routes/__tests__/Notifications.test.ts:231">
P2: Avoid conditional guards that allow the test to pass when the delete button is missing. Assert the button exists so the test fails if the UI regresses.</violation>
</file>
<file name="frontend/src/routes/admin/__tests__/AdminSettings.test.ts">
<violation number="1" location="frontend/src/routes/admin/__tests__/AdminSettings.test.ts:93">
P3: The test stubs the global confirm but never restores it, which can leak into other tests in the same worker. Add a cleanup (e.g., vi.unstubAllGlobals or restore the original confirm) in afterEach/afterAll to avoid cross-test pollution.</violation>
</file>
<file name="frontend/e2e/notifications.spec.ts">
<violation number="1" location="frontend/e2e/notifications.spec.ts:97">
P2: The assertion always passes because `.or(notificationCard)` includes the already-visible card, so the test doesn’t verify the read state change. Assert a real state change (e.g., class removal or the "Read" label alone).</violation>
</file>
<file name="frontend/e2e/editor.spec.ts">
<violation number="1" location="frontend/e2e/editor.spec.ts:158">
P2: This test silently skips verification when the saved script is not visible, which can hide regressions in the load flow. Assert visibility and proceed so the test fails when the script doesn’t appear.</violation>
<violation number="2" location="frontend/e2e/editor.spec.ts:180">
P2: The delete test can skip all assertions if the row or delete button is missing, so regressions won’t fail the suite. Assert visibility and use a one-time dialog handler to keep the test deterministic.</violation>
<violation number="3" location="frontend/e2e/editor.spec.ts:200">
P3: The export test passes even if no download fires, which hides export regressions. Treat the download as required and assert its filename unconditionally.</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: 12
🤖 Fix all issues with AI agents
In `@frontend/e2e/editor.spec.ts`:
- Around line 199-208: The test silently passes when the download never fires
because userPage.waitForEvent('download') is swallowed by .catch(() => null);
remove the .catch and make the download assertion unconditional so a missing
download fails the test: await Promise.all([...]) should let
userPage.waitForEvent('download', { timeout: 5000 }) throw on timeout (or
explicitly assert download is not null) and then assert
download.suggestedFilename() contains 'export-test' after triggering the Export
button; update the block around userPage.waitForEvent and the Export click to
fail loudly instead of returning null.
- Around line 156-162: The test currently skips asserting the saved script by
guarding with an if on savedScript.isVisible, letting failures pass silently;
replace that conditional with an explicit assertion that the saved script is
visible (use expect(savedScript).toBeVisible() or await savedScript.waitFor({
state: 'visible' })) immediately after expectToastVisible(userPage), then click
savedScript and assert the script name input value via
expect(userPage.locator('#scriptNameInput')).toHaveValue(scriptName); keep
references to savedScript, expectToastVisible, '#scriptNameInput', and
scriptName so the failure surfaces when the saved entry is missing.
- Around line 178-188: Replace the silent-conditional checks around scriptRow
and deleteBtn with explicit assertions so failures fail the test: assert
visibility of scriptRow (the locator named scriptRow) using
expect(scriptRow).toBeVisible(...) instead of if(...).catch(() => false), and
assert visibility of the delete button (deleteBtn) similarly; remove the .catch
swallowing. Replace the brittle parent-traversal locator that creates deleteBtn
with an attribute-based locator (e.g., button[aria-label="Delete"] or
button[title="Delete"]) to target the delete action more robustly, keep the
dialog handler userPage.on('dialog', dialog => dialog.accept()) registered
before clicking, and then click the asserted delete button and call
expectToastVisible(userPage) as before.
In `@frontend/e2e/notifications.spec.ts`:
- Around line 96-99: The assertion uses
notificationCard.locator('text=Read').or(notificationCard) which is vacuous
because .or(notificationCard) always matches; remove the fallback and assert the
actual read-state change instead by targeting the "Read" label or a read-state
attribute/class on notificationCard. Replace the or(...) expression with a
direct assertion such as
expect(notificationCard.locator('text=Read')).toBeVisible({ timeout: 3000 }) or
assert a class/attribute (e.g., expect(notificationCard).toHaveClass(/read/) or
expect(notificationCard).toHaveAttribute('data-state','read')) depending on the
app's implementation so the test fails when the card does not transition to
read.
In `@frontend/e2e/settings.spec.ts`:
- Around line 166-172: The test currently silently skips the restore flow
because it guards on the visibility check for restoreBtn (created via
userPage.getByRole('button', { name: 'Restore' }).first()), so replace the
conditional with a hard assertion that the button is visible (use the Playwright
expect API against restoreBtn with the same timeout) before attaching the dialog
handler and clicking, then continue to await expectToastVisible(userPage); if
the Restore button is legitimately optional, mark the test with test.skip or
test.fixme instead to make the skip explicit.
In `@frontend/src/routes/__tests__/Editor.test.ts`:
- Around line 258-268: The "Delete script" test is only asserting the mock
exists instead of executing the delete flow; update the test to simulate the
user action that triggers deletion (e.g., call renderEditor(), find and click
the delete control that invokes deleteScript), set
mocks.mockConfirm.mockReturnValueOnce(true), then await assertions that
mocks.mockConfirm was called and that
mocks.deleteSavedScriptApiV1ScriptsScriptIdDelete was invoked (optionally with
the expected scriptId); keep using renderEditor and waitFor to ensure async
operations complete and reset/clear mocks as needed.
- Around line 270-291: The tests merely check that the hoisted vi.fn() stubs
exist (mocks.mockExecutionState.execute and mocks.mockExecutionState.reset)
instead of asserting the Editor actually uses them; update the two tests to
render the Editor via renderEditor(), then simulate the real flows that should
call those mocks (invoke the execution flow by triggering whatever UI action or
function causes createExecutionState().execute to run, and invoke the new-script
flow by calling the UI button or function that triggers newScript() so
execution.reset should be called), and replace toBeDefined() assertions with
precise call assertions (e.g.,
expect(mocks.mockExecutionState.execute).toHaveBeenCalled() and
expect(mocks.mockExecutionState.reset).toHaveBeenCalled()) to verify the
component interacts with createExecutionState and newScript as intended.
- Around line 217-256: Tests in the "Save script" suite are only asserting mock
wiring rather than component behavior; update them to exercise the Editor's save
logic by capturing and invoking the callback prop passed to the mocked
ScriptActions (or by calling the extracted save function) after renderEditor()
and then assert on outcomes: for the unauthenticated case verify mocks.addToast
was called with the expected warning, for the create case invoke the
saved-script callback and assert mocks.createSavedScriptApiV1ScriptsPost was
called and response-handling behavior occurred, and for the 404 fallback
configure mocks.updateSavedScriptApiV1ScriptsScriptIdPut to return 404, invoke
the same saved-script callback, and assert that
mocks.createSavedScriptApiV1ScriptsPost was called (not by calling the mock
directly). Locate the mocked child by name ScriptActions and the save-related
mocks createSavedScriptApiV1ScriptsPost /
updateSavedScriptApiV1ScriptsScriptIdPut and use renderEditor() to obtain the
bound props to trigger real component logic.
- Around line 94-129: The test mocks (Spinner, MockIcon in the '@lucide/svelte'
mock, and editor mocks CodeMirrorEditor, OutputPanel, LanguageSelect,
ResourceLimits, EditorToolbar, ScriptActions, SavedScripts) use Svelte 4
internals (the $$ object) which is incompatible with Svelte 5; update each
vi.mock to import createRawSnippet from 'svelte' and return components via
createRawSnippet that expose a render() returning the mock HTML (e.g., replace
the MockSpinner, MockIcon and the functions like CodeMirrorEditor.render = ...
with createRawSnippet(() => ({ render: () => '<...>' })) for each mocked
component so tests work with Svelte 5).
In `@frontend/src/routes/__tests__/Notifications.test.ts`:
- Around line 220-280: The delete tests in the "Delete" describe block silently
pass when the delete button lookup returns null because each test guards with
"if (deleteBtn) {...}" — replace those conditional guards in the three tests
(the ones that call renderNotifications and then find deleteBtn) with an
explicit assertion or throw (e.g., expect(deleteBtn).toBeDefined() or if
(!deleteBtn) throw new Error('delete button not found')) immediately after the
waitFor so the test fails if the selector fails; keep the rest of the logic that
clicks the button and asserts calls to mocks.mockNotificationStore.delete and
mocks.addToast unchanged (references: renderNotifications,
mocks.mockNotificationStore.delete, mocks.addToast, and the three tests in the
"Delete" suite).
- Around line 33-47: The mocks for Svelte components (MockSpinner and MockIcon)
use Svelte 4 internal shapes and must be converted to Svelte 5 component stubs:
change each vi.mock to async-import createRawSnippet from 'svelte' and return
default: createRawSnippet(() => ({ render: () => '<...>' })) so the mocked
component is a proper Svelte 5 function; likewise update the helper
createMockSvelteComponent in test-utils.ts to produce createRawSnippet-based
stubs instead of objects with a $$ property. Ensure the rendered HTML strings
(e.g., '<span data-testid="spinner">Loading</span>' and '<svg></svg>') remain
the same so tests keep the same selectors.
In `@frontend/src/routes/admin/__tests__/AdminSettings.test.ts`:
- Around line 71-85: The current vi.mock factories create MockSpinner and
MockIcon using the Svelte 4 internal `$$` structure which fails on Svelte 5;
replace these with Svelte‑5 compatible mocks by either (A) creating simple
Svelte files (e.g., MockSpinner.svelte with <span
data-testid="spinner">Loading</span> and MockIcon.svelte with <svg></svg>) and
update the vi.mock calls to return those modules (like the existing pattern used
around lines 67–69), or (B) inside the mock factory use Svelte's
createRawSnippet() to return a valid renderable snippet for MockSpinner and
MockIcon (instead of touching `$$`). Ensure you reference the same exported
symbols used in the tests (default export for Spinner and ShieldCheck for
`@lucide/svelte`) so AdminSettings.test imports render correctly.
🧹 Nitpick comments (19)
frontend/src/routes/__tests__/Login.test.ts (1)
55-61: Spinner mock uses Svelte 4 internal shape ($$,.render)Since this PR migrates to Svelte 5, the mock's
$$object and staticrendermethod reflect Svelte 4 internals. This works today because the mock just prevents import failures, but it may break if test infrastructure starts validating Svelte 5 component shapes. A simpler stub (e.g., returning an empty element or usingvi.fn()as the default export) would be more future-proof.frontend/src/routes/__tests__/Privacy.test.ts (2)
35-53: Consider consolidatingit.eachtests that re-render per iteration.Each
it.eachiteration callsrenderPrivacy(), so the component is rendered 14 times for section headings and 5 times for mailto links. You could collapse each group into a single test that renders once and asserts all items, improving test speed while retaining coverage. This is purely a performance/style nit—the current approach gives better per-heading failure messages.♻️ Example consolidation for section headings
- it.each([ - 'Hi there!', - "Who's responsible for your data?", - ... - ])('renders section heading "%s"', async (heading) => { - await renderPrivacy(); - expect(screen.getByRole('heading', { name: heading })).toBeInTheDocument(); - }); + it('renders all expected section headings', async () => { + await renderPrivacy(); + for (const heading of [ + 'Hi there!', + "Who's responsible for your data?", + 'What information do I collect?', + 'Why do I need this information?', + 'How long do I keep your data?', + 'Who else sees your data?', + 'How do I protect your data?', + 'Your rights (GDPR stuff)', + "Where's your data stored?", + 'About cookies', + 'Kids and this service', + 'The tech stack', + 'Changes to this policy', + 'Get in touch', + ]) { + expect(screen.getByRole('heading', { name: heading })).toBeInTheDocument(); + } + });Also applies to: 60-71
8-10: Consider restoring global stubs after the suite.
vi.stubGlobalreplacesglobalThis.scrollTobut there's noafterAll(vi.unstubAllGlobals)to restore it. While vitest isolates files by default, adding cleanup is a good defensive habit—especially if file-level isolation settings change.♻️ Add cleanup
beforeEach(() => { vi.stubGlobal('scrollTo', mocks.scrollTo); }); + +afterAll(() => { + vi.unstubAllGlobals(); +});frontend/e2e/settings.spec.ts (1)
169-169: Register the dialog handler before triggering the action that opens it.The current ordering (line 169 before line 170) is technically correct for Playwright, but the safer and more idiomatic pattern is to set up the dialog listener before any action that could trigger it — ideally right before the click or even earlier. If a future refactor moves the click or adds an intermediate action that triggers a dialog, this could miss it.
As-is this works, just flagging as a minor nit for robustness.
frontend/src/routes/__tests__/Register.test.ts (2)
43-49: Spinner mock uses Svelte 4 internal component shape.This mock returns
$$withon_mount,on_destroy, etc., and a staticrendermethod — both are Svelte 4 SSR/internal patterns. Since the PR migrates to Svelte 5, consider simplifying this to a minimal Svelte 5-compatible mock (e.g., a simple function component or a.sveltestub) to avoid confusion and potential breakage if@testing-library/sveltetightens its internal checks.Simpler mock alternative
-vi.mock('$components/Spinner.svelte', () => { - const MockSpinner = function() { - return { $$: { on_mount: [], on_destroy: [], before_update: [], after_update: [], context: new Map() } }; - }; - MockSpinner.render = () => ({ html: '<span>Loading</span>', css: { code: '', map: null }, head: '' }); - return { default: MockSpinner }; -}); +vi.mock('$components/Spinner.svelte', () => ({ + default: () => {}, +}));
4-4: Nit: unusual relative import path through$libalias.
$lib/../__tests__/test-utilsnavigates out of the$libalias directory. If the project has a$orsrcalias, a direct path likesrc/__tests__/test-utilswould be clearer. Not a functional issue — just a readability nit.#!/bin/bash # Check how other test files import from test-utils to see if there's an established pattern rg "test-utils" --type=ts -n frontend/src/routes/__tests__/ | head -20frontend/e2e/notifications.spec.ts (2)
107-107: Fragile selector for the delete button.Matching any
buttoncontaining ansvgwill break if the card gains another icon-button (e.g., a "pin" or "archive" action). Consider adding adata-testid="delete-notification"oraria-label="Delete notification"to the component and targeting that here instead.
86-123: All three tests silently pass when no notifications exist.Each test is wrapped in an
if (await ...isVisible())guard, so if the test environment has no notifications, these tests pass vacuously without exercising any action. Consider either:
- Seeding notification data in a
beforeEach/ fixture setup, or- Calling
test.skip()inside the guard so CI reports clearly show the tests were skipped rather than green.frontend/e2e/editor.spec.ts (1)
139-150: Consider extracting the repeated "save script" preamble into a helper.All three new tests duplicate the same ~8-line setup (load example → fill name → open options → save → await toast). A small helper like
saveScript(page, name)would reduce duplication and make future maintenance easier.Also applies to: 165-176, 191-197
frontend/src/routes/__tests__/Home.test.ts (2)
3-3: Consider using a cleaner import alias for test utilities.
$lib/../__tests__/test-utilsnavigates out of the$libalias, which is fragile and non-obvious. A dedicated path alias (e.g.,$tests/test-utilsorsrc/__tests__/test-utils) would be clearer. That said, if this pattern is already established across other test files in this PR, it's fine to keep consistent and address later.
39-48: Minor: redundant heading query on line 44.The heading is already located inside the
waitForon line 42. You can reuse that result instead of querying again.Proposed simplification
it('renders hero heading with "Code, Run,", "Integrate", and "Instantly."', async () => { await renderHome(); - await waitFor(() => { - expect(screen.getByRole('heading', { level: 1 })).toBeInTheDocument(); - }); - const heading = screen.getByRole('heading', { level: 1 }); + const heading = await screen.findByRole('heading', { level: 1 }); expect(heading.textContent).toContain('Code, Run,'); expect(heading.textContent).toContain('Integrate'); expect(heading.textContent).toContain('Instantly.'); });frontend/src/routes/__tests__/Settings.test.ts (1)
253-262: Cache test assertion at line 261 may be timing-sensitive.After clicking "View History" the second time, the assertion
toHaveBeenCalledTimes(1)is outside anywaitFor. If the component performs any async work before deciding to use the cache (e.g., checking a reactive timestamp), this could intermittently fail. Consider wrapping in a small delay orwaitForthat also asserts the modal re-appeared before checking call count.Slightly more robust approach
await user.click(screen.getByRole('button', { name: 'Close' })); await user.click(screen.getByRole('button', { name: /view history/i })); + await waitFor(() => { + expect(screen.getByRole('heading', { name: 'Settings History' })).toBeInTheDocument(); + }); expect(mocks.getSettingsHistoryApiV1UserSettingsHistoryGet).toHaveBeenCalledTimes(1);frontend/src/routes/__tests__/Editor.test.ts (1)
132-132:useris declared but never used in any test.
userEvent.setup()is initialized but no test in this file callsuser.click(),user.type(), or any other interaction method. This indicates the tests don't exercise any actual user interactions, which is a gap for an Editor component.frontend/src/routes/admin/__tests__/AdminSettings.test.ts (2)
172-186: Consider asserting the error toast is shown on save failure.The error path test only asserts that a success toast was not shown. It doesn't verify that an error toast was shown. If the component is expected to display an error notification on failure, asserting that would strengthen coverage.
expect(mocks.addToast).not.toHaveBeenCalledWith('success', expect.anything()); + expect(mocks.addToast).toHaveBeenCalledWith('error', expect.anything());
232-278:resolveSave/resolveResetused with!assertion before guaranteed assignment.Lines 234 and 259:
resolveSaveandresolveResetare declared without initialization, then used with!(non-null assertion). While this works in practice because the Promise constructor runs synchronously insidemockImplementation, TypeScript strict mode may flag these as "used before assigned." Initializing with a no-op avoids the lint warning and makes intent clearer.Proposed fix
- let resolveSave: (v: unknown) => void; + let resolveSave: (v: unknown) => void = () => {};- let resolveReset: (v: unknown) => void; + let resolveReset: (v: unknown) => void = () => {};frontend/src/routes/admin/__tests__/AdminLayout.test.ts (2)
37-51: Extract duplicated mock factories into shared test utilities.The Spinner mock (lines 37-43), Icon mock (lines 45-51), and toast mock (lines 28-35) are duplicated verbatim between
AdminLayout.test.tsandAdminSettings.test.ts. Thetest-utils.tsfile already exists in this directory and exportsmockElementAnimate— these shared mocks could live there too.This would reduce maintenance overhead when mocking patterns need to change (e.g., adapting to Svelte 5 component internals).
Also applies to: 71-77, 79-85
102-114: UsingclassList.toString().match(...)— considertoContainfor clarity.
link.classList.toString()returns a space-separated string, sotoMatch(/bg-primary/)works. A minor alternative isexpect(link.className).toContain('bg-primary'), which is slightly more readable and avoids the regex. Not a blocking issue.frontend/src/routes/__tests__/Notifications.test.ts (2)
227-229: Fragile delete-button selector relies on Tailwind utility class.Locating the button via
classList.contains('text-red-600')ties the test to a specific Tailwind class. Consider adding a stabledata-testid="delete-notification"to the delete button inNotifications.svelteand querying by that instead. The fallback[data-testid="trash-icon"]is better but the mock forTrash2(line 46) doesn't render adata-testid, so that branch never matches either.
88-96:channelproperty forced via type cast — update the mock helper instead.
channelis cast onto the notification object (line 95) becausecreateMockNotificationdoesn't include it in its type. If the component renders this field, the helper's type and default should be updated to includechannelso the cast isn't needed and other tests can use it too.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/routes/__tests__/Notifications.test.ts`:
- Around line 70-86: The test title claims to verify "channel" but the mock
created by createMockNotification (and the assertions after renderNotifications)
never include or check a channel; either add a channel value to the mock (e.g.,
include channel: 'email' in the object passed to createMockNotification and then
assert screen.getByText('email') or the specific rendered channel element is
present) or change the it(...) title to remove "channel" so it correctly
reflects what's asserted; update references around createMockNotification and
the assertions after renderNotifications (and
mocks.mockNotificationStore.notifications) accordingly.
🧹 Nitpick comments (10)
frontend/src/routes/__tests__/Home.test.ts (2)
3-3: Nit: unusual import path with$lib/../__tests__/.This resolves correctly but reads oddly — navigating out of
$libto reach__tests__. If there's a path alias like$__tests__or$srcavailable (or one could be added to the Vite/TS config), it would be cleaner. Not blocking since this pattern is consistent across sibling test files.
49-59: Nit: hardcoded UI copy in parameterized tests.The full description strings (e.g.,
'Run code online effortlessly in isolated Kubernetes pods with near-native speed.') are brittle to copy edits. Consider using substring or regex matchers if exact wording isn't the contract under test. That said, this is a common and acceptable pattern for route-level smoke tests.frontend/src/__tests__/test-utils.ts (2)
197-209: Optional: Extract shared mock component builder to reduce duplication withcreateMockSvelteComponent.The Mock constructor body (lines 200-204) and
render()shape (line 205) are identical tocreateMockSvelteComponent(lines 88-104). A small shared helper likebuildMockComponent(html)could be extracted and reused by both functions. Low priority since this is test-only code.
240-245: Nit: Simplifygotoassignment.The wrapping arrow function on line 242 is redundant when
gotoFnis already a function —gotoFn ?? vi.fn()achieves the same result more concisely.Proposed simplification
export function createMockRouterModule(gotoFn?: (...args: unknown[]) => void) { return { - goto: gotoFn ? (...args: unknown[]) => gotoFn(...args) : vi.fn(), + goto: gotoFn ?? vi.fn(), route: () => {}, }; }frontend/src/routes/__tests__/Register.test.ts (1)
107-125: Consider assertinggetErrorMessagewas called with the API error.The test verifies the output of
getErrorMessageappears in the DOM and toast, but doesn't confirm the component passed the correct error object to it. Adding an assertion like:expect(mocks.mockGetErrorMessage).toHaveBeenCalledWith( { detail: 'Username taken' }, expect.any(String) );would catch regressions where the component stops forwarding the API error to the utility.
frontend/src/routes/__tests__/Editor.test.ts (1)
101-101: Unuseduservariable.
userEvent.setup()is assigned but never used in any test. Remove it or use it in the tests that should be exercising user interactions (save, delete, execution flows).- const user = userEvent.setup();frontend/src/routes/admin/__tests__/AdminLayout.test.ts (3)
34-39:beforeEachonly resets two of the sixmockAuthStoreproperties.
clearAllMocksresetsverifyAuth(the onlyvi.fn()), but the plain-value properties (isAuthenticated,userId,userEmail,csrfToken) are not restored. If a future test mutates any of them, later tests will inherit stale state. Consider resetting the full object to prevent subtle ordering-dependent failures.Suggested fix
beforeEach(() => { vi.clearAllMocks(); mockElementAnimate(); mocks.mockAuthStore.userRole = 'admin'; mocks.mockAuthStore.username = 'adminuser'; + mocks.mockAuthStore.isAuthenticated = true; + mocks.mockAuthStore.userId = 'user-1'; + mocks.mockAuthStore.userEmail = 'admin@test.com'; + mocks.mockAuthStore.csrfToken = 'token'; });
86-93: Consider usingtoHaveClassinstead ofclassList.toString()+ regex.
link.classList.toString().toMatch(/bg-primary/)works but is fragile—it matches substrings (e.g., would also match a hypotheticalbg-primary-dark).@testing-library/jest-dom'stoHaveClassor anexpect(link).toHaveClass(expect.stringContaining('bg-primary'))pattern is more idiomatic in testing-library tests.That said, since Tailwind classes are typically exact tokens, this is a minor nit.
46-53:mockGotoassertion is outsidewaitFor— verify ordering is reliable.Line 52 asserts
mockGotowas called, but this assertion lives outside thewaitForblock. If the component's redirect is triggered asynchronously (e.g., in a$effectoronMountcallback that runs after the toast), this could be flaky. If both the toast and goto are guaranteed to fire in the same synchronous block, this is fine.frontend/src/routes/__tests__/Notifications.test.ts (1)
258-278: Filter test looks good overall.One subtle note:
store.loadis called once on mount (with100) and again on filter click (with100, { ... }). ThetoHaveBeenCalledWithmatcher checks if any call matched the expected args, so this works correctly, but if you ever want to be more precise,toHaveBeenLastCalledWithwould be more explicit.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/routes/Editor.svelte (1)
94-128:⚠️ Potential issue | 🟡 Minor
loadSavedScripts()called twice on initial mount for authenticated users.When
authStore.verifyAuth()(line 110) succeeds,authenticatedflips fromfalsetotrue. This triggers the$effecton line 97 (!prevAuth && isAuth), which callsloadSavedScripts(). Then line 127 also callsloadSavedScripts()becauseauthenticatedis now true. This results in two concurrent API requests for the same data on every page load for a logged-in user.Remove the redundant call in
onMount, since the effect already handles the transition:Proposed fix
- if (authenticated) await loadSavedScripts(); + // loadSavedScripts is triggered by the auth-change $effect when authenticated becomes true });frontend/src/lib/api-interceptors.ts (1)
99-111:⚠️ Potential issue | 🟡 MinorFix ESLint pipeline failure: unnecessary optional chain on
request.The CI pipeline reports
@typescript-eslint/no-unnecessary-conditionon line 100. Therequestparameter in the error interceptor is typed as non-nullable, sorequest?.urlshould berequest.url.Proposed fix
client.interceptors.error.use((error, response, request, _opts) => { const status = response?.status; - const url = request?.url || ''; + const url = request.url || ''; const isAuthEndpoint = AUTH_ENDPOINTS.some(ep => url.includes(ep));frontend/src/components/__tests__/ProtectedRoute.test.ts (1)
239-261:⚠️ Potential issue | 🟡 MinorNo functional issue with test correctness, but minor mock setup inconsistency.
The
ProtectedRoutecomponent does not actually use the return value ofwaitForInit()— it only awaits it as a synchronization mechanism, then checksauthStore.isAuthenticatedto determine auth status. The "props handling" tests will pass for the correct reason (withisAuthenticated = false), regardless of whetherwaitForInitresolves totrueorfalse. That said, for code clarity and consistency with the "unauthenticated state"beforeEach, the "props handling" tests should explicitly setmocks.mockAuthStore.waitForInit.mockResolvedValue(false)to match the other unauthenticated scenarios.
🤖 Fix all issues with AI agents
In `@frontend/src/routes/Settings.svelte`:
- Around line 189-210: The restoreSettings function may access data.theme when
data can be undefined from restoreSettingsApiV1UserSettingsRestorePost; update
the function to guard against a missing data object by checking data and
data.theme before calling setTheme (e.g., only call setTheme if data?.theme is
truthy), and ensure any follow-up logic that assumes data exists is similarly
protected; references: restoreSettings,
restoreSettingsApiV1UserSettingsRestorePost, data, setTheme, loadSettings.
🧹 Nitpick comments (10)
frontend/src/__tests__/test-utils.ts (2)
74-90: Stale JSDoc: references$$structure that was removed.Line 75 still says "Creates a mock Svelte 5 component with proper $$ structure" but the constructor on line 88 no longer creates any
$$property. Update the doc to match the current implementation.
183-199: Stale doc + duplicated mock-component pattern.
- Line 185 mentions "$$ structure" but no such structure exists in the implementation.
- The mock component creation logic (lines 192-195) duplicates
createMockSvelteComponent(lines 88-96). Consider extracting a shared internal helper to keep mock shape consistent in one place.♻️ Suggested refactor
+/** Internal helper: creates a single mock Svelte 5 component. */ +function buildMockComponent(html: string) { + const Mock = function () { + return {}; + } as unknown as { new (): object; render: () => { html: string; css: { code: string; map: null }; head: string } }; + Mock.render = () => ({ html, css: { code: '', map: null }, head: '' }); + return Mock; +} + export function createMockSvelteComponent(html: string, testId?: string) { const htmlWithTestId = testId ? html.replace('>', ` data-testid="${testId}">`) : html; - const MockComponent = function () { - return {}; - } as unknown as { new (): object; render: () => { html: string; css: { code: string; map: null }; head: string } }; - MockComponent.render = () => ({ - html: htmlWithTestId, - css: { code: '', map: null }, - head: '', - }); + const MockComponent = buildMockComponent(htmlWithTestId); return { default: MockComponent }; } export function createMockNamedComponents(components: Record<string, string>): Record<string, unknown> { const module: Record<string, unknown> = {}; for (const [name, html] of Object.entries(components)) { - const Mock = function () { - return {}; - } as unknown as { new (): object; render: () => { html: string; css: { code: string; map: null }; head: string } }; - Mock.render = () => ({ html, css: { code: '', map: null }, head: '' }); - module[name] = Mock; + module[name] = buildMockComponent(html); } return module; }frontend/e2e/notifications.spec.ts (2)
87-99: Potential stale locator after marking as read.
notificationCardis bound to[aria-label="Mark notification as read"]. After clicking (Line 93), if the component updates or removes that aria-label on the now-read card, this Playwright locator will no longer resolve to the same element, making the assertion on Line 96 unreliable.Consider capturing a more stable locator (e.g., by card index or a
data-testid) before clicking, or assert against the broader list rather than the clicked element.Additionally, the nested
ifguards (Lines 90, 95) mean the test silently passes when no notifications exist or none are unread—this can mask regressions. Consider usingtest.skipwith a message when preconditions aren't met.Suggested approach
test('can mark notification as read by clicking', async ({ userPage }) => { await gotoAndWaitForNotifications(userPage); const notificationCard = userPage.locator('[aria-label="Mark notification as read"]').first(); - if (await notificationCard.isVisible({ timeout: 3000 }).catch(() => false)) { - // Check if it's unread (has blue background class) - const hasBlue = await notificationCard.evaluate(el => el.classList.toString().includes('bg-blue')); - await notificationCard.click(); - // After clicking, check if styling changed or "Read" label appeared - if (hasBlue) { - await expect(notificationCard.locator('text=Read')).toBeVisible({ timeout: 3000 }); - } - } + const isVisible = await notificationCard.isVisible({ timeout: 3000 }).catch(() => false); + if (!isVisible) { + test.skip(true, 'No unread notifications available to test'); + return; + } + const hasBlue = await notificationCard.evaluate(el => el.classList.toString().includes('bg-blue')); + await notificationCard.click(); + if (hasBlue) { + // Assert the blue background is gone, indicating the card was marked read + await expect(notificationCard).not.toHaveClass(/bg-blue/, { timeout: 3000 }); + } });
101-111: Fragile delete-button locator and unchecked toast content.Line 105: the locator
notificationCard.locator('button').filter({ has: userPage.locator('svg') }).first()will match any button containing an SVG inside the card. If the card has multiple icon buttons (e.g., mark-read, delete), this could click the wrong one. Prefer a more specific selector like[aria-label="Delete notification"]or adata-testid.Also,
expectToastVisible(Line 108) only checks that a toast appeared—it doesn't distinguish a success toast from an error toast. Consider extending the assertion to verify the toast message content.frontend/src/routes/Editor.svelte (1)
107-128:onMounthas no error handling — a failedverifyAuth()or limits call prevents the editor from rendering.If
authStore.verifyAuth()or the limits/examples API calls throw (e.g., network error), the entireonMountaborts with an unhandled rejection. Consider wrapping the initialization in a try/catch with user-facing feedback, or at minimum wrappingverifyAuthso the editor can still function in a degraded (unauthenticated) mode.frontend/src/components/__tests__/Header.test.ts (1)
77-93: Consider resettinglogin,verifyAuth, andfetchUserProfilemocks too.
logoutandmockToggleThemeare reset inbeforeEach, butlogin,verifyAuth, andfetchUserProfileare not. Whilevi.clearAllMocks()isn't called here (only individual.mockReset()calls), if any test ever asserts on these mocks, stale state could leak between tests.Proposed fix
mocks.mockThemeStore.value = 'auto'; mocks.mockAuthStore.logout.mockReset(); + mocks.mockAuthStore.login.mockReset(); + mocks.mockAuthStore.verifyAuth.mockReset(); + mocks.mockAuthStore.fetchUserProfile.mockReset(); mocks.mockToggleTheme.mockReset(); mocks.mockGoto.mockReset();frontend/src/routes/admin/__tests__/AdminSettings.test.ts (2)
49-49: Inconsistent auth store mock pattern compared to other test files.Header.test.ts and ProtectedRoute.test.ts use a getter (
get authStore() { return mocks.mockAuthStore; }) for the auth store mock, while this file uses a direct reference (authStore: mocks.mockAuthStore). Both work because the same object reference is mutated, but using getters is the safer pattern (survives object reassignment) and would be more consistent across the test suite.Proposed fix
-vi.mock('$stores/auth.svelte', () => ({ authStore: mocks.mockAuthStore })); +vi.mock('$stores/auth.svelte', () => ({ + get authStore() { return mocks.mockAuthStore; }, +}));
66-67:userEvent.setup()called once at describe scope — prefer per-test setup.The
@testing-library/user-eventdocumentation recommends callinguserEvent.setup()per test to avoid state leakage between tests (e.g., keyboard modifiers held down). Other test files in this PR (Header.test.ts) correctly calluserEvent.setup()inside individual test helpers.Proposed fix — move setup into the helper or each test
describe('AdminSettings', () => { - const user = userEvent.setup(); beforeEach(() => { ... async function renderAdminSettings() { const { default: AdminSettings } = await import('$routes/admin/AdminSettings.svelte'); - return render(AdminSettings); + const result = render(AdminSettings); + return { ...result, user: userEvent.setup() }; }Then update call sites to destructure
userfromrenderAdminSettings():- await user.click(screen.getByRole('button', { name: /save settings/i })); + const { user } = await renderAdminSettings(); + // ... + await user.click(screen.getByRole('button', { name: /save settings/i }));frontend/src/routes/Settings.svelte (2)
71-90:mapApiToFormData—||vs??for numeric fields.Lines 83-84 use
||forfont_sizeandtab_size, meaning a legitimate API value of0would be replaced by the default. While0isn't a practical font/tab size, using??would be more semantically correct and consistent with the??usage for booleans on lines 85-87.Proposed fix
editor: { theme: data.editor?.theme || 'auto', - font_size: data.editor?.font_size || 14, - tab_size: data.editor?.tab_size || 4, + font_size: data.editor?.font_size ?? 14, + tab_size: data.editor?.tab_size ?? 4, use_tabs: data.editor?.use_tabs ?? false, word_wrap: data.editor?.word_wrap ?? true, show_line_numbers: data.editor?.show_line_numbers ?? true, }
116-128:loadSettingsdoes not show an error toast on failure.When the API call fails (line 119), the function silently returns without notifying the user. The API interceptor may handle generic errors, but if you rely on that, consider adding a comment to clarify. Otherwise, users see an empty form with no explanation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/lib/api-interceptors.ts`:
- Around line 77-83: In the 422 validation-error branch (the if block checking
status === 422 in frontend/src/lib/api-interceptors.ts) remove the embedded "\n"
from the toast message so the call to toast.error uses a single-line string
(e.g. change toast.error(`Validation error:\n${getErrorMessage(error)}`) to a
string without the newline) — update the toast.error invocation that references
getErrorMessage(error) to concatenate or format without "\n".
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/lib/api-interceptors.ts (1)
99-112:⚠️ Potential issue | 🔴 CriticalFix error interceptor signature to match the generated client's type contract.
The
ErrInterceptortype expects(error, response: Response, request, options), but the generated client passesundefinedfor response in error scenarios (see client.gen.ts line 110). Update the callback signature to accurately reflect this—either by removing the| undefinedfrom response (if the client is corrected upstream) or by updating the type toresponse: Response | undefinedif this is intentional. Ensure all parameters are properly typed.
🧹 Nitpick comments (1)
frontend/src/lib/api-interceptors.ts (1)
41-58:isHandling401debounce relies on a fixed 1-second timeout.The
setTimeoutat line 54 resetsisHandling401after 1 second. Ifgoto('/login')is slow or fails (e.g., network issues delaying route resolution), a subsequent 401 arriving after the timeout could trigger a second redirect and toast. Conversely, rapid 401s within the 1s window are correctly coalesced. This is acceptable for a SPA but worth noting as a minor fragility.
|
1 participant
Summary by cubic
Move the frontend to Svelte 5 with class-based $state/$derived stores and a unified auth/notification model. Tightens types and linting, hardens streaming, and stabilizes tests with new route coverage and e2e helpers.
Refactors
Bug Fixes
Written for commit 4bf75b3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation / Public API