Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3095a7e20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "engines": { | ||
| "node": "^20.19.0 || >=22.12.0" |
There was a problem hiding this comment.
After this upgrade, the UI explicitly requires Node ^20.19.0 || >=22.12.0 (ui/package.json:6-7), and the new lockfile entries for vite / @vitejs/plugin-react carry the same floor (ui/pnpm-lock.yaml:1485-1487, 2617-2619). The source-build path in Dockerfile:1-11 still starts from node:20-alpine, so any machine or CI runner that has a cached 20.18.x image and runs docker build without --pull will now fail during pnpm install or pnpm run build. Please update/pin that builder image alongside the dependency bump so local/source image builds remain reproducible.
Useful? React with 👍 / 👎.
Signed-off-by: Zzde <zhangxh1997@gmail.com>
There was a problem hiding this comment.
Pull request overview
Updates the UI build tooling to Vite 8 and replaces the previous dynamic-base approach with a custom runtime asset-base placeholder that is rewritten at build time and injected at runtime by the Go server.
Changes:
- Upgrade
viteto^8.0.1(and related Tailwind/Vite deps), removingvite-plugin-dynamic-base. - Add a Vite build plugin to rewrite built
index.htmlasset URLs to a__KITE_ASSET_BASE__placeholder for runtime substitution. - Update backend HTML injection logic + add tests to ensure runtime base handling works without
<base href>.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/vite.config.ts | Upgrades Vite config and adds a build-time HTML rewrite plugin for runtime asset base. |
| ui/package.json | Bumps Vite/Tailwind versions, removes vite-plugin-dynamic-base, adds Node engines. |
| ui/pnpm-lock.yaml | Lockfile updates for Vite 8 + dependency graph changes (rolldown, etc.). |
| pkg/utils/utils.go | Updates InjectKiteBase to replace the asset-base placeholder and adjusts injected script quoting. |
| pkg/utils/utils_test.go | Adds coverage for the new placeholder-based base injection behavior. |
| Dockerfile | Updates frontend builder base image to Node 24 Alpine. |
| .github/workflows/release.yaml | Updates GitHub Actions versions used in release workflow. |
| .github/workflows/docs-ci.yml | Updates checkout action version. |
| .github/workflows/ci.yml | Updates checkout/setup-node/setup-go action versions. |
| .github/workflows/build-preview-docker-image.yml | Updates action versions for preview build/push workflow. |
Files not reviewed (1)
- ui/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ui/vite.config.ts
Outdated
| const devSubPath = process.env.KITE_BASE?.replace(/\/$/, '') || '' | ||
| const runtimeAssetBasePlaceholder = '__KITE_ASSET_BASE__' | ||
|
|
||
| function getDevBase() { | ||
| return devSubPath ? `${devSubPath}/` : '/' | ||
| } |
There was a problem hiding this comment.
devSubPath is derived from process.env.KITE_BASE but isn’t normalized to ensure a leading /. The backend normalizes KITE_BASE to always start with / and trims the trailing / (see pkg/common/common.go), so allowing KITE_BASE=kite here would produce an invalid Vite base (kite/) and proxy key (kite/api/). Consider mirroring the backend normalization (prefix / when missing) before using it for base and server.proxy keys.
| assetBase := base | ||
| if assetBase == "/" { | ||
| assetBase = "" | ||
| } | ||
|
|
||
| htmlContent = strings.ReplaceAll(htmlContent, kiteAssetBasePlaceholder, assetBase) | ||
|
|
There was a problem hiding this comment.
assetBase is inserted into HTML attribute values via a raw string replacement. While baseScript uses %q (safe for JS), the attribute injection path isn’t escaped/validated, so a misconfigured KITE_BASE containing quotes/</whitespace could break the HTML or enable XSS. Consider validating base to a safe URL-path subset and/or HTML-escaping assetBase before replacing it into href/src attributes.
No description provided.