build(ui): pin trailingSlash=true and verify Node v20 in build_ui.sh#29259
Conversation
Greptile SummaryThis PR adds two build-time locks to prevent committed UI artifacts from drifting:
Confidence Score: 5/5Safe to merge — all three files contain targeted, well-scoped changes with no logic regressions introduced. The previously flagged build-failure exit-code issue is now fixed with No files require special attention.
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/build_ui.sh | Replaces fragile nvm use v20 with nvm install v20 + nvm use v20 + node -v version guard; adds exit 1 to the npm build failure branch (fixing the previously flagged P1). |
| ui/litellm-dashboard/next.config.mjs | Adds trailingSlash: true to pin static export layout to <route>/index.html, preventing cache/version-driven flips between that form and <route>.html. |
| tests/test_litellm/ui/test_ui_build_pinning_lit_2723.py | New regression tests asserting the presence of trailingSlash: true and the nvm/node version guard markers; file-level assertions only, no network calls. |
Reviews (2): Last reviewed commit: "fix(build_ui): exit 1 when npm run build..." | Re-trigger Greptile
|
@greptileai please re-review — addressed the P1 in commit f0fa7d1: the |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
Summary
LIT-2723. Two small build-time locks for the admin UI:
ui/litellm-dashboard/next.config.mjs— settrailingSlash: true. The static export then consistently emits<route>/index.html(directory-index). Different Next.js patch versions and stale.next/caches were flipping the export betweenfoo.htmlandfoo/index.html, producing massive rename-only diffs inlitellm/proxy/_experimental/outand breaking deployments that rely on the directory-index form (e.g. nested OAuth callback routes — see fix(admin-ui): emit nested routes as <dir>/index.html so /ui/mcp/oauth/callback works #28106 / chore(admin-ui): regenerate static export with trailingSlash: true #28112).ui/litellm-dashboard/build_ui.sh—nvm install v20first (the barenvm use v20silently no-ops when v20 isn't already installed in non-interactive shells, e.g. CI), error-check both nvm steps, then verifynode -vactually reportsv20.*before runningnpm run build. Without this guard, committed artifacts can slip in built against Node v22 (which has happened on recent commits per the ticket).The ticket also mentions
docker/Dockerfile.custom_ui— that file no longer exists in this tree. The Dockerfile.non_root path was already updated in #28112, so there's nothing else to touch on the Docker side here.Note on review diff
The fork's
GITHUB_TOKENlacksrepo+workflowscopes, so I pushed via thePUT /repos/{owner}/{repo}/contents/{path}REST API rather thangit push. The merge-base diff is exactly the three files below; see "Files changed" for the canonical view.Evidence
1.
trailingSlash: trueflips the export from<route>.htmlto<route>/index.htmlBuilt a minimal Next.js 15 project mirroring the dashboard's nested route structure (
/,/login,/settings/admin-settings,/tools/mcp-servers) and rannext buildwithoutput: "export"both ways.Before —
output: "export"only (current state ofui/litellm-dashboard/next.config.mjs):After —
output: "export"+trailingSlash: true(this PR):Every nested route now lives at
<route>/index.html. The existing committedlitellm/proxy/_experimental/out/onlitellm_oss_agent_shin_daily_branchcurrently has the bare-.htmlform for nested routes (e.g.organizations.html,settings/admin-settings.html,tools/mcp-servers.html), which is exactly the form the customer fix in #28106 / #28112 needed to move away from. WithouttrailingSlash: true, the very next local rebuild flips it right back.2. build_ui.sh node-version guard fires on the wrong Node
Stubbed
nodeto returnv22.16.0(the version that was sneaking into recent committed artifacts per the ticket), then exercised the new guard:Same fragment with a stub returning
v20.18.0:Same fragment with
nodemissing (the silent-no-op-on-non-interactive-shell scenario):The previous code in
build_ui.shwasnvm use v20followed by a check of$?. That check fires only whennvm useactually returned non-zero — which it doesn't when nvm isn't loaded in the shell at all (thenvmfunction simply isn't defined, thenvm use v20line itself errors with "command not found" butset -eis not in effect, so the script just walks past it andnpm run buildruns against whatevernodeis onPATH). The newnvm install v20+nvm use v20+node -vtriple closes that hole.Tests
tests/test_litellm/ui/test_ui_build_pinning_lit_2723.py:The tests assert on the locking lines —
trailingSlash: trueinnext.config.mjs, and thenvm install v20/Failed to switch to Node.js v20/node -v/v20.markers inbuild_ui.sh— so a later cleanup can't silently regress either piece without breaking the test.Out of scope
docker/Dockerfile.custom_uiwas named in the ticket but no longer exists in this tree (Dockerfile.non_root was updated in chore(admin-ui): regenerate static export with trailingSlash: true #28112).docker/build_admin_ui.shseparately usesnvm install v18.17.0before invokingbuild_ui.sh. That's the enterprise UI build path; touching it would expand scope. Left as-is.Linear
LIT-2723.
Verification (ship-pr)
litellm_oss_agent_shin_daily_branch(required for fork PRs).f0fa7d1.mergeable_state=clean.npm run buildelse-branch missingexit 1; addressed inf0fa7d1, re-review bumped to 5/5).### Evidencesection above (Next.js export layout before/after + build_ui.sh node-version guard fires on v22 / empty / passes on v20).pytest tests/test_litellm/ui/test_ui_build_pinning_lit_2723.py— 2 passed). Tests are in addition to runtime evidence, not in place of.