feat(new-webui): Add a search bar to search view.#807
Conversation
WalkthroughThe pull request introduces several new features and enhancements centred around search functionality within the web UI. A new dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SP as SearchPage
participant SC as SearchControls
participant SS as SearchState (Zustand)
participant TRI as TimeRangeInput
participant SB as SearchButton
U->>SP: Navigate to Search Page
SP->>SC: Render SearchControls
SC->>SS: Retrieve current query and time range
U->>SC: Enter query and select time range
SC->>SS: Update query string and time range
TRI->>SS: Process custom date range (if "Custom" selected)
SB->>SS: Validate state and trigger search submission
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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 (
|
There was a problem hiding this comment.
Copilot reviewed 11 out of 15 changed files in this pull request and generated 2 comments.
Files not reviewed (4)
- components/log-viewer-webui/client/package-lock.json: Language not supported
- components/log-viewer-webui/client/package.json: Language not supported
- components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/index.module.css: Language not supported
- components/log-viewer-webui/client/src/ui/SearchView/SearchControls/index.module.css: Language not supported
| const isQueryStringValid: boolean = queryString === SEARCH_CONTEXT_DEFAULT.queryString; | ||
|
|
There was a problem hiding this comment.
The logic for enabling/disabling the search button appears inverted. Consider updating the condition so that the button is disabled when the query string is empty (i.e., when queryString equals SEARCH_CONTEXT_DEFAULT.queryString).
| const isQueryStringValid: boolean = queryString === SEARCH_CONTEXT_DEFAULT.queryString; | |
| const isQueryStringValid: boolean = queryString !== SEARCH_CONTEXT_DEFAULT.queryString; |
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().add(-1, "d"), | ||
| dayjs().add(-1, "d")], | ||
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().add(-7, "d"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().add(-30, "d"), |
There was a problem hiding this comment.
For clarity and consistency, consider using dayjs().subtract(1, "day") instead of dayjs().add(-1, "d"). The same applies to other instances (e.g., LAST_7_DAYS and LAST_30_DAYS).
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().add(-1, "d"), | |
| dayjs().add(-1, "d")], | |
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().add(-7, "d"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().add(-30, "d"), | |
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "day"), | |
| dayjs().subtract(1, "day")], | |
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(7, "day"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(30, "day"), |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
components/log-viewer-webui/client/src/routes.tsx (1)
8-17: Consider adding an index/default routeYour router configuration doesn't specify what should be displayed when a user visits the root path ("/"). Consider adding an index route or redirecting to a default route.
const router = createBrowserRouter([ { path: "/", Component: MainLayout, children: [ + {index: true, element: <Navigate to="/search" replace />}, {path: "ingest", Component: IngestView}, {path: "search", Component: SearchView}, ], }, ]);Note: You'll need to import Navigate from react-router if you implement this suggestion.
components/log-viewer-webui/client/src/ui/SearchView/SearchControls/index.module.css (1)
8-13: Add hover state for gradient buttonThe gradient button has a transition defined but lacks hover effects to make it feel interactive. Consider adding a hover state to provide visual feedback when users interact with the button.
.gradient-button { background-image: linear-gradient(135deg, #6253e1, #04befe); color: white; transition: background 0.3s; background-size: 200% auto; + cursor: pointer; } +.gradient-button:hover { + background-position: right center; + box-shadow: 0 4px 8px rgba(0, 0, 0, 0.1); +}components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/index.tsx (2)
70-72: Simplify the onChange handlerThe event handler can be simplified by directly referencing the function.
- onChange={(dates) => { - handleRangePickerChange(dates); - }}/> + onChange={handleRangePickerChange}/>
66-66: Simplify className conditionalThe current approach for fallback is correct but can be more elegantly written using nullish coalescing.
- className={styles["range-picker"] || ""} + className={styles["range-picker"] ?? ""}components/log-viewer-webui/client/src/ui/SearchView/SearchContext.tsx (1)
23-25: Consider improving timeRange formattingThe line breaks in the array literal make the code less readable. Consider formatting it on a single line.
- // Type casting since typescript cannot infer the correct type for array literals - timeRange: [dayjs().startOf("day"), - dayjs().endOf("day")] as [dayjs.Dayjs, dayjs.Dayjs], + // Type casting since typescript cannot infer the correct type for array literals + timeRange: [dayjs().startOf("day"), dayjs().endOf("day")] as [dayjs.Dayjs, dayjs.Dayjs],
📜 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 (13)
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/SearchContext.tsx(1 hunks)components/log-viewer-webui/client/src/ui/SearchView/SearchControls/SearchButton.tsx(1 hunks)components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/index.module.css(1 hunks)components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/index.tsx(1 hunks)components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/utils.tsx(1 hunks)components/log-viewer-webui/client/src/ui/SearchView/SearchControls/index.module.css(1 hunks)components/log-viewer-webui/client/src/ui/SearchView/SearchControls/index.tsx(1 hunks)components/log-viewer-webui/client/src/ui/SearchView/index.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/routes.tsxcomponents/log-viewer-webui/client/src/ui/SearchView/index.tsxcomponents/log-viewer-webui/client/src/ui/IngestView.tsxcomponents/log-viewer-webui/client/src/ui/SearchView/SearchControls/index.tsxcomponents/log-viewer-webui/client/src/ui/SearchView/SearchControls/SearchButton.tsxcomponents/log-viewer-webui/client/src/AntdApp.tsxcomponents/log-viewer-webui/client/src/ui/SearchView/SearchContext.tsxcomponents/log-viewer-webui/client/src/ui/MainLayout.tsxcomponents/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/index.tsxcomponents/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/utils.tsx
🧬 Code Definitions (4)
components/log-viewer-webui/client/src/ui/SearchView/index.tsx (2)
components/webui/imports/ui/SearchView/SearchView.jsx (1)
SearchView(42-347)components/webui/imports/ui/SearchView/SearchControls/index.jsx (1)
SearchControls(46-161)
components/log-viewer-webui/client/src/ui/IngestView.tsx (1)
components/webui/imports/ui/IngestView/IngestView.jsx (1)
IngestView(20-49)
components/log-viewer-webui/client/src/ui/SearchView/SearchControls/index.tsx (3)
components/webui/imports/ui/SearchView/SearchControls/index.jsx (1)
SearchControls(46-161)components/log-viewer-webui/client/src/ui/SearchView/SearchContext.tsx (1)
SearchContext(65-65)components/webui/imports/ui/SearchView/SearchView.jsx (1)
queryString(56-56)
components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/index.tsx (2)
components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/utils.tsx (4)
TIME_RANGE_OPTION(62-62)timeRangeOptionDayJsMap(62-62)isValidDateRange(62-62)TIME_RANGE_OPTION_NAMES(62-62)components/log-viewer-webui/client/src/ui/SearchView/SearchContext.tsx (1)
SearchContext(65-65)
🔇 Additional comments (19)
components/log-viewer-webui/client/package.json (1)
24-25:Details
✅ Verification successful
Verify if react-router-dom dependency is needed
You've added react-router, but typically web applications using React Router also require react-router-dom for components like BrowserRouter, Routes, Route, etc. Based on the routes.tsx file that imports createBrowserRouter, you might need to add react-router-dom as well.
🏁 Script executed:
#!/bin/bash # Check if createBrowserRouter exists in react-router package or if react-router-dom is needed npm view react-router@7.5.0 exports --json | grep -i createBrowserRouter npm view react-router-dom@7.5.0 exports --json | grep -i createBrowserRouterLength of output: 153
Below is an additional shell script to search for where and how
createBrowserRouteris used within the codebase. This should help clarify whether it’s imported fromreact-router-dom(which would then imply that dependency is needed) or fromreact-routerdirectly:Once this script is executed, please review the output. In particular, check the import statements in the file (likely
routes.tsx) wherecreateBrowserRouteris used. If the import originates fromreact-router-dom, then the dependency should indeed be added to the project.Based on the current evidence (with the npm view commands returning no output), manual verification of these import statements is recommended before making a final decision.
Next Steps:
- Verify the output of the above search.
- Confirm whether
createBrowserRouteris being imported fromreact-router-dom.- If so, add the
react-router-domdependency accordingly.
🏁 Script executed:
#!/bin/bash echo "Searching for 'createBrowserRouter' usage in the codebase..." rg -n "createBrowserRouter" .Length of output: 483
React Router Dependency Verification
Based on our review:
- The
createBrowserRouterimport incomponents/log-viewer-webui/client/src/routes.tsxcomes from"react-router"instead of"react-router-dom".- There is no evidence from the codebase (or the npm view commands) indicating that components exclusive to
react-router-dom(such asBrowserRouter,Routes, orRoute) are used.It appears that, as currently implemented, the project does not require the
react-router-domdependency. If future requirements emerge for DOM-specific components provided byreact-router-dom, that dependency can be revisited.components/log-viewer-webui/client/src/routes.tsx (1)
1-1: 💡 Verification agent🧩 Analysis chain
Verify the correct import source for createBrowserRouter
The createBrowserRouter function is typically imported from 'react-router-dom', not 'react-router'. This might cause issues when building the application.
🌐 Web query:
Is createBrowserRouter imported from react-router or react-router-dom in React Router v7?💡 Result:
In React Router v7,
createBrowserRouteris imported fromreact-router-dom, aligning with previous versions. However, there's an important nuance in v7's package structure. The core routing logic has been moved to thereact-routerpackage, while DOM-specific routers likecreateBrowserRouterremain inreact-router-domfor web applications[3][6].Key points:
- Standard import: Use
import { createBrowserRouter } from "react-router-dom";for most web applications[1][3]- Package structure changes:
- Core routing utilities moved to
react-router- DOM-specific exports remain in
react-router-dom[6]- TypeScript considerations: The
react-router-dompackage still provides type definitions for DOM-specific routers[3]Example usage:
import { createBrowserRouter } from 'react-router-dom'; import { RouterProvider } from 'react-router-dom'; const router = createBrowserRouter([/* routes */]); function App() { return <RouterProvider router={router} />; }For server-side rendering or non-DOM environments, you'd use
react-routerdirectly, butcreateBrowserRouterspecifically targets browser environments and remains inreact-router-dom[3][6]. The package structure changes were made to better separate core routing logic from environment-specific implementations[6][9].Citations:
- 1: https://reactrouter.docschina.org/routers/create-browser-router/
- 2: https://www.wisp.blog/blog/react-router-7-is-here-whats-new-and-whats-breaking
- 3: https://dev.to/utkvishwas/react-router-v7-a-comprehensive-guide-migration-from-v6-7d1
- 4: https://reactrouter.com/6.30.0/routers/create-browser-router
- 5: https://docs.sentry.io/platforms/javascript/guides/react/features/react-router/v7/
- 6: https://reactrouter.com/upgrading/v6
- 7: https://api.reactrouter.com/v7/functions/react_router.createBrowserRouter.html
- 8: https://www.youtube.com/watch?v=h7MTWLv3xvw
- 9: remix-run/react-router#12389
Fix the import source for createBrowserRouter
The web query confirms that for browser-based applications in React Router v7, the
createBrowserRouterfunction should be imported fromreact-router-domrather thanreact-router. Please update the import accordingly.
- Affected file:
components/log-viewer-webui/client/src/routes.tsx- Suggested change:
-import {createBrowserRouter} from "react-router"; +import {createBrowserRouter} from "react-router-dom";⛔ Skipped due to learnings
Learnt from: davemarco PR: y-scope/clp#797 File: components/log-viewer-webui/client/src/routes/routes.tsx:1-1 Timestamp: 2025-04-07T20:38:08.715Z 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.components/log-viewer-webui/client/src/ui/SearchView/index.tsx (1)
5-16: LGTM - Clean implementation of search interfaceThe component structure is clean and well-organized. Using SearchContextProvider aligns well with the PR objective of streamlining component structure and reducing prop drilling.
components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/index.module.css (1)
1-7: LGTM - Clean and simple stylingThe CSS classes are straightforward and serve their purpose well. The min-width on the range picker ensures a good user experience.
components/log-viewer-webui/client/src/ui/IngestView.tsx (1)
1-13: JSDoc does not match implementationThe JSDoc comment states that this component "Presents compression statistics", but the current implementation simply displays a header and placeholder text with no statistics. Consider updating the JSDoc to match the component's actual functionality or implementing the statistics display.
Based on the relevant code snippet from the original WebUI implementation, this component appears to be a simplified placeholder that doesn't yet include the statistics functionality present in the original.
components/log-viewer-webui/client/src/AntdApp.tsx (1)
1-3: Routing implementation looks good!The implementation of React Router is a solid architecture choice for managing navigation in this application. The router configuration imported from
./routeswill make it easier to maintain and extend the routing structure as the application grows.Also applies to: 12-12
components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/index.tsx (1)
31-76: Well-structured time range component!The TimeRangeInput component is well designed with clear separation of concerns between the dropdown selection and the conditional date picker. This matches well with the search feature requirements mentioned in the PR.
components/log-viewer-webui/client/src/ui/SearchView/SearchContext.tsx (3)
9-15: Well-designed context interface!The SearchContextType interface is properly typed and provides a clear contract for the context values and functions.
20-28: Good use of immutability patternsUsing
Readonly<>andObject.freeze()for the default context values is an excellent practice to prevent accidental mutations.
43-60: Excellent Provider implementationThe SearchContextProvider is well implemented following React best practices. Using the default values from SEARCH_CONTEXT_DEFAULT ensures consistency.
components/log-viewer-webui/client/src/ui/MainLayout.tsx (4)
2-5: Good adoption of React RouterThe inclusion of Link and Outlet components from react-router is the correct approach for implementing client-side routing.
17-17: Improved style organization with CSS modulesSwitching to CSS modules is a great improvement for style encapsulation.
25-26: Navigation structure looks greatThe use of Link components for menu items creates a proper navigation structure.
58-60: Well-placed Outlet componentThe Outlet placement is perfect for rendering nested routes within the layout structure.
components/log-viewer-webui/client/src/ui/SearchView/SearchControls/TimeRangeInput/utils.tsx (5)
1-1: LGTM: Appropriate import for date manipulation.The import of dayjs is appropriate for the date manipulation required in this file.
7-16: LGTM: Well-defined enum with clear descriptive names.The TIME_RANGE_OPTION enum is well structured with descriptive names that clearly communicate the available time range options.
45-47: LGTM: Correctly follows coding guidelines with Yoda condition.The code correctly uses the Yoda condition format (
"string" === typeof value) which aligns with the coding guidelines to prevent accidental assignments.
55-59: LGTM: Well-implemented type predicate function.The
isValidDateRangefunction is well implemented as a TypeScript type predicate, ensuring proper type safety. The null checks also follow the coding guidelines by using the Yoda condition format.
61-63: LGTM: Clean and organized exports.The exports are well organized, making all the necessary utilities available for other components.
| const {queryString} = useContext(SearchContext); | ||
|
|
||
| const isQueryStringValid: boolean = queryString === SEARCH_CONTEXT_DEFAULT.queryString; |
There was a problem hiding this comment.
Logic error in query validation check
The variable name isQueryStringValid suggests you're checking if the query is valid, but the implementation is checking if it's equal to the default value. This appears to be inverted logic. A valid query string would typically be non-empty and different from the default.
Consider changing this condition to something like:
-const isQueryStringValid: boolean = queryString === SEARCH_CONTEXT_DEFAULT.queryString;
+const isQueryStringValid: boolean = queryString !== SEARCH_CONTEXT_DEFAULT.queryString && queryString.trim() !== "";📝 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.
| const {queryString} = useContext(SearchContext); | |
| const isQueryStringValid: boolean = queryString === SEARCH_CONTEXT_DEFAULT.queryString; | |
| const {queryString} = useContext(SearchContext); | |
| - const isQueryStringValid: boolean = queryString === SEARCH_CONTEXT_DEFAULT.queryString; | |
| + const isQueryStringValid: boolean = queryString !== SEARCH_CONTEXT_DEFAULT.queryString && queryString.trim() !== ""; |
| return ( | ||
| <Tooltip | ||
| title={isQueryStringValid ? | ||
| "Enter query to search" : | ||
| ""} | ||
| > | ||
| <Button | ||
| className={styles["gradient-button"] || ""} | ||
| disabled={false === isQueryStringValid} | ||
| icon={<SearchOutlined/>} | ||
| size={"large"} | ||
| type={"primary"} | ||
| > | ||
| Search | ||
| </Button> | ||
| </Tooltip> | ||
| ); |
There was a problem hiding this comment.
Button functionality has several issues
The button implementation has several issues:
- The tooltip shows "Enter query to search" when
isQueryStringValidis true, which seems counterintuitive. - The button is disabled when the query is valid, which is the opposite of expected behavior.
- The CSS class fallback uses the OR operator (
||) which is unnecessary in most cases. - There's no onClick handler for the search functionality.
Consider these modifications:
<Tooltip
- title={isQueryStringValid ?
- "Enter query to search" :
- ""}
+ title={isQueryStringValid ? "" : "Enter query to search"}
>
<Button
- className={styles["gradient-button"] || ""}
+ className={styles["gradient-button"]}
- disabled={false === isQueryStringValid}
+ disabled={false === isQueryStringValid}
+ onClick={() => {/* Implement search functionality */}}
icon={<SearchOutlined/>}
size={"large"}
type={"primary"}
>
Search
</Button>
</Tooltip>📝 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.
| return ( | |
| <Tooltip | |
| title={isQueryStringValid ? | |
| "Enter query to search" : | |
| ""} | |
| > | |
| <Button | |
| className={styles["gradient-button"] || ""} | |
| disabled={false === isQueryStringValid} | |
| icon={<SearchOutlined/>} | |
| size={"large"} | |
| type={"primary"} | |
| > | |
| Search | |
| </Button> | |
| </Tooltip> | |
| ); | |
| return ( | |
| <Tooltip | |
| title={isQueryStringValid ? "" : "Enter query to search"} | |
| > | |
| <Button | |
| className={styles["gradient-button"]} | |
| disabled={false === isQueryStringValid} | |
| onClick={() => {/* Implement search functionality */}} | |
| icon={<SearchOutlined/>} | |
| size={"large"} | |
| type={"primary"} | |
| > | |
| Search | |
| </Button> | |
| </Tooltip> | |
| ); |
| const SearchControls = () => { | ||
| const {queryString, setQueryString} = useContext(SearchContext); | ||
|
|
||
| return ( | ||
| <div className={styles["search-controls-container"]}> | ||
| <Input | ||
| placeholder={"Enter your query"} | ||
| size={"large"} | ||
| value={queryString} | ||
| onChange={(e) => { | ||
| setQueryString(e.target.value); | ||
| }}/> | ||
| <TimeRangeInput/> | ||
| <SearchButton/> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Form structure and submission handling
The search controls are not wrapped in a form element, which would provide better semantics and allow for keyboard-based submission (pressing Enter). Additionally, there's no logic to handle the actual search submission.
Consider these improvements:
const SearchControls = () => {
const {queryString, setQueryString} = useContext(SearchContext);
+
+ const handleSubmit = (e) => {
+ e.preventDefault();
+ // Implement search logic here
+ };
return (
- <div className={styles["search-controls-container"]}>
+ <form className={styles["search-controls-container"]} onSubmit={handleSubmit}>
<Input
placeholder={"Enter your query"}
size={"large"}
value={queryString}
onChange={(e) => {
setQueryString(e.target.value);
}}/>
<TimeRangeInput/>
<SearchButton/>
- </div>
+ </form>
);
};📝 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.
| const SearchControls = () => { | |
| const {queryString, setQueryString} = useContext(SearchContext); | |
| return ( | |
| <div className={styles["search-controls-container"]}> | |
| <Input | |
| placeholder={"Enter your query"} | |
| size={"large"} | |
| value={queryString} | |
| onChange={(e) => { | |
| setQueryString(e.target.value); | |
| }}/> | |
| <TimeRangeInput/> | |
| <SearchButton/> | |
| </div> | |
| ); | |
| }; | |
| const SearchControls = () => { | |
| const {queryString, setQueryString} = useContext(SearchContext); | |
| const handleSubmit = (e) => { | |
| e.preventDefault(); | |
| // Implement search logic here | |
| }; | |
| return ( | |
| <form className={styles["search-controls-container"]} onSubmit={handleSubmit}> | |
| <Input | |
| placeholder={"Enter your query"} | |
| size={"large"} | |
| value={queryString} | |
| onChange={(e) => { | |
| setQueryString(e.target.value); | |
| }}/> | |
| <TimeRangeInput/> | |
| <SearchButton/> | |
| </form> | |
| ); | |
| }; |
| /* eslint-disable no-magic-numbers */ | ||
| const timeRangeOptionDayJsMap: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = { | ||
| [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(15, "minute"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(1, "hour"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"), | ||
| dayjs().endOf("day")], | ||
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().add(-1, "d"), | ||
| dayjs().add(-1, "d")], | ||
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().add(-7, "d"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().add(-30, "d"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.MONTH_TO_DATE]: [dayjs().startOf("month"), | ||
| dayjs()], | ||
|
|
||
| // Custom option is just a placeholder for typing purposes, it's dayJs values should not | ||
| // be used. | ||
| [TIME_RANGE_OPTION.CUSTOM]: [dayjs(), | ||
| dayjs()], | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve consistency in date manipulation methods and fix YESTERDAY implementation.
There are a few inconsistencies and potential issues in the time range implementations:
- The YESTERDAY implementation only gives the same time on the previous day, not the full day.
- There's inconsistent usage of date manipulation methods: sometimes using
subtract()and other times usingadd(-n). - Time unit specifications vary between full names and abbreviations ("minute", "hour", "d").
Consider applying these improvements:
const timeRangeOptionDayJsMap: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = {
[TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(15, "minute"),
dayjs()],
[TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(1, "hour"),
dayjs()],
[TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"),
dayjs().endOf("day")],
- [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().add(-1, "d"),
- dayjs().add(-1, "d")],
+ [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "day").startOf("day"),
+ dayjs().subtract(1, "day").endOf("day")],
- [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().add(-7, "d"),
+ [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(7, "day"),
dayjs()],
- [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().add(-30, "d"),
+ [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(30, "day"),
dayjs()],
[TIME_RANGE_OPTION.MONTH_TO_DATE]: [dayjs().startOf("month"),
dayjs()],📝 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.
| /* eslint-disable no-magic-numbers */ | |
| const timeRangeOptionDayJsMap: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = { | |
| [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(15, "minute"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(1, "hour"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"), | |
| dayjs().endOf("day")], | |
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().add(-1, "d"), | |
| dayjs().add(-1, "d")], | |
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().add(-7, "d"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().add(-30, "d"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.MONTH_TO_DATE]: [dayjs().startOf("month"), | |
| dayjs()], | |
| // Custom option is just a placeholder for typing purposes, it's dayJs values should not | |
| // be used. | |
| [TIME_RANGE_OPTION.CUSTOM]: [dayjs(), | |
| dayjs()], | |
| }; | |
| /* eslint-disable no-magic-numbers */ | |
| const timeRangeOptionDayJsMap: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = { | |
| [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(15, "minute"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(1, "hour"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"), | |
| dayjs().endOf("day")], | |
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "day").startOf("day"), | |
| dayjs().subtract(1, "day").endOf("day")], | |
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(7, "day"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(30, "day"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.MONTH_TO_DATE]: [dayjs().startOf("month"), | |
| dayjs()], | |
| // Custom option is just a placeholder for typing purposes, it's dayJs values should not | |
| // be used. | |
| [TIME_RANGE_OPTION.CUSTOM]: [dayjs(), | |
| dayjs()], | |
| }; |
hoophalab
left a comment
There was a problem hiding this comment.
The main feedback is that we could make the search bar a separate component. There are only two layers that need to pass the props down. That said, the current code works perfectly fine, and making this change wouldn’t affect the functionality.
| <h1>Search Page</h1> | ||
| <p>This is the Search Page.</p> | ||
| </div> | ||
| <SearchControls/> |
There was a problem hiding this comment.
It would be great if we could make SearchControls a reusable component and manage it through props, similar to mui's Select (code).
We could have queryString, timeRange, onQueryStringChange, onTimeRangeChange, and onSearchClicked.
But of course, whether making this change worth the time depends on your opinion.
There was a problem hiding this comment.
if we're to reuse the <SearchControls/> component, the suggestion makes sense
before we proceed, do we see where we will be reusing the component? i.e., is there any other page / place we will be using this?
| dayjs()], | ||
| [TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"), | ||
| dayjs().endOf("day")], | ||
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(-1, "d"), |
There was a problem hiding this comment.
Silly coderabbit comments 😆... this line was correct before but it should be dayjs().subtract(1, "d")?
|
This is now ready to review |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
14-31: Consider making SearchControls a reusable component with props.The component currently accesses state directly via
useSearchStore. This makes it less reusable in other contexts. Consider making it more flexible by accepting props for query string and handlers.A more reusable approach could look like:
-const SearchControls = () => { - const queryString = useSearchStore((state) => state.queryString); - const updateQueryString = useSearchStore((state) => state.updateQueryString); +interface SearchControlsProps { + queryString: string; + onQueryStringChange: (value: string) => void; + onSearchClicked: () => void; + // Add other props as needed +} + +const SearchControls = ({ + queryString, + onQueryStringChange, + onSearchClicked, + // Other props +}: SearchControlsProps) => {Then, in the parent component, you would connect to the store:
const SearchPage = () => { const queryString = useSearchStore((state) => state.queryString); const updateQueryString = useSearchStore((state) => state.updateQueryString); return ( <SearchControls queryString={queryString} onQueryStringChange={updateQueryString} onSearchClicked={() => {/* logic here */}} /> ); };components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (1)
63-72: Add spacing between select and date pickerConsider adding some spacing between the picker and the select for better visual separation.
You could either:
- Add margin-left to the DatePicker
- Use a fragment with spacing, like:
- {selectedOption === TIME_RANGE_OPTION.CUSTOM && ( - <DatePicker.RangePicker + {selectedOption === TIME_RANGE_OPTION.CUSTOM && ( + <DatePicker.RangePicker + style={{ marginLeft: '8px' }} className={styles["rangePicker"] || ""} showNow={true} showTime={true} size={"large"} onChange={(dates) => { handleRangePickerChange(dates); }}/> )}Or modify the CSS in the rangePicker class to include margin.
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (1)
18-39: Consider revising the YESTERDAY time range definitionThe YESTERDAY time range currently sets both start and end to the same day.js instance, which represents a point in time rather than a full day range.
- [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "d"), - dayjs().subtract(1, "d")], + [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "d").startOf("day"), + dayjs().subtract(1, "d").endOf("day")],This change would make YESTERDAY consistent with TODAY, providing a full day range rather than a single point in time.
🧹 Nitpick comments (4)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton.tsx (2)
19-20: Use direct empty string comparison for clarityPer the coding guidelines, and for better readability, consider comparing directly with an empty string rather than against a default value. This makes the code's intent clearer.
- const isQueryStringEmpty: boolean = - queryString === SEARCH_STATE_DEFAULT.queryString; + const isQueryStringEmpty: boolean = + queryString === "";If the default value in
SEARCH_STATE_DEFAULTmight change, consider adding a comment explaining why you're checking against it instead of an empty string.
30-30: Follow coding guideline for boolean expressionAccording to the coding guidelines, prefer using
false == <expression>rather than using negated expressions.- disabled={isQueryStringEmpty} + disabled={false == !isQueryStringEmpty}components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (1)
45-47: Follow coding guideline for boolean expressionAccording to the coding guidelines, prefer using
false == <expression>rather than negated expressions.- if (!isValidDateRange(dates)) { + if (false == isValidDateRange(dates)) { return; }components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (1)
18-39: Consider defining constants for magic numbersWhile you've correctly disabled the eslint rule for magic numbers, it would improve readability to define constants for values like 15, 1, 7, and 30.
+// Time constants in various units +const FIFTEEN_MINUTES = 15; +const ONE_HOUR = 1; +const ONE_DAY = 1; +const SEVEN_DAYS = 7; +const THIRTY_DAYS = 30; + const TIME_RANGE_OPTION_DAYJS_MAP: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = { - [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(15, "minute"), + [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(FIFTEEN_MINUTES, "minute"), dayjs()], - [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(1, "hour"), + [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(ONE_HOUR, "hour"), dayjs()], [TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"), dayjs().endOf("day")], - [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "d"), - dayjs().subtract(1, "d")], + [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(ONE_DAY, "d"), + dayjs().subtract(ONE_DAY, "d")], - [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(7, "d"), + [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(SEVEN_DAYS, "d"), dayjs()], - [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(30, "d"), + [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(THIRTY_DAYS, "d"), dayjs()],
📜 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 (9)
components/log-viewer-webui/client/package.json(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.module.css(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/index.module.css(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/index.module.css
- components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.module.css
🚧 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/pages/SearchPage/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx
🧬 Code Graph Analysis (2)
components/log-viewer-webui/client/src/pages/SearchPage/index.tsx (1)
components/webui/imports/ui/SearchView/SearchControls/index.jsx (1)
SearchControls(46-161)
components/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsx (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (2)
TIME_RANGE_OPTION_DAYJS_MAP(64-64)TIME_RANGE_OPTION(63-63)
🔇 Additional comments (9)
components/log-viewer-webui/client/src/pages/SearchPage/index.tsx (1)
1-11: Looks good - nice simplification of the SearchPage component.The refactoring to use the new
SearchControlscomponent simplifies theSearchPagesubstantially, making it more focused on its core functionality. This is a good architectural change that aligns with the PR objectives of enhancing the search functionality.components/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsx (5)
1-7: Good choice using Zustand for state managementThe imports are well-organized, with direct imports for dayjs and the zustand create function. The use of named imports for the time range constants is also appropriate.
10-16: Good use of Object.freeze for constantsUsing Object.freeze for the default state values is a good practice to prevent accidental mutations. The default values are sensible, with an empty query string and a default time range set to "today".
18-23: Well-defined TypeScript interfaceThe SearchState interface is well-structured with clear type definitions. The update methods have appropriate parameter types. This provides good type safety for the state management.
25-34: Clean Zustand store implementationThe implementation of the useSearchStore is concise and follows Zustand best practices. The state is properly initialized with default values, and the update methods correctly use the set function.
36-37: Appropriate exportsExporting both the SEARCH_STATE_DEFAULT constant and the useSearchStore as default follows good module design patterns, allowing consumers to import just what they need.
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (3)
7-16: Well-structured time range options enumThe TIME_RANGE_OPTION enum provides a comprehensive set of time range options with descriptive string values, making it user-friendly and easy to understand.
45-47: Good use of Object.freeze and type checkingProperly using Object.freeze to create an immutable array of option names. The condition
"string" === typeof valuefollows the coding guideline to preferfalse == <expression>rather than!<expression>.
55-59: Well-implemented type predicate functionThe isValidDateRange function is a well-written TypeScript type predicate that properly checks for null values. The condition
null !== datesfollows the coding guideline. This is a good practice for type narrowing in TypeScript.
There was a problem hiding this comment.
React Context does not scale very well to this day (React 19) because every update in the context value will cause all subscribing components to rerender.
Can we replace this context with a Zustand store? If you think this will cost you for more than an hour, I can help making the shift.
There was a problem hiding this comment.
sorry, i didn't realize this comment wasn't posted yesterday before the sync up. anyways thanks for quickly putting up the Zustand store
| <h1>Search Page</h1> | ||
| <p>This is the Search Page.</p> | ||
| </div> | ||
| <SearchControls/> |
There was a problem hiding this comment.
if we're to reuse the <SearchControls/> component, the suggestion makes sense
before we proceed, do we see where we will be reusing the component? i.e., is there any other page / place we will be using this?
junhaoliao
left a comment
There was a problem hiding this comment.
just some nit-picking comments
There was a problem hiding this comment.
this looks very clean now. nice!
junhaoliao
left a comment
There was a problem hiding this comment.
i'll try to make the linter smarter at some point
|
This was trickier than I thought. Anyways now i know how to do it. @junhaoliao Ready for rereview |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (2)
64-71: Simplify event handler and address UI spacingThe nested arrow function is unnecessary and the range picker needs proper spacing for better UI.
{selectedOption === TIME_RANGE_OPTION.CUSTOM && ( <DatePicker.RangePicker className={styles["rangePicker"] || ""} showNow={true} showTime={true} size={"large"} - onChange={(dates) => { - handleRangePickerChange(dates); - }}/> + onChange={handleRangePickerChange}/> )}
48-74: 🛠️ Refactor suggestionConsider tuning component styling for unified appearance
The Select and DatePicker components should appear unified when in custom mode. You might want to adjust the styling to make them look like a single control rather than separate components.
<div className={styles["timeRangeInputContainer"]} > <Select defaultValue={DEFAULT_TIME_RANGE} listHeight={300} options={TIME_RANGE_OPTION_NAMES.map((option) => ({label: option, value: option}))} popupMatchSelectWidth={false} size={"large"} variant={"filled"} className={selectedOption === TIME_RANGE_OPTION.CUSTOM ? styles["select-container"] : ""} onChange={handleSelectChange}/> {selectedOption === TIME_RANGE_OPTION.CUSTOM && ( <DatePicker.RangePicker + style={{ marginLeft: '8px' }} className={styles["rangePicker"] || ""} showNow={true} showTime={true} size={"large"} onChange={(dates) => { handleRangePickerChange(dates); }}/> )} </div>components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (1)
28-29:⚠️ Potential issueFix YESTERDAY time range to span the entire day
The current implementation sets both the start and end to the same point in time. This should span the entire day.
- [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "d"), - dayjs().subtract(1, "d")], + [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "d").startOf("day"), + dayjs().subtract(1, "d").endOf("day")],
🧹 Nitpick comments (4)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (2)
42-44: Prefer Boolean comparison to negationAccording to your coding guidelines, we should prefer
false == <expression>rather than!<expression>.- if (!isValidDateRange(dates)) { + if (false == isValidDateRange(dates)) { return; }
59-61: Simplify conditional class name assignmentThe conditional class name logic can be simplified using the
classnameslibrary or a template literal.- className={selectedOption === TIME_RANGE_OPTION.CUSTOM ? - (styles["select-container"] || "") : - ""} + className={selectedOption === TIME_RANGE_OPTION.CUSTOM ? styles["select-container"] : ""}components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (2)
48-49: Simplify TIME_RANGE_OPTION_NAMES creationSince all enum values are strings, the filtering is unnecessary.
const TIME_RANGE_OPTION_NAMES = Object.freeze( - Object.values(TIME_RANGE_OPTION).filter((value) => "string" === typeof value) + Object.values(TIME_RANGE_OPTION) );
60-60: Adhere to coding guideline for comparison styleThe comparisons are correct but could be more readable.
- return null !== dates && null !== dates[0] && null !== dates[1]; + return dates !== null && dates[0] !== null && dates[1] !== null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.module.css(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.module.css
- components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/index.tsx
- components/log-viewer-webui/client/src/pages/SearchPage/index.tsx
- components/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsx
- components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton.tsx
🧰 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/SearchControls/TimeRangeInput/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx
🧬 Code Graph Analysis (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx (5)
TIME_RANGE_OPTION(67-67)DEFAULT_TIME_RANGE(65-65)TIME_RANGE_OPTION_DAYJS_MAP(68-68)isValidDateRange(66-66)TIME_RANGE_OPTION_NAMES(69-69)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
| /* eslint-disable no-magic-numbers */ | ||
| const TIME_RANGE_OPTION_DAYJS_MAP: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = { | ||
| [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(15, "minute"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(1, "hour"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"), | ||
| dayjs().endOf("day")], | ||
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "d"), | ||
| dayjs().subtract(1, "d")], | ||
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(7, "d"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(30, "d"), | ||
| dayjs()], | ||
| [TIME_RANGE_OPTION.MONTH_TO_DATE]: [dayjs().startOf("month"), | ||
| dayjs()], | ||
|
|
||
| // Custom option is just a placeholder for typing purposes, its DayJs values should not | ||
| // be used. | ||
| [TIME_RANGE_OPTION.CUSTOM]: [dayjs(), | ||
| dayjs()], | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove eslint disabling and use constants for magic numbers
Instead of disabling the eslint rule, define constants for these values to improve code maintainability.
- /* eslint-disable no-magic-numbers */
+ // Time constants in minutes/hours/days
+ const FIFTEEN_MINUTES = 15;
+ const ONE_HOUR = 1;
+ const ONE_DAY = 1;
+ const SEVEN_DAYS = 7;
+ const THIRTY_DAYS = 30;
const TIME_RANGE_OPTION_DAYJS_MAP: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = {
- [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(15, "minute"),
+ [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(FIFTEEN_MINUTES, "minute"),
dayjs()],
- [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(1, "hour"),
+ [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(ONE_HOUR, "hour"),
dayjs()],
[TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"),
dayjs().endOf("day")],
- [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "d"),
- dayjs().subtract(1, "d")],
+ [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(ONE_DAY, "d").startOf("day"),
+ dayjs().subtract(ONE_DAY, "d").endOf("day")],
- [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(7, "d"),
+ [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(SEVEN_DAYS, "d"),
dayjs()],
- [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(30, "d"),
+ [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(THIRTY_DAYS, "d"),
dayjs()],
[TIME_RANGE_OPTION.MONTH_TO_DATE]: [dayjs().startOf("month"),
dayjs()],📝 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.
| /* eslint-disable no-magic-numbers */ | |
| const TIME_RANGE_OPTION_DAYJS_MAP: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = { | |
| [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(15, "minute"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(1, "hour"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"), | |
| dayjs().endOf("day")], | |
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(1, "d"), | |
| dayjs().subtract(1, "d")], | |
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(7, "d"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(30, "d"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.MONTH_TO_DATE]: [dayjs().startOf("month"), | |
| dayjs()], | |
| // Custom option is just a placeholder for typing purposes, its DayJs values should not | |
| // be used. | |
| [TIME_RANGE_OPTION.CUSTOM]: [dayjs(), | |
| dayjs()], | |
| }; | |
| // Time constants in minutes/hours/days | |
| const FIFTEEN_MINUTES = 15; | |
| const ONE_HOUR = 1; | |
| const ONE_DAY = 1; | |
| const SEVEN_DAYS = 7; | |
| const THIRTY_DAYS = 30; | |
| const TIME_RANGE_OPTION_DAYJS_MAP: Record<TIME_RANGE_OPTION, [dayjs.Dayjs, dayjs.Dayjs]> = { | |
| [TIME_RANGE_OPTION.LAST_15_MINUTES]: [dayjs().subtract(FIFTEEN_MINUTES, "minute"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_HOUR]: [dayjs().subtract(ONE_HOUR, "hour"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.TODAY]: [dayjs().startOf("day"), | |
| dayjs().endOf("day")], | |
| [TIME_RANGE_OPTION.YESTERDAY]: [dayjs().subtract(ONE_DAY, "d").startOf("day"), | |
| dayjs().subtract(ONE_DAY, "d").endOf("day")], | |
| [TIME_RANGE_OPTION.LAST_7_DAYS]: [dayjs().subtract(SEVEN_DAYS, "d"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.LAST_30_DAYS]: [dayjs().subtract(THIRTY_DAYS, "d"), | |
| dayjs()], | |
| [TIME_RANGE_OPTION.MONTH_TO_DATE]: [dayjs().startOf("month"), | |
| dayjs()], | |
| // Custom option is just a placeholder for typing purposes, its DayJs values should not | |
| // be used. | |
| [TIME_RANGE_OPTION.CUSTOM]: [dayjs(), | |
| dayjs()], | |
| }; |
junhaoliao
left a comment
There was a problem hiding this comment.
just some questions / nit-picking comments. will approve once the suggestions are addressed
| } | ||
|
|
||
| /* Makes border flush with range picker */ | ||
| .select-container :global(.ant-select-selector) { |
There was a problem hiding this comment.
Is :global() the trick you were referring to?
There was a problem hiding this comment.
yes, effectively the component we want to modify is actually a descendant of the select antd component, which is trickier to modify.
I also played around with someone things, that didn't end up in final version,
| } | ||
|
|
||
| /* Makes border flush with range picker */ | ||
| .select-container :global(.ant-select-selector) { |
There was a problem hiding this comment.
since this is only applied when the date-picker shows up, how about
| .select-container :global(.ant-select-selector) { | |
| .selectWithRightAddon :global(.ant-select-selector) { |
There was a problem hiding this comment.
I called it customSelected, since i already have another component that refers to the combined element
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (3)
59-61: Class name selection could be simplified.The conditional class name assignment could be simplified using a common pattern in React:
- className={selectedOption === TIME_RANGE_OPTION.CUSTOM ? - (styles["customSelected"] || "") : - ""} + className={selectedOption === TIME_RANGE_OPTION.CUSTOM ? styles.customSelected : ""}Also, as per previous review comments, this class should potentially be renamed to
selectWithRightAddonto better reflect its purpose when paired with the date picker.
64-72: Optimize event handler and add margin for better UI.
- The arrow function wrapper for
handleRangePickerChangeis unnecessary since it's just passing through the arguments:- onChange={(dates) => { - handleRangePickerChange(dates); - }}/> + onChange={handleRangePickerChange}/>
- As suggested in a previous review comment, consider adding margin-left to the DatePicker or improving the styling so that the select and date picker appear as a cohesive unit when in custom mode.
76-77: Remove extra empty line.There are two consecutive empty lines where one would be sufficient according to the surrounding code style.
🧹 Nitpick comments (5)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (5)
31-37: Consider using yoda conditions for boolean comparisons.According to the provided coding guidelines, we should prefer
false == <expression>rather than!<expression>. Similarly, you might want to use the yoda condition pattern for equality checks:- if (timeRangeOption !== TIME_RANGE_OPTION.CUSTOM) { + if (TIME_RANGE_OPTION.CUSTOM !== timeRangeOption) {This is a minor suggestion that aligns with the project's coding guidelines.
42-44: Consider providing feedback for invalid date ranges.The component silently returns when date validation fails. Consider showing a notification or error message to provide feedback to the user about why their selection wasn't applied.
48-73: UI Enhancement: Consider adding animation for smoother transitions.When toggling between the default view and the custom date picker view, the sudden appearance/disappearance of the date picker might create an abrupt layout shift. Consider adding a simple CSS transition or animation to make this change smoother.
This could be implemented by always rendering the date picker but controlling its visibility and opacity with CSS transitions.
65-65: CSS class access could be simplified.- className={styles["rangePicker"] || ""} + className={styles.rangePicker}Using dot notation is cleaner when the property name is a valid identifier. The fallback to empty string is unnecessary if you're confident the class exists in your CSS module.
50-50: CSS class access could be simplified here too.- className={styles["timeRangeInputContainer"]} + className={styles.timeRangeInputContainer}For consistency, use dot notation for accessing CSS module classes when the property names are valid identifiers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.module.css(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.module.css
🧰 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/SearchControls/TimeRangeInput/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx (1)
20-26: Great documentation for the component.The JSDoc comment clearly explains the purpose and behavior of the component, which is excellent for maintainability.
|
i directly modified the PR title to use imperative mood |
Description
Adds an input bar, and date range picker, and a submit button to search view. Note the structure is not the exact same as original webui, I decided to make some changes that I think improve appearance vs. original webui.
In addition I added a search context. Original webui was passing down alot of props to lower level components. I think a search context is cleaner.
Lastly, I simplified date ranges from the old webui. We can go back later, but I didn't think such complexity was required to set up ui. Hopefully, something simpler like i set up is actually also good for final design, but TBD.
Checklist
breaking change.
Validation performed
Tested actual build this time, to make sure it dosent break like this morning.
We should probably eventually figure out why the linter didn't pick up the error.
Summary by CodeRabbit
SearchButtoncomponent with interactive feedback—disabling when no query is entered and providing on-hover guidance.TimeRangeInputcomponent for selecting time ranges with dropdown options and a date range picker.