Conversation
prem-k-r
added
enhancement
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces a comprehensive theming refactoring alongside UI enhancements. Changes include migrating theme management from scattered globals to a centralized ThemeManager class, introducing a new color editor modal interface, consolidating icon usage, removing the Changes
Sequence DiagramsequenceDiagram
actor User
participant Modal as Color Editor Modal
participant ThemeManager as ThemeManager
participant Storage as LocalStorage
participant DOM as DOM/Favicon
User->>Modal: Clicks color editor button
Modal->>ThemeManager: openModal()
ThemeManager->>Storage: Load modalOriginalValues
ThemeManager->>Modal: setupModalInputSync(container)
Modal->>DOM: Initialize color inputs with current values
alt User makes color changes
User->>Modal: Adjusts color inputs
Modal->>ThemeManager: Live color updates
ThemeManager->>DOM: updateFaviconColor()
DOM-->>Modal: Preview changes
end
alt User saves changes
User->>Modal: Clicks Save
Modal->>ThemeManager: saveCustomVariables(variables)
ThemeManager->>Storage: Store in CUSTOM_VARIABLES
ThemeManager->>DOM: Apply changes to page
ThemeManager->>Modal: closeModal()
else User resets changes
User->>Modal: Clicks Reset
Modal->>ThemeManager: resetToOriginal()
ThemeManager->>Storage: Restore from modalOriginalValues
ThemeManager->>DOM: Revert all changes
ThemeManager->>Modal: closeModal()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
style.css (1)
225-281: Remove commented-out code if no longer needed.This large block of commented CSS appears to be legacy dark mode logic that was replaced by the new ThemeManager approach. If it's no longer needed, it should be removed to maintain code cleanliness. If it's being kept temporarily for reference, consider adding a TODO comment explaining when it can be deleted.
scripts/theme.js (3)
302-306: Variable shadowing -minandmaxshadow built-in Math functions.The destructuring assignment uses
minandmaxwhich shadowMath.minandMath.maxused earlier in the function. While not causing immediate bugs here, this can lead to confusion.🔎 Proposed fix
// Clamp lightness if specified (values are in percentage, convert to 0-1) if (lightClamp) { - const [min, max] = lightClamp; - l = Math.max(min / 100, Math.min(max / 100, l)); + const [minL, maxL] = lightClamp; + l = Math.max(minL / 100, Math.min(maxL / 100, l)); }
759-764: Redundant value restoration after save.After saving,
modalOriginalValuesis updated to the new values, thencloseModal()is called which restores those same values. While not broken, this is unnecessary work.🔎 Proposed fix - skip restoration after save
// Update original values to current values (so closing modal won't revert) - this.modalOriginalValues = { ...variables }; - - // Close modal - this.closeModal(); + // Clear original values and close modal without reverting + this.modalOriginalValues = {}; + const modal = document.getElementById('colorModal'); + if (modal) modal.classList.remove('active'); }
342-351: Add defensive check forisRTLvariableThe
isRTLvariable is defined globally inmenu-shortcut-page.jswhich loads before this script, but this dependency is implicit. For better robustness and code clarity, add a defensive check:Suggested fix
moveIndicator(mode) { const modeIndex = { light: 0, dark: 1, system: 2 }; const ltrIndex = modeIndex[mode] || 0; - const index = isRTL ? 2 - ltrIndex : ltrIndex; + const index = (typeof isRTL !== 'undefined' && isRTL) ? 2 - ltrIndex : ltrIndex; const indicator = document.querySelector('.themeIndicator'); if (indicator) { indicator.style.transform = `translateX(${index * 100}%)`; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
LICENSEREADME.mdindex.htmllocales/en.jsmanifest(firefox).jsonmanifest.jsonscripts/languages.jsscripts/theme.jsstyle.css
💤 Files with no reviewable changes (2)
- locales/en.js
- scripts/languages.js
🧰 Additional context used
🪛 Biome (2.1.2)
style.css
[error] 4246-4246: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
width is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🔇 Additional comments (15)
manifest.json (1)
4-4: Version bump to 3.3.4 looks good.Straightforward version update aligned with the theming refactor and UI enhancements described in the PR.
manifest(firefox).json (1)
4-4: Version bump to 3.3.4 consistent across both manifests.Mirrors the Chrome manifest update for consistency across all supported platforms.
LICENSE (1)
634-638: Verify authorship and copyright transition.The LICENSE file updates the author from "XengShi" to "Prem" and shifts the copyright year range from 2023-2025 to 2024-2026. This represents a significant ownership/maintainership change. Please confirm this reflects the intended project governance and that the original author(s) have consented to this transition.
Also applies to: 658-658
README.md (1)
290-299: LGTM!The privacy commitments clearly communicate the extension's data handling practices. The copyright year update is consistent with the LICENSE file changes.
style.css (4)
4237-4248: Static analysis false positive - intentional vendor prefixes.The duplicate
widthproperty reported by Biome is intentional here. The code uses-webkit-fill-availablefor Chrome/Safari and-moz-availablefor Firefox as progressive enhancement. This is a standard cross-browser compatibility pattern.
3943-3966: LGTM!The modal system is well-structured with proper layering, smooth animations, and accessibility considerations (ESC key handling is wired in the JS). The fadeIn/slideUp animations provide good UX.
350-351: Verify intentional hardcoding of color values.The change from
var(--whitishColor-blue)towhitereduces theming flexibility. This appears intentional for ensuring contrast on dark backgrounds, but please verify this doesn't cause issues when users apply custom themes where the background might be light.
652-688: Verify bookmark button positioning calculation.The sidebar width was increased to
420pxbut the rotated button position is set to450px. This 30px difference may be intentional for spacing, but ensure this doesn't create visual gaps or overlap issues when the sidebar is open.scripts/theme.js (4)
794-812: LGTM!The throttle implementation is correctly implemented using an arrow function class property for proper
thisbinding, with appropriate closure handling for timing variables.
7-37: Well-structured ThemeManager class.The centralization of theme logic into a class with clear storage keys, color variable definitions, and system theme detection is a solid refactor. The separation of concerns between theme modes, color application, and modal handling improves maintainability.
814-815: The script is loaded with thedeferattribute in index.html (line 26), which ensures the DOM is fully parsed before execution. The initialization at lines 814-815 is correct and the comment accurately reflects this behavior. No changes needed.Likely an incorrect or invalid review comment.
585-587: No action needed.closeMenuBar()is properly defined at global scope inscripts/menu-shortcut-page.jsand will be available whenopenModal()executes due to the load order inindex.html(menu-shortcut-page.js loads before theme.js). The defensive check is unnecessary.Likely an incorrect or invalid review comment.
index.html (3)
3-3: LGTM!Copyright year update is appropriate for the new year.
1197-1203: LGTM!The reusable SVG symbol pattern is well-implemented. Consolidating the adjustment icon into a shared symbol promotes maintainability and reduces code duplication.
1278-1280: LGTM!The adjustment icon is correctly referenced using the
<use>element, successfully replacing inline SVG duplication with a shared symbol reference.Also applies to: 1341-1343
| } else if (customVars) { | ||
| const vars = JSON.parse(customVars); | ||
| const primaryColor = vars['--darkColor-blue'] || '#4382EC'; | ||
| colorPickerLabel.style.borderColor = ''; | ||
| colorEditorBtn.style.borderColor = primaryColor; | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON.parse.
Unlike loadStoredTheme() which wraps JSON.parse in try-catch, this method parses stored JSON without protection. If the stored data is corrupted, this will throw an unhandled exception.
🔎 Proposed fix
} else if (customVars) {
- const vars = JSON.parse(customVars);
- const primaryColor = vars['--darkColor-blue'] || '#4382EC';
- colorPickerLabel.style.borderColor = '';
- colorEditorBtn.style.borderColor = primaryColor;
+ try {
+ const vars = JSON.parse(customVars);
+ const primaryColor = vars['--darkColor-blue'] || '#4382EC';
+ colorPickerLabel.style.borderColor = '';
+ colorEditorBtn.style.borderColor = primaryColor;
+ } catch (e) {
+ console.error('Failed to parse custom variables:', e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (customVars) { | |
| const vars = JSON.parse(customVars); | |
| const primaryColor = vars['--darkColor-blue'] || '#4382EC'; | |
| colorPickerLabel.style.borderColor = ''; | |
| colorEditorBtn.style.borderColor = primaryColor; | |
| } | |
| } else if (customVars) { | |
| try { | |
| const vars = JSON.parse(customVars); | |
| const primaryColor = vars['--darkColor-blue'] || '#4382EC'; | |
| colorPickerLabel.style.borderColor = ''; | |
| colorEditorBtn.style.borderColor = primaryColor; | |
| } catch (e) { | |
| console.error('Failed to parse custom variables:', e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In scripts/theme.js around lines 509 to 514, the code calls
JSON.parse(customVars) without protection which will throw on malformed stored
data; wrap JSON.parse in a try-catch, on failure fall back to an empty object or
defaults (so primaryColor uses '#4382EC'), optionally remove or reset the
corrupted storage entry and log the error to console for debugging, and then
continue without applying broken values so the UI remains stable.
1 participant
📌 Description
🎨 Visual Changes (Screenshots / Videos)
Preview
🔗 Related Issues
✅ Checklist
Customizable Theme Implementation
This PR introduces a comprehensive customizable theme system, refactoring the existing theme handling from scattered global interactions into a centralized, object-oriented architecture. The implementation spans UI, styling, logic, and configuration layers.
Major Changes
Theme Management System (scripts/theme.js)
ThemeManagerclass to replace ad-hoc DOM/storage interactions with structured theme managementgenerateDarkModeColors()andtransformColorForDark()STORAGE_KEYSfor theme, custom colors, custom variables, and loading colorUser Interface (index.html)
colorModal) with dedicated controls for editing theme colors (Save, Reset actions)pickerContainerStyling (style.css)
#darkThemeto#blackThemeselectorcolorModal,modalContent, and related UI scaffoldingfadeIn,slideUp,invalid-shake) for modal interactionsLocalization & Configuration
rangColortranslation key fromlocales/en.jsandscripts/languages.jsImplementation Details
The new
ThemeManagerclass provides:Code Quality Impact
This refactor consolidates previously scattered theme logic into a single, maintainable class, improving code organization and enabling future theme customization features.