feat: Replace existing webui codebase with new-webui.#939
Merged
Conversation
Contributor
## Walkthrough
This change removes the separate log-viewer-webui component and merges its functionality into a unified webui component. All code, configuration, and documentation related to the Meteor-based log-viewer-webui are deleted. The build, validation, and startup logic are refactored to support the consolidated webui, now structured as a React client and a Fastify server.
## Changes
| Files/Paths (grouped) | Change Summary |
|----------------------------------------------------------------------------------------------------------------|---------------|
| `components/log-viewer-webui/**`, `components/webui/imports/**`, `components/webui/client/main.*`, `components/webui/client/package.json`, `components/webui/client/public/settings.json`, `components/webui/client/src/*`, `components/webui/server/**`, `components/webui/settings.json`, `components/webui/launcher.js`, `components/webui/tests/**` | Deleted all Meteor-based log-viewer-webui and webui code, React components, server code, config, and tests. |
| `components/clp-py-utils/clp_py_utils/clp_config.py`, `components/clp-package-utils/clp_package_utils/general.py`, `components/clp-package-utils/clp_package_utils/scripts/start_clp.py`, `components/clp-package-utils/clp_package_utils/scripts/stop_clp.py`, `components/clp-py-utils/clp_py_utils/s3_utils.py` | Removed log-viewer-webui references, consolidated config/validation/start logic for unified webui. |
| `components/package-template/src/etc/clp-config.yml` | Updated config template: removed log_viewer_webui block, added results_metadata_collection_name. |
| `components/webui/.gitignore`, `components/webui/client/package.json`, `components/webui/server/package.json` | Updated ignore rules and package metadata for unified webui; renamed packages. |
| `components/webui/README.md`, `docs/src/dev-guide/components-webui.md` | Updated documentation for new unified webui architecture, removed Meteor references. |
| `docs/src/dev-guide/components-log-viewer-webui.md`, `docs/src/dev-guide/index.md` | Removed documentation for the old log-viewer-webui component. |
| `taskfile.yaml`, `taskfiles/lint.yaml`, `taskfiles/deps/main.yaml` | Refactored build, clean, and lint tasks to remove log-viewer-webui and support unified webui. |
| `components/webui/client/src/App.tsx`, `components/webui/client/src/main.tsx`, `components/webui/client/vite.config.ts` | Renamed and simplified App component; removed conditional logic for log-viewer-webui. |
| `components/webui/client/public/settings.json`, `components/webui/server/settings.json` | Reordered JSON keys for clarity. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant WebUI Client (React)
participant WebUI Server (Fastify)
participant CLP Core
User->>WebUI Client (React): Initiate action (e.g., search, ingest)
WebUI Client (React)->>WebUI Server (Fastify): API request (search, ingest, etc.)
WebUI Server (Fastify)->>CLP Core: Query/command (if needed)
CLP Core-->>WebUI Server (Fastify): Response/data
WebUI Server (Fastify)-->>WebUI Client (React): API response
WebUI Client (React)-->>User: Render results/UI updatePossibly related PRs
Suggested reviewers
|
3 tasks
Contributor
|
as discussed in meeting. I will make changes but I need someone (@haiqi96 , @kirkrodrigues , other?) to review original PR which replaces webui. Once everything else is done it can be merged |
# Conflicts: # components/log-viewer-webui/client/package-lock.json # components/log-viewer-webui/client/src/AntdApp.tsx # components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline.tsx
# Conflicts: # components/log-viewer-webui/client/package-lock.json
…nd update validation for web UI settings
…and task configurations
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (4)
components/clp-py-utils/clp_py_utils/s3_utils.py (2)
104-108: Docstring still references removed componentThe docstring mentions
log_viewer, which no longer exists after the merge. Update for clarity and future maintainability.- :param component_type: Type of calling container (compression, log_viewer, or query). + :param component_type: Calling component name + (compression-scheduler/worker, webui, query-scheduler/worker).
112-120: Edge case: tuple check hides accidental pluralUsing the singleton tuple
(WEBUI_COMPONENT_NAME,)is correct; however, if someone later appends another element without the trailing comma, the comparison will silently break. Consider converting to asetfor safer membership tests:- elif component_type in (WEBUI_COMPONENT_NAME,): + elif component_type in {WEBUI_COMPONENT_NAME}:components/clp-py-utils/clp_py_utils/clp_config.py (1)
571-590: New field validated correctly – just one minor style tweakLogic is fine and the validator signature matches pydantic’s expectations (Pylint’s E0213 is a false positive).
For readability, wrap the long error message at 100 chars to satisfy the project formatter.- raise ValueError(f"{WEBUI_COMPONENT_NAME}.results_metadata_collection_name cannot be empty.") + raise ValueError( + f"{WEBUI_COMPONENT_NAME}.results_metadata_collection_name cannot be empty." + )components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
842-968: Consider refactoring to reduce function complexity.The function has 28 local variables (recommended maximum is 15) and 55 statements (recommended maximum is 50). Consider extracting the settings update logic into separate helper functions to improve readability and maintainability.
For example, you could extract:
- Client settings update logic into
_update_client_settings()- Server settings update logic into
_update_server_settings()- S3 configuration logic into
_configure_s3_settings()Would you like me to generate the refactored code or open an issue to track this improvement?
♻️ Duplicate comments (1)
taskfiles/lint.yaml (1)
760-765: Same undefined variable used injstask
cd "{{.G_WEBUI_SRC_DIR}}"will fail for the same reason. Once the variable is re-established (see previous comment), this line becomes safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (7)
components/log-viewer-webui/package-lock.jsonis excluded by!**/package-lock.jsoncomponents/webui/client/package-lock.jsonis excluded by!**/package-lock.jsoncomponents/webui/client/public/clp-logo.pngis excluded by!**/*.pngcomponents/webui/client/public/favicon.svgis excluded by!**/*.svgcomponents/webui/linter/package-lock.jsonis excluded by!**/package-lock.jsoncomponents/webui/package-lock.jsonis excluded by!**/package-lock.jsoncomponents/webui/server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (100)
components/clp-package-utils/clp_package_utils/general.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/start_clp.py(6 hunks)components/clp-package-utils/clp_package_utils/scripts/stop_clp.py(0 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(2 hunks)components/clp-py-utils/clp_py_utils/s3_utils.py(2 hunks)components/log-viewer-webui/.gitignore(0 hunks)components/log-viewer-webui/README.md(0 hunks)components/log-viewer-webui/client/src/AntdApp.tsx(0 hunks)components/log-viewer-webui/client/src/App.tsx(0 hunks)components/log-viewer-webui/package.json(0 hunks)components/package-template/src/etc/clp-config.yml(1 hunks)components/webui/.gitignore(1 hunks)components/webui/.meteor/.finished-upgraders(0 hunks)components/webui/.meteor/.gitignore(0 hunks)components/webui/.meteor/.id(0 hunks)components/webui/.meteor/packages(0 hunks)components/webui/.meteor/platforms(0 hunks)components/webui/.meteor/release(0 hunks)components/webui/.meteor/versions(0 hunks)components/webui/.meteorignore(0 hunks)components/webui/README.md(1 hunks)components/webui/client/main.css(0 hunks)components/webui/client/main.html(0 hunks)components/webui/client/main.jsx(0 hunks)components/webui/client/package.json(1 hunks)components/webui/client/public/settings.json(1 hunks)components/webui/client/src/App.tsx(1 hunks)components/webui/client/src/main.tsx(1 hunks)components/webui/client/vite.config.ts(1 hunks)components/webui/imports/api/constants.js(0 hunks)components/webui/imports/api/ingestion/collections.js(0 hunks)components/webui/imports/api/ingestion/constants.js(0 hunks)components/webui/imports/api/ingestion/server/CompressionDbManager.js(0 hunks)components/webui/imports/api/ingestion/server/StatsDbManager.js(0 hunks)components/webui/imports/api/ingestion/server/publications.js(0 hunks)components/webui/imports/api/ingestion/types.js(0 hunks)components/webui/imports/api/search/SearchJobCollectionsManager.js(0 hunks)components/webui/imports/api/search/collections.js(0 hunks)components/webui/imports/api/search/constants.js(0 hunks)components/webui/imports/api/search/server/QueryJobsDbManager.js(0 hunks)components/webui/imports/api/search/server/collections.js(0 hunks)components/webui/imports/api/search/server/methods.js(0 hunks)components/webui/imports/api/search/server/publications.js(0 hunks)components/webui/imports/api/search/types.js(0 hunks)components/webui/imports/ui/App.jsx(0 hunks)components/webui/imports/ui/App.scss(0 hunks)components/webui/imports/ui/IngestView/IngestView.jsx(0 hunks)components/webui/imports/ui/IngestView/IngestView.scss(0 hunks)components/webui/imports/ui/IngestView/Panel.jsx(0 hunks)components/webui/imports/ui/IngestView/panels/Details/DetailsRow.jsx(0 hunks)components/webui/imports/ui/IngestView/panels/Details/index.jsx(0 hunks)components/webui/imports/ui/IngestView/panels/IngestionJobs/IngestionJobRow.jsx(0 hunks)components/webui/imports/ui/IngestView/panels/IngestionJobs/IngestionJobs.scss(0 hunks)components/webui/imports/ui/IngestView/panels/IngestionJobs/PlaceholderText.jsx(0 hunks)components/webui/imports/ui/IngestView/panels/IngestionJobs/PlaceholderText.scss(0 hunks)components/webui/imports/ui/IngestView/panels/IngestionJobs/index.jsx(0 hunks)components/webui/imports/ui/IngestView/panels/SpaceSavings.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsCaseSensitivity/SearchControlsCaseSensitivityCheck.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsCaseSensitivity/index.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsFilterDrawer.scss(0 hunks)components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsFilterLabel.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsTimeRangeInput/SearchControlsDatePicker.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsTimeRangeInput/SearchControlsTimeRangeInput.scss(0 hunks)components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsTimeRangeInput/index.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/index.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchControls/index.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchResults/SearchResultsHeader/SearchResultsHeader.scss(0 hunks)components/webui/imports/ui/SearchView/SearchResults/SearchResultsHeader/index.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/SearchResultsLoadSensor.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/SearchResultsLoadSensor.scss(0 hunks)components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/SearchResultsTable.scss(0 hunks)components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/index.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchResults/SearchResultsTimeline/SearchResultsTimeline.scss(0 hunks)components/webui/imports/ui/SearchView/SearchResults/SearchResultsTimeline/index.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchResults/index.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchStatus.jsx(0 hunks)components/webui/imports/ui/SearchView/SearchStatus.scss(0 hunks)components/webui/imports/ui/SearchView/SearchView.jsx(0 hunks)components/webui/imports/ui/Sidebar/Sidebar.jsx(0 hunks)components/webui/imports/ui/Sidebar/Sidebar.scss(0 hunks)components/webui/imports/ui/Sidebar/SidebarButton.jsx(0 hunks)components/webui/imports/ui/bootstrap-customized.scss(0 hunks)components/webui/imports/ui/constants.js(0 hunks)components/webui/imports/utils/DbManager.js(0 hunks)components/webui/imports/utils/datetime.js(0 hunks)components/webui/imports/utils/logger.js(0 hunks)components/webui/imports/utils/misc.js(0 hunks)components/webui/imports/utils/mongo.js(0 hunks)components/webui/launcher.js(0 hunks)components/webui/linter/package.json(0 hunks)components/webui/package.json(1 hunks)components/webui/server/main.js(0 hunks)components/webui/server/package.json(1 hunks)components/webui/server/settings.json(1 hunks)components/webui/settings.json(0 hunks)components/webui/tests/main.js(0 hunks)components/webui/tests/misc.js(0 hunks)taskfile.yaml(8 hunks)taskfiles/deps/main.yaml(1 hunks)taskfiles/lint.yaml(2 hunks)
💤 Files with no reviewable changes (82)
- components/webui/.meteor/release
- components/webui/.meteor/.id
- components/webui/.meteorignore
- components/webui/.meteor/platforms
- components/webui/imports/ui/IngestView/panels/IngestionJobs/PlaceholderText.scss
- components/log-viewer-webui/README.md
- components/webui/client/main.css
- components/webui/.meteor/.gitignore
- components/clp-package-utils/clp_package_utils/scripts/stop_clp.py
- components/webui/imports/ui/App.scss
- components/log-viewer-webui/.gitignore
- components/webui/imports/ui/SearchView/SearchResults/SearchResultsHeader/SearchResultsHeader.scss
- components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsTimeRangeInput/SearchControlsTimeRangeInput.scss
- components/webui/imports/ui/IngestView/panels/IngestionJobs/IngestionJobs.scss
- components/webui/imports/ui/SearchView/SearchResults/SearchResultsTimeline/SearchResultsTimeline.scss
- components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/SearchResultsLoadSensor.scss
- components/webui/.meteor/.finished-upgraders
- components/webui/imports/api/search/server/collections.js
- components/webui/imports/ui/IngestView/IngestView.jsx
- components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsCaseSensitivity/SearchControlsCaseSensitivityCheck.jsx
- components/log-viewer-webui/package.json
- components/webui/imports/api/constants.js
- components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsFilterDrawer.scss
- components/webui/client/main.html
- components/webui/tests/main.js
- components/log-viewer-webui/client/src/App.tsx
- components/webui/imports/api/ingestion/types.js
- components/webui/client/main.jsx
- components/webui/.meteor/versions
- components/webui/imports/ui/SearchView/SearchStatus.scss
- components/webui/imports/ui/Sidebar/Sidebar.scss
- components/webui/tests/misc.js
- components/webui/imports/utils/mongo.js
- components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsFilterLabel.jsx
- components/webui/imports/api/search/collections.js
- components/webui/linter/package.json
- components/webui/imports/ui/IngestView/panels/IngestionJobs/index.jsx
- components/webui/imports/ui/IngestView/panels/Details/DetailsRow.jsx
- components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsCaseSensitivity/index.jsx
- components/webui/imports/api/search/types.js
- components/webui/imports/ui/IngestView/panels/Details/index.jsx
- components/webui/imports/utils/DbManager.js
- components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsTimeRangeInput/SearchControlsDatePicker.jsx
- components/webui/imports/ui/IngestView/panels/SpaceSavings.jsx
- components/webui/imports/ui/constants.js
- components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/SearchResultsTable.scss
- components/webui/imports/ui/IngestView/panels/IngestionJobs/PlaceholderText.jsx
- components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/index.jsx
- components/webui/imports/api/ingestion/collections.js
- components/webui/imports/ui/SearchView/SearchStatus.jsx
- components/webui/imports/ui/Sidebar/Sidebar.jsx
- components/webui/imports/api/ingestion/constants.js
- components/webui/imports/ui/IngestView/IngestView.scss
- components/webui/imports/ui/IngestView/panels/IngestionJobs/IngestionJobRow.jsx
- components/webui/launcher.js
- components/webui/imports/ui/Sidebar/SidebarButton.jsx
- components/webui/imports/api/ingestion/server/CompressionDbManager.js
- components/webui/imports/ui/App.jsx
- components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/SearchResultsLoadSensor.jsx
- components/webui/imports/utils/misc.js
- components/webui/server/main.js
- components/webui/imports/ui/IngestView/Panel.jsx
- components/webui/imports/api/ingestion/server/StatsDbManager.js
- components/webui/imports/utils/logger.js
- components/webui/imports/api/search/server/QueryJobsDbManager.js
- components/webui/imports/ui/SearchView/SearchResults/index.jsx
- components/webui/imports/api/search/server/publications.js
- components/webui/imports/ui/SearchView/SearchView.jsx
- components/log-viewer-webui/client/src/AntdApp.tsx
- components/webui/imports/ui/SearchView/SearchResults/SearchResultsTimeline/index.jsx
- components/webui/imports/ui/SearchView/SearchResults/SearchResultsHeader/index.jsx
- components/webui/imports/ui/SearchView/SearchResults/SearchResultsTable/index.jsx
- components/webui/.meteor/packages
- components/webui/imports/ui/bootstrap-customized.scss
- components/webui/imports/ui/SearchView/SearchControls/SearchControlsFilterDrawer/SearchControlsTimeRangeInput/index.jsx
- components/webui/imports/utils/datetime.js
- components/webui/imports/api/search/server/methods.js
- components/webui/imports/api/ingestion/server/publications.js
- components/webui/settings.json
- components/webui/imports/api/search/constants.js
- components/webui/imports/api/search/SearchJobCollectionsManager.js
- components/webui/imports/ui/SearchView/SearchControls/index.jsx
🧰 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/webui/client/vite.config.tscomponents/webui/client/src/main.tsxcomponents/webui/client/src/App.tsx
🧬 Code Graph Analysis (2)
components/clp-package-utils/clp_package_utils/general.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
CLPConfig(593-778)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (2)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
CLPConfig(593-778)components/clp-package-utils/clp_package_utils/general.py (3)
CLPDockerMounts(80-90)get_clp_home(93-109)validate_webui_config(512-519)
🪛 Pylint (3.3.7)
components/clp-package-utils/clp_package_utils/general.py
[convention] 512-512: Missing function or method docstring
(C0116)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py
[convention] 869-869: Line too long (104/100)
(C0301)
[convention] 885-885: Line too long (104/100)
(C0301)
[convention] 842-842: Missing function or method docstring
(C0116)
[refactor] 842-842: Too many local variables (28/15)
(R0914)
[warning] 849-849: Use lazy % formatting in logging functions
(W1203)
[warning] 874-874: Using open without explicitly specifying an encoding
(W1514)
[refactor] 842-842: Too many statements (55/50)
(R0915)
[warning] 916-916: Using open without explicitly specifying an encoding
(W1514)
components/clp-py-utils/clp_py_utils/clp_config.py
[convention] 589-589: Line too long (105/100)
(C0301)
[convention] 587-587: Missing function or method docstring
(C0116)
[error] 587-587: Method 'validate_results_metadata_collection_name' should have "self" as first argument
(E0213)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (17)
components/webui/.gitignore (1)
1-1: Add a header to group dependency rules
The# Dependenciescomment improves readability by clearly separating dependency patterns from other ignores.components/webui/server/settings.json (1)
11-11: Reordering JSON keys is cosmetic.
Moving"MongoDbStreamFilesCollectionName"below"MongoDbSearchResultsMetadataCollectionName"does not change any values or runtime behaviour.components/webui/server/package.json (1)
2-2: Package name updated to unified component.
Renaming"name"to"webui-server"aligns with the consolidated WebUI architecture. Ensure there are no leftover references to the old package name by running:#!/bin/bash # Search for legacy package name rg -l '"log-viewer-webui-server"'components/webui/client/vite.config.ts (1)
27-27: Updated comment path for.envlocation.
The comment now correctly points tocomponents/webui/server/.env, matching the refactored server layout.components/webui/client/public/settings.json (1)
2-3: Key order swap is non-functional.
Reordering"ClpStorageEngine"and"MongoDbSearchResultsMetadataCollectionName"has no impact on the JSON semantics or application behaviour.components/webui/client/package.json (2)
10-10: Simplified start script.
Removing the"antd"script streamlines the workflow. Confirm that all development and build tasks still succeed withnpm start.
2-2: Client package name updated.
Changing"name"to"webui-client"matches the merged WebUI structure. Verify no stale references to the old name remain:#!/bin/bash # Search for legacy client package name rg -l '"log-viewer-webui-client"'taskfiles/deps/main.yaml (1)
52-55: Confirm new variable.G_WEBUI_SRC_DIRexists in all include chains
OUTPUT_DIRnow relies on.G_WEBUI_SRC_DIR, but this variable is not declared in the localvars:block. Please verify it’s exported by one of the included taskfiles (utils.yaml,yscope-dev-utils/.../utils.yaml) or the global environment; otherwise this task will fail at runtime whenyscope-dev-utils:remote:download-and-extract-tartries to resolve the path.
If it’s missing, add it alongside the other directory vars.components/webui/client/src/main.tsx (1)
16-19: Simplified entry point looks correctRemoving the
VITE_USE_ANTD_APPbranch reduces complexity and the remaining JSX is straightforward. No concerns.components/package-template/src/etc/clp-config.yml (1)
61-65: Double-check downstream support forresult_metadata_collection_name
result_metadata_collection_nameis now the only tunable underwebui.
Ensure the server code that consumes this value (e.g., results-metadata DAO, validation layer) reads the key from the new location; otherwise custom configs will be silently ignored.components/webui/client/src/App.tsx (1)
1-6: RouterProvider import appears to target the wrong package
RouterProvideris exported byreact-router-dom; importing fromreact-routerwill break at runtime once the bundle is loaded.-import {RouterProvider} from "react-router"; +import {RouterProvider} from "react-router-dom";Please adjust before shipping.
⛔ 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-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.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.components/webui/package.json (1)
3-16:npm clean-installmay not be available on older npm versions
npm clean-installis an alias introduced in newer npm releases. If the CI or downstream users run an older npm (≤ 8.5), these scripts will fail. Consider switching to the widely-supportednpm ci.- "init": "npm clean-install && (cd client && npm clean-install) && (cd server && npm clean-install)", + "init": "npm ci && (cd client && npm ci) && (cd server && npm ci)",taskfile.yaml (5)
39-44: Good approach to separate checksum files.The separation of checksum files for different node_modules directories (client, log-viewer, package, server) provides better dependency tracking and allows for more granular rebuilds. This is a clean architectural improvement.
84-90: Comprehensive cleanup task.The
clean-webuitask properly removes all node_modules directories from the consolidated webui structure. This ensures a clean state for rebuilds.
157-158: Correct use of npm clean-install for production.Using
npm clean-installin the production package build ensures reproducible builds by installing from package-lock.json. The path correctly points to the consolidated webui server directory.
261-302: Well-structured webui build process.The consolidated webui build task properly handles all three components:
- Client build with Vite
- Server TypeScript compilation with package files
- Log-viewer build
The use of PATH override ensures consistent Node.js version usage across all build steps.
350-356: Excellent documentation of the node_modules structure.The comment clearly explains why there are four different node_modules directories, which helps future maintainers understand the architecture. This is a great practice.
Comment on lines
512
to
518
| def validate_webui_config(clp_config: CLPConfig, | ||
| client_settings_json_path: pathlib.Path, | ||
| server_settings_json_path: pathlib.Path): | ||
| for path in [client_settings_json_path, server_settings_json_path]: | ||
| if not path.exists(): | ||
| raise ValueError(f"{WEBUI_COMPONENT_NAME} {path} is not a valid path to settings.json") | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen validation & add minimal docstring
- The helper does not verify that the paths point to files named
settings.json; it only checks existence. - It silently accepts directories.
- A short docstring would suppress Pylint C0116.
def validate_webui_config(clp_config: CLPConfig,
client_settings_json_path: pathlib.Path,
server_settings_json_path: pathlib.Path):
- for path in [client_settings_json_path, server_settings_json_path]:
- if not path.exists():
- raise ValueError(f"{WEBUI_COMPONENT_NAME} {path} is not a valid path to settings.json")
+ """
+ Validate WebUI port and that both settings.json files exist and are regular files.
+ """
+ for path in (client_settings_json_path, server_settings_json_path):
+ if not path.is_file() or path.name != "settings.json":
+ raise ValueError(
+ f"{WEBUI_COMPONENT_NAME}: '{path}' must be an existing settings.json file"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def validate_webui_config(clp_config: CLPConfig, | |
| client_settings_json_path: pathlib.Path, | |
| server_settings_json_path: pathlib.Path): | |
| for path in [client_settings_json_path, server_settings_json_path]: | |
| if not path.exists(): | |
| raise ValueError(f"{WEBUI_COMPONENT_NAME} {path} is not a valid path to settings.json") | |
| def validate_webui_config(clp_config: CLPConfig, | |
| client_settings_json_path: pathlib.Path, | |
| server_settings_json_path: pathlib.Path): | |
| """ | |
| Validate WebUI port and that both settings.json files exist and are regular files. | |
| """ | |
| for path in (client_settings_json_path, server_settings_json_path): | |
| if not path.is_file() or path.name != "settings.json": | |
| raise ValueError( | |
| f"{WEBUI_COMPONENT_NAME}: '{path}' must be an existing settings.json file" | |
| ) |
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 512-512: Missing function or method docstring
(C0116)
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/general.py around lines 512 to
518, the validate_webui_config function only checks if the paths exist but does
not verify that they point to files named settings.json and allows directories.
Add a docstring briefly describing the function's purpose. Then, enhance the
validation by checking that each path is a file and that its name is exactly
settings.json, raising a ValueError if these conditions are not met.
Comment on lines
+60
to
63
| - "{{.G_WEBUI_SRC_DIR}}/**/*" | ||
| - exclude: "{{.G_WEBUI_SRC_DIR}}/**/dist/*" | ||
| - "{{.ROOT_DIR}}/taskfile.yaml" | ||
| - "{{.TASKFILE}}" |
Contributor
There was a problem hiding this comment.
G_WEBUI_SRC_DIR is referenced but no longer defined
The variable was deleted from the vars: block in this Taskfile yet is still used here. Task invocation will abort with “undefined variable” errors.
Update the variable reference or add a replacement definition (G_WEBUI_SRC_DIR: "{{.ROOT_DIR}}/components/webui").
🤖 Prompt for AI Agents
In taskfiles/lint.yaml around lines 60 to 63, the variable G_WEBUI_SRC_DIR is
used but no longer defined, causing undefined variable errors. To fix this,
either add a definition for G_WEBUI_SRC_DIR in the vars: block, for example
setting it to "{{.ROOT_DIR}}/components/webui", or update the references here to
use an existing defined variable that points to the correct directory.
| # Read, update, and write back client's and server's settings.json | ||
| client_settings_json_updates = { | ||
| "ClpStorageEngine": clp_config.package.storage_engine, | ||
| "MongoDbSearchResultsMetadataCollectionName": clp_config.webui.results_metadata_collection_name, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix line length violations.
Lines 869 and 885 exceed the 100-character limit. Consider breaking them into multiple lines for better readability.
- "MongoDbSearchResultsMetadataCollectionName": clp_config.webui.results_metadata_collection_name,
+ "MongoDbSearchResultsMetadataCollectionName": (
+ clp_config.webui.results_metadata_collection_name
+ ),- "MongoDbSearchResultsMetadataCollectionName": clp_config.webui.results_metadata_collection_name,
+ "MongoDbSearchResultsMetadataCollectionName": (
+ clp_config.webui.results_metadata_collection_name
+ ),Also applies to: 885-885
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 869-869: Line too long (104/100)
(C0301)
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/start_clp.py at lines
869 and 885, the lines exceed the 100-character limit. To fix this, break these
long lines into multiple shorter lines by splitting after commas or operators,
aligning the continued lines properly for readability and adhering to the line
length limit.
Comment on lines
+874
to
+875
| with open(client_settings_json_path, "w") as client_settings_json_file: | ||
| client_settings_json_file.write(json.dumps(client_settings_json)) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify encoding when opening files.
When opening files for reading or writing, it's best practice to explicitly specify the encoding to avoid potential issues with non-ASCII characters.
- with open(client_settings_json_path, "w") as client_settings_json_file:
- client_settings_json_file.write(json.dumps(client_settings_json))
+ with open(client_settings_json_path, "w", encoding="utf-8") as client_settings_json_file:
+ client_settings_json_file.write(json.dumps(client_settings_json))- with open(server_settings_json_path, "w") as settings_json_file:
- settings_json_file.write(json.dumps(server_settings_json))
+ with open(server_settings_json_path, "w", encoding="utf-8") as settings_json_file:
+ settings_json_file.write(json.dumps(server_settings_json))Also applies to: 916-917
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 874-874: Using open without explicitly specifying an encoding
(W1514)
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/start_clp.py at lines
874-875 and also at lines 916-917, the open() calls for writing files do not
specify the encoding, which can cause issues with non-ASCII characters. Fix this
by adding the encoding="utf-8" parameter to the open() function calls to
explicitly set the file encoding when opening files for writing.
|
|
||
|
|
||
| def start_log_viewer_webui( | ||
| def start_webui( |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a docstring to document the function.
The function is missing documentation. Consider adding a docstring that describes the purpose, parameters, and any exceptions that might be raised.
def start_webui(
instance_id: str,
clp_config: CLPConfig,
container_clp_config: CLPConfig,
mounts: CLPDockerMounts,
):
+ """
+ Starts the web UI component with both client and server configurations.
+
+ :param instance_id: Unique identifier for this CLP instance.
+ :param clp_config: Host CLP configuration.
+ :param container_clp_config: Container-specific CLP configuration.
+ :param mounts: Docker mount configurations.
+ :raises ValueError: If settings JSON files are invalid.
+ """
component_name = WEBUI_COMPONENT_NAME📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def start_webui( | |
| def start_webui( | |
| instance_id: str, | |
| clp_config: CLPConfig, | |
| container_clp_config: CLPConfig, | |
| mounts: CLPDockerMounts, | |
| ): | |
| """ | |
| Starts the web UI component with both client and server configurations. | |
| :param instance_id: Unique identifier for this CLP instance. | |
| :param clp_config: Host CLP configuration. | |
| :param container_clp_config: Container-specific CLP configuration. | |
| :param mounts: Docker mount configurations. | |
| :raises ValueError: If settings JSON files are invalid. | |
| """ | |
| component_name = WEBUI_COMPONENT_NAME |
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 842-842: Missing function or method docstring
(C0116)
[refactor] 842-842: Too many local variables (28/15)
(R0914)
[refactor] 842-842: Too many statements (55/50)
(R0915)
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/start_clp.py at line
842, the function start_webui lacks a docstring. Add a clear docstring
immediately below the function definition that explains the function's purpose,
describes its parameters, and notes any exceptions it may raise to improve code
readability and maintainability.
Comment on lines
+404
to
425
| - task: "clean-webui" | ||
| - "PATH='{{.G_NODEJS_22_BIN_DIR}}':$PATH npm run init" | ||
| - |- | ||
| cd yscope-log-viewer | ||
| PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm install | ||
| PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm clean-install | ||
| # These commands must be last | ||
| - task: "utils:checksum:compute" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.CLIENT_CHECKSUM_FILE}}" | ||
| CHECKSUM_FILE: "{{.G_WEBUI_CLIENT_NODE_MODULES_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.CLIENT_OUTPUT_DIR}}"] | ||
| - task: "utils:checksum:compute" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.LOG_VIEWER_CHECKSUM_FILE}}" | ||
| CHECKSUM_FILE: "{{.G_WEBUI_LOG_VIEWER_NODE_MODULES_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.LOG_VIEWER_OUTPUT_DIR}}"] | ||
| - task: "utils:checksum:compute" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.PACKAGE_CHECKSUM_FILE}}" | ||
| CHECKSUM_FILE: "{{.G_WEBUI_PACKAGE_NODE_MODULES_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.PACKAGE_OUTPUT_DIR}}"] | ||
| - task: "utils:checksum:compute" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.SERVER_CHECKSUM_FILE}}" | ||
| CHECKSUM_FILE: "{{.G_WEBUI_SERVER_NODE_MODULES_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.SERVER_OUTPUT_DIR}}"] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using parallel checksum computation.
The checksum computation commands are executed sequentially. Since they operate on independent directories, you could potentially speed up the build by computing them in parallel.
However, the current sequential approach is perfectly fine and may be easier to debug if issues arise.
🤖 Prompt for AI Agents
In taskfile.yaml around lines 404 to 425, the checksum computation tasks are run
sequentially, which can be slow. To improve build speed, refactor these tasks to
run in parallel since they operate on independent directories. You can achieve
this by defining a single task that triggers all checksum computations
concurrently or by using a parallel execution feature if supported by the task
runner. Ensure that the parallel execution does not affect dependencies or order
of other tasks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/src/dev-guide/components-log-viewer-webui.md(0 hunks)docs/src/dev-guide/components-webui.md(1 hunks)docs/src/dev-guide/index.md(0 hunks)
💤 Files with no reviewable changes (2)
- docs/src/dev-guide/index.md
- docs/src/dev-guide/components-log-viewer-webui.md
🧰 Additional context used
🪛 LanguageTool
docs/src/dev-guide/components-webui.md
[grammar] ~52-~52: The word ‘lint’ is a noun or an adjective. A verb or adverb is missing or misspelled here, or maybe a comma is missing.
Context: ...gs in .env. ## Linting You can lint this component either as part of the en...
(PRP_MD_NN)
🪛 markdownlint-cli2 (0.17.2)
docs/src/dev-guide/components-webui.md
27-27: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
47-47: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
66-66: Link and image reference definitions should be needed
Unused link or image reference definition: "eslint"
(MD053, link-image-reference-definitions)
70-70: Files should end with a single newline character
null
(MD047, single-trailing-newline)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (10)
docs/src/dev-guide/components-webui.md (10)
3-4: Approve updated component description.
The removal of Meteor.js and the clarification that the WebUI uses a React client and Fastify server (while still serving the log-viewer) is accurate and aligns with the new architecture.
8-8: Node.js requirement bump is correct.
Updating the requirement to Node.js v22+ matches the removal of Meteor.js and is consistent with the PR objectives.
11-11: Setup section header is clear.
The new “## Setup” heading correctly introduces the installation steps.
13-17: Step 1 instructions for log-viewer dependency remain valid.
Runningtask deps:log-viewerto fetch the log-viewer source is still required before installing dependencies.
30-30: Running section header is correct.
“## Running” appropriately introduces the development run steps.
32-37: Client development instructions are accurate.
The commands to start the React client undercomponents/webui/clientmatch the new layout.
39-44: Server development instructions are accurate.
The Fastify server startup undercomponents/webui/serveris correctly documented.
50-50: Linting section header is clear.
“## Linting” succinctly introduces the linting instructions.
54-58: Lint check commands are correct.
Thetask lint:check-jsinvocation is consistent with the updated task definitions.
60-64: Lint fix commands are correct.
Thetask lint:fix-jscommand properly covers the auto-fix step.
kirkrodrigues
requested changes
Jun 19, 2025
kirkrodrigues
left a comment
Member
There was a problem hiding this comment.
Some comments about the docs.
Comment on lines
+66
to
+70
| [eslint]: https://eslint.org/ | ||
| [nvm]: https://github.com/nvm-sh/nvm | ||
| [Fastify]: https://www.fastify.io/ | ||
| [log-viewer]: https://github.com/y-scope/yscope-log-viewer | ||
| [React]: https://reactjs.org/ No newline at end of file |
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/src/dev-guide/components-webui.md (1)
70-70: Ensure single trailing newline
Please confirm the file ends with exactly one newline to satisfy MD047.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/src/dev-guide/components-webui.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/dev-guide/components-webui.md
[grammar] ~52-~52: The word ‘lint’ is a noun or an adjective. A verb or adverb is missing or misspelled here, or maybe a comma is missing.
Context: ...gs in .env. ## Linting You can lint this component either as part of the en...
(PRP_MD_NN)
🪛 markdownlint-cli2 (0.17.2)
docs/src/dev-guide/components-webui.md
27-27: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
47-47: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
66-66: Link and image reference definitions should be needed
Unused link or image reference definition: "eslint"
(MD053, link-image-reference-definitions)
70-70: Files should end with a single newline character
null
(MD047, single-trailing-newline)
🔇 Additional comments (10)
docs/src/dev-guide/components-webui.md (10)
3-4: Component description aligns with new architecture
The intro correctly reflects the unified WebUI with a React client and Fastify server, replacing Meteor.
8-8: Node.js version requirement is clear
Specifying "Node.js v22 or higher" matches the updated tech stack.
11-11: The "## Setup" header is unchanged in meaning and appears correctly.
13-17: Step 1 instructions are accurate
The task command to fetch the log-viewer source is clear and uses the updatedtask deps:log-viewer.
19-25: Step 2 dependency install path is correct
The path has been updated fromlog-viewer-webuitocomponents/webuiand splits client/server installs.
34-37: Client run command is properly scoped
The development start instructions forcomponents/webui/clientare clear.
42-43: Server run command is properly scoped
The development start instructions forcomponents/webui/servermatch expectations.
57-58: Lint check command is correct
task lint:check-jsis the proper command for JS lint validation.
63-64: Lint fix command is correct
task lint:fix-jsproperly triggers auto-fixable lint corrections.
68-70: Link definitions are consistent with references
All three definitions (Fastify,log-viewer,React) are used in the text and valid.
| ## Linting | ||
|
|
||
| To integrate ESLint into IDEs like WebStorm and VSCode, follow these steps: | ||
| You can lint this component either as part of the entire project. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve grammar in linting description
Remove "either" to correct "You can lint this component as part of the entire project."
🧰 Tools
🪛 LanguageTool
[grammar] ~52-~52: The word ‘lint’ is a noun or an adjective. A verb or adverb is missing or misspelled here, or maybe a comma is missing.
Context: ...gs in .env. ## Linting You can lint this component either as part of the en...
(PRP_MD_NN)
🤖 Prompt for AI Agents
In docs/src/dev-guide/components-webui.md at line 52, the sentence "You can lint
this component either as part of the entire project." contains a grammatical
error due to the unnecessary use of "either." Remove the word "either" so the
sentence reads "You can lint this component as part of the entire project."
…view Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/index.html(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: junhaoliao
PR: y-scope/clp#937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.653Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The webui server logging can be configured via LOG_LEVEL and WEBUI_LOGS_DIR environment variables, with file logging enabled automatically in non-TTY environments when a logs directory is specified.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:35-41
Timestamp: 2024-11-19T19:52:43.429Z
Learning: For internal APIs in `components/log-viewer-webui/client/src/api/query.js`, runtime validation of parameters may not be necessary since the APIs are not exposed to end users, and JsDoc type annotations may be sufficient.
components/webui/client/index.html (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
Comment on lines
4
to
8
| <title>Log Viewer Web UI</title> | ||
| <meta charset="utf-8"/> | ||
| <link rel="icon" type="image/svg+xml" href="/favicon.svg" /> | ||
| <meta name="description" content="YScope Log Viewer Web UI"> | ||
| <meta name="description" content="YScope CLP"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"/> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Title remains outdated – update for consistency with the new branding
The <title> tag (line 4) still reads “Log Viewer Web UI” while the neighbouring description meta (line 7) now says “YScope CLP”. Keeping two different product names in the page header is confusing for users and hurts SEO. Please align the title with the new branding, e.g.:
- <title>Log Viewer Web UI</title>
+ <title>YScope CLP Web UI</title>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <title>Log Viewer Web UI</title> | |
| <meta charset="utf-8"/> | |
| <link rel="icon" type="image/svg+xml" href="/favicon.svg" /> | |
| <meta name="description" content="YScope Log Viewer Web UI"> | |
| <meta name="description" content="YScope CLP"> | |
| <meta name="viewport" content="width=device-width, initial-scale=1.0"/> | |
| <title>YScope CLP Web UI</title> | |
| <meta charset="utf-8"/> | |
| <link rel="icon" type="image/svg+xml" href="/favicon.svg" /> | |
| <meta name="description" content="YScope CLP"> | |
| <meta name="viewport" content="width=device-width, initial-scale=1.0"/> |
🤖 Prompt for AI Agents
In components/webui/client/index.html around lines 4 to 8, the <title> tag
currently reads "Log Viewer Web UI" which is inconsistent with the updated
branding "YScope CLP" used in the meta description. Update the <title> tag
content to "YScope CLP" to ensure consistent product naming and improve user
clarity and SEO.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
taskfile.yaml (3)
39-45: Checksum-file vars still repetitiveA prior review proposed grouping these four paths under a single map to remove duplication; that feedback hasn’t been applied.
84-90: Unconditionalrm -rfnegates node_modules caching
clean-webuistill wipes everynode_modulesregardless of checksum validity, forcing re-installs even when nothing changed—exactly the issue flagged earlier.
273-274: Node-modules exclude pattern not fully recursiveUsing
**/node_modules/*excludes contents but not the directory itself; the empty dir’s mtime triggers rebuilds. The earlier suggestion to use**/node_modules/**remains unaddressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfile.yaml(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: junhaoliao
PR: y-scope/clp#937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.653Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The webui server logging can be configured via LOG_LEVEL and WEBUI_LOGS_DIR environment variables, with file logging enabled automatically in non-TTY environments when a logs directory is specified.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:35-41
Timestamp: 2024-11-19T19:52:43.429Z
Learning: For internal APIs in `components/log-viewer-webui/client/src/api/query.js`, runtime validation of parameters may not be necessary since the APIs are not exposed to end users, and JsDoc type annotations may be sufficient.
taskfile.yaml (7)
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The webui server logging can be configured via LOG_LEVEL and WEBUI_LOGS_DIR environment variables, with file logging enabled automatically in non-TTY environments when a logs directory is specified.
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those in `components/core/tools/scripts/lib_install/`, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-19
Timestamp: 2025-05-26T18:39:51.727Z
Learning: In CMakeLists.txt files, anlowee prefers to explicitly list source files one by one rather than using file(GLOB ...) or file(GLOB_RECURSE ...) to automatically include files, maintaining consistency with other CMake files in the project.
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
Comment on lines
+388
to
+404
| CHECKSUM_FILE: "{{.G_WEBUI_CLIENT_NODE_MODULES_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.CLIENT_OUTPUT_DIR}}"] | ||
| EXCLUDE_PATTERNS: | ||
| - ".tmp" | ||
| - ".vite-temp" | ||
| - task: "utils:checksum:validate" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.SERVER_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.SERVER_OUTPUT_DIR}}"] | ||
| CHECKSUM_FILE: "{{.G_WEBUI_LOG_VIEWER_NODE_MODULES_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.LOG_VIEWER_OUTPUT_DIR}}"] | ||
| - task: "utils:checksum:validate" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.PACKAGE_CHECKSUM_FILE}}" | ||
| CHECKSUM_FILE: "{{.G_WEBUI_PACKAGE_NODE_MODULES_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.PACKAGE_OUTPUT_DIR}}"] | ||
| - task: "utils:checksum:validate" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.LOG_VIEWER_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.LOG_VIEWER_OUTPUT_DIR}}"] | ||
| CHECKSUM_FILE: "{{.G_WEBUI_SERVER_NODE_MODULES_CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: ["{{.SERVER_OUTPUT_DIR}}"] |
Contributor
There was a problem hiding this comment.
Inconsistent checksum validation allows cache thrash
Only the client directory ignores transient .tmp / .vite-temp files; the log-viewer, package, and server validations don’t, so ephemeral files break the cache.
@@
- task: "utils:checksum:validate"
vars:
CHECKSUM_FILE: "{{.G_WEBUI_LOG_VIEWER_NODE_MODULES_CHECKSUM_FILE}}"
INCLUDE_PATTERNS: ["{{.LOG_VIEWER_OUTPUT_DIR}}"]
+ EXCLUDE_PATTERNS:
+ - ".tmp"
+ - ".vite-temp"
@@
- task: "utils:checksum:validate"
vars:
CHECKSUM_FILE: "{{.G_WEBUI_PACKAGE_NODE_MODULES_CHECKSUM_FILE}}"
INCLUDE_PATTERNS: ["{{.PACKAGE_OUTPUT_DIR}}"]
+ EXCLUDE_PATTERNS:
+ - ".tmp"
+ - ".vite-temp"
@@
- task: "utils:checksum:validate"
vars:
CHECKSUM_FILE: "{{.G_WEBUI_SERVER_NODE_MODULES_CHECKSUM_FILE}}"
INCLUDE_PATTERNS: ["{{.SERVER_OUTPUT_DIR}}"]
+ EXCLUDE_PATTERNS:
+ - ".tmp"
+ - ".vite-temp"Ensures all directories share identical ignore rules and stabilises caching.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CHECKSUM_FILE: "{{.G_WEBUI_CLIENT_NODE_MODULES_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.CLIENT_OUTPUT_DIR}}"] | |
| EXCLUDE_PATTERNS: | |
| - ".tmp" | |
| - ".vite-temp" | |
| - task: "utils:checksum:validate" | |
| vars: | |
| CHECKSUM_FILE: "{{.SERVER_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.SERVER_OUTPUT_DIR}}"] | |
| CHECKSUM_FILE: "{{.G_WEBUI_LOG_VIEWER_NODE_MODULES_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.LOG_VIEWER_OUTPUT_DIR}}"] | |
| - task: "utils:checksum:validate" | |
| vars: | |
| CHECKSUM_FILE: "{{.PACKAGE_CHECKSUM_FILE}}" | |
| CHECKSUM_FILE: "{{.G_WEBUI_PACKAGE_NODE_MODULES_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.PACKAGE_OUTPUT_DIR}}"] | |
| - task: "utils:checksum:validate" | |
| vars: | |
| CHECKSUM_FILE: "{{.LOG_VIEWER_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.LOG_VIEWER_OUTPUT_DIR}}"] | |
| CHECKSUM_FILE: "{{.G_WEBUI_SERVER_NODE_MODULES_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.SERVER_OUTPUT_DIR}}"] | |
| - task: "utils:checksum:validate" | |
| vars: | |
| CHECKSUM_FILE: "{{.G_WEBUI_LOG_VIEWER_NODE_MODULES_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.LOG_VIEWER_OUTPUT_DIR}}"] | |
| EXCLUDE_PATTERNS: | |
| - ".tmp" | |
| - ".vite-temp" | |
| - task: "utils:checksum:validate" | |
| vars: | |
| CHECKSUM_FILE: "{{.G_WEBUI_PACKAGE_NODE_MODULES_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.PACKAGE_OUTPUT_DIR}}"] | |
| EXCLUDE_PATTERNS: | |
| - ".tmp" | |
| - ".vite-temp" | |
| - task: "utils:checksum:validate" | |
| vars: | |
| CHECKSUM_FILE: "{{.G_WEBUI_SERVER_NODE_MODULES_CHECKSUM_FILE}}" | |
| INCLUDE_PATTERNS: ["{{.SERVER_OUTPUT_DIR}}"] | |
| EXCLUDE_PATTERNS: | |
| - ".tmp" | |
| - ".vite-temp" |
🤖 Prompt for AI Agents
In taskfile.yaml around lines 388 to 404, the checksum validation for the
log-viewer, package, and server tasks lacks the EXCLUDE_PATTERNS that ignore
transient files like ".tmp" and ".vite-temp" present in the client task. To fix
this, add the same EXCLUDE_PATTERNS list with ".tmp" and ".vite-temp" to each of
these tasks to ensure consistent ignore rules and prevent cache thrashing caused
by ephemeral files.
kirkrodrigues
approved these changes
Jun 27, 2025
This was referenced Jun 29, 2025
Closed
quinntaylormitchell
pushed a commit
to quinntaylormitchell/clp
that referenced
this pull request
Jul 4, 2025
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
This was referenced Jul 8, 2025
Merged
3 tasks
3 tasks
3 tasks
junhaoliao
added a commit
to junhaoliao/clp
that referenced
this pull request
May 17, 2026
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
log-viewer-webuiaswebui.Checklist
breaking change.
(docs to be updated in a separate PR)
Validation performed
task.task lint:check-no-cppSummary by CodeRabbit