feat(new-webui): Add support for page switching using react-router. #797
Conversation
WalkthroughThe changes introduce a new dependency ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as AntdApp
participant Router as RouterProvider
participant Layout as MainLayout
participant Page as IngestPage/SearchPage
User->>App: Navigates to a URL
App->>Router: Forwards request to RouterProvider
Router->>Layout: Renders MainLayout based on route configuration
Layout->>Router: Uses Outlet to load child component
Router-->>Layout: Renders IngestPage or SearchPage
Layout->>User: Displays the routed content
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
🧠 Learnings (2)📓 Common learningscomponents/log-viewer-webui/client/src/router.tsx (3)⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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 (
|
davemarco
changed the title
![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 (11)
components/log-viewer-webui/client/README.md (1)
3-14: Fix the heading hierarchy in the READMEThe heading structure jumps from level 1 (# Log Viewer WebUI Client) to level 3 (### Start a Dev Server). Heading levels should only increment by one level at a time.
# Log Viewer WebUI Client -### Start a Dev Server +## Start a Dev Server Run: ```bash npm run start-### Start a Dev Server for the New WebUI (Under Development)
+## Start a Dev Server for the New WebUI (Under Development)
Run:npm run antd<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 3-3: Heading levels should only increment by one level at a time Expected: h2; Actual: h3 (MD001, heading-increment) </details> </details> </blockquote></details> <details> <summary>components/log-viewer-webui/client/src/ui/IngestView.tsx (2)</summary><blockquote> `1-2`: **Fix import spacing according to lint rules** There should be 2 empty lines after the import statement when it's not followed by another import. ```diff import React from "react"; + + const IngestView = () => {🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 1-1: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
3-10: Add JSDoc comment to component functionThe linter requires JSDoc comments for functions. Add documentation to describe what this component does.
+/** + * Renders the Ingest View page. + * + * @returns {JSX.Element} The Ingest View component + */ const IngestView = () => { return ( <div> <h1>Ingest View</h1> <p>This is the Ingest view.</p> </div> ); };🧰 Tools
🪛 GitHub Actions: clp-lint
[warning] 3-3: Missing JSDoc comment jsdoc/require-jsdoc
components/log-viewer-webui/client/src/ui/SearchView.tsx (2)
1-2: Fix import spacing according to lint rulesThere should be 2 empty lines after the import statement when it's not followed by another import.
import React from "react"; + + const SearchView = () => {🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 1-1: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
3-10: Add JSDoc comment to component functionThe linter requires JSDoc comments for functions. Add documentation to describe what this component does.
+/** + * Renders the Search View page. + * + * @returns {JSX.Element} The Search View component + */ const SearchView = () => { return ( <div> <h1>Search View</h1> <p>This is the Search view.</p> </div> ); };🧰 Tools
🪛 GitHub Actions: clp-lint
[warning] 3-3: Missing JSDoc comment jsdoc/require-jsdoc
components/log-viewer-webui/client/src/AntdApp.tsx (3)
4-5: Fix import spacing according to lint rulesThere should be 2 empty lines after import statement when it's not followed by another import.
import React from "react"; import {RouterProvider} from "react-router"; import router from "./routes/routes"; + + /**🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 4-4: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
6-10: Complete the JSDoc comment with return informationThe JSDoc comment is missing the return type information. Complete the documentation to describe what the component returns.
/** * Renders Web UI app. * - * @return + * @returns {JSX.Element} The AntApp component with router configuration */
12-12: Fix JSX tag spacingRemove the space before the closing bracket according to the style rule.
- return <RouterProvider router={router} />; + return <RouterProvider router={router}/>;🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 12-12: A space is forbidden before closing bracket @stylistic/jsx-tag-spacing
components/log-viewer-webui/client/src/routes/routes.tsx (2)
1-5: Sort imports and fix import spacingSort imports according to the linter rules and add required empty lines after imports.
-import {createBrowserRouter} from "react-router"; +import {createBrowserRouter} from "react-router-dom"; + +import IngestView from "../ui/IngestView"; +import MainLayout from "../ui/MainLayout"; +import SearchView from "../ui/SearchView"; -import MainLayout from "../ui/MainLayout"; -import IngestView from "../ui/IngestView"; -import SearchView from "../ui/SearchView"; + +🧰 Tools
🪛 GitHub Actions: clp-lint
[warning] 1-1: Run autofix to sort these imports! simple-import-sort/imports
[error] 5-5: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
7-16: Consider adding a default routeCurrently, there's no default route specified when users navigate to the root path "/". Consider adding a redirect to one of the existing views or adding an index route.
const router = createBrowserRouter([ { path: "/", Component: MainLayout, children: [ + {index: true, element: <Navigate to="search" replace />}, {path: "ingest", Component: IngestView}, {path: "search", Component: SearchView}, ], }, ]);Don't forget to import Navigate:
import {Navigate} from "react-router-dom";components/log-viewer-webui/client/src/ui/MainLayout.tsx (1)
34-57: Ensure proper path for logo source and verify routing works correctly.While the layout component looks structurally sound, please verify that:
- The logo path
/clp-logo.pngcorrectly resolves in your app's public directory- The routing configuration properly uses this layout component with its outlet
Additionally, consider using boolean attributes without redundant values:
<Sider collapsed={collapsed} - collapsible={true} + collapsible theme={"light"} width={"150"} onCollapse={(value) => { setCollapsed(value); }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.jsoncomponents/log-viewer-webui/client/public/clp-logo.pngis excluded by!**/*.png
📒 Files selected for processing (9)
components/log-viewer-webui/client/README.md(1 hunks)components/log-viewer-webui/client/package.json(2 hunks)components/log-viewer-webui/client/src/AntdApp.tsx(1 hunks)components/log-viewer-webui/client/src/main.tsx(2 hunks)components/log-viewer-webui/client/src/routes/routes.tsx(1 hunks)components/log-viewer-webui/client/src/ui/IngestView.tsx(1 hunks)components/log-viewer-webui/client/src/ui/MainLayout.css(1 hunks)components/log-viewer-webui/client/src/ui/MainLayout.tsx(1 hunks)components/log-viewer-webui/client/src/ui/SearchView.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/ui/MainLayout.tsxcomponents/log-viewer-webui/client/src/routes/routes.tsxcomponents/log-viewer-webui/client/src/ui/SearchView.tsxcomponents/log-viewer-webui/client/src/AntdApp.tsxcomponents/log-viewer-webui/client/src/main.tsxcomponents/log-viewer-webui/client/src/ui/IngestView.tsx
🪛 GitHub Actions: clp-lint
components/log-viewer-webui/client/src/ui/MainLayout.tsx
[error] 2-2: Imports must be broken into multiple lines if there are more than 1 elements import-newlines/enforce
[error] 2-2: Expected a line break after this opening brace @stylistic/object-curly-newline
[error] 2-2: Expected a line break before this closing brace @stylistic/object-curly-newline
[error] 14-14: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
[error] 21-21: Need to wrap this literal in a JSX expression @stylistic/jsx-curly-brace-presence
[error] 22-22: Need to wrap this literal in a JSX expression @stylistic/jsx-curly-brace-presence
components/log-viewer-webui/client/src/routes/routes.tsx
[warning] 1-1: Run autofix to sort these imports! simple-import-sort/imports
[error] 5-5: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
components/log-viewer-webui/client/src/ui/SearchView.tsx
[error] 1-1: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
[warning] 3-3: Missing JSDoc comment jsdoc/require-jsdoc
components/log-viewer-webui/client/src/AntdApp.tsx
[error] 4-4: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
[error] 12-12: A space is forbidden before closing bracket @stylistic/jsx-tag-spacing
components/log-viewer-webui/client/src/ui/IngestView.tsx
[error] 1-1: Expected 2 empty lines after import statement not followed by another import import/newline-after-import
[warning] 3-3: Missing JSDoc comment jsdoc/require-jsdoc
🪛 markdownlint-cli2 (0.17.2)
components/log-viewer-webui/client/README.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (6)
components/log-viewer-webui/client/src/ui/MainLayout.css (1)
1-16: LGTM! Clean CSS structure that follows best practices.The CSS styles are well-organized, with clear class names that follow a logical structure. The layout styles ensure the application takes full viewport height, and the logo styling maintains proper proportions while centered in its container.
components/log-viewer-webui/client/src/main.tsx (3)
15-21: LGTM! Clean conditional app rendering implementation.The conditional rendering approach properly follows the project's style guide with the literal on the left side of the comparison (
"true" === VITE_USE_ANTD_APP). Good practice to include the TODO comment with eslint disable explanation for later cleanup.
25-25: Component usage is consistent with React 19.Good usage of the self-closing tag syntax for the AppComponent, which is consistent with React 19 best practices.
4-4:❓ Verification inconclusive
Please ensure the AntdApp component is implemented properly.
I notice we're importing AntdApp but don't see this component in the files for review. Make sure this component:
- Is properly implemented with RouterProvider from react-router-dom
- Is using the proper routing configuration
🏁 Script executed:
#!/bin/bash # Check if AntdApp.tsx exists and how it's structured if [ -f "components/log-viewer-webui/client/src/AntdApp.tsx" ]; then echo "AntdApp.tsx exists, showing its content:" cat "components/log-viewer-webui/client/src/AntdApp.tsx" else echo "AntdApp.tsx file not found!" fi # Check if routes.tsx exists (likely imported by AntdApp) if [ -f "components/log-viewer-webui/client/src/routes.tsx" ]; then echo -e "\nroutes.tsx exists, showing its content:" cat "components/log-viewer-webui/client/src/routes.tsx" else echo -e "\nroutes.tsx file not found!" fiLength of output: 597
AntdApp Component Verification Notice
- The file AntdApp.tsx is present in the codebase and correctly wraps the application with the
<RouterProvider>using the imported router.- There is a slight inconsistency: the component imports
RouterProviderfrom"react-router"instead of"react-router-dom"as suggested in the review comment. If the intention is to use the higher-level abstractions fromreact-router-dom, please update the import accordingly.- Ensure that the routing configuration is correctly set up. The router is imported from the relative path
"./routes/routes", so please verify that the actual file (likely namedroutes.tsx,routes.ts, or perhaps an index file in aroutesfolder) is in the correct location and contains the intended routing definitions.Please review these points to confirm that the implementation meets the expectations.
components/log-viewer-webui/client/package.json (2)
10-11: LGTM! Well-structured script for conditional app rendering.The new "antd" script appropriately sets the environment variable needed for the conditional rendering in main.tsx.
21-25:✅ Verification successful
Add missing react-router-dom dependency.
The MainLayout component uses
LinkandOutletcomponents which come from the react-router-dom package, not react-router. You need to add this dependency."antd": "^5.24.5", "axios": "^1.7.9", "react": "^19.0.0", "react-dom": "^19.0.0", - "react-router": "^7.4.1" + "react-router": "^7.4.1", + "react-router-dom": "^7.4.1"
🌐 Web query:
What is the latest version of react-router and react-router-dom? Is version 7.4.1 valid?💡 Result:
React Router's latest version as of March 2025 is 7.4.1, which is valid and includes several key updates:
Latest Package Versions
- react-router@7.4.1 ([NPM][8])
- react-router-dom@7.4.1 (now deprecated, but maintained as a re-export of
react-router[8])Key Changes in v7.x
Simplified Packages
react-router-dommerged intoreact-router[2][9]- Install everything via
npm install react-router@latest[2]New Requirements
Requires:
- Node.js ≥20
- React ≥18
- React-DOM ≥18[2]
Future-First Architecture
- Relative path handling for nested routes[2]
- Transition-based state management via
React.useTransition[2]- Enhanced fetcher lifecycle persistence[2]
Compatibility Notes
- Migration from v6:
Non-breaking if all future flags are enabled (likev7_relativeSplatPath)[2]- Deprecated Features:
- Separate
react-router-dompackage (usereact-routerimports)[2][8]json/deferutilities (use nativeResponse.json())[2]Security fixes in 7.4.1 address URL manipulation risks via improved header sanitization[10].
For new projects, the official docs recommend using
react-routerdirectly with the new framework-style routing features[14][16].Citations:
- 1: https://www.youtube.com/watch?v=4SgEThQ05q8
- 2: https://reactrouter.com/upgrading/v6
- 3: ionic-team/ionic-framework#29620
- 4: https://reactrouter.com/6.30.0/start/changelog
- 5: https://reactrouter.com/start/library/installation
- 6: https://www.jsdocs.io/package/react-router
- 7: https://www.youtube.com/watch?v=NVuxUktUvxc
- 8: https://www.npmjs.com/package/react-router-dom
- 9: https://reactrouter.com/upgrading/component-routes
- 10: https://reactrouter.com/changelog
- 11: https://www.w3schools.com/react/react_router.asp
- 12: https://reactrouter.com/start/framework/route-module
- 13: https://www.guvi.in/blog/how-to-setup-react-router-v6-tutorial/
- 14: https://reactrouter.com/home
- 15: https://reactrouter.com
- 16: https://www.youtube.com/watch?v=h7MTWLv3xvw
- 17: https://react.dev/blog/2025/02/14/sunsetting-create-react-app
- 18: https://github.com/remix-run/react-router
- 19: https://github.com/remix-run/react-router/releases
Action: Add the missing react-router-dom dependency
The MainLayout component is importing
LinkandOutletfrom react-router-dom. Although version 7.4.1 is valid for both packages—and even though the react-router-dom package is now maintained as a re-export—the dependency is still required to avoid breaking these imports. Please update your dependencies as follows:
File:
components/log-viewer-webui/client/package.json(Lines 21–25)Change:
"antd": "^5.24.5", "axios": "^1.7.9", "react": "^19.0.0", "react-dom": "^19.0.0", - "react-router": "^7.4.1" + "react-router": "^7.4.1", + "react-router-dom": "^7.4.1"This update ensures that the components relying on react-router-dom are available while remaining consistent with the latest valid versions.
|
@junhaoliao - this is now ready for review |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/log-viewer-webui/client/src/ui/MainLayout.tsx (1)
2-5:⚠️ Potential issueIncorrect import source for React Router components.
You're importing
LinkandOutletfrom "react-router" when they should be imported from "react-router-dom". This could lead to runtime errors.import { Link, Outlet, -} from "react-router"; +} from "react-router-dom";
🧹 Nitpick comments (2)
components/log-viewer-webui/client/src/ui/SearchView.tsx (1)
4-8: Complete the JSDoc @return tagThe JSDoc comment is missing information for the @return tag. Completing this will improve documentation quality and help other developers understand what the component returns.
/** * Provides a search interface that allows users to query archives and visualize search results. * - * @return + * @return {JSX.Element} A React component that displays the search interface */components/log-viewer-webui/client/src/ui/MainLayout.tsx (1)
38-38: Consider using dot notation for CSS modules.While the bracket notation for CSS modules works, dot notation is often preferred for better readability when property names don't contain special characters or hyphens. If you're going to stick with hyphenated class names, this is fine, but consider renaming your CSS classes to use camelCase for consistency with JavaScript naming conventions.
-<Layout className={styles["main-layout"]}> +<Layout className={styles.mainLayout}> -<div className={styles["sider-logo-container"]}> +<div className={styles.siderLogoContainer}> -className={styles["sider-logo"]} +className={styles.siderLogo}This would require updating your CSS module class names as well:
/* In MainLayout.module.css */ .mainLayout { /* styles */ } .siderLogoContainer { /* styles */ } .siderLogo { /* styles */ }Also applies to: 48-48, 51-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
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/routes.tsx(1 hunks)components/log-viewer-webui/client/src/ui/IngestView.tsx(1 hunks)components/log-viewer-webui/client/src/ui/MainLayout.tsx(4 hunks)components/log-viewer-webui/client/src/ui/SearchView.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/log-viewer-webui/client/package.json
🧰 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/ui/IngestView.tsxcomponents/log-viewer-webui/client/src/routes.tsxcomponents/log-viewer-webui/client/src/ui/SearchView.tsxcomponents/log-viewer-webui/client/src/ui/MainLayout.tsxcomponents/log-viewer-webui/client/src/AntdApp.tsx
🧠 Learnings (1)
components/log-viewer-webui/client/src/routes.tsx (1)
Learnt from: davemarco
PR: y-scope/clp#797
File: components/log-viewer-webui/client/src/routes/routes.tsx:1-1
Timestamp: 2025-04-04T15:15:59.432Z
Learning: In React Router v7, `createBrowserRouter` and other router primitives should be imported from `react-router` package, not from `react-router-dom` as in previous versions. This represents a significant change in the library's structure where router primitives are now in the core package.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (7)
components/log-viewer-webui/client/src/ui/SearchView.tsx (1)
9-16: Implementation looks goodThe component provides a simple placeholder for the Search view that works well with the routing system being implemented. As development progresses, this component can be expanded with actual search functionality.
components/log-viewer-webui/client/src/ui/IngestView.tsx (1)
9-16: Implementation looks goodThe component provides a clean placeholder for the Ingest view that works well with the routing system being implemented. This can be expanded with actual ingest functionality as development progresses.
components/log-viewer-webui/client/src/AntdApp.tsx (1)
2-13: Routing implementation looks goodThe application now properly uses RouterProvider to implement dynamic page switching, which meets the PR objectives. This change effectively transforms the static menu into an interactive navigation system.
components/log-viewer-webui/client/src/routes.tsx (1)
1-19: Routing configuration is well-structuredThe router setup correctly implements nested routes with MainLayout as the parent container, which will maintain consistent navigation elements while switching between views. This is a good pattern for single-page applications.
The import from "react-router" (rather than "react-router-dom") follows the correct pattern for React Router v7.
components/log-viewer-webui/client/src/ui/MainLayout.tsx (3)
17-17: Missing empty lines after import statement.According to the linting rules, you need two empty lines after import statements that aren't followed by another import.
import styles from "./MainLayout.module.css"; const {Sider} = Layout;
25-26: JSX string literals need to be wrapped in curly braces.According to the linting rules, JSX string literals like "Ingest" and "Search" should be wrapped in curly braces.
- {label: <Link to={"/ingest"}>Ingest</Link>, key: "/ingest", icon: <UploadOutlined/>}, - {label: <Link to={"/search"}>Search</Link>, key: "/search", icon: <SearchOutlined/>}, + {label: <Link to={"/ingest"}>{"Ingest"}</Link>, key: "/ingest", icon: <UploadOutlined/>}, + {label: <Link to={"/search"}>{"Search"}</Link>, key: "/search", icon: <SearchOutlined/>},
58-60: LGTM! Good implementation of nested routing.The
Outletcomponent is properly wrapped in aLayoutcomponent, which will enable proper rendering of nested routes. This is a key part of implementing react-router functionality.
| @@ -31,7 +35,7 @@ const MainLayout = () => { | |||
| const [collapsed, setCollapsed] = useState(false); | |||
|
|
|||
| return ( | |||
| <Layout className={"main-layout"}> | |||
| <Layout className={styles["main-layout"]}> | |||
There was a problem hiding this comment.
Let's use camelCasing for class names in CSS modules, following the recommendation at https://github.com/css-modules/css-modules/blob/master/docs/naming.md
Then we can use the dot-notation here
There was a problem hiding this comment.
directly accesing them dosen't work with typescript. the linter complains
CSS modules dosent have native typescript support
I tried this plugin but i can't really seem to get it working - https://github.com/mrmckeb/typescript-plugin-css-modules
Let me know if I should just go back to regular css.
There was a problem hiding this comment.
Let me quickly check the linter on my end.
There was a problem hiding this comment.
this plugin is only a Language Service Plugin, which means it only have effects in IDEs (than tsc -b).
How about this:
- We always name the classes in camelCase in CSS modules.
- If we want to use dot-notation access, we shall disable
noPropertyAccessFromIndexSignatureintsconfig.app.json. - Alternatively, we can update
eslint.config.mjsto add below block:
{
files: [
"**/*.ts",
"**/*.tsx",
],
rules: {
"dot-notation": "off",
"@typescript-eslint/dot-notation": "error",
},
},I will also be adding the above to eslint-config-yscope so the config lib users do not have to config those explicitly.
Let me know what you think.
There was a problem hiding this comment.
It's neat that you found a fix for this. Turning this off noPropertyAccessFromIndexSignature is okay but not prefered.I still have slight preference to go with regular css. I will add kirk as arbiter @kirkrodrigues
There was a problem hiding this comment.
@kirkrodrigues - no longer needed. I resolved with junhao
There was a problem hiding this comment.
@junhaoliao - i got some error adding "@typescript-eslint/dot-notation": "error",, but it worked just fine with only "dot-notation": "off". If this is an issue let me know.
There was a problem hiding this comment.
Might have to do with secondary dependency and make u have to change in eslint yscope
There was a problem hiding this comment.
For now i just added "dot-notation": "off"
There was a problem hiding this comment.
Note this creates some problems with antd not supporting underfined className type. So we have to do like this in somecases [key] | "" for antd
I created an issue to track this: react-component/select#1143
If time permits, i'll submit a fix to them. Let me know if there're other components giving you issues.
| import SearchView from "./ui/SearchView"; | ||
|
|
||
|
|
||
| const router = createBrowserRouter([ |
There was a problem hiding this comment.
| const router = createBrowserRouter([ | |
| const ROUTER = createBrowserRouter([ |
There was a problem hiding this comment.
I took this syntax from their docs. Unless u have strong opinions i would leave it to match
There was a problem hiding this comment.
we've been using SNAKE_CASE for global constants but i do get variables are mostly const since EM6. I prefer keeping SNAKE_CASES, but let's quickly check with @kirkrodrigues before we commit.
There was a problem hiding this comment.
Router may also make sense if it is a component
There was a problem hiding this comment.
I discussed this offline with @kirkrodrigues and here're some consolidated thoughts (a copy of section https://www.notion.so/yscope/WIP-Coding-Guidelines-9a308b847a5343958ba3cb97a850be66?pvs=4#1cf04e4d9e6b80c7ac4fe69b846f5791)
Naming convention for constants in JavaScript / TypeScript
JavaScript doesn't enforce deep immutability for const objects or arrays, so we apply a semantic naming convention to indicate intent:
-
SCREAMING_SNAKE_CASE
Use this for truly immutable values. This includes:
- Primitive constant values
e.g.,const SECONDS_IN_MINUTE = 60; - Deeply frozen objects
e.g.,const CONFIG = Object.freeze({...}); - Any value that is intended to remain constant throughout its lifetime.
- Primitive constant values
-
camelCase
Use this for variables declared with
constwhose contents may still change (e.g., properties of objects or elements of arrays). This keeps the code readable and avoids excessive SCREAMING_SNAKE_CASE.
Additional Notes
-
React
useStatevariables should not use SCREAMING_SNAKE_CASE, even though the bindings are immutable. Their values can change via re-renders, so they are not considered truly constant. -
Constant functions should follow normal function naming rules, based on their role:
- Use PascalCase for JSX functional components (e.g.,
const MyComponent = () => { ... }), as required by React. - Use camelCase for all other functions (e.g.,
const formatDate = () => { ... }).
Even though function bindings with
constare immutable, the function itself is a callable entity, not a constant value, so we do not use SCREAMING_SNAKE_CASE. - Use PascalCase for JSX functional components (e.g.,
There was a problem hiding this comment.
That said, let's make this object deeply frozen since the routes are not expected to be modified elsewhere. We will then rename the object and the filename to ROUTES as well.
There was a problem hiding this comment.
I think this router has state and manages stuff like breadcrumbs. See their comment.
I dont think its just a config. Anyways for now as router... i left it. Let me know if u disagree
There was a problem hiding this comment.
sure. i just thought of another reason that we don't want to make it deeply frozen: if we are to support any plugins like an optional page. this is fine to leave then
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>
| import SearchView from "./ui/SearchView"; | ||
|
|
||
|
|
||
| const router = createBrowserRouter([ |
There was a problem hiding this comment.
I took this syntax from their docs. Unless u have strong opinions i would leave it to match
| @@ -31,7 +35,7 @@ const MainLayout = () => { | |||
| const [collapsed, setCollapsed] = useState(false); | |||
|
|
|||
| return ( | |||
| <Layout className={"main-layout"}> | |||
| <Layout className={styles["main-layout"]}> | |||
There was a problem hiding this comment.
directly accesing them dosen't work with typescript. the linter complains
CSS modules dosent have native typescript support
I tried this plugin but i can't really seem to get it working - https://github.com/mrmckeb/typescript-plugin-css-modules
Let me know if I should just go back to regular css.
| @@ -10,16 +14,16 @@ import { | |||
| MenuProps, | |||
| } from "antd"; | |||
|
|
|||
| import "./MainLayout.css"; | |||
| import styles from "./MainLayout.module.css"; | |||
|
|
|||
|
|
|||
| const {Sider} = Layout; | |||
|
|
|||
| type MenuItem = Required<MenuProps>["items"][number]; | |||
|
|
|||
| const items: MenuItem[] = [ | |||
There was a problem hiding this comment.
How about SIDEBAR_MENU_ITEMS?
There was a problem hiding this comment.
Let's make this deeply frozen with Object.freeze().
There was a problem hiding this comment.
There is some issue with freezing the antd types. I left for now
| import SearchView from "./ui/SearchView"; | ||
|
|
||
|
|
||
| const router = createBrowserRouter([ |
There was a problem hiding this comment.
we've been using SNAKE_CASE for global constants but i do get variables are mostly const since EM6. I prefer keeping SNAKE_CASES, but let's quickly check with @kirkrodrigues before we commit.
| @@ -31,7 +35,7 @@ const MainLayout = () => { | |||
| const [collapsed, setCollapsed] = useState(false); | |||
|
|
|||
| return ( | |||
| <Layout className={"main-layout"}> | |||
| <Layout className={styles["main-layout"]}> | |||
There was a problem hiding this comment.
Let me quickly check the linter on my end.
|
@davemarco |
| import SearchView from "./ui/SearchView"; | ||
|
|
||
|
|
||
| const router = createBrowserRouter([ |
There was a problem hiding this comment.
I discussed this offline with @kirkrodrigues and here're some consolidated thoughts (a copy of section https://www.notion.so/yscope/WIP-Coding-Guidelines-9a308b847a5343958ba3cb97a850be66?pvs=4#1cf04e4d9e6b80c7ac4fe69b846f5791)
Naming convention for constants in JavaScript / TypeScript
JavaScript doesn't enforce deep immutability for const objects or arrays, so we apply a semantic naming convention to indicate intent:
-
SCREAMING_SNAKE_CASE
Use this for truly immutable values. This includes:
- Primitive constant values
e.g.,const SECONDS_IN_MINUTE = 60; - Deeply frozen objects
e.g.,const CONFIG = Object.freeze({...}); - Any value that is intended to remain constant throughout its lifetime.
- Primitive constant values
-
camelCase
Use this for variables declared with
constwhose contents may still change (e.g., properties of objects or elements of arrays). This keeps the code readable and avoids excessive SCREAMING_SNAKE_CASE.
Additional Notes
-
React
useStatevariables should not use SCREAMING_SNAKE_CASE, even though the bindings are immutable. Their values can change via re-renders, so they are not considered truly constant. -
Constant functions should follow normal function naming rules, based on their role:
- Use PascalCase for JSX functional components (e.g.,
const MyComponent = () => { ... }), as required by React. - Use camelCase for all other functions (e.g.,
const formatDate = () => { ... }).
Even though function bindings with
constare immutable, the function itself is a callable entity, not a constant value, so we do not use SCREAMING_SNAKE_CASE. - Use PascalCase for JSX functional components (e.g.,
| import SearchView from "./ui/SearchView"; | ||
|
|
||
|
|
||
| const router = createBrowserRouter([ |
There was a problem hiding this comment.
That said, let's make this object deeply frozen since the routes are not expected to be modified elsewhere. We will then rename the object and the filename to ROUTES as well.
| @@ -10,16 +14,16 @@ import { | |||
| MenuProps, | |||
| } from "antd"; | |||
|
|
|||
| import "./MainLayout.css"; | |||
| import styles from "./MainLayout.module.css"; | |||
|
|
|||
|
|
|||
| const {Sider} = Layout; | |||
|
|
|||
| type MenuItem = Required<MenuProps>["items"][number]; | |||
|
|
|||
| const items: MenuItem[] = [ | |||
There was a problem hiding this comment.
Let's make this deeply frozen with Object.freeze().
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/log-viewer-webui/client/src/pages/SearchPage.tsx (1)
1-5: JSDoc return type is missingThe JSDoc comment for the
SearchViewcomponent has an empty@returntag. You should specify what the component returns for better documentation./** * Provides a search interface that allows users to query archives and visualize search results. * - * @return + * @return {JSX.Element} The rendered Search view */components/log-viewer-webui/client/src/pages/IngestPage.tsx (1)
1-5: JSDoc comment doesn't match implementationThe JSDoc comment states "Presents compression statistics" but the component doesn't actually show any statistics. The comment should be updated to match the actual implementation.
/** - * Presents compression statistics. + * Provides an interface for the ingest functionality. * * @return */Additionally, the
@returntag is empty and should specify the return type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
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(1 hunks)components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx(4 hunks)components/log-viewer-webui/client/src/pages/IngestPage.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage.tsx(1 hunks)components/log-viewer-webui/client/src/routes.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/log-viewer-webui/client/src/components/Layout/MainLayout.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- components/log-viewer-webui/client/src/routes.tsx
- components/log-viewer-webui/client/package.json
🧰 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/pages/SearchPage.tsxcomponents/log-viewer-webui/client/src/AntdApp.tsxcomponents/log-viewer-webui/client/src/pages/IngestPage.tsxcomponents/log-viewer-webui/client/src/components/Layout/MainLayout.tsx
🧬 Code Graph Analysis (2)
components/log-viewer-webui/client/src/pages/SearchPage.tsx (1)
components/webui/imports/ui/SearchView/SearchView.jsx (1)
SearchView(42-347)
components/log-viewer-webui/client/src/pages/IngestPage.tsx (1)
components/webui/imports/ui/IngestView/IngestView.jsx (1)
IngestView(20-49)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-14, false)
🔇 Additional comments (8)
components/log-viewer-webui/client/src/pages/SearchPage.tsx (1)
6-13: Placeholder component needs implementationThe current implementation is just a placeholder and doesn't include the functionality present in the legacy component (from the relevant code snippets). This component needs to be expanded to include actual search functionality such as query input, result visualization, and timeline features.
The current component is a minimal placeholder while the legacy component in
components/webui/imports/ui/SearchView/SearchView.jsxhas extensive search functionality including:
- Query handling
- Search job management
- Results display
- Timeline visualization
Will this functionality be implemented in a future PR, or should it be part of this one?
components/log-viewer-webui/client/src/AntdApp.tsx (1)
1-3: Implementation of react-router looks goodThe integration of react-router is properly implemented with
RouterProvider. This aligns with the PR objective of adding support for page switching.Also applies to: 12-12
components/log-viewer-webui/client/src/pages/IngestPage.tsx (1)
6-13: Placeholder component needs implementationSimilar to the SearchView, this is just a placeholder and doesn't include the functionality present in the legacy component. The legacy IngestView component from the code snippets includes statistics display and ingestion jobs.
Is the plan to enhance this component in a future PR, or should the full functionality be implemented as part of this PR?
components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx (5)
17-17: Good use of CSS modulesMoving to CSS modules with the
.module.cssnaming convention is a good practice for style encapsulation.
24-26: Good use of constants naming and navigation structureRenaming to
SIDEBAR_MENU_ITEMSfollows the convention for constants. The Link components are correctly implemented for navigation.
38-38: CSS module class names are properly appliedThe class names are correctly updated to use the CSS module syntax with
styles.className.Also applies to: 48-48, 51-51
55-55: Menu items correctly updatedThe menu items reference has been updated to use the renamed constant.
58-60: Proper implementation of nested routes with OutletThe addition of the
Outletcomponent within the Layout is the correct way to implement nested routes in react-router. This allows child routes to render within the parent layout.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/log-viewer-webui/client/eslint.config.mjs (1)
31-31: ESLint rule change appears to support the routing implementationThe addition of
"dot-notation": "off"disables the ESLint rule that enforces dot notation for property access. This change likely supports the newreact-routerimplementation, as routing code often uses bracket notation for dynamic property access, especially when working with route parameters or dynamic routes.While this change is acceptable, consider documenting the rationale behind disabling this rule with a comment in the code to help future maintainers understand why this exception was made.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/client/eslint.config.mjs(1 hunks)components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx(4 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/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.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";`.
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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (5)
components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx (5)
2-5: Import from "react-router" is correctThis import statement correctly uses the React Router v7.4.1 package structure, where components like
LinkandOutletcan be imported directly from "react-router" rather than "react-router-dom" as in previous versions.
17-17: Good use of CSS modulesSwitching to CSS modules provides better encapsulation of styles and prevents class name collisions. This is a good practice for component-scoped styling.
24-27: Good implementation of clickable menu itemsThe constant naming convention to uppercase (SIDEBAR_MENU_ITEMS) follows best practices for constants. The use of
Linkcomponents for menu labels successfully implements the clickable menu items requirement for page switching.
38-38: Consistent use of CSS modules syntaxThe class names have been correctly updated to use CSS modules syntax. The bracket notation (
styles["className"]) is valid, though dot notation (styles.className) is also commonly used when class names don't contain special characters.Also applies to: 48-48, 51-51, 55-55
58-60: Proper implementation of nested routingAdding the
Outletcomponent within the Layout is essential for the proper functioning of nested routes. This allows child routes to be rendered in the specified location, enabling the page switching functionality that is the core objective of this PR.
| import SearchView from "./ui/SearchView"; | ||
|
|
||
|
|
||
| const router = createBrowserRouter([ |
There was a problem hiding this comment.
sure. i just thought of another reason that we don't want to make it deeply frozen: if we are to support any plugins like an optional page. this is fine to leave then
There was a problem hiding this comment.
- don't forget updating the page component names inside the files.
- since this
routes.tsxdefault exportsrouter, let's name this filerouter.tsxto match the default export name then.
| @@ -28,6 +28,7 @@ const EslintConfig = [ | |||
| ...ReactConfigArray, | |||
| { | |||
| rules: { | |||
| "dot-notation": "off", | |||
There was a problem hiding this comment.
Let's turn it off for TS only. like instead, we want to add this object to the end of the EslintConfig array:
{
'files: [
"**/*.ts",
"**/*.tsx",
],
rules: {
"dot-notation": "off",
"@typescript-eslint/dot-notation": "error",
},
},
junhaoliao
changed the title
|
i edited the PR title directly to change the scope "webui" -> "new-webui" |
Description
The menu in the new webui is currently static, and does nothing.
This PR adds react-router library which makes the menu links clickable.
In old webui just relied on regular links, but using react router is supposedly more performant.
Note we are using data mode for react-router
Checklist
breaking change.
Validation performed
Pages switch
Summary by CodeRabbit
New Features
Style