Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces accent color support across backend and frontend. Backend adds AccentColor setting, defaults, DTO, and passes Settings into ApplicationImagesService, which now recolors SVG logos. Frontend adds accent color picker, custom color dialog, radio group UI components, utilities to apply CSS variables, reactive logo URLs, settings schema updates, and related UI/i18n tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
🧹 Nitpick comments (6)
frontend/src/lib/components/accent-color/custom-color.svelte (1)
25-30: Consider optimizing color validation.The
isValidColorfunction creates a new DOM element on every validation call, which could impact performance during rapid input changes (e.g., typing or pasting).Consider using CSS.supports for validation:
function isValidColor(color: string): boolean { - // Create a temporary element to test if the color is valid - const testElement = document.createElement('div'); - testElement.style.color = color; - return testElement.style.color !== ''; + return CSS.supports('color', color); }This approach is more efficient and doesn't create DOM elements. However, verify browser compatibility if targeting older browsers.
backend/internal/services/settings_service.go (1)
101-101: Enforce and document OKLCH accentColor support
- In SettingsService.UpdateSettings (backend/internal/services/settings_service.go), validate
AccentColor(e.g., via a CSS-color parser) and reject or default invalid formats before saving.- Document OKLCH support minimum versions: Chrome/Edge 111+, Firefox 113+, Safari 15.4+, Opera 97+, Samsung Internet recent.
- Provide a CSS fallback (e.g., hex) for unsupported browsers in applyAccentColorToSVG or the frontend.
frontend/src/lib/components/sidebar/sidebar-logo.svelte (1)
6-6: Remove unused import.The
settingsStoreimport appears unused in this file. WhilegetApplicationLogo()does usesettingsStoreinternally, Svelte 5's$derivedautomatically tracks reactive dependencies without requiring explicit imports.Apply this diff to remove the unused import:
- import settingsStore from '$lib/stores/config-store';frontend/src/lib/components/accent-color/accent-color-picker.svelte (1)
15-23: Consider extracting the color palette to a shared config.The hardcoded
accentColorsarray is fine for now, but if these colors are used elsewhere or need to be maintained centrally, consider extracting them to a separate configuration file (e.g.,$lib/config/accent-colors.ts).frontend/src/lib/utils/accent-color-util.ts (1)
45-45: Refactor the RGB value parsing for clarity.Line 45 applies
.map(Number)to the regex match array, which includes the full match at index 0 (resulting inNaN), before destructuring. This works but is inefficient and unclear.Apply this diff for a clearer approach:
- const [, r, g, b] = rgbMatch.map(Number); + const r = Number(rgbMatch[1]); + const g = Number(rgbMatch[2]); + const b = Number(rgbMatch[3]);backend/internal/services/app_images_service.go (1)
96-96: Consider more robust color replacement.Line 96 uses a simple string replacement that only targets
fill:#6D28D9in the SVG. If the SVG uses the color in other contexts (e.g., directfillattributes,stroke, or class-based styles), this replacement won't catch them.If the SVG structure is guaranteed to only use this specific style pattern, the current approach is acceptable. Otherwise, consider a more comprehensive replacement strategy, such as parsing the SVG as XML and updating all relevant color attributes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
backend/internal/bootstrap/services_bootstrap.go(1 hunks)backend/internal/dto/settings_dto.go(1 hunks)backend/internal/models/settings.go(1 hunks)backend/internal/services/app_images_service.go(2 hunks)backend/internal/services/settings_service.go(1 hunks)frontend/messages/en.json(2 hunks)frontend/src/lib/components/accent-color/accent-color-picker.svelte(1 hunks)frontend/src/lib/components/accent-color/custom-color.svelte(1 hunks)frontend/src/lib/components/quick-actions.svelte(1 hunks)frontend/src/lib/components/sidebar/sidebar-env-switcher.svelte(3 hunks)frontend/src/lib/components/sidebar/sidebar-logo.svelte(2 hunks)frontend/src/lib/components/ui/radio-group/index.ts(1 hunks)frontend/src/lib/components/ui/radio-group/radio-group-item.svelte(1 hunks)frontend/src/lib/components/ui/radio-group/radio-group.svelte(1 hunks)frontend/src/lib/stores/config-store.ts(1 hunks)frontend/src/lib/types/settings.type.ts(1 hunks)frontend/src/lib/utils/accent-color-util.ts(1 hunks)frontend/src/lib/utils/image.util.ts(1 hunks)frontend/src/routes/auth/login/+page.svelte(2 hunks)frontend/src/routes/onboarding/+layout.svelte(2 hunks)frontend/src/routes/settings/general/+page.svelte(5 hunks)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: Run Golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (35)
backend/internal/models/settings.go (1)
58-58: LGTM!The
AccentColorfield is properly structured and follows the established pattern for public settings fields.frontend/messages/en.json (2)
4-8: LGTM!The translation keys for the accent color feature are well-structured and appropriately descriptive.
1265-1266: No duplicate translation keys.cancelandcommon_cancelare distinct entries sharing the same English value by design; no change needed.Likely an incorrect or invalid review comment.
backend/internal/dto/settings_dto.go (1)
27-27: LGTM!The
AccentColorfield is correctly added to the DTO, following the established pattern with optional pointer type andomitemptytag.frontend/src/lib/components/accent-color/custom-color.svelte (1)
1-75: Well-structured component with good UX.The component properly handles validation, state management, and provides good user feedback with the color swatch preview.
frontend/src/lib/components/sidebar/sidebar-env-switcher.svelte (1)
92-92: LGTM! Styling aligns with accent color system.The changes from
sidebar-primarytoprimaryclasses enable the environment switcher to use the customizable accent color. The implementation is consistent throughout the component.Also applies to: 108-108, 160-167
frontend/src/lib/types/settings.type.ts (1)
23-23: LGTM!The
accentColorfield is properly added to theSettingstype and aligns with the backend implementation.frontend/src/lib/stores/config-store.ts (1)
3-3: Excellent refactoring for centralized accent color management.The changes ensure that accent color is consistently applied whenever settings are loaded or updated. The refactoring to call
set()fromreload()eliminates code duplication.Also applies to: 8-16
frontend/src/routes/onboarding/+layout.svelte (1)
13-14: LGTM! Proper reactive binding for accent color support.The implementation correctly uses Svelte 5's
$derivedto make the logo URL reactive to accent color changes. The binding is clean and follows the pattern consistently applied across other route files.Also applies to: 31-31
frontend/src/routes/auth/login/+page.svelte (1)
27-28: LGTM! Consistent reactive logo implementation.The reactive
logoUrlimplementation matches the pattern used in other routes, ensuring the login page logo responds to accent color changes.Also applies to: 209-209
frontend/src/lib/components/quick-actions.svelte (1)
112-112: LGTM! Theme color migration to primary accent.The styling changes correctly update the refresh button from hardcoded violet colors to the primary theme color, aligning with the new accent color system.
Also applies to: 118-118, 120-120
frontend/src/lib/components/sidebar/sidebar-logo.svelte (1)
10-11: LGTM! Proper reactive logo with collapsed state handling.The implementation correctly makes the logo reactive to accent color changes while also respecting the sidebar's collapsed state.
Also applies to: 27-27
backend/internal/bootstrap/services_bootstrap.go (1)
44-44: LGTM! Proper dependency injection for settings.The ApplicationImagesService initialization correctly receives the settings service, maintaining proper dependency order since Settings is initialized earlier in the bootstrap sequence.
frontend/src/lib/utils/image.util.ts (1)
8-18: LGTM! Correct accent color integration with cache busting.The implementation properly:
- Reads accent color from settings with a sensible default
- Constructs the URL with correct query parameter separators
- Uses
encodeURIComponentfor safety- Integrates with the existing cache busting mechanism
The use of
get(settingsStore)is appropriate here since this function is called from$derivedcontexts in components, ensuring efficient reactivity.frontend/src/lib/components/ui/radio-group/radio-group.svelte (1)
1-19: LGTM! Well-structured radio group wrapper.The component follows best practices for Svelte 5:
- Correct use of
$bindablefor two-way binding- Proper prop spreading with
...restProps- Uses
cnutility for class merging- Includes
data-slotattribute for styling customizationfrontend/src/lib/components/ui/radio-group/index.ts (1)
1-10: LGTM! Standard barrel export pattern.The exports properly expose both internal component names and user-facing aliases, providing a clean public API while maintaining flexibility.
frontend/src/lib/components/ui/radio-group/radio-group-item.svelte (2)
1-11: LGTM!The script section is well-structured with proper imports, type annotations, and Svelte 5 runes syntax for reactive props.
13-31: LGTM!The template implementation is correct. The conditional rendering of the checked indicator using Svelte 5 snippets is idiomatic, and the styling is appropriate for a radio button.
frontend/src/lib/components/accent-color/accent-color-picker.svelte (3)
29-32: LGTM!The handler correctly updates the selected color and applies it via the utility function.
44-50: LGTM!The conditional rendering logic correctly displays predefined colors, a custom color swatch if applicable, and a button to open the custom color dialog.
79-89: LGTM!The conditional rendering of the Plus icon for custom color selection and Check icon for selected items is implemented correctly.
frontend/src/routes/settings/general/+page.svelte (7)
12-12: LGTM!The new imports for accent color functionality are appropriate.
Also applies to: 18-19
22-25: LGTM!Proper initialization with a fallback to
'default'ensuresaccentColoris always defined.
31-36: LGTM!The form schema correctly includes
accentColoras a string. The validation is appropriate for this use case.
40-46: LGTM!Change detection correctly includes
accentColoralongside other form fields.
56-66: LGTM!The
updateSettingsConfigfunction properly handles settings updates with proper error handling.
89-89: LGTM!The reset function correctly includes
accentColor.
155-171: LGTM!The accent color picker UI is properly integrated with correct prop bindings and respects the read-only state.
frontend/src/lib/utils/accent-color-util.ts (3)
1-20: LGTM!The
applyAccentColorfunction correctly handles both the default case (by removing properties) and custom colors (by setting CSS variables with proper foreground contrast and ring colors).
22-27: LGTM!The contrast calculation uses an appropriate threshold and returns suitable foreground colors in the oklch format.
47-57: LGTM!The WCAG relative luminance calculation is correctly implemented following the standard formula.
backend/internal/services/app_images_service.go (4)
15-19: LGTM!Adding the
settingsServicefield is appropriate for retrieving the accent color configuration.
21-26: LGTM!The constructor properly accepts and initializes the
settingsServicedependency.
61-77: LGTM!The refactored
GetImagemethod with early unlock is a good improvement. The conditional SVG color application for logo images is implemented correctly.
79-99: Add fallback for oklch() accent color in SVG
- oklch() works only in modern browsers (Chrome ≥116, Edge ≥116, Firefox ≥113, Safari ≥16.2); older versions and some inline-SVG contexts may ignore it.
- Provide an sRGB fallback (e.g. hex/rgb equivalent) or wrap the oklch() fill in a
@supports(color: oklch(...))(or@media (color-gamut: p3)) rule and test across your target browser matrix.
1 participant
Summary by CodeRabbit