[WEB-2532] fix: custom theme mutation logic #5685
Conversation
WalkthroughThe changes across the components focus on simplifying the theme application logic by removing unnecessary dependencies on the DOM. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
web/app/profile/appearance/page.tsx (1)
Line range hint
1-89: Summary of changes and next stepsThe changes made to the
ProfileAppearancePagecomponent, specifically in theapplyThemeChangefunction, align with the PR objectives of improving the performance of the theme mutation logic. The removal of the custom theme element parameter from theapplyThemefunction call simplifies the logic and potentially enhances efficiency.To ensure the effectiveness of these changes:
- Verify that the
applyThemefunction has been updated in its implementation file to work with the new calling convention.- Consider adding unit tests or updating existing ones to cover the new behavior of the
applyThemeChangefunction.- If possible, perform performance measurements to quantify the improvement gained from this change.
To further improve the theme application process:
- Consider memoizing the
applyThemeChangefunction to prevent unnecessary re-renders.- Evaluate if the theme palette parsing logic (
theme?.palette !== ",,,,") could be moved to a utility function for better reusability and testability.web/helpers/theme.helper.ts (4)
64-64: ApprovethemeElementselection and suggest minor improvement.The introduction of
themeElementcentralizes the theme application to a specific element identified by thedata-themeattribute. This approach is more robust and aligns with the PR objectives.Consider adding a comment explaining the purpose of this selection for better code readability:
+ // Select the first element with a data-theme attribute for applying the theme const themeElement = window.document?.querySelector<HTMLElement>("[data-theme]");
84-88: Approve CSS property setting changes and suggest optimization.The changes to set CSS properties on
themeElementinstead ofdomare consistent with the new approach and maintain the existing functionality. The use of optional chaining is a good practice for handling potential null values.Consider optimizing the performance by caching the
styleproperty ofthemeElement:+ const themeStyle = themeElement?.style; + if (!themeStyle) return; - themeElement?.style.setProperty(`--color-background-${shade}`, bgRgbValues); - themeElement?.style.setProperty(`--color-text-${shade}`, textRgbValues); - themeElement?.style.setProperty(`--color-primary-${shade}`, primaryRgbValues); - themeElement?.style.setProperty(`--color-sidebar-background-${shade}`, sidebarBackgroundRgbValues); - themeElement?.style.setProperty(`--color-sidebar-text-${shade}`, sidebarTextRgbValues); + themeStyle.setProperty(`--color-background-${shade}`, bgRgbValues); + themeStyle.setProperty(`--color-text-${shade}`, textRgbValues); + themeStyle.setProperty(`--color-primary-${shade}`, primaryRgbValues); + themeStyle.setProperty(`--color-sidebar-background-${shade}`, sidebarBackgroundRgbValues); + themeStyle.setProperty(`--color-sidebar-text-${shade}`, sidebarTextRgbValues); // ... (apply similar changes to other setProperty calls)This optimization reduces the number of property accesses and null checks, potentially improving performance, especially in the loop where these properties are set multiple times.
Also applies to: 93-97, 104-104
Line range hint
107-121: UpdateunsetCustomCssVariablesto matchapplyThemechanges.The
unsetCustomCssVariablesfunction hasn't been updated to match the changes made inapplyTheme. It still uses[data-theme='custom']to select the DOM element, which is inconsistent with the new approach and could lead to issues where custom CSS variables are not properly unset.Update the
unsetCustomCssVariablesfunction to use the same selector asapplyTheme:export const unsetCustomCssVariables = () => { + const themeElement = window.document?.querySelector<HTMLElement>("[data-theme]"); + if (!themeElement) return; for (let i = 10; i <= 900; i >= 100 ? (i += 100) : (i += 10)) { - const dom = document.querySelector<HTMLElement>("[data-theme='custom']"); - dom?.style.removeProperty(`--color-background-${i}`); - dom?.style.removeProperty(`--color-text-${i}`); - dom?.style.removeProperty(`--color-border-${i}`); - dom?.style.removeProperty(`--color-primary-${i}`); - dom?.style.removeProperty(`--color-sidebar-background-${i}`); - dom?.style.removeProperty(`--color-sidebar-text-${i}`); - dom?.style.removeProperty(`--color-sidebar-border-${i}`); - dom?.style.removeProperty("--color-scheme"); + themeElement.style.removeProperty(`--color-background-${i}`); + themeElement.style.removeProperty(`--color-text-${i}`); + themeElement.style.removeProperty(`--color-border-${i}`); + themeElement.style.removeProperty(`--color-primary-${i}`); + themeElement.style.removeProperty(`--color-sidebar-background-${i}`); + themeElement.style.removeProperty(`--color-sidebar-text-${i}`); + themeElement.style.removeProperty(`--color-sidebar-border-${i}`); } + themeElement.style.removeProperty("--color-scheme"); };This change ensures consistency with the
applyThemefunction and properly unsets all custom CSS variables.
Line range hint
1-128: Summary of changes and suggestionsThe changes in this file align well with the PR objectives of improving the theme mutation logic. The centralization of theme application to a specific element with the
data-themeattribute is a good approach. However, there are a few areas for improvement:
- Consider adding comments to explain the purpose of the
themeElementselection.- Optimize performance in the
applyThemefunction by caching thestyleproperty ofthemeElement.- Update the
unsetCustomCssVariablesfunction to use the same selector asapplyThemefor consistency.These changes will further enhance the robustness and efficiency of the theme helper functions.
To further improve the theme management system, consider the following architectural suggestions:
- Implement a Theme Manager class that encapsulates all theme-related operations, including applying and unsetting themes.
- Use a configuration object or enum to define theme-related constants (e.g., shade values, CSS variable names) to improve maintainability.
- Implement a caching mechanism for calculated shades to avoid redundant calculations when applying the same theme multiple times.
These architectural improvements would make the theme system more modular, maintainable, and performant.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- web/app/profile/appearance/page.tsx (1 hunks)
- web/core/lib/wrappers/store-wrapper.tsx (1 hunks)
- web/helpers/theme.helper.ts (2 hunks)
Additional comments not posted (3)
web/core/lib/wrappers/store-wrapper.tsx (1)
45-52: 🛠️ Refactor suggestionApproved: Theme application logic simplified
The changes successfully simplify the theme application logic, which aligns with the PR objectives to improve performance. The removal of the
domstate and its associated logic (as mentioned in the AI summary) contributes to this simplification.However, there's a potential for further improvement:
Consider removing the
currentTheme === "custom"check if theapplyThemefunction has been updated to work with all themes as mentioned in the PR objectives. This could further simplify the logic:- if (currentTheme === "custom" && currentThemePalette) { - applyTheme( - currentThemePalette !== ",,,," ? currentThemePalette : "#0d101b,#c5c5c5,#3f76ff,#0d101b,#c5c5c5", - false - ); - } else unsetCustomCssVariables(); + if (currentThemePalette) { + applyTheme( + currentThemePalette !== ",,,," ? currentThemePalette : "#0d101b,#c5c5c5,#3f76ff,#0d101b,#c5c5c5", + false + ); + } else { + unsetCustomCssVariables(); + }To ensure the changes align with the PR objectives, let's verify the implementation of the
applyThemefunction:This will help us confirm if the
applyThemefunction has been updated to select the theme element using[data-theme]instead of[data-theme="custom"]as mentioned in the PR objectives.Verification successful
Verified: applyTheme function aligns with PR objectives
The
applyThemefunction inweb/helpers/theme.helper.tscorrectly selects the theme element using[data-theme]instead of[data-theme="custom"]. This change simplifies the theme application logic and aligns with the PR objectives to improve performance.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the applyTheme function # Test: Search for the applyTheme function definition rg --type typescript -A 10 'function applyTheme'Length of output: 85
Script:
#!/bin/bash # Description: Verify the implementation of the applyTheme function with correct file type # Test: Search for the applyTheme function definition in TypeScript React files rg --type typescriptreact -A 10 'function applyTheme'Length of output: 95
Script:
#!/bin/bash # Description: Verify the implementation of the applyTheme function by searching within .tsx files # Test: Search for the applyTheme function definition in .tsx files rg -g "*.tsx" 'function\s+applyTheme|const\s+applyTheme\s*='Length of output: 62
Script:
#!/bin/bash # Description: Search for the applyTheme function in both .ts and .tsx files with flexible patterns rg -g "*.ts*" 'function\s+applyTheme|const\s+applyTheme\s*='Length of output: 163
Script:
#!/bin/bash # Description: Retrieve the implementation of the applyTheme function from theme.helper.ts # Test: Extract the applyTheme function definition and its contents rg 'export\s+const\s+applyTheme\s*=' -A 20 web/helpers/theme.helper.tsLength of output: 1364
web/app/profile/appearance/page.tsx (1)
55-57: Approve changes with verification requestThe modifications to the
applyThemefunction call align with the PR objectives of improving performance. The logic for applying custom themes remains robust, handling empty palettes and maintaining the correct conditions for application.To ensure the changes are fully effective and error-free:
- Please confirm that the
applyThemefunction has been updated elsewhere to work without the custom theme element parameter.- Consider measuring the performance impact of this change to quantify the improvement.
Run the following script to verify the
applyThemefunction implementation:This script will help us confirm that the
applyThemefunction has been properly updated and that there are no remaining instances of the old implementation using[data-theme="custom"].web/helpers/theme.helper.ts (1)
62-62: Approve function signature change and verify usage.The removal of the
domparameter from theapplyThemefunction signature aligns with the PR objective of improving the theme mutation logic. This change makes the function more focused and less dependent on external DOM elements.To ensure this change doesn't break existing code, please run the following script to verify the function usage across the codebase:
Verification successful
Function signature change verified successfully.
All existing calls to
applyThemematch the updated signature without thedomparameter. No issues found with the current usage across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `applyTheme` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'applyTheme\('Length of output: 77
Script:
#!/bin/bash # Description: Verify all function calls to `applyTheme` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type=ts -A 5 'applyTheme\('Length of output: 869
* fix: custom theme mutation logic * chore: update querySelector element
Problem:
MutationObserverlogic to update the custom theme is too expensive and unnecessary.Solution:
Update the
applyThemefunction to select the themeElement with the attribute[data-theme]instead of[data-theme="custom"]to make sure themeElement always exists.Plane issue: WEB-2532
Summary by CodeRabbit
New Features
Bug Fixes