chore: frontend updates #44
Conversation
📝 WalkthroughWalkthroughEditor theme default changed to "auto". Event and execution types moved from plain strings to enum types. Language/runtime metadata introduced as typed LanguageInfo. SSE/Redis messages and mappers converted to typed Pydantic schemas. Frontend removed the localStorage settings-cache in favor of a stores-based userSettings flow and unified saveUserSettings; many new UI components, admin utilities, and path-alias migrations were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser UI
participant Auth as Auth Store
participant API as Backend API
participant Store as userSettings Store
participant Theme as Theme Handler
Note over Browser,API: User saves settings via unified flow
Browser->>Auth: subscribe isAuthenticated?
alt authenticated
Browser->>API: PUT /api/v1/user/settings (partial UserSettingsUpdate)
API-->>Browser: 200 OK (updated settings)
Browser->>Store: setUserSettings(updated)
Store-->>Theme: if theme present -> setTheme(theme)
else unauthenticated
Browser->>Store: setUserSettings(local or null)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
frontend/src/routes/Editor.svelte (1)
54-61: Remove redundant local editor settings definition.This local
editorSettingsobject duplicatesDEFAULT_EDITOR_SETTINGSfrom the store and is immediately overwritten by the store subscription on line 219. This creates maintenance overhead if defaults need to change.🔎 Recommended refactor
Remove this local definition entirely and rely on the store subscription:
- // Default editor settings - let editorSettings = { - theme: 'auto', // Default to following app theme - font_size: 14, - tab_size: 4, - use_tabs: false, - word_wrap: true, - show_line_numbers: true, - }; + // Editor settings loaded from store + let editorSettings = $state({ + theme: 'auto', + font_size: 14, + tab_size: 4, + use_tabs: false, + word_wrap: true, + show_line_numbers: true, + });Or better yet, import and use the DEFAULT_EDITOR_SETTINGS directly from the store if you need an initial value:
+ import { editorSettings as editorSettingsStore, DEFAULT_EDITOR_SETTINGS } from '../stores/userSettings'; - // Default editor settings - let editorSettings = { - theme: 'auto', // Default to following app theme - font_size: 14, - tab_size: 4, - use_tabs: false, - word_wrap: true, - show_line_numbers: true, - }; + let editorSettings = $state(DEFAULT_EDITOR_SETTINGS);Note: You'd need to export DEFAULT_EDITOR_SETTINGS from the store for the second approach.
🧹 Nitpick comments (5)
backend/app/domain/user/settings_models.py (1)
21-29: Editor theme default aligned with “auto” model-wideSetting
DomainEditorSettings.themedefault to"auto"matches the rest of the settings model and the frontend behavior; change looks good. If you ever introduce anEditorThemeenum, this field would be a good candidate to migrate for stronger typing, but that’s not urgent.frontend/src/stores/__tests__/theme.test.ts (1)
5-7: Mock update matches new user-settings APIRenaming the mock export to
saveUserSettingskeeps the test aligned with the updated../lib/user-settingssurface. You might later add expectations around whensaveUserSettingsis (not) called under different auth states, but the current tests remain valid.frontend/src/stores/theme.ts (1)
26-37: Theme store wiring to unified user settings looks solidUsing a
saveUserSettingsfunction slot and callingsaveUserSettings({ theme: value })only when the auth store says the user is logged in keeps theme persistence consistent with the new unified settings API, while still supporting local-only behavior when unauthenticated.Note that because
saveUserSettingsitself callssetThemeLocalwhen athemeis present, authenticated changes will now trigger a second internal set/localStorage write + DOM update for the same value. It’s harmless, but if you ever care about reducing duplicate work, you could special-case the call path from the theme store.Also applies to: 39-51, 81-87
frontend/src/routes/Settings.svelte (1)
14-15: Settings page correctly integrated with central userSettings storeUpdating both
loadSettingsandsaveSettingsto callsetUserSettings(settings)keeps the new store in sync with the latest backend state, which is exactly what you want for other consumers (editor, theme, etc.). The history mapping change todisplayField: item.fieldplusitem.reason?.includes('restore')also hardens the UI against missing reasons without changing semantics.If you ever revisit error handling, consider whether you want to also clear or flag the global
userSettingsstore whenloadSettingsfails, so other views don’t keep showing stale data, but that can be deferred.Also applies to: 95-103, 135-159, 196-201
backend/app/schemas_pydantic/user_settings.py (1)
21-30: Pydantic EditorSettings default matches the “auto” theme modelChanging
EditorSettings.themedefault to"auto"is consistent with the domain model and OpenAPI schema, so new users get the system-following behavior end-to-end. If you later formalize editor themes as an enum, this field would be a natural place to tighten the type, but it’s fine as-is.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/app/domain/user/settings_models.pybackend/app/schemas_pydantic/user_settings.pydocs/reference/openapi.jsonfrontend/src/lib/__tests__/settings-cache.test.tsfrontend/src/lib/__tests__/user-settings.test.tsfrontend/src/lib/api/types.gen.tsfrontend/src/lib/auth-init.tsfrontend/src/lib/settings-cache.tsfrontend/src/lib/user-settings.tsfrontend/src/routes/Editor.sveltefrontend/src/routes/Settings.sveltefrontend/src/stores/__tests__/theme.test.tsfrontend/src/stores/theme.tsfrontend/src/stores/userSettings.ts
💤 Files with no reviewable changes (3)
- frontend/src/lib/api/types.gen.ts
- frontend/src/lib/settings-cache.ts
- frontend/src/lib/tests/settings-cache.test.ts
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/src/stores/userSettings.ts (2)
backend/app/schemas_pydantic/user_settings.py (2)
EditorSettings(21-43)UserSettings(46-59)frontend/src/lib/api/types.gen.ts (2)
EditorSettings(463-488)UserSettings(2710-2748)
backend/app/domain/user/settings_models.py (1)
frontend/src/stores/theme.ts (1)
theme(39-51)
frontend/src/stores/theme.ts (2)
frontend/src/lib/user-settings.ts (1)
saveUserSettings(33-55)frontend/src/stores/userSettings.ts (1)
userSettings(13-13)
frontend/src/lib/auth-init.ts (1)
frontend/src/stores/userSettings.ts (1)
clearUserSettings(28-30)
frontend/src/lib/user-settings.ts (6)
frontend/src/stores/userSettings.ts (2)
setUserSettings(20-22)updateSettings(24-26)frontend/src/lib/api/types.gen.ts (2)
UserSettingsUpdate(2755-2777)UserSettings(2710-2748)frontend/src/lib/api/index.ts (3)
UserSettingsUpdate(4-4)updateUserSettingsApiV1UserSettingsPut(3-3)UserSettings(4-4)frontend/src/stores/auth.ts (1)
isAuthenticated(43-43)frontend/src/lib/api/sdk.gen.ts (1)
updateUserSettingsApiV1UserSettingsPut(556-563)frontend/src/stores/theme.ts (1)
setThemeLocal(81-87)
frontend/src/lib/__tests__/user-settings.test.ts (2)
frontend/src/lib/user-settings.ts (2)
loadUserSettings(12-31)saveUserSettings(33-55)frontend/src/stores/userSettings.ts (1)
editorSettings(15-18)
backend/app/schemas_pydantic/user_settings.py (1)
frontend/src/stores/theme.ts (1)
theme(39-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (10)
frontend/src/lib/auth-init.ts (1)
3-3: Clearing user settings on auth reset looks correctHooking
_clearAuthintoclearUserSettings()ensures stale settings aren’t left around after auth is invalidated, which aligns with the new centralized settings store. 👍Also applies to: 151-160
docs/reference/openapi.json (1)
4989-4995: OpenAPI editor theme default now consistent with implementationSetting
EditorSettings.theme.defaultto"auto"brings the generated API docs in line with the backend models and frontend behavior. No further changes needed here.frontend/src/stores/userSettings.ts (2)
1-18: LGTM! Well-structured store setup.The store architecture is clean:
- DEFAULT_EDITOR_SETTINGS aligns with backend defaults (theme: 'auto', etc.)
- The derived store correctly merges defaults with user settings, preserving the right precedence
- Type safety is maintained with UserSettings and EditorSettings types
20-22: LGTM! Simple and correct helper functions.Both functions provide a clean API for external modules to interact with the store.
Also applies to: 28-30
frontend/src/routes/Editor.svelte (2)
44-45: LGTM! Correct imports for the new store pattern.The imports align with the refactored user settings architecture.
217-223: LGTM! Proper store subscription and cleanup.The subscription correctly:
- Updates local state from the store
- Applies settings to the editor when available
- Unsubscribes on component destroy (line 284)
Also applies to: 284-284
frontend/src/lib/user-settings.ts (2)
1-31: LGTM! Clean integration with the new store.The function correctly:
- Fetches settings from the API
- Updates the centralized store via
setUserSettings- Applies theme locally when available
- Handles errors gracefully
33-34: LGTM! Appropriate function signature and auth guard.The refactored function:
- Accepts
UserSettingsUpdatefor partial updates- Returns a boolean to indicate success/failure
- Properly guards against unauthenticated access
frontend/src/lib/__tests__/user-settings.test.ts (2)
1-55: LGTM! Comprehensive mock setup.The test mocks correctly cover:
- API calls (getUserSettings, updateUserSettings)
- Store mutations (setUserSettings, updateSettings)
- Theme management (setThemeLocal)
- Authentication state
Reset logic in beforeEach ensures test isolation.
57-228: LGTM! Excellent test coverage.The test suite thoroughly covers:
- loadUserSettings: API fetching, store updates, theme application, error handling
- saveUserSettings: Authentication guards, various setting combinations, store updates, theme application logic, error scenarios
The tests properly verify both success and failure paths, ensuring robust behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
frontend/src/routes/Editor.svelte (1)
58-71: Consider logging fallback to default language.The function correctly maps languages to CodeMirror extensions. However, when falling back to Python for unrecognized languages, consider logging this to help with debugging.
🔎 Optional: Add logging for default case
function getLanguageExtension(lang: string) { switch (lang) { case 'python': return python(); case 'node': return javascript(); case 'go': return go(); case 'ruby': return StreamLanguage.define(ruby); case 'bash': return StreamLanguage.define(shell); - default: return python(); + default: + console.warn(`Unknown language "${lang}", defaulting to Python`); + return python(); } }docs/reference/openapi.json (5)
1149-1192: Health/readiness/shutdown response schemas look coherent; consider minor timestamp consistencyThe new
LivenessResponse,ReadinessResponse, andShutdownStatusResponsetypes are wired correctly into/api/v1/health/live,/api/v1/health/ready, andSSEHealthResponse.shutdownand therequiredlists match the defined properties. For consistency with the rest of the spec (where timestamps generally useformat: "date-time"), you may want to add that format toLivenessResponse.timestamp(and optionally add a timestamp toReadinessResponse) so clients can uniformly parse health timestamps.Also applies to: 6405-6429, 7104-7124, 7891-7947, 8415-8464
3107-3159: DeleteUserResponse wiring is correct; naming is slightly overloaded across delete responsesThe
DELETE /api/v1/admin/users/{user_id}endpoint now correctly returnsDeleteUserResponse, and the schema itself (message + deleted_counts map) is well-formed and marked as required. Note there are several similarly named delete schemas (DeleteResponse,DeleteEventResponse,DeleteNotificationResponse,EventDeleteResponse,DeleteUserResponse), which may be mildly confusing for client authors; not a blocker, but consider converging on a single naming convention the next time you touch these models.Also applies to: 4953-4974
3257-3348: Rate limit response models are consistent; could reuse existing enums for stronger typingThe new rate-limit-related schemas (
RateLimitRuleResponse,UserRateLimitConfigResponse,UserRateLimitsResponse,RateLimitUpdateResponse) are internally consistent and correctly referenced by the admin rate-limit endpoints. One improvement you might consider is havingRateLimitRuleResponse.groupandRateLimitRuleResponse.algorithmreuse the existingEndpointGroupandRateLimitAlgorithmenums instead of plainstring, to keep the response surface as strongly typed as the request/config side.Also applies to: 6993-7041, 7081-7103, 8785-8883
2618-2632: Admin events export/replay/admin detail endpoints look structurally soundThe
export/json, admin event detail, replay, and replay-status endpoints are all using valid, connected schemas (EventDetailResponse,EventReplayRequest/Response/StatusResponse, etc.) and the required fields align with their properties. Theschema: {}on the export endpoints is acceptable as “any JSON”, but be aware that generated clients won’t get strong typing for those responses; if you stabilize a concrete shape later, tightening this will help downstream tooling.Also applies to: 2635-2799
4034-4215: Saga endpoint descriptions expose internal implementation detailsThe enriched descriptions for the saga endpoints now include Python-style docstrings with
Args:and references to “FastAPI request object” and DI services. These are accurate but a bit implementation-centric for public API docs; consider trimming them down to user-facing behavior and moving framework/DI notes back into code-only docstrings.
📜 Review details
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 (12)
backend/app/domain/execution/__init__.pybackend/app/domain/execution/models.pybackend/app/runtime_registry.pybackend/app/schemas_pydantic/execution.pybackend/app/services/execution_service.pybackend/app/settings.pydocs/reference/openapi.jsonfrontend/package.jsonfrontend/src/lib/api/index.tsfrontend/src/lib/api/sdk.gen.tsfrontend/src/lib/api/types.gen.tsfrontend/src/routes/Editor.svelte
✅ Files skipped from review due to trivial changes (1)
- backend/app/services/execution_service.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/app/schemas_pydantic/execution.py (2)
frontend/src/lib/api/index.ts (1)
LanguageInfo(4-4)frontend/src/lib/api/types.gen.ts (1)
LanguageInfo(1301-1310)
backend/app/domain/execution/__init__.py (1)
backend/app/domain/execution/models.py (3)
LanguageInfoDomain(68-72)ResourceLimitsDomain(76-84)ResourceUsageDomain(43-64)
backend/app/settings.py (1)
backend/app/domain/execution/models.py (1)
LanguageInfoDomain(68-72)
backend/app/runtime_registry.py (2)
backend/tests/conftest.py (1)
app(130-138)backend/app/domain/execution/models.py (1)
LanguageInfoDomain(68-72)
🪛 GitHub Actions: Ruff Linting
backend/app/schemas_pydantic/execution.py
[error] 82-82: E501 Line too long (125 > 120) detected by ruff check . --config pyproject.toml. Line 82 exceeds the maximum line length.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (27)
backend/app/settings.py (1)
6-6: LGTM!The import and type annotation update for
SUPPORTED_RUNTIMEScorrectly aligns with theLanguageInfoDomaintype now used inruntime_registry.py. The change maintains type consistency across the codebase.Also applies to: 41-41
backend/app/domain/execution/__init__.py (1)
10-10: LGTM!The
LanguageInfoDomainis correctly exported from the package, following the established pattern for other domain models. The__all__list is properly updated.Also applies to: 18-18
backend/app/schemas_pydantic/execution.py (1)
111-126: LGTM!The
LanguageInfomodel and updatedResourceLimits.supported_runtimestype correctly mirror the domain layer changes. The schema cleanly separates API concerns from domain models while maintaining field consistency.backend/app/domain/execution/models.py (2)
67-73: LGTM!The
LanguageInfoDomaindataclass is well-structured, follows existing patterns in the file, and is correctly positioned beforeResourceLimitsDomainwhich references it.
84-84: LGTM!The
supported_runtimestype update todict[str, LanguageInfoDomain]is consistent with the new model and aligns with changes across the codebase.backend/app/runtime_registry.py (2)
3-4: LGTM!The import of
LanguageInfoDomainis correctly placed and follows the existing import structure.
183-186: LGTM!The
SUPPORTED_RUNTIMESconstruction cleanly mapsLANGUAGE_SPECStoLanguageInfoDomainobjects using a dict comprehension. This serves as the single source of truth for language runtime information consumed bysettings.py.frontend/package.json (1)
21-25: LGTM!The CodeMirror language dependencies are correctly added to support Go, JavaScript, and legacy modes (Ruby, Shell) in the editor. Version ranges are appropriate.
frontend/src/routes/Editor.svelte (11)
17-17: LGTM!The new language imports correctly support multi-language editing with CodeMirror, including Go, JavaScript, and legacy modes for Ruby and Shell.
Also applies to: 37-41
50-51: Settings store integration looks good.The shift from localStorage-based settings to a reactive
editorSettingsStoreimproves consistency and reactivity across the app.
240-248: LGTM!The settings store subscription correctly updates editor settings reactively when the store changes.
264-280: LGTM!The runtime loading logic properly handles errors, validates the current selection against available runtimes, and falls back gracefully to the first available option.
298-306: LGTM!The language subscription correctly updates CodeMirror's syntax highlighting when the selected language changes.
673-687: LGTM!The export function correctly determines file extension from runtime info with a sensible fallback, and ensures the exported filename has the proper extension.
689-737: Excellent UX improvement!The file upload now intelligently detects the language from the file extension and auto-selects the appropriate runtime. Error handling is thorough with informative messages about supported file types.
820-820: LGTM!The dynamic
acceptattribute correctly restricts file uploads to supported language extensions.
1046-1095: LGTM!The runtime selector properly disables when runtimes are unavailable, provides clear visual feedback, and prevents showing empty version menus.
1095-1095: LGTM!The Run Script button is correctly disabled both during execution and when runtimes are unavailable, preventing invalid execution attempts.
158-160: The concern is not valid.file_extis a required property in theLanguageInfotype definition (defined asfile_ext: stringintypes.gen.tsand marked as required in the OpenAPI schema), not optional. The type system guarantees thatfile_extwill always be a string, so the code at lines 158-160 cannot produce.undefined. The derived value safely accessesi.file_extwithout risk of undefined values.Likely an incorrect or invalid review comment.
docs/reference/openapi.json (1)
5007-5042: EditorSettings default and LanguageInfo-based runtimes are aligned with the rest of the specChanging
EditorSettings.theme’s default to"auto"matches theThemeenum and the default underUserSettings.theme, andResourceLimits.supported_runtimesnow mapping toLanguageInfo(versions + file_ext) gives the frontend a much cleaner contract for runtime metadata. No correctness issues here; this is a good tightening of the model.Also applies to: 6383-6404, 7715-7733
frontend/src/lib/api/types.gen.ts (7)
432-448: New response types are well-structured.The addition of
DeleteUserResponse,LanguageInfo,LivenessResponse, andReadinessResponseprovides stronger typing for API responses. These types follow consistent patterns with clear field documentation.Also applies to: 1296-1336, 1787-1805
1712-1750: Minor type asymmetry between request and response models.
RateLimitRuleResponse.groupis typed asstring(line 1725), whileRateLimitRule.groupuses theEndpointGroupenum. This is likely intentional for backend flexibility, but consumers should be aware that the response type is more permissive than the input type.Since this is an auto-generated file, any schema alignment would need to happen in the backend OpenAPI spec.
2261-2261: Improved type safety for shutdown status.The
SseHealthResponse.shutdownfield now uses the concreteShutdownStatusResponsetype instead of a generic object, providing better type safety and documentation for shutdown state properties likephase,initiated,complete, and connection counts.Also applies to: 2565-2607
2826-2881: User rate limit response types are comprehensive.
UserRateLimitConfigResponseandUserRateLimitsResponseprovide well-structured types for rate limit configuration and current usage statistics. The nestedcurrent_usagemap allows for flexible usage data representation.
4661-4774: Admin event endpoint types are well-defined.The new admin event endpoint types (
DeleteEventApiV1AdminEventsEventIdDelete*,GetEventDetailApiV1AdminEventsEventIdGet*,ReplayEventsApiV1AdminEventsReplayPost*,GetReplayStatusApiV1AdminEventsReplaySessionIdStatusGet*) properly use concrete response types and follow the established patterns in the file.
3815-3815: Endpoint response types consistently use new concrete types.Health endpoints now use
LivenessResponse/ReadinessResponse, user deletion usesDeleteUserResponse, and rate limit endpoints useUserRateLimitsResponse/RateLimitUpdateResponse. This improves type safety across the API client.Also applies to: 3831-3831, 4932-4932, 5082-5082, 5112-5112
2107-2109: Breaking type change insupported_runtimes- verify all consumers are updated.The type changed from
{ [key: string]: Array<string> }to{ [key: string]: LanguageInfo }. This is a significant structural change where each runtime key now maps to an object containingversionsandfile_extinstead of just an array of strings.Ensure all frontend code consuming
ResourceLimits.supported_runtimeshas been updated to handle the newLanguageInfostructure:#!/bin/bash # Search for usages of supported_runtimes in the frontend rg -n -C5 'supported_runtimes' --type=ts --type=svelte -g '!types.gen.ts'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/integration/test_user_settings_routes.py (1)
132-132: LGTM! Test correctly updated for new default theme value.The addition of "auto" to the valid editor theme values correctly aligns with the backend changes that switched the default from "one-dark" to "auto". This prevents test failures and properly validates the expanded set of allowed values.
Note: Line 373 already uses "auto" in test scenarios, providing additional coverage.
Optional: Consider adding explicit default value verification
While the current test validates that "auto" is an acceptable value, you could optionally add an explicit assertion to verify that new users receive "auto" as their default editor theme:
# After line 132, optionally add: # Verify new default value for editor theme # (Note: This assumes the backend has been updated to default to "auto")This would provide more explicit coverage of the default value behavior, though it's not essential since the test already validates the value is within the acceptable range.
backend/app/api/routes/execution.py (1)
61-61: Consider usingasdict()for consistency with line 340.Line 61 uses
vars()to convertdomain_exec.resource_usageto the API model, while line 340 usesasdict()for the same pattern. Althoughvars()works here sinceResourceUsageDomaincontains only primitive fields, usingasdict()throughout would be more consistent. Additionally,ResourceUsageDomaindefines its ownto_dict()method which could be leveraged instead.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/routes/execution.pybackend/tests/integration/test_user_settings_routes.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (1)
backend/app/api/routes/execution.py (1)
1-1: Correct use ofasdict()for handling nested dataclass structures.The change from
vars()toasdict()at line 340 is appropriate and necessary.ResourceLimitsDomaincontains a nestedLanguageInfoDomainin itssupported_runtimesfield. Usingasdict()ensures the nested dataclass is recursively converted to a dictionary, whichvars()would not do.Line 61 correctly uses
vars()forResourceUsageDomainsince it contains only primitive fields and no nested dataclasses.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/NotificationCenter.sveltefrontend/src/stores/notificationStore.ts
🧰 Additional context used
🪛 GitHub Actions: Frontend CI
frontend/src/stores/notificationStore.ts
[error] 1-1: Vitest: No "loading" export is defined on the "../../stores/notificationStore" mock. Did you forget to return it from vi.mock? Ensure the mock provides a 'loading' export or use importOriginal to extend the mock.
frontend/src/components/NotificationCenter.svelte
[error] 65-70: Vitest: No "loading" export is defined on the "../../stores/notificationStore" mock. Did you forget to return it from vi.mock? Ensure the mocked module provides a 'loading' export or adjust the mock implementation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (1)
frontend/src/components/NotificationCenter.svelte (1)
7-7: LGTM! Clean refactoring to centralized loading state.The import at line 7 and usage with
$loadingat line 248 are both correct. The Svelte 5 store subscription syntax properly unwraps the derived store value, and this aligns well with centralizing state management. The local loading state declaration has been properly removed with no lingering references.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/__tests__/NotificationCenter.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (1)
frontend/src/components/__tests__/NotificationCenter.test.ts (1)
89-89: LGTM! Mock export follows established pattern.The
loadinggetter properly exposes themockLoadingderived store through the mocked module, consistent with hownotificationsandunreadCountare exported.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/NotificationCenter.svelte (1)
209-219: Consider syncing permission state when it changes externally.The
notificationPermissionstate is only updated whenrequestNotificationPermission()is called. If the user changes the permission through browser settings or another browser tab, the UI could display a stale state (e.g., the "Enable desktop notifications" button might remain visible even after permission is granted elsewhere).Consider adding a visibility change or focus listener to refresh the permission state when the user returns to the page.
🔎 Example approach to keep permission state fresh
Add a visibility change listener in the
onMountblock:onMount(async () => { // ... existing subscription code ... // Sync permission state when page becomes visible const handleVisibilityChange = () => { if (!document.hidden && 'Notification' in window) { notificationPermission = Notification.permission; } }; document.addEventListener('visibilitychange', handleVisibilityChange); return () => { // ... existing cleanup ... document.removeEventListener('visibilitychange', handleVisibilityChange); }; });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/NotificationCenter.sveltefrontend/src/lib/__tests__/user-settings.test.tsfrontend/src/lib/user-settings.tsfrontend/src/routes/Settings.sveltefrontend/src/stores/__tests__/theme.test.tsfrontend/src/stores/theme.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/stores/theme.ts (2)
frontend/src/lib/user-settings.ts (1)
saveUserSettings(34-50)frontend/src/stores/userSettings.ts (1)
userSettings(13-13)
frontend/src/lib/__tests__/user-settings.test.ts (4)
frontend/src/stores/theme.ts (1)
theme(39-48)frontend/src/components/__tests__/Header.test.ts (1)
theme(38-38)frontend/src/lib/user-settings.ts (2)
loadUserSettings(13-32)saveUserSettings(34-50)frontend/src/stores/userSettings.ts (1)
editorSettings(15-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (12)
frontend/src/components/NotificationCenter.svelte (3)
253-263: LGTM! Good implementation of double opt-in pattern.The permission request UI is well-designed:
- Only shown when permission is needed (
defaultstate)- Clear call-to-action with icon and descriptive text
- Positioned appropriately in the dropdown header
- Implements explicit opt-in, which is a best practice for notification permissions
This aligns well with the PR objective of "double opt-in for notifications."
267-267: LGTM! Consistent use of centralized loading state.The template correctly uses
$loadingto reactively subscribe to the imported loading store, which is consistent with the move to centralized state management.
7-7: The import statement is correct.notificationStoreexportsloadingas a derived store that properly extracts theloadingboolean from the store's state.frontend/src/stores/__tests__/theme.test.ts (1)
6-6: LGTM: Mock updated to match unified API.The mock correctly reflects the migration from
saveThemeSettingtosaveUserSettings, maintaining the same Promise-based signature.frontend/src/stores/theme.ts (2)
26-26: LGTM: Signature updated for unified settings API.The migration from
saveThemeSetting(value: string)tosaveUserSettings(partial: { theme?: ThemeValue })correctly aligns with the new unified user settings flow.Also applies to: 34-34
72-74: LGTM: Theme toggle now persists to backend.The addition of
saveUserSettings({ theme: next })ensures theme changes via toggle are persisted when authenticated, with proper null and authentication guards.frontend/src/routes/Settings.svelte (3)
102-102: LGTM: Store updates correctly integrated.The
setUserSettings(settings)calls appropriately update the reactive store after loading and saving settings, ensuring UI consistency.Also applies to: 158-158
233-233: LGTM: Theme application unified viasetThemehelper.Replacing direct theme store manipulation with the
setThemehelper provides a cleaner, more maintainable API surface for theme changes.Also applies to: 330-330
197-201: No issues found. Thereasonfield is typed asstring | nullin the API response, making the optional chaining operator (?.) safe and appropriate. Whenreasonexists, it is guaranteed to be a string, so.includes('restore')will not throw an error.frontend/src/lib/user-settings.ts (1)
13-32: LGTM: Settings loading correctly implemented.The function properly loads settings from the API, updates the store via
setUserSettings, and applies theme when present. Error handling is appropriate.frontend/src/lib/__tests__/user-settings.test.ts (2)
1-48: LGTM: Test infrastructure correctly updated.The mock setup properly reflects the unified API surface with
mockUpdateUserSettingsreplacing separate endpoint mocks, and includes appropriate mocks forsetUserSettingsandsetThemestore functions.
54-228: LGTM: Comprehensive test coverage for unified API.The tests thoroughly validate:
- API integration for loading and saving settings
- Store updates via
setUserSettings- Conditional theme application via
setTheme- Authentication guards
- Error handling for both API and network failures
Particularly good: line 189-197 verifies theme is not applied when only editor settings are saved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/routes/Settings.svelte (2)
105-105: Consider restoring nullish coalescing for consistency.Lines 105 and 111 changed from
??to||, which alters the fallback behavior. While??only defaults onnull/undefined,||also defaults on empty strings and other falsy values. For consistency with the other defaults informData(lines 107-110, 114-120 use??), consider whether this change was intentional.🔎 Proposed change for consistency
- theme: settings.theme || 'auto', + theme: settings.theme ?? 'auto', notifications: { execution_completed: settings.notifications?.execution_completed ?? true, execution_failed: settings.notifications?.execution_failed ?? true, system_updates: settings.notifications?.system_updates ?? true, security_alerts: settings.notifications?.security_alerts ?? true, - channels: settings.notifications?.channels || ['in_app'] + channels: settings.notifications?.channels ?? ['in_app'] },Also applies to: 111-111
197-201: Remove unusedisRestorefield.The
isRestorefield is computed but never used in the template. Consider removing it to reduce unnecessary computation.🔎 Proposed change to remove unused field
history = (data?.history || []) .map(item => ({ ...item, - displayField: item.field, - isRestore: item.reason?.includes('restore') + displayField: item.field })) .sort((a, b) => new Date(b.timestamp) - new Date(a.timestamp));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/main.tsfrontend/src/routes/Settings.svelte
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/main.ts (1)
frontend/src/stores/errorStore.ts (1)
appError(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (5)
frontend/src/main.ts (1)
18-20: LGTM! Enhanced error handling for unhandled promise rejections.The changes improve robustness by:
- Logging rejections at the appropriate error level
- Normalizing non-Error rejection reasons into Error objects for consistent handling
- Leveraging centralized error management via the appError store
The type guard and String() conversion safely handle edge cases (null, undefined, primitives, objects).
frontend/src/routes/Settings.svelte (4)
10-10: LGTM!The new imports align with the unified settings architecture, replacing legacy localStorage-based caching with reactive stores.
Also applies to: 14-14
102-102: LGTM!Settings are correctly persisted to the store after successful load and save operations, ensuring the reactive store stays synchronized with backend state.
Also applies to: 158-158
232-234: LGTM!Applying the theme immediately after restore provides good visual feedback. The implementation correctly awaits
loadSettings()before applying the theme, avoiding race conditions.
326-332: Verify that immediate theme application without auto-save is the intended UX.When a user selects a theme from the dropdown,
setThemeapplies it immediately (line 330) but doesn't save the settings to the backend. If the user doesn't click "Save Settings," the theme change won't persist across sessions. This provides instant visual feedback but could confuse users who expect the change to be permanent.Consider whether:
- This preview behavior is intentional and acceptable
- Auto-save should be triggered on theme change
- A visual indicator should inform users that changes aren't saved yet
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/stores/auth.ts (1)
49-57: Inconsistent user settings cleanup across auth-clearing paths.The
clearAuthfunction in this file doesn't callclearUserSettings(), but_clearAuthinfrontend/src/lib/auth-init.ts(line 150) does. This means user settings are only cleared when auth is cleared through the auth-init path, not through direct calls to logout/clearAuth in this module.Additionally, the AI summary claims
clearUserSettings()is called in this function, but it's not present in the code.🔎 Verify user settings clearing behavior
#!/bin/bash # Search for all auth clearing paths to ensure consistent user settings cleanup echo "=== Searching for clearAuth function definitions ===" ast-grep --pattern 'function clearAuth() { $$$ }' echo -e "\n=== Searching for clearUserSettings usage ===" rg -n 'clearUserSettings\(\)' --type ts echo -e "\n=== Searching for logout function implementations ===" ast-grep --pattern 'export async function logout() { $$$ }'
🧹 Nitpick comments (1)
frontend/src/lib/api-interceptors.ts (1)
42-50: Consider adding user settings cleanup to auth failure path.The
clearAuthStatefunction is called when handling 401 errors (line 78), but unlike_clearAuthin auth-init.ts, it doesn't callclearUserSettings(). This creates inconsistent cleanup behavior across different auth-clearing code paths.🔎 Verify if user settings should be cleared on auth failure
#!/bin/bash # Check how clearAuthState is used and if it should clear user settings echo "=== Usage of clearAuthState ===" rg -n -C 3 'clearAuthState\(\)' --type ts echo -e "\n=== Compare with other auth clearing functions ===" rg -n -C 5 'function.*clearAuth' --type ts
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/lib/__tests__/auth-init.test.tsfrontend/src/lib/api-interceptors.tsfrontend/src/lib/auth-init.tsfrontend/src/stores/__tests__/auth.test.tsfrontend/src/stores/auth.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/stores/__tests__/auth.test.ts (1)
frontend/src/stores/auth.ts (1)
logout(100-109)
frontend/src/lib/__tests__/auth-init.test.ts (1)
frontend/src/lib/auth-init.ts (1)
AuthInitializer(16-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (10)
frontend/src/lib/auth-init.ts (3)
3-3: Good integration of user settings cleanup.The import and usage of
clearUserSettings()ensures user settings are properly cleared when auth is invalidated through this initialization path.
128-137: Simplified auth persistence without timestamp expiration.The removal of timestamp-based expiration logic from
_getPersistedAuthis reasonable since sessionStorage automatically expires when the browser session ends. However, verify that this doesn't create issues with stale auth data within a long-running session.Note: The
_isRecentAuthcheck (line 139) still validates timestamp for network error handling, which provides some staleness protection.
143-152: LGTM - Consistent auth and settings cleanup.The
_clearAuthfunction properly clears both authentication state and user settings, then removes the sessionStorage entry. This ensures a clean logout experience.frontend/src/lib/__tests__/auth-init.test.ts (3)
58-72: Thorough test migration to sessionStorage.The test setup correctly migrates from localStorage to sessionStorage mocks. The renaming from
localStorageDatatosessionStorageDataimproves clarity, and all mock implementations are properly updated.
156-198: Comprehensive coverage of sessionStorage persistence behavior.The tests properly verify auth state persistence and cleanup with sessionStorage, including setting stores from persisted data and clearing sessionStorage on verification failure.
336-344: Good error handling coverage.The test correctly validates graceful handling of malformed JSON in sessionStorage, ensuring the auth initialization doesn't crash on corrupted data.
frontend/src/stores/__tests__/auth.test.ts (3)
19-30: Consistent test setup for sessionStorage migration.The mock implementation correctly simulates sessionStorage behavior, providing good test isolation for the storage migration.
62-80: Test confirms sessionStorage restoration.The test properly validates that auth state is restored from sessionStorage on module initialization, ensuring the persistence mechanism works correctly within a session.
109-130: sessionStorage persistence and cleanup verified.The tests confirm that auth state is both persisted to sessionStorage on login and properly cleared on logout. This ensures the full lifecycle of session-scoped auth storage is working correctly.
Note on UX implications: The migration from localStorage to sessionStorage means users will need to re-authenticate when:
- Opening the application in a new tab
- Closing and reopening the browser
- The browser session expires
This is more secure but may impact user experience compared to the previous localStorage behavior.
Also applies to: 186-193
frontend/src/stores/auth.ts (1)
22-25: Clarify that sessionStorage improves session data hygiene but doesn't directly mitigate CSRF vulnerability.The migration from localStorage to sessionStorage is a reasonable session-scoped data practice—sessionStorage clears on tab close, reducing stale session data persistence. However, this change does not directly address CSRF vulnerability. Both sessionStorage and localStorage are equally exposed to XSS since they are readable by JavaScript.
The actual CSRF protection in this codebase relies on the backend's double-submit cookie validation pattern: the server validates that the
X-CSRF-Tokenheader matches thecsrf_tokencookie (using constant-time comparison inapp/core/security.py), combined withSameSite=stricton the access token cookie. The storage location of the client-side token copy is irrelevant to CSRF defense—only the server-side validation matters.If the goal was to address CSRF vulnerability, sessionStorage is an appropriate choice since it limits token lifetime to the active session, but document that CSRF protection itself depends on server-side validation, not storage mechanism.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/src/components/ToastContainer.svelte (1)
22-46: Consider using immutable updates instead of direct mutation.The pattern of mutating
toast.progressandtoast.timerStarteddirectly (lines 27, 35, 45) followed bytoasts.update(n => n)works but is a code smell. It relies on the store holding mutable references.🔎 Suggested immutable alternative
- toast.progress = progress; - toasts.update(n => n); + toasts.update(n => n.map(t => + t.id === toast.id ? { ...t, progress } : t + ));This ensures explicit store updates without relying on reference mutation. Not blocking, as the current approach functions correctly.
frontend/src/routes/Settings.svelte (1)
197-203: History mapping simplified, but sort comparison may have TypeScript issues.The simplified
displayFieldandisRestorederivation is cleaner. However, the sort comparisonnew Date(b.timestamp) - new Date(a.timestamp)works at runtime (Date subtraction coerces to numbers) but may trigger TypeScript strictness warnings in some configurations.🔎 Optional: Explicit numeric comparison for type safety
.sort((a, b) => new Date(b.timestamp) - new Date(a.timestamp)); + .sort((a, b) => new Date(b.timestamp).getTime() - new Date(a.timestamp).getTime());frontend/src/routes/Editor.svelte (1)
527-529: Empty catch block silently swallows errors.The empty catch block after attempting to fetch the result will silently fail. Consider logging the error for debugging purposes.
🔎 Add error logging in catch block
} catch { - // Keep the apiError + // Keep the original apiError - secondary fetch also failed + console.warn('Fallback result fetch also failed'); }frontend/src/components/NotificationCenter.svelte (1)
204-214: Consider tracking external permission changes.The permission state is initialized correctly and SSR-safe. However, if the user changes notification permission in browser settings (without using the in-app button), the UI won't reflect that change. Consider adding error handling and optionally listening for permission changes.
🔎 Optional improvements
Add error handling to
requestNotificationPermission:async function requestNotificationPermission(): Promise<void> { if (!('Notification' in window)) return; + try { const result = await Notification.requestPermission(); notificationPermission = result; + } catch (err) { + console.error('Failed to request notification permission:', err); + } }Optionally, sync permission state when the window regains focus:
onMount(() => { const handleVisibilityChange = () => { if (document.visibilityState === 'visible' && 'Notification' in window) { notificationPermission = Notification.permission; } }; document.addEventListener('visibilitychange', handleVisibilityChange); return () => document.removeEventListener('visibilitychange', handleVisibilityChange); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/src/components/ErrorDisplay.sveltefrontend/src/components/Footer.sveltefrontend/src/components/Header.sveltefrontend/src/components/NotificationCenter.sveltefrontend/src/components/ToastContainer.sveltefrontend/src/components/__tests__/Header.test.tsfrontend/src/components/__tests__/NotificationCenter.test.tsfrontend/src/routes/Editor.sveltefrontend/src/routes/Home.sveltefrontend/src/routes/Notifications.sveltefrontend/src/routes/Settings.sveltefrontend/src/routes/admin/AdminLayout.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/tests/NotificationCenter.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (27)
frontend/src/components/Footer.svelte (2)
2-2: LGTM! Clean migration to Lucide icons.Using
@lucide/svelteis an excellent choice for standardizing icons across the codebase. The library is well-maintained and provides consistent, accessible icon components.
54-64: Excellent implementation with proper accessibility and security.The icon component usage demonstrates best practices:
sr-onlyspans ensure screen reader accessibilityrel="noopener noreferrer"prevents security vulnerabilities with external links- Icon choice is intuitive (
Sendfor Telegram,Githubfor GitHub)- Consistent sizing and styling across both icons
frontend/src/components/ToastContainer.svelte (4)
1-5: LGTM! Clean migration to Lucide icons.The import of semantic icon components (
CircleCheck,CircleX,AlertTriangle,Info,X) from@lucide/svelteproperly replaces the inline SVG approach, improving maintainability and consistency with the PR objective of using an icons library.
55-59: Good cleanup implementation.The
onDestroyhook properly unsubscribes from the store and clears all active timers, preventing memory leaks.
61-107: Well-structured styling helpers.The helper functions provide clean separation of styling logic with consistent patterns across all toast types, proper dark mode support, and sensible defaults.
121-124: Correct usage of Lucide icon components.The conditional icon rendering properly maps each toast type to its semantic icon (
CircleCheckfor success,CircleXfor error,AlertTrianglefor warning,Infofor info), and the close button appropriately uses theXicon with consistent sizing.Also applies to: 136-136
frontend/src/routes/admin/AdminLayout.svelte (1)
9-9: LGTM! Clean icon component migration.The ShieldCheck import and usage follow the Lucide Svelte conventions correctly. The class prop for sizing and theming is appropriate.
Also applies to: 82-82
frontend/src/components/ErrorDisplay.svelte (1)
2-3: LGTM! Appropriate icon migration for error display.AlertTriangle is semantically appropriate for an error display component. The styling matches the error theme context.
Also applies to: 38-38
frontend/src/routes/Home.svelte (2)
4-12: LGTM! Well-structured icon component migration.The use of
Componenttype for the features array and<svelte:component>for dynamic rendering is the correct Svelte 5 pattern. The explicit typing{ icon: Component; title: string; content: string }[]improves type safety.
63-63: LGTM! Dynamic icon rendering.Using
<svelte:component this={feature.icon} class="h-6 w-6" />correctly renders the Lucide icon components with proper sizing.frontend/src/components/Header.svelte (3)
8-8: LGTM! Comprehensive icon imports.All required Lucide icons are properly imported for the various header states (theme toggle, user menu, mobile menu, auth actions).
98-104: LGTM! Theme toggle icons.The conditional rendering for Sun/Moon/MonitorCog based on theme state is correct and follows the expected behavior.
192-196: LGTM! Mobile menu toggle icons.The X/Menu icon swap based on
isMenuActivestate correctly indicates menu open/closed state.frontend/src/routes/Settings.svelte (3)
10-16: LGTM! Updated imports for settings and theme management.The imports correctly bring in
setThemefor immediate theme application andsetUserSettingsfor reactive store updates, along with the ChevronDown icon.
103-106: Good integration: Settings sync to reactive store.Calling
setUserSettings(settings)after loading ensures the reactive store stays synchronized with the fetched settings. The theme fallback to'auto'is appropriate.
233-235: LGTM! Theme restoration applies immediately.Calling
setTheme(settings.theme)after restore ensures the UI theme updates immediately without requiring a page refresh.frontend/src/routes/Editor.svelte (5)
59-71: LGTM! Language extension mapping for multi-language support.The
getLanguageExtensionfunction correctly maps runtime languages to CodeMirror language extensions. UsingStreamLanguage.define()for legacy modes (ruby, shell) is the correct approach for CodeMirror 6.
159-163: Good use of derived state for runtime availability.The derived
acceptedFileExtsandruntimesAvailablevalues reactively update based onsupportedRuntimes, providing clean UI state management.
283-290: LGTM! Dynamic syntax highlighting on language change.The subscription to
selectedLangthat reconfigures thelanguageCompartmentensures syntax highlighting updates when users switch languages.
672-708: Dynamic file extension detection for multi-language upload.The file upload handler correctly builds an extension-to-language map from
supportedRuntimesand auto-selects the appropriate language and version. Good UX improvement.
1091-1093: Run button correctly disabled when runtimes unavailable.Disabling execution when
!runtimesAvailableprevents users from attempting to run code when the backend configuration isn't loaded.frontend/src/routes/Notifications.svelte (3)
2-2: LGTM! Icon imports for notification UI.The Component type import and Lucide icons are correctly added for the notification rendering refactor.
Also applies to: 11-11
114-120: Good semantic icon selection based on notification tags.The
getNotificationIconfunction now returns appropriate icon components:
AlertCirclefor failures/errors/security alertsClockfor timeouts/warningsCircleCheckfor success statesInfoas a sensible defaultThe return type change to
Componentenables dynamic rendering with<svelte:component>.
166-168: LGTM! Dynamic icon rendering in notification list.The empty state uses a centered Bell icon appropriately, and the notification items use
<svelte:component this={getNotificationIcon(notification.tags)}>for dynamic icon selection.Also applies to: 192-192
frontend/src/components/NotificationCenter.svelte (3)
2-2: LGTM: Icon library migration.The imports correctly add the Component type for type safety and Lucide icon components to replace hardcoded SVGs. The
loadingstore import provides better state management integration.Also applies to: 7-7, 9-9
19-25: LGTM: Type-safe icon mapping.The refactored function correctly returns Lucide component types based on notification tags. The Set-based tag checking and semantic icon mappings (error→AlertCircle, warning→AlertTriangle, success→CircleCheck) are appropriate.
223-223: LGTM: Clean component-based icon rendering.The template changes correctly use Lucide components:
- Bell icon in the notification button (line 223)
- Conditional permission request button with good UX (lines 248-256)
- Store-derived loading state for consistency (line 260)
- Dynamic icon rendering via
<svelte:component>(line 305)Also applies to: 248-256, 260-260, 305-305
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
frontend/src/lib/api-interceptors.ts (1)
117-125: Verify fallthrough behavior for unhandled status codes.The changes correctly suppress toasts for auth endpoints. However, consider the fallthrough for status codes not explicitly handled (e.g., 400, 404, 405):
- If
!response && isAuthEndpoint: skips network error toast, falls through to line 122 where it's also silenced ✓- If
responseexists with unhandled status (e.g., 404) on non-auth endpoint: shows generic toast ✓- If
responseexists with unhandled status on auth endpoint: returns silently ✓The logic appears correct. For clarity, consider adding a brief comment noting which status codes fall through to the generic handler (4xx not in {401, 403, 422, 429}).
🔎 Optional: Add comment for clarity on fallthrough cases
return error; } + // Fallthrough: handles remaining 4xx errors (400, 404, etc.) not caught above // Don't toast for auth-related silent failures if (!isAuthEndpoint) { addToast(getErrorMessage(error, 'An error occurred'), 'error'); }frontend/src/lib/editor/languages.ts (1)
17-19: Consider logging unknown language requests.The silent fallback to Python when a language is not found could make debugging difficult if unsupported languages are requested.
🔎 Add a console warning for unsupported languages
export function getLanguageExtension(lang: string): LanguageSupport | Extension { + if (!languageExtensions[lang]) { + console.warn(`Language '${lang}' not supported, falling back to Python`); + } return languageExtensions[lang]?.() ?? python(); }frontend/src/components/editor/SavedScripts.svelte (1)
45-64: Consider extracting fallback language constants.The hardcoded fallback values
'python'and'3.11'appear multiple times (lines 49, 53). Extracting these as named constants would improve maintainability.🔎 Extract constants
<script lang="ts"> import { slide } from 'svelte/transition'; import { List, Trash2 } from '@lucide/svelte'; + + const DEFAULT_LANGUAGE = 'python'; + const DEFAULT_VERSION = '3.11'; interface SavedScript { // ... }Then use
DEFAULT_LANGUAGEandDEFAULT_VERSIONinstead of the string literals.frontend/src/lib/editor/execution.svelte.ts (1)
35-82: Consider adding timeout and cleanup safeguards for EventSource.The EventSource implementation lacks explicit timeout handling and may not properly clean up in all scenarios:
- No timeout for the EventSource connection - if events stop arriving, the promise hangs indefinitely
- If the component unmounts during execution, the EventSource may remain open
- Multiple fallback calls could race (e.g., if onerror fires while handling a failure event)
🔎 Recommended improvements
// Add timeout handling const EXECUTION_TIMEOUT = 300000; // 5 minutes const timeout = setTimeout(() => { eventSource.close(); reject(new Error('Execution timeout')); }, EXECUTION_TIMEOUT); // Ensure single fallback call let fallbackCalled = false; const fetchFallback = async () => { if (fallbackCalled) return; fallbackCalled = true; // ... existing fallback logic }; // Clear timeout on completion eventSource.onmessage = async (event) => { try { // ... existing logic if (eventType === 'result_stored') { clearTimeout(timeout); eventSource.close(); resolve(eventData.result); } } catch (err) { clearTimeout(timeout); // ... error handling } };Also consider exposing an
abort()method from the returned state to allow external cleanup.frontend/src/components/editor/ResourceLimits.svelte (1)
13-18: Consider a more semantically appropriate icon.
MessageSquaredoesn't convey "resource limits" well. Consider using an icon likeGauge,Activity, orSettings2from Lucide to better represent system resource constraints.frontend/src/components/editor/CodeMirrorEditor.svelte (1)
69-76: Batch multiple editor dispatches into a single transaction for better performance.Dispatching 5 separate transactions causes 5 DOM updates. CodeMirror supports batching effects in a single dispatch.
🔎 Proposed fix
function applySettings() { if (!view) return; - view.dispatch({ effects: themeCompartment.reconfigure(getThemeExtension()) }); - view.dispatch({ effects: fontSizeCompartment.reconfigure(EditorView.theme({ ".cm-content": { fontSize: `${settings.font_size ?? 14}px` } })) }); - view.dispatch({ effects: tabSizeCompartment.reconfigure(EditorState.tabSize.of(settings.tab_size ?? 4)) }); - view.dispatch({ effects: lineNumbersCompartment.reconfigure(settings.show_line_numbers ? lineNumbers() : []) }); - view.dispatch({ effects: lineWrappingCompartment.reconfigure(settings.word_wrap ? EditorView.lineWrapping : []) }); + view.dispatch({ + effects: [ + themeCompartment.reconfigure(getThemeExtension()), + fontSizeCompartment.reconfigure(EditorView.theme({ ".cm-content": { fontSize: `${settings.font_size ?? 14}px` } })), + tabSizeCompartment.reconfigure(EditorState.tabSize.of(settings.tab_size ?? 4)), + lineNumbersCompartment.reconfigure(settings.show_line_numbers ? lineNumbers() : []), + lineWrappingCompartment.reconfigure(settings.word_wrap ? EditorView.lineWrapping : []) + ] + }); }frontend/src/components/editor/OutputPanel.svelte (1)
144-147: CPU time calculation assumes 100Hz jiffies.The formula
cpu_time_jiffies * 10assumes the kernel reports jiffies at 100Hz (10ms per jiffy). This is common on Linux but not universal (some systems use 250Hz or 1000Hz). Consider documenting this assumption or making it configurable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
frontend/rollup.config.jsfrontend/src/components/editor/CodeMirrorEditor.sveltefrontend/src/components/editor/EditorToolbar.sveltefrontend/src/components/editor/LanguageSelect.sveltefrontend/src/components/editor/OutputPanel.sveltefrontend/src/components/editor/ResourceLimits.sveltefrontend/src/components/editor/SavedScripts.sveltefrontend/src/components/editor/ScriptActions.sveltefrontend/src/components/editor/index.tsfrontend/src/lib/api-interceptors.tsfrontend/src/lib/editor/execution.svelte.tsfrontend/src/lib/editor/index.tsfrontend/src/lib/editor/languages.tsfrontend/src/routes/Editor.sveltefrontend/src/styles/pages.css
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/lib/api-interceptors.ts (6)
frontend/src/lib/editor/execution.svelte.ts (1)
error(106-106)frontend/src/lib/auth-init.ts (1)
isAuthenticated(154-160)frontend/src/components/__tests__/Header.test.ts (1)
isAuthenticated(31-31)frontend/src/components/__tests__/NotificationCenter.test.ts (1)
isAuthenticated(81-81)frontend/src/stores/toastStore.ts (1)
addToast(16-25)frontend/src/lib/__mocks__/api-interceptors.ts (1)
getErrorMessage(7-7)
frontend/src/lib/editor/execution.svelte.ts (1)
frontend/src/lib/api-interceptors.ts (1)
getErrorMessage(18-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (25)
frontend/rollup.config.js (2)
187-187: LGTM! TypeScript extension support added.Adding
.tsto the resolver extensions properly aligns with the project's TypeScript configuration and the.tsentry point. This ensures TypeScript module imports are resolved correctly.
147-148: Implementation is correct. Both@codemirror/lang-javascriptand@codemirror/lang-goare properly declared inpackage.jsonand actively imported infrontend/src/lib/editor/languages.ts. The manual chunk declarations in rollup.config.js are justified and appropriate.frontend/src/lib/api-interceptors.ts (2)
49-49: Consistent with session-scoped auth handling.The change from
localStoragetosessionStoragealigns with the broader auth state persistence shift mentioned in the context (auth-init.ts, auth.ts). Session-scoped storage is appropriate for auth state as it clears when the browser session ends.
73-91: Improved 401 handling with proper session-awareness.The logic correctly differentiates between:
- Auth endpoints (return early, let them handle their own messaging)
- Previously authenticated users (show toast + redirect)
- Unauthenticated users or concurrent 401s (silent clear)
The
isHandling401flag with 1-second cooldown infinallyprevents toast spam during rapid 401 responses.One minor observation: if
wasAuthenticatedisfalse, callingclearAuthState()on line 89 is harmless but redundant since there's nothing to clear. Consider guarding it, though the current approach is safer for edge cases.frontend/src/styles/pages.css (1)
23-23: LGTM! Consistent bottom padding improvement.The addition of 1rem bottom padding across all breakpoints provides better spacing for the editor layout and aligns well with the editor refactoring effort.
Also applies to: 28-28, 87-87, 113-113, 119-119
frontend/src/lib/editor/languages.ts (1)
1-15: LGTM! Clean language extension mapping.The lazy evaluation pattern using functions in the map is efficient and well-structured for dynamic language support.
frontend/src/components/editor/EditorToolbar.svelte (2)
1-11: LGTM! Well-typed component props.The Props interface and $props() usage follow Svelte 5 best practices correctly.
13-26: LGTM! Good accessibility and responsive design.The toolbar includes proper accessibility labels and responsive text visibility. The use of
currentTarget.valuein the input handler is correct.frontend/src/components/editor/LanguageSelect.svelte (2)
1-19: LGTM! Well-structured component state.The reactive state management and derived availability check are implemented correctly following Svelte 5 patterns.
21-31: LGTM! Proper disabled state handling.The button correctly handles the unavailable state with appropriate styling and cursor changes.
frontend/src/components/editor/SavedScripts.svelte (2)
1-27: LGTM! Clean component structure and refresh pattern.The toggle function correctly refreshes the script list when expanding the panel, ensuring up-to-date data.
29-39: LGTM! Excellent accessibility features.The toggle button includes proper ARIA attributes and dynamic labels for screen readers.
frontend/src/lib/editor/execution.svelte.ts (3)
1-19: LGTM! Clean state management pattern.The factory function and reset logic follow Svelte 5 patterns correctly with proper state encapsulation.
21-33: LGTM! Proper initialization and error handling.The execution start correctly resets state and handles API errors appropriately.
84-113: LGTM! Robust error recovery pattern.The error handling attempts to recover results even after failures and properly resets state in the finally block. The getter-based API maintains reactivity correctly.
frontend/src/components/editor/ScriptActions.svelte (1)
1-37: LGTM! Clean action buttons with proper authentication check.The component is well-structured with appropriate conditional rendering for the Save button based on authentication state. All buttons include accessibility-friendly titles.
frontend/src/lib/editor/index.ts (1)
1-2: LGTM! Standard barrel export pattern.The index file properly exposes the editor utilities for external consumption.
frontend/src/components/editor/ResourceLimits.svelte (1)
1-9: LGTM - Clean component setup with Svelte 5 runes.The component correctly uses
$props()and$state()for reactive state management. The imports are appropriately scoped.frontend/src/components/editor/CodeMirrorEditor.svelte (2)
114-124: LGTM - Public API methods are well-designed.The
setContentandgetViewmethods provide clean escape hatches for parent components to interact with the editor imperatively when needed.
109-112: Thevoid settingspattern correctly tracks nested property changes in this context.The
editorSettingsstore creates a new object instance on every change via object spread syntax ({...DEFAULT_EDITOR_SETTINGS, ...$userSettings?.editor}), not mutating the existing object. When any nested property changes, the entire object reference changes, which Svelte 5's effect system will detect. Thevoid settingsdependency will correctly triggerapplySettings()re-runs when any property is modified. Destructuring is unnecessary.Likely an incorrect or invalid review comment.
frontend/src/components/editor/OutputPanel.svelte (2)
16-29: LGTM - ANSI conversion with proper sanitization.Good security practice using DOMPurify with a restricted allowlist of tags and attributes before rendering ANSI-converted HTML.
58-170: Well-structured output panel with comprehensive state handling.The component cleanly handles idle, executing, error, and result states with appropriate visual feedback. The copy-to-clipboard functionality with toast notifications enhances UX.
frontend/src/components/editor/index.ts (1)
1-7: LGTM - Clean barrel exports for editor components.This consolidates editor component imports nicely, enabling consumers to use
import { CodeMirrorEditor, OutputPanel, ... } from '../components/editor'.frontend/src/routes/Editor.svelte (2)
101-143: Well-structured initialization with proper cleanup.The
onMountcorrectly sets up subscriptions and stores unsubscribe functions, andonDestroyproperly cleans them up. The auth state handling and runtime configuration loading are logically organized.
233-263: Robust file upload handling with language detection.The file upload logic correctly builds a reverse mapping from file extensions to languages, validates against supported runtimes, and provides clear error feedback for unsupported file types.
- keyboard navi support in editor - smaller fixes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/routes/Editor.svelte (1)
189-199: Add error feedback for non-404 update failures (past issue still present).The error handling still silently fails when
updateSavedScriptApiV1ScriptsScriptIdPutreturns a non-404 error. Users receive no feedback for failures like network errors, 500 errors, or authentication issues.🔎 Proposed fix
if (id) { const { error, response } = await updateSavedScriptApiV1ScriptsScriptIdPut({ path: { script_id: id }, body }); if (error) { if (response?.status === 404) { currentScriptId.set(null); const data = unwrap(await createSavedScriptApiV1ScriptsPost({ body })); currentScriptId.set(data.id); addToast('Script saved successfully.', 'success'); + } else { + addToast('Failed to update script. Please try again.', 'error'); } return; } addToast('Script updated successfully.', 'success');
🧹 Nitpick comments (3)
frontend/src/components/__tests__/NotificationCenter.test.ts (1)
526-580: Desktop notification permission tests are comprehensive.The new test suite thoroughly covers the notification permission flow:
- Enable button visibility based on permission state
- Permission request invocation
- Browser notification display on SSE message
The tests are well-structured and cover the critical paths.
Optional: Consider refactoring the browser notification test for clarity
The test at lines 558-579 redefines the global
Notificationin the middle of execution. For improved clarity and maintainability, consider extracting this test into a separatedescribeblock with its ownbeforeEachto set up the Notification mock:+ describe('browser notifications on SSE', () => { + let mockNotificationConstructor: ReturnType<typeof vi.fn>; + + beforeEach(() => { + mockNotificationConstructor = vi.fn(); + (globalThis as unknown as { Notification: unknown }).Notification = class { + constructor(title: string, options?: NotificationOptions) { + mockNotificationConstructor(title, options); + } + static get permission() { return 'granted'; } + static requestPermission = mockRequestPermission; + }; + }); + + it('shows browser notification when SSE notification received and permission granted', async () => { + mockNotificationPermission = 'granted'; + const instance = await setupSSE(); + instance.simulateMessage({ notification_id: 'n1', subject: 'Test Title', body: 'Test Body' }); + await waitFor(() => { + expect(mockNotificationConstructor).toHaveBeenCalledWith('Test Title', expect.objectContaining({ body: 'Test Body' })); + }); + }); + });This isolates the global mock setup and makes the test's intent clearer.
frontend/src/components/editor/OutputPanel.svelte (2)
50-56: Acknowledge partial fix from past review; consider explicit 'idle' case for completeness.The 'starting' phase is now handled explicitly (line 51), which addresses part of the previous concern. However, the 'idle' case still falls through to "Executing...". While this fallback is never reached in practice because line 64 filters out
phase !== 'idle', adding an explicit case would improve code clarity and maintainability:const phaseLabel = $derived.by(() => { + if (phase === 'idle') return 'Ready'; if (phase === 'starting') return 'Starting...'; if (phase === 'queued') return 'Queued...'; if (phase === 'scheduled') return 'Scheduled...'; if (phase === 'running') return 'Running...'; return 'Executing...'; });
98-115: Optional: Remove redundant fallback operators.Lines 102 and 122 use
result.stdout || ''andresult.stderr || ''respectively, but these are already inside{#if result.stdout}and{#if result.stderr}blocks. The fallback|| ''is redundant since the condition guarantees the value is truthy:🔎 Proposed simplification
{#if result.stdout} <div class="output-section"> <h4 class="text-xs font-medium text-fg-muted dark:text-dark-fg-muted mb-1 uppercase tracking-wider">Output:</h4> <div class="relative"> - <pre class="output-pre custom-scrollbar">{@html sanitize(ansiConverter.toHtml(result.stdout || ''))}</pre> + <pre class="output-pre custom-scrollbar">{@html sanitize(ansiConverter.toHtml(result.stdout))}</pre>Apply the same pattern to line 122 for
result.stderr.Also applies to: 117-136
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/__tests__/NotificationCenter.test.tsfrontend/src/components/editor/LanguageSelect.sveltefrontend/src/components/editor/OutputPanel.sveltefrontend/src/routes/Editor.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (11)
frontend/src/components/__tests__/NotificationCenter.test.ts (4)
51-51: Loading state mock infrastructure is now in place.The
mockLoadingderived store and its export are correctly implemented. However, the past review comment remains relevant: despite this infrastructure now being available, there are still no test cases that verify the loading UI behavior (e.g., spinner visibility when loading is true/false).Based on the past review comment, consider adding a test case that:
- Sets
mocks.mockNotificationsStatewithloading: true- Renders the component
- Asserts the spinner element is visible
- Sets
loading: false- Asserts the spinner is removed
Also applies to: 89-89
127-134: LGTM: Clean Notification mock implementation.The configurable Notification mock with permission state tracking is well-structured. Resetting the permission state and mock function in
beforeEachensures test isolation.Also applies to: 272-273
210-219: LGTM: Icon test data updated for lucide icons.The test cases correctly map notification tags to their corresponding lucide icon classes. The coverage is comprehensive and aligns with the component's migration to lucide icons.
362-370: LGTM: Icon tests correctly verify lucide icon classes.The test implementation properly verifies that the SVG element has the expected lucide icon class in its classList. The approach is suitable for testing class-based icon rendering.
frontend/src/components/editor/OutputPanel.svelte (1)
138-164: LGTM! Resource usage calculations are correct.The resource usage display correctly handles:
- CPU time: Converts jiffies to milliseconds (× 10) with proper handling of zero values
- Memory: Converts KB to MiB (÷ 1024)
- Wall time: Displays seconds with appropriate precision
frontend/src/components/editor/LanguageSelect.svelte (2)
35-85: Excellent keyboard navigation implementation! Past accessibility concern resolved.The keyboard navigation implementation comprehensively addresses the accessibility barrier flagged in the previous review:
- ✅ Arrow key navigation between languages and versions
- ✅ Enter/Space to open submenus and select items
- ✅ Escape to close menus (submenu first, then main menu)
- ✅ Focus management with roving tabindex pattern
- ✅ Visual focus indicators via conditional styling
The state management with
focusedLangIndexandfocusedVersionIndexcleanly tracks focus position, and the handlers properly distinguish between main menu and submenu contexts.
88-158: LGTM! Proper ARIA implementation with correct role hierarchy.The template correctly implements accessible menu patterns:
- Proper ARIA roles (
menu,menuitem,menuitemradio)- Appropriate ARIA properties (
aria-haspopup,aria-expanded,aria-checked)- Roving tabindex pattern for keyboard navigation (lines 117, 137)
- Focus and selection states properly reflected in styling
- Mouse and keyboard interactions work harmoniously together
frontend/src/routes/Editor.svelte (4)
41-69: LGTM! Persistent store implementation resolves past memory leak concern.The new implementation correctly addresses the previous memory leak issue by wrapping
setandupdatemethods to write to localStorage, rather than creating a subscription. This pattern:
- Eliminates the need for cleanup (no subscription to unsubscribe)
- Is more efficient (only writes on mutations, not on every reactive update)
- Properly handles SSR (returns plain writable when localStorage unavailable)
- Includes error handling for malformed JSON
314-323: LGTM! Reactive store access now correct (past issue resolved).Line 314 now correctly uses
$script.trim()instead ofget(script), ensuring the condition re-evaluates reactively when the script content changes. The empty state UI with the example loading prompt is a nice touch.
244-274: LGTM! Comprehensive file upload with runtime-aware validation.The file upload implementation correctly:
- Dynamically builds extension-to-language mapping from
supportedRuntimes- Validates uploaded file extensions against supported runtimes
- Auto-detects and sets language/version based on file extension
- Provides clear error feedback listing accepted extensions
- Clears input value to allow re-uploading the same file
- Updates editor content via both store and ref
97-101: LGTM! Proper subscription cleanup prevents memory leaks.The subscription management correctly:
- Declares unsubscribe handlers at component scope (lines 97-101)
- Stores unsubscribe functions from all subscriptions in
onMount- Cleans up all subscriptions in
onDestroywith optional chaining for safety- Handles edge cases where cleanup might be called before initialization completes
Also applies to: 156-161
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/lib/api-interceptors.ts (4)
25-30: Type assertion could lead to incorrect typing.The function assumes
detailis alwaysstring | ValidationError[], but the actual property could be any type. If the API returns an unexpecteddetailformat (e.g., a number or nested object), the return type would be misleading.🔎 Suggested improvement for safer type handling
function extractDetail(err: unknown): string | ValidationError[] | null { if (typeof err === 'object' && err !== null && 'detail' in err) { - return (err as { detail: string | ValidationError[] }).detail; + const detail = (err as { detail: unknown }).detail; + if (typeof detail === 'string') return detail; + if (Array.isArray(detail)) return detail as ValidationError[]; } return null; }
50-52: Duplicate logic and same potential runtime error.This function duplicates the mapping logic from
getErrorMessage(line 38), and shares the same vulnerability to empty/missinglocarrays. Consider consolidating to a single helper.🔎 Suggested consolidation
+function formatSingleValidationError(e: ValidationError): string { + const field = e.loc?.length ? e.loc[e.loc.length - 1] : 'field'; + return `${field}: ${e.msg}`; +} + function formatValidationErrors(detail: ValidationError[]): string { - return detail.map((e) => `${e.loc[e.loc.length - 1]}: ${e.msg}`).join('\n'); + return detail.map(formatSingleValidationError).join('\n'); }Then reuse
formatSingleValidationErroringetErrorMessageas well.
100-106: Consider validating array elements before treating as ValidationError[].When checking
Array.isArray(detail), the code assumes all elements conform toValidationError. If the API returns a malformed array,formatValidationErrorscould fail.🔎 Suggested defensive check
if (status === 422) { const detail = extractDetail(error); - if (Array.isArray(detail) && detail.length > 0) { + if (Array.isArray(detail) && detail.length > 0 && detail.every(e => e?.loc && e?.msg)) { addToast(`Validation error:\n${formatValidationErrors(detail)}`, 'error'); return true; } }
148-155: Type assertion masks potential undefined return.If
result.dataisundefined(no error but also no data),unwrapreturnsundefinedcast asT, which could cause downstream type errors. Consider throwing or providing a clearer contract.🔎 Suggested improvement
export function unwrap<T>(result: { data?: T; error?: unknown }): T { if (result.error) throw result.error; + if (result.data === undefined) throw new Error('No data returned'); return result.data as T; }Alternatively, change the return type to
T | undefinedif undefined is a valid outcome.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/lib/api-interceptors.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (1)
frontend/src/lib/api-interceptors.ts (1)
64-81: LGTM!The 401 handling logic properly debounces repeated failures, preserves the redirect path for post-login navigation, and gracefully handles edge cases. The
isHandling401guard prevents cascading toasts during concurrent failures.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (21)
frontend/src/lib/admin/__tests__/constants.test.ts (2)
32-34: Expand dark mode variant testing to all colors.The dark mode variant test only validates the
greencolor. For comprehensive coverage, verify that all color entries inSTATS_BG_COLORSinclude dark mode variants.🔎 Proposed test expansion
it('includes dark mode variants', () => { expect(STATS_BG_COLORS.green).toContain('dark:bg-green'); + expect(STATS_BG_COLORS.red).toContain('dark:bg-red'); + expect(STATS_BG_COLORS.yellow).toContain('dark:bg-yellow'); + expect(STATS_BG_COLORS.blue).toContain('dark:bg-blue'); + expect(STATS_BG_COLORS.purple).toContain('dark:bg-purple'); + expect(STATS_BG_COLORS.orange).toContain('dark:bg-orange'); + expect(STATS_BG_COLORS.neutral).toContain('dark:bg-neutral'); });
45-47: Expand dark mode variant testing to all text colors.Similar to the background colors, the dark mode variant test only validates the
greentext color. Consider testing all color entries for consistency.🔎 Proposed test expansion
it('includes dark mode variants', () => { expect(STATS_TEXT_COLORS.green).toContain('dark:text-green'); + expect(STATS_TEXT_COLORS.red).toContain('dark:text-red'); + expect(STATS_TEXT_COLORS.yellow).toContain('dark:text-yellow'); + expect(STATS_TEXT_COLORS.blue).toContain('dark:text-blue'); + expect(STATS_TEXT_COLORS.purple).toContain('dark:text-purple'); + expect(STATS_TEXT_COLORS.orange).toContain('dark:text-orange'); + expect(STATS_TEXT_COLORS.neutral).toContain('dark:text-neutral'); });frontend/src/components/admin/StatusBadge.svelte (1)
16-20: Tighten the type for sizeClasses.The
sizeClassesmapping usesRecord<string, string>, butsizeis constrained to'sm' | 'md' | 'lg'. Using a more precise type improves type safety and prevents runtime errors.🔎 Proposed type refinement
- const sizeClasses: Record<string, string> = { + const sizeClasses: Record<'sm' | 'md' | 'lg', string> = { sm: 'text-xs px-1.5 py-0.5', md: 'text-sm px-2 py-1', lg: 'text-base px-3 py-1.5' };frontend/src/components/admin/events/ReplayProgressBanner.svelte (1)
1-112: Well-structured progress banner with comprehensive error handling.The component effectively displays replay progress with detailed sections for failures and execution results. The conditional rendering properly guards against null/undefined values, and the visual hierarchy is clear.
Consider adding ARIA attributes to the progress bar (lines 35-40) for improved accessibility, such as
role="progressbar",aria-valuenow,aria-valuemin, andaria-valuemax.frontend/src/components/admin/sagas/SagaStatsCards.svelte (1)
11-13: Consider memoizing counts for better performance.The
getCountfunction is invoked for each state during render, causing repeated array filtering. For small datasets this is fine, but if saga counts grow, consider pre-computing counts with a derived state.🔎 Optional optimization using $derived
+ const countsByState = $derived( + sagas.reduce((acc, s) => { + acc[s.state] = (acc[s.state] || 0) + 1; + return acc; + }, {} as Record<string, number>) + ); + function getCount(state: string): number { - return sagas.filter(s => s.state === state).length; + return countsByState[state] || 0; }frontend/src/components/admin/sagas/SagaFilters.svelte (1)
25-32: Consider debouncing the search inputs to reduce API call frequency.The
oninput={onSearch}on both search inputs triggers the search callback on every keystroke, which may cause excessive API calls and degrade performance. Consider implementing a debounce mechanism (e.g., 300-500ms delay) to wait until the user has finished typing before triggering the search.💡 Example debounce implementation
You could create a debounced search handler in the script section:
+import { debounce } from '../../../lib/utils'; + interface Props { searchQuery: string; stateFilter: string; executionIdFilter: string; onSearch: () => void; onClear: () => void; } let { searchQuery = $bindable(''), stateFilter = $bindable(''), executionIdFilter = $bindable(''), onSearch, onClear }: Props = $props(); + +const debouncedSearch = debounce(onSearch, 300);Then use
oninput={debouncedSearch}instead ofoninput={onSearch}for the text inputs.Also applies to: 45-52
frontend/src/components/admin/users/DeleteUserModal.svelte (1)
26-30: Potential redundancy with $bindable directive.The
handleCascadeChangefunction manually updatescascadeDelete(line 28) and then callsonCascadeChange. However, sincecascadeDeleteuses the$bindabledirective, the binding should automatically sync the prop when the checkbox changes. The manual assignment might be redundant or could potentially interfere with the binding mechanism.Consider simplifying to just call the optional callback if you need to notify the parent, or rely solely on the reactive binding without the manual handler.
💡 Simplified approach
If you only need to notify the parent of changes, you could use a reactive statement:
- function handleCascadeChange(e: Event): void { - const target = e.target as HTMLInputElement; - cascadeDelete = target.checked; - onCascadeChange?.(cascadeDelete); - } + $effect(() => { + onCascadeChange?.(cascadeDelete); + });And update the checkbox:
<input type="checkbox" - checked={cascadeDelete} - onchange={handleCascadeChange} + bind:checked={cascadeDelete} class="rounded border-border-default dark:border-dark-border-default" />Alternatively, if the parent doesn't need notifications and just reads the bound value, you can remove
onCascadeChangeentirely and usebind:checked={cascadeDelete}.frontend/src/components/admin/users/UsersTable.svelte (1)
94-114: Consider addingaria-labelto icon-only action buttons for screen reader accessibility.The desktop action buttons rely only on
titlefor context. Screen readers may not announce title attributes, so addingaria-labelwould improve accessibility.🔎 Suggested improvement
<button onclick={() => onEdit(user)} class="text-green-600 hover:text-green-800 dark:text-green-400" title="Edit User" + aria-label="Edit User" > <Pencil class="w-5 h-5" /> </button> <button onclick={() => onRateLimits(user)} class="text-blue-600 hover:text-blue-800 dark:text-blue-400" title="Manage Rate Limits" + aria-label="Manage Rate Limits" > <Clock class="w-5 h-5" /> </button> <button onclick={() => onDelete(user)} class="text-red-600 hover:text-red-800 dark:text-red-400" title="Delete User" + aria-label="Delete User" > <Trash2 class="w-5 h-5" /> </button>frontend/src/lib/admin/autoRefresh.svelte.ts (2)
88-93: Simplify the$effectcondition for clarity.The condition
if (enabled || rate)is confusing:rateis always truthy (default 5), so this effectively always runs. Sincestart()already guards withif (enabled), you can simplify by just referencing both reactive variables without the conditional.🔎 Suggested improvement
$effect(() => { - if (enabled || rate) { - start(); - } + // Track both enabled and rate for reactivity + void enabled; + void rate; + start(); return () => stop(); });
60-65: Consider documenting async callback behavior.If
onRefreshis async and exceeds the interval duration, calls may overlap. The consumer should manage this (e.g., with a loading flag that skips refresh if already in progress). Consider documenting this behavior in the JSDoc.frontend/src/components/admin/FilterPanel.svelte (1)
2-2: Remove unused icon imports.
XandChevronDownare imported but not used in the template.🔎 Suggested fix
-import { X, ChevronDown, Filter } from '@lucide/svelte'; +import { Filter } from '@lucide/svelte';frontend/src/components/admin/users/UserFormModal.svelte (1)
46-57: Consider addingrequiredattribute to mandatory fields.The username field is marked with a visual indicator (
*) but lacks the HTML5requiredattribute, which would provide native browser validation and improve accessibility.🔎 Suggested improvement
<input id="user-form-username" type="text" bind:value={form.username} class="form-input-standard" placeholder="johndoe" disabled={saving} + required autocomplete="username" autocorrect="off" autocapitalize="off" spellcheck="false" />Similarly for the password field when creating a new user (not editing).
frontend/src/components/admin/events/EventsTable.svelte (1)
42-48: Inconsistent timestamp formatting between desktop and mobile views.Desktop view uses inline
toLocaleDateString()/toLocaleTimeString()(lines 44-48), while mobile view usesformatTimestamp()(line 143). This could result in different display formats. Consider usingformatTimestampconsistently, or split it into date/time helpers.🔎 Suggested improvement for consistency
<td class="px-3 py-2 text-sm text-fg-default dark:text-dark-fg-default"> <div class="text-xs text-fg-muted dark:text-dark-fg-muted"> - {new Date(event.timestamp).toLocaleDateString()} + {formatTimestamp(event.timestamp, 'date')} </div> <div class="text-sm"> - {new Date(event.timestamp).toLocaleTimeString()} + {formatTimestamp(event.timestamp, 'time')} </div> </td>This assumes extending
formatTimestampto accept an optional format parameter, or creating separate helpers.frontend/src/components/admin/ActionButtons.svelte (1)
20-27: Consider adding fallback for unknown color values.If
action.coloris set to an unrecognized value (not incolorClasses), the lookup returnsundefined. Adding a fallback ensures consistent styling.🔎 Suggested improvement
-const colorClasses: Record<string, string> = { +const colorClasses: Record<string, string> = { primary: 'text-primary hover:text-primary-dark dark:hover:text-primary-light', success: 'text-green-600 hover:text-green-800 dark:text-green-400', danger: 'text-red-600 hover:text-red-800 dark:text-red-400', warning: 'text-yellow-600 hover:text-yellow-800 dark:text-yellow-400', info: 'text-blue-600 hover:text-blue-800 dark:text-blue-400', default: 'text-fg-muted hover:text-fg-default dark:hover:text-dark-fg-default' }; + +function getColorClass(color?: string): string { + return colorClasses[color || 'default'] || colorClasses.default; +}Then use
getColorClass(action.color)instead of the direct lookup.frontend/src/components/admin/users/RateLimitsModal.svelte (1)
237-244: Clarify the usage value display logic.The fallback chain
usageData.count || usageData.tokens_remaining || 0may unintentionally showtokens_remainingwhencountis0(a valid count). Consider using nullish coalescing for more precise handling.🔎 Proposed fix
<span class="text-sm font-semibold text-fg-default dark:text-dark-fg-default"> - {usageData.count || usageData.tokens_remaining || 0} + {usageData.count ?? usageData.tokens_remaining ?? 0} </span>frontend/src/components/admin/events/EventDetailsModal.svelte (1)
59-67: Consider constraining max-height for large JSON payloads.The
<pre>elements haveoverflow-autowhich is good, but without amax-heightconstraint, extremely large metadata or payloads could make the modal unusable. Consider adding a height limit.🔎 Proposed fix
- <pre class="bg-neutral-100 dark:bg-neutral-800 p-3 rounded overflow-auto text-sm font-mono text-fg-default dark:text-dark-fg-default">{JSON.stringify(event.event.metadata, null, 2)}</pre> + <pre class="bg-neutral-100 dark:bg-neutral-800 p-3 rounded overflow-auto max-h-48 text-sm font-mono text-fg-default dark:text-dark-fg-default">{JSON.stringify(event.event.metadata, null, 2)}</pre>- <pre class="bg-neutral-100 dark:bg-neutral-800 p-3 rounded overflow-auto text-sm font-mono text-fg-default dark:text-dark-fg-default">{JSON.stringify(event.event.payload, null, 2)}</pre> + <pre class="bg-neutral-100 dark:bg-neutral-800 p-3 rounded overflow-auto max-h-64 text-sm font-mono text-fg-default dark:text-dark-fg-default">{JSON.stringify(event.event.payload, null, 2)}</pre>frontend/src/lib/admin/events/eventTypes.ts (1)
44-57: SimplifygetEventTypeLabel- redundant conditional.Lines 52-55 reconstruct the same string that was passed in. If
eventTypeis"execution.started", splitting and rejoining produces the identical string.🔎 Proposed fix
export function getEventTypeLabel(eventType: string): string { // For execution.requested, show icon only (with tooltip) if (eventType === 'execution.requested') { return ''; } - - // For all other events, show full name - const parts = eventType.split('.'); - if (parts.length === 2) { - return `${parts[0]}.${parts[1]}`; - } return eventType; }frontend/src/lib/admin/sagas/sagaStates.ts (1)
79-83: Replace magic numbers with derived constants.The hardcoded
5and3for total steps reduce maintainability. IfEXECUTION_SAGA_STEPSis modified, this function won't automatically update.🔎 Proposed fix
+const DEFAULT_SAGA_STEPS = 3; + export function getSagaProgressPercentage(completedSteps: string[], sagaName: string): number { if (!completedSteps?.length) return 0; - const totalSteps = sagaName === 'execution_saga' ? 5 : 3; + const totalSteps = sagaName === 'execution_saga' + ? EXECUTION_SAGA_STEPS.length + : DEFAULT_SAGA_STEPS; return Math.min(100, (completedSteps.length / totalSteps) * 100); }frontend/src/lib/admin/pagination.svelte.ts (1)
17-27: Consider exposingpageSizeOptionsin the returned state.
pageSizeOptionsis defined inPaginationOptionsand merged intoopts, but it's never exposed in the returnedPaginationState. Consumers rendering page-size selectors would benefit from access to this array.🔎 Proposed fix to PaginationState interface and return object
export interface PaginationState { currentPage: number; pageSize: number; totalItems: number; readonly totalPages: number; readonly skip: number; + readonly pageSizeOptions: number[]; handlePageChange: (page: number, onLoad?: () => void) => void; handlePageSizeChange: (size: number, onLoad?: () => void) => void; reset: () => void; }And in the return statement:
return { // ... existing properties get skip() { return skip; }, + get pageSizeOptions() { return opts.pageSizeOptions; }, handlePageChange, // ... };frontend/src/routes/admin/AdminSagas.svelte (1)
132-137: Simplify auto-refresh effect dependency tracking.The condition
autoRefresh || refreshRateis always truthy whenautoRefreshistrue. In Svelte 5$effect, simply reading the variables establishes reactivity. The boolean expression could be misleading.🔎 Proposed fix
// Re-setup auto-refresh when settings change $effect(() => { - if (autoRefresh || refreshRate) { - setupAutoRefresh(); - } + // Track both values to trigger effect on changes + void autoRefresh; + void refreshRate; + setupAutoRefresh(); });frontend/src/routes/admin/AdminEvents.svelte (1)
346-395: Consider using the Pagination component for consistency.This section implements pagination UI inline with custom buttons and page size selection. However,
AdminUsers.svelte(lines 274-283) uses a reusablePaginationcomponent that provides the same functionality. Using the shared component would improve consistency and reduce code duplication.💡 Suggested refactor to use Pagination component
Replace the inline pagination UI with:
- <div class="flex items-center gap-4"> - <div class="flex items-center gap-2"> - <label for="events-page-size" class="text-sm text-fg-muted dark:text-dark-fg-muted">Show:</label> - <select - id="events-page-size" - bind:value={pageSize} - onchange={() => { currentPage = 1; loadEvents(); }} - class="px-3 py-1.5 pr-8 rounded-lg border border-neutral-300 dark:border-neutral-600 bg-surface-overlay dark:bg-dark-surface-overlay text-neutral-900 dark:text-neutral-100 text-sm focus:outline-hidden focus:ring-2 focus:ring-blue-500 focus:border-blue-500 appearance-none cursor-pointer" - style="background-image: url('data:image/svg+xml;utf8,<svg xmlns=%22http://www.w3.org/2000/svg%22 fill=%22none%22 viewBox=%220 0 20 20%22><path stroke=%22%236b7280%22 stroke-linecap=%22round%22 stroke-linejoin=%22round%22 stroke-width=%221.5%22 d=%22M6 8l4 4 4-4%22/></svg>'); background-repeat: no-repeat; background-position: right 0.5rem center; background-size: 16px;" - > - <option value={10}>10</option> - <option value={25}>25</option> - <option value={50}>50</option> - <option value={100}>100</option> - </select> - <span class="text-sm text-fg-muted dark:text-dark-fg-muted">per page</span> - </div> - - {#if totalPages > 1} - <div class="flex items-center gap-1"> - <button onclick={() => { currentPage = 1; loadEvents(); }} disabled={currentPage === 1} class="pagination-button" title="First page"> - <ChevronsLeft size={16} /> - </button> - <button onclick={() => { currentPage--; loadEvents(); }} disabled={currentPage === 1} class="pagination-button" title="Previous page"> - <ChevronLeft size={16} /> - </button> - <div class="pagination-text"> - <span class="font-medium">{currentPage}</span> - <span class="text-fg-muted dark:text-dark-fg-muted mx-1">/</span> - <span class="font-medium">{totalPages}</span> - </div> - <button onclick={() => { currentPage++; loadEvents(); }} disabled={currentPage === totalPages} class="pagination-button" title="Next page"> - <ChevronRight size={16} /> - </button> - <button onclick={() => { currentPage = totalPages; loadEvents(); }} disabled={currentPage === totalPages} class="pagination-button" title="Last page"> - <ChevronsRight size={16} /> - </button> - </div> - {/if} - </div> - - <div class="text-xs sm:text-sm text-fg-muted dark:text-dark-fg-muted"> - Showing {(currentPage - 1) * pageSize + 1} to {Math.min(currentPage * pageSize, totalEvents)} of {totalEvents} events - </div> + <Pagination + {currentPage} + {totalPages} + totalItems={totalEvents} + {pageSize} + onPageChange={(page) => { currentPage = page; loadEvents(); }} + onPageSizeChange={(size) => { pageSize = size; currentPage = 1; loadEvents(); }} + pageSizeOptions={[10, 25, 50, 100]} + itemName="events" + />Don't forget to add the import at the top if not already present:
+ import Pagination from '../../components/Pagination.svelte';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
frontend/src/components/admin/ActionButtons.sveltefrontend/src/components/admin/AutoRefreshControl.sveltefrontend/src/components/admin/FilterPanel.sveltefrontend/src/components/admin/StatsCard.sveltefrontend/src/components/admin/StatusBadge.sveltefrontend/src/components/admin/events/EventDetailsModal.sveltefrontend/src/components/admin/events/EventFilters.sveltefrontend/src/components/admin/events/EventStatsCards.sveltefrontend/src/components/admin/events/EventsTable.sveltefrontend/src/components/admin/events/ReplayPreviewModal.sveltefrontend/src/components/admin/events/ReplayProgressBanner.sveltefrontend/src/components/admin/events/UserOverviewModal.sveltefrontend/src/components/admin/events/index.tsfrontend/src/components/admin/index.tsfrontend/src/components/admin/sagas/SagaDetailsModal.sveltefrontend/src/components/admin/sagas/SagaFilters.sveltefrontend/src/components/admin/sagas/SagaStatsCards.sveltefrontend/src/components/admin/sagas/SagasTable.sveltefrontend/src/components/admin/sagas/index.tsfrontend/src/components/admin/users/DeleteUserModal.sveltefrontend/src/components/admin/users/RateLimitsModal.sveltefrontend/src/components/admin/users/UserFilters.sveltefrontend/src/components/admin/users/UserFormModal.sveltefrontend/src/components/admin/users/UsersTable.sveltefrontend/src/components/admin/users/index.tsfrontend/src/lib/admin/__tests__/constants.test.tsfrontend/src/lib/admin/autoRefresh.svelte.tsfrontend/src/lib/admin/constants.tsfrontend/src/lib/admin/events/__tests__/eventTypes.test.tsfrontend/src/lib/admin/events/eventTypes.tsfrontend/src/lib/admin/events/index.tsfrontend/src/lib/admin/index.tsfrontend/src/lib/admin/pagination.svelte.tsfrontend/src/lib/admin/sagas/__tests__/sagaStates.test.tsfrontend/src/lib/admin/sagas/index.tsfrontend/src/lib/admin/sagas/sagaStates.tsfrontend/src/lib/admin/users/__tests__/rateLimits.test.tsfrontend/src/lib/admin/users/index.tsfrontend/src/lib/admin/users/rateLimits.tsfrontend/src/routes/admin/AdminEvents.sveltefrontend/src/routes/admin/AdminSagas.sveltefrontend/src/routes/admin/AdminUsers.sveltefrontend/src/routes/admin/__tests__/AdminSagas.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/lib/admin/__tests__/constants.test.ts (1)
frontend/src/lib/admin/constants.ts (5)
STATUS_COLORS(6-12)STATS_BG_COLORS(15-23)STATS_TEXT_COLORS(26-34)ROLE_COLORS(37-40)ACTIVE_STATUS_COLORS(43-47)
frontend/src/lib/admin/users/__tests__/rateLimits.test.ts (1)
frontend/src/lib/admin/users/rateLimits.ts (7)
GROUP_COLORS(7-15)getGroupColor(17-19)ENDPOINT_GROUP_PATTERNS(22-29)detectGroupFromEndpoint(31-37)getDefaultRules(44-53)getDefaultRulesWithMultiplier(55-62)createEmptyRule(65-76)
🪛 Gitleaks (8.30.0)
frontend/src/lib/admin/users/rateLimits.ts
[high] 13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (39)
frontend/src/components/admin/sagas/SagaDetailsModal.svelte (1)
1-195: Well-architected saga details modal.The component provides comprehensive saga information with proper null handling and conditional rendering. The execution steps visualization (lines 88-136) is particularly well-done, with clear visual states and proper fallback handling.
frontend/src/lib/admin/constants.ts (1)
1-51: Clean and well-organized constants file.The color mappings are consistent, well-documented, and properly typed with const assertions. The type aliases provide useful type safety for consumers.
Note: The static analysis warning about a "Generic API Key" on line 13 is a false positive—it's detecting the string 'api' in the CSS class name
'bg-neutral-100 text-neutral-800 dark:bg-neutral-700 dark:text-neutral-200', not an actual API key.frontend/src/lib/admin/users/rateLimits.ts (1)
1-76: Well-structured rate limit utilities.The module provides clear group mappings, pattern detection, and default rule generation. The use of regex patterns for endpoint grouping is pragmatic and maintainable.
Note: The static analysis warning about a "Generic API Key" on line 13 is a false positive—it's flagging the string 'api' in the CSS class definition, not an actual credential.
frontend/src/lib/admin/events/index.ts (1)
1-1: LGTM!Standard barrel export pattern for consolidating event type exports.
frontend/src/lib/admin/sagas/index.ts (1)
1-1: LGTM!Standard barrel export pattern for consolidating saga state exports.
frontend/src/lib/admin/users/index.ts (1)
1-1: LGTM!Clean barrel export that properly exposes the rate limits utilities from the users module.
frontend/src/components/admin/sagas/SagaStatsCards.svelte (1)
16-36: LGTM!Clean responsive grid layout with proper theme-aware styling. The dynamic icon rendering pattern is idiomatic Svelte 5.
frontend/src/components/admin/sagas/index.ts (1)
1-4: LGTM!Clean barrel export providing a unified entry point for saga admin components.
frontend/src/components/admin/events/EventFilters.svelte (1)
27-157: LGTM!Well-structured filter form with proper accessibility (label associations), responsive grid layout, and theme-aware styling. The
$bindablepattern correctly enables two-way binding with the parent component.frontend/src/components/admin/index.ts (1)
1-6: LGTM!Clean barrel export for shared admin components with a helpful comment indicating the module's purpose.
frontend/src/components/admin/events/EventStatsCards.svelte (1)
12-46: LGTM!Clean stats card grid with proper null safety and theme-aware styling. The conditional color for error rate provides good visual feedback.
One minor note: inside the
{#if stats}block, the optional chaining (stats?.) is technically redundant sincestatsis guaranteed to be truthy, but it's harmless and adds defensive safety.frontend/src/components/admin/users/index.ts (1)
1-5: LGTM!Clean barrel export providing a complete set of user management admin components.
frontend/src/lib/admin/index.ts (1)
1-9: LGTM!Well-organized central barrel with clear separation between shared utilities and domain-specific exports. Verification confirms no export name collisions across the re-exported modules. The wildcard re-exports provide a clean single entry point for the admin module with no risk of naming conflicts.
frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts (1)
1-105: Excellent test coverage for saga state utilities.The test suite thoroughly validates the saga state utilities with comprehensive coverage of:
- State configuration completeness and structure
- Label mappings and fallback behavior
- Step definitions and compensation logic
- Progress calculation including edge cases (empty, null/undefined, capping at 100%)
This provides good confidence in the reliability of the saga state management logic.
frontend/src/routes/admin/__tests__/AdminSagas.test.ts (1)
1-495: Comprehensive test suite with excellent coverage.This test suite provides thorough validation of the AdminSagas component functionality including:
- Loading states and data rendering
- Filter interactions (state, search, execution ID)
- Auto-refresh behavior with fake timers
- Modal interactions and content display
- Pagination controls
- Edge cases and error scenarios
The use of helper functions (
createMockSaga,renderWithSagas) and proper setup/teardown demonstrates good testing practices.frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (1)
1-246: Thorough test coverage for event type utilities.The test suite comprehensively validates event type utilities with good coverage of:
- Event type constants and categories
- Color mapping logic including dark mode variants
- Label formatting for different event type patterns
- Filter creation, detection, counting, and summary generation
- Edge cases for singular/plural handling and empty states
frontend/src/lib/admin/users/__tests__/rateLimits.test.ts (1)
1-190: Comprehensive test coverage for rate limit utilities.The test suite provides excellent validation of rate limit utilities with thorough coverage of:
- Group color mappings including dark mode variants
- Endpoint pattern detection across various endpoint types
- Default rate limit rule generation and structure validation
- Multiplier application with edge cases (default, falsy, fractional values)
- Empty rule creation
The edge case testing (lines 154-168) for multiplier handling is particularly valuable for ensuring robust behavior.
frontend/src/components/admin/StatsCard.svelte (1)
1-44: LGTM!Clean and well-structured presentational component. The Props interface is properly typed, conditional rendering is handled correctly, and the responsive design with compact mode support is a nice touch.
frontend/src/components/admin/AutoRefreshControl.svelte (1)
1-88: LGTM!Well-designed auto-refresh control component. The use of
$bindable()for two-way binding with optional callback notifications is a clean pattern. The responsive layout and loading state handling are properly implemented.frontend/src/components/admin/FilterPanel.svelte (1)
35-82: LGTM!The toggle button interaction pattern with conditional styling and the optional action buttons (Clear All, Apply) provide good flexibility. The use of
{@render children()}for content projection is correct Svelte 5 syntax.frontend/src/components/admin/users/UserFormModal.svelte (1)
38-122: LGTM on overall form structure.The modal form is well-organized with clear field groupings, appropriate autocomplete attributes for security, and proper loading state handling. The conditional UI for edit vs. create mode is cleanly implemented.
frontend/src/components/admin/events/EventsTable.svelte (1)
34-41: Good accessibility pattern for clickable rows.The table rows correctly implement keyboard navigation with
role="button",tabindex="0",aria-label, andonkeydownhandler for Enter key. ThestopPropagation()on action buttons prevents double navigation.frontend/src/components/admin/ActionButtons.svelte (1)
30-74: LGTM!Clean implementation of a reusable action buttons component with three variants. The use of
{@const IconComponent = action.icon}for dynamic component rendering is correct Svelte 5 syntax, and the disabled state handling with both attribute and visual styling is thorough.frontend/src/components/admin/users/RateLimitsModal.svelte (1)
1-57: LGTM! Well-structured modal component.The component is cleanly organized with proper TypeScript interfaces, reactive derivations, and mutation handlers. The use of
$bindableforconfigenables two-way binding from the parent while keeping the component self-contained.frontend/src/components/admin/users/UserFilters.svelte (1)
1-112: LGTM! Clean and accessible filter component.The component demonstrates good practices:
- Proper label-input associations for accessibility
- Responsive layout using Tailwind utilities
- Two-way binding with
$bindablefor parent-child state sync- Collapsible advanced section with visual rotation indicator
frontend/src/components/admin/events/EventDetailsModal.svelte (1)
92-102: LGTM! Footer actions are well-implemented.The snippet-based footer with conditional replay invocation and clear action handlers is clean.
frontend/src/lib/admin/events/eventTypes.ts (1)
84-122: LGTM! Filter utilities are well-implemented.The
hasActiveFilters,getActiveFilterCount, andgetActiveFilterSummaryfunctions have consistent logic and handle all filter fields appropriately. The time range consolidation in the summary is a nice UX touch.frontend/src/components/admin/events/ReplayPreviewModal.svelte (1)
1-89: LGTM! Well-designed replay preview component.The component has a clear structure with:
- Proper null checks before actions
- Good UX with dry-run indicator and warning about potential duplicates
- Correct pluralization logic
- Clear visual hierarchy for event details
frontend/src/lib/admin/sagas/sagaStates.ts (1)
14-51: LGTM! Comprehensive state configuration.The
SAGA_STATESmap provides a clean, centralized configuration for all saga states with consistent structure. The icon assignments are semantically appropriate (e.g.,XCirclefor failed,Clockfor timeout).frontend/src/lib/admin/pagination.svelte.ts (1)
36-76: LGTM! Clean reactive pagination factory.The factory pattern with Svelte 5 runes (
$state,$derived) and explicit getters/setters provides a well-encapsulated pagination state. Thereset()function and callback-based page handlers offer good flexibility for consumers.frontend/src/routes/admin/AdminSagas.svelte (1)
1-210: Good refactor to modular component architecture.The migration from a monolithic implementation to component-based UI (SagaStatsCards, SagaFilters, SagasTable, SagaDetailsModal) improves maintainability and testability. The lifecycle management with
onMount/onDestroyand auto-refresh logic is correctly implemented.frontend/src/routes/admin/AdminUsers.svelte (3)
19-26: Excellent componentization!The refactor from inline modal implementations to modular components (UserFilters, UsersTable, UserFormModal, DeleteUserModal, RateLimitsModal) significantly improves maintainability and separation of concerns.
67-78: Well-structured derived state for filter tracking.The
hasFiltersActivederived value cleanly checks all filter conditions to determine whether any filters are applied, which is useful for UI feedback.
95-121: Comprehensive filter implementation.The
filterUsersfunction correctly handles all filter types including search, role, status, and advanced filters (bypass rate limit, custom limits, global multiplier).frontend/src/components/admin/events/index.ts (1)
1-7: Clean barrel export pattern.This centralized export file simplifies imports for consuming modules and provides a clear public API for the admin events components.
frontend/src/routes/admin/AdminEvents.svelte (4)
21-42: Well-structured imports for modular components.The refactor introduces clean separation with dedicated components (EventStatsCards, EventFilters, EventsTable, and various modals) and helper functions from lib/admin/events.
92-110: Correct filter handling with date conversion.The
loadEventsfunction properly converts date strings to ISO format for the API call and uses skip/limit for pagination.
140-171: Well-implemented replay functionality.The replay feature correctly handles both dry-run preview and actual replay modes, with proper status polling and progress tracking for scheduled replays.
256-260: Nice use of active filter count badge.The filter button includes a visual badge showing the count of active filters using the
getActiveFilterCounthelper, providing clear feedback to users.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/routes/admin/AdminSagas.svelte (1)
45-74: Client-side filtering pagination inconsistency persists despite partial mitigation.The pagination issue flagged in the previous review remains:
totalItemsreflects the server's pre-filter count (line 57), whilesagasis reduced by client-side filtering (lines 61-72). Although the UI now shows a "filtered locally" message (lines 193-196), users still see pagination controls based on the server total rather than the filtered count. For example, if the server returns 50 sagas but filtering leaves only 3 visible, pagination shows "Page 1 of 5" rather than "Page 1 of 1."The previous review recommended either:
- Moving
executionIdFilterandsearchQueryto server-side query parameters- Updating
totalItemsafter filtering (breaking server pagination)- Documenting this as "local search within current page"
The current implementation partially adopts option 3 with the added message, but the pagination controls remain inconsistent.
🧹 Nitpick comments (3)
frontend/src/routes/admin/AdminSagas.svelte (3)
85-94: Consider pagination for execution-specific saga loading.When loading sagas for a specific execution, no
limitoroffsetparameters are applied. If an execution generates many sagas, this could load a large result set into memory and impact performance. Consider applying the same pagination approach used inloadSagasor documenting the expected maximum saga count per execution.🔎 Suggested approach: Apply pagination to execution saga queries
async function loadExecutionSagas(executionId: string): Promise<void> { loading = true; const data = unwrapOr(await getExecutionSagasApiV1SagasExecutionExecutionIdGet({ - path: { execution_id: executionId } + path: { execution_id: executionId }, + query: { + limit: pageSize, + offset: (currentPage - 1) * pageSize + } }), null); loading = false; sagas = data?.sagas || []; totalItems = data?.total || 0; executionIdFilter = executionId; }
136-141: Consider clarifying the auto-refresh effect condition.The condition
if (autoRefresh || refreshRate)triggers the effect when either value changes, butsetupAutoRefreshonly acts whenautoRefreshis true. While functionally safe, this can be confusing—changingrefreshRatewhileautoRefreshis false will trigger the effect unnecessarily.Consider making the dependency explicit:
🔎 Suggested clarification
// Re-setup auto-refresh when settings change $effect(() => { - if (autoRefresh || refreshRate) { - setupAutoRefresh(); - } + autoRefresh; // track dependency + refreshRate; // track dependency + setupAutoRefresh(); });Or alternatively, track only when
autoRefreshis enabled:$effect(() => { - if (autoRefresh || refreshRate) { + if (autoRefresh) { + refreshRate; // track rate changes too setupAutoRefresh(); } });
61-62: Case-sensitivity inconsistency between filter types.The
executionIdFilteruses case-sensitive matching (.includes(executionIdFilter)), whilesearchQueryapplies case-insensitive matching to the sameexecution_idfield (line 69:.toLowerCase().includes(q)). This inconsistency may confuse users when the same execution ID matches differently depending on which filter is used.Consider normalizing both to case-insensitive matching for consistent behavior:
🔎 Suggested consistency fix
// Client-side filtering for execution ID and search (filters within current page only) if (executionIdFilter) { - result = result.filter(s => s.execution_id.includes(executionIdFilter)); + result = result.filter(s => + s.execution_id.toLowerCase().includes(executionIdFilter.toLowerCase()) + ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/src/components/admin/events/EventFilters.sveltefrontend/src/components/admin/users/DeleteUserModal.sveltefrontend/src/lib/admin/events/__tests__/eventTypes.test.tsfrontend/src/lib/admin/users/__tests__/rateLimits.test.tsfrontend/src/lib/admin/users/rateLimits.tsfrontend/src/routes/admin/AdminSagas.sveltefrontend/src/routes/admin/AdminUsers.sveltefrontend/src/routes/admin/__tests__/AdminEvents.test.tsfrontend/src/routes/admin/__tests__/AdminSagas.test.tsfrontend/src/routes/admin/__tests__/AdminUsers.test.tsfrontend/src/routes/admin/__tests__/test-utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/admin/events/EventFilters.svelte
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/routes/admin/__tests__/AdminEvents.test.ts (2)
frontend/src/lib/admin/events/eventTypes.ts (1)
EVENT_TYPES(6-17)frontend/src/routes/admin/__tests__/test-utils.ts (2)
mockElementAnimate(34-36)mockWindowGlobals(41-44)
frontend/src/routes/admin/__tests__/AdminSagas.test.ts (2)
frontend/src/lib/admin/sagas/sagaStates.ts (1)
SAGA_STATES(14-51)frontend/src/routes/admin/__tests__/test-utils.ts (1)
mockElementAnimate(34-36)
frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (1)
frontend/src/lib/admin/events/eventTypes.ts (7)
EventFilters(60-69)EVENT_TYPES(6-17)getEventTypeColor(22-42)getEventTypeLabel(45-57)hasActiveFilters(84-95)getActiveFilterCount(97-108)getActiveFilterSummary(110-122)
frontend/src/routes/admin/__tests__/AdminUsers.test.ts (1)
frontend/src/routes/admin/__tests__/test-utils.ts (1)
mockElementAnimate(34-36)
frontend/src/lib/admin/users/__tests__/rateLimits.test.ts (1)
frontend/src/lib/admin/users/rateLimits.ts (7)
getDefaultRules(44-53)GROUP_COLORS(7-15)getGroupColor(17-19)ENDPOINT_GROUP_PATTERNS(22-29)detectGroupFromEndpoint(31-37)getDefaultRulesWithMultiplier(55-63)createEmptyRule(66-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (16)
frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (8)
1-14: LGTM! Clean test setup.The imports are comprehensive, and the
withFilterhelper function provides a concise way to create test fixtures with partial overrides. This improves test readability throughout the suite.
16-26: LGTM! Comprehensive constant validation.The test ensures all expected event types are present in the
EVENT_TYPESconstant. UsingforEachwith individual assertions provides complete feedback if multiple events are missing.
28-46: LGTM! Excellent parameterized color testing.The test suite comprehensively validates all color mappings, including the unknown event fallback. Checking for both the color class and dark mode variant ensures proper styling across themes.
48-58: LGTM! Label generation well tested.The tests cover the special case for
execution.requested(empty label for icon-only display), standard two-part events, and edge cases like single words and deeply nested event names.
60-77: LGTM! Default filter creation properly tested.The tests validate both the structure of default filters and that each call returns a new object instance. The identity check on Line 75 is important to ensure callers can safely mutate their filter objects without affecting other consumers.
79-96: LGTM! Comprehensive active filter detection.The test suite validates detection of active filters across all 8 fields, plus the empty case. The parameterized approach ensures consistent coverage.
98-110: LGTM! Filter count calculation validated.The tests cover the key scenarios: no filters, multiple filters, and all filters active. This provides sufficient confidence in the counting logic without exhaustive permutation testing.
112-136: LGTM! Excellent summary generation testing.The test suite is thorough, covering empty filters, individual labels with proper singular/plural handling for event types (lines 118-119), time range coalescing for start/end times (lines 122-123), and comprehensive multi-filter scenarios. This ensures the summary UI will display correctly across all use cases.
frontend/src/routes/admin/__tests__/AdminSagas.test.ts (1)
1-476: LGTM! Comprehensive test coverage for AdminSagas.The test file is well-structured with proper mock setup, test isolation, and comprehensive coverage across loading, rendering, filters, pagination, and modal interactions. The use of hoisted mocks, fake timers, and proper cleanup demonstrates good testing practices.
frontend/src/components/admin/users/DeleteUserModal.svelte (1)
1-69: LGTM! Safe default for cascade delete.The component now correctly defaults
cascadeDeletetofalse, requiring explicit user opt-in for the destructive cascade operation. The conditional warning and proper loading states enhance the user experience.frontend/src/routes/admin/__tests__/AdminEvents.test.ts (1)
1-892: LGTM! Good refactor to centralized test utilities.The migration to use
mockElementAnimate()andmockWindowGlobals()from test-utils improves maintainability by eliminating duplicate mock setup code. The cleanup withvi.unstubAllGlobals()ensures proper test isolation.frontend/src/routes/admin/__tests__/test-utils.ts (1)
1-44: LGTM! Well-structured test utilities.The centralized test utilities provide proper mocks for DOM animations and window globals. The
animateMockcomprehensively covers the Web Animations API, and the use ofvi.stubGlobal()is the correct approach for mocking window methods.frontend/src/routes/admin/__tests__/AdminUsers.test.ts (1)
394-500: LGTM! Tests verify safe cascade delete behavior.The new tests properly validate that cascade delete defaults to
false(safe default) and that the warning message appears only when cascade is explicitly enabled. This ensures the dangerous operation requires user opt-in.frontend/src/lib/admin/users/__tests__/rateLimits.test.ts (1)
1-131: LGTM! Comprehensive test coverage for rate limits.The test suite thoroughly validates the rate limit utilities including group detection, default rules, multiplier calculations, and edge cases. The tests for non-positive multipliers (lines 104-109) correctly verify that 0 and negative values default to 1.0, matching the implementation's intent.
frontend/src/lib/admin/users/rateLimits.ts (1)
55-63: LGTM! Intentional handling of non-positive multipliers.The implementation explicitly treats 0 and negative multipliers as invalid (line 58:
multiplier > 0 ? multiplier : 1.0), which is a reasonable design choice. The inline comment (line 57) clearly documents this behavior, and the test suite verifies it. This prevents edge cases like 0 effective requests that would block all access.frontend/src/routes/admin/AdminUsers.svelte (1)
1-325: LGTM! Clean refactor to modular components.The component has been successfully refactored to use dedicated modal components (
DeleteUserModal,RateLimitsModal,UserFormModal) and specialized UI components (UserFilters,UsersTable). This improves maintainability and separation of concerns. The cascade delete integration (lines 158-170, 291-299) correctly manages state and passes it to the modal component.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/lib/api-interceptors.ts (1)
50-52: Same unsafe array access informatValidationErrors.This function has the same vulnerability as
getErrorMessage. Apply the same defensive fix here.🔎 Suggested defensive fix
function formatValidationErrors(detail: ValidationError[]): string { - return detail.map((e) => `${e.loc[e.loc.length - 1]}: ${e.msg}`).join('\n'); + return detail.map((e) => { + const field = Array.isArray(e.loc) && e.loc.length ? e.loc[e.loc.length - 1] : 'field'; + return `${field}: ${e.msg}`; + }).join('\n'); }frontend/src/routes/admin/AdminUsers.svelte (1)
136-156: Unhandled error from unwrap in saveUser.At line 153,
unwrap(result)is called after settingsavingUser = false. If the API returns an error,unwrapwill throw, but there's no try-catch block. This will cause an unhandled exception.🔎 Proposed fix
async function saveUser(): Promise<void> { if (!userForm.username) { addToast('Username is required', 'error'); return; } if (!editingUser && !userForm.password) { addToast('Password is required', 'error'); return; } savingUser = true; - let result; - if (editingUser) { - const updateData: Record<string, string | boolean | null> = { - username: userForm.username, email: userForm.email || null, role: userForm.role, is_active: userForm.is_active - }; - if (userForm.password) updateData.password = userForm.password; - result = await updateUserApiV1AdminUsersUserIdPut({ path: { user_id: editingUser.user_id }, body: updateData }); - } else { - result = await createUserApiV1AdminUsersPost({ - body: { username: userForm.username, email: userForm.email || null, password: userForm.password, role: userForm.role, is_active: userForm.is_active } - }); + try { + let result; + if (editingUser) { + const updateData: Record<string, string | boolean | null> = { + username: userForm.username, email: userForm.email || null, role: userForm.role, is_active: userForm.is_active + }; + if (userForm.password) updateData.password = userForm.password; + result = await updateUserApiV1AdminUsersUserIdPut({ path: { user_id: editingUser.user_id }, body: updateData }); + } else { + result = await createUserApiV1AdminUsersPost({ + body: { username: userForm.username, email: userForm.email || null, password: userForm.password, role: userForm.role, is_active: userForm.is_active } + }); + } + unwrap(result); + showUserModal = false; + await loadUsers(); + addToast(editingUser ? 'User updated successfully.' : 'User created successfully.', 'success'); + } catch (err) { + addToast('Failed to save user. Please try again.', 'error'); + } finally { + savingUser = false; } - savingUser = false; - unwrap(result); - showUserModal = false; - await loadUsers(); }frontend/src/routes/admin/AdminEvents.svelte (1)
92-110: Potential runtime error on invalid date input.If
filters.start_timeorfilters.end_timecontains an invalid date string,new Date(invalidString).toISOString()will throw aRangeError: Invalid time value. Consider adding validation or wrapping in try-catch.🔎 Suggested defensive fix
body: { filters: { ...filters, - start_time: filters.start_time ? new Date(filters.start_time).toISOString() : null, - end_time: filters.end_time ? new Date(filters.end_time).toISOString() : null + start_time: filters.start_time && !isNaN(Date.parse(filters.start_time)) + ? new Date(filters.start_time).toISOString() + : null, + end_time: filters.end_time && !isNaN(Date.parse(filters.end_time)) + ? new Date(filters.end_time).toISOString() + : null },
♻️ Duplicate comments (9)
frontend/src/components/__tests__/Header.test.ts (1)
118-129: Verify that Header.svelte now includes the icon-specific CSS classes.The test expects SVG elements to have icon-specific classes (
lucide-sun,lucide-moon,lucide-monitor-cog), but a previous review flagged that Header.svelte only passesclass="w-5 h-5"to Lucide icon components without these icon-specific classes. Since @lucide/svelte components don't automatically add type-based classes, they must be explicitly passed as props (e.g.,<Sun class="w-5 h-5 lucide-sun" />).Confirm that Header.svelte has been updated to include these classes in the icon component props; otherwise, this test will fail.
Run the following script to verify the current Header.svelte implementation:
#!/bin/bash # Description: Check if Header.svelte includes icon-specific classes on Lucide icon components # Search for Lucide icon component usage in Header.svelte around the theme toggle button rg -n -A2 -B2 'lucide-(sun|moon|monitor-cog)' frontend/src/components/Header.svelte # If no matches, check how the theme toggle icons are currently implemented echo "---" echo "Current icon component usage in theme toggle:" rg -n -A5 -B5 'Sun|Moon.*class=' frontend/src/components/Header.svelte || echo "No matches found with icon-specific classes"frontend/src/lib/api-interceptors.ts (1)
37-39: Unresolved: Potential runtime error iflocarray is empty or undefined.The previous review identified that accessing
e.loc[e.loc.length - 1]without validation can throw if the API returns a malformedValidationErrorwith an empty or missinglocarray. This issue remains unaddressed.🔎 Suggested defensive fix
if (Array.isArray(detail) && detail.length > 0) { - return detail.map((e) => `${e.loc[e.loc.length - 1]}: ${e.msg}`).join(', '); + return detail.map((e) => { + const field = Array.isArray(e.loc) && e.loc.length ? e.loc[e.loc.length - 1] : 'field'; + return `${field}: ${e.msg}`; + }).join(', '); }frontend/src/lib/admin/users/rateLimits.ts (1)
55-63: Multiplier fallback behavior differs from past review suggestion.The past review suggested using nullish coalescing (
??) to allowmultiplier=0as a valid input. The current implementation explicitly rejects0and negative values, falling back to1.0.If disabling rate limits via
multiplier=0is a valid use case (e.g., for testing or special accounts), the current logic would prevent this. If the intent is that0should never be valid, please document this constraint.frontend/src/components/NotificationCenter.svelte (1)
204-214: Permission handling looks correct, but tests are still needed.The SSR-safe permission initialization and
requestNotificationPermissionfunction are implemented correctly. The existing past review comment about missing test coverage for this flow remains valid.frontend/src/components/__tests__/NotificationCenter.test.ts (1)
51-51: Loading store mock is exposed but loading UI test is still missing.The
mockLoadingderived store is correctly implemented. However, the past review comment about adding a test to verify the loading spinner displays when$loadingis true remains unaddressed.frontend/src/lib/user-settings.ts (2)
38-43: Type mismatch persists: API response may contain nulls.The
unwrap()returnsUserSettingsfrom the API, butupdateUserSettingsApiV1UserSettingsPutactually returns a response where fields can benull(matchingUserSettingsUpdate). When passing directly tosetUserSettings(data), null values may violate the store's type contract.This was flagged in a past review. Consider filtering out null values before updating the store.
22-26: Same potential issue in loadUserSettings.The API response from
getUserSettingsApiV1UserSettingsGetis passed directly tosetUserSettings(data). If the API returns null fields, this could cause type inconsistencies in the store.frontend/src/routes/admin/AdminSagas.svelte (1)
60-73: Client-side filtering still causes pagination inconsistency.The past review comment remains partially valid: while UI messaging was added, the core issue persists. When client-side filters are active,
totalPagesis still calculated from server'stotalItems, but the displayed items are filtered locally. Navigating to page 2 could show 0 results if all matching items were on page 1.Consider documenting this as intentional "search within current page" behavior or moving filters server-side for accurate pagination.
frontend/src/routes/Editor.svelte (1)
189-206: Silent failure persists for non-404 update errors.When
updateSavedScriptApiV1ScriptsScriptIdPutreturns an error that's not a 404, the function returns early at line 198 without user feedback. This was flagged in a past review.🔎 Proposed fix
if (id) { const { error, response } = await updateSavedScriptApiV1ScriptsScriptIdPut({ path: { script_id: id }, body }); if (error) { if (response?.status === 404) { currentScriptId.set(null); const data = unwrap(await createSavedScriptApiV1ScriptsPost({ body })); currentScriptId.set(data.id); addToast('Script saved successfully.', 'success'); + } else { + addToast('Failed to update script. Please try again.', 'error'); } return; }
🧹 Nitpick comments (9)
frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (1)
42-44: Consider exact color string validation for stricter regression detection.The current substring checks (
.toContain()) are reasonable and flexible, but exact string assertions would catch mismatches between light and dark color variants (e.g., if someone accidentally usedtext-green-600 dark:text-red-400).💡 Example: exact string matching
-const color = getEventTypeColor(eventType); -expect(color).toContain(expectedColor); -expect(color).toContain('dark:'); +expect(getEventTypeColor(eventType)).toBe('text-green-600 dark:text-green-400');This would require updating the test data structure to include full expected strings rather than just the color prefix, but it provides stronger guarantees.
frontend/src/routes/admin/__tests__/AdminEvents.test.ts (1)
27-30: Consider using the canonical EVENT_TYPES from the source module.The local
EVENT_TYPESarray contains only 6 event types, while the source module (frontend/src/lib/admin/events/eventTypes.ts) defines 11 event types. While this may be intentional for test brevity, using the canonical list ensures tests remain in sync with production code.🔎 Proposed fix
-const EVENT_TYPES = [ - 'execution.requested', 'execution.started', 'execution.completed', - 'execution.failed', 'pod.created', 'pod.running' -]; +import { EVENT_TYPES } from '$lib/admin/events/eventTypes';frontend/src/components/admin/events/EventDetailsModal.svelte (1)
92-102: Consider simplifying the footer snippet.The footer snippet pattern is good, but the guard on line 94 (
event && onReplay(...)) could benefit from a brief comment explaining why it's needed despite the parent{#if event}block—the snippet may execute independently of that conditional.frontend/src/routes/Settings.svelte (1)
81-87: Add type annotation to event handler parameter.The
eventparameter inhandleClickOutsidelacks a type annotation, which could cause TypeScript warnings depending on compiler strictness.🔎 Proposed fix
- const handleClickOutside = (event) => { + const handleClickOutside = (event: MouseEvent) => { const target = event.target;frontend/src/components/editor/LanguageSelect.svelte (1)
110-113: Minor: Mouse leave may interfere with keyboard navigation.When a user navigates with the keyboard and then moves the mouse out of the menu,
onmouseleaveresetshoveredLangandfocusedVersionIndex, potentially disrupting keyboard-focused state. Consider preserving keyboard focus state unless a new mouse interaction occurs.🔎 Possible approach
Track whether the last interaction was keyboard or mouse, and only reset on mouseleave if the last interaction was mouse-based:
let lastInteractionType = $state<'mouse' | 'keyboard'>('mouse');Then conditionally reset in
onmouseleaveonly whenlastInteractionType === 'mouse'.frontend/src/components/__tests__/NotificationCenter.test.ts (1)
362-370: Icon class assertion may fail for missing icons.The selector
container.querySelector(\[aria-label*="${subject}"] svg`)may return null if the notification button isn't found, causingsvg?.classList.contains(iconClass)to evaluateundefined?.contains()which returnsundefined, notfalse`.🔎 Consider adding an explicit existence check
await waitFor(() => { const svg = container.querySelector(`[aria-label*="${subject}"] svg`); + expect(svg).toBeInTheDocument(); expect(svg?.classList.contains(iconClass)).toBe(true); });frontend/src/lib/editor/execution.svelte.ts (2)
35-82: EventSource is not cleaned up if the component unmounts during execution.The EventSource created inside the Promise has no external cleanup mechanism. If the calling component unmounts while execution is in progress, the stream will remain open and the state machine may attempt to update destroyed state.
Consider returning an abort mechanism from
execute()or storing the EventSource reference for cleanup.
93-96: Consider adding a comment to clarify the empty catch block.The empty catch on line 96 is intentional (to preserve the original error), but this intent isn't obvious. A brief comment would improve clarity.
🔎 Proposed improvement
if (data) { result = data; error = null; } - } catch { /* keep error */ } + } catch { + // Ignore - preserve the original error message + }frontend/src/routes/admin/AdminEvents.svelte (1)
402-428: Consider simplifying redundant modalopenprops.The
openprop values are redundant with their surrounding{#if}conditions. For example, on line 405open={!!selectedEvent}is alwaystrueinside{#if selectedEvent}. This works but adds unnecessary complexity.🔎 Simplified modal declarations
{#if selectedEvent} <EventDetailsModal event={selectedEvent} - open={!!selectedEvent} + open={true} onClose={() => selectedEvent = null} onReplay={handleReplayFromModal} onViewRelated={loadEventDetail} /> {/if} {#if showReplayPreview && replayPreview} <ReplayPreviewModal preview={replayPreview} - open={showReplayPreview && !!replayPreview} + open={true} onClose={() => { showReplayPreview = false; replayPreview = null; }} onConfirm={handleReplayConfirm} /> {/if}Alternatively, if the modal components handle their own visibility via the
openprop, you could remove the{#if}wrappers entirely and rely solely onopen.
📜 Review details
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 (84)
frontend/package.jsonfrontend/rollup.config.jsfrontend/src/App.sveltefrontend/src/components/Header.sveltefrontend/src/components/NotificationCenter.sveltefrontend/src/components/ProtectedRoute.sveltefrontend/src/components/ToastContainer.sveltefrontend/src/components/__tests__/ErrorDisplay.test.tsfrontend/src/components/__tests__/Footer.test.tsfrontend/src/components/__tests__/Header.test.tsfrontend/src/components/__tests__/NotificationCenter.test.tsfrontend/src/components/__tests__/ProtectedRoute.test.tsfrontend/src/components/__tests__/Spinner.test.tsfrontend/src/components/__tests__/ToastContainer.test.tsfrontend/src/components/admin/AutoRefreshControl.sveltefrontend/src/components/admin/events/EventDetailsModal.sveltefrontend/src/components/admin/events/EventFilters.sveltefrontend/src/components/admin/events/EventStatsCards.sveltefrontend/src/components/admin/events/EventsTable.sveltefrontend/src/components/admin/events/ReplayPreviewModal.sveltefrontend/src/components/admin/events/ReplayProgressBanner.sveltefrontend/src/components/admin/events/UserOverviewModal.sveltefrontend/src/components/admin/events/index.tsfrontend/src/components/admin/index.tsfrontend/src/components/admin/sagas/SagaDetailsModal.sveltefrontend/src/components/admin/sagas/SagaFilters.sveltefrontend/src/components/admin/sagas/SagaStatsCards.sveltefrontend/src/components/admin/sagas/SagasTable.sveltefrontend/src/components/admin/sagas/index.tsfrontend/src/components/admin/users/DeleteUserModal.sveltefrontend/src/components/admin/users/RateLimitsModal.sveltefrontend/src/components/admin/users/UserFormModal.sveltefrontend/src/components/admin/users/UsersTable.sveltefrontend/src/components/admin/users/index.tsfrontend/src/components/editor/CodeMirrorEditor.sveltefrontend/src/components/editor/LanguageSelect.sveltefrontend/src/components/editor/OutputPanel.sveltefrontend/src/components/editor/ResourceLimits.sveltefrontend/src/components/editor/index.tsfrontend/src/lib/__mocks__/api-interceptors.tsfrontend/src/lib/__tests__/auth-init.test.tsfrontend/src/lib/__tests__/user-settings.test.tsfrontend/src/lib/admin/__tests__/constants.test.tsfrontend/src/lib/admin/events/__tests__/eventTypes.test.tsfrontend/src/lib/admin/events/index.tsfrontend/src/lib/admin/index.tsfrontend/src/lib/admin/sagas/__tests__/sagaStates.test.tsfrontend/src/lib/admin/sagas/index.tsfrontend/src/lib/admin/sagas/sagaStates.tsfrontend/src/lib/admin/users/__tests__/rateLimits.test.tsfrontend/src/lib/admin/users/index.tsfrontend/src/lib/admin/users/rateLimits.tsfrontend/src/lib/api-interceptors.tsfrontend/src/lib/auth-init.tsfrontend/src/lib/editor/execution.svelte.tsfrontend/src/lib/editor/index.tsfrontend/src/lib/user-settings.tsfrontend/src/main.tsfrontend/src/routes/Editor.sveltefrontend/src/routes/Home.sveltefrontend/src/routes/Login.sveltefrontend/src/routes/Notifications.sveltefrontend/src/routes/Register.sveltefrontend/src/routes/Settings.sveltefrontend/src/routes/admin/AdminEvents.sveltefrontend/src/routes/admin/AdminLayout.sveltefrontend/src/routes/admin/AdminSagas.sveltefrontend/src/routes/admin/AdminSettings.sveltefrontend/src/routes/admin/AdminUsers.sveltefrontend/src/routes/admin/__tests__/AdminEvents.test.tsfrontend/src/routes/admin/__tests__/AdminSagas.test.tsfrontend/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/__tests__/theme.test.tsfrontend/src/stores/__tests__/toastStore.test.tsfrontend/src/stores/auth.tsfrontend/src/stores/notificationStore.tsfrontend/src/stores/theme.tsfrontend/src/stores/userSettings.tsfrontend/src/utils/__tests__/meta.test.tsfrontend/tsconfig.jsonfrontend/vitest.config.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/tests/ProtectedRoute.test.ts
- frontend/src/App.svelte
🚧 Files skipped from review as they are similar to previous changes (28)
- frontend/src/components/admin/sagas/index.ts
- frontend/src/lib/admin/index.ts
- frontend/src/lib/admin/sagas/index.ts
- frontend/src/stores/userSettings.ts
- frontend/src/lib/admin/events/index.ts
- frontend/src/components/admin/users/UsersTable.svelte
- frontend/src/components/ToastContainer.svelte
- frontend/src/components/admin/index.ts
- frontend/src/components/editor/CodeMirrorEditor.svelte
- frontend/src/components/admin/events/EventsTable.svelte
- frontend/src/components/admin/users/UserFormModal.svelte
- frontend/src/components/editor/index.ts
- frontend/src/components/Header.svelte
- frontend/src/routes/Home.svelte
- frontend/rollup.config.js
- frontend/src/components/admin/events/ReplayPreviewModal.svelte
- frontend/src/lib/admin/sagas/sagaStates.ts
- frontend/src/lib/editor/index.ts
- frontend/src/lib/auth-init.ts
- frontend/src/components/admin/events/index.ts
- frontend/src/main.ts
- frontend/src/components/admin/users/RateLimitsModal.svelte
- frontend/src/stores/theme.ts
- frontend/src/components/editor/ResourceLimits.svelte
- frontend/src/stores/auth.ts
- frontend/package.json
- frontend/src/stores/notificationStore.ts
- frontend/src/lib/admin/users/index.ts
🧰 Additional context used
🧬 Code graph analysis (13)
frontend/src/lib/api-interceptors.ts (4)
frontend/src/stores/toastStore.ts (2)
ToastType(3-3)addToast(16-25)frontend/src/lib/__mocks__/api-interceptors.ts (1)
getErrorMessage(7-7)frontend/src/lib/auth-init.ts (1)
isAuthenticated(154-160)frontend/src/stores/auth.ts (1)
isAuthenticated(38-38)
frontend/src/routes/admin/__tests__/AdminEvents.test.ts (2)
frontend/src/lib/admin/events/eventTypes.ts (1)
EVENT_TYPES(6-17)frontend/src/routes/admin/__tests__/test-utils.ts (2)
mockElementAnimate(34-36)mockWindowGlobals(41-44)
frontend/src/lib/admin/__tests__/constants.test.ts (1)
frontend/src/lib/admin/constants.ts (5)
STATUS_COLORS(6-12)STATS_BG_COLORS(15-23)STATS_TEXT_COLORS(26-34)ROLE_COLORS(37-40)ACTIVE_STATUS_COLORS(43-47)
frontend/src/routes/admin/__tests__/AdminUsers.test.ts (1)
frontend/src/routes/admin/__tests__/test-utils.ts (1)
mockElementAnimate(34-36)
frontend/src/lib/admin/sagas/__tests__/sagaStates.test.ts (1)
frontend/src/lib/admin/sagas/sagaStates.ts (4)
SAGA_STATES(14-51)getSagaStateInfo(60-62)EXECUTION_SAGA_STEPS(71-77)getSagaProgressPercentage(79-83)
frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (1)
frontend/src/lib/admin/events/eventTypes.ts (8)
EventFilters(60-69)createDefaultEventFilters(71-82)EVENT_TYPES(6-17)getEventTypeColor(22-42)getEventTypeLabel(45-57)hasActiveFilters(84-95)getActiveFilterCount(97-108)getActiveFilterSummary(110-122)
frontend/src/lib/user-settings.ts (7)
frontend/src/stores/userSettings.ts (1)
setUserSettings(20-22)frontend/src/stores/theme.ts (1)
setTheme(77-79)frontend/src/lib/api/index.ts (2)
UserSettingsUpdate(4-4)updateUserSettingsApiV1UserSettingsPut(3-3)frontend/src/lib/api/types.gen.ts (1)
UserSettingsUpdate(2989-3011)frontend/src/stores/auth.ts (1)
isAuthenticated(38-38)frontend/src/lib/api-interceptors.ts (1)
unwrap(148-151)frontend/src/lib/api/sdk.gen.ts (1)
updateUserSettingsApiV1UserSettingsPut(556-563)
frontend/src/stores/__tests__/notificationStore.test.ts (2)
frontend/src/components/__tests__/NotificationCenter.test.ts (1)
notificationStore(86-86)frontend/src/stores/notificationStore.ts (1)
notificationStore(99-99)
frontend/src/stores/__tests__/theme.test.ts (2)
frontend/src/components/__tests__/Header.test.ts (2)
theme(38-38)toggleTheme(39-39)frontend/src/stores/theme.ts (2)
theme(39-48)toggleTheme(68-75)
frontend/src/lib/__tests__/user-settings.test.ts (3)
frontend/src/stores/theme.ts (1)
theme(39-48)frontend/src/lib/user-settings.ts (2)
loadUserSettings(13-32)saveUserSettings(34-50)frontend/src/stores/userSettings.ts (1)
editorSettings(15-18)
frontend/src/routes/admin/__tests__/AdminSagas.test.ts (2)
frontend/src/lib/admin/sagas/sagaStates.ts (1)
SAGA_STATES(14-51)frontend/src/routes/admin/__tests__/test-utils.ts (1)
mockElementAnimate(34-36)
frontend/src/stores/__tests__/auth.test.ts (2)
frontend/src/lib/auth-init.ts (1)
isAuthenticated(154-160)frontend/src/stores/auth.ts (5)
isAuthenticated(38-38)username(39-39)userRole(41-41)login(59-88)logout(100-109)
frontend/src/lib/admin/users/__tests__/rateLimits.test.ts (1)
frontend/src/lib/admin/users/rateLimits.ts (7)
getDefaultRules(44-53)GROUP_COLORS(7-15)getGroupColor(17-19)ENDPOINT_GROUP_PATTERNS(22-29)detectGroupFromEndpoint(31-37)getDefaultRulesWithMultiplier(55-63)createEmptyRule(66-77)
🪛 Gitleaks (8.30.0)
frontend/src/lib/admin/users/rateLimits.ts
[high] 13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (72)
frontend/src/stores/__tests__/toastStore.test.ts (1)
3-3: LGTM! Clean import alias refactor.The migration from relative import path to the alias-based
$stores/toastStoreimproves maintainability and aligns perfectly with the PR objectives. The test coverage remains comprehensive and unchanged.frontend/src/components/__tests__/ErrorDisplay.test.ts (1)
4-4: LGTM! Clean alias migration.The import path change from relative to alias-based resolution aligns with the PR's module resolution strategy. The test suite remains comprehensive with proper mocking, security checks, and user interaction testing.
frontend/src/components/__tests__/Header.test.ts (1)
47-47: LGTM: Alias import aligns with PR objectives.The change from relative import to alias path (
$components/) is consistent with the PR's goal of standardizing imports across the codebase.frontend/src/components/ProtectedRoute.svelte (1)
4-6: LGTM! Import path aliases improve maintainability.The migration from relative imports to path aliases (
$stores/,$lib/,$components/) aligns with the PR objectives and improves code organization. No functional changes to the authentication flow.frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (1)
1-137: Excellent test coverage and structure!This test suite provides comprehensive coverage of the eventTypes utilities with well-organized test cases. The use of parameterized tests (
.each), the clean helper functionwithFilter, and coverage of edge cases (immutability check on line 74-76, pluralization on lines 118-119) demonstrate solid testing practices.frontend/src/components/__tests__/Footer.test.ts (1)
3-3: LGTM! Clean migration to alias-based imports.The import path update from relative to alias-based (
$components/Footer.svelte) improves consistency across the codebase while preserving all test logic.frontend/src/routes/admin/AdminLayout.svelte (2)
4-7: LGTM! Import paths migrated to aliases.The migration from relative imports to alias-based paths (
$stores/...,$components/...) improves code maintainability and aligns with the project-wide aliasing strategy.
9-9: LGTM! Icon componentization improves maintainability.Replacing the inline SVG with the
ShieldCheckcomponent from@lucide/sveltereduces code duplication and makes icon usage more consistent across the application.Also applies to: 82-82
frontend/src/utils/__tests__/meta.test.ts (1)
2-2: LGTM! Import path migrated to alias.The update to use
$utils/metainstead of a relative path improves consistency with the project's aliasing strategy.frontend/src/routes/Register.svelte (1)
3-8: LGTM! Imports migrated to alias-based paths.All import paths have been updated to use project aliases (
$stores/...,$lib/...,$utils/...,$components/...), improving consistency and maintainability without altering any logic.frontend/src/components/__tests__/ToastContainer.test.ts (1)
4-5: LGTM! Test imports migrated to aliases.The imports now use
$components/...and$stores/...aliases, aligning with the project-wide import strategy while preserving all test logic.frontend/src/lib/admin/__tests__/constants.test.ts (1)
1-68: LGTM! Comprehensive test coverage for admin constants.This new test file provides thorough validation of all admin color constants, including:
- Status, background, and text color mappings
- Dark mode variant verification
- Role color lookups with appropriate handling of unknown roles
- Active/inactive/disabled status colors
The test structure is clear and covers expected behavior well.
frontend/src/routes/admin/__tests__/AdminUsers.test.ts (5)
5-5: LGTM! Centralized test utilities improve maintainability.The migration to alias-based imports and centralized mock utilities (
mockElementAnimate,MockAdminLayout) reduces test setup duplication and improves consistency across admin test suites.Also applies to: 89-93
7-43: LGTM! Centralized mock data factories improve test clarity.The introduction of
DEFAULT_USER,createMockUser, andcreateMockUsersprovides a consistent foundation for test data creation, making tests more readable and maintainable.
394-414: LGTM! Important test coverage for default deletion behavior.This new test ensures that user deletion defaults to
cascade: false, which is the safer behavior. The explicit verification of the query parameter is valuable for preventing accidental data loss.
426-428: LGTM! Cascade deletion test now properly toggles the checkbox.The updated test correctly simulates enabling cascade deletion via the checkbox before confirming, ensuring the UI flow matches the expected
cascade: trueAPI call.
481-500: LGTM! Comprehensive test for cascade warning visibility.This test properly validates that:
- The cascade warning is hidden by default (when cascade is disabled)
- The warning appears when the cascade checkbox is enabled
This ensures users are appropriately warned about the destructive nature of cascade deletion.
frontend/src/lib/admin/users/__tests__/rateLimits.test.ts (1)
1-131: LGTM! Excellent test coverage for rate limit utilities.This new test file provides comprehensive validation of the rate limit module:
- GROUP_COLORS & getGroupColor: Verifies all expected groups with dark mode support and proper fallback behavior
- detectGroupFromEndpoint: Extensive test cases covering all endpoint categories (execution, admin, auth, sse, websocket, public, api) with regex pattern handling
- getDefaultRules: Validates rule structure and default values
- getDefaultRulesWithMultiplier: Tests multiplier calculations including edge cases (non-positive multipliers default to 1.0)
- createEmptyRule: Verifies default structure and object identity
The parameterized tests (using
it.each) provide thorough coverage while maintaining readability.frontend/src/components/admin/users/DeleteUserModal.svelte (1)
19-19: LGTM! Critical issue resolved.The
cascadeDeleteprop now correctly defaults tofalse, requiring explicit opt-in for the destructive cascade operation. This addresses the critical safety concern from the previous review.frontend/src/lib/api-interceptors.ts (2)
64-81: LGTM! Clean 401 handling with deduplication.The
handle401function properly prevents repeated handling with theisHandling401flag and provides clear user feedback via toast. The session expiry flow is well-structured.
83-114: LGTM! Consolidated error status handling.The
handleErrorStatusfunction effectively consolidates status-based error handling into a single, maintainable location. The logic covers network errors, auth errors, validation errors, and server errors appropriately.frontend/src/stores/__tests__/errorStore.test.ts (1)
3-3: LGTM! Import path updated to use alias.The migration from relative import to
$stores/errorStorealigns with the project's path aliasing strategy.frontend/src/stores/__tests__/notificationStore.test.ts (1)
46-458: LGTM! Import paths updated to use alias.All dynamic imports have been migrated from relative paths to
$stores/notificationStore, aligning with the project's path aliasing strategy. Test logic and assertions remain unchanged.frontend/src/components/__tests__/Spinner.test.ts (1)
3-3: LGTM! Import path updated to use alias.The migration to
$components/Spinner.sveltealigns with the project's path aliasing strategy.frontend/src/routes/Login.svelte (1)
3-9: LGTM! Import paths updated to use aliases.All imports have been migrated to use path aliases (
$stores,$components,$utils,$lib), aligning with the project's standardized import strategy. Component logic remains unchanged.frontend/tsconfig.json (1)
19-30: LGTM! Comprehensive path alias configuration.The expanded path mappings provide clean module resolution across the frontend codebase. This configuration properly supports both direct and wildcard imports for all major module categories.
frontend/src/components/admin/users/index.ts (1)
1-5: LGTM! Clean barrel export for admin user components.This consolidates the public API surface for admin user UI components, making imports cleaner and more maintainable. The use of path aliases is consistent with the project's import strategy.
frontend/vitest.config.ts (1)
32-36: LGTM!The new path aliases are correctly configured and follow the same pattern as the existing
$libalias. This enables consistent alias-based imports across tests.frontend/src/routes/admin/AdminSettings.svelte (1)
7-10: LGTM!The import paths have been correctly migrated from relative paths to alias-based paths (
$lib,$stores,$routes,$components), consistent with the project-wide alias strategy.frontend/src/stores/__tests__/theme.test.ts (2)
5-7: LGTM!The mock is correctly renamed from
saveThemeSettingtosaveUserSettings, aligning with the unified user settings API change in the codebase.
42-42: LGTM!The dynamic imports throughout the test file correctly use the
$stores/themealias path, consistent with the project-wide alias migration.frontend/src/routes/admin/__tests__/AdminEvents.test.ts (2)
5-6: LGTM!Good use of centralized test utilities. The
mockElementAnimateandmockWindowGlobalshelpers are correctly imported from the shared test-utils module, reducing code duplication across admin tests.
125-129: LGTM!The mock AdminLayout and AdminEvents component imports correctly use alias-based paths, consistent with the project-wide migration.
frontend/src/components/admin/sagas/SagaStatsCards.svelte (2)
1-14: LGTM!The component is well-structured with proper TypeScript typing and Svelte 5 runes usage. The
getCounthelper is appropriately scoped for this component's needs.
16-35: LGTM!The template uses a clean responsive grid layout with proper dark mode support. The dynamic icon rendering via
{@const IconComponent = info.icon}is an appropriate pattern for this use case.frontend/src/components/admin/events/EventStatsCards.svelte (2)
1-10: LGTM!Props are correctly typed, and the distinction between
stats.total_events(24h events) and thetotalEventsprop (total count) is appropriate for the UI's display of "X of Y total" pattern.
12-45: LGTM!The component handles nullable data gracefully with optional chaining and appropriate fallbacks. The conditional styling for error rate (green for 0, red for > 0) provides good visual feedback.
frontend/src/lib/__mocks__/api-interceptors.ts (1)
7-7: LGTM!The re-export path correctly uses the
$libalias, consistent with the project-wide migration. The mock correctly separates mocked side-effect functions from real pure function re-exports.frontend/src/components/admin/events/EventFilters.svelte (2)
1-24: LGTM!The component correctly uses Svelte 5's
$bindablefor two-way binding of the filters object, and the default values provide a clean initial state. The past review concern about an unusedXimport has been addressed.
26-156: LGTM!The template is well-structured with:
- Proper label-input associations via
for/idattributes for accessibility- Responsive grid layouts for different screen sizes
- Helpful
titleattribute on the multi-select explaining Ctrl/Cmd usage- Consistent styling classes throughout
frontend/src/lib/__tests__/auth-init.test.ts (2)
58-72: LGTM! Clean migration from localStorage to sessionStorage.The mock implementation correctly handles
getItem,setItem, andremoveItemoperations with proper type annotations. The migration aligns with the broader PR goal of moving auth persistence to sessionStorage.
108-108: Import path alias update looks correct.The dynamic import path changed from
'../auth-init'to'$lib/auth-init', which aligns with the PR's goal of using alias-based imports consistently across the codebase.frontend/src/components/admin/events/EventDetailsModal.svelte (1)
1-17: Well-structured component with proper type safety.The Props interface is clearly defined, and the component properly uses Svelte 5's
$props()runes syntax. Imports use the alias paths consistently.frontend/src/components/admin/sagas/SagaDetailsModal.svelte (2)
17-22: Good UX pattern for modal navigation.The
handleViewExecutionfunction correctly closes the modal before invoking the callback, preventing potential visual glitches from having multiple modals open simultaneously.
88-136: Complex but well-structured execution steps visualization.The step visualization logic handles multiple states (completed, compensated, current, failed) with appropriate visual feedback. The use of
@constfor derived values within the loop is a clean pattern.One consideration: the hardcoded
min-w-[600px]on line 90 might cause horizontal scrolling issues on smaller admin panels. Theoverflow-x-autoon the parent handles this gracefully.frontend/src/routes/Settings.svelte (2)
8-16: Clean migration to centralized settings management.The imports properly use alias paths and introduce
setThemeandsetUserSettingsfrom the new centralized stores, replacing the previous scattered settings management approach.
103-106: Settings initialization and caching updated correctly.The
setUserSettings(settings)call properly updates the centralized store after loading from the API, and the fallback to'auto'for theme is sensible.frontend/src/stores/__tests__/auth.test.ts (2)
19-30: Consistent sessionStorage migration in auth store tests.The mock setup mirrors the pattern established in
auth-init.test.ts, ensuring consistent behavior across related test files.
62-80: Test description updated to reflect sessionStorage.The test "restores auth state from sessionStorage" correctly uses
sessionStorageData['authState']and imports from the aliased path, maintaining consistency with the implementation changes.frontend/src/lib/admin/users/rateLimits.ts (2)
7-15: Static analysis false positive - not an API key.The Gitleaks warning on line 13 is a false positive. The string
'bg-neutral-100 text-neutral-800 dark:bg-neutral-700 dark:text-neutral-200'is a CSS class string for styling, not an API key. Theapikey in the object refers to an endpoint group name.
66-77: Factory function provides sensible defaults.The
createEmptyRulefunction returns a completeRateLimitRulewith reasonable defaults, making it easy to create new rules in the UI.frontend/src/components/editor/OutputPanel.svelte (2)
16-29: Proper XSS prevention with ANSI rendering.The combination of
escapeXML: truein the ANSI converter and DOMPurify sanitization with restricted tags/attributes provides good defense-in-depth against XSS when rendering terminal output.
50-56: Phase label handling addresses past review feedback.The
phaseLabelderived value now explicitly handles 'starting' (line 51). The 'idle' phase is correctly excluded by the conditional on line 64 (phase !== 'idle'), sophaseLabelis never evaluated for idle state. The fallback "Executing..." serves as a catch-all for any future phases.frontend/src/components/editor/LanguageSelect.svelte (2)
35-85: Keyboard navigation implementation addresses accessibility feedback.The component now fully supports keyboard navigation with:
- Arrow keys for menu traversal
- Enter/Space for selection
- Escape to close menus progressively
- Proper focus management with indices
This addresses the accessibility concerns from the past review.
103-157: Well-structured accessible menu implementation.The two-tier menu uses proper ARIA roles (
menu,menuitem,menuitemradio) with correctaria-expanded,aria-checked, andtabindexmanagement. Theflytransitions provide smooth UX.frontend/src/components/NotificationCenter.svelte (1)
2-9: Clean migration to component-based icons and alias imports.The refactoring from inline SVG to Lucide icon components improves maintainability. The
type Componentimport from Svelte and the alias-based paths ($stores/,$lib/) are correctly applied.frontend/src/components/__tests__/NotificationCenter.test.ts (1)
526-580: Good coverage for desktop notification permission flow.The new tests comprehensively cover:
- Button visibility based on permission state (default/granted/denied)
- Request permission click behavior
- Browser notification creation on SSE events
This addresses the past review comment about missing permission flow tests.
frontend/src/routes/admin/AdminUsers.svelte (1)
248-256: Good refactoring to component-based UI.The extraction of filters to
UserFilterscomponent with proper two-way binding is clean. ThehasFiltersActivederived state is a nice touch for UX.frontend/src/routes/admin/AdminSagas.svelte (1)
193-196: Good UX improvement for client-side filtering clarity.The messaging "Showing X of Y on this page (filtered locally)" clearly communicates to users that client-side filtering is active, addressing the past review concern about pagination inconsistency.
frontend/src/routes/Editor.svelte (3)
41-69: Persistent store implementation is now correct.The refactored
createPersistentStoreproperly wrapssetandupdateto sync with localStorage on each mutation, rather than using a subscription. This addresses the past memory leak concern.
314-314: Reactive store access is now correct.The template uses
$script.trim()which creates a proper reactive subscription, addressing the past review about non-reactiveget(script)usage.
97-100: Good cleanup pattern for subscriptions.The unsubscribe functions are properly declared, assigned in
onMount, and called inonDestroy. This prevents memory leaks from dangling subscriptions.Also applies to: 156-161
frontend/src/lib/__tests__/user-settings.test.ts (2)
123-228: Comprehensive test coverage for saveUserSettings.The test suite thoroughly covers:
- Auth guard (returns false when not authenticated)
- Partial settings saves (theme, editor, combined)
- Store updates on success
- Theme application when theme is included
- Success/failure return values
- Network error handling
This aligns well with the unified settings flow implementation.
54-65: Good test structure with module reset pattern.Using
vi.resetModules()and dynamic imports ensures each test gets fresh module state. The mock setup is clean and the assertions verify the expected behavior.frontend/src/routes/admin/AdminEvents.svelte (8)
1-42: LGTM on imports refactoring.The migration to alias-based imports (
$lib,$stores,$components,$routes) improves maintainability and avoids fragile relative path navigation. The type aliasing (type EventFilters as EventFiltersType) cleanly resolves the naming conflict with the component.
44-76: Well-structured state management.Good use of Svelte 5 runes with
$statefor reactive values and$derivedfor computed properties liketotalPagesandskip. The organization with section comments aids readability.
78-90: LGTM on lifecycle management.Both intervals (
refreshIntervalandreplayCheckInterval) are properly cleaned up inonDestroy, preventing memory leaks. The 30-second auto-refresh is a reasonable polling interval for admin dashboards.
120-138: LGTM on replay status polling.Terminal states (
completed,failed,cancelled) correctly stop the polling interval, and appropriate user feedback is provided via toasts.
140-171: LGTM on replay functionality.Good null-safety with optional chaining on
response?.events_previewandresponse?.session_id. The dry-run preview flow and the real replay with status polling are well-structured.
173-193: LGTM on delete and export handlers.The delete function properly uses
Promise.allfor parallel data refresh. The export function correctly builds query parameters and useswindow.openfor file downloads, which is appropriate for triggering browser download dialogs.
195-204: LGTM on user overview loading.Good UX pattern: the modal opens immediately with a loading state, then either displays data or closes on failure. The early return guard for empty
userIdprevents unnecessary API calls.
206-232: LGTM on filter and handler functions.Clean separation of concerns with thin handler wrappers that provide appropriate interfaces for child components. The
handleReplayFromModalcorrectly closes the modal before initiating the replay action.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/routes/admin/__tests__/AdminEvents.test.ts (1)
234-234: Fix incorrect event type format.Line 234 uses
'execution.completed'(dot separator) while all other event types in this file and the EventType enum use underscore format ('execution_completed'). This inconsistency should be corrected.🔎 Proposed fix
- const events = [createMockEvent({ event_type: 'execution.completed', metadata: { user_id: 'user-1', service_name: 'test-service' } })]; + const events = [createMockEvent({ event_type: 'execution_completed', metadata: { user_id: 'user-1', service_name: 'test-service' } })];backend/app/services/notification_service.py (1)
1-1: Fix import sorting to resolve pipeline failure.The Ruff linter flagged an import sorting issue. Run
ruff --fixto automatically organize imports.
♻️ Duplicate comments (4)
frontend/src/components/editor/OutputPanel.svelte (2)
50-56: Add explicit handler foridlephase.The
phaseLabelderived value still lacks an explicit case for theidlephase. Whenphase === 'idle', it falls through to return'Executing...', which is semantically incorrect since no execution is in progress.🔎 Suggested fix
const phaseLabel = $derived.by(() => { + if (phase === 'idle') return 'Ready'; if (phase === 'starting') return 'Starting...'; if (phase === 'queued') return 'Queued...'; if (phase === 'scheduled') return 'Scheduled...'; if (phase === 'running') return 'Running...'; return 'Executing...'; });
147-150: Fix unit label typo:mshould bemsfor milliseconds.The CPU time values are in milliseconds, but the display shows
minstead ofms.🔎 Proposed fix
<span class="text-fg-default dark:text-dark-fg-default font-medium"> {result.resource_usage.cpu_time_jiffies === 0 - ? `< ${(1000 / clkTck).toFixed(0)} m` - : `${cpuMs.toFixed(3)} m`} + ? `< ${(1000 / clkTck).toFixed(0)} ms` + : `${cpuMs.toFixed(3)} ms`} </span>frontend/src/lib/editor/execution.svelte.ts (2)
82-85: Same issue:fetchFallbackrejection inonerrorleaves promise unsettled.The
onerrorhandler is now async and awaitsfetchFallback, but if it throws, the rejection escapes unhandled and the wrapping promise never settles.🔎 Proposed fix
- eventSource.onerror = async () => { - eventSource.close(); - await fetchFallback(); - }; + eventSource.onerror = () => { + eventSource.close(); + fetchFallback().catch(reject); + };
73-80:fetchFallbackrejection inonmessagemay leave the promise unsettled.While
fetchFallbackis now awaited (addressing the previous review), if it throws after closing the EventSource on lines 73-76, the rejection propagates to the asynconmessagehandler but isn't caught—leaving the outerPromise<ExecutionResult>pending indefinitely.🔎 Proposed fix
if (FAILURE_EVENTS.includes(eventType as EventType)) { eventSource.close(); - await fetchFallback(); + try { + await fetchFallback(); + } catch (e) { + reject(e); + } } } catch (err) { console.error('SSE parse error:', err); }
🧹 Nitpick comments (2)
backend/app/dlq/models.py (1)
118-118: Consider updatingDLQMessageFilter.event_typetoEventType | Nonefor consistency.While other
event_typefields in this file and related schemas have been migrated toEventType, this filter field remainsstr | None. If the filter is used to match againstDLQMessage.event_type(nowEventType), consider updating for type consistency.🔎 Proposed fix
@dataclass class DLQMessageFilter: """Filter criteria for querying DLQ messages.""" status: DLQMessageStatus | None = None topic: str | None = None - event_type: str | None = None + event_type: EventType | None = Nonebackend/app/domain/events/event_models.py (1)
97-105: Consider:EventSummary.event_typeremainsstrwhileEvent.event_typeis nowEventType.This asymmetry may be intentional for lightweight serialization, but worth confirming it aligns with the type consistency goals of the broader refactor.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
backend/app/api/routes/events.pybackend/app/api/routes/execution.pybackend/app/db/repositories/event_repository.pybackend/app/db/repositories/sse_repository.pybackend/app/dlq/models.pybackend/app/domain/events/event_models.pybackend/app/domain/sse/models.pybackend/app/domain/user/settings_models.pybackend/app/infrastructure/mappers/execution_api_mapper.pybackend/app/infrastructure/mappers/sse_mapper.pybackend/app/schemas_pydantic/dlq.pybackend/app/schemas_pydantic/execution.pybackend/app/schemas_pydantic/sse.pybackend/app/schemas_pydantic/user_settings.pybackend/app/services/event_service.pybackend/app/services/kafka_event_service.pybackend/app/services/notification_service.pybackend/app/services/sse/redis_bus.pybackend/app/services/sse/sse_service.pybackend/app/services/user_settings_service.pybackend/tests/unit/db/repositories/test_sse_repository.pybackend/tests/unit/infrastructure/mappers/test_execution_api_mapper.pybackend/tests/unit/services/sse/test_redis_bus.pybackend/tests/unit/services/sse/test_shutdown_manager.pybackend/tests/unit/services/sse/test_sse_service.pybackend/tests/unit/services/sse/test_sse_shutdown_manager.pydocs/reference/openapi.jsonfrontend/src/components/editor/OutputPanel.sveltefrontend/src/lib/admin/events/__tests__/eventTypes.test.tsfrontend/src/lib/admin/events/eventTypes.tsfrontend/src/lib/api/index.tsfrontend/src/lib/api/types.gen.tsfrontend/src/lib/editor/execution.svelte.tsfrontend/src/routes/admin/__tests__/AdminEvents.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/admin/events/eventTypes.ts
- frontend/src/lib/api/index.ts
🧰 Additional context used
🧬 Code graph analysis (20)
frontend/src/routes/admin/__tests__/AdminEvents.test.ts (2)
frontend/src/lib/admin/events/eventTypes.ts (1)
EVENT_TYPES(10-21)frontend/src/routes/admin/__tests__/test-utils.ts (2)
mockElementAnimate(34-36)mockWindowGlobals(41-44)
backend/app/domain/events/event_models.py (3)
frontend/src/lib/admin/events/eventTypes.ts (1)
EventType(7-7)frontend/src/lib/api/types.gen.ts (1)
EventType(1028-1028)backend/app/dlq/models.py (1)
event_type(93-95)
backend/app/services/user_settings_service.py (2)
backend/app/dlq/models.py (1)
event_type(93-95)backend/tests/unit/services/idempotency/test_middleware.py (1)
event(32-36)
backend/app/schemas_pydantic/dlq.py (4)
frontend/src/lib/admin/events/eventTypes.ts (1)
EventType(7-7)frontend/src/lib/api/index.ts (1)
EventType(4-4)frontend/src/lib/api/types.gen.ts (1)
EventType(1028-1028)backend/app/dlq/models.py (1)
event_type(93-95)
backend/app/domain/sse/models.py (2)
frontend/src/lib/api/index.ts (1)
ExecutionStatus(4-4)frontend/src/lib/api/types.gen.ts (1)
ExecutionStatus(1213-1213)
frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (1)
frontend/src/lib/admin/events/eventTypes.ts (8)
EventFilters(58-67)createDefaultEventFilters(69-80)EVENT_TYPES(10-21)getEventTypeColor(24-44)getEventTypeLabel(47-55)hasActiveFilters(82-93)getActiveFilterCount(95-106)getActiveFilterSummary(108-120)
backend/tests/unit/services/sse/test_shutdown_manager.py (3)
frontend/src/lib/editor/execution.svelte.ts (1)
phase(108-108)backend/app/services/sse/sse_shutdown_manager.py (1)
initiate_shutdown(119-146)backend/tests/unit/services/sse/test_sse_service.py (1)
get_shutdown_status(88-95)
backend/tests/unit/infrastructure/mappers/test_execution_api_mapper.py (2)
frontend/src/lib/api/index.ts (1)
ExecutionErrorType(4-4)frontend/src/lib/api/types.gen.ts (1)
ExecutionErrorType(1049-1049)
backend/tests/unit/services/sse/test_sse_service.py (2)
backend/app/schemas_pydantic/sse.py (2)
RedisNotificationMessage(102-112)RedisSSEMessage(63-68)backend/app/services/sse/redis_bus.py (1)
get(21-29)
backend/app/schemas_pydantic/sse.py (1)
backend/app/schemas_pydantic/execution.py (2)
ExecutionResult(97-110)ResourceUsage(57-67)
backend/tests/unit/services/sse/test_sse_shutdown_manager.py (2)
backend/tests/unit/services/sse/test_sse_service.py (1)
get_shutdown_status(88-95)frontend/src/lib/editor/execution.svelte.ts (1)
phase(108-108)
backend/app/services/sse/sse_service.py (2)
backend/app/schemas_pydantic/sse.py (4)
RedisNotificationMessage(102-112)RedisSSEMessage(63-68)SSEExecutionEventData(18-60)SSENotificationEventData(75-99)backend/app/services/sse/redis_bus.py (3)
open_subscription(62-66)get(21-29)open_notification_subscription(72-76)
backend/app/infrastructure/mappers/execution_api_mapper.py (1)
backend/app/domain/execution/models.py (1)
DomainExecution(13-26)
backend/app/services/kafka_event_service.py (4)
backend/app/dlq/models.py (1)
event_type(93-95)frontend/src/lib/admin/events/eventTypes.ts (1)
EventType(7-7)frontend/src/lib/api/types.gen.ts (1)
EventType(1028-1028)backend/app/infrastructure/kafka/mappings.py (1)
get_event_class_for_type(75-144)
backend/app/infrastructure/mappers/sse_mapper.py (1)
backend/app/domain/execution/models.py (2)
DomainExecution(13-26)ResourceUsageDomain(43-64)
backend/app/schemas_pydantic/user_settings.py (3)
backend/app/dlq/models.py (1)
event_type(93-95)frontend/src/lib/admin/events/eventTypes.ts (1)
EventType(7-7)frontend/src/lib/api/types.gen.ts (1)
EventType(1028-1028)
backend/tests/unit/services/sse/test_redis_bus.py (3)
backend/app/schemas_pydantic/sse.py (2)
RedisNotificationMessage(102-112)RedisSSEMessage(63-68)backend/app/services/sse/redis_bus.py (3)
SSERedisBus(38-76)get(21-29)publish_notification(68-70)backend/tests/unit/services/sse/test_sse_service.py (1)
get(24-33)
backend/app/services/notification_service.py (2)
backend/app/schemas_pydantic/sse.py (1)
RedisNotificationMessage(102-112)backend/app/services/sse/redis_bus.py (1)
publish_notification(68-70)
backend/app/domain/user/settings_models.py (2)
backend/app/dlq/models.py (1)
event_type(93-95)frontend/src/lib/api/types.gen.ts (1)
EventType(1028-1028)
backend/app/services/event_service.py (4)
frontend/src/lib/api/index.ts (2)
SortOrder(4-4)EventType(4-4)frontend/src/lib/api/types.gen.ts (2)
SortOrder(2602-2602)EventType(1028-1028)frontend/src/lib/admin/events/eventTypes.ts (1)
EventType(7-7)backend/app/domain/events/event_models.py (1)
Event(77-94)
🪛 GitHub Actions: Ruff Linting
backend/app/services/notification_service.py
[error] 1-1: Import block is un-sorted or un-formatted. Organize imports with 'ruff --fix'.
backend/app/schemas_pydantic/execution.py
[error] 1-1: Import block is un-sorted or un-formatted. Organize imports with 'ruff --fix'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (60)
backend/tests/unit/services/sse/test_sse_shutdown_manager.py (2)
34-34: LGTM: Improved type safety with attribute access.The migration from dictionary-based access to attribute access on
ShutdownStatusimproves type safety and code clarity.
45-45: LGTM: Consistent attribute access for shutdown status.The attribute-based access to
.completeis consistent with the earlier change on line 34 and improves type safety.frontend/src/routes/admin/__tests__/AdminEvents.test.ts (2)
5-6: AI summary inconsistency: Mock helpers not imported as claimed.The AI summary states that DEFAULT_EVENT, EVENT_TYPES, createMockEvent, and createMockEvents are "imported test-utils helpers," but lines 17-42 show they're still defined locally in this file rather than imported from
test-utils.
147-148: LGTM: Clean integration of test-utils functions.The usage of
mockElementAnimate()andmockWindowGlobals()from the centralized test-utils module is correct and demonstrates the benefits of modular test utilities.backend/app/schemas_pydantic/dlq.py (1)
7-7: LGTM! Consistent enum typing for event_type fields.The migration from
strtoEventTypealigns with the domain model (DLQMessage.event_typeproperty) and frontend type definitions. Pydantic 2.x will handle serialization correctly.Also applies to: 24-24, 86-86
backend/app/dlq/models.py (1)
92-95: LGTM! Property now returns strongly-typed EventType.The return type aligns with
BaseEvent.event_typebeing anEventTypeenum.backend/app/domain/sse/models.py (1)
6-7: LGTM! Status field now uses strongly-typed ExecutionStatus enum.This aligns with the broader migration to enum-based typing across execution-related domain models and is consistent with the SSERepository construction.
Also applies to: 33-37
backend/tests/unit/db/repositories/test_sse_repository.py (2)
9-16: LGTM! Test correctly validates enum-based status.The test inserts a document with string status
"running"and correctly asserts that the repository returnsExecutionStatus.RUNNING, validating the conversion logic inSSERepository.get_execution_status.
25-41: LGTM! Streamlined test coverage for simplified repository.The
test_get_executionandtest_get_execution_not_foundtests adequately cover the newget_executionmethod behavior.backend/tests/unit/infrastructure/mappers/test_execution_api_mapper.py (1)
1-226: LGTM! Test assertions updated to use ExecutionErrorType enum.Comprehensive test coverage for all
ExecutionErrorTypevariants (SCRIPT_ERROR, TIMEOUT, RESOURCE_LIMIT, SYSTEM_ERROR, PERMISSION_DENIED) with correct enum comparisons. The tests properly validate the mapper's direct pass-through behavior.backend/app/domain/user/settings_models.py (2)
21-29: LGTM! Editor theme default updated to "auto".The default theme change from
"one-dark"to"auto"aligns with the PR objectives for updated editor settings.
74-82: LGTM! History entry enhanced with EventType and tracking fields.The
event_typemigration toEventTypeis consistent with other domain models. The newreasonandcorrelation_idfields provide useful context for history/audit tracking.backend/app/db/repositories/sse_repository.py (2)
17-27: Simplified status retrieval with direct domain construction.The refactored method efficiently projects only the
statusfield and constructsSSEExecutionStatusDomaindirectly.Note:
ExecutionStatus(doc["status"])at line 25 will raiseValueErrorif the database contains an unexpected status string. If data integrity is guaranteed, this is fine. Otherwise, consider defensive handling:🔎 Optional defensive handling
+ try: + status = ExecutionStatus(doc["status"]) + except ValueError: + return None # or log and handle gracefully return SSEExecutionStatusDomain( execution_id=execution_id, - status=ExecutionStatus(doc["status"]), + status=status, timestamp=datetime.now(timezone.utc).isoformat(), )
29-33: LGTM! Streamlined execution retrieval.The method correctly delegates to the mapper for document-to-domain conversion with proper null handling.
backend/app/infrastructure/mappers/execution_api_mapper.py (1)
16-31: LGTM! Simplified mapper with direct pass-through.The null check for
resource_usageand direct pass-through oferror_typesimplify the logic correctly.ResourceUsageDomain.to_dict()exists and works as expected, andExecutionResult.error_typeacceptsExecutionErrorTypedirectly since bothDomainExecutionandExecutionResultuse the sameOptional[ExecutionErrorType]type.frontend/src/components/editor/OutputPanel.svelte (1)
138-166: Resource usage display implementation looks good.The CPU time conversion now correctly uses
clk_tck_hertzfrom the result data with a sensible fallback to 100 Hz. This addresses the previous concern about hardcoded jiffy conversion values.frontend/src/lib/editor/execution.svelte.ts (1)
107-115: Clean public API surface for execution state.The returned object provides a well-encapsulated interface with getters for reactive state (
phase,result,error,isExecuting) and methods (execute,reset). This is a good pattern for Svelte 5's runes-based reactivity.backend/app/services/user_settings_service.py (1)
248-276: Type alignment forevent_typefield looks correct.The change from
str(event.event_type)toevent.event_typealigns with the updatedDomainSettingsHistoryEntry.event_typefield type (nowEventTypeinstead ofstr). This maintains type consistency across the domain layer.backend/app/api/routes/events.py (1)
327-333: Verify: Removingreplay.prefix may impact downstream event consumers.The event type now publishes
event.event_typedirectly instead of the previousf"replay.{event.event_type}". Whilereplay_correlation_idstill identifies these as replayed events, consumers that filter or handle events by type may not distinguish replays from originals.If this is intentional (replayed events should be indistinguishable), consider documenting this behavior. If not, the prefix can be preserved while maintaining type safety:
await kafka_event_service.publish_event( - event_type=event.event_type, + event_type=f"replay.{event.event_type}", payload=event.payload,Note: This would require ensuring the Kafka service accepts string event types for replay scenarios, or defining dedicated
REPLAY_*EventType variants.backend/app/domain/events/event_models.py (1)
76-91: Event domain model correctly typed withEventType.The
Event.event_typefield is now properly typed asEventTypeenum, aligning with the broader migration toward strongly-typed event handling across the codebase.backend/tests/unit/services/sse/test_shutdown_manager.py (2)
29-31: Test correctly updated to use attribute access onShutdownStatus.The change from dict-style access (
status["phase"]) to attribute access (status.phase) aligns with theShutdownStatusbeing a typed object rather than a dictionary.
62-69: Phase transition test correctly validates typed status object.The assertions properly check
st0.phaseandst1.phaseas attributes, consistent with theShutdownStatusmodel.frontend/src/lib/admin/events/__tests__/eventTypes.test.ts (3)
13-14: Nice helper function for creating test fixtures.The
withFilterhelper provides a clean way to create filter objects with specific overrides while maintaining default values. Good pattern for test readability.
28-46: Thorough parameterized tests for color mapping.The
getEventTypeColortests cover all color categories and verify both the color class and dark mode variant presence. Good use ofit.eachfor maintainable test cases.
112-136: Comprehensive filter summary tests with edge cases.The
getActiveFilterSummarytests cover empty filters, individual active filters (including singular/plural event type labels), and combinations. This ensures the summary generation logic handles all scenarios correctly.backend/tests/unit/services/sse/test_redis_bus.py (2)
80-86: Tests correctly updated to use typedRedisSSEMessagemodel.The subscription
.get()method now properly receives a model type and returns a typed object. Assertions correctly accessmsg.event_typeandmsg.execution_idas attributes.
100-116: Notification test properly validates typedRedisNotificationMessageflow.The test constructs a typed
RedisNotificationMessage, publishes it, and verifies the subscription returns the correctly typed object with expected attributes. Good coverage of the typed message round-trip.backend/tests/unit/services/sse/test_sse_service.py (2)
24-33: LGTM: Typed subscription validation enhances test coverage.The updated
getmethod now validates messages against Pydantic models, which better mirrors production behavior and catches schema violations during testing.
193-202: Well-structured notification payload for schema validation.The notification payload correctly matches the
RedisNotificationMessageschema fields, ensuring the test validates against the actual message structure used in production.backend/app/services/notification_service.py (2)
794-804: Excellent migration to typed SSE messages.The notification service now publishes strongly-typed
RedisNotificationMessageobjects instead of raw dictionaries, improving type safety and validation across the SSE delivery pipeline.
806-828: Improved skip reason handling with explicit return type.The updated signature
-> str | Nonemakes the skip logic clearer by returning the skip reason directly, rather than relying on side effects or logging alone.backend/app/schemas_pydantic/user_settings.py (2)
24-24: Sensible default theme change to "auto".Changing the default editor theme from
"one-dark"to"auto"enables automatic theme detection, improving the user experience by respecting system preferences.
94-94: Stronger typing with EventType enum.Replacing
strwithEventTypefor event_type fields eliminates string typos and enables better IDE support and validation.Also applies to: 119-119
backend/app/db/repositories/event_repository.py (1)
118-122: Proper EventType integration with MongoDB filtering.The updated signature accepts
list[EventType]and correctly converts to enum values via.valuefor MongoDB's$inoperator. This maintains type safety at the API boundary while ensuring database compatibility.backend/app/api/routes/execution.py (3)
54-54: Cleaner conversion with model_validate.Using
ExecutionInDB.model_validate(domain_exec)is more idiomatic than manual field mapping and leverages Pydantic's validation and conversion logic.
240-247: EventType parameter enables type-safe event filtering.The route now accepts
list[EventType]for event filtering, providing IDE autocomplete and preventing invalid event type strings. The response construction viamodel_validateensures consistency.
298-298: Consistent model_validate usage for resource limits.Direct model validation maintains the pattern established elsewhere in the routes and simplifies the conversion logic.
backend/app/services/event_service.py (2)
2-2: Modern Python typing with built-in generics.Replacing
typing.Listwith built-inlist[...]syntax aligns with PEP 585 (Python 3.9+) and modern typing conventions, improving readability and reducing imports.Also applies to: 37-37, 59-59, 121-121, 160-160, 176-176
198-198: EventType parameter enables type-safe aggregate queries.The updated signature accepts
list[EventType]for event filtering, ensuring type safety when querying events by aggregate ID.backend/app/schemas_pydantic/sse.py (4)
12-12: Well-defined control event types using Literal.Using
Literaltypes for SSE control events ('connected','heartbeat', etc.) provides compile-time type checking and clear documentation of valid event types.Also applies to: 72-72
18-61: Comprehensive SSE execution event payload model.
SSEExecutionEventDataprovides a unified, well-documented model for all execution-related SSE events. The optional fields accommodate different event types while maintaining type safety.
63-68: Redis message wrapper for typed SSE delivery.
RedisSSEMessageprovides a clean wrapper for publishing execution events to Redis, separating the event type and execution ID from the full event payload.
75-100: Well-structured notification SSE schemas.The separation between
SSENotificationEventData(client-facing payload) andRedisNotificationMessage(Redis transport) provides clear boundaries for notification delivery through SSE.Also applies to: 102-112
backend/app/services/kafka_event_service.py (2)
30-37: No changes needed.EventTypeextendsStringEnum, which is based on Python'sStrEnum. InStrEnum, enum members are string instances themselves (not wrapped values), so assigningevent_typedirectly toheadersis correct and type-safe. Thedict[str, str]type annotation acceptsStrEnummembers without requiring.value.Likely an incorrect or invalid review comment.
84-91: No changes needed — EventType usage is correct.EventType extends StringEnum, which is explicitly designed to behave like a plain string in all representations. Passing the enum value directly (without
.value) is the correct approach for both Pydantic models and string-typed dictionaries. The concrete event classes confirm this pattern by usingevent_type: Literal[EventType.XXX] = EventType.XXXwithout conversions.backend/app/services/sse/sse_service.py (3)
54-59: Connected / error / status events look consistent with typed SSE payloadsThe handshake (
connected), earlyerroron shutdown, and initialstatusevents are now all going throughSSEExecutionEventDataand_format_sse_event, which keeps the SSE payload shape consistent. No issues spotted here.Also applies to: 66-71, 80-85
148-164: Result enrichment onRESULT_STOREDlooks correctFetching the execution and validating it into
ExecutionResultonly forEventType.RESULT_STORED, then mergingmsg.datawith explicitevent_type/execution_id/type/resultis sane and matches the new typed schema usage. Just be aware that ifmsg.dataalready contains those keys, they will be overwritten by the explicit values (which is likely what you want).
169-209: Notification stream flow and formatting are consistent with the execution SSE pathThe notification stream now uses
RedisNotificationMessage→SSENotificationEventData→_format_notification_event, with connected/heartbeat/notification events carrying the expected fields. This mirrors the execution SSE shape and should make the frontend consumption uniform.Also applies to: 233-235
docs/reference/openapi.json (1)
369-382: OpenAPI updates correctly mirror new typed backend models; watch for client compatibilityThe spec changes (wiring
event_typesparams and various event/notification/ DLQ fields toEventType, introducingExecutionErrorType,LanguageInfo, health (LivenessResponse/ReadinessResponse/ShutdownStatusResponse), and the rate‑limit/user‑admin response models) all look internally consistent and match the backend schema changes. The main follow‑up is ensuring any external clients/frontends that previously treated these fields as free‑form strings or simple arrays are updated to handle the new enum values and nested shapes.Also applies to: 1165-1189, 2638-2760, 3145-3146, 3284-3285, 3335-3336, 4950-4975, 5012-5039, 6015-6026, 6033-6035, 6183-6247, 7718-7720, 7927-7928, 8384-8388, 8415-8465, 8786-8884
backend/app/services/sse/redis_bus.py (2)
14-21: Typed subscription wrapper is appropriate for the new Pydantic SSE messagesWrapping the Redis
PubSubwithSSERedisSubscription.get(model, timeout)andclose()(unsubscribe +aclose()) is a clean abstraction over the raw pubsub API and lines up with howSSEServicenow consumesRedisSSEMessage/RedisNotificationMessage.Also applies to: 31-36
54-61: Event/notification publishing now correctly reuses the typed Redis message schemasBuilding a
RedisSSEMessagefromBaseEventand publishing its JSON, and similarly publishingRedisNotificationMessageJSON for notifications, keeps serialization centralized in the Pydantic models and matches how the subscribers now parse messages. No functional issues spotted.Also applies to: 68-71
backend/app/schemas_pydantic/execution.py (5)
32-44: Execution models now align withExecutionErrorTypeand attribute-based validation*Switching
error_typeinExecutionInDBandExecutionResulttoExecutionErrorType | Noneand enablingfrom_attributes=Trueis consistent with the domain enums and supports validating from ORM/domain objects. This looks correct assuming the stored values are the same enum string values.Also applies to: 97-111
57-68:ResourceUsage,LanguageInfo, andResourceLimits.supported_runtimeswiring looks coherentAdding
model_config = ConfigDict(from_attributes=True)toResourceUsageandLanguageInfo, and changingResourceLimits.supported_runtimestodict[str, LanguageInfo]matches the new runtime metadata structure and the OpenAPI spec. No functional issues apparent; just ensure settings and any persistence layer storesupported_runtimesin the matching shape.Also applies to: 113-120, 122-133
77-85: Runtime validation now leverages structured runtimes; behavior change is acceptableUsing
SUPPORTED_RUNTIMESwithlang_info.versionstightensExecutionRequestvalidation and should give clearer errors when a language or version isn't allowed. This is good, but note it's a behavioral change: previously accepted languages/versions that are no longer present inSUPPORTED_RUNTIMESwill now raiseValueErrorand surface as 422s. That seems intentional; just verify the settings are populated correctly in all envs.
155-163: BindingExecutionEventResponse.event_typetoEventTypeand enablingfrom_attributesis consistentTyping
event_typeasEventTypeand addingfrom_attributeswill help keep execution events in sync with the central enum and simplify mapping from domain objects. This matches the OpenAPI updates and looks correct.
9-13: Fix import ordering to satisfy Ruff lintingThe imports at lines 9-11 are not in alphabetical order. Reorder them as follows:
-from app.domain.enums.storage import ExecutionErrorType -from app.domain.enums.events import EventType -from app.domain.enums.execution import ExecutionStatus +from app.domain.enums.events import EventType +from app.domain.enums.execution import ExecutionStatus +from app.domain.enums.storage import ExecutionErrorTypefrontend/src/lib/api/types.gen.ts (4)
2074-2101: No action required: supported_runtimes structure change has been properly implemented throughout the codebase.The type change from
{ [key: string]: Array<string> }to{ [key: string]: LanguageInfo }(line 2099) is confirmed. However, all usages in Editor.svelte correctly access the new structure:
- Accessing
.versionswhen needing the version array (lines 141, 145, 267)- Accessing
.file_extwhen needing the file extension (lines 232, 249, 255)- Backend models already use the correct
dict[str, LanguageInfo]structureNo broken array-like access patterns exist in the codebase.
173-173: EventType is a discriminated union type, not an enum—the type safety improvement is compatible with existing code.The
event_typefields at lines 173, 243, 1061, and 2514 have been changed fromstringtoEventType. EventType is defined as a union of 53 specific string literal values (line 1028), making this a type safety improvement rather than a breaking change. The consuming code uses valid EventType values ('execution_completed', 'pod_created', 'execution_started', etc.), and no direct string assignments toevent_typefields were found in the codebase. The defensive fallback pattern inexecution.svelte.ts:59remains compatible with the stricter typing.
3263-3284: Theevent_typesquery parameter already expectsArray<EventType> | nulland is properly handled by the API client's built-in array serialization. No evidence suggests this is a breaking change—consuming code throughout the codebase (e.g.,FAILURE_EVENTSinexecution.svelte.ts,EventFiltersineventTypes.ts) already passes arrays to this parameter. The client'sserializeArrayParamutility correctly handles array serialization.Likely an incorrect or invalid review comment.
3792-3823: Type changes for health and user management endpoints are correctly implemented.The upgrade from generic response types to concrete types (LivenessResponse, ReadinessResponse, DeleteUserResponse, UserRateLimitsResponse, RateLimitUpdateResponse) is properly integrated throughout the SDK and consuming code. Test mocks match the expected response shapes, and all property accesses align with the concrete type definitions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
frontend/src/lib/editor/execution.svelte.ts (3)
63-65: Type assertion without validation (duplicate concern).Similar to line 37, casting
eventData.status as ExecutionPhasewithout validation could assign an invalid phase if the backend sends an unexpected status string.
82-85: Critical: Unhandled rejection in onerror handler.If
fetchFallback()throws on line 84, the async arrow function will have an unhandled promise rejection. The outer promise (created on line 39) will never be rejected, leaving the execution in a pending state indefinitely.🔎 Proposed fix
eventSource.onerror = async () => { eventSource.close(); - await fetchFallback(); + try { + await fetchFallback(); + } catch (e) { + reject(e); + } };
73-79: Critical: Errors from fetchFallback are caught and logged but don't reject the promise.When a failure event occurs,
fetchFallback()is awaited (line 75), but if it throws, the error is caught by the try-catch on line 77 and only logged to console. The outer promise (created on line 39) is never rejected or resolved, leaving the execution hanging indefinitely.The past review comment was marked as addressed, but the error handling is still incomplete.
🔎 Proposed fix
try { const eventData = JSON.parse(event.data); const eventType = eventData?.event_type || eventData?.type; if (eventType === 'heartbeat' || eventType === 'connected') return; if (eventData.status) { phase = eventData.status as ExecutionPhase; } if (eventType === 'result_stored' && eventData.result) { eventSource.close(); resolve(eventData.result); return; } if (FAILURE_EVENTS.has(eventType)) { eventSource.close(); - await fetchFallback(); + try { + await fetchFallback(); + } catch (e) { + reject(e); + } + return; } } catch (err) { - console.error('SSE parse error:', err); + // Only log parse errors, not fetchFallback errors + if (err instanceof SyntaxError) { + console.error('SSE parse error:', err); + } else { + throw err; + } }backend/app/services/sse/sse_service.py (1)
146-156: Exception swallowing makes debugging malformed messages difficult.The exception handler on lines 154-156 silently drops malformed Redis messages without logging. This was previously flagged and remains unaddressed, making it difficult to diagnose schema drift or publisher issues.
🔎 Add logging before continuing
try: sse_event = await self._build_sse_event_from_redis(execution_id, msg) yield self._format_sse_event(sse_event) # End on terminal event types if msg.event_type in self.TERMINAL_EVENT_TYPES: logger.info(f"Terminal event for execution {execution_id}: {msg.event_type}") break - except Exception: - # Ignore malformed messages + except Exception: + logger.exception( + "Failed to build SSE event from Redis message for execution %s", + execution_id, + extra={"event_type": str(msg.event_type), "execution_id": execution_id}, + ) continueBased on past review feedback that has not been addressed.
backend/app/services/sse/redis_bus.py (1)
21-29: Deserialization errors silently swallowed.The
get()method (lines 26-29) catches all exceptions duringmodel_validate_json()and returnsNonewithout logging. This was flagged in a previous review and makes debugging schema drift or malformed messages difficult.🔎 Add minimal logging
try: return model.model_validate_json(msg["data"]) - except Exception: + except Exception: + logger.warning( + "Failed to parse Redis message as %s on channel %s", + model.__name__, + self._channel, + exc_info=True, + ) return NoneBased on past review feedback that has not been addressed.
🧹 Nitpick comments (5)
frontend/src/lib/admin/sagas/sagaStates.ts (1)
85-89: Consider deriving step count from EXECUTION_SAGA_STEPS.The hardcoded
totalSteps = 5forexecution_sagacreates a maintenance risk if steps are added or removed fromEXECUTION_SAGA_STEPS. Similarly, the magic number3for other sagas lacks a clear source of truth.🔎 Suggested refactor to derive step counts
export function getSagaProgressPercentage(completedSteps: string[], sagaName: string): number { if (!completedSteps?.length) return 0; - const totalSteps = sagaName === 'execution_saga' ? 5 : 3; + const totalSteps = sagaName === 'execution_saga' ? EXECUTION_SAGA_STEPS.length : 3; return Math.min(100, (completedSteps.length / totalSteps) * 100); }If other saga types also have defined steps, consider creating a mapping of saga names to their step arrays, then derive all step counts dynamically.
backend/app/schemas_pydantic/sse.py (3)
18-61: Consider discriminated unions for stronger type safety.The
SSEExecutionEventDatamodel uses a flexible approach with many optional fields and a union type forevent_type. While functional, this design makes it difficult to enforce type-specific field requirements at the schema level.The duplicate
typefield (line 37-39) marked as "legacy" adds maintenance overhead.💡 Alternative approach using discriminated unions
Consider splitting into specific event types with a discriminator:
class SSEControlEvent(BaseModel): event_type: SSEControlEventType execution_id: str timestamp: str # control-specific fields class SSEBusinessEvent(BaseModel): event_type: EventType execution_id: str timestamp: str # business-specific fields SSEExecutionEventData = SSEControlEvent | SSEBusinessEventThis would provide compile-time type safety and eliminate the need for the legacy
typefield.
75-100: Consistent notification event structure.The
SSENotificationEventDatafollows the same flexible pattern asSSEExecutionEventData, using optional fields to accommodate different event types (connected, heartbeat, notification).Similar to the execution events, discriminated unions could provide stronger type safety if needed in the future.
102-113: Well-structured notification message with minor suggestion.The
RedisNotificationMessagemodel is well-typed with appropriate use of enums for severity and status. All critical fields are required.Consider using
Noneinstead of""as the default foraction_url(line 111) to better distinguish between "no URL provided" vs "empty string URL".🔎 Suggested adjustment
- action_url: str = Field(default="", description="Optional action URL") + action_url: str | None = Field(default=None, description="Optional action URL")backend/app/services/sse/sse_service.py (1)
158-174: Add error handling for validation failures.The
_build_sse_event_from_redismethod performs validation (line 164) and dict spreading (line 167) without handling potential failures. Ifmodel_validateormodel_validateon line 166 fails, the exception propagates to the caller's exception handler (which currently swallows it silently).Consider adding specific error handling or validation checks here to provide better diagnostics.
🔎 Suggested error handling
async def _build_sse_event_from_redis(self, execution_id: str, msg: RedisSSEMessage) -> SSEExecutionEventData: """Build typed SSE event from Redis message.""" result: ExecutionResult | None = None if msg.event_type == EventType.RESULT_STORED: - exec_domain = await self.repository.get_execution(execution_id) - if exec_domain: + try: + exec_domain = await self.repository.get_execution(execution_id) + if exec_domain: - result = ExecutionResult.model_validate(exec_domain) + result = ExecutionResult.model_validate(exec_domain) + except Exception: + logger.warning( + "Failed to fetch or validate execution result for %s", + execution_id, + ) - return SSEExecutionEventData.model_validate( - { - **msg.data, - "event_type": msg.event_type, - "execution_id": execution_id, - "type": msg.event_type, - "result": result, - } - ) + try: + return SSEExecutionEventData.model_validate( + { + **msg.data, + "event_type": msg.event_type, + "execution_id": execution_id, + "type": msg.event_type, + "result": result, + } + ) + except Exception: + logger.exception("Failed to validate SSE event data for %s", execution_id) + raise
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/app/db/repositories/sse_repository.pybackend/app/schemas_pydantic/execution.pybackend/app/schemas_pydantic/sse.pybackend/app/services/notification_service.pybackend/app/services/sse/redis_bus.pybackend/app/services/sse/sse_service.pybackend/tests/integration/notifications/test_notification_sse.pybackend/tests/integration/services/sse/test_partitioned_event_router_integration.pybackend/tests/integration/test_sse_routes.pyfrontend/src/lib/admin/sagas/sagaStates.tsfrontend/src/lib/admin/users/__tests__/rateLimits.test.tsfrontend/src/lib/admin/users/rateLimits.tsfrontend/src/lib/editor/execution.svelte.tsfrontend/src/stores/userSettings.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/lib/admin/users/tests/rateLimits.test.ts
- backend/app/db/repositories/sse_repository.py
- frontend/src/stores/userSettings.ts
🧰 Additional context used
🧬 Code graph analysis (8)
backend/tests/integration/services/sse/test_partitioned_event_router_integration.py (2)
backend/app/schemas_pydantic/sse.py (1)
RedisSSEMessage(63-68)backend/app/services/sse/redis_bus.py (1)
get(21-29)
backend/app/services/notification_service.py (2)
backend/app/schemas_pydantic/sse.py (1)
RedisNotificationMessage(102-112)backend/app/services/sse/redis_bus.py (1)
publish_notification(68-70)
backend/app/services/sse/redis_bus.py (2)
backend/app/infrastructure/kafka/events/base.py (1)
BaseEvent(13-37)backend/app/schemas_pydantic/sse.py (2)
RedisNotificationMessage(102-112)RedisSSEMessage(63-68)
backend/tests/integration/notifications/test_notification_sse.py (2)
backend/app/schemas_pydantic/sse.py (1)
RedisNotificationMessage(102-112)backend/app/services/sse/redis_bus.py (1)
get(21-29)
backend/app/schemas_pydantic/sse.py (1)
backend/app/schemas_pydantic/execution.py (2)
ExecutionResult(98-111)ResourceUsage(57-67)
frontend/src/lib/editor/execution.svelte.ts (4)
frontend/src/lib/editor/index.ts (3)
ExecutionPhase(2-2)createExecutionState(2-2)ExecutionState(2-2)frontend/src/lib/api/index.ts (4)
EventType(4-4)ExecutionResult(4-4)createExecutionApiV1ExecutePost(3-3)getResultApiV1ResultExecutionIdGet(3-3)frontend/src/lib/api/types.gen.ts (2)
EventType(1028-1028)ExecutionResult(1178-1206)frontend/src/lib/api-interceptors.ts (1)
getErrorMessage(32-48)
backend/tests/integration/test_sse_routes.py (2)
backend/app/schemas_pydantic/sse.py (1)
RedisNotificationMessage(102-112)backend/app/services/sse/redis_bus.py (1)
publish_notification(68-70)
backend/app/services/sse/sse_service.py (3)
backend/app/schemas_pydantic/execution.py (1)
ExecutionResult(98-111)backend/app/schemas_pydantic/sse.py (2)
RedisNotificationMessage(102-112)RedisSSEMessage(63-68)backend/app/services/sse/redis_bus.py (1)
get(21-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (28)
frontend/src/lib/admin/sagas/sagaStates.ts (3)
1-12: LGTM! Clean interface definition.The imports and
SagaStateConfiginterface are well-structured. Usingtypeof CheckCircleas the icon type is a reasonable way to type-hint lucide icon components, since they all share the same component signature.
14-68: Excellent state configuration and defensive fallback handling.The
SAGA_STATESmapping provides comprehensive coverage of all saga lifecycle states with semantically appropriate colors and icons. ThegetSagaStateInfofunction correctly uses the spread operator to avoid mutatingDEFAULT_STATEwhen handling unknown states—good defensive programming.
70-83: Well-designed saga step definitions following the saga pattern.The
EXECUTION_SAGA_STEPScorrectly implements the saga pattern with compensating transactions for steps that have side effects (resource allocation, queueing, pod creation), while validation and monitoring appropriately have no compensations.frontend/src/lib/admin/users/rateLimits.ts (1)
55-63: Multiplier fallback logic has been addressed.The previous concern about the multiplier fallback has been resolved. The current implementation explicitly validates that only positive multipliers are accepted, with
0and negative values falling back to1. This is a reasonable design choice if the business logic requires strictly positive multipliers.Note: This differs from the previous suggestion to use nullish coalescing (
??), but achieves the intended goal of handling invalid multipliers appropriately.frontend/src/lib/editor/execution.svelte.ts (4)
1-12: LGTM: Imports and type definitions are well-structured.The imports are appropriate, and using
Set<EventType>forFAILURE_EVENTSprovides type-safe, efficient membership testing.
14-23: LGTM: State initialization and reset logic are clean.The use of Svelte 5 runes and the straightforward reset implementation are appropriate.
88-104: Good error handling with fallback attempt.The try-catch-finally structure is well-designed. The fallback fetch in the catch block (lines 91-100) provides resilience, and the silent catch on line 100 is acceptable since an error is already set. The finally block ensures proper cleanup.
107-117: LGTM: Clean API surface with proper encapsulation.The use of getters for reactive state, the computed
isExecutingproperty, and theReturnTypetype alias are all excellent TypeScript and Svelte 5 patterns.backend/app/schemas_pydantic/sse.py (2)
2-16: LGTM! Strong typing foundation established.The imports and type definitions properly support the typed SSE payload models. The use of
Literalfor control event types and the boundedTypeVarfor generic message parsing are good patterns.
63-69: LGTM! Clean Redis message wrapper.The
RedisSSEMessagemodel provides a clear structure for Redis pub/sub messages. The separation between Redis wire format and SSE client format is well-designed.Note:
execution_idis optional (line 67), which allows for flexibility but requires consumers to handleNonecases.backend/app/services/sse/sse_service.py (4)
11-31: LGTM! Typed model imports properly integrated.The imports of typed SSE models (
SSEExecutionEventData,SSENotificationEventData,RedisSSEMessage,RedisNotificationMessage) support the refactoring toward strongly-typed event handling throughout the service.
49-106: LGTM! Consistent typed event construction.The stream creation logic properly constructs typed
SSEExecutionEventDatainstances for error, connected, and status events. The use of explicit field names and ISO 8601 timestamps is good practice.
176-230: LGTM! Well-structured notification stream with typed messages.The notification stream properly uses typed models throughout:
SSENotificationEventDatafor control events (connected, heartbeat)RedisNotificationMessagefor Redis subscription- Clean mapping between Redis messages and SSE events
245-251: LGTM! Clean JSON serialization for SSE.The formatting methods properly use
model_dump_json(exclude_none=True)to serialize typed events, reducing payload size by omitting null fields.backend/app/schemas_pydantic/execution.py (7)
9-55: LGTM! Improved type specificity and ORM compatibility.The changes enhance type safety:
ExecutionErrorTypeis more specific than the genericErrorTypeConfigDict(from_attributes=True)enables population from ORM objects- Consistent application across
ExecutionInDBandExecutionUpdate
57-68: LGTM! ORM compatibility added.Adding
ConfigDict(from_attributes=True)enables direct population from ORM models, consistent with other schema updates.
77-87: LGTM! Runtime validation properly refactored.The validation logic now uses structured
LanguageInfowithversionslist instead of plain lists. The walrus operator on line 80 provides clean existence checking.The line-length issue from the previous review has been addressed by splitting the error message across lines 83-85.
98-112: LGTM! Consistent type and config updates.The
ExecutionResultmodel receives the same improvements as other execution schemas:ExecutionErrorTypefor specificity andfrom_attributes=Truefor ORM compatibility.
114-121: LGTM! Well-designed language metadata model.The new
LanguageInfomodel provides structured runtime information withversionsandfile_extfields. This is a significant improvement over the previousdict[str, list[str]]approach, offering better type safety and extensibility.
123-134: LGTM! Enhanced runtime metadata structure.The
ResourceLimitsmodel now usesdict[str, LanguageInfo]forsupported_runtimes, providing richer language metadata compared to the previous simple list-based approach.
155-164: LGTM! Stronger typing with EventType enum.Changing
event_typefromstrtoEventType(line 159) provides compile-time type safety and eliminates magic strings. This aligns with the broader typed-events refactoring across the codebase.backend/tests/integration/services/sse/test_partitioned_event_router_integration.py (1)
11-51: LGTM! Test properly updated for typed Redis messages.The integration test now uses
RedisSSEMessagefor type-safe message retrieval (line 46) and properly handles enum comparisons withstr()conversion (line 51). This aligns with the typed message infrastructure introduced in the codebase.backend/tests/integration/test_sse_routes.py (1)
9-87: LGTM! Test properly uses typed notification messages.The test now constructs a fully-typed
RedisNotificationMessage(lines 77-86) instead of a dict, with proper enum values forseverityandstatus. This ensures the test validates the actual message structure used in production.backend/tests/integration/notifications/test_notification_sse.py (1)
7-47: LGTM! Test refactored for typed message consumption.The test helper function now returns
RedisNotificationMessage(line 38) and uses typedget()(line 39), with clean attribute access (lines 45-47) replacing dict key access. This validates the typed message contract.backend/app/services/sse/redis_bus.py (2)
54-60: LGTM! Clean typed message publication.The
publish_event()method now constructs a typedRedisSSEMessage(lines 55-59) and serializes it withmodel_dump_json()(line 60), ensuring consistent message structure across the Redis bus.
68-70: LGTM! Type-safe notification publishing.The
publish_notification()method now accepts a typedRedisNotificationMessageparameter instead of a generic mapping, providing better type safety and making the API contract explicit.backend/app/services/notification_service.py (2)
792-804: LGTM! Type-safe SSE notification publishing.The
_publish_notification_sse()method now constructs a fully-typedRedisNotificationMessage(lines 794-803) with proper field mapping from the domain model. The use oflist()for tags andisoformat()for timestamps ensures correct data types.
806-828: LGTM! Enhanced skip detection with reason reporting.The
_should_skip_notification()method now returnsstr | None(line 808), providing the skip reason when a notification should be skipped. This improves observability and allows callers to log specific reasons for skipped notifications (used at line 854).
There was a problem hiding this comment.
Actionable comments posted: 0
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/admin/__tests__/AdminEvents.test.ts (1)
163-163: Fix event_type format inconsistency.Line 163 uses
'execution.completed'(dot notation) while all other event_type values in this file use snake_case format ('execution_completed'). This inconsistency should be corrected to match the format used throughout the codebase and in the centralized test utilities.🔎 Proposed fix
- const events = [createMockEvent({ event_type: 'execution.completed', metadata: { user_id: 'user-1', service_name: 'test-service' } })]; + const events = [createMockEvent({ event_type: 'execution_completed', metadata: { user_id: 'user-1', service_name: 'test-service' } })];
♻️ Duplicate comments (1)
backend/app/services/sse/sse_service.py (1)
146-159: Enhance exception logging with full context.The current exception handling logs a warning but doesn't capture the full stack trace or the complete message payload. The past review comment requested using
logger.exceptionand logging the raw Redis message details. This would significantly improve debugging when schema mismatches or validation errors occur.🔎 Recommended enhancement
try: sse_event = await self._build_sse_event_from_redis(execution_id, msg) yield self._format_sse_event(sse_event) # End on terminal event types if msg.event_type in self.TERMINAL_EVENT_TYPES: logger.info(f"Terminal event for execution {execution_id}: {msg.event_type}") break except Exception as e: - logger.warning( - f"Failed to process SSE message for execution {execution_id}: {e}", - extra={"execution_id": execution_id, "event_type": str(msg.event_type)}, + logger.exception( + "Failed to process SSE message for execution %s: %r", + execution_id, + msg, + extra={"execution_id": execution_id, "event_type": str(msg.event_type), "msg_data": msg.data}, ) continue
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/services/sse/redis_bus.pybackend/app/services/sse/sse_service.pyfrontend/src/routes/admin/__tests__/AdminEvents.test.tsfrontend/src/routes/admin/__tests__/test-utils.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/routes/admin/__tests__/AdminEvents.test.ts (1)
frontend/src/routes/admin/__tests__/test-utils.ts (3)
mockElementAnimate(35-37)mockWindowGlobals(42-45)createMockEvent(74-74)
backend/app/services/sse/sse_service.py (4)
backend/app/schemas_pydantic/execution.py (1)
ExecutionResult(98-111)backend/app/schemas_pydantic/sse.py (4)
RedisNotificationMessage(102-112)RedisSSEMessage(63-68)SSEExecutionEventData(18-60)SSENotificationEventData(75-99)backend/app/services/sse/redis_bus.py (1)
get(22-34)backend/app/dlq/models.py (1)
event_type(93-95)
backend/app/services/sse/redis_bus.py (1)
backend/app/schemas_pydantic/sse.py (2)
RedisNotificationMessage(102-112)RedisSSEMessage(63-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (13)
backend/app/services/sse/redis_bus.py (3)
1-12: LGTM!The imports are clean and well-organized. The
TypeVarbound toBaseModelprovides proper type safety for the generic message parsing pattern.
22-34: LGTM!The typed message parsing implementation is well done. The
model: Type[T]parameter with return typeT | Noneprovides good type safety. The logging for parse failures includes appropriate context (channel and model name), which addresses the previous review feedback about silent error swallowing.
59-75: LGTM!The typed message publishing is clean and well-implemented:
publish_eventcorrectly constructsRedisSSEMessagewith proper field mapping and usesmodel_dump_json()for serialization.publish_notificationnow accepts a pre-constructedRedisNotificationMessage, providing a cleaner API with full type safety.This is a solid improvement over the previous dict-based approach.
frontend/src/routes/admin/__tests__/AdminEvents.test.ts (3)
5-13: LGTM! Successfully centralized test utilities.The refactor to import mock helpers from the centralized test-utils module addresses the previous review concern about code duplication. The centralized approach improves maintainability and consistency across admin test files.
76-77: LGTM! Clean mock setup.Using the centralized mock functions simplifies the test setup and improves readability.
394-394: LGTM! Consistent event_type format.The event_type values correctly use snake_case format (e.g.,
execution_completed,execution_started,execution_failed,pod_created) throughout these test cases, maintaining consistency with the centralized test utilities.Also applies to: 407-407, 427-428, 742-742, 751-751, 759-759
frontend/src/routes/admin/__tests__/test-utils.ts (3)
4-37: LGTM! Comprehensive Web Animations API mock.The
animateMockobject thoroughly covers the Web Animations API interface with all necessary properties and methods. The promise-returning getters forfinishedandreadyare correctly implemented, and themockElementAnimatefunction properly sets up the mock for Svelte transition testing.
42-45: LGTM! Clean window globals mock.The
mockWindowGlobalsfunction provides a straightforward way to stub window globals for testing, properly using the vitest API to control window.open and window.confirm behavior.
51-126: LGTM! Well-designed mock factories with complete event type coverage.The event mock helpers and factory functions are well-structured and address the previous review concern about incomplete event type coverage. By importing
EVENT_TYPESdirectly from the source ($lib/admin/events/eventTypes) rather than maintaining a local array, the tests will automatically stay in sync with the full set of event types. The factory functions provide flexible, type-safe mock data generation with sensible defaults and override capabilities.Based on learnings, this successfully resolves the past review feedback about centralizing mock helpers and ensuring complete event type coverage.
backend/app/services/sse/sse_service.py (4)
11-17: LGTM: Typed imports strengthen contract.The addition of typed Pydantic models (ExecutionResult, RedisSSEMessage, SSEExecutionEventData, SSENotificationEventData) replaces ad-hoc dictionary construction with structured payloads, improving type safety and maintainability.
161-177: Validation pattern is sound but may silently skip malformed events.The method correctly enriches RESULT_STORED events with execution details and uses Pydantic's
model_validatefor type safety. Validation errors (e.g., schema mismatches) are caught by the caller's exception handler at line 146-159, which logs and continues processing. This prevents one malformed message from breaking the entire stream, though it means clients won't receive that particular event. Ensure the logging enhancement suggested above is applied so debugging is tractable.
248-254: LGTM: Clean formatting helpers.Both methods correctly serialize Pydantic models to JSON with
exclude_none=True, which keeps SSE payloads lean by omitting null fields. The return structure{"data": ...}aligns with sse-starlette's expected format.
54-61: All string event type literals used in this code are valid.SSEControlEventTypeandSSENotificationControlEventTypeareLiteraltype aliases (not enums), and Pydantic validates them correctly. The values"error","connected","status","shutdown","heartbeat", and"notification"all match their respectiveLiteraldefinitions inbackend/app/schemas_pydantic/sse.pylines 12 and 72, so validation will succeed at runtime.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/lib/editor/execution.svelte.ts (2)
98-108: Consider preserving original error context when fallback succeeds.Lines 103-106 clear the error state when the fallback fetch successfully retrieves a result. While this may be intentional, it could hide valuable diagnostic information about why the SSE stream failed. If the execution itself succeeded but only the streaming mechanism failed, users/developers might want to know about the SSE issue.
Consider one of these approaches:
- Log the original error even when clearing it:
console.warn('SSE failed but result retrieved:', error);- Preserve both error and result if result indicates success
- Add a separate state field for "streaming warnings" distinct from execution errors
21-122: Verify test coverage for critical execution paths.The Codecov report indicates 82 missing lines of coverage in this file. Given that this module manages the entire execution lifecycle (SSE streaming, fallback fetching, error handling), ensuring comprehensive test coverage—especially for error paths, SSE failures, and timeout scenarios—would significantly improve reliability.
Consider adding tests for:
- Successful execution with SSE
- SSE failures triggering fallback
- Parse errors in event handling
- API errors during execution start
- Fallback fetch failures
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/admin/users/rateLimits.tsfrontend/src/lib/editor/execution.svelte.ts
🧰 Additional context used
🪛 Gitleaks (8.30.0)
frontend/src/lib/admin/users/rateLimits.ts
[high] 13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (5)
frontend/src/lib/admin/users/rateLimits.ts (2)
31-38: Regex anchor removal logic is correct.The endpoint cleaning logic now correctly removes only the leading
^anchor and trailing$anchor, which aligns with the stated intent. The previous regex issue has been properly resolved.
56-64: Multiplier validation logic addresses previous concern.The function now explicitly validates that only positive multipliers are accepted, with 0 and negative values falling back to 1. This is a clear design decision (documented in the comment on line 58) that differs from the previous review suggestion but appears intentional and reasonable.
frontend/src/lib/editor/execution.svelte.ts (3)
9-16: Previous type safety issue resolved.The validation helper
toExecutionPhasewithVALID_BACKEND_PHASESsuccessfully addresses the prior concern about unsafe type casting. Backend statuses are now validated before assignment, and invalid values fall back to a safe default.
80-92: Previous awaiting issues resolved.Both identified issues from prior reviews have been addressed:
- Line 82:
fetchFallback()is now properly awaited when failure events occur- Line 91: The
onerrorhandler is now async and awaitsfetchFallback()
114-121: Well-designed API surface.The return object provides a clean, reactive API with getters for state and appropriate methods. The computed
isExecutingproperty nicely encapsulates the execution status check.
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)
backend/app/db/repositories/saga_repository.py (1)
126-126: Inconsistent string literal — useSagaState.COMPLETED.value.Line 126 uses a hardcoded
"completed"string in the aggregation pipeline, which is inconsistent with the enum migration applied to other methods in this file. This should useSagaState.COMPLETED.valuefor consistency and type safety.🔎 Proposed fix
- {"$match": {**query, "state": "completed", "completed_at": {"$ne": None}}}, + {"$match": {**query, "state": SagaState.COMPLETED.value, "completed_at": {"$ne": None}}},frontend/src/routes/admin/AdminUsers.svelte (1)
143-163: Unhandled error in saveUser.The
unwrap(result)at line 160 can throw if the create/update fails, but there's no error handling. The user won't see feedback on failure, and the form state may become inconsistent.🔎 Proposed fix: wrap in try-catch with toast feedback
async function saveUser(): Promise<void> { if (!userForm.username) { addToast('Username is required', 'error'); return; } if (!editingUser && !userForm.password) { addToast('Password is required', 'error'); return; } savingUser = true; - let result; - if (editingUser) { - const updateData: Record<string, string | boolean | null> = { - username: userForm.username, email: userForm.email || null, role: userForm.role, is_active: userForm.is_active - }; - if (userForm.password) updateData.password = userForm.password; - result = await updateUserApiV1AdminUsersUserIdPut({ path: { user_id: editingUser.user_id }, body: updateData }); - } else { - result = await createUserApiV1AdminUsersPost({ - body: { username: userForm.username, email: userForm.email || null, password: userForm.password, role: userForm.role, is_active: userForm.is_active } - }); + try { + let result; + if (editingUser) { + const updateData: Record<string, string | boolean | null> = { + username: userForm.username, email: userForm.email || null, role: userForm.role, is_active: userForm.is_active + }; + if (userForm.password) updateData.password = userForm.password; + result = await updateUserApiV1AdminUsersUserIdPut({ path: { user_id: editingUser.user_id }, body: updateData }); + } else { + result = await createUserApiV1AdminUsersPost({ + body: { username: userForm.username, email: userForm.email || null, password: userForm.password, role: userForm.role, is_active: userForm.is_active } + }); + } + unwrap(result); + showUserModal = false; + await loadUsers(); + addToast(editingUser ? 'User updated successfully.' : 'User created successfully.', 'success'); + } catch (err) { + addToast('Failed to save user. Please try again.', 'error'); + } finally { + savingUser = false; } - savingUser = false; - unwrap(result); - showUserModal = false; - await loadUsers(); }
♻️ Duplicate comments (3)
frontend/src/routes/admin/AdminUsers.svelte (2)
165-177: Unhandled error in deleteUser – previously flagged.This issue was identified in a prior review. The
unwrap(result)at line 173 can throw without error handling.
195-213: Unhandled errors in rate limit operations – previously flagged.This issue was identified in a prior review. Both
saveRateLimitsandresetRateLimitscallunwrap()without error handling.frontend/src/routes/admin/AdminSagas.svelte (1)
137-142: Auto-refresh effect condition – previously flagged.The condition
if (autoRefresh || refreshRate)was flagged in a prior review. WhilesetupAutoRefresh()handles this correctly internally (checkingif (autoRefresh)), the effect still runs unnecessarily when onlyrefreshRatechanges and auto-refresh is disabled.
🧹 Nitpick comments (2)
frontend/src/components/admin/users/UserFilters.svelte (1)
1-36: LGTM – well-structured component with proper Svelte 5 patterns.The props interface is well-defined, and the use of
$bindablefor two-way binding is appropriate. Accessibility is handled correctly with proper label associations.Note: The filter types (
RoleFilter,StatusFilter,BooleanFilter,MultiplierFilter,AdvancedFilters) are duplicated inAdminUsers.svelte. Consider extracting them into a shared types file (e.g.,$lib/admin/users/types.ts) to maintain a single source of truth.frontend/src/lib/admin/sagas/sagaStates.ts (1)
88-92: Replace magic numbers with constants or document the assumptions.The function uses hardcoded values (
5and3) without clear documentation. The5should referenceEXECUTION_SAGA_STEPS.length, and the fallback3needs either a corresponding constant or a comment explaining what saga types use 3 steps.🔎 Proposed fix
+// Default step count for non-execution sagas (e.g., cleanup, compensation sagas) +const DEFAULT_SAGA_STEP_COUNT = 3; + export function getSagaProgressPercentage(completedSteps: string[], sagaName: string): number { if (!completedSteps?.length) return 0; - const totalSteps = sagaName === 'execution_saga' ? 5 : 3; + const totalSteps = sagaName === 'execution_saga' + ? EXECUTION_SAGA_STEPS.length + : DEFAULT_SAGA_STEP_COUNT; return Math.min(100, (completedSteps.length / totalSteps) * 100); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/app/api/routes/dlq.pybackend/app/db/repositories/dlq_repository.pybackend/app/db/repositories/replay_repository.pybackend/app/db/repositories/saga_repository.pybackend/app/dlq/models.pybackend/tests/unit/db/repositories/test_replay_repository.pybackend/tests/unit/db/repositories/test_saga_repository.pyfrontend/src/components/admin/sagas/SagaFilters.sveltefrontend/src/components/admin/users/UserFilters.sveltefrontend/src/lib/admin/events/eventTypes.tsfrontend/src/lib/admin/sagas/sagaStates.tsfrontend/src/routes/admin/AdminSagas.sveltefrontend/src/routes/admin/AdminUsers.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/admin/events/eventTypes.ts
- frontend/src/components/admin/sagas/SagaFilters.svelte
🧰 Additional context used
🧬 Code graph analysis (6)
backend/app/db/repositories/replay_repository.py (1)
backend/app/domain/replay/models.py (1)
ReplaySessionState(85-102)
backend/app/dlq/models.py (4)
frontend/src/lib/admin/events/eventTypes.ts (1)
EventType(7-7)frontend/src/lib/api/index.ts (1)
EventType(4-4)frontend/src/lib/api/types.gen.ts (1)
EventType(1028-1028)backend/tests/unit/services/idempotency/test_middleware.py (1)
event(32-36)
backend/app/db/repositories/dlq_repository.py (1)
backend/app/dlq/models.py (4)
DLQMessageStatus(10-16)event_type(93-95)DLQMessageListResult(219-225)DLQMessageFilter(113-118)
backend/tests/unit/db/repositories/test_replay_repository.py (2)
backend/tests/unit/db/repositories/test_saga_repository.py (1)
repo(13-14)backend/app/db/repositories/replay_repository.py (1)
update_session_status(62-65)
backend/app/api/routes/dlq.py (2)
frontend/src/lib/admin/events/eventTypes.ts (1)
EventType(7-7)backend/app/dlq/models.py (1)
event_type(93-95)
backend/app/db/repositories/saga_repository.py (3)
backend/app/services/coordinator/queue_manager.py (1)
execution_id(30-31)backend/app/domain/saga/models.py (1)
Saga(10-25)backend/tests/unit/dlq/test_dlq_models.py (1)
value(60-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan Frontend
- GitHub Check: E2E Tests
- GitHub Check: Integration Tests
🔇 Additional comments (21)
backend/tests/unit/db/repositories/test_replay_repository.py (1)
29-29: LGTM! Test correctly updated to use enum.The change from string
"running"toReplayStatus.RUNNINGcorrectly aligns the test with the updated repository signature and improves type safety.backend/app/db/repositories/replay_repository.py (4)
8-8: LGTM! Enum import enables type-safe status handling.The addition of
ReplayStatusimport supports the migration from string-based to enum-based status handling throughout the repository.
45-52: LGTM! Type-safe signature with correct enum-to-value conversion.The signature change to accept
ReplayStatus | Noneand the use ofstatus.valuefor MongoDB queries correctly implements enum-based status handling. The explicit query dict typing is also appropriate.Note: This is a breaking API change for any callers passing string literals, but this aligns with the broader enum migration across the codebase.
62-64: LGTM! Enum-based status update with correct persistence.The method signature now enforces type-safe
ReplayStatusenum usage, and the implementation correctly usesstatus.valueto persist the enum's string representation to MongoDB.
67-77: LGTM! Enum-derived terminal statuses improve maintainability.Deriving
terminal_statusesfrom theReplayStatusenum eliminates hard-coded strings and establishes a single source of truth. This reduces the risk of typos and inconsistencies when status values change.backend/app/db/repositories/saga_repository.py (3)
49-56: LGTM! State parameter correctly migrated to enum.The signature change from
str | NonetoSagaState | Noneimproves type safety, and the use ofstate.valueon line 52 correctly extracts the enum's string value for the MongoDB query.
72-80: LGTM! State parameter correctly migrated to enum.The signature change from
strtoSagaStateimproves type safety, and the use ofstate.valueon line 73 correctly extracts the enum's string value for the database update.
49-80: All existing callers correctly useSagaStateenum.The identified callers of
get_sagas_by_executioninsaga_service.py:75andsaga_orchestrator.py:393both properly use theSagaStateenum. Theupdate_saga_statemethod has no production callers at this time; its only caller is in the test suite which correctly passesSagaState.COMPLETED.backend/tests/unit/db/repositories/test_saga_repository.py (1)
6-6: LGTM! Test correctly updated to useSagaStateenum.The import of
SagaStateon line 6 and its usage on line 34 correctly align with the repository method signature changes. The raw string literals in the test data setup (lines 22-23) are appropriate since they're direct MongoDB document inserts.Also applies to: 34-34
frontend/src/components/admin/users/UserFilters.svelte (1)
38-120: LGTM – clean and accessible filter UI.The template is well-structured with:
- Proper responsive layout using Tailwind's breakpoint classes
- Accessible labels for all form controls
- Conditional rendering for advanced filters with visual feedback (chevron rotation)
- Disabled state for reset button when no filters are active
frontend/src/lib/admin/sagas/sagaStates.ts (2)
1-57: LGTM – well-organized state configuration.Clean mapping of saga states to their visual configurations with appropriate icons and colors for each state.
59-68: LGTM – safe fallback pattern.The
getSagaStateInfofunction correctly falls back toDEFAULT_STATEwhile preserving the unknown state value as the label.frontend/src/routes/admin/AdminUsers.svelte (1)
241-296: LGTM – clean component composition.The refactoring to use modular components (
UserFilters,UsersTable,Pagination) improves maintainability and separates concerns appropriately.frontend/src/routes/admin/AdminSagas.svelte (2)
41-44: Good fix for client-side filtering UX concern.The addition of
hasClientFilterstracking, the explanatory comment, and the UI message ("Showing X of Y on this page (filtered locally)") effectively addresses the pagination confusion previously flagged. Users now have clear feedback about local filtering behavior.Also applies to: 192-209
155-220: LGTM – well-structured component composition.The refactoring to use modular components (
SagaStatsCards,SagaFilters,SagasTable,SagaDetailsModal,AutoRefreshControl) significantly improves maintainability and readability.backend/app/api/routes/dlq.py (2)
13-13: LGTM: EventType import added.The import supports the type-safe event filtering introduced at line 54.
54-59: Verify EventType enum Query parameter handling in FastAPI.The migration from
str | NonetoEventType | Noneimproves type safety, but requires confirmation that:
- The EventType enum is properly defined and imported
- FastAPI correctly validates and deserializes enum values from query parameters
- The repository's
get_messages()method signature accepts the EventType typebackend/app/dlq/models.py (2)
6-6: LGTM: EventType import added.The import enables type-safe event type handling in DLQMessage and DLQMessageFilter.
93-95: Verify BaseEvent.event_type returns EventType.The property now returns
self.event.event_typedirectly without string conversion, which requiresBaseEvent.event_typeto be typed asEventType. Verify that:
BaseEvent.event_typeis defined asEventType(notstr)- All event classes properly set
event_typeas anEventTypeenum valuebackend/app/db/repositories/dlq_repository.py (2)
21-21: LGTM: EventType import added.The import supports type-safe event filtering in the repository layer.
107-118: The enum-to-string conversion concern is unfounded.DLQMessageStatusandEventTypeareStringEnumsubclasses that inherit from Python'sStrEnum, making them first-class string objects. When passed to MongoDB queries, they function as strings without requiring explicit.valueconversion. The mapper implementation is correct.However, the test file should be updated to use
EventType.EXECUTION_REQUESTEDinstead of the string literal"execution_requested"to align with the new type signature that expectsEventType | None.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/routes/admin/AdminUsers.svelte (1)
143-163: Unhandled error in saveUser.The
unwrap(result)at line 160 can throw if the API call fails, but there's no error handling. The user won't see feedback on failure, and the modal will remain open.🔎 Proposed fix: wrap in try-catch with toast feedback
async function saveUser(): Promise<void> { if (!userForm.username) { addToast('Username is required', 'error'); return; } if (!editingUser && !userForm.password) { addToast('Password is required', 'error'); return; } savingUser = true; - let result; - if (editingUser) { - const updateData: Record<string, string | boolean | null> = { - username: userForm.username, email: userForm.email || null, role: userForm.role, is_active: userForm.is_active - }; - if (userForm.password) updateData.password = userForm.password; - result = await updateUserApiV1AdminUsersUserIdPut({ path: { user_id: editingUser.user_id }, body: updateData }); - } else { - result = await createUserApiV1AdminUsersPost({ - body: { username: userForm.username, email: userForm.email || null, password: userForm.password, role: userForm.role, is_active: userForm.is_active } - }); + try { + let result; + if (editingUser) { + const updateData: Record<string, string | boolean | null> = { + username: userForm.username, email: userForm.email || null, role: userForm.role, is_active: userForm.is_active + }; + if (userForm.password) updateData.password = userForm.password; + result = await updateUserApiV1AdminUsersUserIdPut({ path: { user_id: editingUser.user_id }, body: updateData }); + } else { + result = await createUserApiV1AdminUsersPost({ + body: { username: userForm.username, email: userForm.email || null, password: userForm.password, role: userForm.role, is_active: userForm.is_active } + }); + } + unwrap(result); + showUserModal = false; + await loadUsers(); + addToast(editingUser ? 'User updated successfully' : 'User created successfully', 'success'); + } catch (err) { + addToast('Failed to save user. Please try again.', 'error'); + } finally { + savingUser = false; } - savingUser = false; - unwrap(result); - showUserModal = false; - await loadUsers(); }backend/app/db/repositories/replay_repository.py (1)
97-112: Critical: Caller still uses oldfilterkeyword argument instead ofreplay_filter.The method signature was updated to use
replay_filter, butbackend/app/services/event_replay/replay_service.pylines 189-191 still passesfilter=as a keyword argument:async for batch_docs in self._repository.fetch_events( filter=session.config.filter, batch_size=session.config.batch_size ):Update this call to use
replay_filter=instead offilter=.
♻️ Duplicate comments (3)
frontend/src/routes/admin/AdminUsers.svelte (3)
165-177: Still unhandled: error in deleteUser.This issue was flagged in a previous review but remains unfixed. The
unwrap(result)at line 173 can throw if the delete fails, but there's no error handling. The user won't see feedback on failure.🔎 Proposed fix: wrap in try-catch with toast feedback
async function deleteUser(): Promise<void> { if (!userToDelete) return; deletingUser = true; - const result = await deleteUserApiV1AdminUsersUserIdDelete({ - path: { user_id: userToDelete.user_id }, - query: { cascade: cascadeDelete } - }); - deletingUser = false; - unwrap(result); - await loadUsers(); - showDeleteModal = false; - userToDelete = null; + try { + const result = await deleteUserApiV1AdminUsersUserIdDelete({ + path: { user_id: userToDelete.user_id }, + query: { cascade: cascadeDelete } + }); + unwrap(result); + await loadUsers(); + showDeleteModal = false; + userToDelete = null; + addToast('User deleted successfully', 'success'); + } catch (err) { + addToast('Failed to delete user. Please try again.', 'error'); + } finally { + deletingUser = false; + } }
202-212: Still unhandled: error in saveRateLimits.This issue was flagged in a previous review but remains unfixed. The
unwrap(result)at line 210 can throw if the save fails, but there's no error handling. The modal won't close, and the user won't see feedback on failure.🔎 Proposed fix: wrap in try-catch with toast feedback
async function saveRateLimits(): Promise<void> { if (!rateLimitUser || !rateLimitConfig) return; savingRateLimits = true; - const result = await updateUserRateLimitsApiV1AdminUsersUserIdRateLimitsPut({ - path: { user_id: rateLimitUser.user_id }, - body: rateLimitConfig - }); - savingRateLimits = false; - unwrap(result); - showRateLimitModal = false; + try { + const result = await updateUserRateLimitsApiV1AdminUsersUserIdRateLimitsPut({ + path: { user_id: rateLimitUser.user_id }, + body: rateLimitConfig + }); + unwrap(result); + showRateLimitModal = false; + addToast('Rate limits updated successfully', 'success'); + } catch (err) { + addToast('Failed to update rate limits. Please try again.', 'error'); + } finally { + savingRateLimits = false; + } }
214-220: Still unhandled: error in resetRateLimits.This issue was flagged in a previous review but remains unfixed. The
unwrapat line 216 can throw if the reset fails, but there's no error handling. The user won't see feedback on failure.🔎 Proposed fix: wrap in try-catch with toast feedback
async function resetRateLimits(): Promise<void> { if (!rateLimitUser) return; - unwrap(await resetUserRateLimitsApiV1AdminUsersUserIdRateLimitsResetPost({ - path: { user_id: rateLimitUser.user_id } - })); - rateLimitUsage = {}; + try { + unwrap(await resetUserRateLimitsApiV1AdminUsersUserIdRateLimitsResetPost({ + path: { user_id: rateLimitUser.user_id } + })); + rateLimitUsage = {}; + addToast('Rate limits reset successfully', 'success'); + } catch (err) { + addToast('Failed to reset rate limits. Please try again.', 'error'); + } }
🧹 Nitpick comments (2)
backend/app/db/repositories/replay_repository.py (1)
67-77: Consider a more concise list comprehension (optional).The enum-based status filtering is correct. For slightly cleaner code, you could use a list comprehension.
🔎 Optional refactor using list comprehension
async def delete_old_sessions(self, cutoff_time: str) -> int: """Delete old completed/failed/cancelled sessions""" - terminal_statuses = [ - ReplayStatus.COMPLETED.value, - ReplayStatus.FAILED.value, - ReplayStatus.CANCELLED.value, - ] + terminal_statuses = [s.value for s in [ReplayStatus.COMPLETED, ReplayStatus.FAILED, ReplayStatus.CANCELLED]] result = await self.replay_collection.delete_many( {"created_at": {"$lt": cutoff_time}, "status": {"$in": terminal_statuses}} )backend/tests/unit/infrastructure/mappers/test_dlq_mapper.py (1)
116-116: Stylistic: Consistent quote usage in patch decorators.Changed patch path strings from single to double quotes. This is purely stylistic and has no functional impact. Consider using a formatter (like
blackorruff format) to automatically maintain consistent quote styles across the codebase.Also applies to: 140-140, 161-161, 175-175, 188-188, 201-201, 212-212
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/app/api/routes/admin/events.pybackend/app/db/repositories/admin/admin_events_repository.pybackend/app/db/repositories/dlq_repository.pybackend/app/db/repositories/replay_repository.pybackend/app/db/repositories/saga_repository.pybackend/app/domain/events/query_builders.pybackend/app/infrastructure/mappers/saga_mapper.pybackend/app/services/admin/admin_events_service.pybackend/tests/unit/dlq/test_dlq_models.pybackend/tests/unit/infrastructure/mappers/test_dlq_mapper.pyfrontend/src/routes/admin/AdminUsers.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/db/repositories/dlq_repository.py
🧰 Additional context used
🧬 Code graph analysis (7)
backend/app/db/repositories/admin/admin_events_repository.py (4)
backend/app/domain/events/event_models.py (4)
EventFields(13-45)SortDirection(53-55)EventBrowseResult(149-155)EventExportRow(248-259)backend/app/infrastructure/mappers/event_mapper.py (1)
EventFilterMapper(317-366)backend/app/api/routes/admin/events.py (1)
export_events_csv(80-103)backend/app/services/admin/admin_events_service.py (1)
export_events_csv(183-185)
backend/app/infrastructure/mappers/saga_mapper.py (1)
backend/app/domain/saga/models.py (1)
SagaFilter(29-38)
backend/app/api/routes/admin/events.py (1)
backend/app/services/admin/admin_events_service.py (2)
export_events_csv_content(187-217)export_events_json_content(219-258)
backend/tests/unit/dlq/test_dlq_models.py (2)
backend/app/dlq/models.py (9)
AgeStatistics(180-185)DLQFields(29-49)DLQMessageFilter(113-118)DLQMessageStatus(10-16)DLQStatistics(189-196)EventTypeStatistic(172-176)RetryPolicy(122-158)TopicStatistic(163-168)event_type(93-95)backend/app/infrastructure/mappers/dlq_mapper.py (2)
DLQMapper(22-239)filter_to_query(231-239)
backend/tests/unit/infrastructure/mappers/test_dlq_mapper.py (2)
backend/app/infrastructure/mappers/dlq_mapper.py (1)
filter_to_query(231-239)backend/app/dlq/models.py (2)
event_type(93-95)DLQFields(29-49)
backend/app/db/repositories/saga_repository.py (2)
backend/app/domain/saga/models.py (3)
Saga(10-25)SagaFilter(29-38)SagaListResult(53-64)backend/app/infrastructure/mappers/saga_mapper.py (3)
from_mongo(13-29)from_mongo(120-149)to_mongodb_query(217-247)
backend/app/db/repositories/replay_repository.py (1)
backend/app/domain/replay/models.py (2)
ReplaySessionState(85-102)ReplayFilter(11-53)
🪛 GitHub Actions: MyPy Type Checking
backend/app/db/repositories/replay_repository.py
[error] 97-97: ReplayRepository.fetch_events signature does not accept a 'filter' keyword argument. This is likely causing the mypy error reported in the caller.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests
- GitHub Check: Build Frontend
- GitHub Check: Integration Tests
🔇 Additional comments (23)
backend/app/domain/events/query_builders.py (1)
54-56: Good refactor: Avoids shadowing the built-informatfunction.Renaming the parameter from
formattodate_formatis a good practice as it avoids shadowing Python's built-informat()function. The change is backward compatible since callers use the default value.backend/app/api/routes/admin/events.py (3)
45-51: LGTM: Parameter naming aligned with service layer.The keyword argument
event_filter=event_filtercorrectly matches the service method signaturebrowse_events(*, event_filter: EventFilter, ...).
95-95: LGTM: Consistent parameter naming.
131-131: LGTM: Consistent parameter naming.backend/app/db/repositories/admin/admin_events_repository.py (2)
59-68: LGTM: Parameter rename improves clarity and avoids shadowing built-infilter.The rename from
filtertoevent_filteris more descriptive and avoids shadowing Python's built-infilter()function. The internal usage at line 68 is correctly updated.
194-196: LGTM: Consistent parameter naming withbrowse_events.backend/app/services/admin/admin_events_service.py (3)
59-70: LGTM: Service method correctly propagates the renamed parameter.The parameter rename and repository call are consistent with the repository layer changes.
183-188: LGTM: Consistent parameter naming across export methods.Both
export_events_csvandexport_events_csv_contentcorrectly use the renamedevent_filterparameter.
219-244: LGTM: Complete and consistent parameter rename in JSON export.The method signature, repository call, and all metadata field accesses (lines 237-243) correctly reference
event_filter. The coordinated refactor across routes, service, and repository layers is complete.backend/app/infrastructure/mappers/saga_mapper.py (1)
217-247: LGTM - Excellent type safety and naming improvements!The changes improve code quality in several ways:
- Renaming
filter→saga_filteravoids shadowing Python's built-infilterfunction- Making the parameter nullable (
SagaFilter | None) adds defensive flexibility- Early return for None case is clean and efficient
- All field accesses include appropriate None checks
- Proper use of
saga_filter.state.valuefor enum-to-string conversionbackend/app/db/repositories/saga_repository.py (4)
49-56: LGTM - Proper enum type usage!The explicit
SagaState | Nonetyping and use ofstate.valuefor the MongoDB query ensures type safety and correct enum-to-string conversion.
58-70: LGTM - Consistent parameter naming!The parameter renaming from
filtertosaga_filtermaintains consistency with the mapper and avoids shadowing Python's built-infilterfunction. The non-nullable type here is appropriate since this method requires a filter, while the mapper defensively accepts nullable values.
72-80: LGTM - Type-safe state updates!Explicit
SagaStatetyping and use ofstate.valueensures type safety and correct enum-to-string conversion for MongoDB updates.
111-136: LGTM - Clean optional filtering!The nullable
saga_filterparameter with conditional query construction is a clean pattern for optional filtering. The parameter renaming maintains consistency across the codebase.frontend/src/routes/admin/AdminUsers.svelte (1)
180-200: Good fix: error handling added to openRateLimitModal.The try-catch-finally block properly handles errors when loading rate limits. The catch block closes the modal and shows user feedback, and the finally block ensures the loading state is always reset. This addresses the issue from the previous review.
backend/app/db/repositories/replay_repository.py (4)
8-8: LGTM!The import of
ReplayStatusenables type-safe enum usage throughout the repository methods.
92-95: Parameter rename is good and safe.Renaming the parameter from
filtertoreplay_filteravoids shadowing the Python builtin and improves clarity. All callers use positional arguments (count_events(ReplayFilter())andcount_events(session.config.filter)), so the rename introduces no breaking changes.
62-65: All callers correctly use ReplayStatus enum values.The method signature and implementation are correct, and all callers in
replay_service.pyand the test file passReplayStatusenum values (e.g.,ReplayStatus.RUNNING,ReplayStatus.PAUSED,ReplayStatus.CANCELLED).
45-60: Type-safe refactoring is complete and well-integrated.The method correctly accepts
ReplayStatusenum instead of raw strings, improving type safety. All callers—including the API endpoint, service wrappers, and tests—are properly updated. FastAPI's automatic enum deserialization on the Query parameter ensures type safety at the API boundary, and the implementation correctly usesstatus.valuefor MongoDB queries.backend/tests/unit/dlq/test_dlq_models.py (2)
2-2: LGTM! Necessary imports for enum-based testing.The addition of
timezone,DLQStatistics, andEventTypeimports properly supports the test updates that migrate from string literals to type-safe enum values.Also applies to: 9-9, 15-15
98-98: LGTM! Enum-based filtering improves type safety.The migration from string literal
"execution_requested"toEventType.EXECUTION_REQUESTEDenum aligns with the domain model changes and provides better type safety, IDE support, and prevents typos.Also applies to: 102-102
backend/tests/unit/infrastructure/mappers/test_dlq_mapper.py (2)
17-21: LGTM! Necessary import additions.The
EventTypeimport addition supports enum-based filtering in the test assertions. Minor import reorganization has no functional impact.
403-403: LGTM! Type-safe enum usage in filter tests.Replacing string literal with
EventType.EXECUTION_REQUESTEDenum improves type safety and aligns with the domain model's migration to enum-based event typing. The test correctly verifies that the mapper preserves enum values in query construction.Also applies to: 410-410
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.