chore: frontend updates #44
Conversation
📝 WalkthroughWalkthroughBackend: editor theme default set to "auto" and runtime metadata (versions, file_ext) introduced; supported_runtimes now uses LanguageInfo/LanguageInfoDomain. Frontend: removed localStorage settings-cache, added userSettings/editorSettings stores and unified saveUserSettings API; large componentization of editor and admin UIs, icon replacements, tests and storage moved to sessionStorage. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI
participant Auth as Auth Store
participant API as updateUserSettings API
participant Store as userSettings Store
participant Theme as Theme handler
Note over UI,API: User saves settings (partial UserSettingsUpdate)
UI->>Auth: check isAuthenticated()
alt authenticated
UI->>API: PUT /api/v1/user/settings (partial)
API-->>UI: 200 OK with updated UserSettings
UI->>Store: setUserSettings(updated settings)
alt updated settings include theme
Store->>Theme: setTheme(theme)
end
else not authenticated
UI->>Store: setUserSettings locally (if applicable)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.