feat(new-webui): Add global theming config and adjust sider appearance.#937
Conversation
## Walkthrough
The changes introduce a new Ant Design theme configuration and update the main application component to wrap the router with a `ConfigProvider` using this theme. Additionally, layout styling was adjusted by changing the sidebar width from string to number and modifying padding and height rules for the logo area. No component signatures or control flow were altered.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------|----------------------------------------------------------------------------------------------------|
| .../client/src/AntdApp.tsx | Wrapped `RouterProvider` with Ant Design `ConfigProvider` using imported `THEME_CONFIG`. |
| .../client/src/theme.tsx | Added new file defining and exporting `THEME_CONFIG` for Ant Design theming. |
| .../client/src/components/Layout/MainLayout.tsx | Changed Sider width from string `"150"` to numeric `150`. |
| .../client/src/components/Layout/MainLayout.module.css | Removed fixed height on `.siderLogo`; added `padding-inline: 15%` and `padding-block: 16px`. |
| .../client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx | Modified React import to use named imports (`useEffect`, `useRef`, `useState`) instead of default. |
## Possibly related PRs
- y-scope/clp#791: Introduced the `AntApp` component and Ant Design dependency, which the current PR extends by adding theming support.
- y-scope/clp#797: Established the initial routing setup with `RouterProvider` in `AntdApp.tsx`, upon which the current PR builds by adding theming.
## Suggested reviewers
- davemarco
- kirkrodrigues📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
✨ Finishing Touches
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. 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/client/src/AntdApp.tsx(2 hunks)components/log-viewer-webui/client/src/theme.tsx(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}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/AntdApp.tsxcomponents/log-viewer-webui/client/src/theme.tsx
🔇 Additional comments (2)
components/log-viewer-webui/client/src/theme.tsx (1)
7-15: Immutable configuration
Freezing the theme configuration withObject.freezeis a robust way to guard against accidental mutations at runtime. Nice use of immutability for a global config object.components/log-viewer-webui/client/src/AntdApp.tsx (1)
16-24: Verify dynamic theme integration
Right nowConfigProvideris passed the staticTHEME_CONFIG. To support toggling between light, dark and system modes, ensure that the runtime theme—particularly the correctalgorithmsetting—is computed (e.g. fromlocalStorageorprefers-color-scheme) and fed intoConfigProviderhere.
| import router from "./router"; | ||
| import THEME_CONFIG from "./theme"; | ||
|
|
||
| import "@ant-design/v5-patch-for-react-19"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure patch import precedence
Consider moving the import of @ant-design/v5-patch-for-react-19 to the very top of this file, before any other imports, so that the patch takes effect before Ant Design and related modules are loaded.
🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/AntdApp.tsx at line 8, move the import
of "@ant-design/v5-patch-for-react-19" to the very top of the file, before any
other imports, to ensure the patch is applied before Ant Design and related
modules are loaded.
| const AntApp = () => { | ||
| return <RouterProvider router={router}/>; | ||
| return ( | ||
| <ConfigProvider | ||
| theme={THEME_CONFIG} | ||
| > | ||
| <RouterProvider router={router}/> | ||
| </ConfigProvider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add explicit component return type
Since this is a TSX file, you may want to annotate AntApp with an explicit return type (: JSX.Element) for clarity and stricter typing:
const AntApp = (): JSX.Element => { … }🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/AntdApp.tsx around lines 16 to 24, the
AntApp component lacks an explicit return type annotation. Add the return type
annotation `: JSX.Element` to the AntApp function declaration to improve clarity
and enforce stricter typing in this TSX file.
There was a problem hiding this comment.
unnecessary for components in JSX / TSX sources
There was a problem hiding this comment.
@junhaoliao, understood! I'll remember this preference for future reviews of React components in your codebase.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.631Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| import {ConfigProvider} from "antd"; | ||
|
|
||
| import router from "./router"; | ||
| import THEME_CONFIG from "./theme"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve import grouping
Group external dependencies (e.g. react-router, antd) separately from local imports (./router, ./theme) and maintain a consistent ordering (alphabetical or logical) to enhance readability.
🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/AntdApp.tsx around lines 3 to 6, the
import statements are not grouped properly. Separate external dependencies like
"antd" from local imports such as "./router" and "./theme". Arrange each group
in a consistent order, either alphabetically or logically, to improve code
readability and maintainability.
There was a problem hiding this comment.
this needs to be fixed in https://github.com/y-scope/eslint-config-yscope
@coderabbitai create an issue there
davemarco
left a comment
There was a problem hiding this comment.
Code looks good to me. I am okay with styles as well
|
Note should probably wait until final UI pr are merged, before merging this. kirk may also have comments on style |
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
| @@ -1,4 +1,4 @@ | |||
| import React, { | |||
| import { | |||
There was a problem hiding this comment.
Do we want to fix this in this PR? I'm not necessarily against it, it's just it would be weird to write (fixes #942) in the title.
There was a problem hiding this comment.
I can fix it in timelinePR i think it is more appropriate there
This reverts commit b840845.
…e. (y-scope#937) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Description
Checklist
breaking change.
Validation performed
Summary by CodeRabbit