feat: implement ThemeHelper class to centralize theme usage#2131
Conversation
…improve color handling across various pages
… consistency across multiple pages
… consistency across multiple pages
… in DefaultPageLayout.php to enhance clarity while maintaining backwards compatibility
…or and remove redundant checks in theme methods
…s to streamline code
WalkthroughThe pull request refactors theme-related logic across multiple modules by replacing manual string comparisons and array checks with method calls on a new and centralized Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Page Controller
participant TH as ThemeHelper
participant FS as File System
Page->>TH: new ThemeHelper($theme)
TH->>TH: initWithCurrentThemeSetting($theme)
Page->>TH: getThemeName(), isTopNavTheme(), isLightTheme(), etc.
TH-->>Page: Returns theme attributes
Page->>Page: Render UI components based on theme
TH->>FS: (Optional) getThemesFromFileSystem() for available themes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
emhttp/plugins/dynamix.vm.manager/VMSettings.page (1)
95-95: Good refactoring with a minor documentation suggestion.This change follows the same pattern seen throughout the codebase, using
ThemeHelper->isLightTheme()for theme detection. For consistency with other files, consider adding the comment// $themeHelper set in DefaultPageLayout.phpto help future developers understand where this dependency comes from.emhttp/plugins/dynamix/include/ThemeHelper.php (3)
3-6: Consider implementing the TODO for dynamic theme detection.The TODO comment correctly identifies a limitation in the current implementation with hardcoded theme values. In the future, extracting theme colors dynamically would make the class more maintainable and adaptable to new themes.
109-119: Robust theme detection from filesystem.The static method for theme detection is well-implemented, but its dependency on a specific directory structure could make it brittle if the structure changes. Consider adding error handling for cases where the directory doesn't exist or has unexpected content.
public static function getThemesFromFileSystem(string $docroot): array { $themes = []; - $themeFiles = glob("$docroot/webGui/styles/themes/*.css"); + $themePath = "$docroot/webGui/styles/themes"; + if (!is_dir($themePath)) { + error_log("Theme directory not found: $themePath"); + return $themes; + } + $themeFiles = glob("$themePath/*.css"); foreach ($themeFiles as $themeFile) { $themeName = basename($themeFile, '.css'); $themes[] = $themeName; } return $themes; }
42-47: Consider adding a fallback theme.The constructor throws an exception if no theme is provided. Consider adding a fallback to a default theme instead of throwing an exception, which could make the class more resilient in edge cases.
public function __construct(?string $theme = null) { if ($theme === null) { - throw new \RuntimeException(self::INIT_ERROR); + $theme = self::THEME_BLACK; // Use a default theme + error_log(self::INIT_ERROR . " Using default theme: $theme"); } $this->initWithCurrentThemeSetting($theme); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
emhttp/plugins/dynamix.docker.manager/DockerContainers.page(3 hunks)emhttp/plugins/dynamix.docker.manager/DockerSettings.page(1 hunks)emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php(1 hunks)emhttp/plugins/dynamix.gui.search/gui_search.page(4 hunks)emhttp/plugins/dynamix.vm.manager/VMMachines.page(3 hunks)emhttp/plugins/dynamix.vm.manager/VMSettings.page(1 hunks)emhttp/plugins/dynamix.vm.manager/include/VMedit.php(1 hunks)emhttp/plugins/dynamix/DashStats.page(4 hunks)emhttp/plugins/dynamix/DisplaySettings.page(6 hunks)emhttp/plugins/dynamix/include/.login.php(4 hunks)emhttp/plugins/dynamix/include/.set-password.php(2 hunks)emhttp/plugins/dynamix/include/Boot.php(2 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout.php(9 hunks)emhttp/plugins/dynamix/include/ThemeHelper.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
emhttp/plugins/dynamix/include/ThemeHelper.php (1)
isLightTheme(87-89)
emhttp/plugins/dynamix.vm.manager/include/VMedit.php (1)
emhttp/plugins/dynamix/include/ThemeHelper.php (1)
getThemeName(71-73)
emhttp/plugins/dynamix/include/.login.php (1)
emhttp/plugins/dynamix/include/ThemeHelper.php (2)
ThemeHelper(2-120)isDarkTheme(83-85)
emhttp/plugins/dynamix/include/Boot.php (1)
emhttp/plugins/dynamix/include/ThemeHelper.php (3)
ThemeHelper(2-120)getThemeName(71-73)getThemeHtmlClass(91-93)
emhttp/plugins/dynamix/include/.set-password.php (1)
emhttp/plugins/dynamix/include/ThemeHelper.php (2)
ThemeHelper(2-120)isDarkTheme(83-85)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (1)
emhttp/plugins/dynamix/include/ThemeHelper.php (6)
ThemeHelper(2-120)getThemeName(71-73)isTopNavTheme(75-77)isSidebarTheme(79-81)getThemeHtmlClass(91-93)updateDockerLogColor(99-101)
🔇 Additional comments (35)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
322-322: Good refactoring to centralize theme handling.The change nicely replaces direct theme checking with the
ThemeHelperclass methodisLightTheme(). This improves maintainability by centralizing theme logic and makes the code more readable.emhttp/plugins/dynamix.docker.manager/DockerSettings.page (1)
126-126: Good consistent refactoring for theme handling.This change follows the same pattern used elsewhere in the codebase, leveraging the
ThemeHelperclass'sisLightTheme()method instead of direct theme checks. This improves code maintainability through centralization of theme logic.emhttp/plugins/dynamix/DisplaySettings.page (1)
6-7: Good code standardization and theme logic centralization.The changes improve the codebase in two ways:
- Code style improvements: Standardizing PHP opening tags (
<?php) and adding consistent spacing in translation function calls- Functional improvement: Using
ThemeHelpermethods for theme handling (lines 254-256)Both aspects contribute to a more maintainable and consistent codebase.
Also applies to: 18-18, 132-133, 151-157, 163-169, 175-180, 185-192, 196-197, 204-205, 212-213, 220-221, 226-227, 234-235, 240-241, 246-247, 254-256, 261-267, 285-286, 291-292, 312-313, 319-320, 327-328
emhttp/plugins/dynamix/include/.login.php (4)
239-242: Implementation of ThemeHelper looks good.The code properly integrates the ThemeHelper class to centralize theme detection logic. This is a good refactoring that improves code organization and maintainability.
280-281: Theme conditionals updated correctly.The background and text color conditional statements now properly use the new
$isDarkThemevariable instead of direct theme checks. This aligns with the centralized theme management approach.
365-365: Background color handling improved.The background color for the login section now correctly uses the centralized theme variable, maintaining consistent styling while improving code organization.
457-457: Media query theme implementation is consistent.The mobile responsive background color now properly uses the centralized theme variable, ensuring consistency across different screen sizes.
emhttp/plugins/dynamix.gui.search/gui_search.page (4)
35-35: Sidebar theme detection improved.The code now properly uses
$themeHelper->isSidebarTheme()for determining when to apply hover events, which is more maintainable and consistent than the previous approach.
54-56: Top navigation theme check updated correctly.The keydown event handler now properly uses
$themeHelper->isTopNavTheme()for conditional logic, aligning with the centralized theme detection approach.
66-73: Search function theme logic improved.The
gui_searchfunction now properly differentiates between top navigation and sidebar themes using the ThemeHelper methods, making the code more maintainable and readable.
89-95: Search box closing function updated correctly.The
closeSearchBoxfunction now properly handles theme-specific behaviors using the ThemeHelper methods, ensuring consistent UI behavior across different themes.emhttp/plugins/dynamix.vm.manager/include/VMedit.php (1)
25-25: Theme name retrieval improved.The switch statement now properly uses
$themeHelper->getThemeName()instead of directly accessing the theme from the display array. This centralizes theme-related logic and improves maintainability.emhttp/plugins/dynamix/include/.set-password.php (2)
43-45: ThemeHelper integration looks good.The code correctly integrates the ThemeHelper class to determine the theme, following the same pattern used in other files. This centralized approach improves maintainability.
83-89: CSS variables updated correctly.The CSS variables now properly use the
$isDarkThemevariable from ThemeHelper, ensuring consistent styling across the application while improving code organization.emhttp/plugins/dynamix.docker.manager/DockerContainers.page (2)
24-25: Good use of ThemeHelper for theme-based UI adjustments.The code now uses the
$themeHelper->isTopNavTheme()method instead of directly checking theme names, which centralizes theme-related logic and improves maintainability.
85-88: ThemeHelper properly used for UI element styling based on theme.The conditional styling for the LockButton is now handled through the ThemeHelper class, making the code more maintainable and consistent with the theme management approach used throughout the application.
Also applies to: 102-105
emhttp/plugins/dynamix/DashStats.page (3)
53-53: Docker log color update properly handled by ThemeHelper.Using
$themeHelper->updateDockerLogColor($docroot)encapsulates the theme-specific logic for updating the Docker log color, which is more maintainable than the previous direct approach.
194-194: Theme-based switch statement now uses ThemeHelper.Using
$themeHelper->getThemeName()instead of direct access to the theme variable centralizes theme management and improves code consistency.
1556-1559: Theme-specific UI adjustments properly handled by ThemeHelper.The LockButton styling logic is now consistently managed through the ThemeHelper class, making the code more maintainable and aligned with the centralized theme management approach.
Also applies to: 1588-1591
emhttp/plugins/dynamix/include/Boot.php (2)
27-31: Well-structured initialization of ThemeHelper.The code properly initializes the ThemeHelper class and retrieves the necessary theme properties, following the same pattern used in DefaultPageLayout.php.
46-46: Theme stylesheet link correctly uses ThemeHelper-provided theme name.Using
$themeNamefrom ThemeHelper ensures consistency in how the theme is referenced throughout the application.emhttp/plugins/dynamix/include/DefaultPageLayout.php (7)
14-20: Well-structured ThemeHelper initialization with good backward compatibility.The code properly initializes the ThemeHelper and maintains backward compatibility by keeping the original variables. The helpful comment explains why these variables are retained.
62-62: CSS media queries now use ThemeHelper for theme-specific styling.Replacing direct theme checks with
$themeHelper->isTopNavTheme()makes the CSS generation more maintainable and consistent with the overall theme management approach.Also applies to: 66-67
81-87: Theme-specific styling now uses consistent ThemeHelper methods.The conditional styling for navigation elements is now handled through the ThemeHelper class, which improves code consistency and maintainability.
100-104: Sidebar theme detection properly handled by ThemeHelper.Using
$themeHelper->isSidebarTheme()for conditional DOM structure creation ensures consistent theme-based layout decisions throughout the application.Also applies to: 714-714, 755-755
728-728: UI element attributes correctly handled based on theme type.The code now consistently uses ThemeHelper methods to determine UI element properties based on the current theme.
Also applies to: 744-744
762-762: Good documentation with TODO for future improvement.The comment about adding a deprecation notice is helpful for future maintenance work on theme-related functionality.
1094-1099: Scroll behavior correctly adjusted based on theme type.Using ThemeHelper for determining scroll-related UI behavior ensures consistent handling of fixed/absolute positioning across different themes.
emhttp/plugins/dynamix.vm.manager/VMMachines.page (4)
23-23: Good addition of centralized theme management.Using the theme helper to get theme name is a good practice and aligns with the centralization of theme-related functionality.
26-29: Improved theme condition handling.Replacing direct string comparisons with abstracted methods like
isLightTheme()makes the code more maintainable and readable. The conditional color assignments are now cleaner and follow a consistent pattern.
407-410: Clean refactoring of theme-dependent UI logic.Using the
isTopNavTheme()method for conditional rendering of UI elements based on theme type improves code organization and maintainability.
430-433: Consistent use of theme helper methods.The use of
isTopNavTheme()here maintains consistency with the earlier implementation, making the code more readable and maintainable.emhttp/plugins/dynamix/include/ThemeHelper.php (3)
2-120: Well-structured ThemeHelper class with good organization and documentation.The ThemeHelper class effectively centralizes theme-related functionality with clear method names and organization. The documentation is good and the method responsibilities are well-defined.
56-56: Potential issue with theme name extraction.Using
strtok($theme, '-')to extract the theme name may not handle all theme name formats correctly, especially if theme names contain hyphens as part of their actual name rather than just as separators.Consider adding test cases for different theme name formats to ensure consistent behavior.
99-101:Details
❓ Verification inconclusive
Security consideration in exec command.
Direct execution of shell commands can pose security risks if inputs aren't properly sanitized, though in this case
$this->fgcoloris defined internally so the risk is minimal. Still, consider using more PHP-native approaches for file manipulation when possible.
🏁 Script executed:
#!/bin/bash # Check if the color is properly sanitized before being used in the sed command echo "Current fgcolor format: ${$this->fgcolor}" grep -r "updateDockerLogColor" --include="*.php" . | grep -v "ThemeHelper.php"Length of output: 308
Action Required: Review exec usage for file manipulation security and maintainability
The current implementation in
ThemeHelper.php(lines 99–101) calls an externalsedcommand viaexec(). Since$this->fgcoloris defined internally and its value isn’t derived from user input, the immediate security risk is minimal. However, directly executing shell commands can introduce maintainability and potential security concerns in the long term.
- Observation: The usage of
exec()for performing file modifications may be less robust compared to PHP’s native file operations.- Suggestion: Consider refactoring to use PHP-native functions (e.g.,
file_get_contents(),preg_replace(), andfile_put_contents()) for updating the log file. This would improve code portability and clarity.- Note: The verification output confirms that
updateDockerLogColor()is used as expected (e.g., viaDefaultPageLayout.php), and no external input affects$this->fgcolor.Please review the implementation to determine if a PHP-native approach might enhance overall security and maintainability.
|
Nitpicks for Nitpicks for
It's a todo for a reason Señor Rabbit
If a theme isn't passed to the constructor there's a much larger issue at hand with the VARs config not being parsed. Not implementing a fallback until larger theme changes are made. |
3 participants
Summary by CodeRabbit
Refactor
Style