chore(web): remove the admin SPA and its OAuth worker routes - #6413
Conversation
Site previewPreview: https://022343c9-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 3:03 PM UTC · Completed 3:24 PM UTC Commit: |
PR Summary by QodoRemove web admin SPA and OAuth worker routes; make site Worker assets-only
AI Description
Diagram
High-Level Assessment
Files changed (102)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. ADR script reference stale
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Info
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
70cbb88 to
083f769
Compare
|
🤖 Review · Commit: |
083f769 to
9301a27
Compare
|
🤖 Finished Review · ✅ Success · Started 3:53 PM UTC · Completed 4:14 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Work on the web admin SPA is stopped and the feature is being removed rather than left paused. Paused-but-present code kept advertising itself as live work: the pause was recorded only in the title of docs/web-admin-deployment.md, while CI still ran the admin test suite and issue #2528 carried both good-first-issue and ready-to-code, so the code agent and human contributors were both routed at it (PR #6369). Removed: - web/admin/ (Svelte SPA) and web/docs/ (orphan test whose source was already gone) - The OAuth BFF in the site Worker: /api/oauth/authorize, /api/oauth/token and /api/github/user, plus oauthCors.ts and the admin API tests. The Worker is now a passthrough to the ASSETS binding. - The two [[ratelimits]] blocks, run_worker_first, and the wrangler.toml patch script that existed only to keep their namespace ids unique. The deploy already passes --name explicitly, so the name patch was redundant. - GITHUB_APP_* and TURNSTILE_* wiring from site-deploy.yml (both the production deploy and the PR preview upload), sample.env.local, and the matching miniflare test bindings. - The root Vite build, svelte-check, eslint config, the Svelte half of the Prettier config, and every runtime npm dependency, all of which were admin-only. vite.config.ts survives as a vitest-only config so the VitePress theme tests keep running. Note that removing run_worker_first makes the passthrough the Worker's only code path. The ASSETS binding was never declared in wrangler.toml, which was harmless while the Worker only ran for /api/* (those paths returned JSON before reaching the ASSETS branch) but would now 503 every request that reaches the Worker. wrangler.toml declares binding = "ASSETS", and a test asserts it is present. Unaffected, and verified so: - The VitePress documentation site builds and ships unchanged; docs:build passes and /docs/ is still assembled into the deploy bundle. - The public mint at mint.fullsend.sh is a separate Worker provisioned from internal/dispatch/cf/ with its own wrangler.toml. No file under internal/ is touched. - e2e/admin/ is the CLI install e2e suite, not the SPA, and is untouched. Cloudflare-side secrets (GITHUB_APP_CLIENT_SECRET, TURNSTILE_SECRET_KEY) and the FULLSEND_GITHUB_APP_* / FULLSEND_TURNSTILE_* repo secrets and variables are now unused and should be deleted out of band. The admin SPA was never rolled out, so this is not a user-facing breaking change and carries no `!` marker: there is no deployed UI for anyone to migrate off. Installation has always been driven by the CLI (`fullsend github setup`, `fullsend repos`). Stale /admin/* paths fall back to the landing page rather than 404, since not_found_handling stays "single-page-application". Refs #2528 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
9301a27 to
8e6cdd1
Compare
|
🤖 Finished Review · ✅ Success · Started 4:19 PM UTC · Completed 4:35 PM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 5:08 PM UTC · Completed 5:29 PM UTC Commit: |
Retro: PR #6413 — Remove the admin SPA and its OAuth worker routesOverall assessment: The workflow went well. This was a large deletion PR (104 files, −12,771 lines) by waynesun09 removing the admin SPA. It merged in ~2 hours with zero false-positive review findings and thorough author engagement. All improvement opportunities identified are already tracked by existing open issues. Timeline
What went right
Gaps identified (all covered by existing issues)ADR contradiction missed (evidence for agents#720 and agents#265): Qodo caught that the new removal note added to ADR 0019 (stating there is no root Provenance validation failures (evidence for fullsend#5388 and agents#203): All review runs on this PR failed provenance validation with "unverifiable-wrong-app", forcing prior review context to be discarded. Each re-review started from scratch. In this case the impact was minimal because the author fixed the flagged issues between pushes, but this is a recurring pattern caused by empty Cancelled review run (evidence for fullsend#4960): Run 2 was cancelled after ~17 minutes because a force-push arrived while it was running. This is a known waste pattern tracked across multiple open issues. No new proposalsAll improvement opportunities are already covered by existing open issues. No new proposals are warranted. |
3 participants
Summary
Work on the web admin SPA is stopped, and we are removing the feature rather than leaving it paused.
Leaving it paused was not holding. The pause lived in exactly one place — the title of
docs/web-admin-deployment.md, a Cloudflare deployment doc — while every operative signal in the repo pointed the other way: CI still ran the admin test suite,web/admin/README.mdcarried no notice, CODEOWNERS had no carve-out, and #2528 carried bothgood first issueandready-to-code. Sinceready-to-coderoutes to the code agent (internal/dispatch/router.go:125), both human contributors and our own automation were being aimed at paused code. #6369 is what that looks like in practice: a correct, well-tested PR against code that is on its way out.What is removed
web/admin/— the Svelte SPA — andweb/docs/, an orphan test whose source module was already gone and which no vitest config collected./api/oauth/authorize,/api/oauth/token,/api/github/user,oauthCors.ts, and the admin API test suite. The Worker is now a passthrough to theASSETSbinding.[[ratelimits]],run_worker_first, andpatch-wrangler-rate-limit-namespace-ids.mjs. The patch script existed to keep rate-limit namespace ids unique per Worker name; its only other job was patching top-levelname, which is redundant because both deploy paths already pass--nameexplicitly. Left in place it would have failed the deploy withmissing [[ratelimits]].GITHUB_APP_*/TURNSTILE_*wiring fromsite-deploy.yml(production deploy and PR preview upload) andsample.env.local. The site Worker now needs no vars and no secrets.svelte-check,eslint.config.js, and every runtime npm dependency — all admin-only.vite.config.tssurvives as a vitest-only config so the VitePress theme tests keep running.eslint.config.jsis removed rather than retargeted: it explicitly ignoreddocs/,cloudflare_site/, andweb/public/, so with the SPA gone it had nothing left in scope. Happy to reinstate it pointed somewhere useful if a reviewer prefers.Explicitly not affected — verified
npm run docs:build/docs/still assembled into the deploy bundle;site-deployment.html,doc-site.html,architecture.htmlall rendermint.fullsend.shgit status internal/dispatch/cf/wrangler.tomlunderinternal/dispatch/cf/workersrc/;DefaultPublicMintURL/DefaultPerRepoMintURLunchangedweb/public/index.html→/,web/public/graph.html→/graph.html, unchangede2e/admin/isTestAdminInstallUninstall/TestVendorFromSubdirectoryagainst the CLI, not the SPA. Untouched.npm testgo build ./...internal/orcmd/is touchedpre-commit run --all-filesNot a breaking change
The admin SPA was never rolled out, so removing it and its Worker routes breaks nothing for users — there is no deployed UI for anyone to migrate off, and installation has always been CLI-driven (
fullsend github setup,fullsend repos). No!marker orBREAKING CHANGE:trailer, deliberately: a migration notice in the GoReleaser release notes for a surface nobody had would be actively misleading.not_found_handling = "single-page-application"is left as-is, so any stale/admin/*path falls back to the landing page rather than returning a JSON 404. That is intentional and asserted by a test.Docs
docs/web-admin-deployment.mdwas mostly documentation-site deployment guidance that happened to live under an admin-titled page, so it is not simply deleted — the still-relevant content (Cloudflare Worker setup, API token, account id and Worker name, custom domains, Pages migration, fork/upstream GitHub setup, local preview, troubleshooting) is preserved asdocs/site-deployment.md, with the admin OAuth and Turnstile material dropped and a note that the mint is a separate Worker.docs/admin-oauth-worker.mdis deleted outright.doc-site.md's cross-link, both VitePress sidebar entries,web/README.md,docs/architecture.md's repository-layout section, and a dated note on ADR 0019 are all updated to match.Follow-ups, not in this PR
Ops — these cannot be done from the repo:
GITHUB_APP_CLIENT_SECRETandTURNSTILE_SECRET_KEY, and varsGITHUB_APP_CLIENT_ID/TURNSTILE_SITE_KEY, from thedocumentation-siteWorker.FULLSEND_GITHUB_APP_CLIENT_SECRET,FULLSEND_TURNSTILE_SECRET_KEYand variablesFULLSEND_GITHUB_APP_CLIENT_ID,FULLSEND_TURNSTILE_SITE_KEY.Backlog: the issue sweep is now folded into this PR — see Issues closed by this PR below.
Governance: whether removing a component warrants its own ADR beyond the note on ADR 0019 is a maintainer call; happy to write one.
Issues closed by this PR
Every open issue scoped to the removed SPA or its OAuth Worker routes, including the sub-issue tree under epic #509. GitHub closes these on merge.
Epic and feature work
Closes #509 — Admin web UI / SPA: delivery index for Story #124
Closes #512 — Admin web: organization hub
Closes #513 — Admin web: install or repair Fullsend for an organization
Closes #514 — Admin web: onboard and offboard repos from the org dashboard
Closes #547 — Admin web: trustworthy org listing and GitHub App install guidance
SPA defects and chores
Closes #541 — Admin SPA: token-at-rest strategy (localStorage vs mitigations)
Closes #542 — Admin SPA: surface non-401 errors from refreshSession
Closes #618 — Admin SPA: overlapping org-list layer analysis
Closes #619 — Admin SPA: GitHub REST rate-limit handling and retry policy
Closes #620 — Admin SPA: batch org GraphQL drops per-org hints
Closes #621 — Admin SPA: empty OAuth authorization code handoff error
Closes #623 — Admin SPA: install-readiness probes report only first missing capability
Closes #626 — Admin SPA: extract shared components and CSS design tokens
Closes #674 — admin: parallelize inner-layer API calls in enrollment and secrets analysis
Closes #675 — admin: wire up /org/:login and /install/:login routes in App.svelte
Closes #550 — Browser-based test suite for admin SPA with parity vs Go CLI flows
Closes #622 — Dev experiment: OAuth localhost helper uses a fixed OAuth state value
Worker OAuth, Turnstile, and the GitHub proxy — all routes removed here
Closes #505 — Pass client IP to Turnstile siteverify on admin token exchange
Closes #506 — Align production and preview admin hostnames for OAuth redirect validation
Closes #543 — Site Worker: generic JSON errors for GitHub /user proxy
Closes #546 — Admin OAuth: explicit GitHub authorize scope
Closes #585 — Site / admin SPA: Content-Security-Policy headers for Worker and static responses
web-admin drift from the Go backend
Closes #2526 — fix(web-admin): agentsFromConfig() references removed agents block
Closes #2528 — fix(web-admin): VALID_ROLES has 4 roles vs Go's 8
Closed separately, not by this PR
fullsend admin installis the per-org installer deprecated by ADR 0044 (ADR 0033 makes per-repo the sole supported model), and the install path is slated for removal. Theadminnamespace itself is being repurposed for org-administrator commands —admin foreignfor mint service management is the first. These two were closed as not planned on that basis; they are Go CLI issues and this PR does not touch them:Deliberately not closed
These matched an "admin" search, or hang off epic #509, but survive the removal. Worth noting that #509 is closed by this PR while two of its children (#544, #545) are not — closing the epic does not mean the whole subtree goes.
mise.toml. Filed under epic #509 but unrelated to the SPA.npm ciacross site-build and site-deploy. Verified both workflows still install, so still valid.internal/cli/admin.go/internal/layers/enrollment.go. Its callers areadmin enable/admin disable— per-org commands — so it is covered by the per-org install-mode deprecation (ADR 0044), not by the SPA removal. Owned by that effort.defaults.auto_mergenever consumed. Mentions the SPA schema in passing; the Go half stands.Supersedes #6369.