Conversation
📝 WalkthroughWalkthroughAdds a centralized consola-based logger, standardizes date handling with date-fns and new formatter APIs, inlines/removes the auto-refresh abstraction across admin stores, deletes several admin UI components and related tests, and replaces manual lifecycle listeners with Svelte reactive/global bindings. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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 56 files
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/formatters.ts">
<violation number="1" location="frontend/src/lib/formatters.ts:22">
P2: `formatTimestamp` is no longer actually locale-aware; it now uses `date-fns` default locale instead of the user/browser locale.</violation>
</file>
<file name="frontend/src/lib/admin/stores/__tests__/eventsStore.test.ts">
<violation number="1" location="frontend/src/lib/admin/stores/__tests__/eventsStore.test.ts:400">
P2: `vi.clearAllTimers()` masks whether teardown really stops auto-refresh, so this test can pass even when teardown is broken.</violation>
</file>
<file name="frontend/src/stores/__tests__/errorStore.test.ts">
<violation number="1" location="frontend/src/stores/__tests__/errorStore.test.ts:54">
P2: This assertion is too permissive and no longer verifies the expected log content. Use `toHaveBeenCalledWith(...)` (or equivalent argument assertions) so the test fails when the logged title/message format regresses.</violation>
</file>
<file name="frontend/src/lib/admin/stores/executionsStore.svelte.ts">
<violation number="1" location="frontend/src/lib/admin/stores/executionsStore.svelte.ts:28">
P2: The interval triggers `loadData` even when a previous refresh is still in flight, which can cause overlapping requests and race-prone state updates.</violation>
</file>
<file name="frontend/src/stores/auth.svelte.ts">
<violation number="1" location="frontend/src/stores/auth.svelte.ts:166">
P1: `login()` can return success even when forced post-login verification fails and clears auth state, causing callers to treat a failed/invalid session as a successful login.</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: 4
🧹 Nitpick comments (3)
frontend/src/stores/__tests__/errorStore.test.ts (1)
49-55: Tighten logging assertions to avoid false positives.
toHaveBeenCalled()is permissive and may pass from unrelated console activity. A call-count delta assertion aroundsetError(...)is more deterministic.Suggested test assertion tightening
it('logs error via consola', async () => { const { appError } = await import('$stores/errorStore.svelte'); const consoleSpy = vi.spyOn(console, 'error'); + const before = consoleSpy.mock.calls.length; appError.setError('Logged error', 'Error Title'); - expect(consoleSpy).toHaveBeenCalled(); + expect(consoleSpy.mock.calls.length).toBe(before + 1); }); it('logs error without title via consola', async () => { const { appError } = await import('$stores/errorStore.svelte'); const consoleSpy = vi.spyOn(console, 'error'); + const before = consoleSpy.mock.calls.length; appError.setError('Logged error without title'); - expect(consoleSpy).toHaveBeenCalled(); + expect(consoleSpy.mock.calls.length).toBe(before + 1); });Also applies to: 57-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/__tests__/errorStore.test.ts` around lines 49 - 55, The test uses a permissive expect(consoleSpy).toHaveBeenCalled() which can pass from unrelated console activity; update the assertion around appError.setError to assert a call-count delta instead (e.g., record initial consoleSpy.mock.calls.length or clear the spy, invoke appError.setError('Logged error','Error Title'), then assert the spy was called exactly once more using toHaveBeenCalledTimes(1) or by comparing before/after counts). Apply the same change to the second test covering lines 57-63 so both use deterministic call-count assertions against console.error when calling appError.setError.frontend/src/routes/admin/AdminExecutions.svelte (1)
29-39: Debounce search-driven reloads to prevent request bursts.The effect currently reloads on every
userSearchkeystroke, which can generate excessive API calls and race responses.♻️ Suggested debounce for search-triggered reloads
let prevFilters = $state({ status: store.statusFilter, priority: store.priorityFilter, search: store.userSearch }); + let searchReloadTimer: ReturnType<typeof setTimeout> | null = null; $effect(() => { const current = { status: store.statusFilter, priority: store.priorityFilter, search: store.userSearch }; - if (current.status !== prevFilters.status || current.priority !== prevFilters.priority || current.search !== prevFilters.search) { + const changed = + current.status !== prevFilters.status || + current.priority !== prevFilters.priority || + current.search !== prevFilters.search; + + if (changed) { + const searchChanged = current.search !== prevFilters.search; prevFilters = current; untrack(() => { store.pagination.currentPage = 1; - store.loadExecutions(); + if (searchReloadTimer) clearTimeout(searchReloadTimer); + if (searchChanged) { + searchReloadTimer = setTimeout(() => { + void store.loadExecutions(); + }, 250); + } else { + void store.loadExecutions(); + } }); } + + return () => { + if (searchReloadTimer) clearTimeout(searchReloadTimer); + }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/admin/AdminExecutions.svelte` around lines 29 - 39, The effect currently calls store.loadExecutions on every change including each keystroke in store.userSearch; debounce the search-driven reloads so rapid keystrokes don't trigger multiple API calls: inside the $effect that compares current vs prevFilters (variables: prevFilters, current), detect if the only changed key is search (compare status and priority unchanged but search changed) and in that branch schedule a debounced reload (use a timeout/clearTimeout stored in a closure or component-level variable) to set store.pagination.currentPage = 1 and call store.loadExecutions after a short delay (e.g., 300ms); keep immediate reload behavior for status/priority changes (call untrack + store.loadExecutions immediately). Ensure you clear the timeout on component destroy to avoid leaks.frontend/src/lib/admin/stores/eventsStore.svelte.ts (1)
42-46: Guard interval refreshes to avoid overlappingloadAll()calls.This interval can trigger a new refresh before the previous one finishes, causing concurrent API calls and potential state races.
♻️ Suggested in-flight guard for interval refresh
class EventsStore { + private refreshInFlight = false; + constructor() { $effect(() => { - const id = setInterval(() => this.loadAll(), 30_000); + const id = setInterval(() => { + if (this.refreshInFlight) return; + this.refreshInFlight = true; + void this.loadAll().finally(() => { + this.refreshInFlight = false; + }); + }, 30_000); return () => { clearInterval(id); }; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/admin/stores/eventsStore.svelte.ts` around lines 42 - 46, The interval in the constructor that calls this.loadAll() can start a new refresh while a previous one is still running; add an in-flight guard (e.g. a boolean like this._isLoading or a simple Promise lock) checked inside the $effect callback before invoking loadAll(), set the guard just before the asynchronous work starts and clear it in a finally block after loadAll() completes (or await loadAll() inside the guarded section), so the interval handler skips starting a new call when a previous call is still in-flight; update references in constructor/$effect and the loadAll-related logic to use this guard.
🤖 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/app.css`:
- Around line 114-143: Add Tailwind directives to the stylelint config and
replace the invalid :where dark nesting inside the `@utility` block: update
.stylelintrc.json to include ignoreAtRules entries for "utility", "apply",
"variant", and "theme" so `@utility/`@apply/@variant are not flagged, then modify
the `@utility` form-control-base block in frontend/src/app.css to remove the
:where(.dark, .dark *) selector and instead nest the dark styles using
Tailwind's `@variant` dark (e.g., move the dark-specific
border-color/background-color/color/hover rules under an `@variant` dark block
within the same file); also update other places that use `@apply`
form-control-base (lines noted in the review) to remain compatible with the new
`@variant-based` dark styles.
In `@frontend/src/components/NotificationCenter.svelte`:
- Around line 163-165: The custom button element in NotificationCenter.svelte
(role="button", tabindex="0") only handles Enter activation; update the keyboard
handler so handleNotificationClick(notification) is invoked for both Enter and
Space key presses—modify the onkeydown callback that references
handleNotificationClick(notification) to check for e.key === 'Enter' OR e.key
=== ' ' OR e.code === 'Space' (and/or 'Spacebar' for legacy) and call
handleNotificationClick accordingly to ensure full keyboard accessibility.
In `@frontend/src/stores/notificationStore.svelte.ts`:
- Line 42: The add() method currently prepends new notifications with
"this.notifications = [notification, ...this.notifications]" causing unbounded
growth; change it to keep prepend behavior but clamp length by defining a
MAX_NOTIFICATIONS (e.g., 50) and after prepending assign only the first
MAX_NOTIFICATIONS items (e.g., this.notifications = [notification,
...this.notifications].slice(0, MAX_NOTIFICATIONS)). Update the
notificationStore export to include the MAX_NOTIFICATIONS constant so it’s easy
to tune and test.
In `@frontend/src/styles/components.css`:
- Line 104: The stylelint violation is caused by a missing empty line before the
background-image declaration; open the CSS block containing the
background-image: url('data:image/svg+xml;...') declaration and insert a single
blank line immediately above that declaration (i.e., add an empty line before
the "background-image: url(...)" rule) so the declaration-empty-line-before rule
is satisfied, or alternatively update the stylelint rule configuration to allow
this pattern if preferred.
---
Nitpick comments:
In `@frontend/src/lib/admin/stores/eventsStore.svelte.ts`:
- Around line 42-46: The interval in the constructor that calls this.loadAll()
can start a new refresh while a previous one is still running; add an in-flight
guard (e.g. a boolean like this._isLoading or a simple Promise lock) checked
inside the $effect callback before invoking loadAll(), set the guard just before
the asynchronous work starts and clear it in a finally block after loadAll()
completes (or await loadAll() inside the guarded section), so the interval
handler skips starting a new call when a previous call is still in-flight;
update references in constructor/$effect and the loadAll-related logic to use
this guard.
In `@frontend/src/routes/admin/AdminExecutions.svelte`:
- Around line 29-39: The effect currently calls store.loadExecutions on every
change including each keystroke in store.userSearch; debounce the search-driven
reloads so rapid keystrokes don't trigger multiple API calls: inside the $effect
that compares current vs prevFilters (variables: prevFilters, current), detect
if the only changed key is search (compare status and priority unchanged but
search changed) and in that branch schedule a debounced reload (use a
timeout/clearTimeout stored in a closure or component-level variable) to set
store.pagination.currentPage = 1 and call store.loadExecutions after a short
delay (e.g., 300ms); keep immediate reload behavior for status/priority changes
(call untrack + store.loadExecutions immediately). Ensure you clear the timeout
on component destroy to avoid leaks.
In `@frontend/src/stores/__tests__/errorStore.test.ts`:
- Around line 49-55: The test uses a permissive
expect(consoleSpy).toHaveBeenCalled() which can pass from unrelated console
activity; update the assertion around appError.setError to assert a call-count
delta instead (e.g., record initial consoleSpy.mock.calls.length or clear the
spy, invoke appError.setError('Logged error','Error Title'), then assert the spy
was called exactly once more using toHaveBeenCalledTimes(1) or by comparing
before/after counts). Apply the same change to the second test covering lines
57-63 so both use deterministic call-count assertions against console.error when
calling appError.setError.
ℹ️ 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 (55)
frontend/package.jsonfrontend/rollup.config.jsfrontend/src/App.sveltefrontend/src/__tests__/test-utils.tsfrontend/src/app.cssfrontend/src/components/Header.sveltefrontend/src/components/Modal.sveltefrontend/src/components/NotificationCenter.sveltefrontend/src/components/ProtectedRoute.sveltefrontend/src/components/__tests__/Header.test.tsfrontend/src/components/__tests__/NotificationCenter.test.tsfrontend/src/components/__tests__/ProtectedRoute.test.tsfrontend/src/components/admin/ActionButtons.sveltefrontend/src/components/admin/StatsCard.sveltefrontend/src/components/admin/StatusBadge.sveltefrontend/src/components/admin/__tests__/ActionButtons.test.tsfrontend/src/components/admin/__tests__/StatsCard.test.tsfrontend/src/components/admin/__tests__/StatusBadge.test.tsfrontend/src/components/admin/events/EventsTable.sveltefrontend/src/components/admin/events/__tests__/EventsTable.test.tsfrontend/src/components/admin/index.tsfrontend/src/lib/__tests__/formatters.test.tsfrontend/src/lib/admin/__tests__/autoRefresh.test.tsfrontend/src/lib/admin/__tests__/constants.test.tsfrontend/src/lib/admin/autoRefresh.svelte.tsfrontend/src/lib/admin/constants.tsfrontend/src/lib/admin/index.tsfrontend/src/lib/admin/stores/__tests__/eventsStore.test.tsfrontend/src/lib/admin/stores/__tests__/executionsStore.test.tsfrontend/src/lib/admin/stores/__tests__/sagasStore.test.tsfrontend/src/lib/admin/stores/eventsStore.svelte.tsfrontend/src/lib/admin/stores/executionsStore.svelte.tsfrontend/src/lib/admin/stores/sagasStore.svelte.tsfrontend/src/lib/api-interceptors.tsfrontend/src/lib/formatters.tsfrontend/src/lib/logger.tsfrontend/src/lib/notifications/stream.svelte.tsfrontend/src/main.tsfrontend/src/routes/Editor.sveltefrontend/src/routes/Login.sveltefrontend/src/routes/Notifications.sveltefrontend/src/routes/Settings.sveltefrontend/src/routes/__tests__/Notifications.test.tsfrontend/src/routes/admin/AdminEvents.sveltefrontend/src/routes/admin/AdminExecutions.sveltefrontend/src/routes/admin/AdminSagas.sveltefrontend/src/routes/admin/__tests__/AdminUsers.test.tsfrontend/src/stores/__tests__/auth.test.tsfrontend/src/stores/__tests__/errorStore.test.tsfrontend/src/stores/__tests__/notificationStore.test.tsfrontend/src/stores/auth.svelte.tsfrontend/src/stores/errorStore.svelte.tsfrontend/src/stores/notificationStore.svelte.tsfrontend/src/stores/theme.svelte.tsfrontend/src/styles/components.css
💤 Files with no reviewable changes (16)
- frontend/src/components/tests/Header.test.ts
- frontend/src/lib/admin/tests/constants.test.ts
- frontend/src/components/admin/StatsCard.svelte
- frontend/src/lib/admin/constants.ts
- frontend/src/lib/admin/index.ts
- frontend/src/lib/admin/autoRefresh.svelte.ts
- frontend/src/components/admin/StatusBadge.svelte
- frontend/src/components/admin/tests/StatusBadge.test.ts
- frontend/src/components/tests/ProtectedRoute.test.ts
- frontend/src/lib/admin/tests/autoRefresh.test.ts
- frontend/src/components/admin/ActionButtons.svelte
- frontend/src/components/admin/tests/StatsCard.test.ts
- frontend/src/components/admin/index.ts
- frontend/src/components/ProtectedRoute.svelte
- frontend/src/components/admin/tests/ActionButtons.test.ts
- frontend/src/tests/test-utils.ts
There was a problem hiding this comment.
Pull request overview
This PR modernizes the frontend by introducing a unified logging + formatting layer, refactoring admin auto-refresh behavior, and tightening up several UI interaction patterns (dropdowns, modal escape handling, shared form control styles).
Changes:
- Add a shared browser logger (
$lib/logger) and replace manyconsole.*calls with tagged logging. - Centralize timestamp/relative-time formatting in
$lib/formatters(now usingdate-fns) and update UI/tests accordingly. - Refactor admin auto-refresh from a shared helper to store-local
$effectintervals; remove several unused admin UI components/utilities.
Reviewed changes
Copilot reviewed 55 out of 56 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/styles/components.css | Adds data-URI dropdown arrow styling for the shared pagination selector class. |
| frontend/src/stores/theme.svelte.ts | Adds logger usage and error handling when persisting theme settings. |
| frontend/src/stores/notificationStore.svelte.ts | Removes notification list size cap when adding new notifications. |
| frontend/src/stores/errorStore.svelte.ts | Switches from console.error to shared logger. |
| frontend/src/stores/auth.svelte.ts | Switches to shared logger; replaces post-login profile fetch with verifyAuth(true). |
| frontend/src/stores/tests/notificationStore.test.ts | Removes tests asserting the 100-notification cap; renames unreadCount suite. |
| frontend/src/stores/tests/errorStore.test.ts | Updates logging assertions after switching to logger/consola. |
| frontend/src/stores/tests/auth.test.ts | Adjusts fixtures/expectations around profile response and removed fetchUserProfile. |
| frontend/src/routes/admin/tests/AdminUsers.test.ts | Updates mocks to use $lib/* aliases for API imports. |
| frontend/src/routes/admin/AdminSagas.svelte | Updates filter-change behavior using untrack; binds auto-refresh to new store fields. |
| frontend/src/routes/admin/AdminExecutions.svelte | Uses shared formatters and improves filter-change paging/load behavior. |
| frontend/src/routes/admin/AdminEvents.svelte | Adds document click handler to close export dropdown; uses shared pagination selector class. |
| frontend/src/routes/tests/Notifications.test.ts | Updates relative-time expectations to the new compact format. |
| frontend/src/routes/Settings.svelte | Uses shared formatter and date-fns parsing; replaces manual click-outside handler with <svelte:document>. |
| frontend/src/routes/Notifications.svelte | Replaces local timestamp formatter with shared formatRelativeTime. |
| frontend/src/routes/Login.svelte | Switches warning logging to shared logger. |
| frontend/src/routes/Editor.svelte | Adds logger; guards localStorage persistence for quota/private-mode errors. |
| frontend/src/main.ts | Routes global error handling through shared logger. |
| frontend/src/lib/notifications/stream.svelte.ts | Switches notification stream connection error logging to shared logger. |
| frontend/src/lib/logger.ts | Introduces browser logger based on consola/browser with build-time prod level replacement. |
| frontend/src/lib/formatters.ts | Refactors timestamp/duration/relative-time formatting using date-fns; removes bytes/number helpers. |
| frontend/src/lib/api-interceptors.ts | Switches API error logging to shared logger. |
| frontend/src/lib/admin/stores/sagasStore.svelte.ts | Replaces createAutoRefresh with store-local $effect interval + enable/rate state. |
| frontend/src/lib/admin/stores/executionsStore.svelte.ts | Replaces createAutoRefresh with store-local fixed interval refresh. |
| frontend/src/lib/admin/stores/eventsStore.svelte.ts | Replaces createAutoRefresh with store-local interval refresh; improves SSE parsing logging. |
| frontend/src/lib/admin/stores/tests/sagasStore.test.ts | Updates tests for the new refreshEnabled behavior and removes cleanup usage. |
| frontend/src/lib/admin/stores/tests/executionsStore.test.ts | Updates tests to rely on teardown/timer clearing instead of autoRefresh cleanup. |
| frontend/src/lib/admin/stores/tests/eventsStore.test.ts | Updates tests to rely on teardown/timer clearing instead of autoRefresh cleanup. |
| frontend/src/lib/admin/index.ts | Removes re-export of the deleted autoRefresh helper. |
| frontend/src/lib/admin/constants.ts | Removes unused badge/status/role color constants and types. |
| frontend/src/lib/admin/autoRefresh.svelte.ts | Removes the shared auto-refresh helper implementation. |
| frontend/src/lib/admin/tests/constants.test.ts | Removes tests for deleted constants. |
| frontend/src/lib/admin/tests/autoRefresh.test.ts | Removes tests for the deleted autoRefresh helper. |
| frontend/src/lib/tests/formatters.test.ts | Updates formatter tests to reflect new date-fns-based formatting API. |
| frontend/src/components/admin/index.ts | Stops exporting removed admin components (StatsCard/StatusBadge/ActionButtons). |
| frontend/src/components/admin/events/tests/EventsTable.test.ts | Updates formatting assertions to match date-fns formatting outputs. |
| frontend/src/components/admin/events/EventsTable.svelte | Uses shared date-only/time-only formatters instead of toLocale*. |
| frontend/src/components/admin/tests/StatusBadge.test.ts | Removes tests for deleted StatusBadge component. |
| frontend/src/components/admin/tests/StatsCard.test.ts | Removes tests for deleted StatsCard component. |
| frontend/src/components/admin/tests/ActionButtons.test.ts | Removes tests for deleted ActionButtons component. |
| frontend/src/components/admin/StatusBadge.svelte | Removes StatusBadge component. |
| frontend/src/components/admin/StatsCard.svelte | Removes StatsCard component. |
| frontend/src/components/admin/ActionButtons.svelte | Removes ActionButtons component. |
| frontend/src/components/tests/ProtectedRoute.test.ts | Removes tests for deleted ProtectedRoute component. |
| frontend/src/components/tests/NotificationCenter.test.ts | Updates expectations for new relative-time output. |
| frontend/src/components/tests/Header.test.ts | Removes mock method no longer present/used (fetchUserProfile). |
| frontend/src/components/ProtectedRoute.svelte | Removes ProtectedRoute component. |
| frontend/src/components/NotificationCenter.svelte | Uses shared formatRelativeTime, adds auto-read effect and logger; simplifies click handling. |
| frontend/src/components/Modal.svelte | Moves Escape handling to <svelte:window> keydown handler. |
| frontend/src/components/Header.svelte | Uses <svelte:window bind:innerWidth> + <svelte:document onclick> for responsive + click-outside behavior. |
| frontend/src/app.css | Adds reduced-motion media query and consolidates form control styles into a utility. |
| frontend/src/tests/test-utils.ts | Removes section divider comments; keeps utilities intact. |
| frontend/src/App.svelte | Switches app init logging to shared logger. |
| frontend/rollup.config.js | Injects build-time isProduction into logger; stops dropping console at minification. |
| frontend/package.json | Adds consola and date-fns dependencies. |
| frontend/package-lock.json | Locks new dependencies and updates consola dev/prod flags. |
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 191-193: The forEach callback in the test that iterates over
expectedContent should use a block body instead of an expression to avoid an
implicit return; update the call around expectedContent.forEach(text =>
expect(mobileMenu.textContent).toContain(text)) to use a block callback (e.g.,
expectedContent.forEach(text => {
expect(mobileMenu.textContent).toContain(text); })) so the callback does not
return a value and the Biome lint rule
(lint/suspicious/useIterableCallbackReturn) is satisfied.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/Header.sveltefrontend/src/components/__tests__/Header.test.tsfrontend/src/lib/admin/__tests__/pagination.test.tsfrontend/src/lib/admin/pagination.svelte.tsfrontend/src/stores/__tests__/errorStore.test.tsfrontend/src/stores/errorStore.svelte.ts
💤 Files with no reviewable changes (2)
- frontend/src/lib/admin/tests/pagination.test.ts
- frontend/src/lib/admin/pagination.svelte.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/stores/tests/errorStore.test.ts
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/Home.svelte (1)
3-3:⚠️ Potential issue | 🟡 MinorRemove unused
fadeandflyimports.Line 3 imports
fadeandflyfromsvelte/transition, but they are not used anywhere in the template. All animations use thehero-animate-fly-inCSS class instead. Remove these unused imports to keep the code clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Home.svelte` at line 3, Remove the unused imports "fade" and "fly" from the import statement that currently reads import { fade, fly } from "svelte/transition"; in Home.svelte — update the import to remove those symbols so only used exports remain (or remove the entire import if nothing else is imported), ensuring no unused variables named fade or fly remain in the module.
🧹 Nitpick comments (3)
frontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.ts (1)
1-8: Consider importingEndpointGrouptype at the top.The inline type assertion
as import('$lib/api').EndpointGroupon line 13 is valid but could be cleaner. Per coding guidelines, preferimport typeat the top of the file, which also enables type-checking for theEXPECTED_GROUPSarray.♻️ Proposed refactor
import { describe, it, expect } from 'vitest'; +import type { EndpointGroup } from '$lib/api'; import { getGroupColor, detectGroupFromEndpoint, createEmptyRule } from '$lib/admin/rate-limits/rateLimits'; -const EXPECTED_GROUPS = ['execution', 'admin', 'sse', 'websocket', 'auth', 'api', 'public']; +const EXPECTED_GROUPS: EndpointGroup[] = ['execution', 'admin', 'sse', 'websocket', 'auth', 'api', 'public'];Then on line 13:
- const color = getGroupColor(group as import('$lib/api').EndpointGroup); + const color = getGroupColor(group);As per coding guidelines: "use import type for type-only imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.ts` around lines 1 - 8, Add a top-level type-only import for EndpointGroup (e.g., import type { EndpointGroup } from '$lib/api') and use it to annotate EXPECTED_GROUPS instead of the inline assertion; update the EXPECTED_GROUPS declaration to be typed as EndpointGroup[] and remove the inline "as import('$lib/api').EndpointGroup" usage so the array is type-checked; ensure the import is a type import to avoid runtime overhead and reference EXPECTED_GROUPS, getGroupColor, detectGroupFromEndpoint, and createEmptyRule when locating the change.frontend/src/styles/pages.css (1)
296-303: Consider usingwidth < 1024pxfor cleaner breakpoint boundary.The
width <= 1023.98pxapproach works but relies on a decimal to avoid overlap. Usingwidth < 1024pxwould be cleaner and more explicit:♻️ Suggested improvement
- `@media` (width >= 640px) and (width <= 1023.98px) { + `@media` (width >= 640px) and (width < 1024px) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/styles/pages.css` around lines 296 - 303, Replace the media query condition that uses the decimal upper bound with a clearer strict less-than breakpoint: change the media rule using "(width >= 640px) and (width <= 1023.98px)" to use "(width >= 640px) and (width < 1024px)" so the selector .features-grid > :last-child:nth-child(2n + 1) keeps the same styles but the breakpoint boundary is explicit and non-overlapping.frontend/src/lib/formatters.ts (1)
102-121: Minor optimization opportunity informatRelativeTime.Multiple
differenceIn*calls recalculate differences from the samenowanddatevalues. This is functionally correct but slightly redundant since these functions traverse the same interval. Given the simplicity of the operations, this is acceptable, but if performance becomes a concern, you could compute only the necessary difference based on magnitude.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/formatters.ts` around lines 102 - 121, The formatRelativeTime function is calling differenceInSeconds, differenceInMinutes, differenceInHours, and differenceInDays redundantly; change it to compute a single diffSecs = differenceInSeconds(now, date) and derive diffMins = Math.floor(diffSecs/60), diffHrs = Math.floor(diffSecs/3600), diffDys = Math.floor(diffSecs/86400) (keep the existing early-return logic: check <60, <3600, <86400, <7*86400) so you avoid repeated interval calculations while preserving output; update the function name references (formatRelativeTime, diffSecs/diffMins/diffHrs/diffDys) accordingly.
🤖 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/Home.svelte`:
- Line 3: Remove the unused imports "fade" and "fly" from the import statement
that currently reads import { fade, fly } from "svelte/transition"; in
Home.svelte — update the import to remove those symbols so only used exports
remain (or remove the entire import if nothing else is imported), ensuring no
unused variables named fade or fly remain in the module.
---
Nitpick comments:
In `@frontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.ts`:
- Around line 1-8: Add a top-level type-only import for EndpointGroup (e.g.,
import type { EndpointGroup } from '$lib/api') and use it to annotate
EXPECTED_GROUPS instead of the inline assertion; update the EXPECTED_GROUPS
declaration to be typed as EndpointGroup[] and remove the inline "as
import('$lib/api').EndpointGroup" usage so the array is type-checked; ensure the
import is a type import to avoid runtime overhead and reference EXPECTED_GROUPS,
getGroupColor, detectGroupFromEndpoint, and createEmptyRule when locating the
change.
In `@frontend/src/lib/formatters.ts`:
- Around line 102-121: The formatRelativeTime function is calling
differenceInSeconds, differenceInMinutes, differenceInHours, and
differenceInDays redundantly; change it to compute a single diffSecs =
differenceInSeconds(now, date) and derive diffMins = Math.floor(diffSecs/60),
diffHrs = Math.floor(diffSecs/3600), diffDys = Math.floor(diffSecs/86400) (keep
the existing early-return logic: check <60, <3600, <86400, <7*86400) so you
avoid repeated interval calculations while preserving output; update the
function name references (formatRelativeTime, diffSecs/diffMins/diffHrs/diffDys)
accordingly.
In `@frontend/src/styles/pages.css`:
- Around line 296-303: Replace the media query condition that uses the decimal
upper bound with a clearer strict less-than breakpoint: change the media rule
using "(width >= 640px) and (width <= 1023.98px)" to use "(width >= 640px) and
(width < 1024px)" so the selector .features-grid > :last-child:nth-child(2n + 1)
keeps the same styles but the breakpoint boundary is explicit and
non-overlapping.
ℹ️ 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 (21)
.github/workflows/frontend-ci.yml.pre-commit-config.yamlfrontend/.stylelintrc.jsonfrontend/package.jsonfrontend/src/app.cssfrontend/src/components/Header.sveltefrontend/src/components/__tests__/Header.test.tsfrontend/src/components/admin/events/__tests__/EventsTable.test.tsfrontend/src/components/editor/OutputPanel.sveltefrontend/src/lib/__tests__/formatters.test.tsfrontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.tsfrontend/src/lib/admin/rate-limits/rateLimits.tsfrontend/src/lib/admin/stores/__tests__/eventsStore.test.tsfrontend/src/lib/formatters.tsfrontend/src/lib/notifications/stream.svelte.tsfrontend/src/routes/Home.sveltefrontend/src/stores/__tests__/auth.test.tsfrontend/src/stores/__tests__/errorStore.test.tsfrontend/src/stores/auth.svelte.tsfrontend/src/styles/components.cssfrontend/src/styles/pages.css
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/lib/tests/formatters.test.ts
- frontend/src/stores/tests/auth.test.ts
- frontend/src/lib/notifications/stream.svelte.ts
- frontend/src/stores/tests/errorStore.test.ts
- frontend/package.json
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/admin/rate-limits/__tests__/rateLimits.test.ts`:
- Around line 1-11: The test file is missing explicit lifecycle hooks required
by repo policy; add beforeEach(() => vi.clearAllMocks()) to clear mocks and
afterEach(() => cleanup()) to run DOM cleanup, placing the beforeEach at the top
of the describe('rateLimits', ...) block and afterEach inside the same block;
ensure imports for vi/cleanup are available in the test environment (vitest
provides vi and cleanup from testing-library if needed) and keep existing tests
that reference getGroupColor, detectGroupFromEndpoint, and createEmptyRule
unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/NotificationCenter.sveltefrontend/src/lib/admin/rate-limits/__tests__/rateLimits.test.tsfrontend/src/styles/pages.css
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/NotificationCenter.svelte
This was referenced Mar 4, 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
Adds a tagged consola logger and date-fns time formatters, simplifies admin auto‑refresh with plain intervals, cleans up dead code, and improves accessibility and menu behavior. Also auto‑marks notifications as read after a short delay and adds Stylelint.
New Features
Refactors
Written for commit 02937c1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Refactor
Removals