feat(webui): Migrate to npm workspace; Update taskfile and the package scripts accordingly. #1119
Conversation
WalkthroughAdds a new workspace package Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Repo as Repository
participant Common as @webui/common
participant Client as @webui/client
participant Server as @webui/server
participant Launcher as start_clp.py
Note over Dev,Repo: add common workspace, migrate type imports, update manifests, packaging, and launcher paths
Dev->>Repo: add components/webui/common (package, tsconfig, eslint, src)
Dev->>Client: update manifest, remove TS/Vite @common alias, change type imports -> @webui/common
Dev->>Server: update manifest/scripts to dist/src/*, remove TS paths, change imports -> @webui/common
Client->>Common: import types/schemas (ClientToServerEvents, Presto types, SEARCH_SIGNAL)
Server->>Common: import signals/types (SEARCH_SIGNAL, QueryId, engine constants)
Launcher->>Server: execute corrected node command: node dist/src/main.js
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-19T18:14:01.607ZApplied to files:
📚 Learning: 2025-08-14T18:23:39.476ZApplied 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). (14)
🔇 Additional comments (10)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
davemarco
changed the title
| "common", | ||
| "client", | ||
| "server" |
There was a problem hiding this comment.
would it be better to put those under a packages/ directory? this seems to be the recommendation in the npm docs and a commonly used conventions in many projects.
| "common", | |
| "client", | |
| "server" | |
| "packages/common", | |
| "packages/client", | |
| "packages/server" |
There was a problem hiding this comment.
I think this is a preference, i dont mind changing but maybe in a different PR so we dont need to modify the taskfile that much in this pr
There was a problem hiding this comment.
if we specify a components/webui/.npmrc file with
install-strategy = "nested"
("linked" might also work just fine though the command like to give a warning to say it might have bugs)
we can remove the init script in this file. any npm command like npm install and npm clean-install should install the node_modules inside the packages' own directory.
most importantly, the webui node_modules install task in taskfile.yaml will be much simplified
There was a problem hiding this comment.
I tried this and it dosent seem to actually install the node_modules in the packages directory.
There was a problem hiding this comment.
to get this install-strategy config into effect, we need to clean the existing node_modules and package-lock.json
There was a problem hiding this comment.
do you know if this will keep nested package.lock? it if it dosent we still run into the same problem where we only have one lock file
There was a problem hiding this comment.
i remember individual packages will have their own locks. this is fine to skip for now given the current structure already works
There was a problem hiding this comment.
these were not neccesary
|
@junhaoliao - there was an issue with the linter script due to workspaces, i believe i just fixed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/webui/package.json (2)
5-9: Workspaces list looks fine; note on directory convention (already discussed).The explicit list is OK. Prior feedback about moving under packages/ remains a style preference and can be deferred as agreed.
11-11: Prefer deterministic installs with npm ci.Replace clean-install with ci for lockfile-only reproducible installs across workspaces.
Apply this diff:
- "init": "npm clean-install --workspaces", + "init": "npm ci --workspaces",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/webui/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T18:14:01.607Z
Learnt from: davemarco
PR: y-scope/clp#1119
File: taskfile.yaml:302-305
Timestamp: 2025-08-19T18:14:01.607Z
Learning: npm ci in workspaces can work without physical package.json files in all workspace directories if the workspace information is properly defined in package-lock.json. Built static assets (like from Vite builds) don't require runtime package.json files since they're self-contained. npm ci uses the lockfile for dependency resolution, not individual workspace manifests.
Applied to files:
components/webui/package.json
🔇 Additional comments (1)
components/webui/package.json (1)
14-14: Update root “start” script to invoke workspace start commandsThe top-level “client:start” and “server:start” composites were removed—each package now exposes its own
startscript. We’ve verified that:
- @webui/client defines
start: “vite”- @webui/server defines
start: “npm run build && fastify start -l info dist/src/app.js”Adjust the root
components/webui/package.jsonaccordingly:--- a/components/webui/package.json +++ b/components/webui/package.json @@ lines 14 - "start": "concurrently \"npm run client:start\" \"npm run server:start\"" + "start": "concurrently \"npm --workspace @webui/client run start\" \"npm --workspace @webui/server run start\""This ensures
npm startcorrectly spins up both client and server via their workspace scripts.
| "name": "webui", | ||
| "version": "0.1.0", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: align root package name with the scope used elsewhere.
Consider renaming to @webui/webui for consistency with @webui/client and @webui/server. Non-blocking.
🤖 Prompt for AI Agents
In components/webui/package.json around lines 2 to 3, the package name "webui"
is inconsistent with the scoped names used elsewhere; change the "name" field to
"@webui/webui" and update any local references (workspaces, import paths,
README, CI/publish config) to the new scoped name so tooling and dependency
resolution remain correct; ensure package.json's other fields
(private/publishConfig) are adjusted if necessary for publishing under the
scope.
components/webui/package.json
Outdated
| "lint:check": "npm --workspace common run build && npm run lint:check --workspaces", | ||
| "lint:fix": "npm --workspace common run build && npm run lint:fix --workspaces", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Harden lint scripts and address coupling to folder name.
- Use workspace selector by package name (stable) instead of folder path "common".
- Add --if-present so packages without the script don’t error.
Apply this diff:
- "lint:check": "npm --workspace common run build && npm run lint:check --workspaces",
- "lint:fix": "npm --workspace common run build && npm run lint:fix --workspaces",
+ "lint:check": "npm --workspace @webui/common run build && npm run --workspaces --if-present lint:check",
+ "lint:fix": "npm --workspace @webui/common run build && npm run --workspaces --if-present lint:fix",📝 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.
| "lint:check": "npm --workspace common run build && npm run lint:check --workspaces", | |
| "lint:fix": "npm --workspace common run build && npm run lint:fix --workspaces", | |
| "lint:check": "npm --workspace @webui/common run build && npm run --workspaces --if-present lint:check", | |
| "lint:fix": "npm --workspace @webui/common run build && npm run --workspaces --if-present lint:fix", |
🤖 Prompt for AI Agents
In components/webui/package.json around lines 12 to 13, the lint scripts are
hard-coded to the workspace folder name "common" and don't use --if-present;
change them to target the workspace by package name and add --if-present so
missing scripts won't fail. Replace "npm --workspace common run build" with "npm
--workspace=\"<package-name>\" --if-present run build" and append "--if-present"
to the downstream npm run calls (e.g. "npm run lint:check --workspaces
--if-present" and "npm run lint:fix --workspaces --if-present"); set
<package-name> to the actual package name for the common package as declared in
the root package.json.
components/webui/package.json
Outdated
| "init": "npm clean-install --workspaces", | ||
| "lint:check": "npm --workspace common run build && npm run lint:check --workspaces", | ||
| "lint:fix": "npm --workspace common run build && npm run lint:fix --workspaces", | ||
| "start": "concurrently \"npm run client:start\" \"npm run server:start\"" |
There was a problem hiding this comment.
let's fix this broken script
i believe a more elegant way is possible once we unify the dev script names, for now this
| "start": "concurrently \"npm run client:start\" \"npm run server:start\"" | |
| "start": "concurrently \"npm run start:*\"", | |
| "start:client": "npm --workspace client run start", | |
| "start:common": "npm --workspace common run build:watch", | |
| "start:server": "npm --workspace server run dev" |
also we should update the concurrently version to match the server's version (see below). otherwise, concurrently won't be installed due to version conflict.
"concurrently": "^9.1.2"
then
rm -rf node_modules package-lock.json
npm i --workspaces
There was a problem hiding this comment.
i used dev instead since i think better than start. I dont think i need to remove, i think i can just write npm i --save-dev [email protected]
davemarco
changed the title
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
components/webui/client/tsconfig/tsconfig.app.json (1)
4-4: Include glob looks correct; resolves prior ask.Using "../src/**/*" is the right scope for the app TS inputs and aligns with removing the old @common alias.
components/webui/package.json (2)
14-14: Start script is broken; invoke package starts via workspace selectors.Root no longer has client:start/server:start. Call each package’s start directly.
- "start": "concurrently \"npm run client:start\" \"npm run server:start\"" + "start": "concurrently \"npm --workspace @webui/client run start\" \"npm --workspace @webui/server run start\""
5-9: Optional: pin concurrently to match other packages and avoid version conflicts.If server/common use concurrently 9.x, align here to prevent dedupe churn. Non-blocking.
- "concurrently": "^8.2.2" + "concurrently": "^9.1.2"components/webui/client/package.json (2)
2-2: Mark app package as private to prevent accidental publish.{ - "name": "@webui/client", + "name": "@webui/client", + "private": true,
24-24: Replace wildcard on @webui/common with a pinned workspace version."*" harms reproducibility and can pull a registry artefact if it ever exists. Pin to common’s version.
- "@webui/common": "*", + "@webui/common": "^0.1.0",Adjust the version to match components/webui/common/package.json if different.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
components/webui/client/package.json(3 hunks)components/webui/client/tsconfig/tsconfig.app.json(1 hunks)components/webui/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T18:14:01.607Z
Learnt from: davemarco
PR: y-scope/clp#1119
File: taskfile.yaml:302-305
Timestamp: 2025-08-19T18:14:01.607Z
Learning: npm ci in workspaces can work without physical package.json files in all workspace directories if the workspace information is properly defined in package-lock.json. Built static assets (like from Vite builds) don't require runtime package.json files since they're self-contained. npm ci uses the lockfile for dependency resolution, not individual workspace manifests.
Applied to files:
components/webui/package.json
🔇 Additional comments (2)
components/webui/client/package.json (2)
21-21: Good: synced @sinclair/typebox with common.
46-47: Tooling versions look stable and aligned.
| "init": "npm clean-install --workspaces && npm --workspace common run build", | ||
| "lint:check": "npm run lint:check --workspaces", | ||
| "lint:fix": "npm run lint:fix --workspaces", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer lockfile-only installs and target the workspace by package name.
Use npm ci for reproducible installs and refer to @webui/common by package name (not folder).
- "init": "npm clean-install --workspaces && npm --workspace common run build",
+ "init": "npm ci --workspaces && npm --workspace @webui/common run build",📝 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.
| "init": "npm clean-install --workspaces && npm --workspace common run build", | |
| "lint:check": "npm run lint:check --workspaces", | |
| "lint:fix": "npm run lint:fix --workspaces", | |
| // components/webui/package.json | |
| { | |
| // … | |
| "scripts": { | |
| "init": "npm ci --workspaces && npm --workspace @webui/common run build", | |
| "lint:check": "npm run lint:check --workspaces", | |
| "lint:fix": "npm run lint:fix --workspaces", | |
| // … | |
| } | |
| // … | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/webui/package.json (3)
5-9: Workspace layout: prior suggestion still stands (packages/ or globs).Using folder names is fine, but adopting a packages/ directory and/or a glob (e.g., "packages/*") is the common convention and scales better. Non-blocking.
11-11: Use lockfile-only installs and target the workspace by package name.Deterministic installs via npm ci; address coupling to folder name.
- "init": "npm clean-install --workspaces && npm --workspace common run build", + "init": "npm ci --workspaces && npm --workspace @webui/common run build",Note: Based on the retrieved learnings for this repo, npm ci works well with workspaces and lockfile-driven resolution.
14-14: Fix start script to actually target workspace packages.client:start/server:start aren’t root scripts; current command will fail. Use npm’s workspace selector.
- "start": "concurrently \"npm run client:start\" \"npm run server:start\"" + "start": "concurrently \"npm -w @webui/client run start\" \"npm -w @webui/server run start\""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
components/webui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
components/webui/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T18:14:01.607Z
Learnt from: davemarco
PR: y-scope/clp#1119
File: taskfile.yaml:302-305
Timestamp: 2025-08-19T18:14:01.607Z
Learning: npm ci in workspaces can work without physical package.json files in all workspace directories if the workspace information is properly defined in package-lock.json. Built static assets (like from Vite builds) don't require runtime package.json files since they're self-contained. npm ci uses the lockfile for dependency resolution, not individual workspace manifests.
Applied to files:
components/webui/package.json
🔇 Additional comments (1)
components/webui/package.json (1)
19-19: concurrently version bump aligns with server; LGTM.
| "lint:check": "npm run lint:check --workspaces", | ||
| "lint:fix": "npm run lint:fix --workspaces", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Harden lint scripts; add --if-present and correct flag placement.
Prevents failures when some packages lack lint scripts and uses the supported flag order.
- "lint:check": "npm run lint:check --workspaces",
- "lint:fix": "npm run lint:fix --workspaces",
+ "lint:check": "npm --workspaces run --if-present lint:check",
+ "lint:fix": "npm --workspaces run --if-present lint:fix",If type-aware linting needs built types, consider prepending: npm -w @webui/common run build.
📝 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.
| "lint:check": "npm run lint:check --workspaces", | |
| "lint:fix": "npm run lint:fix --workspaces", | |
| "lint:check": "npm --workspaces run --if-present lint:check", | |
| "lint:fix": "npm --workspaces run --if-present lint:fix", |
🤖 Prompt for AI Agents
In components/webui/package.json around lines 12-13, the lint scripts should
include the --if-present option and place the flags before the run command;
change them to run with flags first (e.g., npm --workspaces --if-present run
lint:check and npm --workspaces --if-present run lint:fix) so npm won't fail if
some workspaces lack the scripts and the flag order is valid; if type-aware
linting requires built types, optionally prepend an npm -w @webui/common run
build step.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/webui/server/package.json (1)
19-19: Optionally declare Node engine.
Helps CI and devs align with Node 22."type": "module", + "engines": { + "node": ">=22 <23" + },
♻️ Duplicate comments (4)
components/webui/server/package.json (2)
8-12: Fix compiled path: TypeScript emits to dist/, not dist/src.
Update script targets or builds will fail to start.Apply:
- "dev:start": "fastify start --ignore-watch=.ts$ -w -l info -P dist/src/app.js", + "dev:start": "fastify start --ignore-watch=.ts$ -w -l info -P dist/app.js", - "start": "npm run build && fastify start -l info dist/src/app.js", + "start": "npm run build && fastify start -l info dist/app.js", - "standalone": "npm run build && node --env-file=.env dist/src/main.js", + "standalone": "npm run build && node --env-file=.env dist/main.js",#!/bin/bash # Inspect TS config to confirm emit layout sed -n '1,200p' components/webui/server/tsconfig.json | sed -n '1,120p' rg -nP '"(outDir|rootDir)"' components/webui/server/tsconfig.json || true # Check entry sources exist rg -n 'src/(app|main)\.ts' components/webui/server -S -C1 || true
34-34: Move @types/node to devDependencies.
Avoids bundling types at runtime (see #1116)."dependencies": { - "@types/node": "^22.13.9", }, "devDependencies": { + "@types/node": "^22.13.9", }components/webui/package.json (2)
11-11: Use lockfile-only installs and target workspace by package name.
More reproducible; decouples from folder names.- "init": "npm clean-install --workspaces && npm --workspace common run build", + "init": "npm ci --workspaces && npm -w @webui/common run build",
12-13: Harden lint scripts; add --if-present and correct flag order.
Prevents failures if a package lacks lint scripts.- "lint:check": "npm run lint:check --workspaces", - "lint:fix": "npm run lint:fix --workspaces", + "lint:check": "npm --workspaces run --if-present lint:check", + "lint:fix": "npm --workspaces run --if-present lint:fix",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
components/webui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
components/webui/package.json(1 hunks)components/webui/server/package.json(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T18:14:01.607Z
Learnt from: davemarco
PR: y-scope/clp#1119
File: taskfile.yaml:302-305
Timestamp: 2025-08-19T18:14:01.607Z
Learning: npm ci in workspaces can work without physical package.json files in all workspace directories if the workspace information is properly defined in package-lock.json. Built static assets (like from Vite builds) don't require runtime package.json files since they're self-contained. npm ci uses the lockfile for dependency resolution, not individual workspace manifests.
Applied to files:
components/webui/package.jsoncomponents/webui/server/package.json
🔇 Additional comments (6)
components/webui/server/package.json (4)
2-2: Package rename—LGTM.
Scoped name matches the workspace.
33-33: TypeBox version bump—LGTM.
Matches prior suggestion.
35-35: Workspace dep on @webui/common—LGTM.
Using "*" with npm workspaces resolves to the local package.
47-51: Tooling bumps—LGTM.
Pinned eslint config, updated concurrently, tap, and typescript are reasonable.components/webui/package.json (2)
5-9: Workspaces declaration—LGTM.
Simple, clear layout for common/client/server.
22-22: concurrently ^9.2.1—LGTM.
Matches server’s version; resolves the install conflict noted earlier.
| "name": "webui", | ||
| "version": "0.1.0", | ||
| "description": "", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mark root as private and (optionally) add Node engine.
Prevents accidental publish; documents required Node.
"name": "webui",
"version": "0.1.0",
"description": "",
+ "private": true,
+ "engines": { "node": ">=22 <23" },📝 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.
| "name": "webui", | |
| "version": "0.1.0", | |
| "description": "", | |
| "name": "webui", | |
| "version": "0.1.0", | |
| "description": "", | |
| "private": true, | |
| "engines": { "node": ">=22 <23" }, | |
| // … the rest of your existing fields … |
🤖 Prompt for AI Agents
In components/webui/package.json around lines 2 to 4, the package.json is
missing the "private": true flag (and optionally an engines field) which allows
accidental publish and doesn't document Node requirements; add "private": true
at the top-level (e.g., immediately after "version") to prevent publishing, and
optionally add an "engines" entry such as "engines": { "node": ">=16" } (or your
supported range) to document the required Node version.
components/webui/package.json
Outdated
| "dev": "concurrently --raw \"npm run dev:*\"", | ||
| "dev:client": "npm --workspace client run start", | ||
| "dev:common": "npm --workspace common run build:watch", | ||
| "dev:server": "npm --workspace server run dev" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Select workspaces by package name, not folder.
More robust to renames and matches scoped package IDs.
- "dev": "concurrently --raw \"npm run dev:*\"",
- "dev:client": "npm --workspace client run start",
- "dev:common": "npm --workspace common run build:watch",
- "dev:server": "npm --workspace server run dev"
+ "dev": "concurrently --raw \"npm run dev:*\"",
+ "dev:client": "npm -w @webui/client run start",
+ "dev:common": "npm -w @webui/common run build:watch",
+ "dev:server": "npm -w @webui/server run dev"🏁 Script executed:
#!/bin/bash
# Verify workspace package names exist
rg -nP '^\s*"name":\s*"@webui/(client|common|server)"' -S components/webui -C1Length of output: 1169
Use workspace package names for run scripts
Verified that @webui/client, @webui/common and @webui/server exist; you may optionally switch to npm -w @webui/<pkg> (e.g. npm -w @webui/client run start) for greater resilience to folder renames.
🤖 Prompt for AI Agents
In components/webui/package.json around lines 14 to 17, the dev scripts use
workspace folder names (npm --workspace client/common/server ...) which can
break if folders are renamed; update the scripts to use workspace package names
with npm -w @webui/client run start, npm -w @webui/common run build:watch and
npm -w @webui/server run dev respectively so the commands target the package by
its workspace name rather than the folder path.
| "name": "@webui/server", | ||
| "version": "0.1.0", | ||
| "description": "", | ||
| "main": "src/main.ts", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nit: point “main” to built output (or drop it).
As an app, either set dist/main.js or omit “main”.
- "main": "src/main.ts",
+ "main": "dist/main.js",📝 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.
| "main": "src/main.ts", | |
| "main": "dist/main.js", |
🤖 Prompt for AI Agents
In components/webui/server/package.json around line 5, the "main" field points
to the TypeScript source ("src/main.ts") which is incorrect for a
published/built app; update the field to the built output (e.g., "dist/main.js")
or remove the "main" entry entirely if you don't need it, and ensure your build
script outputs to the referenced path (or adjust the build output path
accordingly) so the package.json points to the actual runtime artifact.
components/webui/package.json
Outdated
| "init": "npm clean-install --workspaces && npm --workspace common run build", | ||
| "lint:check": "npm run lint:check --workspaces", | ||
| "lint:fix": "npm run lint:fix --workspaces", | ||
| "dev": "concurrently --raw \"npm run dev:*\"", |
There was a problem hiding this comment.
qq: any reason we want "raw"? were the logs a bit clustered without --raw?
There was a problem hiding this comment.
I was playing around with console printing. this one i can remove. I also added --preserveWatchOutput fyi, since it was annoying that tsc would keep clearing terminal
There was a problem hiding this comment.
nice. i've been finding that annoying as well
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (2)
14-18: Simplify the guard with optional chaining; drop brittle typeof checks.Current checks are redundant and rely on string
"undefined". Prefer a single nullish check.Apply:
- if (0 === data.length || - "undefined" === typeof data[0] || - "undefined" === typeof data[0].row - ) { - return []; - } + const firstRow = data[0]?.row; + if (undefined === firstRow) { + return []; + }
21-30: Consider stable column ordering.If server/object key insertion order isn’t guaranteed, sort keys for deterministic column layout. Skip if order is intentionally meaningful.
- return Object.keys(data[0].row) - .map((key) => ({ + const keys = Object.keys(firstRow).sort(); + return keys.map((key) => ({ dataIndex: [ "row", key, ], key: key, title: key, width: 100, }));
♻️ Duplicate comments (5)
components/webui/client/package.json (3)
2-3: Mark the app package as private to prevent accidental publish.{ "name": "@webui/client", + "private": true, "version": "0.1.0",
24-24: Replace "*" with a pinned workspace version for @webui/common.
Using "*" hurts reproducibility and could pull a registry copy if one appears. Pin to the version declared in components/webui/common/package.json.- "@webui/common": "*", + "@webui/common": "^0.1.0",
49-49: Vite ^6.3.5 — confirm against prior “revert” request; consider pinning exact.
If we keep 6.3.5, prefer exact pin to reduce churn and ensure plugin parity.- "vite": "^6.3.5" + "vite": "6.3.5"components/clp-package-utils/clp_package_utils/scripts/start_clp.py (2)
845-847: Path fix to server/dist/settings.json is correct; ensure the build copies settings.json into distThe runtime expects server/dist/settings.json, but tsc won’t emit JSON. Add an assets copy step to the server build to prevent a 404 at startup.
Apply in components/webui/server/package.json:
{ "scripts": { - "build": "tsc" + "build": "tsc && mkdir -p dist && cp src/settings.json dist/settings.json" } }Quick check after building:
#!/bin/bash # Verify server settings.json is emitted to dist test -f components/webui/server/dist/settings.json && echo "OK: settings.json present" || (echo "Missing: components/webui/server/dist/settings.json"; exit 1)
965-968: Entrypoint updated to dist/src/main.js; verify build emits it and fail fast if missingMany tsconfig setups output dist/main.js (rootDir: "src"). Either adjust tsconfig or the path, and add a host-side existence check for clearer errors.
Option A — keep dist/src/main.js: set rootDir to "." in components/webui/server/tsconfig.json.
{ "compilerOptions": { - "rootDir": "src", + "rootDir": ".", "outDir": "dist" } }Option B — if you prefer dist/main.js, change the Python path back accordingly.
Also add a preflight check in this script:
node_cmd = [ str(CONTAINER_CLP_HOME / "bin" / "node-22"), - str(container_webui_dir / "server" / "dist" / "src" / "main.js"), + str(container_webui_dir / "server" / "dist" / "src" / "main.js"), ] + # Fail fast if entrypoint is missing on host + main_js_host_path = ( + get_clp_home() / "var" / "www" / "webui" / "server" / "dist" / "src" / "main.js" + ) + if not main_js_host_path.exists(): + raise FileNotFoundError( + f"WebUI server entrypoint not found at {main_js_host_path}. " + "Build the server package before starting (e.g., `task webui:build` or `npm -w components/webui/server run build`)." + )Quick check after building:
#!/bin/bash # Verify server entrypoint exists test -f components/webui/server/dist/src/main.js && echo "OK: main.js present" || (echo "Missing: components/webui/server/dist/src/main.js"; exit 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
components/webui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/webui/client/package.json(3 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.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/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: junhaoliao
PR: y-scope/clp#1119
File: taskfile.yaml:429-433
Timestamp: 2025-08-14T18:23:39.476Z
Learning: In the webui workspace, the exclude patterns ".tmp" and ".vite-temp" are Vite-specific temporary directories that only apply to the client package. The common and server packages use TypeScript compiler (tsc) directly and don't generate these Vite temporary files, so these exclude patterns should not be applied to their node_modules checksum computations.
🧬 Code graph analysis (1)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
get_clp_home(109-125)
🔇 Additional comments (5)
components/webui/client/package.json (2)
21-21: Aligned TypeBox to ^0.34.38 — good.
47-47: Pinning eslint-config-yscope is correct — avoids CI drift.components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx (2)
50-51: Verify rowKey compatibility with the centralised type.Ensure PrestoSearchResult in @webui/common guarantees a unique string/number
_id; otherwise VirtualTable row identity may break.
6-6: Type import migration to @webui/common verified
PrestoSearchResult is exported in @webui/common with both_idandrowas expected; no further action required.components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (1)
1-1: Type import switch to @webui/common is correct.Matches the workspace type centralisation; no behavioural change.
| "eslint-config-yscope": "latest", | ||
| "typescript": "~5.6.2", | ||
| "eslint-config-yscope": "1.1.1", | ||
| "typescript": "~5.7.3", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm TS version alignment across workspace.
Ensure all packages use the same operator (~5.7.3) to avoid duplicate installs.
🏁 Script executed:
#!/bin/bash
rg -n --hidden --glob '!node_modules' -e '"typescript"\s*:\s*"[^"]+"' -g '*package.json' -C1Length of output: 667
Align TypeScript specifier across all packages
In components/webui/common/package.json and components/webui/server/package.json, change
"typescript": "^5.7.3"to
"typescript": "~5.7.3"so all workspaces use the same operator and avoid duplicate installs.
🤖 Prompt for AI Agents
In components/webui/client/package.json around line 48, update the TypeScript
dependency specifier to match the other webui packages: change occurrences using
the caret operator to use the tilde operator so all workspaces declare
"typescript" with "~5.7.3" to ensure consistent versions and avoid duplicate
installs; modify the specifier in components/webui/common/package.json and
components/webui/server/package.json accordingly.
2 participants
Description
Converts webui to npm workspace, and modifies taskfiles to deal with node modules in root and packages.
npm workspace allows us to install dependencies in common, like typebox, allowing for more shared types between client and server.
Checklist
breaking change.
Validation performed
Package starts, and webui can compress and search logs.
Summary by CodeRabbit
New Features
Bug Fixes
Chores