feat(admin)!: serve the dashboard from the SPA bundle and delete the string literals - #526
Merged
Merged
Conversation
…string literals Completes ADR-0003 item 4. PR #508 ported the views onto the embedded bundle but left `GET /admin` on the server-rendered page so the two could be diffed against each other line for line; that comparison is done, so the literals go. Deleted: `src/admin/script.rs` (646 lines) and `html::dashboard_page` with its 15 source-text tests (`html.rs` 887 -> 150). Those tests matched emitted JavaScript source, so they could never distinguish a guard that runs from one merely present; `ui/`'s suite renders components and asserts on what an operator sees. `html.rs` keeps the login page, which stays server-rendered on purpose — a sign-in screen cannot depend on a bundle only a signed-in session is meant to reach (`admin-ui-delivery.md`, Resolution 6). BREAKING CHANGE: `GET /admin` answers `200` with the SPA shell instead of `303 /admin/login` when the caller is unauthenticated. The shell is one static file embedded at compile time and identical for every visitor, so it carries no operator data and needs no credential; the redirect did not disappear but moved into the bundle, which follows a `401` from `GET /admin/api/session` to the same page. A script that treated the `303` as "not signed in" must read the bootstrap endpoint instead. Without `--features ui` there is no bundle, and `/admin` answers `404` with a body naming the feature. Resolution 1 already accepts that a from-source build has no dashboard; what it does not license is an empty body, which is what dropping the route would leave — indistinguishable from an unconfigured `[server.admin]`, and the two have different fixes. Release binaries and the Homebrew formula build with the feature, so this gap is from-source only. Two test assertions moved to `/admin/api/session`, where they belong now that `/admin` is unauthenticated: "the OIDC callback minted a usable session" and "logout invalidated it" were both reading a status that no longer says anything about the caller's cookie. Both would have gone on passing while testing nothing. Also lands the three items #508 deferred into this PR: - the empty-usage copy test, the one of those 15 properties not carried forward. Each of its four branches names a different operator action, and nothing else on the page distinguishes them. - a settle-race in the `ui/` harness. `Upstream status` renders `null` — not a "Loading…" row — until its read resolves, so the existing wait was already satisfied while the section was still pending; every assertion about it would have raced the fetch. `renderDashboard` now waits for the section itself, and the new test holds that one read open so the wait is load-bearing. - `accountGroups` (106 lines -> 45) split into named helpers. It was left matching `script.rs`'s coalescing block line for line because the port review depended on that correspondence; with `script.rs` deleted there is nothing to correspond to. Closes #504: the README's admin row now says the dashboard needs a `--features ui` build, and the Install section notes that its own `cargo install` line does not produce one. Mirrored into the ko/ja/zh-CN READMEs and the four locale copies of the endpoints reference. `cargo test` both with and without `--all-features`, `cargo clippy --all-targets --all-features -- -D warnings`, `cargo fmt --all --check`, and `ui/` (50 tests, tsc, build) all pass. The two new `ui/` properties and the harness fix were each checked by deleting the guard and confirming the test fails.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request completes the migration of the admin dashboard from a server-rendered page to an embedded Single Page Application (SPA) bundle behind the --features ui flag. It removes the legacy server-rendered dashboard HTML and inline scripts, updates the /admin route to serve the SPA shell (or a descriptive 404 when the feature is disabled), and updates documentation across multiple languages. Additionally, it refactors the test suites to align with the new architecture, adds frontend unit tests for empty usage states and upstream status rendering, and improves the maintainability of the account grouping logic. No review comments were provided, so there is no feedback to evaluate.
 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 23 files
Architecture diagram
sequenceDiagram
participant Browser as Operator Browser
participant AdminSvr as Admin Router (axum)
participant SPA as SPA Bundle (ui/dist)
participant Green as GET /admin/api/session
participant Login as Login Page (server-rendered)
participant JSON as Admin JSON API
Note over Browser,JSON: SPA Delivery of Admin Dashboard
Browser->>AdminSvr: GET /admin
alt --features ui enabled
AdminSvr->>SPA: serve embedded shell (static, unauthenticated)
SPA-->>Browser: 200 text/html + strict CSP (script-src 'self')
Browser->>AdminSvr: GET /admin/api/session (bootstrap)
alt authenticated (session cookie or header)
AdminSvr-->>Browser: 200 {csrf, expiry_buffer_ms}
else unauthenticated (401)
AdminSvr-->>Browser: 401 Unauthorized
Browser->>Login: client-side redirect to /admin/login
Login-->>Browser: 200 login form + 'unsafe-inline' CSP
end
else no --features ui
AdminSvr-->>Browser: 404 body names "--features ui" (not empty)
end
Note over Browser,JSON: Post-login Data Reads
Browser->>JSON: fetch /admin/api/observed, /admin/api/accounts, /admin/api/pool
JSON-->>Browser: JSON (accounts, usage, pool health)
Browser->>Browser: accountGroups() coalesces managed rows + observations
Note over Browser,AdminSvr: Sign-out (validated against session)
Browser->>AdminSvr: POST /admin/api/logout
AdminSvr-->>Browser: 303 (clears cookie)
Browser->>AdminSvr: GET /admin/api/session (old cookie)
AdminSvr-->>Browser: 401 Unauthorized (proves logout)
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…utover falsified In-house review findings on the cutover commit. Four of the five are places where `--features ui` newly became load-bearing and a doc did not notice. The comment finding is the one worth stating plainly, because I got it backwards. `html.rs`'s module doc — and `ui/README.md` after it — said the login page stays server-rendered "because a sign-in page cannot depend on a bundle only a signed-in session is meant to reach". The bundle is not session-gated at all: the shell is served unauthenticated, which this very PR says three other times (`ui.rs`, `dashboard`, Decision 5). The real reason is availability, not authentication — the bundle exists only in a `--features ui` build, and an admin surface whose *sign-in* page vanished with the feature would be unusable rather than merely dashboard-less. Resolution 6 was also miscited: it keeps `/admin/login` and `/admin/oidc/callback` on their paths because they are pages rather than JSON with somewhere to move to, and says nothing about bundles. The rest follow from `GET /admin` now needing the feature, which it never did before — the server-rendered dashboard answered there in any build: - `site/.../guides/admin-remote-provisioning.mdx` (+ ko/ja/zh-cn) is the guide whose whole subject is this surface, and step 1 is "Open `/admin`". A reader on a from-source build got an unexplained 404. Adds the build note, linking each locale's own installation page. - The README's `[server.status]` row promised "Statuspage indicators in the dashboard and as a metric". On a default build only the metric half is now true. Reworded in all four READMEs. - `docs/desktop-app.md` justified pre-authenticating the webview with "`/admin` redirects to `/admin/login`", which it no longer does. The app's need is unchanged but its reason moved into the bundle, and the doc now also says the sidecar has to be a `--features ui` build. The test finding: `tests/router_surface.rs`'s three `#[cfg(not(feature = "ui"))]` arms — including the one this PR added for the default build's `/admin` — never ran in CI, because the only `cargo test` in `ci.yml` passes `--all-features`, which compiles them out. Adds a step running that one test binary with default features. Scoped to the binary rather than a second full suite: the marginal cost is this crate compiled once more, not every test binary rebuilt. Two findings were not acted on. The security review's note that the login page keeps `script-src 'unsafe-inline'` is already #525 and deliberately out of this PR. The silent-failure review's note that `renderDashboard`'s new wait would time out opaquely for a future test that supplies `fixtures.status` *and* forces the read to fail describes a test that does not exist, and the file already documents why that test is not written. Gates re-run: `cargo fmt --all --check`, `cargo clippy --all-targets --all-features -- -D warnings` (0), `cargo test --test router_surface` (14, the new default-feature path), the three admin test binaries under `--all-features` (45 + 11 + 13), and `ui/` (50).
There was a problem hiding this comment.
All reported issues were addressed across 19 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
Applies five cubic review findings on #526, plus two from the local gpt/ocr finders. - ui/README.md said every other path under the /admin mount serves the bundle. /admin/login, /admin/oidc/callback and /admin/api/* are routes of their own, and /admin/ is registered nowhere -- an axum wildcard must match at least one character, so it 404s. - The endpoints reference listed only the two wildcards as served without admin authentication, contradicting its own GET /admin row one screen above. Bare /admin joins the list, and the --features ui gating clause now names the two wildcard routes, since /admin is registered in both builds. All four locales. - Three .claude/agent-memory notes carried claims this PR falsified: a CI blind spot that ci.yml's default-build step closes, two doc gaps that 7fdaaf9 closed, and an overbroad no-leftover-references grep claim.
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The count covered `src/` and `tests/` together, which read as ambiguous. It is 4 in `src/` and 1 in `tests/admin_ui.rs`, now stated separately.
|
amondnet
added a commit
that referenced
this pull request
Sep 11, 2026
… line The #526 index line still claimed the new cfg(not(ui)) router test never runs in CI. That claim was corrected in the note itself during #526 but not in MEMORY.md, which is the file loaded first -- so the stale version was the one being read. It is run, by the default-build step that PR added. The #528 docs-review note cited a nonexistent `ADMIN_ROUTES` fixture; the constant is `ADMIN_PATHS`.
amondnet
added a commit
that referenced
this pull request
Sep 11, 2026
* fix(admin): serve the SPA shell on /admin/ as well as /admin
An axum `{*path}` segment must match at least one character, so `/admin/`
matched neither `.route("/admin", ...)` nor `.route("/admin/{*path}", ...)`
and answered a bare 404 on the dashboard's own mount root.
Register `/admin/` beside `/admin` in the base router, pointing at the same
`dashboard()` handler. Registering it there rather than beside the
`--features ui` catch-alls keeps the trailing-slash form's answer identical
to `/admin`'s in both builds -- the shell with the feature, and without it
the 404 whose body names `--features ui` rather than axum's empty one.
The repo already knew this rule: `/admin/api` and `/admin/api/` are
registered separately for exactly the same reason. The JSON namespace root
had both spellings; the mount root had only the bare one.
Closes #527.
* docs(memory): record the #528 review notes and correct the #526 index line
The #526 index line still claimed the new cfg(not(ui)) router test never runs
in CI. That claim was corrected in the note itself during #526 but not in
MEMORY.md, which is the file loaded first -- so the stale version was the one
being read. It is run, by the default-build step that PR added.
The #528 docs-review note cited a nonexistent `ADMIN_ROUTES` fixture; the
constant is `ADMIN_PATHS`.
amondnet
added a commit
that referenced
this pull request
Sep 11, 2026
…m misreporting a login (#530) * fix(admin): converge the dashboard state ladders and stop the add form misreporting a login Three defects split out of PR #508, where each was declined because that port had to keep the React page identical to the server-rendered original. That original is gone (#526), so the TypeScript under `ui/` is the only implementation left and the "must not diverge" constraint no longer applies. `statusNote` tested the account-wide and Fable cooldowns in sequence and returned on the first, so an account cooling both ways showed only the account-wide deadline while the pool table listed both. The active notes are now joined with ` · ` the way `cooldownText` already joins them; a single cooldown renders exactly as before, and neither renders as before. `poolState` and `managedState` disagreed about whether `near_quota` or the account-wide cooldown wins, so one account could read "near quota" in the pool table and "Cooling" in the accounts table. They converge on `managedState`'s ordering — cooldown first — because an account-wide cooldown is the fact that *all* of this account's traffic is gated right now, while `near_quota` is a threshold warning about what is coming; reporting the warning while hiding the active gate is the defect. Only `poolState` changes. `disabled` → `needs_relogin` → `!has_state` stay ahead of every cooldown/quota state in both, which is what keeps a permanently dead credential distinguishable from a pause. The add-account form left the login-method radios live after Start, so it could read "Setup token" over a pending OAuth login the server had already fixed; both mode inputs are now closed while `authorizeUrl` is non-null. `start` also left the previous authorization step on screen and clickable, with its Complete button posting to the name captured for that earlier flow; it now clears `authorizeUrl` and `currentName` up front. The epoch mechanism, the `complete` path, and the `completingNow` marker are untouched. Closes #511 Closes #512 Closes #513 * fix(admin): explain why the login-method radios lock, and document it Review follow-up on the #513 half of this branch. Disabling the mode radios when the authorization step opens is the right fix, but a `disabled` attribute states no reason: nothing in the DOM said why the choice went dead, and the form's own live region is empty at exactly that moment, because `start` clears it and a successful start never sets one. The radio group now carries `aria-describedby` — the mode help text it had never referenced, plus a `role="status"` note naming the lock, which announces itself as it appears. Both stylesheets gain a disabled-input rule so the dimming is not left to the browser default (`ui/src/index.css` mirrors `html::STYLE`). The lock was also undocumented. `docs/m9-admin-surface.md` records it beside the flow-epoch reasoning it belongs with, together with the start-clears-the- step half of #513, and the remote-provisioning guide gains a sentence in all four locales beside the existing "the server records the selected mode with the pending attempt" guarantee the lock enforces in the UI. * fix(admin): hold the login-method lock across the start request Two review findings, each raised independently by more than one reviewer. `start` cleared the authorization step but not the code that belonged to it. `complete` clears the code only when the exchange succeeds, so a failed one leaves it in the box, and the next flow submits it against a pending entry it was never issued for — which fails on a state mismatch, blaming the operator's fresh paste for a stale one. `start` clears it now, the way `prime` already did. (gemini-code-assist, cubic) The mode lock had the same shape of gap as the bug it was added for. `start` nulls `authorizeUrl` before it sends, so a lock keyed on that alone reopened the radios for exactly the length of the request that had already captured `mode`: the operator could switch method mid-request and be handed an authorize link for the other one, with the form then locking around the wrong answer. The flow exposes a `starting` flag instead, true from the moment the request is issued, and the form locks on `flow.starting || authorizeUrl`. A superseded start does not release it — `prime` and the newer start each set it as they take over, so the flag always belongs to whoever owns the epoch. (greptile P1, cubic P2, chatgpt-codex-connector P1) Both tests fail if their fix is reverted: dropping `flow.starting` from `locked` reds the in-flight test alone, and dropping `setCode('')` reds the stale-code test alone. * fix(admin): stop the lock note promising a release it cannot give Round two of review findings. The note said "Start another login to change it", and with the lock now spanning the whole flow that is not reachable: a replacement Start re-arms `starting` before any render, so there is no frame in which the radios are selectable. The note and the guide now name the two things that do release the choice — completing the flow, or reloading the page. Whether the form should offer an explicit way to abandon an open flow is a separate question, tracked in #531. (chatgpt-codex-connector P2) `start`'s catch reported a bare `'Request failed'`. Two reviewers asked for `copy.startFailure` instead; that string is `'Failed to start'`, the message for a rejection the server *answered*, and the catch is the case where it answered nothing. This hook already distinguishes the two on the completion path, so the literal becomes a documented module constant beside `UNKNOWN_COMPLETION` rather than folding into the caller's rejection wording. The other `'Request failed'` sites are in files this change does not touch. (gemini-code-assist MEDIUM, cubic P2 — applied as an alternative) `docs/m9-admin-surface.md` also records why the lock is keyed on `starting || authorizeUrl` and why a superseded start does not release it. * style(admin): drop an unneeded :has() from the disabled-choice rule `.choice:has(input:disabled) span` becomes `.choice input:disabled ~ span`. The span is a following sibling of the input inside `.choice`, so the two select the same elements and the sibling form needs no `:has()`. Applied for the simpler selector rather than for the reason given: the reviewer cited `:has()` browser support, but the rule two lines down (`.choice:has(input:focus-visible)`) predates this PR and already requires it, so a browser without `:has()` loses the focus ring either way. Both mirrored stylesheets change together, per the note on `html::STYLE`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes ADR-0003 item 4 ("port the existing views and delete the dashboard string literals"). #508 did the porting; this does the deletion, and closes #504.
#508 deliberately left
GET /adminon the server-rendered page so the ported components could be diffed against the original line for line. That comparison is done.What goes
src/admin/script.rssrc/admin/html.rshtml.rstestsThose 15 tests matched emitted JavaScript source, so they could never distinguish a guard that runs from one that is merely present.
ui/'s suite renders components and asserts on what an operator sees (50 tests).html.rskeeps the login page. It stays server-rendered on purpose: a sign-in screen cannot depend on a bundle only a signed-in session is meant to reach (admin-ui-delivery.md, Resolution 6).The breaking change
GET /adminanswers200with the SPA shell instead of303 /admin/loginwhen the caller is unauthenticated.The shell is one static file embedded at compile time, identical for every visitor — it carries no operator data and needs no credential, which is already true of every other path under the mount. The redirect did not disappear; it moved into the bundle, which follows a
401fromGET /admin/api/sessionto the same page (ui/src/App.tsx). A script that read the303as "not signed in" must read the bootstrap endpoint instead.Without
--features ui/adminanswers404with a body naming the feature.Resolution 1 already accepts that a from-source build has no dashboard. What it does not license is an empty body — which is what dropping the route would leave, since axum's own
404for an unregistered path carries nothing. An operator would then have no way to tell a missing feature from a missing[server.admin]block, and those have different fixes. Release binaries and the Homebrew formula build with the feature (verified: the generated formula downloads the release artifacts), so the gap is from-source only.Two assertions that were testing nothing
/adminbeing unauthenticated makes a200there say nothing about the caller's cookie. Two tests were reading exactly that:admin_oidc_full_flow_mints_session_and_preserves_header_auth— "the callback minted a usable session"browser_session_dashboard_csrf_accept_and_logout— "logout invalidated it"Both now assert against
/admin/api/session, where authentication is actually decided — and which, unlike/admin, answers identically in both builds (this file runs in both). Left alone they would have gone on passing.The three items #508 deferred here
pendinglocal that was itselfstate === "connected", so the port's'connected'check is the same condition, not a divergence.ui/harness —Upstream statusrendersnull, not aLoading…row, until its read resolves, sorenderDashboard's existing wait was already satisfied while the section was still pending. Every assertion about it raced the fetch: absence would have passed for the wrong reason, presence would have flaked. The harness now waits for the section itself, and the new test holds that one read open for 40 ms so the wait is load-bearing rather than decorative.accountGroups, 106 → 45 lines — split intomanagedState/managedRow/observedRow/foldObservation/uuidsByAccountName. It was left mirroringscript.rs's coalescing block because the line-for-line port review depended on that correspondence; withscript.rsdeleted there is nothing to correspond to. Behaviour unchanged —coalescing.test.tsxand the rest of the suite are untouched.Closes #504
The README's admin row now states that the dashboard needs a
--features uibuild, and the Install section notes that its owncargo install --gitline does not produce one — which was a real trap, since that command is three lines above. Mirrored intoREADME.ko.md/.ja.md/.zh-CN.mdand the four locale copies of the endpoints reference, plusdocs/admin-ui-delivery.md,docs/m9-admin-surface.md, andui/README.md.Deliberately not here
html_body_with_form_actionstill sendsscript-src 'unsafe-inline'andconnect-src 'self'on a login page that has neither a script nor a fetch — both could be'none'. The stale comment claiming otherwise is corrected in this PR; the policy change itself is #525, because tightening a live page's CSP has its own failure mode (a directive that turns out to be load-bearing breaks the one page an operator cannot route around) and does not belong in a deletion.Verification
cargo test --all-features --workspaceandcargo test --workspace— both green, includingadmin_surface45/45 andadmin_ui11/11 in their respective configurations.cargo clippy --all-targets --all-features -- -D warningsandcargo clippy --all-targets -- -D warnings— 0.cargo fmt --all --check— clean.ui/: 50 tests,tsc --noEmit,vite build— all clean.ui/properties and the harness fix were each checked by deleting the guard and confirming the test fails. The first attempt at the harness test stayed GREEN under mutation (the mock resolved too fast for the race to occur); the slow route is what makes it RED.Summary by cubic
Serves the admin dashboard entirely from the embedded SPA bundle, deleting the server-rendered string literals (
src/admin/script.rsandhtml::dashboard_page).GET /adminnow answers200with the SPA shell instead of303 /admin/loginwhen unauthenticated; the redirect moved into the bundle, which follows a401fromGET /admin/api/session. The login page stays server-rendered because the bundle exists only in a--features uibuild — an admin surface whose sign-in page vanished with the feature would be unusable rather than merely dashboard-less.Breaking change
303as "not signed in" must readGET /admin/api/sessioninstead.--features ui,/adminanswers404naming the feature so a missing feature is distinguishable from a missing[server.admin]block.Also
/admin's200as proof of authentication now assert against/admin/api/session.ui/harness aroundUpstream status, and splitsaccountGroupsinto named helpers.ui/README.mdnow say only/admin,/admin/{*path}, and/admin/api/{*path}serve the bundle, while/admin/login,/admin/oidc/callback, and/admin/api/*are their own routes and/admin/404s.tests/router_surface.rs, whose#[cfg(not(feature = "ui"))]arms--all-featurescompiled out entirely.Written for commit d309487. Summary will update on new commits.