Repository navigation
Conversation
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Runtime
server.base_pathconfig — added to the full and partial server schemas with an arktypenarrowregex that rejects empty/root paths, trailing slashes,//, and./..segments; mirrored inconfig.example.yaml,nix/options.nix, and the regenerateddocs/NixOS-options.md, defaulting to build-time/admin. - Runtime basename override —
app/server/app.tssets the process prefix and feeds React Router a patched build (configureBuildin prod) so route matching, SSR handoff, redirects, and the client'swindow.__reactRouterContext.basenameall reflect the configured path. - Asset-manifest prefixing — new
app/server/assets.tsrewritesassets.url, entry/route module/imports/css, and client chunk modules; Vitebaseis now./in builds andapp.tsserves the rewritten manifest dynamically (no-cache) ahead of the stale on-disk copy viadynamicPaths. - Client prefix accessor — new
~/utils/prefixgetPrefix()replaces all client__PREFIX__uses (favicon, SSE, SSHwindow.open, WASM/font URLs, preload); module-level constants were moved into functions to avoid pre-hydration evaluation. - Server plumbing — cookie path, OIDC
redirect_uri/post-logout, OIDC state cookie, listener basename, and the Docker listen-file healthcheck URL all useconfig.server.base_path. - Tests/docs — new base-path unit + platform integration tests (prod build booted under
/admin,/web,/tools/web), an OIDC callback/logout unit, and updated docs/changelog.
I verified the new unit tests (75 passed), the platform integration test (3/3 passed on a real production build), lint (0 errors), and formatting. The pnpm run typecheck failures are pre-existing in untouched test files (describe.sequential on Vitest 5) and unrelated to this PR.
One carry-forward risk (not blocking): the approach depends on React Router internals — the window.__reactRouterContext.basename handoff and the spread-patchable server build object. The integration test is the right guard for that.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers commit 567e5e2f, the only commit since the prior pullfrog review at 1f5bd192.
- Client prefix now sourced from an SSR-emitted meta tag —
app/utils/prefix.tsgetPrefix()readsmeta[name="headplane-base-path"]in the browser instead ofwindow.__reactRouterContext.basename, andapp/root.tsxrenders that tag during SSR so the runtime prefix is available before hydration and for SSE, WASM/font, SSH, and favicon URLs. - Asset-manifest unit tests — new
tests/unit/config/assets.test.tsassertsconfigureBuildrebases entry/route/module/css/client URLs and leaves the source build unmutated. - Prefix unit tests — new
tests/unit/config/prefix.test.tscovers the SSR value, the meta-tag read before hydration, and the build-time fallback. - Browser integration coverage — extended
tests/integration/platform/base-path.test.tswith a Playwright navigation check (hydration, API-key login, client navigation, no stray/adminrequests, no console errors) and installed Chromium in CI. - Dependency/docs —
playwrightdevDependency + lockfile,nix/package.nixhash bump, anddocs/CONTRIBUTING.mdbrowser-test instructions.
I verified the two new unit suites (5 tests passing) and that pnpm install --frozen-lockfile --lockfile-only is consistent. The remaining typecheck failures are pre-existing describe.sequential errors in untouched files; the extra playwright module-not-found errors are local to this environment (dependencies were installed before the PR lockfile) and CI installs it via build.sh's pnpm install --frozen-lockfile.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
tale
left a comment
There was a problem hiding this comment.
I left 1 nit but there's some bigger structural issues that I don't mind cleaning up.
- There's quite a few AI generated comments across your changes, I'd appreciate if they were trimmed up and necessary ones were hand-written.
- We can just drop the full __PREFIX and PREFIX calculation stuff in vite.config.ts, it's still leftover for some reason
- I'm not a fan of the tests and listener, your AI agent has written some older style TS. We are on Node 24 (soon to be 26 in a few weeks), we have modern APIs.
| const basePath = type("string").narrow( | ||
| (value, ctx) => | ||
| (/^\/[A-Za-z0-9._~-]+(?:\/[A-Za-z0-9._~-]+)*$/.test(value) && | ||
| !value.split("/").some((segment) => segment === "." || segment === "..")) || | ||
| ctx.mustBe("an absolute path of non-empty URL-safe segments without a trailing slash"), | ||
| ); |
There was a problem hiding this comment.
I'm fairly sure that arktype has a type available to us instead of needing to do a custom narrow. You might have to upgrade the version of arktype.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers the commits since the prior pullfrog review at 567e5e2f: the upstream main merge (7d41cf42) and the follow-up refactor (df722994) that addresses the maintainer's requested changes.
- Removed the build-time prefix entirely —
__PREFIX__is gone fromapp/globals.d.tsand the Vite/Vitestdefines, and the__INTERNAL_PREFIXfallback is gone fromreact-router.config.tsand the Vite/VitestPREFIXhelpers;server.base_pathnow defaults to the literal/adminin the schema, cookie/OIDC fallbacks, andgetPrefix(). - Swapped the custom
narrowfor ArkType's native regex inconfig-schema.ts, preserving the same validation semantics (leading slash, URL-safe non-empty segments, no trailing slash, no./..). - Modernized the runtime and tests to Node 24 APIs —
URL.parse,import.meta.dirname,mkdtempDisposable/await using,AbortSignal.timeout,TextDecoderStream, andgetSetCookie();composeListeneris now a singleasynchandler with unified error handling. - Trimmed comments across
assets.ts,main.ts,app.ts, and the tests, and documented the__INTERNAL_PREFIXremoval inCHANGELOG.mdand the native-mode guide.
Verified the base-path, prefix, assets, and OIDC unit suites (86 tests) pass, and confirmed the new regex accepts and rejects exactly the cases the unit tests assert. pnpm run typecheck reports only the local playwright-module-not-found errors (the dependency is installed in CI, not in this environment).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers the commits since the prior pullfrog review at df722994: the Nix dependency-hash refresh (4d723b49, carried over) and the documentation and comment trim (4c14d57f).
- Removed the remaining generated comments in
app/server/app.ts,app/utils/prefix.ts,vite.config.ts, andtests/integration/platform/base-path.test.ts. - Consolidated the custom-path documentation —
docs/configuration/index.md## Dashboard Pathnow owns the validation rules and the reverse-proxy/OIDC/cookie migration guidance, whiledocs/install/native-mode.mdanddocs/features/sso.mdwere slimmed to link to it (anchors and__INTERNAL_PREFIXremoval verified). - Reworded
config.example.yamlanddocs/CONTRIBUTING.mdfor brevity.
No behavioral surface in this delta; the runtime, asset-manifest, and config paths were reviewed and approved in the prior runs and are unchanged here.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers db377681, the only commit since the prior pullfrog review at 4c14d57f.
- Consolidated runtime manifest serving into the listener chain — replaced the
dynamicPathsbypass with anassetManifestoption socomposeListenerintercepts GET/HEAD on the generated manifest path and returns the rewritten manifestno-cacheahead of the static handler. - Simplified the app entrypoint —
app/server/app.tsnow exports React Router'screateRequestListener({ build: runtimeBuild })directly; the hand-written wrapping listener that served the manifest and delegated is gone. - Streamlined
composeListener— a singleasynchandler built onURL.parse, unified try/catch (500 /res.destroy), andstream/promisespipelinefor static files. - Restructured the platform integration test into per-path
describe.eachblocks backed by anAsyncDisposableStack, splitting route/asset/manifest/session assertions into separate tests and adding manifest HEAD and query-string cases.
The move is behavior-preserving: production still serves the rewritten manifest before static files (main.ts passes assetManifest from runtimeBuild.assets.url), dev still relies on Vite's manifest, and no dangling dynamicPaths references remain.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
@tale thank you for review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers fa46f75b, the only commit since the prior pullfrog review at db377681 — a merge of upstream main into the feature branch.
- Merged newer upstream
main(7d41cf42..b19c638c) while preserving everyserver.base_pathchange. The conflict areas all landed correctly: the auth cookie path (context.ts), OIDC redirect/end-session URLs (provider.ts), the OIDC state cookie with upstream's new signing secret, the runtime manifest interception incomposeListener, andsetServerPrefixinapp.ts. - Adopted upstream APIs in the base-path code paths —
alignOriginSchemeorigin rewriting inruntime/http.ts, the signedcreateOidcStateCookie(config), and theautoLink/SSHErrorrenames inapp/routes/ssh/page.tsx. - Adapted the platform integration test to the merged UI and CSRF behavior: updated login/sort selectors (
Sign in to Headplane,API key,Sign in with API key,Name), sent anOriginheader on the login POST so it exercisesalignOriginScheme, and added a wrong-cookie_secretparse assertion to the OIDC state-cookie test.
Verified the full unit suite (304 tests) passes on the merged tree, and confirmed the base-path plumbing is intact across the resolved files. pnpm run typecheck reports only the local playwright module-not-found error (the dependency installs from the PR lockfile in CI).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
I'm resolving these conflicts and I'll merge it, LGTM! |
|
Merged in #675, which builds on your commits. Thanks! |
Headplane currently fixes the dashboard prefix at build time. This adds
server.base_pathso the same build can serve a custom path. The default remains/admin.The setting is also available through
HEADPLANE_SERVER__BASE_PATHand NixOS. Routes, assets, cookies, OIDC URLs, browser SSH, live data, and Docker healthchecks use the configured path.Production rebases React Router's asset manifest at startup while keeping chunk and CSS imports relative. The HTTP runtime serves the prepared manifest before static files; the application exports React Router's listener directly. An SSR meta tag supplies the path before hydration.
Closes #637.
Compatibility
Paths require a leading slash, non-empty URL-safe segments, and no trailing slash.
/and./..segments are rejected. Custom paths require matching reverse-proxy and registered OIDC URLs. Replace the removed__INTERNAL_PREFIXbuild variable withserver.base_path.Validation
/admin,/web, and/tools/webusing one build. Routes, assets/fonts, manifests, and sessions/live data have separate cases with shared setup and cleanup. All previous checks remain, with added manifest HEAD and query-string checks. Chromium checks hydration, login, and client navigation under the nested path.app/utils/live-data.tsx; repository-wide formatting also flags two unchanged files.The runtime adapts React Router's generated build and manifest; the production tests cover this integration when the router is upgraded.