Skip to content

feat(new-webui): Add theme mode support with a toggle menu. - #938

Open
junhaoliao wants to merge 10 commits into
y-scope:mainfrom
junhaoliao:antd-theme-toggle
Open

feat(new-webui): Add theme mode support with a toggle menu.#938
junhaoliao wants to merge 10 commits into
y-scope:mainfrom
junhaoliao:antd-theme-toggle

Conversation

@junhaoliao

@junhaoliao junhaoliao commented May 28, 2025

Copy link
Copy Markdown
Member

This PR depends on #937 .

Description

  1. Add theme mode support.
    1. Persist theme mode with localStorage.
  2. Add @mui/icons-material as a dependency.
  3. Add a menu item in the sider menu for theme switching.
    image

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  1. Verified that theme mode switching by clicking the newly added menu item works.
  2. Verified that theme mode setting is persisted across page refreshes.
  3. Verified that the "System" theme mode works by changing the OS's theme mode between dark / light.

Summary by CodeRabbit

  • New Features
    • Introduced a theme toggle menu item, allowing users to switch between system, dark, and light modes.
    • Theme selection now persists across sessions and adapts dynamically to system preferences.
  • Improvements
    • Updated sidebar layout for improved structure and usability.
    • Enhanced theme management for a more consistent and dynamic appearance.
  • Chores
    • Added new icon library dependency for richer UI elements.

@coderabbitai

coderabbitai Bot commented May 28, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

This change introduces a dynamic theming system to the log-viewer web UI client. It adds a global theme store using Zustand, a theme toggle menu item, theme mode enumeration, utility functions for theme detection, and updates the layout and theme provider to support runtime theme switching based on user preference or system settings.

Changes

File(s) Change Summary
client/package.json Added @mui/icons-material dependency.
src/AntdApp.tsx Switched theme config in ConfigProvider to use dynamic algorithm from global theme store via useThemeStore.
src/components/Layout/MainLayout.module.css Modified .mainLayout for fixed height and overflow; added .siderContainer (flex column for sidebar) and .siderTopMenu (flex grow).
src/components/Layout/MainLayout.tsx Refactored sidebar: removed internal collapse state, split menu into top and bottom groups, added ThemeToggleMenuItem, restructured layout for new sidebar container and menu arrangement.
src/components/Layout/ThemeToggleMenuItem.tsx Added new component for toggling theme mode (system/dark/light) with icon and label, updating global theme store on click.
src/theme/index.tsx Changed ThemeConfig import path, added and exported THEME_MODE enum and useThemeStore hook.
src/theme/themeStore.ts Introduced Zustand store (useThemeStore) for theme mode and algorithm state, with persistence and custom merge for rehydration.
src/theme/typings.ts Added and exported THEME_MODE enum for theme mode constants.
src/theme/utils.ts Added utility functions: getSystemAlgorithm (detects system theme) and getAlgorithm (returns correct Antd algorithm for mode).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ThemeToggleMenuItem
    participant useThemeStore
    participant AntdApp
    participant ConfigProvider

    User->>ThemeToggleMenuItem: Clicks theme toggle menu item
    ThemeToggleMenuItem->>useThemeStore: setMode(nextMode)
    useThemeStore-->>ThemeToggleMenuItem: Updates mode and algorithm
    useThemeStore-->>AntdApp: Triggers state update
    AntdApp->>ConfigProvider: Passes updated theme algorithm
    ConfigProvider-->>User: UI updates to new theme
Loading

Possibly related PRs

Suggested reviewers

  • junhaoliao

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

junhaoliao and others added 5 commits May 28, 2025 18:09
# Conflicts:
#	components/log-viewer-webui/client/src/AntdApp.tsx
#	components/log-viewer-webui/client/src/components/Layout/MainLayout.module.css
#	components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx
@junhaoliao
junhaoliao marked this pull request as ready for review May 31, 2025 04:08
@junhaoliao
junhaoliao requested a review from a team as a code owner May 31, 2025 04:08
@junhaoliao
junhaoliao requested a review from kirkrodrigues May 31, 2025 04:08
@junhaoliao

Copy link
Copy Markdown
Member Author

Note this PR adds @mui/icons-material as a dependency. If we agree with the change, we should also consider replacing all antd icons with @mui/icons-material "outlined" icons for a more streamlined appearance.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b4d73d4 and dd8f898.

⛔ Files ignored due to path filters (1)
  • components/log-viewer-webui/client/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (9)
  • components/log-viewer-webui/client/package.json (1 hunks)
  • components/log-viewer-webui/client/src/AntdApp.tsx (2 hunks)
  • components/log-viewer-webui/client/src/components/Layout/MainLayout.module.css (2 hunks)
  • components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx (2 hunks)
  • components/log-viewer-webui/client/src/components/Layout/ThemeToggleMenuItem.tsx (1 hunks)
  • components/log-viewer-webui/client/src/theme/index.tsx (2 hunks)
  • components/log-viewer-webui/client/src/theme/themeStore.ts (1 hunks)
  • components/log-viewer-webui/client/src/theme/typings.ts (1 hunks)
  • components/log-viewer-webui/client/src/theme/utils.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

  • components/log-viewer-webui/client/src/theme/typings.ts
  • components/log-viewer-webui/client/src/theme/index.tsx
  • components/log-viewer-webui/client/src/components/Layout/ThemeToggleMenuItem.tsx
  • components/log-viewer-webui/client/src/theme/utils.ts
  • components/log-viewer-webui/client/src/theme/themeStore.ts
  • components/log-viewer-webui/client/src/AntdApp.tsx
  • components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx
🧠 Learnings (1)
components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx (2)
Learnt from: davemarco
PR: y-scope/clp#797
File: components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx:2-5
Timestamp: 2025-04-08T22:32:05.366Z
Learning: In this codebase using React Router v7.4.1, components should be imported directly from "react-router" (e.g., `import { Link, Outlet } from "react-router";`) rather than from "react-router-dom" as was common in previous versions of React Router.
Learnt from: davemarco
PR: y-scope/clp#797
File: components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx:2-5
Timestamp: 2025-04-08T22:32:05.367Z
Learning: React Router v7.4.1 uses a different package structure than previous versions. Components can be imported directly from "react-router" (e.g., `import { Link, Outlet } from "react-router";`) rather than from "react-router-dom" as was common in previous versions. Alternatively, DOM-specific components can also be imported using deep imports like `import { Link } from "react-router/dom";`.
🧬 Code Graph Analysis (4)
components/log-viewer-webui/client/src/theme/typings.ts (1)
components/log-viewer-webui/client/src/theme/index.tsx (1)
  • THEME_MODE (22-22)
components/log-viewer-webui/client/src/components/Layout/ThemeToggleMenuItem.tsx (2)
components/log-viewer-webui/client/src/theme/index.tsx (2)
  • THEME_MODE (22-22)
  • useThemeStore (23-23)
components/log-viewer-webui/client/src/theme/typings.ts (1)
  • THEME_MODE (8-8)
components/log-viewer-webui/client/src/theme/utils.ts (1)
components/log-viewer-webui/client/src/theme/typings.ts (1)
  • THEME_MODE (8-8)
components/log-viewer-webui/client/src/AntdApp.tsx (1)
components/log-viewer-webui/client/src/theme/index.tsx (1)
  • useThemeStore (23-23)
🔇 Additional comments (18)
components/log-viewer-webui/client/package.json (1)

20-20:

Details

❓ Verification inconclusive

Verify the package version is current and secure.

Ensure the specified version of @mui/icons-material exists and check for any associated security advisories.


🏁 Script executed:

#!/bin/bash
# Description: Verify @mui/icons-material version 7.1.0 exists and check for security advisories

# Check if version 7.1.0 exists on npm
npm view @mui/icons-material@7.1.0 version 2>/dev/null || echo "Version 7.1.0 not found"

# Get latest version
npm view @mui/icons-material version

# Check for security advisories
npm audit --audit-level=moderate --package-lock-only 2>/dev/null || echo "No package-lock.json found for audit"

Length of output: 233


Missing audit results due to absent lock file

  • Verified @mui/icons-material@7.1.0 exists and is the latest published version.
  • No package-lock.json found in the repo, so npm audit couldn’t run.
  • Please generate a lock file (e.g. run npm install or yarn install) and execute npm audit or yarn audit to confirm there are no vulnerabilities.
components/log-viewer-webui/client/src/theme/typings.ts (1)

1-8: Well-structured enum implementation.

The THEME_MODE enum is cleanly implemented with appropriate string values that will be useful for debugging and API communication. The naming convention and export pattern follow TypeScript best practices.

components/log-viewer-webui/client/src/theme/index.tsx (2)

1-4: Improved import specificity and module organization.

The more specific import path for ThemeConfig and the addition of theme store and enum imports create a well-organized barrel export pattern for the theme module.


21-24: Good centralization of theme exports.

The export structure properly centralizes all theme-related functionality, making it easier for consumers to import what they need from a single location.

components/log-viewer-webui/client/src/AntdApp.tsx (3)

6-6: Proper integration of theme store.

The import correctly brings in the theme store hook alongside the existing theme configuration.


17-17: Efficient theme store usage.

Destructuring only the needed algorithm property from the theme store is efficient and follows React best practices.


21-24:

Details

✅ Verification successful

Well-implemented dynamic theming.

The theme configuration properly spreads the existing THEME_CONFIG while dynamically overriding the algorithm property. This maintains backward compatibility while enabling theme switching functionality.

Verify that the theme store provides the correct algorithm values:


🏁 Script executed:

#!/bin/bash
# Description: Verify theme store implementation and algorithm values

# Check if themeStore.ts exists and examine its implementation
fd -t f "themeStore.ts" --exec cat {}

# Check for theme utilities that might provide algorithm values
fd -t f "utils.ts" -p "**/theme/**" --exec cat {}

Length of output: 1566


🏁 Script executed:

#!/bin/bash
# Locate and print the contents of utils.ts to verify getAlgorithm implementations
fd -t f utils.ts --exec sed -n '1,200p' {}

# Search project for definitions of getAlgorithm and getSystemAlgorithm
rg -n "getAlgorithm" -t ts
rg -n "getSystemAlgorithm" -t ts

Length of output: 16983


Approve dynamic theming implementation

I’ve verified that:

  • getSystemAlgorithm and getAlgorithm in components/log-viewer-webui/client/src/theme/utils.ts correctly map modes to Ant Design’s defaultAlgorithm and darkAlgorithm.
  • useThemeStore in components/log-viewer-webui/client/src/theme/themeStore.ts properly initializes and updates algorithm based on the selected mode and persisted state.

No further changes required.

components/log-viewer-webui/client/src/components/Layout/ThemeToggleMenuItem.tsx (1)

20-24: LGTM! Good use of immutable mappings.

The use of Object.freeze for the icon mapping ensures immutability and prevents accidental modifications at runtime. The mapping structure is clear and comprehensive.

components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx (3)

4-4: Correct React Router import usage.

The import of useLocation from "react-router" follows the correct pattern for React Router v7.4.1 as noted in the retrieved learnings.


25-33: Well-structured menu separation.

The separation of sidebar items into top (navigation) and bottom (utilities) sections creates a logical grouping and better user experience. The bottom menu items are appropriately structured as JSX elements rather than menu item objects.


62-67: Appropriate configuration for utility menu.

Setting selectable={false} and selectedKeys={[]} for the bottom menu is correct since theme toggle functionality shouldn't interfere with navigation state.

components/log-viewer-webui/client/src/components/Layout/MainLayout.module.css (2)

2-3: Consider potential scrolling implications.

The change from min-height: 100vh to height: 100vh with overflow: hidden creates a fixed viewport layout. Ensure this doesn't prevent users from accessing content if the viewport becomes too small.

Consider testing the layout behavior on smaller screen sizes to ensure content remains accessible.


13-18: Excellent flexbox layout for sidebar structure.

The flexbox container with justify-content: space-between perfectly supports the new top/bottom menu structure, ensuring the theme toggle stays at the bottom while navigation items remain at the top.

components/log-viewer-webui/client/src/theme/utils.ts (1)

23-31: Clean and logical theme mode handling.

The function correctly handles all theme modes with appropriate fallbacks. The logic is clear: SYSTEM delegates to system detection, DARK uses dark algorithm, and everything else (LIGHT) defaults to the default algorithm.

components/log-viewer-webui/client/src/theme/themeStore.ts (4)

1-9: LGTM! Clean and well-organized imports.

The imports are properly structured and include all necessary dependencies for the theme store implementation.


12-21: LGTM! Well-structured TypeScript interfaces.

The type definitions clearly separate values from actions and follow good TypeScript design patterns.


23-26: LGTM! Sensible default values.

Starting with system theme detection provides good user experience by respecting OS theme preferences.


56-56: LGTM! Standard export pattern.

The default export follows standard conventions for Zustand store hooks.

Comment on lines +43 to +52
const handleClick = useCallback(() => {
const {setMode} = useThemeStore.getState();
if (mode === THEME_MODE.SYSTEM) {
setMode(THEME_MODE.DARK);
} else if (mode === THEME_MODE.DARK) {
setMode(THEME_MODE.LIGHT);
} else {
setMode(THEME_MODE.SYSTEM);
}
}, [mode]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider improving maintainability of the cycling logic.

While the current cycling logic is correct, it could be more maintainable and less error-prone with a more structured approach.

Consider this refactor for better maintainability:

-    const handleClick = useCallback(() => {
-        const {setMode} = useThemeStore.getState();
-        if (mode === THEME_MODE.SYSTEM) {
-            setMode(THEME_MODE.DARK);
-        } else if (mode === THEME_MODE.DARK) {
-            setMode(THEME_MODE.LIGHT);
-        } else {
-            setMode(THEME_MODE.SYSTEM);
-        }
-    }, [mode]);
+    const handleClick = useCallback(() => {
+        const {setMode} = useThemeStore.getState();
+        const cycleOrder = [THEME_MODE.SYSTEM, THEME_MODE.DARK, THEME_MODE.LIGHT];
+        const currentIndex = cycleOrder.indexOf(mode);
+        const nextIndex = (currentIndex + 1) % cycleOrder.length;
+        setMode(cycleOrder[nextIndex]);
+    }, [mode]);

This approach makes it easier to modify the cycling order or add new theme modes in the future.

📝 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.

Suggested change
const handleClick = useCallback(() => {
const {setMode} = useThemeStore.getState();
if (mode === THEME_MODE.SYSTEM) {
setMode(THEME_MODE.DARK);
} else if (mode === THEME_MODE.DARK) {
setMode(THEME_MODE.LIGHT);
} else {
setMode(THEME_MODE.SYSTEM);
}
}, [mode]);
const handleClick = useCallback(() => {
const {setMode} = useThemeStore.getState();
const cycleOrder = [THEME_MODE.SYSTEM, THEME_MODE.DARK, THEME_MODE.LIGHT];
const currentIndex = cycleOrder.indexOf(mode);
const nextIndex = (currentIndex + 1) % cycleOrder.length;
setMode(cycleOrder[nextIndex]);
}, [mode]);
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/components/Layout/ThemeToggleMenuItem.tsx
around lines 43 to 52, the theme mode cycling logic is currently implemented
with multiple if-else statements. To improve maintainability and reduce errors,
refactor this logic to use a structured approach such as an array or map that
defines the cycling order of theme modes. Then, determine the next mode by
finding the current mode's index and selecting the subsequent mode in the
sequence, wrapping around as needed. Update the setMode call accordingly to use
this new approach, making it easier to modify or extend the cycling order in the
future.

Comment on lines +11 to +15
const getSystemAlgorithm = () => {
return window.matchMedia("(prefers-color-scheme: dark)").matches ?
theme.darkAlgorithm :
theme.defaultAlgorithm;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding SSR safety check.

The use of window.matchMedia is correct for client-side theme detection, but consider adding a safety check for server-side rendering environments where window is undefined.

Consider this improvement for SSR compatibility:

 const getSystemAlgorithm = () => {
+    if (typeof window === "undefined") {
+        return theme.defaultAlgorithm;
+    }
     return window.matchMedia("(prefers-color-scheme: dark)").matches ?
         theme.darkAlgorithm :
         theme.defaultAlgorithm;
 };
📝 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.

Suggested change
const getSystemAlgorithm = () => {
return window.matchMedia("(prefers-color-scheme: dark)").matches ?
theme.darkAlgorithm :
theme.defaultAlgorithm;
};
const getSystemAlgorithm = () => {
if (typeof window === "undefined") {
return theme.defaultAlgorithm;
}
return window.matchMedia("(prefers-color-scheme: dark)").matches ?
theme.darkAlgorithm :
theme.defaultAlgorithm;
};
🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/theme/utils.ts around lines 11 to 15,
the function uses window.matchMedia without checking if window is defined, which
causes errors during server-side rendering. Add a check to ensure window is
defined before accessing matchMedia; if window is undefined, return a default
algorithm to maintain SSR compatibility.

Comment on lines +32 to +35
setMode: (newMode: THEME_MODE) => {
set({mode: newMode});
set({algorithm: getAlgorithm(newMode)});
},

@coderabbitai coderabbitai Bot May 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider combining state updates for atomicity.

The two separate set() calls could cause multiple re-renders and potential inconsistency between mode and algorithm during the brief transition.

Apply this diff to combine the updates:

-            setMode: (newMode: THEME_MODE) => {
-                set({mode: newMode});
-                set({algorithm: getAlgorithm(newMode)});
-            },
+            setMode: (newMode: THEME_MODE) => {
+                set({
+                    mode: newMode,
+                    algorithm: getAlgorithm(newMode)
+                });
+            },
📝 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.

Suggested change
setMode: (newMode: THEME_MODE) => {
set({mode: newMode});
set({algorithm: getAlgorithm(newMode)});
},
setMode: (newMode: THEME_MODE) => {
set({
mode: newMode,
algorithm: getAlgorithm(newMode)
});
},
🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/theme/themeStore.ts around lines 32 to
35, the two separate set() calls for updating mode and algorithm should be
combined into a single set() call to ensure atomic state updates, preventing
multiple re-renders and maintaining consistency between mode and algorithm
during the update. Modify the setMode function to call set() once with both mode
and algorithm properties updated together.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the rabbit correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kirkrodrigues Yes—the rabbit’s suggestion is spot-on.

• Calling set() twice triggers two separate state updates. Zustand notifies subscribers after every set, so components that read either mode or algorithm will re-render twice in quick succession.
• A single set() with both keys updates the store atomically, emitting only one notification and guaranteeing that mode and algorithm are always in sync.
• There’s no functional dependency that requires mode to be written first: algorithm is computed from the same newMode argument, so you can safely batch them.

So adopting the one-call version is the cleaner, more efficient approach.

Comment on lines +39 to +50
merge: (persistedState, currentState) => {
const safePersistedState = persistedState as Partial<ThemeState>;

return {
...currentState,
...safePersistedState,
...(
safePersistedState.mode &&
{algorithm: getAlgorithm(safePersistedState.mode)}
),
};
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety and code clarity in the merge function.

The current implementation has some areas for improvement:

  1. Type casting without validation could be unsafe
  2. The conditional spread pattern could be clearer
  3. Missing error handling for algorithm calculation

Apply this diff to improve the implementation:

             merge: (persistedState, currentState) => {
-                const safePersistedState = persistedState as Partial<ThemeState>;
+                const safePersistedState = persistedState as Partial<ThemeState> | null;
+                
+                if (!safePersistedState?.mode) {
+                    return currentState;
+                }
 
+                try {
+                    const algorithm = getAlgorithm(safePersistedState.mode);
                     return {
                         ...currentState,
                         ...safePersistedState,
-                        ...(
-                            safePersistedState.mode &&
-                            {algorithm: getAlgorithm(safePersistedState.mode)}
-                        ),
+                        algorithm,
                     };
+                } catch (error) {
+                    console.warn('Failed to calculate theme algorithm, using current state:', error);
+                    return currentState;
+                }
             },

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/theme/themeStore.ts around lines 39 to
50, improve the merge function by first validating the type of persistedState
instead of directly casting it. Refactor the conditional spread to a clearer
if-statement or variable assignment before returning the merged object. Add
error handling around the getAlgorithm call to catch and handle any exceptions,
ensuring the function does not fail silently or unexpectedly.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd8f898 and 6d050e7.

⛔ Files ignored due to path filters (1)
  • components/log-viewer-webui/client/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • components/log-viewer-webui/client/package.json (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: lint-check (ubuntu-latest)
  • GitHub Check: lint-check (macos-latest)
  • GitHub Check: build (macos-latest)
  • GitHub Check: build (ubuntu-latest)

"@ant-design/v5-patch-for-react-19": "^1.0.3",
"@emotion/react": "^11.14.0",
"@emotion/styled": "^11.14.0",
"@mui/icons-material": "^7.1.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Add @mui/icons-material – verify peer dependencies and version alignment

The new @mui/icons-material@^7.1.0 brings in a peer dependency on @mui/material. Please ensure that:

  1. @mui/material is declared in your dependencies.
  2. The major versions of all MUI packages (@mui/joy, @mui/icons-material, and @mui/material) are aligned to prevent runtime or styling conflicts.

Let’s check all MUI entries in the client’s package.json to confirm which versions are declared:


🏁 Script executed:

#!/bin/bash
rg -n '"@mui/' components/log-viewer-webui/client/package.json

Length of output: 141


Let’s confirm whether @mui/material is declared:


🏁 Script executed:

#!/bin/bash
rg -n '"@mui/material"' components/log-viewer-webui/client/package.json

Length of output: 73


Add @mui/material dependency and align MUI major versions

It looks like you’ve added @mui/icons-material@^7.1.0 without declaring its peer dependency, @mui/material. On top of that, your @mui/joy is at ^5.0.0-beta.51, which is a different major version than the new icons package. To prevent runtime or styling conflicts:

• Add "@mui/material": "^7.x" to your dependencies.
• Ensure all MUI packages share the same major version: either upgrade @mui/joy to ^7.x (once available) or downgrade @mui/icons-material to ^5.x to match your Joy UI version.

Affected file:

  • components/log-viewer-webui/client/package.json
🤖 Prompt for AI Agents
In components/log-viewer-webui/client/package.json at line 20, you added
"@mui/icons-material": "^7.1.0" without including its peer dependency
"@mui/material". To fix this, add "@mui/material": "^7.x" to the dependencies
and ensure all MUI packages like "@mui/joy" and "@mui/icons-material" use the
same major version to avoid conflicts. Either upgrade "@mui/joy" to "^7.x" if
available or downgrade "@mui/icons-material" to "^5.x" to align versions
consistently.

@kirkrodrigues kirkrodrigues left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just on my side, but after merging main, it seems the PR no longer runs.


type ThemeState = ThemeValues & ThemeActions;

const THEME_DEFAULT: ThemeValues = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about THEME_DEFAULT_VALUES?

Comment on lines +32 to +35
setMode: (newMode: THEME_MODE) => {
set({mode: newMode});
set({algorithm: getAlgorithm(newMode)});
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the rabbit correct?

Comment on lines +2 to +4
SYSTEM = "system",
DARK = "dark",
LIGHT = "light",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we alphabetize or does the order matter?

icon={THEME_MODE_TO_ICON[mode]}
onClick={handleClick}
>
{THEME_MODE_TO_LABEL[mode]}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of displaying the name of the current theme, how about just calling this "Toggle theme"?

"@ant-design/v5-patch-for-react-19": "^1.0.3",
"@emotion/react": "^11.14.0",
"@emotion/styled": "^11.14.0",
"@mui/icons-material": "^7.1.0",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"@mui/icons-material": "^7.1.0",
"@mui/icons-material": "^7.1.1",

Comment on lines +46 to 50
collapsedWidth={50}
collapsible={true}
theme={"light"}
width={150}
onCollapse={(value) => {
setCollapsed(value);
}}
>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the point of this change is that we don't actually need to control the collapsed state from this component?

Comment on lines +7 to +9
* Detects Antd theme algorithm from system theme.
*
* @return The system theme algorithm.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the linting settings allow this, but ideally:

Suggested change
* Detects Antd theme algorithm from system theme.
*
* @return The system theme algorithm.
* @return The Antd theme algorithm corresponding to the system theme.

Comment on lines +18 to +21
* Detects Antd theme algorithm for the given theme mode.
*
* @param mode
* @return The theme algorithm.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Detects Antd theme algorithm for the given theme mode.
*
* @param mode
* @return The theme algorithm.
* @param mode
* @return The Antd theme algorithm corresponding to the given theme mode.

Comment on lines +24 to +26
if (mode === THEME_MODE.SYSTEM) {
return getSystemAlgorithm();
} else if (mode === THEME_MODE.DARK) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe YODA conditions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants