feat(webui): Add dataset and paths to compression job table. #1798
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/webui/server/src/routes/api/archive-metadata/index.ts (1)
26-32: Endpoint executes arbitrary SQL queries without authentication or parameterized queries.Line 28 executes user-provided
queryStringdirectly against the database without parameterization or input sanitization. No authentication middleware is applied to this route, making it publicly accessible.Critical issues:
- SQL injection vulnerability: Direct SQL execution allows arbitrary database commands (DROP, DELETE, UPDATE, etc.)
- Missing authentication: No checks verify user identity before accepting queries
- Missing authorization: No role or permission validation
Require authentication on this endpoint and implement parameterized queries or query builder patterns (similar to
compress-metadatawhich usesgetCompressionMetadataQuery()).components/webui/server/src/routes/api/compress/index.ts (1)
69-86: Add type guard before casting to CompressionJobFsInputConfig.Line 72 performs an unsafe type cast to
CompressionJobFsInputConfigwithout verifyingjobConfig.input.typeis actuallyCompressionJobInputType.FS. While currently safe (sinceDEFAULT_COMPRESSION_JOB_CONFIGalways sets type to FS), the TODO comment on line 71 indicates S3 support will be added. When that happens, this cast becomes unsafe and could cause runtime errors.🔎 Recommended fix with type guard
const jobConfig: ClpIoConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG); // eslint-disable-next-line no-warning-comments //TODO: Add support for S3 input -(jobConfig.input as CompressionJobFsInputConfig).paths_to_compress = paths.map( - (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path -); +if (CompressionJobInputType.FS === jobConfig.input.type) { + jobConfig.input.paths_to_compress = paths.map( + (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path + ); +}Note: This assumes TypeScript's discriminated union type narrowing works for
ClpIoConfig. If not, you may need an explicit type assertion after the guard.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
components/webui/client/src/api/compress-metadata/index.ts(1 hunks)components/webui/client/src/pages/IngestPage/Jobs/index.tsx(3 hunks)components/webui/client/src/pages/IngestPage/Jobs/sql.ts(0 hunks)components/webui/client/src/pages/IngestPage/Jobs/typings.tsx(3 hunks)components/webui/client/src/pages/IngestPage/Jobs/utils.ts(2 hunks)components/webui/client/src/pages/IngestPage/sqlConfig.ts(0 hunks)components/webui/common/src/schemas/compress-metadata.ts(1 hunks)components/webui/common/src/schemas/compression.ts(1 hunks)components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts(2 hunks)components/webui/server/src/plugins/app/CompressionJobDbManager/typings.ts(0 hunks)components/webui/server/src/routes/api/archive-metadata/index.ts(1 hunks)components/webui/server/src/routes/api/compress-metadata/index.ts(1 hunks)components/webui/server/src/routes/api/compress-metadata/sql.ts(1 hunks)components/webui/server/src/routes/api/compress-metadata/utils.ts(1 hunks)components/webui/server/src/routes/api/compress/index.ts(2 hunks)
💤 Files with no reviewable changes (3)
- components/webui/client/src/pages/IngestPage/sqlConfig.ts
- components/webui/client/src/pages/IngestPage/Jobs/sql.ts
- components/webui/server/src/plugins/app/CompressionJobDbManager/typings.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/plugins/app/CompressionJobDbManager/index.tscomponents/webui/client/src/api/compress-metadata/index.tscomponents/webui/server/src/routes/api/compress-metadata/sql.tscomponents/webui/client/src/pages/IngestPage/Jobs/typings.tsxcomponents/webui/server/src/routes/api/archive-metadata/index.tscomponents/webui/common/src/schemas/compress-metadata.tscomponents/webui/common/src/schemas/compression.tscomponents/webui/server/src/routes/api/compress-metadata/utils.tscomponents/webui/server/src/routes/api/compress/index.tscomponents/webui/client/src/pages/IngestPage/Jobs/index.tsxcomponents/webui/client/src/pages/IngestPage/Jobs/utils.tscomponents/webui/server/src/routes/api/compress-metadata/index.ts
🧠 Learnings (3)
📚 Learning: 2025-09-28T15:00:22.170Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 1340
File: components/job-orchestration/job_orchestration/executor/compress/compression_task.py:528-528
Timestamp: 2025-09-28T15:00:22.170Z
Learning: In components/job-orchestration/job_orchestration/executor/compress/compression_task.py, there is a suggestion to refactor from passing logger as a parameter through multiple functions to creating a ClpCompressor class that takes the logger as a class member, with current helper functions becoming private member functions.
Applied to files:
components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts
📚 Learning: 2024-11-21T15:51:33.203Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 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.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/sql.tscomponents/webui/client/src/pages/IngestPage/Jobs/typings.tsxcomponents/webui/server/src/routes/api/compress-metadata/utils.tscomponents/webui/server/src/routes/api/compress/index.tscomponents/webui/client/src/pages/IngestPage/Jobs/index.tsx
📚 Learning: 2025-09-15T22:20:40.750Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1125
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:267-291
Timestamp: 2025-09-15T22:20:40.750Z
Learning: For CLP compression jobs, the team has decided to fail the entire job immediately upon encountering any invalid input path, rather than continuing to process valid paths. This decision was made during PR #1125 development.
Applied to files:
components/webui/server/src/routes/api/compress/index.tscomponents/webui/client/src/pages/IngestPage/Jobs/utils.ts
🧬 Code graph analysis (5)
components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts (1)
components/webui/common/src/schemas/compression.ts (1)
ClpIoConfig(166-166)
components/webui/client/src/api/compress-metadata/index.ts (1)
components/webui/common/src/schemas/compress-metadata.ts (1)
CompressionMetadataDecoded(58-58)
components/webui/server/src/routes/api/compress/index.ts (3)
components/webui/common/src/schemas/compression.ts (3)
ClpIoConfig(166-166)CompressionJobInputType(159-159)CompressionJobFsInputConfig(169-169)components/webui/server/src/routes/api/compress/typings.ts (1)
CONTAINER_INPUT_LOGS_ROOT_DIR(7-7)components/webui/common/src/config.ts (1)
CLP_STORAGE_ENGINES(54-54)
components/webui/client/src/pages/IngestPage/Jobs/index.tsx (3)
components/webui/client/src/api/compress-metadata/index.ts (1)
fetchCompressionJobs(19-19)components/webui/client/src/pages/IngestPage/Jobs/typings.tsx (1)
JobData(148-148)components/webui/client/src/pages/IngestPage/Jobs/utils.ts (1)
mapCompressionJobResponseToTableData(142-142)
components/webui/client/src/pages/IngestPage/Jobs/utils.ts (5)
components/webui/common/src/schemas/compression.ts (2)
ClpIoConfig(166-166)CompressionJobInputType(159-159)components/webui/common/src/config.ts (1)
CLP_STORAGE_ENGINES(54-54)components/webui/client/src/config/index.ts (1)
SETTINGS_STORAGE_ENGINE(24-24)components/webui/common/src/schemas/compress-metadata.ts (1)
CompressionMetadataDecoded(58-58)components/webui/client/src/pages/IngestPage/Jobs/typings.tsx (2)
JobData(148-148)CompressionJobStatus(151-151)
🔇 Additional comments (20)
components/webui/server/src/routes/api/archive-metadata/index.ts (1)
28-28: Refactoring tofastify.mysql.queryis correct and idiomatic.The change from using a pool variable to calling
fastify.mysql.querydirectly is cleaner and the API is properly supported. This method exists as a utility on the fastify.mysql namespace and returns an array that can be destructured as shown, where the first element contains the query result set.components/webui/common/src/schemas/compression.ts (3)
75-81: LGTM! Enum matches Python InputType class.The enum definition is clear and aligns with the documented Python class structure.
101-110: Schema fields correctly match Python S3InputConfig definition.The nullable fields (
dataset,keys,timestamp_key) in the TypeScript schema match the Python class definition, where all three are defined as optional with| None. Theunstructuredfield is correctly non-nullable in both versions.
86-96: Verify nullable field requirements with Python FsInputConfig.The schema defines several fields as
Type.Union([Type.String(), Type.Null()]), making them nullable. Ensure this nullability matches the PythonFsInputConfigclass definition, particularly fordataset,path_prefix_to_remove, andtimestamp_key.#!/bin/bash # Description: Verify FsInputConfig field requirements in Python codebase # Search for FsInputConfig class definition rg -n -A 15 "class FsInputConfig" --type=pycomponents/webui/server/src/routes/api/compress-metadata/utils.ts (1)
42-72: LGTM! Mapping function is well-structured.The mapping logic correctly transforms database rows to the decoded metadata format. The destructuring approach is clean and maintainable.
components/webui/server/src/routes/api/compress-metadata/index.ts (1)
35-39: LGTM! Query and mapping logic is sound.The route correctly executes the query and maps the results using the utility function. Error handling will be managed by Fastify's default error handler.
components/webui/client/src/pages/IngestPage/Jobs/index.tsx (2)
20-26: LGTM! Data fetching migration is clean.The switch from SQL-based queries to the new
fetchCompressionJobsAPI withmapCompressionJobResponseToTableDatatransformation is well-structured. The query key remains stable for proper cache management.
38-40: Table configuration enhancements are appropriate.The addition of
rowKey, horizontal scrolling (x: true), and fixed layout are good improvements to accommodate the new dataset and paths columns mentioned in the PR objectives.components/webui/common/src/schemas/compress-metadata.ts (1)
40-47: Good design using partial schema for backward compatibility.Using
ClpIoPartialConfigSchemain the decoded schema properly supports the backward compatibility requirement mentioned in the PR objectives for v0.6.0 data.components/webui/server/src/routes/api/compress-metadata/sql.ts (1)
6-28: LGTM! Enum and interface definitions are well-structured.The enum provides type-safe column name references, and the interface correctly extends
RowDataPacketwith properly typed fields matching the database schema.components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts (1)
44-44: Parameter type change already verified in all call sites.The caller in
components/webui/server/src/routes/api/compress/index.tsat line 89 correctly passesjobConfigof typeClpIoConfig, which matches the updated method signature. The oldCompressionJobConfigtype is not found in the codebase, confirming the change has been fully propagated.components/webui/client/src/pages/IngestPage/Jobs/typings.tsx (3)
1-19: LGTM: Imports and setup are appropriate.The new imports support the conditional column visibility logic and the enhanced Paths column rendering with copyable text.
43-44: LGTM: JobData interface properly extended.The new fields align with the PR objectives and use appropriate nullable types for dataset.
50-130: LGTM: Conditional column logic is well-implemented.The conditional rendering of Dataset and Paths columns based on storage engine configuration is clean and the Paths column render function provides good UX with copyable text and tooltip ellipsis.
components/webui/server/src/routes/api/compress/index.ts (2)
4-9: LGTM: Import additions support the new CLP IO config types.
20-37: LGTM: DEFAULT_COMPRESSION_JOB_CONFIG properly updated to ClpIoConfig type.The new fields have sensible defaults and the object remains frozen to prevent mutations.
components/webui/client/src/pages/IngestPage/Jobs/utils.ts (4)
24-31: LGTM: stripPrefix follows coding guidelines.The implementation correctly uses
false === path.startsWith(prefix)rather than negation, adhering to the project's coding guidelines.
68-77: LGTM: extractDataFromIoConfig logic is correct.The conditional dataset extraction based on storage engine is appropriate, and the code follows coding guidelines.
114-128: LGTM: NaN checks follow coding guidelines.The code correctly uses
false === isNaN(...)pattern rather than!isNaN(...), adhering to the project's coding guidelines.
92-140: No changes required. The type castclpConfig as ClpIoConfigon line 101 is legitimate and intentional. Theclp_configfield is correctly defined asClpIoPartialConfigSchemainCompressionMetadataDecodedto support backward compatibility with older CLP releases, while the cast safely narrows the type to satisfy the function signature.Likely an incorrect or invalid review comment.
components/webui/server/src/routes/api/compress-metadata/index.ts
Outdated
Show resolved
Hide resolved
components/webui/server/src/routes/api/compress-metadata/utils.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Some comments.
One high level question: Why would decoding the ClpIoConfig in the client be too messy? We should be able to just call decodeJobConfig on the client side. This would avoid adding a new schema to common and a new route to the server. Does the Brotli library not work in the browser?
Validation: the dataset and path are displayed correctly for files compressed using compress.sh
components/webui/server/src/routes/api/compress-metadata/index.ts
Outdated
Show resolved
Hide resolved
components/webui/server/src/routes/api/compress-metadata/utils.ts
Outdated
Show resolved
Hide resolved
components/webui/server/src/routes/api/compress-metadata/utils.ts
Outdated
Show resolved
Hide resolved
|
for high level comment. I tried decoding on the client, but Brotli depends on Node zlib, so in the browser it requires JS/WASM libs — similar to how our client-side decompression is in the single-file archive code which looks a bit weird (xz.ts. Moving decompression to the server avoids that complexity, and gives us a typed Fastify response for the UI. |
kinda unrelated. and not to say we should do this today - interestingly, browser implementers have finally been considering adding more decompression formats to the https://developer.mozilla.org/en-US/docs/Web/API/DecompressionStream/DecompressionStream#brotli the diff: mdn/content#42312 the proposals make sense cuz modern browsers can already decode web page contents compressed in such modern compression formats - they just need to make such decoders available to the JS APIs. maybe in the coming year(s) we will not have to install external libraries to decompress data streams in formats the browsers can already decode. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/webui/client/src/pages/IngestPage/Jobs/utils.ts (1)
94-142: Remove redundant type assertion.Line 103 casts
clpIoConfigtoClpIoConfig, but the parameter destructuring at line 98 already infers this type fromCompressionMetadataDecoded. The cast is unnecessary and can be removed.♻️ Proposed fix
- const {dataset, paths} = extractDataFromIoConfig(clpIoConfig as ClpIoConfig); + const {dataset, paths} = extractDataFromIoConfig(clpIoConfig);
🤖 Fix all issues with AI agents
In @components/webui/common/src/schemas/compression.ts:
- Around line 86-110: The Type.Union declarations in ClpIoFsInputConfigSchema
and ClpIoS3InputConfigSchema are split across lines inconsistently; standardize
their formatting (e.g., keep each Type.Union([...]) on a single line) so all
union types (dataset, path_prefix_to_remove, timestamp_key, keys) follow the
same inline style for improved readability; update the Union declarations in
both ClpIoFsInputConfigSchema and ClpIoS3InputConfigSchema to match the chosen
consistent format referencing CompressionJobInputType literals unchanged.
In @components/webui/server/src/routes/api/compress-metadata/index.ts:
- Around line 33-39: The route handler that calls fastify.mysql.query(...) and
mapCompressionMetadataRows lacks explicit error handling; wrap the async handler
body in a try-catch, call getCompressionMetadataQuery() and await
fastify.mysql.query(...) inside the try, and if mapCompressionMetadataRows
throws catch the error, add a structured log entry via the Fastify logger
(including context like the query name/getCompressionMetadataQuery and any
identifiers), then rethrow or return a Fastify error (e.g., throw
fastify.httpErrors.internalServerError) so callers get a clear, observable
failure instead of relying solely on automatic promise rejection handling.
In @components/webui/server/src/routes/api/compress-metadata/utils.ts:
- Around line 64-74: The returned object uses longhand property assignments;
update it to use shorthand property syntax where possible by ensuring the local
variable names match the desired output keys (e.g., use _id, clp_config,
compressed_size, duration, start_time, status, status_msg, uncompressed_size,
update_time as local variables) — either rename the destructured variables
earlier (e.g., produce clp_config instead of decodedClpConfig, compressed_size
instead of compressedSize, start_time instead of startTime, etc.) or change the
returned keys to match existing variable names and then replace pairs like "_id:
_id" with just "_id" to eliminate redundancy.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/webui/client/src/pages/IngestPage/Jobs/utils.tscomponents/webui/common/src/schemas/compression.tscomponents/webui/server/src/routes/api/compress-metadata/index.tscomponents/webui/server/src/routes/api/compress-metadata/sql.tscomponents/webui/server/src/routes/api/compress-metadata/utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/routes/api/compress-metadata/utils.tscomponents/webui/server/src/routes/api/compress-metadata/index.tscomponents/webui/client/src/pages/IngestPage/Jobs/utils.tscomponents/webui/server/src/routes/api/compress-metadata/sql.tscomponents/webui/common/src/schemas/compression.ts
🧠 Learnings (14)
📚 Learning: 2024-11-21T15:51:33.203Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 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.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.tscomponents/webui/server/src/routes/api/compress-metadata/index.tscomponents/webui/server/src/routes/api/compress-metadata/sql.ts
📚 Learning: 2025-09-15T22:20:40.750Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1125
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:267-291
Timestamp: 2025-09-15T22:20:40.750Z
Learning: For CLP compression jobs, the team has decided to fail the entire job immediately upon encountering any invalid input path, rather than continuing to process valid paths. This decision was made during PR #1125 development.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.tscomponents/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2025-05-30T20:43:38.325Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 949
File: components/log-viewer-webui/client/src/components/QueryBox/index.tsx:15-15
Timestamp: 2025-05-30T20:43:38.325Z
Learning: In TypeScript/JavaScript files, exports should be placed at the end of the source file rather than inline with declarations. Prefer pattern like `interface SomeInterface {...}` followed by `export {SomeInterface};` at the end rather than `export interface SomeInterface {...}` at the declaration site.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-06-22T04:01:43.409Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 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.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/index.ts
📚 Learning: 2024-12-05T16:32:21.507Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 619
File: components/core/src/clp/clp/decompression.cpp:313-313
Timestamp: 2024-12-05T16:32:21.507Z
Learning: In the C++ `FileDecompressor` implementations at `components/core/src/clp/clp/FileDecompressor.cpp` and `components/core/src/glt/glt/FileDecompressor.cpp`, the `temp_output_path` variable and associated logic are used to handle multiple compressed files with the same name, and should be kept. This logic is separate from the temporary output directory code removed in PR #619 and is necessary for proper file handling.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2025-01-16T16:58:43.190Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.tscomponents/webui/common/src/schemas/compression.ts
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2025-09-28T15:00:22.170Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 1340
File: components/job-orchestration/job_orchestration/executor/compress/compression_task.py:528-528
Timestamp: 2025-09-28T15:00:22.170Z
Learning: In components/job-orchestration/job_orchestration/executor/compress/compression_task.py, there is a suggestion to refactor from passing logger as a parameter through multiple functions to creating a ClpCompressor class that takes the logger as a class member, with current helper functions becoming private member functions.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2024-12-16T21:49:06.483Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 634
File: components/job-orchestration/job_orchestration/executor/compress/fs_compression_task.py:0-0
Timestamp: 2024-12-16T21:49:06.483Z
Learning: In `fs_compression_task.py`, when refactoring the archive processing loop in Python, the `src_archive_file.unlink()` operation should remain outside of the `process_archive` function.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2025-08-08T06:59:42.436Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1152
File: components/clp-package-utils/clp_package_utils/scripts/start_clp.py:613-613
Timestamp: 2025-08-08T06:59:42.436Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/start_clp.py, generic_start_scheduler sets CLP_LOGGING_LEVEL using clp_config.query_scheduler.logging_level for both schedulers; compression scheduler should use its own logging level. Tracking via an issue created from PR #1152 discussion.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2025-11-17T22:58:50.056Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-11-17T22:58:50.056Z
Learning: In the y-scope/clp repository, when enabling new linting tools (ruff, mypy) on Python components, the team uses an incremental approach: first enable the tools with errors allowed (exit code 0), apply only safe auto-fixable fixes, then address remaining issues in follow-up PRs. During the initial enablement PR, reviews should focus on correctness of auto-fixes rather than flagging new code quality issues.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
components/webui/client/src/pages/IngestPage/Jobs/utils.ts
📚 Learning: 2025-08-04T18:38:33.130Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-08-04T18:38:33.130Z
Learning: User haiqi96 requested creating a GitHub issue to document a documentation discrepancy where Task version requirements in docs weren't updated after yscope-utils upgrade in PR #1158.
Applied to files:
components/webui/common/src/schemas/compression.ts
🧬 Code graph analysis (2)
components/webui/server/src/routes/api/compress-metadata/index.ts (3)
components/webui/common/src/schemas/compress-metadata.ts (1)
CompressionMetadataDecodedSchema(53-53)components/webui/server/src/routes/api/compress-metadata/sql.ts (2)
CompressionMetadataQueryRow(46-46)getCompressionMetadataQuery(44-44)components/webui/server/src/routes/api/compress-metadata/utils.ts (1)
mapCompressionMetadataRows(81-81)
components/webui/server/src/routes/api/compress-metadata/sql.ts (2)
components/webui/common/src/schemas/compress-metadata.ts (1)
CompressionMetadata(57-57)components/webui/client/src/settings.ts (1)
settings(38-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: package-image
- GitHub Check: build (macos-15)
- GitHub Check: check-generated
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (3)
components/webui/client/src/pages/IngestPage/Jobs/utils.ts (3)
25-32: LGTM!The
stripPrefixfunction correctly follows the coding guideline by usingfalse === path.startsWith(prefix)instead of!path.startsWith(prefix). The logic is clear and handles the prefix removal properly.
40-62: Good: Mutation concern resolved.The previous review flagged mutation of the input object. The current implementation (lines 43-46) correctly avoids mutation by checking the type condition and using a type assertion (line 47) instead. This keeps the function pure as the name
extractPathsFromInputimplies.
104-141: LGTM!The calculation logic correctly follows coding guidelines (using
false === isNaN(...)instead of!isNaN(...)at lines 116, 120, 124-125). The duration, size, and speed calculations handle null/zero cases appropriately.
| async () => { | ||
| const [rows] = await fastify.mysql.query<CompressionMetadataQueryRow[]>( | ||
| getCompressionMetadataQuery() | ||
| ); | ||
|
|
||
| return mapCompressionMetadataRows(rows); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider explicit error handling for better observability.
Fastify automatically catches promise rejections, but explicit try-catch with structured logging would provide better debugging context when mapCompressionMetadataRows throws during decompression/decoding failures.
♻️ Optional enhancement for error visibility
async () => {
+ try {
const [rows] = await fastify.mysql.query<CompressionMetadataQueryRow[]>(
getCompressionMetadataQuery()
);
return mapCompressionMetadataRows(rows);
+ } catch (err: unknown) {
+ fastify.log.error({err}, "Failed to fetch or decode compression metadata");
+ throw err;
+ }
}📝 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.
| async () => { | |
| const [rows] = await fastify.mysql.query<CompressionMetadataQueryRow[]>( | |
| getCompressionMetadataQuery() | |
| ); | |
| return mapCompressionMetadataRows(rows); | |
| } | |
| async () => { | |
| try { | |
| const [rows] = await fastify.mysql.query<CompressionMetadataQueryRow[]>( | |
| getCompressionMetadataQuery() | |
| ); | |
| return mapCompressionMetadataRows(rows); | |
| } catch (err: unknown) { | |
| fastify.log.error({err}, "Failed to fetch or decode compression metadata"); | |
| throw err; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @components/webui/server/src/routes/api/compress-metadata/index.ts around
lines 33 - 39, The route handler that calls fastify.mysql.query(...) and
mapCompressionMetadataRows lacks explicit error handling; wrap the async handler
body in a try-catch, call getCompressionMetadataQuery() and await
fastify.mysql.query(...) inside the try, and if mapCompressionMetadataRows
throws catch the error, add a structured log entry via the Fastify logger
(including context like the query name/getCompressionMetadataQuery and any
identifiers), then rethrow or return a Fastify error (e.g., throw
fastify.httpErrors.internalServerError) so callers get a clear, observable
failure instead of relying solely on automatic promise rejection handling.
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
The table isn't displayed properly after the latest change.
Possibly related logs:
{"level":30,"time":1767818553716,"pid":1,"hostname":"webui","reqId":"req-11","req":{"method":"GET","url":"/api/compress-metadata","host":"localhost:4000","remoteAddress":"192.168.16.1","remotePort":47762},"msg":"incoming request"}
AssertError: Expected union value
at AssertValue (file:///opt/clp/var/www/webui/node_modules/@sinclair/typebox/build/esm/value/assert/assert.mjs:44:11)
at Assert (file:///opt/clp/var/www/webui/node_modules/@sinclair/typebox/build/esm/value/assert/assert.mjs:48:32)
at file:///opt/clp/var/www/webui/node_modules/@sinclair/typebox/build/esm/value/parse/parse.mjs:25:51
at file:///opt/clp/var/www/webui/node_modules/@sinclair/typebox/build/esm/value/parse/parse.mjs:70:16
at Array.reduce (<anonymous>)
at ParseValue (file:///opt/clp/var/www/webui/node_modules/@sinclair/typebox/build/esm/value/parse/parse.mjs:66:23)
at Module.Parse (file:///opt/clp/var/www/webui/node_modules/@sinclair/typebox/build/esm/value/parse/parse.mjs:80:12)
at decodeJobConfig (file:///opt/clp/var/www/webui/server/dist/src/routes/api/compress-metadata/utils.js:10:40)
at file:///opt/clp/var/www/webui/server/dist/src/routes/api/compress-metadata/utils.js:20:50
at Array.map (<anonymous>) {
error: {
type: 62,
schema: { anyOf: [Array], [Symbol(TypeBox.Kind)]: 'Union' },
path: '/input',
value: {
dataset: 'postgres',
paths_to_compress: [Array],
path_prefix_to_remove: '/mnt/logs',
timestamp_key: 'timestamp',
unstructured: false
},
message: 'Expected union value',
errors: [ [ValueErrorIterator], [ValueErrorIterator] ]
}
}
{"level":50,"time":1767818553719,"pid":1,"hostname":"webui","err":{"type":"Error","message":"Failed to decode clp_config buffer","stack":"Error: Failed to decode clp_config buffer\n at decodeJobConfig (file:///opt/clp/var/www/webui/server/dist/src/routes/api/compress-metadata/utils.js:15:15)\n at file:///opt/clp/var/www/webui/server/dist/src/routes/api/compress-metadata/utils.js:20:50\n at Array.map (<anonymous>)\n at mapCompressionMetadataRows (file:///opt/clp/var/www/webui/server/dist/src/routes/api/compress-metadata/utils.js:19:17)\n at Object.<anonymous> (file:///opt/clp/var/www/webui/server/dist/src/routes/api/compress-metadata/index.js:16:16)\n at process.processTicksAndRejections (node:internal/process/task_queues:105:5)"},"request":{"method":"GET","url":"/api/compress-metadata","query":{},"params":{}},"msg":"Unhandled error occurred"}
|
@hoophalab the Value.Parse we added for the IO config was rejecting configs from earlier release. I am now using the partial type, then casting it. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @components/webui/server/src/routes/api/compress-metadata/utils.ts:
- Around line 35-37: The catch block catching err in the decode path currently
logs err to console but throws a new generic Error("Failed to decode clp_config
buffer"), losing the original context; update the catch in the function
containing that catch(err: unknown) to either re-throw the original err or
include its message/stack when creating the new Error (e.g., append String(err)
or use the Error cause option) so the thrown error preserves the original error
details instead of discarding them.
- Around line 64-74: The returned object uses redundant assignments; simplify by
using ES6 shorthand for properties whose key name matches the local variable
(e.g., replace `_id: _id`, `duration: duration`, `status: status` with `_id,
duration, status`) while keeping explicit mappings for properties whose keys
differ from variable names (e.g., `clp_config: decodedClpConfig`,
`compressed_size: compressedSize`, `start_time: startTime`, `status_msg:
statusMsg`, `uncompressed_size: uncompressedSize`, `update_time: updateTime`) in
the function that returns this object.
- Around line 29-32: decodedClpConfig is being parsed with
ClpIoPartialConfigSchema and then unsafely cast to ClpIoConfig, hiding missing
required fields; instead either parse with the full ClpIoConfig schema (e.g.,
use ClpIoConfigSchema with Value.Parse) or, if backward compatibility is needed,
validate the partial result at runtime: after
Value.Parse(ClpIoPartialConfigSchema, decode(brotliDecompressSync(jobConfig)))
check that decodedClpConfig.output contains compression_level,
target_archive_size, target_dictionaries_size, target_encoded_file_size,
target_segment_size and that decodedClpConfig.input.type/unstructured and
type-specific fields (paths_to_compress for FS, keys for S3) are present (or
provide sensible defaults) and throw a clear error if any required field is
missing; replace the unsafe cast to ClpIoConfig with these explicit checks or a
proper parse using ClpIoConfigSchema.
- Around line 24-26: Replace the negation pattern in the jobConfig type guard
with the project's preferred equality pattern: change the condition in the check
around jobConfig (currently written as if (!(jobConfig instanceof Buffer)) throw
...) to use the "false == (jobConfig instanceof Buffer)" style (i.e., if (false
== (jobConfig instanceof Buffer)) throw new Error("Missing clp_config buffer for
compression metadata")). Keep the same Error message and the same Buffer
instanceof check, only alter the boolean negation syntax.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2024-11-21T15:51:33.203Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 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.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-09-15T22:20:40.750Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1125
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:267-291
Timestamp: 2025-09-15T22:20:40.750Z
Learning: For CLP compression jobs, the team has decided to fail the entire job immediately upon encountering any invalid input path, rather than continuing to process valid paths. This decision was made during PR #1125 development.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-01-16T16:58:43.190Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-05-30T20:43:38.325Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 949
File: components/log-viewer-webui/client/src/components/QueryBox/index.tsx:15-15
Timestamp: 2025-05-30T20:43:38.325Z
Learning: In TypeScript/JavaScript files, exports should be placed at the end of the source file rather than inline with declarations. Prefer pattern like `interface SomeInterface {...}` followed by `export {SomeInterface};` at the end rather than `export interface SomeInterface {...}` at the declaration site.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧬 Code graph analysis (1)
components/webui/server/src/routes/api/compress-metadata/utils.ts (3)
components/webui/common/src/schemas/compression.ts (2)
ClpIoConfig(165-165)ClpIoPartialConfigSchema(156-156)components/webui/server/src/routes/api/compress-metadata/sql.ts (1)
CompressionMetadataQueryRow(46-46)components/webui/common/src/schemas/compress-metadata.ts (1)
CompressionMetadataDecoded(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: package-image
- GitHub Check: check-generated
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
components/webui/server/src/routes/api/compress-metadata/utils.ts (2)
1-11: LGTM: Imports are appropriate.All necessary imports for decompression (brotli), decoding (msgpack), validation (TypeBox), and type safety are present and properly structured.
79-82: LGTM: Exports follow established patterns.Exports are correctly placed at the end of the file, consistent with the project's coding conventions.
| const decodedClpConfig = Value.Parse( | ||
| ClpIoPartialConfigSchema, | ||
| decode(brotliDecompressSync(jobConfig)) | ||
| ) as ClpIoConfig; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find schema and type definitions for ClpIoPartialConfigSchema and ClpIoConfig
echo "=== ClpIoPartialConfigSchema definition ==="
ast-grep --pattern 'export const ClpIoPartialConfigSchema = $$$'
echo -e "\n=== ClpIoConfig type/interface definition ==="
ast-grep --pattern 'export type ClpIoConfig = $$$'
echo -e "\n=== ClpIoConfig interface definition ==="
ast-grep --pattern 'export interface ClpIoConfig {
$$$
}'
echo -e "\n=== Searching for schema files ==="
fd -e ts -e tsx | xargs grep -l "ClpIoPartialConfigSchema\|ClpIoConfig" | head -20Repository: y-scope/clp
Length of output: 586
🏁 Script executed:
#!/bin/bash
# Check the schema files directly
echo "=== Examining compress-metadata.ts ==="
wc -l components/webui/common/src/schemas/compress-metadata.ts
echo -e "\n=== Content of compress-metadata.ts ==="
cat -n components/webui/common/src/schemas/compress-metadata.ts
echo -e "\n=== Examining compression.ts ==="
wc -l components/webui/common/src/schemas/compression.ts
echo -e "\n=== Content of compression.ts ==="
cat -n components/webui/common/src/schemas/compression.tsRepository: y-scope/clp
Length of output: 7788
🏁 Script executed:
#!/bin/bash
# Examine the actual usage context in utils.ts
echo "=== Examining utils.ts around lines 29-32 ==="
wc -l components/webui/server/src/routes/api/compress-metadata/utils.ts
echo -e "\n=== Content of utils.ts ==="
cat -n components/webui/server/src/routes/api/compress-metadata/utils.tsRepository: y-scope/clp
Length of output: 3038
Address unsafe type casting from partial to full schema.
The code parses with ClpIoPartialConfigSchema (which uses Type.Partial() to make all fields optional) but casts to ClpIoConfig (the full schema expecting all required fields). This masks missing fields from older CLP releases. Required fields at risk include:
- In
output:compression_level,target_archive_size,target_dictionaries_size,target_encoded_file_size,target_segment_size - In
input:type,unstructured, and type-specific fields (paths_to_compressfor FS,keysfor S3)
The try-catch only captures parsing errors, not validation of required fields. Either guard against missing required fields at runtime, or make the full ClpIoConfig fields optional where backward compatibility requires it.
🤖 Prompt for AI Agents
In @components/webui/server/src/routes/api/compress-metadata/utils.ts around
lines 29 - 32, decodedClpConfig is being parsed with ClpIoPartialConfigSchema
and then unsafely cast to ClpIoConfig, hiding missing required fields; instead
either parse with the full ClpIoConfig schema (e.g., use ClpIoConfigSchema with
Value.Parse) or, if backward compatibility is needed, validate the partial
result at runtime: after Value.Parse(ClpIoPartialConfigSchema,
decode(brotliDecompressSync(jobConfig))) check that decodedClpConfig.output
contains compression_level, target_archive_size, target_dictionaries_size,
target_encoded_file_size, target_segment_size and that
decodedClpConfig.input.type/unstructured and type-specific fields
(paths_to_compress for FS, keys for S3) are present (or provide sensible
defaults) and throw a clear error if any required field is missing; replace the
unsafe cast to ClpIoConfig with these explicit checks or a proper parse using
ClpIoConfigSchema.
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
I remember I was using the latest version and compressed new logs
|
based on ur log. it looks like your missing the type field. Maybe this is an issue with the command line not adding the FS type? I did now change it to be less picky |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @components/webui/server/src/routes/api/compress-metadata/utils.ts:
- Around line 35-38: The catch block that currently logs err then throws a
generic Error("Failed to decode clp_config buffer") should preserve the original
error context: include the original error message/stack when throwing (e.g.,
build the message with String(err) or use (err instanceof Error ? err.stack :
String(err))) so the thrown Error contains both the descriptive text and the
original error details; update the catch in utils.ts (the catch (err: unknown)
block) to construct and throw a new Error that combines "Failed to decode
clp_config buffer" with the original error information.
- Around line 64-74: The returned object in the utils.ts return block is using
redundant key: value pairs for properties with identical names; simplify by
using ES6 shorthand for those keys (e.g., use _id, duration, status) while
keeping renamed properties that map to different variable names (keep
clp_config: decodedClpConfig, compressed_size: compressedSize, start_time:
startTime, status_msg: statusMsg, uncompressed_size: uncompressedSize,
update_time: updateTime) so edit the return statement accordingly in the
function that builds this metadata object.
- Around line 24-26: The boolean negation using "! (jobConfig instanceof
Buffer)" should follow the project's guideline; replace the expression with the
preferred pattern using "false == (jobConfig instanceof Buffer)" in the check
guarding the thrown Error (the jobConfig instanceof Buffer branch in utils.ts)
so the behavior remains identical but conforms to the coding standard; keep the
throw Error("Missing clp_config buffer for compression metadata") unchanged.
- Around line 29-34: The code is unsafely casting decodedClpConfig (produced via
Value.Parse with ClpIoPartialConfigSchema) to ClpIoConfig; either accept and
propagate a distinct partial type or enforce strict validation: update parsing
to use ClpIoConfig (strict schema) if all required fields must be present, or
change the function return type to a ClpIoPartialConfig and stop the cast;
alternatively add explicit runtime checks after Value.Parse to assert presence
and types of required fields (compression_level, target_archive_size,
paths_to_compress, type, unstructured) and throw a clear error before casting to
ClpIoConfig so decodedClpConfig is only cast when validation succeeds.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2024-11-21T15:51:33.203Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 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.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-09-15T22:20:40.750Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1125
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:267-291
Timestamp: 2025-09-15T22:20:40.750Z
Learning: For CLP compression jobs, the team has decided to fail the entire job immediately upon encountering any invalid input path, rather than continuing to process valid paths. This decision was made during PR #1125 development.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-01-16T16:58:43.190Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-05-30T20:43:38.325Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 949
File: components/log-viewer-webui/client/src/components/QueryBox/index.tsx:15-15
Timestamp: 2025-05-30T20:43:38.325Z
Learning: In TypeScript/JavaScript files, exports should be placed at the end of the source file rather than inline with declarations. Prefer pattern like `interface SomeInterface {...}` followed by `export {SomeInterface};` at the end rather than `export interface SomeInterface {...}` at the declaration site.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-06-09T17:48:56.024Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 988
File: components/log-viewer-webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsx:34-37
Timestamp: 2025-06-09T17:48:56.024Z
Learning: In the y-scope/clp project, prefer `false == <expression>` rather than `!<expression>` for boolean expressions in TypeScript/JavaScript files, as specified in the coding guidelines.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-08-25T00:45:05.464Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.464Z
Learning: Task v3.44.1 has a regression that breaks shell command processing, particularly rsync commands with brace expansion (e.g., `file.{d.ts,js,wasm}`). This causes CI failures in clp-ffi-js project (issue #110), so CLP should avoid v3.44.1 and use v3.44.0 instead, which fixes the dynamic variable bug without the shell processing regression.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2024-10-07T21:16:41.660Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:769-779
Timestamp: 2024-10-07T21:16:41.660Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, when handling errors in `parse_from_ir`, prefer to maintain the current mix of try-catch and if-statements because specific messages are returned back up in some cases.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-01-29T22:16:52.665Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.cpp:29-46
Timestamp: 2025-01-29T22:16:52.665Z
Learning: In C++ code, when throwing exceptions, prefer throwing the original return code (RC) from lower-level operations instead of a generic error code to preserve error context and aid in debugging.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧬 Code graph analysis (1)
components/webui/server/src/routes/api/compress-metadata/utils.ts (3)
components/webui/common/src/schemas/compression.ts (2)
ClpIoConfig(165-165)ClpIoPartialConfigSchema(156-156)components/webui/server/src/routes/api/compress-metadata/sql.ts (1)
CompressionMetadataQueryRow(46-46)components/webui/common/src/schemas/compress-metadata.ts (1)
CompressionMetadataDecoded(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: check-generated
🔇 Additional comments (3)
components/webui/server/src/routes/api/compress-metadata/utils.ts (3)
1-11: LGTM!Imports are well-organized and include all necessary dependencies for brotli decompression, msgpack decoding, TypeBox validation, and type definitions.
47-77: Consider error handling for individual row decoding failures.The function calls
decodeJobConfigfor each row (line 62), but provides no error handling. If one row'sclp_configbuffer is corrupted or invalid, the entire operation fails and no results are returned. Consider whether partial results should be returned or if individual failures should be logged and skipped.Verify the expected behavior when a single row fails to decode:
- Should the entire API request fail (current behavior)?
- Should corrupted rows be skipped with a warning?
- Are corrupted configs expected in production?
If partial results are acceptable, consider wrapping
decodeJobConfigin a try-catch within the map callback.
79-82: LGTM!Exports are correctly placed at the end of the file, following the project's coding guidelines.
As per coding guidelines, exports are properly positioned at the end of the source file.
components/webui/server/src/routes/api/compress-metadata/utils.ts
Outdated
Show resolved
Hide resolved
components/webui/server/src/routes/api/compress-metadata/utils.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @components/webui/server/src/routes/api/compress-metadata/utils.ts:
- Line 36: Replace the direct console.error(err) call in
components/webui/server/src/routes/api/compress-metadata/utils.ts with the
project's structured logger (e.g., logger.error or appLogger.error); import the
logger from the shared logging module used across the server, log a descriptive
message alongside the error object as structured metadata (not just
stringifying), and remove the console usage so errors are emitted via the
centralized logging framework.
- Around line 64-74: The returned object in the function in utils.ts uses
verbose property assignments (_id: _id, duration: duration, etc.); replace them
with ES6 property shorthand by returning { _id, clp_config: decodedClpConfig,
compressed_size: compressedSize, duration, start_time: startTime, status,
status_msg: statusMsg, uncompressed_size: uncompressedSize, update_time:
updateTime } — keep the keys that must differ from variable names (clp_config,
compressed_size, start_time, status_msg, uncompressed_size, update_time) mapped
to their variables, and use shorthand for matching names like _id and duration
to simplify the object literal.
- Around line 28-38: The code is parsing jobConfig with Value.Parse using
ClpIoPartialConfigSchema then unsafely casting to ClpIoConfig; replace the cast
with a proper validation step: parse/validate against the full ClpIoConfigSchema
(or run a subsequent validation using Value.Parse with ClpIoConfigSchema) after
decode(brotliDecompressSync(jobConfig)), or explicitly check that required
fields exist on the decodedClpConfig before returning; update the return to only
return when validation passes so callers receive a true ClpIoConfig rather than
an unchecked partial.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2024-11-21T15:51:33.203Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 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.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-09-15T22:20:40.750Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1125
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:267-291
Timestamp: 2025-09-15T22:20:40.750Z
Learning: For CLP compression jobs, the team has decided to fail the entire job immediately upon encountering any invalid input path, rather than continuing to process valid paths. This decision was made during PR #1125 development.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-01-16T16:58:43.190Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-05-30T20:43:38.325Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 949
File: components/log-viewer-webui/client/src/components/QueryBox/index.tsx:15-15
Timestamp: 2025-05-30T20:43:38.325Z
Learning: In TypeScript/JavaScript files, exports should be placed at the end of the source file rather than inline with declarations. Prefer pattern like `interface SomeInterface {...}` followed by `export {SomeInterface};` at the end rather than `export interface SomeInterface {...}` at the declaration site.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-08-25T00:45:05.464Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.464Z
Learning: Task v3.44.1 has a regression that breaks shell command processing, particularly rsync commands with brace expansion (e.g., `file.{d.ts,js,wasm}`). This causes CI failures in clp-ffi-js project (issue #110), so CLP should avoid v3.44.1 and use v3.44.0 instead, which fixes the dynamic variable bug without the shell processing regression.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2024-10-07T21:16:41.660Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:769-779
Timestamp: 2024-10-07T21:16:41.660Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, when handling errors in `parse_from_ir`, prefer to maintain the current mix of try-catch and if-statements because specific messages are returned back up in some cases.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-01-29T22:16:52.665Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.cpp:29-46
Timestamp: 2025-01-29T22:16:52.665Z
Learning: In C++ code, when throwing exceptions, prefer throwing the original return code (RC) from lower-level operations instead of a generic error code to preserve error context and aid in debugging.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
📚 Learning: 2025-06-09T17:48:56.024Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 988
File: components/log-viewer-webui/client/src/components/QueryBox/InputWithCaseSensitive/CaseSenstiveToggle.tsx:34-37
Timestamp: 2025-06-09T17:48:56.024Z
Learning: In the y-scope/clp project, prefer `false == <expression>` rather than `!<expression>` for boolean expressions in TypeScript/JavaScript files, as specified in the coding guidelines.
Applied to files:
components/webui/server/src/routes/api/compress-metadata/utils.ts
🧬 Code graph analysis (1)
components/webui/server/src/routes/api/compress-metadata/utils.ts (3)
components/webui/common/src/schemas/compression.ts (2)
ClpIoConfig(165-165)ClpIoPartialConfigSchema(156-156)components/webui/server/src/routes/api/compress-metadata/sql.ts (1)
CompressionMetadataQueryRow(46-46)components/webui/common/src/schemas/compress-metadata.ts (1)
CompressionMetadataDecoded(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: check-generated
- GitHub Check: build (macos-15)
🔇 Additional comments (2)
components/webui/server/src/routes/api/compress-metadata/utils.ts (2)
47-77: Verify error propagation behavior is intentional.If
decodeJobConfigthrows an error for any row (line 62), the entiremapCompressionMetadataRowsoperation fails. This means one corrupt database row will prevent all compression metadata from being displayed.Based on learnings, the team prefers failing entire operations on invalid input for compression jobs. Please confirm this is the desired behavior for the metadata display as well, or whether partial results with error indicators would be more appropriate for the UI.
79-82: LGTM!The exports are correctly placed at the end of the file, following the project's coding guidelines.
| try { | ||
| const decodedClpConfig = Value.Parse( | ||
| ClpIoPartialConfigSchema, | ||
| decode(brotliDecompressSync(jobConfig)) | ||
| ); | ||
|
|
||
| return {clp_config: decodedClpConfig as ClpIoConfig}; | ||
| } catch (err: unknown) { | ||
| console.error(err); | ||
| throw new Error("Failed to decode clp_config buffer"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find schema files and locate the schema definitions
fd -t f -e ts -e tsx "schema" --search-path . | head -20
# Search for ClpIoConfig and ClpIoPartialConfigSchema definitions
rg -n "export (const|type) Clp" --type ts -A 10 | head -100
# Also search more broadly for Type.Partial usage
rg -n "Type\.Partial|ClpIoPartialConfigSchema" --type ts -B 2 -A 5 | head -150Repository: y-scope/clp
Length of output: 4615
Remove unsafe type cast or validate that all required fields are present.
The code validates against ClpIoPartialConfigSchema but casts to ClpIoConfig on line 34. The partial schema uses Type.Partial() for its fields, making them optional, while ClpIoConfig expects complete fields. This cast is unsafe and may allow code to proceed with incomplete configuration objects at runtime. Either validate against the full ClpIoConfigSchema or ensure required fields are present before casting.
🤖 Prompt for AI Agents
In @components/webui/server/src/routes/api/compress-metadata/utils.ts around
lines 28 - 38, The code is parsing jobConfig with Value.Parse using
ClpIoPartialConfigSchema then unsafely casting to ClpIoConfig; replace the cast
with a proper validation step: parse/validate against the full ClpIoConfigSchema
(or run a subsequent validation using Value.Parse with ClpIoConfigSchema) after
decode(brotliDecompressSync(jobConfig)), or explicitly check that required
fields exist on the decodedClpConfig before returning; update the return to only
return when validation passes so callers receive a true ClpIoConfig rather than
an unchecked partial.
|
|
||
| return {clp_config: decodedClpConfig as ClpIoConfig}; | ||
| } catch (err: unknown) { | ||
| console.error(err); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace console.error with a proper logging framework.
Server code should use a structured logging framework rather than console.error for better observability in production environments.
🤖 Prompt for AI Agents
In @components/webui/server/src/routes/api/compress-metadata/utils.ts at line
36, Replace the direct console.error(err) call in
components/webui/server/src/routes/api/compress-metadata/utils.ts with the
project's structured logger (e.g., logger.error or appLogger.error); import the
logger from the shared logging module used across the server, log a descriptive
message alongside the error object as structured metadata (not just
stringifying), and remove the console usage so errors are emitted via the
centralized logging framework.
| return { | ||
| _id: _id, | ||
| clp_config: decodedClpConfig, | ||
| compressed_size: compressedSize, | ||
| duration: duration, | ||
| start_time: startTime, | ||
| status: status, | ||
| status_msg: statusMsg, | ||
| uncompressed_size: uncompressedSize, | ||
| update_time: updateTime, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using object property shorthand.
The return object uses verbose property assignment (e.g., _id: _id, duration: duration) when shorthand syntax is available.
♻️ Proposed refactor using shorthand
return {
- _id: _id,
+ _id,
clp_config: decodedClpConfig,
- compressed_size: compressedSize,
- duration: duration,
- start_time: startTime,
- status: status,
- status_msg: statusMsg,
- uncompressed_size: uncompressedSize,
- update_time: updateTime,
+ compressed_size: compressedSize,
+ duration,
+ start_time: startTime,
+ status,
+ status_msg: statusMsg,
+ uncompressed_size: uncompressedSize,
+ update_time: updateTime,
};📝 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 { | |
| _id: _id, | |
| clp_config: decodedClpConfig, | |
| compressed_size: compressedSize, | |
| duration: duration, | |
| start_time: startTime, | |
| status: status, | |
| status_msg: statusMsg, | |
| uncompressed_size: uncompressedSize, | |
| update_time: updateTime, | |
| }; | |
| return { | |
| _id, | |
| clp_config: decodedClpConfig, | |
| compressed_size: compressedSize, | |
| duration, | |
| start_time: startTime, | |
| status, | |
| status_msg: statusMsg, | |
| uncompressed_size: uncompressedSize, | |
| update_time: updateTime, | |
| }; |
🤖 Prompt for AI Agents
In @components/webui/server/src/routes/api/compress-metadata/utils.ts around
lines 64 - 74, The returned object in the function in utils.ts uses verbose
property assignments (_id: _id, duration: duration, etc.); replace them with ES6
property shorthand by returning { _id, clp_config: decodedClpConfig,
compressed_size: compressedSize, duration, start_time: startTime, status,
status_msg: statusMsg, uncompressed_size: uncompressedSize, update_time:
updateTime } — keep the keys that must differ from variable names (clp_config,
compressed_size, start_time, status_msg, uncompressed_size, update_time) mapped
to their variables, and use shorthand for matching names like _id and duration
to simplify the object literal.
3 participants
Description
PR adds support for showing dataset and paths in webui ingest table.
Unfortunately these fields cannot be queried since they are compressed blobs in the database.
At first i tried to decompress client side, but code was a mess. So i moved the entire query to the server, and decompressed on the server. I think longer term this is probably a better approach for all yscope (non-user) webui queries. Initially we thought it was better for user dashboard capability in the future to send queries from client, but for querying our own clp metadata server handling may be better. One advantage is fastify will type the response.
Since fastify is typing response. I cleaned up the compress types junhao added for his compress job form, since they are reused here. Unfortunately, some fields were missing, so i added them, and then allowed partial types so it wont give errors for anyone who compressed with v0.6.0 in UI.
Did not add support for s3 here. paths are jsut hidden. The PR is already large.
Checklist
breaking change.
Validation performed
Tested with clp-s
Testing with clp and S3 still underway. Even though there is no s3 support, the fastify validation may still cause errors so need to check with command line compression.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.