feat(webui): Add compression job submission for local files only. #1294
Conversation
WalkthroughAdds end-to-end compression-job support: client API and React form, server POST route and TypeBox schemas, a DB manager plugin that msgpack+Brotli-encodes job configs and inserts them into SQL, new server settings and archive output keys, and shared-config consolidation across client imports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as WebUI Compress Form
participant API as Client API
participant Server as Fastify /api/compress
participant DBM as CompressionJobDbManager
participant SQL as MySQL (compression_jobs)
User->>UI: Enter paths (and optional dataset/timestampKey if CLP‑S)
UI->>API: submitCompressionJob(payload)
API->>Server: POST /api/compress {paths, [dataset], [timestampKey]}
Server->>Server: structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG)
Server->>Server: prefix paths with /mnt/logs\nset dataset/timestampKey if ClpStorageEngine == "clp-s"
Server->>DBM: submitJob(jobConfig)
DBM->>DBM: msgpack encode -> Brotli compress
DBM->>SQL: INSERT job_config BLOB into configured table
SQL-->>DBM: insertId
DBM-->>Server: jobId
Server-->>API: 201 {jobId}
API-->>UI: jobId
UI-->>User: Show success with jobId
alt error
Server-->>API: 500 error
API-->>UI: Error
UI-->>User: Show error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📚 Learning: 2024-11-21T15:51:33.203ZApplied to files:
📚 Learning: 2024-11-19T19:52:43.429ZApplied to files:
📚 Learning: 2025-05-29T20:33:40.653ZApplied to files:
⏰ 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). (2)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
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/plugins/app/QueryJobDbManager/index.ts (1)
28-31: Validate table name to prevent accidental SQL injection and typosEven though the table name is from settings, add a defensive check to restrict it to safe identifiers.
- private constructor (sqlPool: MySQLPromisePool, tableName: string) { - this.#sqlPool = sqlPool; - this.#tableName = tableName; - } + private constructor (sqlPool: MySQLPromisePool, tableName: string) { + this.#sqlPool = sqlPool; + // Allow only [A-Za-z0-9_] + if (false === /^[A-Za-z0-9_]+$/.test(tableName)) { + throw new Error(`Invalid table name: ${tableName}`); + } + this.#tableName = tableName; + }components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
877-891: Also set the compression jobs table in server settings for consistencyServer settings currently omit
SqlDbCompressionJobsTableName. Set it alongside query jobs to avoid drift if the constant differs from the default.server_settings_json_updates = { "SqlDbHost": clp_config.database.host, "SqlDbPort": clp_config.database.port, "SqlDbName": clp_config.database.name, "SqlDbQueryJobsTableName": QUERY_JOBS_TABLE_NAME, + "SqlDbCompressionJobsTableName": COMPRESSION_JOBS_TABLE_NAME, "MongoDbHost": clp_config.results_cache.host,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/webui/client/src/api/compress/index.ts(1 hunks)components/webui/client/src/pages/IngestPage/Compress/index.tsx(1 hunks)components/webui/client/src/pages/IngestPage/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(1 hunks)components/webui/server/eslint.config.mjs(1 hunks)components/webui/server/settings.json(2 hunks)components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts(1 hunks)components/webui/server/src/plugins/app/CompressionJobDbManager/typings.ts(1 hunks)components/webui/server/src/plugins/app/QueryJobDbManager/index.ts(1 hunks)components/webui/server/src/routes/api/compress/index.ts(1 hunks)components/webui/server/src/schemas/compression.ts(1 hunks)components/webui/server/src/typings/compression.ts(1 hunks)components/webui/server/src/typings/query.ts(2 hunks)
🧰 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/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/server/src/plugins/app/CompressionJobDbManager/typings.tscomponents/webui/client/src/pages/IngestPage/index.tsxcomponents/webui/server/src/schemas/compression.tscomponents/webui/server/src/typings/compression.tscomponents/webui/server/src/plugins/app/QueryJobDbManager/index.tscomponents/webui/client/src/api/compress/index.tscomponents/webui/server/src/routes/api/compress/index.tscomponents/webui/server/src/typings/query.tscomponents/webui/client/src/pages/IngestPage/Compress/index.tsxcomponents/webui/server/src/plugins/app/CompressionJobDbManager/index.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#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.
📚 Learning: 2025-08-25T16:27:50.549Z
Learnt from: davemarco
PR: y-scope/clp#1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.
Applied to files:
components/webui/server/settings.json
📚 Learning: 2024-11-21T15:51:33.203Z
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.
Applied to files:
components/webui/server/src/typings/query.ts
🧬 Code graph analysis (4)
components/webui/server/src/schemas/compression.ts (1)
components/webui/client/src/api/compress/index.ts (2)
CompressionJobSchema(29-29)CompressionJobCreationSchema(28-28)
components/webui/client/src/api/compress/index.ts (1)
components/webui/server/src/schemas/compression.ts (2)
CompressionJobSchema(22-22)CompressionJobCreationSchema(21-21)
components/webui/server/src/routes/api/compress/index.ts (1)
components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts (1)
CompressionJobConfig(13-27)
components/webui/client/src/pages/IngestPage/Compress/index.tsx (4)
components/webui/client/src/api/compress/index.ts (2)
CompressionJobSchema(29-29)submitCompressionJob(31-31)components/webui/server/src/schemas/compression.ts (1)
CompressionJobSchema(22-22)components/webui/client/src/settings.ts (1)
settings(36-36)components/webui/client/src/components/DashboardCard/index.tsx (1)
DashboardCard(57-57)
🪛 GitHub Actions: clp-lint
components/clp-package-utils/clp_package_utils/scripts/start_clp.py
[error] 938-948: lint:check-no-cpp failed: 1 file would be reformatted by clang-format: /home/runner/work/clp/clp/components/clp-package-utils/clp_package_utils/scripts/start_clp.py
⏰ 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). (1)
- GitHub Check: package-image
🔇 Additional comments (14)
components/webui/server/src/plugins/app/QueryJobDbManager/index.ts (1)
54-54: Good: table name now instance-scopedUsing
this.#tableNamedecouples from the global settings constant and matches the instance’s config.components/webui/server/settings.json (2)
6-6: LGTM: adds compression jobs table nameThe default
"compression_jobs"aligns with the new manager/route.
23-23: LGTM: exposes storage engine to serverMatches client/server payload wiring in start_clp.py.
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (4)
861-870: Good: client settings include storage engine and compression tableThis unblocks UI gating and API payloads.
1-1: Fix formatting to unblock CIGA “clp-lint” reports clang-format changes pending for this file. Please run the project’s formatter task (e.g.,
clang-format -i components/clp-package-utils/clp_package_utils/scripts/start_clp.py) or the repo’s lint-fix script.
944-946: Restrict input_logs_dir mount to FS type and enforce read-only
In start_clp.py (lines 944–946), only appendmounts.input_logs_dirwhenclp_config.logs_input.type == StorageType.FS, and configure that mount as read-only inCLPDockerMounts. Confirm whether the webui server actually requires direct filesystem access to logs; if not, remove this mount entirely to reduce attack surface.
940-961: No action needed; input_logs_dir is already read-only
The call on general.py (around line 248) passesTrueas the fourth argument toDockerMount, which enables read-only mode formounts.input_logs_dir.components/webui/server/src/typings/query.ts (2)
5-6: LGTM: doc reference fixedComments now point to the correct
job_orchestration.scheduler.constants.
22-24: LGTM: doc reference fixedConsistent with the above.
components/webui/server/src/routes/api/compress/index.ts (2)
75-84: Handle emptytimestampKeyconsistently.Avoid storing an empty string; only set when non-empty string provided.
- if ("clp-s" === settings.ClpStorageEngine) { + if ("clp-s" === settings.ClpStorageEngine) { if ("string" !== typeof dataset || 0 === dataset.length) { jobConfig.input.dataset = CLP_DEFAULT_DATASET_NAME; } else { jobConfig.input.dataset = dataset; } - if ("undefined" !== typeof timestampKey) { - jobConfig.input.timestamp_key = timestampKey; - } + if ("string" === typeof timestampKey && 0 < timestampKey.length) { + jobConfig.input.timestamp_key = timestampKey; + } }⛔ Skipped due to learnings
Learnt from: quinntaylormitchell PR: y-scope/clp#1283 File: components/clp-package-utils/clp_package_utils/scripts/compress.py:206-210 Timestamp: 2025-09-04T13:51:02.311Z Learning: In components/clp-package-utils/clp_package_utils/scripts/compress.py, an empty string ("") is a valid value for the --timestamp-key parameter, so validation should not treat empty strings as missing values.
86-96: RemoveErrorSchemavalidation suggestion
NoErrorSchemaexport exists in this codebase; that check is not applicable.Likely an incorrect or invalid review comment.
components/webui/server/src/typings/compression.ts (2)
9-15: No action needed: COMPRESSION_JOB_STATUS values (PENDING = 0, RUNNING = 1, SUCCEEDED = 2, FAILED = 3, KILLED = 4) match CompressionJobStatus in scheduler.constants.
30-34: Verifyclp_configcolumn exists in thecompression_jobstable
The repo contains no migration or schema files forcompression_jobs; please manually confirm your DB DDL defines aclp_configcolumn to avoid runtime failures.components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts (1)
48-50: LGTM: factory + DI via Fastify instance is clean.
| type CompressionJobSchema = { | ||
| paths: string[]; | ||
| dataset?: string; | ||
| timestampKey?: string; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Type nit: use readonly for input paths
Signals immutability to callers and prevents accidental mutation.
-type CompressionJobSchema = {
- paths: string[];
+type CompressionJobSchema = {
+ paths: readonly string[];📝 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.
| type CompressionJobSchema = { | |
| paths: string[]; | |
| dataset?: string; | |
| timestampKey?: string; | |
| }; | |
| type CompressionJobSchema = { | |
| paths: readonly string[]; | |
| dataset?: string; | |
| timestampKey?: string; | |
| }; |
🤖 Prompt for AI Agents
In components/webui/client/src/api/compress/index.ts around lines 4 to 8, the
CompressionJobSchema type declares paths: string[] which allows mutation; change
it to an immutable type (either readonly string[] or ReadonlyArray<string>) to
signal immutability to callers and prevent accidental mutation, and update any
places that push/pop or assign to paths to instead create new arrays (e.g.,
using spread) if mutation-like behavior is needed.
| const submitCompressionJob = async (payload: CompressionJobSchema): Promise<number> => { | ||
| console.log("Submitting compression job:", JSON.stringify(payload)); | ||
|
|
||
| const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload); | ||
| return response.data.jobId; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid logging sensitive payloads; add basic error handling
console.log can leak local paths. Wrap the call with try/catch and remove the log.
-const submitCompressionJob = async (payload: CompressionJobSchema): Promise<number> => {
- console.log("Submitting compression job:", JSON.stringify(payload));
-
- const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload);
- return response.data.jobId;
-};
+const submitCompressionJob = async (payload: CompressionJobSchema): Promise<number> => {
+ try {
+ const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload);
+ return response.data.jobId;
+ } catch (e) {
+ // Re-throw a concise error; upstream UI can surface a friendly message.
+ throw new Error("Failed to submit compression job.");
+ }
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const submitCompressionJob = async (payload: CompressionJobSchema): Promise<number> => { | |
| console.log("Submitting compression job:", JSON.stringify(payload)); | |
| const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload); | |
| return response.data.jobId; | |
| }; | |
| const submitCompressionJob = async (payload: CompressionJobSchema): Promise<number> => { | |
| try { | |
| const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload); | |
| return response.data.jobId; | |
| } catch (e) { | |
| // Re-throw a concise error; upstream UI can surface a friendly message. | |
| throw new Error("Failed to submit compression job."); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In components/webui/client/src/api/compress/index.ts around lines 20 to 25,
remove the console.log that prints the entire payload (it may leak sensitive
local paths) and wrap the axios.post call in a try/catch; on success return
response.data.jobId as before, and on error log a minimal, non-sensitive message
(or nothing) and rethrow the error (or return a rejected promise) so callers can
handle it—do not print the payload in the catch.
| // Convert multiline input to array of paths | ||
| const paths = values.paths | ||
| .split("\n") | ||
| .map((path) => path.trim()) | ||
| .filter((path) => 0 < path.length); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate that at least one non-empty path is provided (after trimming).
Whitespace-only input passes the required rule and yields an empty paths array. Add a custom validator that enforces at least one trimmed, non-empty line before submit.
- <Form.Item
+ <Form.Item
label={"Paths"}
name={"paths"}
- rules={[{required: true, message: "Please enter at least one path"}]}
+ rules={[
+ {required: true, message: "Please enter at least one path"},
+ {
+ validator: async (_, value?: string) => {
+ const count = (value ?? "")
+ .split("\n")
+ .map((p) => p.trim())
+ .filter((p) => 0 < p.length).length;
+ if (0 < count) { return Promise.resolve(); }
+ return Promise.reject(new Error("Please enter at least one non-empty path"));
+ },
+ },
+ ]}
>
<Input.TextArea
autoSize={{minRows: 4, maxRows: 10}}
- placeholder={"Enter paths to compress, one per line"}/>
+ placeholder={"Enter container-relative paths (e.g., /logs/app.log), one per line"}/>
</Form.Item>Also applies to: 94-102
🤖 Prompt for AI Agents
In components/webui/client/src/pages/IngestPage/Compress/index.tsx around lines
41-46 (and similarly at 94-102), the current splitting logic allows
whitespace-only input to pass validation because trimming happens after required
check; update the form validators to include a custom synchronous validator that
trims values.paths, splits on newlines, filters out empty strings and rejects
the field (returning a validation error) if the resulting paths array has length
0; ensure the same validator logic is applied to both places so submission is
blocked until at least one non-empty trimmed path is provided.
| if ("undefined" !== typeof values.dataset) { | ||
| payload.dataset = values.dataset; | ||
| } | ||
|
|
||
| if ("undefined" !== typeof values.timestampKey) { | ||
| payload.timestampKey = values.timestampKey; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Send optional fields only when non-empty and trim them.
Avoid sending "" for dataset/timestampKey; let the server default logic apply. Also trim whitespace.
- if ("undefined" !== typeof values.dataset) {
- payload.dataset = values.dataset;
- }
+ if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) {
+ payload.dataset = values.dataset.trim();
+ }
- if ("undefined" !== typeof values.timestampKey) {
- payload.timestampKey = values.timestampKey;
- }
+ if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) {
+ payload.timestampKey = values.timestampKey.trim();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ("undefined" !== typeof values.dataset) { | |
| payload.dataset = values.dataset; | |
| } | |
| if ("undefined" !== typeof values.timestampKey) { | |
| payload.timestampKey = values.timestampKey; | |
| } | |
| if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) { | |
| payload.dataset = values.dataset.trim(); | |
| } | |
| if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) { | |
| payload.timestampKey = values.timestampKey.trim(); | |
| } |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/IngestPage/Compress/index.tsx around lines
52 to 58, the payload currently sends dataset and timestampKey when they are
defined even if they are empty or whitespace; update the checks to trim the
values and only assign payload.dataset/payload.timestampKey when typeof value
!== "undefined" and the trimmed string length > 0, assigning the trimmed string
so the server sees no empty strings and can apply its defaults.
| <DashboardCard title={"Start Ingestion"}> | ||
| <Form |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Rename card title to match the feature (“Compression”).
“Start Ingestion” is misleading here.
- <DashboardCard title={"Start Ingestion"}>
+ <DashboardCard title={"Submit Compression Job"}>📝 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.
| <DashboardCard title={"Start Ingestion"}> | |
| <Form | |
| <DashboardCard title={"Submit Compression Job"}> | |
| <Form |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/IngestPage/Compress/index.tsx around lines
84-85, the DashboardCard title is "Start Ingestion" which is misleading for the
Compression feature; rename the title to "Compression" (or "Start Compression"
if you prefer consistency) by updating the DashboardCard title prop accordingly
so the UI accurately reflects the feature.
| jobConfig.input.paths_to_compress = paths.map( | ||
| (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path | ||
| ); |
There was a problem hiding this comment.
Fix path concatenation: use POSIX join and normalise leading slashes.
CONTAINER_INPUT_LOGS_ROOT_DIR + path breaks when path lacks a leading slash (e.g., "logs/app.log" → "/mnt/logslogs/app.log"). Use path.posix.join after removing leading slashes to be robust across inputs.
+import path from "node:path";
@@
- jobConfig.input.paths_to_compress = paths.map(
- (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path
- );
+ jobConfig.input.paths_to_compress = paths.map((p) =>
+ path.posix.join(
+ CONTAINER_INPUT_LOGS_ROOT_DIR,
+ // ensure we don't drop the root when `p` starts with "/"
+ (p ?? "").trim().replace(/^\/+/, "")
+ )
+ );Also applies to: 4-4
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/compress/index.ts around lines 71-73
(and also at lines 4-4), the code concatenates paths with
CONTAINER_INPUT_LOGS_ROOT_DIR using string + which breaks when the incoming path
lacks a leading slash; change it to use
path.posix.join(CONTAINER_INPUT_LOGS_ROOT_DIR, normalizedPath) where
normalizedPath strips any leading slashes (e.g., path.replace(/^\/+/, '') or use
path.posix.normalize on the relative path) so joins are POSIX-safe and produce
correct results regardless of leading slashes.
| const CompressionJobSchema = Type.Object({ | ||
| paths: Type.Array(Type.String()), | ||
| dataset: Type.Optional(Type.String()), | ||
| timestampKey: Type.Optional(Type.String()), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten request schema: require ≥1 non-whitespace path and forbid extras.
Prevents empty submissions and stray fields. Keep empty dataset permitted if desired at the route, but ensure each path has at least one non-whitespace character.
-const CompressionJobSchema = Type.Object({
- paths: Type.Array(Type.String()),
- dataset: Type.Optional(Type.String()),
- timestampKey: Type.Optional(Type.String()),
-});
+const CompressionJobSchema = Type.Object(
+ {
+ paths: Type.Array(
+ // Require at least one non-whitespace character
+ Type.String({minLength: 1, pattern: "\\S"}),
+ {minItems: 1}
+ ),
+ dataset: Type.Optional(Type.String()),
+ timestampKey: Type.Optional(Type.String()),
+ },
+ {additionalProperties: false}
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const CompressionJobSchema = Type.Object({ | |
| paths: Type.Array(Type.String()), | |
| dataset: Type.Optional(Type.String()), | |
| timestampKey: Type.Optional(Type.String()), | |
| }); | |
| const CompressionJobSchema = Type.Object( | |
| { | |
| paths: Type.Array( | |
| // Require at least one non-whitespace character | |
| Type.String({ minLength: 1, pattern: "\\S" }), | |
| { minItems: 1 } | |
| ), | |
| dataset: Type.Optional(Type.String()), | |
| timestampKey: Type.Optional(Type.String()), | |
| }, | |
| { additionalProperties: false } | |
| ); |
🤖 Prompt for AI Agents
In components/webui/server/src/schemas/compression.ts around lines 7–11, tighten
the schema so that "paths" is a non-empty array (minItems:1) of strings that
contain at least one non-whitespace character (use a pattern like \S and/or
minLength:1), keep "dataset" and "timestampKey" as optional string fields, and
forbid extra properties on the object (additionalProperties: false). Update the
Type.Object definition accordingly so requests with an empty paths array,
whitespace-only paths, or unexpected fields are rejected.
| const CompressionJobCreationSchema = Type.Object({ | ||
| jobId: Type.Number(), | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Harden response schema.
Forbid unexpected properties in the response.
-const CompressionJobCreationSchema = Type.Object({
- jobId: Type.Number(),
-});
+const CompressionJobCreationSchema = Type.Object(
+ {jobId: Type.Number()},
+ {additionalProperties: false}
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const CompressionJobCreationSchema = Type.Object({ | |
| jobId: Type.Number(), | |
| }); | |
| const CompressionJobCreationSchema = Type.Object( | |
| { jobId: Type.Number() }, | |
| { additionalProperties: false } | |
| ); |
🤖 Prompt for AI Agents
In components/webui/server/src/schemas/compression.ts around lines 16 to 18, the
CompressionJobCreationSchema currently allows unexpected properties; update the
schema to forbid additional properties by setting the object options to disable
additionalProperties (or use Type.Strict) so responses with extra fields are
rejected by the validator.
| const COMPRESSION_JOB_STATUS_WAITING_STATES = new Set([ | ||
| COMPRESSION_JOB_STATUS.PENDING, | ||
| COMPRESSION_JOB_STATUS.RUNNING, | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Export an immutable view (or a predicate) to avoid accidental mutation.
A Set remains mutable even when the binding is const.
Apply either:
-const COMPRESSION_JOB_STATUS_WAITING_STATES = new Set([
+const COMPRESSION_JOB_STATUS_WAITING_STATES: ReadonlySet<COMPRESSION_JOB_STATUS> = new Set([
COMPRESSION_JOB_STATUS.PENDING,
COMPRESSION_JOB_STATUS.RUNNING,
]);Or replace with a predicate:
-const COMPRESSION_JOB_STATUS_WAITING_STATES = new Set([...]);
+const isWaitingStatus = (s: COMPRESSION_JOB_STATUS): boolean =>
+ (COMPRESSION_JOB_STATUS.PENDING === s) || (COMPRESSION_JOB_STATUS.RUNNING === s);
+
export {
- COMPRESSION_JOB_STATUS_WAITING_STATES,
+ isWaitingStatus,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const COMPRESSION_JOB_STATUS_WAITING_STATES = new Set([ | |
| COMPRESSION_JOB_STATUS.PENDING, | |
| COMPRESSION_JOB_STATUS.RUNNING, | |
| ]); | |
| const COMPRESSION_JOB_STATUS_WAITING_STATES: ReadonlySet<COMPRESSION_JOB_STATUS> = new Set([ | |
| COMPRESSION_JOB_STATUS.PENDING, | |
| COMPRESSION_JOB_STATUS.RUNNING, | |
| ]); |
🤖 Prompt for AI Agents
In components/webui/server/src/typings/compression.ts around lines 20–23, the
current const binding creates a mutable Set which can be accidentally mutated;
replace it with an immutable exported predicate function (e.g. export function
isCompressionJobWaitingStatus(status: COMPRESSION_JOB_STATUS): boolean { return
status === COMPRESSION_JOB_STATUS.PENDING || status ===
COMPRESSION_JOB_STATUS.RUNNING; }) so callers can test membership without
exposing mutable state — alternatively, if you must export a collection, export
a frozen array (Object.freeze([...])) or a readonly tuple to provide an
immutable view.
| interface CompressionJob extends RowDataPacket { | ||
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.ID]: number; | ||
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.STATUS]: COMPRESSION_JOB_STATUS; | ||
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG]: string; | ||
| } |
There was a problem hiding this comment.
Fix type: JOB_CONFIG should be binary (Buffer), not string.
You insert a Buffer in the DB; mysql2 returns BLOBs as Buffer by default. Typing this field as string risks subtle bugs and extra copies.
Apply:
interface CompressionJob extends RowDataPacket {
[COMPRESSION_JOBS_TABLE_COLUMN_NAMES.ID]: number;
[COMPRESSION_JOBS_TABLE_COLUMN_NAMES.STATUS]: COMPRESSION_JOB_STATUS;
- [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG]: string;
+ [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG]: Buffer;
}📝 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.
| interface CompressionJob extends RowDataPacket { | |
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.ID]: number; | |
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.STATUS]: COMPRESSION_JOB_STATUS; | |
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG]: string; | |
| } | |
| interface CompressionJob extends RowDataPacket { | |
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.ID]: number; | |
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.STATUS]: COMPRESSION_JOB_STATUS; | |
| [COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG]: Buffer; | |
| } |
🤖 Prompt for AI Agents
In components/webui/server/src/typings/compression.ts around lines 36–40, the
CompressionJob interface types JOB_CONFIG as string but the DB stores/returns a
binary BLOB (Buffer); change the type of
[COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG] to Buffer (Node.js Buffer) and
update any code that assumed a string to explicitly convert the Buffer to string
when needed (e.g., buffer.toString()) or parse it directly as binary, so
consumers handle binary data correctly.
| import axios from "axios"; | ||
|
|
||
|
|
||
| type CompressionJobSchema = { |
| console.log("Submitting compression job:", JSON.stringify(payload)); | ||
|
|
||
| const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload); | ||
| return response.data.jobId; |
There was a problem hiding this comment.
the way we are doing in other functions is they return the response directly
| payload.timestampKey = values.timestampKey; | ||
| } | ||
|
|
||
| const jobId = await submitCompressionJob(payload); |
There was a problem hiding this comment.
u should probably use TanStack for this. and the isSubmitting states,etc. It will help here. You can look at dataset code for examples
There was a problem hiding this comment.
indeed. i used it to easily solve the issue of the job table not getting updated after a job submission
| } | ||
| }; | ||
|
|
||
| const isStorageEngineClpS = "clp-s" === settings.ClpStorageEngine; |
There was a problem hiding this comment.
we have enums for this. see webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx as an example
| path_prefix_to_remove: CONTAINER_INPUT_LOGS_ROOT_DIR, | ||
| }, | ||
| output: { | ||
| compression_level: 3, |
There was a problem hiding this comment.
Is there a path in the package where this is from
There was a problem hiding this comment.
right, it should come from the settings
| /** | ||
| * Default compression job configuration. | ||
| */ | ||
| const DEFAULT_COMPRESSION_JOB_CONFIG: CompressionJobConfig = Object.freeze({ |
There was a problem hiding this comment.
maybe we dont need object.freeze here, and then we dont need structured clone
There was a problem hiding this comment.
The structured clone was intended to avoid shallow copy. Otherwise, job configs might be mixed across jobs
| /** | ||
| * Interval (in milliseconds) for polling a job's completion status. | ||
| */ | ||
| const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 1000; |
There was a problem hiding this comment.
should be removed. sorry for the confusion
# Conflicts: # components/clp-package-utils/clp_package_utils/scripts/start_clp.py # components/webui/client/src/pages/SearchPage/SearchControls/index.tsx # components/webui/common/src/schemas/compression.ts # components/webui/server/eslint.config.mjs # components/webui/server/src/typings/query.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/webui/server/src/plugins/app/QueryJobDbManager/index.ts (3)
52-60: Escape identifiers; avoid string‑interpolated table/column names.Using template literals for identifiers risks SQL injection via config and breaks on reserved words. Use identifier placeholders.
Apply (and mirror to other queries below):
- const [result] = await this.#sqlPool.query<ResultSetHeader>( - ` - INSERT INTO ${this.#tableName} ( - ${QUERY_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG}, - ${QUERY_JOBS_TABLE_COLUMN_NAMES.TYPE} - ) - VALUES (?, ?) - `, - [ - Buffer.from(encode(jobConfig)), - jobType, - ] - ); + const [result] = await this.#sqlPool.query<ResultSetHeader>( + "INSERT INTO ?? (??, ??) VALUES (?, ?)", + [ + this.#tableName, + QUERY_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG, + QUERY_JOBS_TABLE_COLUMN_NAMES.TYPE, + Buffer.from(encode(jobConfig)), + jobType, + ] + );
76-83: Same escaping issue in UPDATE.- await this.#sqlPool.query( - `UPDATE ${this.#tableName} - SET ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} = ${QUERY_JOB_STATUS.CANCELLING} - WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? - AND ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} - IN (${QUERY_JOB_STATUS.PENDING}, ${QUERY_JOB_STATUS.RUNNING})`, - jobId - ); + await this.#sqlPool.query( + "UPDATE ?? SET ?? = ? WHERE ?? = ? AND ?? IN (?, ?)", + [ + this.#tableName, + QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS, QUERY_JOB_STATUS.CANCELLING, + QUERY_JOBS_TABLE_COLUMN_NAMES.ID, jobId, + QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS, QUERY_JOB_STATUS.PENDING, QUERY_JOB_STATUS.RUNNING, + ] + );
98-103: Same for SELECT in polling loop.- const [queryRows] = await this.#sqlPool.query<QueryJob[]>( - `SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} - FROM ${this.#tableName} - WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ?`, - jobId - ); + const [queryRows] = await this.#sqlPool.query<QueryJob[]>( + "SELECT ?? FROM ?? WHERE ?? = ?", + [ + QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS, + this.#tableName, + QUERY_JOBS_TABLE_COLUMN_NAMES.ID, + jobId, + ] + );
♻️ Duplicate comments (8)
components/webui/server/src/routes/api/compress/index.ts (3)
26-38: Move hard‑coded compression defaults to settings.json (with sane fallbacks).Consider keys for level/target sizes so operators can tune without code changes.
55-61: Declare 400 for bad requests (validation/runtime checks).response: { [StatusCodes.CREATED]: CompressionJobCreationSchema, + [StatusCodes.BAD_REQUEST]: ErrorSchema, [StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema, },
70-74: Robust, POSIX‑safe path handling and jail to /mnt/logs.String concat breaks on missing leading “/” and allows “..” to escape root. Use node:path and validate stays under root.
+import path from "node:path"; @@ - jobConfig.input.paths_to_compress = paths.map( - (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path - ); + if (0 === paths.length) { + return reply.badRequest("At least one path is required."); + } + try { + jobConfig.input.paths_to_compress = paths.map((p) => { + const rel = (p ?? "").trim().replace(/^\/+/, ""); + const abs = path.posix.resolve(CONTAINER_INPUT_LOGS_ROOT_DIR, rel); + if (false === abs.startsWith(CONTAINER_INPUT_LOGS_ROOT_DIR + "/")) { + throw new Error(`Path escapes logs root: ${p}`); + } + return abs; + }); + } catch (e) { + return reply.badRequest("Invalid path provided."); + }components/webui/client/src/api/compress/index.ts (1)
14-19: Remove payload logging and fix generics; optionally bubble errors.-const submitCompressionJob = async (payload: CompressionJobSchema): Promise<number> => { - console.log("Submitting compression job:", JSON.stringify(payload)); - - const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload); - return response.data.jobId; -}; +const submitCompressionJob = async (payload: CompressionJobSchemaStatic): Promise<number> => { + const response = await axios.post<CompressionJobCreationSchemaStatic>("/api/compress", payload); + return response.data.jobId; +};components/webui/client/src/pages/IngestPage/Compress/index.tsx (4)
84-85: Rename card title to match the feature (“Submit Compression Job”).- <DashboardCard title={"Start Ingestion"}> + <DashboardCard title={"Submit Compression Job"}>
88-93: Inline the submit handler; extra .catch is redundant.- onFinish={(values) => { - handleSubmit(values).catch((error: unknown) => { - console.error("Error in handleSubmit:", error); - }); - }} + onFinish={handleSubmit}
94-102: Validate at least one non‑empty path and improve placeholder.<Form.Item label={"Paths"} name={"paths"} - rules={[{required: true, message: "Please enter at least one path"}]} + rules={[ + {required: true, message: "Please enter at least one path"}, + { + validator: async (_, v?: string) => { + const count = (v ?? "").split("\n").map((p) => p.trim()).filter((p) => 0 < p.length).length; + return 0 < count ? Promise.resolve() : Promise.reject(new Error("Please enter at least one non-empty path")); + }, + }, + ]} > <Input.TextArea autoSize={{minRows: 4, maxRows: 10}} - placeholder={"Enter paths to compress, one per line"}/> + placeholder={"Enter container-relative paths (e.g., /logs/app.log), one per line"}/> </Form.Item>
48-59: Send optional fields only when non‑empty; trim values.- if ("undefined" !== typeof values.dataset) { - payload.dataset = values.dataset; - } + if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) { + payload.dataset = values.dataset.trim(); + } - if ("undefined" !== typeof values.timestampKey) { - payload.timestampKey = values.timestampKey; - } + if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) { + payload.timestampKey = values.timestampKey.trim(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(1 hunks)components/webui/client/src/api/compress/index.ts(1 hunks)components/webui/client/src/pages/IngestPage/Compress/index.tsx(1 hunks)components/webui/common/src/schemas/compression.ts(1 hunks)components/webui/server/src/plugins/app/QueryJobDbManager/index.ts(1 hunks)components/webui/server/src/routes/api/compress/index.ts(1 hunks)components/webui/server/src/typings/compression.ts(1 hunks)components/webui/server/src/typings/query.ts(1 hunks)
🧰 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/typings/query.tscomponents/webui/server/src/routes/api/compress/index.tscomponents/webui/server/src/typings/compression.tscomponents/webui/common/src/schemas/compression.tscomponents/webui/server/src/plugins/app/QueryJobDbManager/index.tscomponents/webui/client/src/api/compress/index.tscomponents/webui/client/src/pages/IngestPage/Compress/index.tsx
🧠 Learnings (1)
📚 Learning: 2024-11-21T15:51:33.203Z
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.
Applied to files:
components/webui/server/src/typings/query.ts
🧬 Code graph analysis (3)
components/webui/server/src/routes/api/compress/index.ts (1)
components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts (1)
CompressionJobConfig(13-27)
components/webui/common/src/schemas/compression.ts (1)
components/webui/client/src/api/compress/index.ts (2)
CompressionJobSchema(23-23)CompressionJobCreationSchema(22-22)
components/webui/client/src/api/compress/index.ts (1)
components/webui/common/src/schemas/compression.ts (2)
CompressionJobSchema(29-29)CompressionJobCreationSchema(28-28)
⏰ 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). (2)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (4)
components/webui/server/src/typings/query.ts (1)
5-8: Doc reference fix to constants package looks good.components/webui/server/src/typings/compression.ts (2)
19-23: Column name "clp_config": verify DB schema alignment.Search found only the enum (components/webui/server/src/typings/compression.ts) and a doc comment (components/webui/server/src/routes/api/compress/index.ts:19); no migrations or ORM model references were found — verify DB migrations/schema and any managers (including clp_py_utils) use the exact column name "clp_config" to avoid runtime errors.
6-12: No changes required — enums are in parityCompressionJobStatus in components/job-orchestration/job_orchestration/scheduler/constants.py (PENDING=0, RUNNING=1, SUCCEEDED=2, FAILED=3, KILLED=4) exactly matches COMPRESSION_JOB_STATUS (components/webui/server/src/typings/compression.ts) and CompressionJobStatus (components/webui/client/src/pages/IngestPage/Jobs/typings.tsx).
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
878-894: Propagating ClpStorageEngine to server settings — confirmed; approve changes.
Search under components/webui/server shows only components/webui/server/settings.json:23 containing "ClpStorageEngine"; no other matches detected.
| import type { | ||
| CompressionJobCreationSchema, | ||
| CompressionJobSchema, | ||
| } from "@webui/common/compression"; |
There was a problem hiding this comment.
Type imports are wrong: import the Static types, not the schemas.
The module exports schema values and separate ...Static types. Current import type {CompressionJobSchema} won’t compile.
-import type {
- CompressionJobCreationSchema,
- CompressionJobSchema,
-} from "@webui/common/compression";
+import type {
+ CompressionJobCreationSchemaStatic,
+ CompressionJobSchemaStatic,
+} from "@webui/common/schemas/compression";📝 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.
| import type { | |
| CompressionJobCreationSchema, | |
| CompressionJobSchema, | |
| } from "@webui/common/compression"; | |
| import type { | |
| CompressionJobCreationSchemaStatic, | |
| CompressionJobSchemaStatic, | |
| } from "@webui/common/schemas/compression"; |
🤖 Prompt for AI Agents
In components/webui/client/src/api/compress/index.ts lines 1-4, the file
currently imports schema values instead of the exported Static Type aliases;
replace the imports of CompressionJobCreationSchema and CompressionJobSchema
with the corresponding Static types (e.g., CompressionJobCreationStatic and
CompressionJobStatic) from "@webui/common/compression", update the import
statement to import those Static types using import type, and adjust any local
type references to use the Static names so the module compiles.
There was a problem hiding this comment.
🧩 Analysis chain
@davemarco Thank you for confirming! The type import issue needs to be fixed as suggested. The current import attempts to use the schema definitions as types, but TypeScript requires the Static type aliases that are specifically exported for this purpose.
The fix is to import CompressionJobCreationSchemaStatic and CompressionJobSchemaStatic instead of the schema definitions themselves, and use the correct import path "@webui/common/schemas/compression".
/| /|
( :v: )
|(_)|
| |
/ \
/ \
🏁 Script executed:
#!/bin/bash
# Check the exports from the compression schemas module
cat "components/webui/common/src/schemas/compression.ts"
Length of output: 764
@davemarco Thank you for confirming! You're absolutely right. Looking at the actual exports from the compression schemas module, I can confirm the type import issue needs to be fixed exactly as I suggested.
The module exports:
- The actual schemas:
CompressionJobCreationSchema,CompressionJobSchema(runtime values) - The static types:
CompressionJobCreationSchemaStatic,CompressionJobSchemaStatic(TypeScript types)
The current import is trying to use the runtime schema values as types, which won't work. The fix is to import the Static type aliases instead:
import type {
CompressionJobCreationSchemaStatic,
CompressionJobSchemaStatic,
} from "@webui/common/schemas/compression"; /| /|
( :v: )
|(_)|
| |
/ \
/ \
🧠 Learnings used
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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| export type { | ||
| CompressionJobCreationSchema, | ||
| CompressionJobSchema, | ||
| }; | ||
| export {submitCompressionJob}; |
There was a problem hiding this comment.
🧹 Nitpick
Re‑export the correct types (Static).
Aligns the public surface with real types.
-export type {
- CompressionJobCreationSchema,
- CompressionJobSchema,
-};
+export type {
+ CompressionJobCreationSchemaStatic,
+ CompressionJobSchemaStatic,
+};If other API modules return full Axios responses, confirm whether this one should mirror that pattern.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/webui/client/src/api/compress/index.ts around lines 21 to 25, the
module is re-exporting the runtime schema types (CompressionJobCreationSchema,
CompressionJobSchema) but the public API should expose the static TypeScript
types instead; change the exports to re-export the Static types (e.g., the
Static<typeof ...> aliases or pre-defined XxxStatic types) so consumers get the
correct compile-time shapes, and ensure you import/define those Static types in
this file before exporting; additionally, verify whether other API modules
return full Axios responses and, if so, make submitCompressionJob mirror that
pattern (return full AxiosResponse) or document why this one differs.
| const CompressionJobSchema = Type.Object({ | ||
| paths: Type.Array(Type.String()), | ||
| dataset: Type.Optional(Type.String()), | ||
| timestampKey: Type.Optional(Type.String()), | ||
| }); |
There was a problem hiding this comment.
Enforce non‑empty paths and trim/ban empty optionals at schema level.
Prevents empty submissions and avoids sending “” for optional fields.
-const CompressionJobSchema = Type.Object({
- paths: Type.Array(Type.String()),
- dataset: Type.Optional(Type.String()),
- timestampKey: Type.Optional(Type.String()),
-});
+const CompressionJobSchema = Type.Object(
+ {
+ paths: Type.Array(
+ Type.String({minLength: 1}),
+ {minItems: 1}
+ ),
+ dataset: Type.Optional(Type.String({minLength: 1})),
+ timestampKey: Type.Optional(Type.String({minLength: 1})),
+ },
+ {additionalProperties: false}
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const CompressionJobSchema = Type.Object({ | |
| paths: Type.Array(Type.String()), | |
| dataset: Type.Optional(Type.String()), | |
| timestampKey: Type.Optional(Type.String()), | |
| }); | |
| const CompressionJobSchema = Type.Object( | |
| { | |
| paths: Type.Array( | |
| Type.String({ minLength: 1 }), | |
| { minItems: 1 } | |
| ), | |
| dataset: Type.Optional(Type.String({ minLength: 1 })), | |
| timestampKey: Type.Optional(Type.String({ minLength: 1 })), | |
| }, | |
| { additionalProperties: false } | |
| ); |
🤖 Prompt for AI Agents
In components/webui/common/src/schemas/compression.ts around lines 10–14, the
schema currently allows empty paths arrays and empty string optionals; update it
so paths is an array with minItems:1 and each item is a non-empty string (e.g.
Type.Array(Type.String({ minLength: 1, pattern: "\\S" }), { minItems: 1 })), and
change dataset and timestampKey to Type.Optional(Type.String({ minLength: 1,
pattern: "\\S" })) so blank or whitespace-only values are rejected at the schema
level rather than accepting "".
| if ("clp-s" === settings.ClpStorageEngine) { | ||
| if ("string" !== typeof dataset || 0 === dataset.length) { | ||
| jobConfig.input.dataset = CLP_DEFAULT_DATASET_NAME; | ||
| } else { | ||
| jobConfig.input.dataset = dataset; | ||
| } | ||
| if ("undefined" !== typeof timestampKey) { | ||
| jobConfig.input.timestamp_key = timestampKey; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Trim and ignore empty optionals; set dataset default only when needed.
Avoid persisting empty strings.
- if ("clp-s" === settings.ClpStorageEngine) {
- if ("string" !== typeof dataset || 0 === dataset.length) {
- jobConfig.input.dataset = CLP_DEFAULT_DATASET_NAME;
- } else {
- jobConfig.input.dataset = dataset;
- }
- if ("undefined" !== typeof timestampKey) {
- jobConfig.input.timestamp_key = timestampKey;
- }
- }
+ if ("clp-s" === settings.ClpStorageEngine) {
+ const ds = "string" === typeof dataset ? dataset.trim() : "";
+ jobConfig.input.dataset = 0 < ds.length ? ds : CLP_DEFAULT_DATASET_NAME;
+ if ("string" === typeof timestampKey && 0 < timestampKey.trim().length) {
+ jobConfig.input.timestamp_key = timestampKey.trim();
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ("clp-s" === settings.ClpStorageEngine) { | |
| if ("string" !== typeof dataset || 0 === dataset.length) { | |
| jobConfig.input.dataset = CLP_DEFAULT_DATASET_NAME; | |
| } else { | |
| jobConfig.input.dataset = dataset; | |
| } | |
| if ("undefined" !== typeof timestampKey) { | |
| jobConfig.input.timestamp_key = timestampKey; | |
| } | |
| } | |
| if ("clp-s" === settings.ClpStorageEngine) { | |
| const ds = "string" === typeof dataset ? dataset.trim() : ""; | |
| jobConfig.input.dataset = 0 < ds.length ? ds : CLP_DEFAULT_DATASET_NAME; | |
| if ("string" === typeof timestampKey && 0 < timestampKey.trim().length) { | |
| jobConfig.input.timestamp_key = timestampKey.trim(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/compress/index.ts around lines 75 to
84, the code currently persists empty strings for optional fields; trim inputs
and only set jobConfig.input.dataset to CLP_DEFAULT_DATASET_NAME when dataset is
not a non-empty trimmed string, otherwise set dataset to the trimmed value;
likewise only assign jobConfig.input.timestamp_key when timestampKey is defined
and its trimmed length > 0. Ensure you call .trim() on dataset and timestampKey
(after checking for undefined) and avoid assigning empty strings so empty
optionals are ignored.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
878-898: Also propagate the compression-jobs table name to server settings (avoid drift).Server settings currently rely on the static value in settings.json. Keep them in sync at runtime like other DB keys.
server_settings_json_updates = { + "SqlDbCompressionJobsTableName": COMPRESSION_JOBS_TABLE_NAME, "ArchiveOutputCompressionLevel": clp_config.archive_output.compression_level, "ArchiveOutputTargetArchiveSize": clp_config.archive_output.target_archive_size, "ArchiveOutputTargetDictionariesSize": clp_config.archive_output.target_dictionaries_size, "ArchiveOutputTargetEncodedFileSize": clp_config.archive_output.target_encoded_file_size, "ArchiveOutputTargetSegmentSize": clp_config.archive_output.target_segment_size, "SqlDbHost": clp_config.database.host,
♻️ Duplicate comments (7)
components/webui/server/src/routes/api/compress/index.ts (3)
26-38: Resolved: defaults now sourced from settings.json.Good move away from hard‑coded literals; keeps operations tunable without code changes.
55-61: Declare 400 responses in the route schema.Aligns OpenAPI with runtime validation and new bad-request cases.
schema: { body: CompressionJobSchema, response: { [StatusCodes.CREATED]: CompressionJobCreationSchema, + [StatusCodes.BAD_REQUEST]: ErrorSchema, [StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema, }, tags: ["Compression"], },
75-84: Trim and ignore empty optionals; set dataset default only when needed.Prevents storing empty strings; applies CLP default cleanly.
- if ("clp-s" === settings.ClpStorageEngine) { - if ("string" !== typeof dataset || 0 === dataset.length) { - jobConfig.input.dataset = CLP_DEFAULT_DATASET_NAME; - } else { - jobConfig.input.dataset = dataset; - } - if ("undefined" !== typeof timestampKey) { - jobConfig.input.timestamp_key = timestampKey; - } - } + if ("clp-s" === settings.ClpStorageEngine) { + const ds = "string" === typeof dataset ? dataset.trim() : ""; + jobConfig.input.dataset = 0 < ds.length ? ds : CLP_DEFAULT_DATASET_NAME; + if ("string" === typeof timestampKey && 0 < timestampKey.trim().length) { + jobConfig.input.timestamp_key = timestampKey.trim(); + } + }components/webui/client/src/pages/IngestPage/Compress/index.tsx (4)
52-58: Trim and omit empty optionals before sending.Avoid persisting empty strings; send only meaningful values.
- if ("undefined" !== typeof values.dataset) { - payload.dataset = values.dataset; - } - - if ("undefined" !== typeof values.timestampKey) { - payload.timestampKey = values.timestampKey; - } + if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) { + payload.dataset = values.dataset.trim(); + } + if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) { + payload.timestampKey = values.timestampKey.trim(); + }
82-82: Rename card title to reflect the feature.“Start Ingestion” is misleading here.
- <DashboardCard title={"Start Ingestion"}> + <DashboardCard title={"Submit Compression Job"}>
86-90: Inline the submit handler; the extra .catch is redundant.handleSubmit already handles errors.
- onFinish={(values) => { - handleSubmit(values).catch((error: unknown) => { - console.error("Error in handleSubmit:", error); - }); - }} + onFinish={handleSubmit}
93-100: Validate at least one trimmed, non-empty path; improve placeholder.Prevents whitespace-only submits and clarifies expected input.
<Form.Item label={"Paths"} name={"paths"} - rules={[{required: true, message: "Please enter at least one path"}]} + rules={[ + {required: true, message: "Please enter at least one path"}, + { + validator: async (_, value?: string) => { + const count = (value ?? "") + .split("\n") + .map((p) => p.trim()) + .filter((p) => 0 < p.length).length; + if (0 < count) { return Promise.resolve(); } + return Promise.reject(new Error("Please enter at least one non-empty path")); + }, + }, + ]} > <Input.TextArea autoSize={{minRows: 4, maxRows: 10}} - placeholder={"Enter paths to compress, one per line"}/> + placeholder={"Enter container-relative paths (e.g., /logs/app.log), one per line"}/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/webui/client/src/pages/IngestPage/Compress/index.tsx(1 hunks)components/webui/server/settings.json(2 hunks)components/webui/server/src/routes/api/compress/index.ts(1 hunks)
🧰 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/client/src/pages/IngestPage/Compress/index.tsxcomponents/webui/server/src/routes/api/compress/index.ts
🧬 Code graph analysis (2)
components/webui/client/src/pages/IngestPage/Compress/index.tsx (3)
components/webui/client/src/api/compress/index.ts (2)
CompressionJobSchema(23-23)submitCompressionJob(25-25)components/webui/client/src/components/DashboardCard/index.tsx (1)
DashboardCard(57-57)components/webui/client/src/config/index.ts (2)
CLP_STORAGE_ENGINES(26-26)SETTINGS_STORAGE_ENGINE(28-28)
components/webui/server/src/routes/api/compress/index.ts (1)
components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts (1)
CompressionJobConfig(13-27)
⏰ 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). (1)
- GitHub Check: package-image
🔇 Additional comments (3)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
878-884: LGTM: wiring archive-output knobs and storage engine into server settings.Values align with settings.json keys consumed by the server route; update_settings_object will enforce key correctness.
Also applies to: 897-898
components/webui/server/settings.json (2)
6-6: LGTM: added SqlDbCompressionJobsTableName.Matches backend usage; ensure start_clp also sets this at runtime if you plan to vary it per install (prefixing, etc.).
Would you like me to update start_clp.py to set this key as well (see my suggestion there)?
22-27: LGTM: new archive-output knobs and storage-engine flag.Names match what the server route reads; numeric defaults look sane.
Also applies to: 29-29
| const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG); | ||
| jobConfig.input.paths_to_compress = paths.map( | ||
| (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Prevent path traversal and join paths robustly.
String concatenation allows “..” to escape /mnt/logs. Normalise, resolve, and enforce the root prefix.
+import path from "node:path";
@@
- const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG);
- jobConfig.input.paths_to_compress = paths.map(
- (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path
- );
+ const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG);
+ const absPaths = paths.map((p) => {
+ const rel = (p ?? "").trim().replace(/^\/+/, "");
+ return path.posix.resolve(CONTAINER_INPUT_LOGS_ROOT_DIR, rel);
+ });
+ for (const abs of absPaths) {
+ if (!(abs === CONTAINER_INPUT_LOGS_ROOT_DIR || abs.startsWith(CONTAINER_INPUT_LOGS_ROOT_DIR + "/"))) {
+ const errMsg = "Each path must resolve under /mnt/logs.";
+ return reply.badRequest(errMsg);
+ }
+ }
+ jobConfig.input.paths_to_compress = absPaths;Please confirm @fastify/sensible is registered in the server (reply.badRequest is used already via reply.internalServerError). If not, replace badRequest with an explicit 400:
- return reply.badRequest(errMsg);
+ reply.code(StatusCodes.BAD_REQUEST);
+ return {message: errMsg};🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "1) Try to locate the exact file from the review:"
rg -n --hidden -S "components/webui/server/src/routes/api/compress/index.ts" || true
echo
echo "2) Try to find the file anywhere in the repo and print a snippet if found:"
file=$(rg --hidden --files | rg "components/webui/server/src/routes/api/compress/index.ts" || true)
if [ -n "$file" ]; then
echo "FOUND: $file"
echo "---- file head (1..240) ----"
sed -n '1,240p' "$file" || true
else
echo "File not found at that exact path. Searching for related symbols..."
rg -n --hidden -S "CONTAINER_INPUT_LOGS_ROOT_DIR|paths_to_compress|paths_to_compress\\b" -C3 || true
fi
echo
echo "3) Search for @fastify/sensible in code and package.json files:"
rg -n --hidden -S '"@fastify/sensible"' --glob '!node_modules/**' || true
rg -n --hidden -S "require\\(['\"]@fastify/sensible['\"]\\)|from ['\"]@fastify/sensible['\"]|import .*from ['\"]@fastify/sensible['\"]" --glob '!node_modules/**' || true
echo
echo "4) Search for reply.badRequest / reply.internalServerError usages:"
rg -n --hidden -S "reply\\.badRequest|reply\\.internalServerError" --glob '!node_modules/**' || true
echo
echo "5) Search for fastify.register occurrences mentioning sensible or general plugin registration:"
rg -n --hidden -S "fastify\\.register\\(|register\\(.*sensible|register\\(.*'@fastify/sensible'|register\\(.*\"@fastify/sensible\"" --glob '!node_modules/**' || true
echo
echo "6) Show top-level package.json (first 200 lines) if present:"
if [ -f package.json ]; then
sed -n '1,200p' package.json || true
else
echo "no root package.json"
fi
echo
echo "Done."Length of output: 4463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "1) Show the compression route file (head):"
file="components/webui/server/src/routes/api/compress/index.ts"
if [ -f "$file" ]; then
sed -n '1,240p' "$file"
else
echo "File not found: $file"
fi
echo
echo "----------------------------------------"
echo "2) Search for @fastify/sensible occurrences (search all files, ignore .gitignore):"
rg -n -uu -S "\"@fastify/sensible\"|'@fastify/sensible'|@fastify/sensible" --hidden --no-ignore -g '!*node_modules/**' || true
echo
echo "----------------------------------------"
echo "3) Search for 'sensible' token (plugin may be referenced by name):"
rg -n -uu -S '\bsensible\b' --hidden --no-ignore -g '!*node_modules/**' || true
echo
echo "----------------------------------------"
echo "4) Search for reply.badRequest and reply.internalServerError usages:"
rg -n -uu -S "reply\\.badRequest|reply\\.internalServerError" --hidden --no-ignore -g '!*node_modules/**' -C3 || true
echo
echo "----------------------------------------"
echo "5) Search for fastify.register calls (to find plugin registration):"
rg -n -uu -S "fastify\\.register\\s*\\(" --hidden --no-ignore -g '!*node_modules/**' -C3 || true
echo
echo "6) Search for decorateReply / decorate to see custom decorations:"
rg -n -uu -S "decorateReply|decorate\\s*\\(" --hidden --no-ignore -g '!*node_modules/**' -C3 || true
echo
echo "----------------------------------------"
echo "7) Find package.json files and check for @fastify/sensible in them:"
rg -n -uu -S '"@fastify/sensible"' --hidden --no-ignore -g '!*node_modules/**' || true
echo
echo "8) List nearest package.json files (components/webui/server and parents) if present:"
for p in \
"components/webui/server/package.json" \
"components/webui/package.json" \
"components/package.json" \
"package.json"; do
if [ -f "$p" ]; then
echo "---- $p ----"
sed -n '1,200p' "$p"
echo
fi
done
echo "Done."Length of output: 7575
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "1) Grep for '@fastify/sensible' in code files:"
find . -type f \( -name '*.ts' -o -name '*.js' -o -name '*.mjs' \) -not -path '*/node_modules/*' -not -path '*/.git/*' -print0 | xargs -0 grep -nH --color=never "@fastify/sensible" || true
echo
echo "2) Search for fastify.register lines referencing sensible or autoload usage:"
find . -type f \( -name '*.ts' -o -name '*.js' -o -name '*.mjs' \) -not -path '*/node_modules/*' -not -path '*/.git/*' -print0 | xargs -0 grep -nH --color=never -E "fastify\.register\s*\(|register\(|autoload" || true
echo
echo "3) Search for 'sensible' token anywhere:"
find . -type f \( -name '*.ts' -o -name '*.js' -o -name '*.mjs' -o -name '*.json' \) -not -path '*/node_modules/*' -not -path '*/.git/*' -print0 | xargs -0 grep -nH --color=never -E "\bsensible\b" || true
echo
echo "4) Show plugins directory if present (first 200 lines per file):"
if [ -d components/webui/server/src/plugins ]; then
find components/webui/server/src/plugins -maxdepth 2 -type f -print -exec sed -n '1,200p' {} \; || true
else
echo "No plugins dir at components/webui/server/src/plugins"
fi
echo
echo "5) Show server bootstrap files (src/main.ts, src/app.ts, src/index.ts, src/server.ts):"
for f in components/webui/server/src/main.ts components/webui/server/src/app.ts components/webui/server/src/index.ts components/webui/server/src/server.ts; do
if [ -f "$f" ]; then
echo "---- $f ----"
sed -n '1,240p' "$f"
echo
fi
done
echo
echo "6) Search for reply.badRequest usage:"
find . -type f \( -name '*.ts' -o -name '*.js' -o -name '*.mjs' \) -not -path '*/node_modules/*' -not -path '*/.git/*' -print0 | xargs -0 grep -nH --color=never "reply\\.badRequest" || true
echo
echo "Done."Length of output: 15832
Prevent path traversal and join paths robustly.
Normalise and resolve each supplied path against CONTAINER_INPUT_LOGS_ROOT_DIR; reject any that resolve outside /mnt/logs.
File: components/webui/server/src/routes/api/compress/index.ts (around lines 70–74)
+import path from "node:path";
@@
- const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG);
- jobConfig.input.paths_to_compress = paths.map(
- (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path
- );
+ const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG);
+ const absPaths = paths.map((p) => {
+ const rel = (p ?? "").trim().replace(/^\/+/, "");
+ return path.posix.resolve(CONTAINER_INPUT_LOGS_ROOT_DIR, rel);
+ });
+ for (const abs of absPaths) {
+ if (!(abs === CONTAINER_INPUT_LOGS_ROOT_DIR || abs.startsWith(CONTAINER_INPUT_LOGS_ROOT_DIR + "/"))) {
+ const errMsg = "Each path must resolve under /mnt/logs.";
+ return reply.badRequest(errMsg);
+ }
+ }
+ jobConfig.input.paths_to_compress = absPaths;Confirmed @fastify/sensible is registered (components/webui/server/src/plugins/external/sensible.ts, autoloaded in src/app.ts) — reply.badRequest is available; no replacement with manual 400 required.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG); | |
| jobConfig.input.paths_to_compress = paths.map( | |
| (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path | |
| ); | |
| import path from "node:path"; | |
| const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG); | |
| const absPaths = paths.map((p) => { | |
| // Strip leading slashes and whitespace | |
| const rel = (p ?? "").trim().replace(/^\/+/, ""); | |
| // Resolve against the logs root to collapse any “..” segments | |
| return path.posix.resolve(CONTAINER_INPUT_LOGS_ROOT_DIR, rel); | |
| }); | |
| // Reject any paths that escape the logs directory | |
| for (const abs of absPaths) { | |
| if ( | |
| abs !== CONTAINER_INPUT_LOGS_ROOT_DIR && | |
| !abs.startsWith(CONTAINER_INPUT_LOGS_ROOT_DIR + "/") | |
| ) { | |
| const errMsg = "Each path must resolve under /mnt/logs."; | |
| return reply.badRequest(errMsg); | |
| } | |
| } | |
| jobConfig.input.paths_to_compress = absPaths; |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
878-898: Server settings miss SqlDbCompressionJobsTableName update (prefix drift risk).Client settings set SqlDbCompressionJobsTableName, but server settings do not. If your DB uses a table prefix, the server will keep the default "compression_jobs" and drift from the created/prefixed table. Add it alongside SqlDbQueryJobsTableName.
Apply this diff:
server_settings_json_updates = { + "SqlDbCompressionJobsTableName": COMPRESSION_JOBS_TABLE_NAME, "ArchiveOutputCompressionLevel": clp_config.archive_output.compression_level, "ArchiveOutputTargetArchiveSize": clp_config.archive_output.target_archive_size, "ArchiveOutputTargetDictionariesSize": clp_config.archive_output.target_dictionaries_size, - "ArchiveOutputTargetEncodedFileSize": clp_config.archive_output.target_encoded_file_size, + "ArchiveOutputTargetEncodedFileSize": clp_config.archive_output.target_encoded_file_size, "ArchiveOutputTargetSegmentSize": clp_config.archive_output.target_segment_size, "SqlDbHost": clp_config.database.host, "SqlDbPort": clp_config.database.port, "SqlDbName": clp_config.database.name, "SqlDbQueryJobsTableName": QUERY_JOBS_TABLE_NAME, "MongoDbHost": clp_config.results_cache.host, "MongoDbPort": clp_config.results_cache.port, "MongoDbName": clp_config.results_cache.db_name, "MongoDbSearchResultsMetadataCollectionName": clp_config.webui.results_metadata_collection_name, "MongoDbStreamFilesCollectionName": clp_config.results_cache.stream_collection_name, "ClientDir": str(container_webui_dir / "client"), "LogViewerDir": str(container_webui_dir / "yscope-log-viewer"), "StreamTargetUncompressedSize": container_clp_config.stream_output.target_uncompressed_size, "ClpQueryEngine": clp_config.package.query_engine, "ClpStorageEngine": clp_config.package.storage_engine, }components/webui/server/settings.json (1)
1-32: Add ContainerInputLogsRootDir to settings to avoid hard-coded TS constant.The API route currently hard-codes "/mnt/logs". Centralise it here so it can be overridden consistently.
Apply this diff (adjust placement as you see fit):
{ "SqlDbHost": "localhost", "SqlDbPort": 3306, "SqlDbName": "clp-db", "SqlDbQueryJobsTableName": "query_jobs", "SqlDbCompressionJobsTableName": "compression_jobs", + "ContainerInputLogsRootDir": "/mnt/logs", "MongoDbHost": "localhost",Then consume this in the server route (see related comment).
♻️ Duplicate comments (9)
components/webui/server/src/routes/api/compress/index.ts (5)
26-38: Nice: defaults sourced from settings.json (no hard-coded literals).This addresses prior feedback about making compression parameters configurable.
55-60: Expose 400 Bad Request in schema.Declare 400 for validation/runtime input errors.
schema: { body: CompressionJobSchema, response: { [StatusCodes.CREATED]: CompressionJobCreationSchema, + [StatusCodes.BAD_REQUEST]: ErrorSchema, [StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema, }, tags: ["Compression"], },
75-84: Trim/omit empty optionals; set dataset default only when needed.Prevents persisting empty strings and matches prior feedback.
- if ("clp-s" === settings.ClpStorageEngine) { - if ("string" !== typeof dataset || 0 === dataset.length) { - jobConfig.input.dataset = CLP_DEFAULT_DATASET_NAME; - } else { - jobConfig.input.dataset = dataset; - } - if ("undefined" !== typeof timestampKey) { - jobConfig.input.timestamp_key = timestampKey; - } - } + if ("clp-s" === settings.ClpStorageEngine) { + const ds = "string" === typeof dataset ? dataset.trim() : ""; + jobConfig.input.dataset = 0 < ds.length ? ds : CLP_DEFAULT_DATASET_NAME; + if ("string" === typeof timestampKey && 0 < timestampKey.trim().length) { + jobConfig.input.timestamp_key = timestampKey.trim(); + } + }
70-73: Fix path join and block traversal outside the input root.String concatenation is fragile and allows "../" to escape /mnt/logs. Use POSIX join/normalise and enforce the root.
+import path from "node:path"; @@ - jobConfig.input.paths_to_compress = paths.map( - (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path - ); + jobConfig.input.paths_to_compress = paths.map((raw) => { + const p = String(raw ?? "").replace(/\\/g, "/").trim(); + // Accept both container-absolute and relative inputs + const rel = p.startsWith(CONTAINER_INPUT_LOGS_ROOT_DIR) + ? p.slice(CONTAINER_INPUT_LOGS_ROOT_DIR.length) + : p; + const joined = path.posix.join( + CONTAINER_INPUT_LOGS_ROOT_DIR, + rel.replace(/^\/+/, "") + ); + const normalised = path.posix.normalize(joined); + if ( + normalised !== CONTAINER_INPUT_LOGS_ROOT_DIR && + false == normalised.startsWith(CONTAINER_INPUT_LOGS_ROOT_DIR + "/") + ) { + reply.code(StatusCodes.BAD_REQUEST); + throw new Error(`Path escapes input root: ${p}`); + } + return normalised; + });
13-22: Read CONTAINER_INPUT_LOGS_ROOT_DIR from settings (fallback to "/mnt/logs")Replace the hard-coded constant in components/webui/server/src/routes/api/compress/index.ts so it reads the value from settings with a safe fallback:
-const CONTAINER_INPUT_LOGS_ROOT_DIR = "/mnt/logs"; +const CONTAINER_INPUT_LOGS_ROOT_DIR = + (settings as {ContainerInputLogsRootDir?: string}).ContainerInputLogsRootDir ?? "/mnt/logs";Add "ContainerInputLogsRootDir": "/mnt/logs" to components/webui/server/settings.json and update components/package-template/src/sbin/start-clp.sh and any docs that assume /mnt/logs (so container volume mounts and documentation match the configured default).
components/webui/client/src/pages/IngestPage/Compress/index.tsx (4)
82-82: Rename card title to reflect the feature.Use “Submit Compression Job”.
- <DashboardCard title={"Start Ingestion"}> + <DashboardCard title={"Submit Compression Job"}>
86-90: Inline handler; remove redundant catch.handleSubmit already handles errors.
- onFinish={(values) => { - handleSubmit(values).catch((error: unknown) => { - console.error("Error in handleSubmit:", error); - }); - }} + onFinish={handleSubmit}
92-100: Validate at least one non-empty trimmed path.Block whitespace-only input.
<Form.Item label={"Paths"} name={"paths"} - rules={[{required: true, message: "Please enter at least one path"}]} + rules={[ + {required: true, message: "Please enter at least one path"}, + { + validator: async (_, value?: string) => { + const count = (value ?? "") + .split("\n") + .map((p) => p.trim()) + .filter((p) => 0 < p.length).length; + return 0 < count ? Promise.resolve() : + Promise.reject(new Error("Please enter at least one non-empty path")); + }, + }, + ]} > <Input.TextArea autoSize={{minRows: 4, maxRows: 10}} - placeholder={"Enter paths to compress, one per line"}/> + placeholder={"Enter container‑relative paths (e.g., /app.log), one per line"}/>
52-58: Trim and omit empty optionals.Avoid sending empty strings; let server defaults apply.
- if ("undefined" !== typeof values.dataset) { - payload.dataset = values.dataset; - } + if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) { + payload.dataset = values.dataset.trim(); + } - if ("undefined" !== typeof values.timestampKey) { - payload.timestampKey = values.timestampKey; - } + if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) { + payload.timestampKey = values.timestampKey.trim(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/webui/client/src/pages/IngestPage/Compress/index.tsx(1 hunks)components/webui/server/settings.json(2 hunks)components/webui/server/src/routes/api/compress/index.ts(1 hunks)
🧰 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/index.tscomponents/webui/client/src/pages/IngestPage/Compress/index.tsx
🧬 Code graph analysis (2)
components/webui/server/src/routes/api/compress/index.ts (1)
components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts (1)
CompressionJobConfig(13-27)
components/webui/client/src/pages/IngestPage/Compress/index.tsx (3)
components/webui/client/src/api/compress/index.ts (2)
CompressionJobSchema(23-23)submitCompressionJob(25-25)components/webui/client/src/components/DashboardCard/index.tsx (1)
DashboardCard(57-57)components/webui/client/src/config/index.ts (2)
CLP_STORAGE_ENGINES(26-26)SETTINGS_STORAGE_ENGINE(28-28)
🪛 GitHub Actions: clp-artifact-build
components/webui/client/src/pages/IngestPage/Compress/index.tsx
[error] 1-1: Web UI build failed. Command 'npm run build' exited with code 2 due to TypeScript error referencing src/api/compress/index.ts(4,8).
🔇 Additional comments (5)
components/webui/server/settings.json (2)
6-6: Good: adds SqlDbCompressionJobsTableName.Ensure start_clp.py also updates this at runtime to include any table prefix (see separate comment).
29-29: ClpStorageEngine setting added: OK.This unblocks server/UI conditional logic.
components/webui/client/src/pages/IngestPage/Compress/index.tsx (1)
60-66: Type of jobId: client already returns number — no change requiredsubmitCompressionJob is declared as (payload: CompressionJobSchema): Promise and returns response.data.jobId from axios.post; no changes needed in this client file.
components/webui/server/src/routes/api/compress/index.ts (2)
86-95: jobId type consistent across server, common schema, and client. CompressionJobCreationSchema defines jobId as number; CompressionJobDbManager.submitJob returns Promise; client submitCompressionJob posts CompressionJobCreationSchema and returns number — no change required.
9-9: Import attribute compatibility — confirmed. server/tsconfig.json uses module: "nodenext", moduleResolution: "node16" and resolveJsonModule: true, and components/webui/server/package.json lists TypeScript ^5.7.3 — the static import with {type: "json"} is supported; no TS-config or TS-version changes required.
components/clp-package-utils/clp_package_utils/scripts/start_clp.py
Outdated
Show resolved
Hide resolved
| import {useState} from "react"; | ||
|
|
||
| import { | ||
| Button, | ||
| Form, | ||
| Input, | ||
| Typography, | ||
| } from "antd"; |
There was a problem hiding this comment.
🧩 Analysis chain
CI failure: address TypeScript error in client api/compress.
Build failed (2025-09-17) with an error at src/api/compress/index.ts(4,8). Please surface that file and align types/imports.
🏁 Script executed:
#!/bin/bash
# Locate and print the failing file and imports around the error site.
fd -a 'components/webui/client/src/api/compress/index.ts'
sed -n '1,120p' components/webui/client/src/api/compress/index.tsLength of output: 610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "Searching for type names 'CompressionJobCreationSchema' or 'CompressionJobSchema'..."
rg -n --hidden --no-ignore -S "CompressionJobCreationSchema|CompressionJobSchema" || true
echo
echo "Listing files that import from '@webui/common/compression'..."
rg -n --hidden --no-ignore -S "@webui/common/compression" || true
echo
echo "Show tsconfig path mappings (if present)..."
for f in tsconfig*.json package.json; do
if [ -f "$f" ]; then
echo "==== $f ===="
jq -r 'paths as $p | (getpath($p) | tostring) as $v | ($p|join(".")) + " -> " + $v' "$f" 2>/dev/null || true
fi
done
echo
echo "Print the previously shown file for context:"
sed -n '1,240p' components/webui/client/src/api/compress/index.ts || trueLength of output: 3030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== components/webui/common/src/schemas/compression.ts ==="
sed -n '1,240p' components/webui/common/src/schemas/compression.ts || true
echo
echo "=== components/webui/common/src/index.ts ==="
sed -n '1,240p' components/webui/common/src/index.ts || true
echo
echo "=== components/webui/client/src/api/compress/index.ts ==="
sed -n '1,240p' components/webui/client/src/api/compress/index.ts || true
echo
echo "=== components/webui/client/src/pages/IngestPage/Compress/index.tsx ==="
sed -n '1,240p' components/webui/client/src/pages/IngestPage/Compress/index.tsx || true
echo
echo "=== components/webui/common/package.json ==="
sed -n '1,240p' components/webui/common/package.json || true
echo
echo "=== components/webui/client/package.json ==="
sed -n '1,240p' components/webui/client/package.json || true
echo
echo "=== tsconfig.json (root) ==="
sed -n '1,240p' tsconfig.json || true
echo
echo "=== components/webui/client/tsconfig.json ==="
sed -n '1,240p' components/webui/client/tsconfig.json || trueLength of output: 8991
Fix TypeScript import/type mismatch in client api/compress
components/webui/client/src/api/compress/index.ts imports CompressionJobCreationSchema and CompressionJobSchema as types, but @webui/common/compression exports runtime schema values with those names and the TS types are CompressionJobCreationSchemaStatic / CompressionJobSchemaStatic — update the import (or re-export) to use the Static types and alias them to the current names so callers don’t need changes.
Suggested replacement (in components/webui/client/src/api/compress/index.ts):
import type { CompressionJobCreationSchemaStatic as CompressionJobCreationSchema, CompressionJobSchemaStatic as CompressionJobSchema } from "@webui/common/compression";
🧰 Tools
🪛 GitHub Actions: clp-artifact-build
[error] 1-1: Web UI build failed. Command 'npm run build' exited with code 2 due to TypeScript error referencing src/api/compress/index.ts(4,8).
🤖 Prompt for AI Agents
In components/webui/client/src/pages/IngestPage/Compress/index.tsx lines 1 to 8,
the client currently imports CompressionJobCreationSchema and
CompressionJobSchema as types but @webui/common/compression exports runtime
schema values under those names while the TypeScript types are named
CompressionJobCreationSchemaStatic and CompressionJobSchemaStatic; update the
import in components/webui/client/src/api/compress/index.ts to import the Static
types and alias them to the expected names (e.g. import type {
CompressionJobCreationSchemaStatic as CompressionJobCreationSchema,
CompressionJobSchemaStatic as CompressionJobSchema } from
"@webui/common/compression") so callers keep using the existing type names
without further changes.
components/webui/client/src/pages/IngestPage/Compress/index.tsx
Outdated
Show resolved
Hide resolved
| "ArchiveOutputCompressionLevel": 3, | ||
| "ArchiveOutputTargetArchiveSize": 268435456, | ||
| "ArchiveOutputTargetDictionariesSize": 33554432, | ||
| "ArchiveOutputTargetEncodedFileSize": 268435456, | ||
| "ArchiveOutputTargetSegmentSize": 268435456, |
There was a problem hiding this comment.
🧹 Nitpick
Clarify units and ranges for ArchiveOutput values.*
Document these as bytes in accompanying docs and validate ranges server-side to prevent invalid submissions.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
components/webui/client/src/api/compress/index.ts (4)
5-8: Fix broken type imports: use Static types from common/schemas.Build failure stems from importing runtime schemas as types. Import the Static types and alias to current names.
-import type { - CompressionJobCreationSchema, - CompressionJobSchema, -} from "@webui/common/compression"; +import type { + CompressionJobCreationSchemaStatic as CompressionJobCreationSchema, + CompressionJobSchemaStatic as CompressionJobSchema, +} from "@webui/common/schemas/compression";
18-23: Remove payload logging (paths are sensitive); keep axios error propagation.Local filesystem paths can leak via console. Let React Query surface errors.
-const submitCompressionJob = async (payload: CompressionJobSchema): Promise<number> => { - console.log("Submitting compression job:", JSON.stringify(payload)); - - const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload); +const submitCompressionJob = async (payload: CompressionJobSchema): Promise<number> => { + const response = await axios.post<CompressionJobCreationSchema>("/api/compress", payload); return response.data.jobId; };
42-45: Re‑export Static types, not runtime schemas.Expose compile‑time shapes to consumers.
-export type { - CompressionJobCreationSchema, - CompressionJobSchema, -}; +export type { + CompressionJobCreationSchemaStatic as CompressionJobCreationSchema, + CompressionJobSchemaStatic as CompressionJobSchema, +} from "@webui/common/schemas/compression";
18-23: API return-shape inconsistency — return AxiosResponse or document deviation.Most API helpers under components/webui/client/src/api return the Axios response (e.g., search/presto-search return axios.post directly; stream-files returns Promise<AxiosResponse<...>>). components/webui/client/src/api/compress/index.ts (lines 18–23) returns response.data.jobId (number). Either return the AxiosResponse (change signature to Promise<AxiosResponse> and return axios.post(...)) or add a short comment documenting why this helper intentionally returns only the jobId.
components/webui/client/src/pages/IngestPage/Compress/index.tsx (5)
59-65: Send optional fields only when non‑empty (trim).Prevents sending empty strings and lets server defaults apply.
- if ("undefined" !== typeof values.dataset) { - payload.dataset = values.dataset; - } + if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) { + payload.dataset = values.dataset.trim(); + } - if ("undefined" !== typeof values.timestampKey) { - payload.timestampKey = values.timestampKey; - } + if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) { + payload.timestampKey = values.timestampKey.trim(); + }
100-107: Validate at least one trimmed, non‑empty path.Whitespace‑only input currently passes required.
<Form.Item label={"Paths"} name={"paths"} - rules={[{required: true, message: "Please enter at least one path"}]} + rules={[ + {required: true, message: "Please enter at least one path"}, + { + validator: async (_, value?: string) => { + const count = (value ?? "") + .split(/\r?\n/) + .map((p) => p.trim()) + .filter((p) => 0 < p.length).length; + if (0 < count) { return Promise.resolve(); } + return Promise.reject(new Error("Please enter at least one non-empty path")); + }, + }, + ]} > <Input.TextArea autoSize={{minRows: 4, maxRows: 10}} - placeholder={"Enter paths to compress, one per line"}/> + placeholder={"Enter container-relative paths (e.g., /logs/app.log), one per line"}/>
90-90: Rename card title to match the feature.Improves clarity.
- <DashboardCard title={"Start Ingestion"}> + <DashboardCard title={"Submit Compression Job"}>
93-98: Inline onFinish and disable the form while submitting.Reduces noise and prevents edits during submit.
- <Form + <Form form={form} layout={"vertical"} - onFinish={(values) => { - handleSubmit(values); - }} + onFinish={handleSubmit} + disabled={isSubmitting} >
131-139: Disable the submit button while submitting.Minor UX polish.
<Button htmlType={"submit"} loading={isSubmitting} type={"primary"} + disabled={isSubmitting} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/webui/client/src/api/compress/index.ts(1 hunks)components/webui/client/src/pages/IngestPage/Compress/index.tsx(1 hunks)
🧰 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/client/src/pages/IngestPage/Compress/index.tsxcomponents/webui/client/src/api/compress/index.ts
🧠 Learnings (1)
📚 Learning: 2024-11-21T15:51:33.203Z
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.
Applied to files:
components/webui/client/src/api/compress/index.ts
🧬 Code graph analysis (2)
components/webui/client/src/pages/IngestPage/Compress/index.tsx (4)
components/webui/client/src/api/compress/index.ts (2)
submitCompressionJob(47-47)useSubmitCompressionJob(47-47)components/webui/common/src/schemas/compression.ts (1)
CompressionJobSchemaStatic(33-33)components/webui/client/src/components/DashboardCard/index.tsx (1)
DashboardCard(57-57)components/webui/client/src/config/index.ts (2)
CLP_STORAGE_ENGINES(26-26)SETTINGS_STORAGE_ENGINE(28-28)
components/webui/client/src/api/compress/index.ts (1)
components/webui/common/src/schemas/compression.ts (2)
CompressionJobSchema(29-29)CompressionJobCreationSchema(28-28)
⏰ 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). (1)
- GitHub Check: package-image
| return useMutation({ | ||
| mutationFn: submitCompressionJob, | ||
| onSettled: async () => { | ||
| // Invalidate queries that are affected by a new compression job. | ||
| await queryClient.invalidateQueries({queryKey: ["jobs"]}); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
🧩 Analysis chain
Confirm the query key before invalidation.
Ensure ["jobs"] matches your Jobs table key; centralise if a constant exists.
🏁 Script executed:
#!/bin/bash
# Verify all usages of the jobs query key to ensure consistency.
rg -nP --glob 'components/webui/client/**' '\["jobs"\]' -C2Length of output: 1472
Query key verified — consider centralising
Both usages of queryKey ["jobs"] match: components/webui/client/src/pages/IngestPage/Jobs/index.tsx:28 and components/webui/client/src/api/compress/index.ts:37. No invalidation bug detected.
Optional: extract a shared constant (e.g. const JOBS_QUERY_KEY = ["jobs"]) and use it in both places to avoid future drift.
🤖 Prompt for AI Agents
In components/webui/client/src/api/compress/index.ts around lines 33 to 39, the
queryKey ["jobs"] used in invalidateQueries should be centralized to prevent
future drift; create a shared constant (e.g. export const JOBS_QUERY_KEY =
["jobs"]) in a common place (suggest components/webui/client/src/api/queries.ts
or a nearby shared constants file), export it, then replace the literal ["jobs"]
here with an import of JOBS_QUERY_KEY and update
components/webui/client/src/pages/IngestPage/Jobs/index.tsx to import and use
the same JOBS_QUERY_KEY.
| @@ -0,0 +1,156 @@ | |||
| import {useState} from "react"; | |||
|
|
|||
| import {CompressionJobSchemaStatic} from "@webui/common/schemas/compression"; | |||
There was a problem hiding this comment.
🧹 Nitpick
Type‑only import for TS types.
Avoid emitting a runtime import for a type.
-import {CompressionJobSchemaStatic} from "@webui/common/schemas/compression";
+import type {CompressionJobSchemaStatic} from "@webui/common/schemas/compression";📝 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.
| import {CompressionJobSchemaStatic} from "@webui/common/schemas/compression"; | |
| import type {CompressionJobSchemaStatic} from "@webui/common/schemas/compression"; |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/IngestPage/Compress/index.tsx around line
3, the import of CompressionJobSchemaStatic is currently a runtime import but
it’s only used as a TypeScript type; change the statement to a type-only import
(use "import type { CompressionJobSchemaStatic } from '...';") so the compiler
emits no runtime import, and ensure any usage is purely type-level (or use type
assertions) to avoid needing the value at runtime.
| const payload: CompressionJobSchemaStatic = { | ||
| paths: paths, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick
Optional: de‑duplicate paths before submit.
Prevents redundant work server‑side.
- const payload: CompressionJobSchemaStatic = {
- paths: paths,
- };
+ const payload: CompressionJobSchemaStatic = {
+ paths: Array.from(new Set(paths)),
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const payload: CompressionJobSchemaStatic = { | |
| paths: paths, | |
| }; | |
| const payload: CompressionJobSchemaStatic = { | |
| paths: Array.from(new Set(paths)), | |
| }; |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/IngestPage/Compress/index.tsx around lines
55 to 57, the payload.paths may contain duplicate entries which causes redundant
server work; before constructing the CompressionJobSchemaStatic payload,
deduplicate the paths array (preserve order) and assign the deduplicated array
to payload.paths so only unique paths are submitted; implement using a simple
Set/filter or Array.from(new Set(...)) (or an order-preserving filter) and
ensure the resulting array still matches the expected type.
…iles; use it for checks in the server
…nd update import path for CompressionJobConfig
… value in form input
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (2)
17-19: Avoid referencing React namespace without a type import.
React.FormEventmay error underjsx: react-jsxunlessReactis imported for types. Import the type and drop the namespace.Apply:
+import type { FormEvent } from "react"; @@ -const handleSubmit = (ev: React.FormEvent<HTMLFormElement>) => { +const handleSubmit = (ev: FormEvent<HTMLFormElement>) => {
30-37: Type the JSX and prefer const over mutable let.Avoid implicit any on
controlsand mutation. Compute once and annotate asJSX.Element.Apply:
- let controls; - if (SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO) { - controls = <NativeControls/>; - } else if (isPrestoGuided) { - controls = <GuidedControls/>; - } else { - controls = <FreeformControls/>; - } + const controls: JSX.Element = + SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO + ? <NativeControls/> + : (isPrestoGuided ? <GuidedControls/> : <FreeformControls/>);components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts (2)
99-116: On submit failure, UI state remains QUERY_ID_PENDING. Set FAILED and notify user.Current catch only logs; UX can get stuck.
Apply this diff:
.catch((err: unknown) => { console.error("Failed to submit query:", err); + store.updateSearchUiState(SEARCH_UI_STATE.FAILED); + message.error("Failed to submit query. Please try again."); });
42-47: Guard against NaN after Number() coercion of IDs.If IDs are non‑numeric strings, clearing will send NaN.
Apply this diff:
- clearQueryResults( - { - searchJobId: Number(searchJobId), - aggregationJobId: Number(aggregationJobId), - } - ).catch((err: unknown) => { + const sid = Number(searchJobId); + const aid = Number(aggregationJobId); + if (false == Number.isFinite(sid) || false == Number.isFinite(aid)) { + console.error("Cannot clear results: job IDs are invalid numbers."); + return; + } + clearQueryResults( + { + searchJobId: sid, + aggregationJobId: aid, + } + ).catch((err: unknown) => { console.error("Failed to clear query results:", err); });
♻️ Duplicate comments (10)
components/webui/client/src/pages/IngestPage/Compress/PathsInputFormItem.tsx (1)
13-21: Validate at least one non-empty trimmed path; clarify placeholder.Whitespace-only passes the current rule.
Apply:
<Form.Item label={"Paths"} name={"paths"} - rules={[{required: true, message: "Please enter at least one path"}]} + rules={[ + {required: true, message: "Please enter at least one path"}, + { + validator: async (_, value?: string) => { + const count = (value ?? "") + .split(/\r?\n/) + .map((p) => p.trim()) + .filter((p) => 0 < p.length).length; + return 0 < count ? Promise.resolve() : + Promise.reject(new Error("Please enter at least one non-empty path")); + }, + }, + ]} > <Input.TextArea - autoSize={{minRows: 4, maxRows: 10}} - placeholder={"Enter paths to compress, one per line"}/> + autoSize={{minRows: 4, maxRows: 10}} + placeholder={"Enter container-relative paths under /mnt/logs, one per line"}/>components/webui/client/src/pages/IngestPage/Compress/index.tsx (4)
63-81: CRLF‑safe split; trim and de‑duplicate; send optionals only when non‑empty.Prevents stray “\r”, duplicate paths, and empty strings reaching the server.
Apply:
- const paths = values.paths - .split("\n") - .map((path) => path.trim()) - .filter((path) => 0 < path.length); + const paths = Array.from(new Set( + values.paths + .split(/\r?\n/) + .map((p) => p.trim()) + .filter((p) => 0 < p.length) + )); const payload: CompressionJobCreation = {paths}; - if ("undefined" !== typeof values.dataset) { - payload.dataset = values.dataset; - } - if ("undefined" !== typeof values.timestampKey) { - payload.timestampKey = values.timestampKey; - } + if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) { + payload.dataset = values.dataset.trim(); + } + if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) { + payload.timestampKey = values.timestampKey.trim(); + }
51-60: Surface success via message API.Keep UX consistent with error path.
Apply:
- onSuccess: () => { + onSuccess: ({jobId}) => { + messageApi.success(`Compression job submitted. ID: ${jobId}`); form.resetFields(); }, onError: (err: unknown) => { const errorMessage = err instanceof Error ? err.message : "Unknown error"; messageApi.error(`Failed to submit compression job: ${errorMessage}`); },
85-95: Rename card title to match the feature; disable form while submitting.Apply:
- <DashboardCard title={"Start Ingestion"}> + <DashboardCard title={"Submit Compression Job"}> {contextHolder} <Form form={form} layout={"vertical"} + disabled={isSubmitting} onFinish={handleSubmit} >
5-7: Use type‑only import.Avoids emitting a runtime import for a type.
Apply:
-import {CompressionJobCreation} from "@webui/common/schemas/compression"; +import type {CompressionJobCreation} from "@webui/common/schemas/compression";components/webui/server/src/routes/api/compress/index.ts (3)
48-56: Declare 400 Bad Request in the route schema.Covers validation/runtime input errors.
Apply:
schema: { body: CompressionJobCreationSchema, response: { [StatusCodes.CREATED]: CompressionJobSchema, + [StatusCodes.BAD_REQUEST]: ErrorSchema, [StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema, }, tags: ["Compression"], },
1-3: Robust path join and traversal guard (POSIX).String concatenation is brittle and allows “..” to escape the root.
Apply:
import {FastifyPluginAsyncTypebox} from "@fastify/type-provider-typebox"; import {CLP_STORAGE_ENGINES} from "@webui/common/config"; +import path from "node:path"; @@ - const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG); - jobConfig.input.paths_to_compress = paths.map( - (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path - ); + const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG); + const absPaths = paths.map((p) => { + const rel = (p ?? "").trim().replace(/^\/+/, ""); + return path.posix.resolve(CONTAINER_INPUT_LOGS_ROOT_DIR, rel); + }); + for (const abs of absPaths) { + if (!(abs === CONTAINER_INPUT_LOGS_ROOT_DIR || abs.startsWith(CONTAINER_INPUT_LOGS_ROOT_DIR + "/"))) { + return reply.badRequest("Each path must resolve under the container input logs root."); + } + } + jobConfig.input.paths_to_compress = absPaths;Also applies to: 67-71
72-81: Trim optionals; avoid persisting empty strings; default dataset only when needed.Apply:
- if (CLP_STORAGE_ENGINES.CLP_S === settings.ClpStorageEngine as CLP_STORAGE_ENGINES) { - if ("string" !== typeof dataset || 0 === dataset.length) { - jobConfig.input.dataset = CLP_DEFAULT_DATASET_NAME; - } else { - jobConfig.input.dataset = dataset; - } - if ("undefined" !== typeof timestampKey) { - jobConfig.input.timestamp_key = timestampKey; - } - } + if (CLP_STORAGE_ENGINES.CLP_S === settings.ClpStorageEngine as CLP_STORAGE_ENGINES) { + const ds = "string" === typeof dataset ? dataset.trim() : ""; + jobConfig.input.dataset = 0 < ds.length ? ds : CLP_DEFAULT_DATASET_NAME; + if ("string" === typeof timestampKey && 0 < timestampKey.trim().length) { + jobConfig.input.timestamp_key = timestampKey.trim(); + } + }components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts (2)
67-74: Enforce plugin load order: require mysql decorator and declare dependency.Prevents decoration‑order runtime errors.
Apply:
export default fp( (fastify) => { fastify.decorate("CompressionJobDbManager", CompressionJobDbManager.create(fastify)); }, { name: "CompressionJobDbManager", + decorators: {fastify: ["mysql"]}, + dependencies: ["@fastify/mysql"], } );
45-55: SQL hardening and remove extra copy; prefer prepared execute with identifier placeholders.Avoid interpolation and redundant Buffer.from.
Apply:
- const [result] = await this.#sqlPool.query<ResultSetHeader>( - ` - INSERT INTO ${this.#tableName} ( - ${COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG} - ) - VALUES (?) - `, - [ - Buffer.from(brotliCompressSync(encode(jobConfig))), - ] - ); + const payload = brotliCompressSync(encode(jobConfig)); + const [result] = await this.#sqlPool.execute<ResultSetHeader>( + "INSERT INTO ?? ( ?? ) VALUES ( ? )", + [this.#tableName, COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG, payload] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx(1 hunks)components/webui/client/src/pages/IngestPage/Compress/PathsInputFormItem.tsx(1 hunks)components/webui/client/src/pages/IngestPage/Compress/SubmitFormItem.tsx(1 hunks)components/webui/client/src/pages/IngestPage/Compress/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts(2 hunks)components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts(1 hunks)components/webui/server/src/plugins/app/CompressionJobDbManager/typings.ts(1 hunks)components/webui/server/src/routes/api/compress/index.ts(1 hunks)components/webui/server/src/routes/api/compress/typings.ts(1 hunks)
🧰 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/client/src/pages/SearchPage/SearchControls/search-requests.tscomponents/webui/client/src/pages/IngestPage/Compress/PathsInputFormItem.tsxcomponents/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/server/src/routes/api/compress/typings.tscomponents/webui/server/src/plugins/app/CompressionJobDbManager/typings.tscomponents/webui/client/src/pages/IngestPage/Compress/SubmitFormItem.tsxcomponents/webui/client/src/pages/IngestPage/Compress/index.tsxcomponents/webui/server/src/routes/api/compress/index.tscomponents/webui/server/src/plugins/app/CompressionJobDbManager/index.ts
🧠 Learnings (2)
📚 Learning: 2025-09-17T22:51:15.765Z
Learnt from: hoophalab
PR: y-scope/clp#1304
File: components/webui/client/src/sql-parser/index.ts:98-115
Timestamp: 2025-09-17T22:51:15.765Z
Learning: In the CLP webui codebase, when designing APIs that accept optional string parameters, developers should omit the value (pass undefined) rather than pass empty strings. The API contract should be clear that empty strings are not valid inputs - only undefined should be used to indicate omission of optional clauses.
Applied to files:
components/webui/server/src/routes/api/compress/index.ts
📚 Learning: 2025-09-15T22:20:40.750Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#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.ts
🧬 Code graph analysis (2)
components/webui/client/src/pages/IngestPage/Compress/index.tsx (5)
components/webui/client/src/api/compress/index.ts (1)
submitCompressionJob(23-23)components/webui/common/src/schemas/compression.ts (1)
CompressionJobCreation(33-33)components/webui/client/src/components/DashboardCard/index.tsx (1)
DashboardCard(57-57)components/webui/common/src/config.ts (1)
CLP_STORAGE_ENGINES(21-21)components/webui/client/src/config/index.ts (1)
SETTINGS_STORAGE_ENGINE(21-21)
components/webui/server/src/routes/api/compress/index.ts (5)
components/webui/server/src/plugins/app/CompressionJobDbManager/typings.ts (1)
CompressionJobConfig(18-18)components/webui/server/src/routes/api/compress/typings.ts (1)
CONTAINER_INPUT_LOGS_ROOT_DIR(7-7)components/webui/common/src/schemas/compression.ts (2)
CompressionJobCreationSchema(28-28)CompressionJobSchema(29-29)components/webui/common/src/schemas/error.ts (1)
ErrorSchema(8-8)components/webui/common/src/config.ts (1)
CLP_STORAGE_ENGINES(21-21)
⏰ 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). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (5)
components/webui/server/src/routes/api/compress/index.ts (1)
23-35: Sanity‑check defaults come from settings (not literals).Good move. Please ensure bounds validation elsewhere.
components/webui/client/src/pages/IngestPage/Compress/SubmitFormItem.tsx (1)
20-27: Disable the submit button while submitting.Prevents double-submits.
Apply:
<Button htmlType={"submit"} loading={isSubmitting} type={"primary"} + disabled={isSubmitting} > Submit </Button>Likely an incorrect or invalid review comment.
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
1-3: Import split OK — alias & types verified.No tsconfig "paths" or Vite alias present; client resolves @webui/common via package.json. CLP_QUERY_ENGINES is exported from components/webui/common/src/config.ts and SETTINGS_QUERY_ENGINE is cast to CLP_QUERY_ENGINES in components/webui/client/src/config/index.ts, so comparisons with CLP_QUERY_ENGINES.PRESTO are type-compatible. No changes required.
components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts (2)
72-90: Validation only runs for CLP engine; confirm intent.If CLP‑S or other engines also require non‑empty query strings, move the empty‑string check outside the engine gate (or add engine‑specific rules).
1-1: Approve — CLP_STORAGE_ENGINES exported; client resolves @webui/commonCLP_STORAGE_ENGINES is exported from components/webui/common/src/config.ts; client imports use @webui/common/* and @webui/common is declared in components/webui/client/package.json. No TS "paths" aliases or Jest moduleNameMapper entries were found — no config changes required.
| <Form.Item | ||
| label={"Dataset"} | ||
| name={"dataset"} | ||
| > | ||
| <Input | ||
| placeholder={"The dataset that the archives belong to (optional)"}/> | ||
| </Form.Item> | ||
| <Form.Item | ||
| label={"Timestamp Key"} | ||
| name={"timestampKey"} | ||
| > | ||
| <Input | ||
| placeholder={ | ||
| "The path for the field containing the log event's " + | ||
| "timestamp (optional)" | ||
| }/> | ||
| </Form.Item> |
There was a problem hiding this comment.
🧹 Nitpick
LGTM; add allowClear for minor UX polish.
Apply:
<Form.Item
label={"Dataset"}
name={"dataset"}
>
- <Input
+ <Input allowClear
placeholder={"The dataset that the archives belong to (optional)"}/>
</Form.Item>
<Form.Item
label={"Timestamp Key"}
name={"timestampKey"}
>
- <Input
+ <Input allowClear
placeholder={
"The path for the field containing the log event's " +
"timestamp (optional)"
}/>
</Form.Item>📝 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.
| <Form.Item | |
| label={"Dataset"} | |
| name={"dataset"} | |
| > | |
| <Input | |
| placeholder={"The dataset that the archives belong to (optional)"}/> | |
| </Form.Item> | |
| <Form.Item | |
| label={"Timestamp Key"} | |
| name={"timestampKey"} | |
| > | |
| <Input | |
| placeholder={ | |
| "The path for the field containing the log event's " + | |
| "timestamp (optional)" | |
| }/> | |
| </Form.Item> | |
| <Form.Item | |
| label={"Dataset"} | |
| name={"dataset"} | |
| > | |
| <Input allowClear | |
| placeholder={"The dataset that the archives belong to (optional)"}/> | |
| </Form.Item> | |
| <Form.Item | |
| label={"Timestamp Key"} | |
| name={"timestampKey"} | |
| > | |
| <Input allowClear | |
| placeholder={ | |
| "The path for the field containing the log event's " + | |
| "timestamp (optional)" | |
| }/> | |
| </Form.Item> |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx
around lines 14 to 30, the two Input fields for "Dataset" and "Timestamp Key"
should include the allowClear prop for improved UX; update each <Input> to pass
allowClear (boolean) so users can quickly clear the field (e.g., add allowClear
to both Input components' props), keeping existing placeholders and other props
unchanged.
| CLP_STORAGE_ENGINES, | ||
| SETTINGS_STORAGE_ENGINE, | ||
| } from "../../../config"; | ||
| import {SETTINGS_STORAGE_ENGINE} from "../../../config"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Mixed config sources; align types/values for engine comparison.
Confirm SETTINGS_STORAGE_ENGINE is one of Object.values(CLP_STORAGE_ENGINES); otherwise the equality check will silently fail. Consider consolidating SETTINGS_STORAGE_ENGINE into @webui/common/config or exporting a shared StorageEngine type from there to enforce compatibility.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate SETTINGS_STORAGE_ENGINE definition/initialization:"
rg -nC3 'SETTINGS_STORAGE_ENGINE' components/webui/client/src/config
echo; echo "Inspect CLP_STORAGE_ENGINES values:"
rg -nC3 'export\s+const\s+CLP_STORAGE_ENGINES' components/webui/common/src/config.tsLength of output: 1370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show client config ==="
sed -n '1,240p' components/webui/client/src/config/index.ts || true
echo
echo "=== Search repo for CLP_STORAGE_ENGINES occurrences ==="
rg -n --hidden --no-ignore -S 'CLP_STORAGE_ENGINES' -g '!*node_modules/*' -C2 || true
echo
echo "=== Look specifically for enum/const definitions for CLP_STORAGE_ENGINES ==="
rg -n --hidden --no-ignore -S 'export\s+(const|enum)\s+CLP_STORAGE_ENGINES|enum\s+CLP_STORAGE_ENGINES|CLP_STORAGE_ENGINES\s*=' -g '!*node_modules/*' -C2 || true
echo
echo "=== Search for CLP_QUERY_ENGINES occurrences ==="
rg -n --hidden --no-ignore -S 'CLP_QUERY_ENGINES' -g '!*node_modules/*' -C2 || true
echo
echo "=== Find where settings.ClpStorageEngine is defined/used ==="
rg -n --hidden --no-ignore -S 'ClpStorageEngine' -g '!*node_modules/*' -C2 || true
echo
echo "=== Print likely settings files (if present) ==="
sed -n '1,240p' components/webui/client/src/settings.ts || true
sed -n '1,240p' components/webui/client/src/settings/index.ts || true
sed -n '1,240p' components/webui/client/src/settings/index.tsx || true
echo
echo "=== Search common config directory for storage engine definitions ==="
rg -n --hidden --no-ignore -S 'CLP_STORAGE_ENGINES|CLP_QUERY_ENGINES' components/webui/common -g '!*node_modules/*' -C2 || trueLength of output: 2694
Ensure runtime value of SETTINGS_STORAGE_ENGINE matches CLP_STORAGE_ENGINES
settings.ClpStorageEngine is declared as string and loaded from settings.json but only type-asserted to CLP_STORAGE_ENGINES; the STREAM_TYPE equality can silently fail at runtime.
- Fix options: (A) export a shared StorageEngine enum/type from @webui/common/config and use it for Settings.ClpStorageEngine (compile-time safety), or (B) add a runtime check (e.g. if (!Object.values(CLP_STORAGE_ENGINES).includes(settings.ClpStorageEngine)) throw or fallback).
- Fix locations: components/webui/client/src/config/index.ts (STREAM_TYPE equality) and components/webui/client/src/settings.ts (Settings.ClpStorageEngine typing/loading).
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/search-requests.ts
around line 13, the import of SETTINGS_STORAGE_ENGINE highlights a runtime/type
mismatch: Settings.ClpStorageEngine is typed as string and only type-asserted to
CLP_STORAGE_ENGINES, so STREAM_TYPE equality can silently fail; update code to
ensure runtime safety by either exporting a shared StorageEngine enum/type from
@webui/common/config and using that type for Settings.ClpStorageEngine (change
components/webui/client/src/settings.ts to type the value as the shared enum and
ensure parsed settings.json is validated/typed), and update
components/webui/client/src/config/index.ts to import and compare against the
shared enum, OR add an explicit runtime validation where settings are loaded
(components/webui/client/src/settings.ts) that checks
Object.values(CLP_STORAGE_ENGINES).includes(settings.ClpStorageEngine) and
throws or falls back if invalid, and then adjust
components/webui/client/src/config/index.ts to rely on the validated value;
apply the change consistently in both mentioned files.
| @@ -0,0 +1,74 @@ | |||
| import {brotliCompressSync} from "node:zlib"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid blocking the event loop: switch to async Brotli.
Large configs can stall requests.
Apply:
-import {brotliCompressSync} from "node:zlib";
+import {brotliCompress} from "node:zlib";
+import {promisify} from "node:util";
+const brotliCompressAsync = promisify(brotliCompress);
@@
- const payload = brotliCompressSync(encode(jobConfig));
+ const payload = await brotliCompressAsync(encode(jobConfig));
- const [result] = await this.#sqlPool.execute<ResultSetHeader>(
+ const [result] = await this.#sqlPool.execute<ResultSetHeader>(
"INSERT INTO ?? ( ?? ) VALUES ( ? )",
[this.#tableName, COMPRESSION_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG, payload]
);Also applies to: 44-58
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts
(line 1 and also affecting lines 44-58), replace the synchronous
brotliCompressSync usage with the asynchronous Brotli API so large configs don't
block the event loop: import and use the async brotliCompress (e.g., from
node:zlib/promises or promisify node:zlib.brotliCompress), change the calling
function(s) to async, await the async compress call, propagate/return a Promise
instead of the sync result, and add proper try/catch to handle and log
compression errors without blocking.
| import {encode} from "@msgpack/msgpack"; | ||
| import {FastifyInstance} from "fastify"; | ||
| import fp from "fastify-plugin"; | ||
| import {ResultSetHeader} from "mysql2"; |
There was a problem hiding this comment.
🧹 Nitpick
Import ResultSetHeader as a type‑only import.
Apply:
-import {ResultSetHeader} from "mysql2";
+import type {ResultSetHeader} from "mysql2";📝 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.
| import {ResultSetHeader} from "mysql2"; | |
| import type {ResultSetHeader} from "mysql2"; |
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/app/CompressionJobDbManager/index.ts
around line 7, the import of ResultSetHeader from "mysql2" should be a type-only
import to avoid emitting runtime require for a type; change the import to a type
import (e.g., use "import type { ResultSetHeader } from 'mysql2';") on that line
and ensure any usages remain type annotations so the compiled output has no
runtime dependency.
| interface CompressionJobConfig { | ||
| input: { | ||
| paths_to_compress: string[]; | ||
| path_prefix_to_remove: string; | ||
| dataset?: string; | ||
| timestamp_key?: string; |
There was a problem hiding this comment.
🧹 Nitpick
Export the interface directly.
Simplifies imports and avoids split export.
Apply:
-interface CompressionJobConfig {
+export interface CompressionJobConfig {
@@
-}
-
-
-export type {CompressionJobConfig};
+}Also applies to: 18-18
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/app/CompressionJobDbManager/typings.ts
lines 1-6 (and also apply same change at line 18), the CompressionJobConfig
interface is declared but not exported directly; update the file to export the
interface where it's declared (e.g., use export interface CompressionJobConfig {
... }) and remove any separate later export so imports can reference it directly
from this file; apply the same direct export pattern to the other interface at
line 18.
| const CONTAINER_INPUT_LOGS_ROOT_DIR = "/mnt/logs"; | ||
|
|
||
|
|
||
| export {CONTAINER_INPUT_LOGS_ROOT_DIR}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make input-logs root configurable (settings/env) with safe fallback.
Hard-coding "/mnt/logs" invites drift; read from settings (or env) and fall back to the current default.
Apply:
+import settings from "../../../../settings.json" with {type: "json"};
/**
* Matching the `CONTAINER_INPUT_LOGS_ROOT_DIR` in `clp_package_utils.general`.
*/
-const CONTAINER_INPUT_LOGS_ROOT_DIR = "/mnt/logs";
+const CONTAINER_INPUT_LOGS_ROOT_DIR =
+ settings.ContainerInputLogsRootDir ?? process.env.CONTAINER_INPUT_LOGS_ROOT_DIR ?? "/mnt/logs";
export {CONTAINER_INPUT_LOGS_ROOT_DIR};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const CONTAINER_INPUT_LOGS_ROOT_DIR = "/mnt/logs"; | |
| export {CONTAINER_INPUT_LOGS_ROOT_DIR}; | |
| import settings from "../../../../settings.json" assert { type: "json" }; | |
| /** | |
| * Matching the `CONTAINER_INPUT_LOGS_ROOT_DIR` in `clp_package_utils.general`. | |
| */ | |
| const CONTAINER_INPUT_LOGS_ROOT_DIR = | |
| settings.ContainerInputLogsRootDir ?? process.env.CONTAINER_INPUT_LOGS_ROOT_DIR ?? "/mnt/logs"; | |
| export {CONTAINER_INPUT_LOGS_ROOT_DIR}; |
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/compress/typings.ts around lines 4 to
7, replace the hard-coded "/mnt/logs" with a value first read from an
environment variable (e.g. process.env.INPUT_LOGS_ROOT) or from the
application's settings/config helper, and if neither is present fall back to
"/mnt/logs"; validate/sanitize the resulting path (ensure it is a non-empty
string and normalized) before exporting it so downstream code gets a safe,
configurable CONTAINER_INPUT_LOGS_ROOT_DIR.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx (2)
28-31: Remove defaultValue; make field truly optional and add allowClear.Defaulting here prevents “left empty → default dataset” behaviour; move defaulting to the server and add clear affordance.
Apply:
- <Input - defaultValue={CLP_DEFAULT_DATASET_NAME} - placeholder={DATASET_HELPER_TEXT}/> + <Input + allowClear + placeholder={DATASET_HELPER_TEXT}/>
33-38: Add allowClear to Timestamp Key.Apply:
- <Input placeholder={TIMESTAMP_KEY_HELPER_TEXT}/> + <Input allowClear placeholder={TIMESTAMP_KEY_HELPER_TEXT}/>components/webui/common/src/config.ts (1)
10-16: Unify CLP variant literals to prevent drift across enums.Derive both enums from a single string‑literal source (keep enums if needed, but base values on shared const/type).
Example:
export type CLP_VARIANT = "clp" | "clp-s"; export const CLP_VARIANTS = { CLP: "clp", CLP_S: "clp-s" } as const; // enums can map to these if public API requires themAlso applies to: 1-8
components/webui/server/src/routes/api/compress/index.ts (3)
49-55: Declare 400 Bad Request in the route schema.Keeps OpenAPI in sync with new validation.
Apply:
schema: { body: CompressionJobCreationSchema, response: { [StatusCodes.CREATED]: CompressionJobSchema, + [StatusCodes.BAD_REQUEST]: ErrorSchema, [StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema, },
70-79: Trim inputs; default dataset only when empty; omit empty timestampKey.Prevents persisting empty strings and aligns with the API’s “omit rather than empty” convention.
Apply:
- if (CLP_STORAGE_ENGINES.CLP_S === settings.ClpStorageEngine as CLP_STORAGE_ENGINES) { - if ("string" !== typeof dataset || 0 === dataset.length) { - jobConfig.input.dataset = CLP_DEFAULT_DATASET_NAME; - } else { - jobConfig.input.dataset = dataset; - } - if ("undefined" !== typeof timestampKey) { - jobConfig.input.timestamp_key = timestampKey; - } - } + if (CLP_STORAGE_ENGINES.CLP_S === settings.ClpStorageEngine as CLP_STORAGE_ENGINES) { + const ds = "string" === typeof dataset ? dataset.trim() : ""; + jobConfig.input.dataset = 0 < ds.length ? ds : CLP_DEFAULT_DATASET_NAME; + if ("string" === typeof timestampKey) { + const tk = timestampKey.trim(); + if (0 < tk.length) { + jobConfig.input.timestamp_key = tk; + } + } + }
65-68: Fix path join and block traversal outside /mnt/logs.String concatenation is incorrect and unsafe. Use POSIX resolve, normalise leading slashes, and enforce the root prefix.
Apply:
+import path from "node:path"; @@ - const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG); - jobConfig.input.paths_to_compress = paths.map( - (path) => CONTAINER_INPUT_LOGS_ROOT_DIR + path - ); + const jobConfig: CompressionJobConfig = structuredClone(DEFAULT_COMPRESSION_JOB_CONFIG); + const root = CONTAINER_INPUT_LOGS_ROOT_DIR; + const absPaths = paths.map((p) => { + const rel = ("string" === typeof p ? p : String(p ?? "")).trim().replace(/^\/+/, ""); + return path.posix.resolve(root, rel); + }); + for (const abs of absPaths) { + if (false == (abs === root || abs.startsWith(root + "/"))) { + return reply.badRequest("Each path must resolve under /mnt/logs."); + } + } + jobConfig.input.paths_to_compress = absPaths;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx(1 hunks)components/webui/common/src/config.ts(1 hunks)components/webui/server/src/routes/api/compress/index.ts(1 hunks)
🧰 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/client/src/pages/IngestPage/Compress/ClpSFormItems.tsxcomponents/webui/server/src/routes/api/compress/index.tscomponents/webui/common/src/config.ts
🧠 Learnings (2)
📚 Learning: 2025-09-17T22:51:15.765Z
Learnt from: hoophalab
PR: y-scope/clp#1304
File: components/webui/client/src/sql-parser/index.ts:98-115
Timestamp: 2025-09-17T22:51:15.765Z
Learning: In the CLP webui codebase, when designing APIs that accept optional string parameters, developers should omit the value (pass undefined) rather than pass empty strings. The API contract should be clear that empty strings are not valid inputs - only undefined should be used to indicate omission of optional clauses.
Applied to files:
components/webui/server/src/routes/api/compress/index.ts
📚 Learning: 2025-09-15T22:20:40.750Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#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.ts
🧬 Code graph analysis (2)
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx (1)
components/webui/common/src/config.ts (1)
CLP_DEFAULT_DATASET_NAME(25-25)
components/webui/server/src/routes/api/compress/index.ts (6)
components/webui/server/src/plugins/app/CompressionJobDbManager/typings.ts (1)
CompressionJobConfig(18-18)components/webui/server/src/routes/api/compress/typings.ts (1)
CONTAINER_INPUT_LOGS_ROOT_DIR(7-7)components/webui/client/src/settings.ts (1)
settings(36-36)components/webui/common/src/schemas/compression.ts (2)
CompressionJobCreationSchema(28-28)CompressionJobSchema(29-29)components/webui/common/src/schemas/error.ts (1)
ErrorSchema(8-8)components/webui/common/src/config.ts (2)
CLP_STORAGE_ENGINES(27-27)CLP_DEFAULT_DATASET_NAME(25-25)
⏰ 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 (macos-15)
- GitHub Check: build (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (1)
components/webui/server/src/routes/api/compress/index.ts (1)
21-33: Minor: Object.freeze + structuredClone is OK; confirm Node runtime supports structuredClone.If older Node may run this, consider a shallow spread of a typed literal or a small factory.
Would you like a follow‑up diff to switch to a factory to avoid structuredClone?
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx
Outdated
Show resolved
Hide resolved
| <div className={styles["ingestPageGrid"]}> | ||
| <SpaceSavings/> | ||
| <Details/> | ||
| <div className={styles["jobsGrid"] || ""}> |
There was a problem hiding this comment.
want to rename this style just fullRow?
| "SqlDbPort": 3306, | ||
| "SqlDbName": "clp-db", | ||
| "SqlDbQueryJobsTableName": "query_jobs", | ||
| "SqlDbCompressionJobsTableName": "compression_jobs", |
There was a problem hiding this comment.
i think u might need to add this SqlDbCompressionJobsTableName .py file, like its not set
There was a problem hiding this comment.
sorry i'm not sure if i get this. do you mean this value should be updated in start_clp.py?
if so, it's already done at
There was a problem hiding this comment.
i think that is in the client not server
There was a problem hiding this comment.
right. sorry for missing this
| * | ||
| * @enum {number} | ||
| */ | ||
| enum COMPRESSION_JOB_STATUS { |
There was a problem hiding this comment.
Not sure this is being used atm
There was a problem hiding this comment.
not in this PR. sorry
| mutationFn: submitCompressionJob, | ||
| onSettled: async () => { | ||
| // Invalidate queries that are affected by a new compression job. | ||
| await queryClient.invalidateQueries({queryKey: ["jobs"]}); |
There was a problem hiding this comment.
any reason not to put this in the success handler?
There was a problem hiding this comment.
while we don't expect contents in the job table to change if a row cannot be inserted into the table, i think we should always refresh the job table so it's less error prone and future proof.
for example, if the INSERT query submitted by CompressionJobDbManager was executed but the SQL connection drops right after, mysql2 can still throw and Fastify will report an error status.
| err.message : | ||
| "Unknown error"; | ||
|
|
||
| messageApi.error(`Failed to submit compression job: ${errorMessage}`); |
There was a problem hiding this comment.
i dont think we need this and the rendered error below. Like i think we can use this for error and success, or use the rendered option, but not both
There was a problem hiding this comment.
good point. let me remove this messageApi.error notification then
|
|
||
| const DATASET_HELPER_TEXT = "The dataset that the archives belong to. If left empty, dataset " + | ||
| "\"default\" is used."; | ||
| const TIMESTAMP_KEY_HELPER_TEXT = "The path (e.g. x.y) for the field containing the log event's" + |
There was a problem hiding this comment.
i think we are using this wording in the log viewer
Key that maps to each log event's timestamp
There was a problem hiding this comment.
ah, this was copied from the cli script:
clp/components/clp-package-utils/clp_package_utils/scripts/compress.py
Lines 160 to 163 in ddfcc5f
if you don't feel strongly, i prefer keep the message in sync with the cli script
| tooltip={DATASET_HELPER_TEXT} | ||
| > | ||
| <Input | ||
| defaultValue={CLP_DEFAULT_DATASET_NAME} |
There was a problem hiding this comment.
i feel we dont need the default here, since it is mentioned in the info comment. And we just can put this
The dataset for new archives as a placeholder
| <Form.Item | ||
| label={"Timestamp Key"} | ||
| name={"timestampKey"} | ||
| tooltip={`${TIMESTAMP_KEY_HELPER_TEXT} ${TIMESTAMP_KEY_HELPER_TEXT_EXTRA}`} |
There was a problem hiding this comment.
I dont think we need both for the tooltip, since the first is already in the place holder. like extra is fine
| } from "antd"; | ||
|
|
||
|
|
||
| const DATASET_HELPER_TEXT = "The dataset that the archives belong to. If left empty, dataset " + |
There was a problem hiding this comment.
I think u can remove the first part "The dataset that the archives belong to. since this is already in placeholder per my other comment
| const TIMESTAMP_KEY_HELPER_TEXT = "If not provided, events will not have assigned" + | ||
| " timestamps and can only be searched from the command line without a timestamp filter."; | ||
| const TIMESTAMP_KEY_PLACEHOLDER_TEXT = | ||
| "The path (e.g. x.y) for the field containing the log event's timestamp"; |
There was a problem hiding this comment.
putting this on a new line; otherwise the string has to be broken into two to avoid line length violations
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx(1 hunks)components/webui/client/src/pages/IngestPage/Compress/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx(1 hunks)components/webui/server/src/typings/compression.ts(1 hunks)
🧰 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/typings/compression.tscomponents/webui/client/src/pages/IngestPage/Compress/index.tsxcomponents/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx
🧠 Learnings (2)
📚 Learning: 2025-01-16T16:58:43.190Z
Learnt from: haiqi96
PR: y-scope/clp#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/Compress/ClpSFormItems.tsx
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx
🧬 Code graph analysis (2)
components/webui/client/src/pages/IngestPage/Compress/index.tsx (5)
components/webui/client/src/api/compress/index.ts (1)
submitCompressionJob(23-23)components/webui/common/src/schemas/compression.ts (1)
CompressionJobCreation(33-33)components/webui/client/src/components/DashboardCard/index.tsx (1)
DashboardCard(57-57)components/webui/common/src/config.ts (1)
CLP_STORAGE_ENGINES(27-27)components/webui/client/src/config/index.ts (1)
SETTINGS_STORAGE_ENGINE(21-21)
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx (1)
components/webui/common/src/config.ts (1)
CLP_DEFAULT_DATASET_NAME(25-25)
⏰ 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). (1)
- GitHub Check: package-image
🔇 Additional comments (9)
components/webui/client/src/pages/IngestPage/Compress/ClpSFormItems.tsx (2)
8-8: Good: helper text derives default from shared constant.Prevents drift if the default changes elsewhere.
27-27: Add allowClear to both Inputs for better UX.Quick clearing helps when testing and iterating.
Apply:
- <Input placeholder={DATASET_PLACEHOLDER_TEXT}/> + <Input allowClear placeholder={DATASET_PLACEHOLDER_TEXT}/>- <Input placeholder={TIMESTAMP_KEY_PLACEHOLDER_TEXT}/> + <Input allowClear placeholder={TIMESTAMP_KEY_PLACEHOLDER_TEXT}/>Also applies to: 34-34
components/webui/client/src/pages/IngestPage/Compress/index.tsx (5)
59-63: Handle CRLF when splitting lines.Be robust across platforms.
Apply:
- const paths = values.paths - .split("\n") + const paths = values.paths + .split(/\r?\n/) .map((path) => path.trim()) .filter((path) => 0 < path.length);
64-64: De‑duplicate paths before submit.Avoid redundant server work.
Apply:
- const payload: CompressionJobCreation = {paths}; + const payload: CompressionJobCreation = {paths: Array.from(new Set(paths))};
66-71: Send optional fields only when non‑empty and trim them.Let server defaults apply; avoid sending "".
Apply:
- if ("undefined" !== typeof values.dataset) { - payload.dataset = values.dataset; - } - if ("undefined" !== typeof values.timestampKey) { - payload.timestampKey = values.timestampKey; - } + if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) { + payload.dataset = values.dataset.trim(); + } + if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) { + payload.timestampKey = values.timestampKey.trim(); + }
77-77: Rename card title to match the feature.“Start Ingestion” is misleading here.
Apply:
- <DashboardCard title={"Start Ingestion"}> + <DashboardCard title={"Submit Compression Job"}>
6-6: Use a type‑only import for CompressionJobCreation.Avoids emitting a runtime import for a type.
Apply:
-import {CompressionJobCreation} from "@webui/common/schemas/compression"; +import type {CompressionJobCreation} from "@webui/common/schemas/compression";components/webui/server/src/typings/compression.ts (1)
6-10: LGTM — centralised DB column names; verify schema & runtime typeCentral source of truth is good; I couldn't locate the DB DDL or usages in the repo — confirm clp_config is defined as a binary type (e.g., BLOB) in the DB DDL and that downstream code reads/writes it as a Buffer.
components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx (1)
4-4: Centralized import confirmed — LGTM.
All occurrences import CLP_STORAGE_ENGINES from @webui/common/config and the shared export exists at components/webui/common/src/config.ts.
| CLP_STORAGE_ENGINES, | ||
| SETTINGS_STORAGE_ENGINE, | ||
| } from "../../../../../config"; | ||
| import {SETTINGS_STORAGE_ENGINE} from "../../../../../config"; |
There was a problem hiding this comment.
🧹 Nitpick
Avoid split config sources (consider co-locating).
If @webui/common/config also exports SETTINGS_STORAGE_ENGINE, import both from there to prevent drift between modules; otherwise, consider re-exporting to keep a single source of truth.
Can you confirm whether SETTINGS_STORAGE_ENGINE is (or can be) exported from @webui/common/config as part of this consolidation?
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton/index.tsx
around line 10, the module imports SETTINGS_STORAGE_ENGINE from a relative
config path which risks config drift; update the import to come from
@webui/common/config (or, if that package doesn't currently export
SETTINGS_STORAGE_ENGINE, add a re-export there so it becomes the single source
of truth), then update this file to import SETTINGS_STORAGE_ENGINE from
@webui/common/config and remove the relative import; verify that the constant is
exported from @webui/common/config and adjust its export if necessary so all
modules can import the same symbol.
| enum COMPRESSION_JOBS_TABLE_COLUMN_NAMES { | ||
| ID = "id", | ||
| STATUS = "status", | ||
| JOB_CONFIG = "clp_config", | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Avoid runtime enum footprint: use const enum or a frozen as-const object.
This prevents emitting a JS enum object and can reduce bundle size.
Option A — const enum (ensure tsconfig/build supports const enums):
-enum COMPRESSION_JOBS_TABLE_COLUMN_NAMES {
+const enum COMPRESSION_JOBS_TABLE_COLUMN_NAMES {
ID = "id",
STATUS = "status",
JOB_CONFIG = "clp_config",
}Option B — frozen object + literal type:
-enum COMPRESSION_JOBS_TABLE_COLUMN_NAMES {
- ID = "id",
- STATUS = "status",
- JOB_CONFIG = "clp_config",
-}
+const COMPRESSION_JOBS_TABLE_COLUMN_NAMES = Object.freeze({
+ ID: "id",
+ STATUS: "status",
+ JOB_CONFIG: "clp_config",
+} as const);
+
+type CompressionJobColumnName =
+ typeof COMPRESSION_JOBS_TABLE_COLUMN_NAMES[keyof typeof COMPRESSION_JOBS_TABLE_COLUMN_NAMES];
-export {COMPRESSION_JOBS_TABLE_COLUMN_NAMES};
+export { COMPRESSION_JOBS_TABLE_COLUMN_NAMES };
+export type { CompressionJobColumnName };📝 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.
| enum COMPRESSION_JOBS_TABLE_COLUMN_NAMES { | |
| ID = "id", | |
| STATUS = "status", | |
| JOB_CONFIG = "clp_config", | |
| } | |
| const COMPRESSION_JOBS_TABLE_COLUMN_NAMES = Object.freeze({ | |
| ID: "id", | |
| STATUS: "status", | |
| JOB_CONFIG: "clp_config", | |
| } as const); | |
| type CompressionJobColumnName = | |
| typeof COMPRESSION_JOBS_TABLE_COLUMN_NAMES[keyof typeof COMPRESSION_JOBS_TABLE_COLUMN_NAMES]; | |
| export { COMPRESSION_JOBS_TABLE_COLUMN_NAMES }; | |
| export type { CompressionJobColumnName }; |
| enum COMPRESSION_JOBS_TABLE_COLUMN_NAMES { | ||
| ID = "id", | ||
| STATUS = "status", | ||
| JOB_CONFIG = "clp_config", |
There was a problem hiding this comment.
🧹 Nitpick
Clarify payload semantics for JOB_CONFIG.
Add a short doc noting this is a binary-encoded payload (e.g., msgpack+Brotli) to avoid misuse as string.
enum COMPRESSION_JOBS_TABLE_COLUMN_NAMES {
ID = "id",
STATUS = "status",
- JOB_CONFIG = "clp_config",
+ /** msgpack+Brotli-encoded job config (BLOB/Buffer) */
+ JOB_CONFIG = "clp_config",
}📝 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.
| JOB_CONFIG = "clp_config", | |
| enum COMPRESSION_JOBS_TABLE_COLUMN_NAMES { | |
| ID = "id", | |
| STATUS = "status", | |
| /** msgpack+Brotli-encoded job config (BLOB/Buffer) */ | |
| JOB_CONFIG = "clp_config", | |
| } |
🤖 Prompt for AI Agents
In components/webui/server/src/typings/compression.ts around line 9, the
JOB_CONFIG enum member needs a short inline documentation comment clarifying
that its payload is a binary‑encoded blob (e.g., msgpack serialized then Brotli
compressed) and must not be treated as a plain string; add a JSDoc or
single‑line comment directly above the JOB_CONFIG entry stating the exact
encoding (msgpack + Brotli), the expected type for consumers
(Uint8Array/Buffer), and that callers must decode/decompress before parsing so
it isn't misused as text.
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/src/pages/IngestPage/Compress/index.tsx(1 hunks)
🧰 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/client/src/pages/IngestPage/Compress/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/IngestPage/Compress/index.tsx (5)
components/webui/client/src/api/compress/index.ts (1)
submitCompressionJob(23-23)components/webui/common/src/schemas/compression.ts (1)
CompressionJobCreation(33-33)components/webui/client/src/components/DashboardCard/index.tsx (1)
DashboardCard(57-57)components/webui/common/src/config.ts (1)
CLP_STORAGE_ENGINES(27-27)components/webui/client/src/config/index.ts (1)
SETTINGS_STORAGE_ENGINE(21-21)
⏰ 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). (1)
- GitHub Check: package-image
🔇 Additional comments (7)
components/webui/client/src/pages/IngestPage/Compress/index.tsx (7)
82-82: Ensure PathsInputFormItem enforces at least one trimmed, non‑empty path.If not already inside this component, add a custom validator that trims/splits by newline and rejects when the resulting array is empty.
#!/bin/bash # Verify validation inside PathsInputFormItem fd -a 'PathsInputFormItem.tsx' sed -n '1,200p' components/webui/client/src/pages/IngestPage/Compress/PathsInputFormItem.tsx 2>/dev/null || true rg -n -C2 -S "rules|validator|paths" components/webui/client/src/pages/IngestPage/Compress/PathsInputFormItem.tsx 2>/dev/null || true
58-61: Split lines with CRLF support.Safer across platforms.
- const paths = values.paths - .split("\n") - .map((path) => path.trim()) - .filter((path) => 0 < path.length); + const paths = values.paths + .split(/\r?\n/) + .map((path) => path.trim()) + .filter((path) => 0 < path.length);
63-63: Optional: de‑duplicate paths before submit.Avoids redundant server work.
- const payload: CompressionJobCreation = {paths}; + const payload: CompressionJobCreation = {paths: Array.from(new Set(paths))};
65-70: Only send optional fields when non‑empty (trimmed).Let server defaults apply and avoid sending empty strings.
- if ("undefined" !== typeof values.dataset) { - payload.dataset = values.dataset; - } - if ("undefined" !== typeof values.timestampKey) { - payload.timestampKey = values.timestampKey; - } + if ("string" === typeof values.dataset && 0 < values.dataset.trim().length) { + payload.dataset = values.dataset.trim(); + } + if ("string" === typeof values.timestampKey && 0 < values.timestampKey.trim().length) { + payload.timestampKey = values.timestampKey.trim(); + }
76-76: Rename card title to match the feature.Clearer for users.
- <DashboardCard title={"Start Ingestion"}> + <DashboardCard title={"Submit Compression Job"}>
6-6: Use a type‑only import for CompressionJobCreation.Prevents emitting a runtime import.
-import {CompressionJobCreation} from "@webui/common/schemas/compression"; +import type {CompressionJobCreation} from "@webui/common/schemas/compression";
77-81: Optional: disable the whole form while submitting.Improves UX by preventing edits mid‑submit.
<Form form={form} layout={"vertical"} + disabled={isSubmitting} onFinish={handleSubmit} >
| } = useMutation({ | ||
| mutationFn: submitCompressionJob, | ||
| onSettled: async () => { | ||
| // Invalidate queries that are affected by a new compression job. | ||
| await queryClient.invalidateQueries({queryKey: ["jobs"]}); | ||
| }, | ||
| onSuccess: () => { | ||
| form.resetFields(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Optional: add explicit generics to useMutation for stronger typing.
Keeps the mutation’s data/error/variables types explicit.
- } = useMutation({
+ } = useMutation<{ jobId: string }, Error, CompressionJobCreation>({
mutationFn: submitCompressionJob,
onSettled: async () => {
// Invalidate queries that are affected by a new compression job.
await queryClient.invalidateQueries({queryKey: ["jobs"]});
},
onSuccess: () => {
form.resetFields();
},
});Committable suggestion skipped: line range outside the PR's diff.
@coderabbitai create an issue to track the feature request of displaying the error message in the "Ingestion jobs" table. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@junhaoliao I've created a GitHub issue to track the feature request for displaying error messages as tooltips on error icons in the "Ingestion jobs" table. The issue includes the context from this PR discussion and has been assigned to you. 🏁 Script executed: Length of output: 42 @junhaoliao I've successfully created GitHub issue #1325 to track the feature request for displaying error messages as tooltips on error icons in the "Ingestion jobs" table. The issue includes:
You can view the issue at: #1325 |
| "SqlDbPort": 3306, | ||
| "SqlDbName": "clp-db", | ||
| "SqlDbQueryJobsTableName": "query_jobs", | ||
| "SqlDbCompressionJobsTableName": "compression_jobs", |
There was a problem hiding this comment.
i think that is in the client not server
| /** | ||
| * The `compression_jobs` table's column names. | ||
| * | ||
| * @enum {string} |
There was a problem hiding this comment.
im not sure we are using this @enum syntax elsewhere. do u want to keep it?
There was a problem hiding this comment.
there are a few in components/log-viewer-webui/server/src/typings/query.ts
I think both keeping and removing it could make sense. TypeScript should be smart enough to deduct it's one way or the other though. For consistency, maybe we should remove them all together.
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/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/start_clp.py
⏰ 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). (1)
- GitHub Check: package-image
🔇 Additional comments (1)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
883-883: Resolved — SqlDbCompressionJobsTableName is defined in settingsFound in components/webui/server/settings.json, components/webui/client/public/settings.json and components/webui/client/src/settings.ts; CompressionJobDbManager references settings.SqlDbCompressionJobsTableName, so no change required.
| "ArchiveOutputCompressionLevel": clp_config.archive_output.compression_level, | ||
| "ArchiveOutputTargetArchiveSize": clp_config.archive_output.target_archive_size, | ||
| "ArchiveOutputTargetDictionariesSize": clp_config.archive_output.target_dictionaries_size, | ||
| "ArchiveOutputTargetEncodedFileSize": clp_config.archive_output.target_encoded_file_size, | ||
| "ArchiveOutputTargetSegmentSize": clp_config.archive_output.target_segment_size, | ||
| "ClpQueryEngine": clp_config.package.query_engine, | ||
| "ClpStorageEngine": clp_config.package.storage_engine, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
🧩 Analysis chain
New archive-output + storage-engine keys: verify server settings support; consider container_clp_config for consistency.
- Verify all new keys exist in server settings templates/schemas; otherwise update_settings_object will raise.
- Optional: Use container_clp_config for symmetry with StreamTargetUncompressedSize.
Apply:
- "ArchiveOutputCompressionLevel": clp_config.archive_output.compression_level,
- "ArchiveOutputTargetArchiveSize": clp_config.archive_output.target_archive_size,
- "ArchiveOutputTargetDictionariesSize": clp_config.archive_output.target_dictionaries_size,
- "ArchiveOutputTargetEncodedFileSize": clp_config.archive_output.target_encoded_file_size,
- "ArchiveOutputTargetSegmentSize": clp_config.archive_output.target_segment_size,
+ "ArchiveOutputCompressionLevel": container_clp_config.archive_output.compression_level,
+ "ArchiveOutputTargetArchiveSize": container_clp_config.archive_output.target_archive_size,
+ "ArchiveOutputTargetDictionariesSize": container_clp_config.archive_output.target_dictionaries_size,
+ "ArchiveOutputTargetEncodedFileSize": container_clp_config.archive_output.target_encoded_file_size,
+ "ArchiveOutputTargetSegmentSize": container_clp_config.archive_output.target_segment_size,
- "ClpStorageEngine": clp_config.package.storage_engine,
+ "ClpStorageEngine": container_clp_config.package.storage_engine,Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check presence of the new keys in server settings/templates/schemas
rg -n -C2 -e 'ArchiveOutputCompressionLevel' \
-e 'ArchiveOutputTargetArchiveSize' \
-e 'ArchiveOutputTargetDictionariesSize' \
-e 'ArchiveOutputTargetEncodedFileSize' \
-e 'ArchiveOutputTargetSegmentSize' \
-e 'ClpStorageEngine' \
components/webuiLength of output: 3455
Verified: ArchiveOutput keys + ClpStorageEngine exist; optional refactor available
- Keys found in components/webui/server/settings.json and client/public/settings.json; server references in components/webui/server/src/routes/api/compress/index.ts and client config in components/webui/client/src/config/index.ts — update_settings_object should not raise for missing keys.
- Optional: for symmetry with StreamTargetUncompressedSize, consider switching clp_config → container_clp_config in components/clp-package-utils/clp_package_utils/scripts/start_clp.py (lines 892–899).
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/start_clp.py around
lines 892 to 899, the settings object currently uses clp_config and contains
ArchiveOutput* and ClpStorageEngine keys that are present in server and client
configs so update_settings_object should not error; ensure you do not remove or
rename these keys and avoid raising for missing keys, and optionally rename the
reference from clp_config to container_clp_config for symmetry with
StreamTargetUncompressedSize (update all usages in this block accordingly).
i was planning to merge #1178 , where the limitation is lifted, before this PR, so we don't have to add the handling in this PR and remove it later. let me discuss in the cloud sync up to see if the priorities should be shifted |
3 participants
Description
Checklist
breaking change.
Validation performed
package.stoarge_engineset toclpin etc/clp-config.yml , tested submission with multiple text logs directories in the webui.package.stoarge_engineset toclp-sin etc/clp-config.yml , tested submission with multiple JSON logs file paths in the webui. tried leaving the "dataset" as empty and the file gets compressed into "default" dataset.Summary by CodeRabbit
New Features
Chores