feat(new-webui): Add compression job table to Ingest page.#848
Conversation
WalkthroughThis update introduces a new "Jobs" feature to the IngestPage of the log viewer web UI. It adds a new React component to display a table of ingestion job data, supported by a new TypeScript typings module and associated CSS styles. The IngestPage layout and grid CSS are updated to accommodate and style the new Jobs section, which currently displays hardcoded job data. Additional CSS refinements are made to the Details section and the main page grid for improved layout flexibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IngestPage
participant Jobs
participant AntDesignTable
User->>IngestPage: Loads IngestPage
IngestPage->>Jobs: Renders Jobs component
Jobs->>AntDesignTable: Passes jobColumns and DUMMY_DATA
AntDesignTable-->>Jobs: Renders job table
Jobs-->>IngestPage: Table displayed in grid layout
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
components/log-viewer-webui/client/src/pages/IngestPage/Jobs/typings.tsx (1)
26-57: Well-configured table columns with proper typingThe column definitions are properly typed using Ant Design's TableProps with appropriate fields. The custom render function for the status badge is a good approach for visual indication.
A minor formatting suggestion:
<Badge status={status} - text={status}/> + text={status} />components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsx (4)
1-8: Imports look good, but consider organizing them more consistently.The imports are functionally correct, but consider grouping them by type (external libraries, internal components, types, styles) with a blank line between groups for better readability.
import {Table} from "antd"; import {DashboardCard} from "../../../components/DashboardCard"; import styles from "./index.module.css"; + import { jobColumns, JobData, } from "./typings";
11-70: Consider adding pagination or virtualization for the job table.The dummy data array is well-structured, but having 7 items displayed without pagination might not scale well when real data is implemented. Consider implementing pagination or virtualization for better performance with larger datasets.
Also, the jobId and key properties have the same values. When you implement the real data, ensure that this pattern continues or consider generating the key values separately.
82-82: Defensive coding with styles fallback could be improved.Using
styles["jobs"] || ""is a good defensive approach, but there's a more TypeScript-friendly way to handle this that avoids potential runtime issues.- className={styles["jobs"] || ""} + className={styles.jobs}The TypeScript compiler should correctly type-check the styles object, and modern CSS module implementations automatically generate type definitions that make this safe.
73-78: JSDoc is missing a return type.The JSDoc comment for the Jobs component is incomplete. The @return tag should specify what the component returns.
/** * Renders table with ingestion jobs inside a card. * * @return + * @returns {JSX.Element} A React component displaying job data in a table */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/log-viewer-webui/client/src/pages/IngestPage/Details/index.module.css(1 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.module.css(1 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Jobs/typings.tsx(1 hunks)components/log-viewer-webui/client/src/pages/IngestPage/index.module.css(1 hunks)components/log-viewer-webui/client/src/pages/IngestPage/index.tsx(2 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/pages/IngestPage/index.tsxcomponents/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsxcomponents/log-viewer-webui/client/src/pages/IngestPage/Jobs/typings.tsx
🧬 Code Graph Analysis (1)
components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsx (2)
components/log-viewer-webui/client/src/pages/IngestPage/Jobs/typings.tsx (2)
JobData(59-59)jobColumns(61-61)components/log-viewer-webui/client/src/components/DashboardCard/index.tsx (1)
DashboardCard(48-48)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (9)
components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.module.css (1)
1-3: The CSS looks good!Simple, clean implementation of padding to create vertical spacing for the Jobs component.
components/log-viewer-webui/client/src/pages/IngestPage/index.module.css (2)
3-5: Great responsive grid implementation!The change to use
repeat(auto-fit, minmax(400px, 1fr))creates a flexible layout that adapts to different screen sizes. Adding the max-width constraint with an explanatory comment is a good practice that clearly communicates the intention to limit the dashboard to two columns.
10-12: Good approach for the jobs section layoutUsing
grid-column: 1 / -1for the jobs section ensures it spans the full width of the grid, which is appropriate for displaying tabular data that benefits from maximum available horizontal space.components/log-viewer-webui/client/src/pages/IngestPage/Jobs/typings.tsx (3)
1-8: Good type definitions using Ant Design componentsClean imports and proper type alias definition for the status colors based on Ant Design's Badge component.
10-20: Well-structured interface with good documentationThe JobData interface clearly defines the structure needed for the table data with appropriate types and JSDoc comments.
59-61: Clean exportsAppropriate exports using named export syntax.
components/log-viewer-webui/client/src/pages/IngestPage/Details/index.module.css (1)
3-3: Good responsive grid implementation for detailsThe change to use
repeat(2, minmax(200px, 1fr))makes the details grid responsive while maintaining a minimum column width. This approach is consistent with the responsive grid changes in the main page, creating a unified responsive experience.components/log-viewer-webui/client/src/pages/IngestPage/index.tsx (2)
3-3: Clean import of the new Jobs componentImport statement follows the established pattern in the file.
17-19: Good integration of the Jobs componentThe Jobs component is well-integrated into the page layout with a wrapper div using the appropriate CSS class. This structure allows the component to utilize the grid-column styling defined in the CSS module, ensuring it spans the full width of the grid as intended.
| <DashboardCard title={"Ingestion Jobs"}> | ||
| <Table<JobData> | ||
| className={styles["jobs"] || ""} | ||
| columns={jobColumns} | ||
| dataSource={DUMMY_DATA} | ||
| pagination={false}/> | ||
| </DashboardCard> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Table implementation looks good, but consider adding loading state and empty data handling.
The Table component is properly implemented with TypeScript generics, but it lacks handling for loading states, errors, or empty data scenarios which will be important when connecting to a real API.
Consider adding these properties to the Table component:
<Table<JobData>
className={styles["jobs"] || ""}
columns={jobColumns}
dataSource={DUMMY_DATA}
- pagination={false}/>
+ pagination={false}
+ loading={false}
+ locale={{ emptyText: "No job data available" }}
+ />
junhaoliao
left a comment
There was a problem hiding this comment.
code-wise looks good. will check aesthetics later
| <div className={styles["jobs"]}> | ||
| <Jobs/> | ||
| </div> |
There was a problem hiding this comment.
assuming <Jobs/> is only intended to be used in a grid on the Ingestion page, shall we move this style specification into the <Jobs/> component itself?
There was a problem hiding this comment.
I think I prefer the grid settings outside the component, since it is really layout css and not related to the component itself. We could maybe add a className prop, to hide the divs if you think that looks better
There was a problem hiding this comment.
add className ... to hide the divs
sounds good
junhaoliao
left a comment
There was a problem hiding this comment.
I was thinking if we could make the columns sortable, users will be able to categorize the jobs by their status, or figure out which job has the best / worst compression time. That said, we can (should?) work on that later once we connect the UI with real data.
For the PR title, how about:
feat(new-webui): Add compression job table to Ingest page.
| /** | ||
| * Renders table with ingestion jobs inside a card. | ||
| * | ||
| * @return | ||
| */ | ||
| const Jobs = ({className}: JobsProps) => { | ||
| return ( | ||
| <div className={className}> | ||
| <DashboardCard title={"Ingestion Jobs"}> | ||
| <Table<JobData> | ||
| className={styles["jobs"] || ""} | ||
| columns={jobColumns} | ||
| dataSource={DUMMY_DATA} | ||
| pagination={false}/> | ||
| </DashboardCard> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
would it be better to write
| /** | |
| * Renders table with ingestion jobs inside a card. | |
| * | |
| * @return | |
| */ | |
| const Jobs = ({className}: JobsProps) => { | |
| return ( | |
| <div className={className}> | |
| <DashboardCard title={"Ingestion Jobs"}> | |
| <Table<JobData> | |
| className={styles["jobs"] || ""} | |
| columns={jobColumns} | |
| dataSource={DUMMY_DATA} | |
| pagination={false}/> | |
| </DashboardCard> | |
| </div> | |
| ); | |
| }; | |
| /** | |
| * Renders table with ingestion jobs inside a card. | |
| * | |
| * @return | |
| */ | |
| const Jobs = ({...rest}: JobsProps) => { | |
| return ( | |
| <div {...rest}> | |
| <DashboardCard title={"Ingestion Jobs"}> | |
| <Table<JobData> | |
| className={styles["jobs"] || ""} | |
| columns={jobColumns} | |
| dataSource={DUMMY_DATA} | |
| pagination={false}/> | |
| </DashboardCard> | |
| </div> | |
| ); | |
| }; |
There was a problem hiding this comment.
I think because there is only one prop, for now I think its better as is.
|
I agree sorting may be helpful, but we can add a later date. Especially if we want to support server side filtering. |
Description
Adds a table for ingestion jobs into main dashboard.
The data for the jobs is fake. Once the database is implemented, some more work may need to be done
to process the data from database, but that can dealt with in a future PR.
In addition, some of the grid settings were tweaked slightly.
Checklist
breaking change.
Validation performed
Table renders and squishes to two columns to one column
Summary by CodeRabbit