Replace Jinja UI with React frontend

[shiv-tyagi]

Should be merged after #249.

Renames the web package to backend, adds a React (Vite) frontend that talks to the existing API, and updates docker-compose, README, and flake8 so the new layout runs cleanly locally and in CI.

Made with Cursor

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 72 out of 91 changed files in this pull request and generated no new comments.

Suppressed comments (5)

frontend/index.html:1

• The HTML references /favicon.ico, but this PR adds frontend/public/favicon.svg. Unless a .ico file is also present/generated elsewhere, browsers will 404 the icon. Either change the link to /favicon.svg (and set an appropriate type, e.g. image/svg+xml) or add a favicon.ico asset to frontend/public/.
  frontend/src/hooks/useConfigLoad.ts:1
• The non-null assertions (find(...)!) can crash at runtime if the backing array changes (e.g., async refresh) or an unexpected id is passed. Guard these lookups and handle the 'not found' case by setting an error + returning to idle (or re-opening the conflict state) instead of throwing.
  frontend/src/components/FeaturesModal.tsx:1
• This category grouping is recomputed on every render. In this modal, renders can be frequent (search input, toggles), and features can be large. Wrap this reduction in useMemo keyed on features to avoid repeated work and reduce UI jank.
  frontend/src/components/FeaturesModal.tsx:1
• Using a div with role=\"button\" as a click target that contains another interactive control (TriStateCheckbox renders a button) creates nested interactive elements, which is problematic for keyboard and assistive tech. Prefer making the outer container a real <button type=\"button\"> for the scroll action (or make it non-interactive and add a dedicated 'jump to category' button), and ensure the checkbox control is not nested inside another button-like element.
  frontend/src/featureDeps.ts:1
• reducedDependencyEdges calls reachableWithin(...) inside a nested loop, which can become expensive with many selected features and dependency edges (repeated DFS per candidate edge). Consider memoizing reachableWithin(start) per start for the current ids set, or computing reachability once per node before the inner loop to avoid repeated graph traversals.

[peterbarker]

The security thing here doesn't look great here... I'd definitely want that one explained first!

  ⎿  docker-compose.yml
       ●  58 [security]       The new frontend service publishes
                              "${WEB_PORT:-11080}:80" without the loopback
                              binding the removed app service had
                              ("127.0.0.1:${WEB_PORT:-8080}:8080"),
                              exposing the UI and proxied API on every host
                              interface.
     frontend/src/components/BuildsTable.tsx
       ● 277 [correctness]    Download is gated on `terminal` (which
                              includes FAILURE/ERROR/TIMED_OUT) rather than
                              state === 'SUCCESS', but get_artifact_path
                              (backend/services/builds.py:243) only returns
                              a path when the archive exists.
       ● 106 [error-handling] doFetch chains .then().finally() with no
                              .catch(), so a rejected fetchBuilds becomes
                              an unhandled promise rejection with no error
                              surfaced.
     frontend/nginx.conf
       ●   8 [correctness]    `proxy_pass http://backend:8080/api/;` uses a
                              literal hostname with no resolver or
                              upstream variable, so nginx resolves
                              `backend` once at startup and never
                              re-resolves.
       ●   7 [correctness]    The proxy block sets no proxy_read_timeout,
                              so nginx's 60s default now applies to API
                              calls that legitimately exceed it.
       ●  15 [correctness]    Only /api/ is proxied and the backend port is
                              no longer published, so /health falls
                              through to `try_files … /index.html`.
     frontend/src/buildConfig.ts
       ●  62 [correctness]    fetchSchema only checks res.ok, but nginx's
                              `try_files $uri $uri/ /index.html` returns
                              index.html with status 200 for any unknown
                              path, so a missing schema parses as JSON.
       ●  34 [correctness]    configFromQueryParams requires vehicle_id, so
                              the old UI's ?rebuild_from=<build_id> links
                              are ignored entirely.
     frontend/src/types.ts
       ●  14 [correctness]    backend/services/vehicles.py:79-84 maps
                              unknown release types to "custom"
                              (backend/schemas/vehicles.py:20), but
                              Version['type'] and BuildForm.tsx:43's
                              TYPE_ORDER omit it (same omission at
                              types.ts:62).
     frontend/src/components/StepComponents.tsx
       ● 198 [correctness]    The quick pills pick options.find(o => o.type
                              === qt.type) — the first match across all
                              remotes — and render only version.name with
                              no remote badge, unlike the "More" list.
     frontend/src/components/FeaturesGraphView.tsx
       ● 772 [correctness]    This document-level Escape handler and
                              ModalShell's (ModalShell.tsx:50) are both on
                              `document`; stopPropagation() does not stop
                              other listeners registered on the same
                              target.
     README.md
       ● 112 [documentation]  The PR removed the working `uvicorn
                              web.main:app` instruction, leaving `python3
                              backend/main.py` as the only documented way
                              to run the API locally; it fails immediately
                              because sys.path[0] is …/backend and the repo
                              root is never added.
     frontend/src/components/StepComponents.tsx
       ● 198 [correctness]    The quick pills pick options.find(o => o.type === qt.type) — the first match across all remotes — and render
                              only version.name with no remote badge, unlike the "More" list.
     frontend/src/components/FeaturesGraphView.tsx
       ● 772 [correctness]    This document-level Escape handler and ModalShell's (ModalShell.tsx:50) are both on `document`;
                              stopPropagation() does not stop other listeners registered on the same target.
     README.md
       ● 112 [documentation]  The PR removed the working `uvicorn web.main:app` instruction, leaving `python3 backend/main.py` as the only
                              documented way to run the API locally; it fails immediately because sys.path[0] is …/backend and the repo root
                              is never added.
     .github/workflows/linting.yml
       ● 198 [correctness]    The quick pills pick options.find(o => o.type === qt.type) — the first match across all remotes — and
                              render only version.name with no remote badge, unlike the "More" list.
     frontend/src/components/FeaturesGraphView.tsx
       ● 772 [correctness]    This document-level Escape handler and ModalShell's (ModalShell.tsx:50) are both on `document`;
                              stopPropagation() does not stop other listeners registered on the same target.
     README.md
       ● 112 [documentation]  The PR removed the working `uvicorn web.main:app` instruction, leaving `python3 backend/main.py` as the
                              only documented way to run the API locally; it fails immediately because sys.path[0] is …/backend and the
                              repo root is never added.
     .github/workflows/linting.yml
       ●   1 [test-coverage]  CI still runs only flake8 and pytest; tsc --noEmit, eslint, and vite build never run despite the PR adding
                              an eslint config and a large TypeScript frontend.

[shiv-tyagi]

● 58 [security] The new frontend service publishes
"${WEB_PORT:-11080}:80" without the loopback
binding the removed app service had
("127.0.0.1:${WEB_PORT:-8080}:8080"),
exposing the UI and proxied API on every host
interface.

Yes, this one looks real. I will fix this and other points raised by you. This would have exposed custom build server to the world (bypassing apache), which is not catastrophic but not good either.

[shiv-tyagi]

@peterbarker Addressed all your points.

1. Frontend is now published as `127.0.0.1:${WEB_PORT:-11080}:80`, matching the old loopback bind so the UI and proxied API are not on every host interface.
2. Backend already served the archive whenever it existed (including failed builds); the UI now enables download from `artifact_available` on the build payload instead of any terminal state.
3. A rejected `fetchBuilds` is caught and shown as a full-width “Failed to load builds” row in the table, and the error clears on the next successful fetch.
4. Nginx only runs in Compose with `depends_on: backend`, so a one-shot resolve of the `backend` hostname at container start is acceptable here.
5. All `/api/` proxying uses a 120s `proxy_read_timeout` / `proxy_send_timeout`; a separate `/api/v1/builds/` location was dropped because it 301’d slashless URLs off port 11080.
6. We are not using `/health` yet, so leaving it unpublished behind nginx is fine.
7. `fetchSchema` now requires `Content-Type: application/json` (and a successful JSON parse) so nginx serving `index.html` with 200 cannot be treated as a schema.
8. We are cutting over immediately and will not keep `?rebuild_from=<build_id>`; rebuild stays on the in-app button via `fetchBuildConfig`.
9. Frontend `Version` / `BuildVersionInfo` types, sort order, and badges now include `custom`, matching the backend.
10. Quick pills are only official ArduPilot stable/beta/latest, so a remote badge there is unnecessary; the More list still shows remotes.
11. The graph Escape handler is on the capture phase and calls `stopImmediatePropagation` when it consumes the key, so it no longer races ModalShell on `document`.
12. Local API docs now start the server from the repo root with `PYTHONPATH=. uvicorn backend.main:app --host 0.0.0.0 --port 8080`.
13. CI has a frontend job that runs `npm ci`, `npm run lint`, and `npm run build` (tsc + Vite) on the same PR/push triggers as pytest.

[peterbarker]

README.mnd is trivial.

DId you mean to remove latest and use master?

New findings, ranked

1. README.md line 21 is corrupted. The word "your" now contains roughly 800 spaces, so the line reads "installed on yo ... r machine". Looks like an editor accident. Trivial fix, but it
   should not merge as is.
2. Version names changed in the public API. backend/services/vehicles.py:75 now hard-codes the name "master" for every "latest" release and drops the remote name from all titles. The
   sample remotes file has a "latest" release on a non-official remote pinned to a raw SHA, and it would be labelled "master". The tests were edited to lock in the new strings. The UI
   shows a remote badge so it looks fine there, but any other API consumer sees different names. This is scope creep beyond a rename plus new frontend and deserves a sentence in the PR
   description, or the remote name kept for non-official remotes.
3. Rate limit bypass via X-Forwarded-For (Codex, pre-existing). Uvicorn trusts forwarded headers from every address and nginx appends to whatever the client sends, so a caller can rotate
   the leading address and dodge the 10 builds per hour limit. This was already true with Apache in front of uvicorn on main, so it is not a regression. With a second proxy hop now in the
   chain it is worth setting FORWARDED_ALLOW_IPS to the actual proxy addresses in a follow-up.
4. Health endpoint unreachable through compose (Codex). A request to /health on the published port returns the SPA index with 200. The author already said nothing probes it. If Apache or
   any monitor ever does, add a location block for it.
5. Old rebuild links are dropped (Codex). The author stated this is a deliberate cutover. That is your call to confirm, not a bug.

Where I disagree with Codex. Its highest-severity finding was that the build-complete screen offers the download on SUCCESS before the archive exists. That is wrong. The progress updater
only reports SUCCESS after the archive file is on disk, see build_manager/progress_updater.py:187, so SUCCESS implies the file exists. The old auto-download in index.js relied on the same
guarantee. I would not raise this on the PR.

[shiv-tyagi]

1. Fixed.

2. Yes, it is deliberate. "Latest" is shown as a badge in the "master" version pill. So that we now Latest means master.

3, 4, 5 are all preexisting or not an issue.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-13)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Reviewed at head de0d5e7841.

Full report (every finding with its evidence, plus what was checked and cleared): https://uav.tridgell.net/DevCallReviews/2026_09_13_AIReview/devcall_pr_reviews.html#prCustomBuild-251

Verdict: REQUEST CHANGES — three small verified bugs. The base is solid:

• The web → backend rename is clean.
• All 255 backend tests pass, and flake8, tsc, lint and vite build are clean.
• api.ts matches the backend routes and schemas.
• There is no raw-HTML sink.
• The feature-dependency closure checks out against the real ArduPilot catalogue.

Your earlier answers in this thread (loopback bind, /health, one-shot DNS, the ?rebuild_from cutover, "Latest" shown as master) are taken as read and not re-raised.

Bugs

• backend/services/vehicles.py:78 — the version name is now just version_number, but VersionOut.name is a required string and remotes.schema.json doesn't require a number.
  • One schema-valid non-latest release without version_number makes GET /api/v1/vehicles/<vehicle>/versions return 500 for the whole list, official releases included. main returns beta None (myfork) for the same input.
  • Fix: title = version_info.version_number or version_info.commit_ref, plus a test.
• frontend/src/components/BuildForm.tsx:133 — the vehicle picker can get stuck loading.
  • A /?vehicle_id=... link (old /add_build?... links land here too) starts the config load before the first vehicle fetch finishes. That bumps the load generation, so the fetch never clears vehiclesLoading (:248), and reset() doesn't clear it either.
  • After "change" on the Vehicle pill, "Start over", or cancelling a conflict, the picker shows skeleton tiles forever. Reproduced in Chromium.
  • Fix: add setVehiclesLoading(false) to reset(), which cures all three cases.
• frontend/src/components/BuildForm.tsx:330-344 — "Start over" while a submit is in flight blanks the page.
  • startBuild() always runs setStep('building') and startPolling after the await. Only the Build button is disabled, so "Start over" and the pills still work.
  • After a reset, vehicle!.name at :622 throws on null and the whole app goes blank (no error boundary). The build still counts against the rate limit and gets polled.
  • Fix: capture loadGenRef.current before the await and return if it changed.

Issues

• frontend/src/components/BuildInfoModal.tsx:137-143 — when a build finishes between log polls, the pending log fetch is cancelled and never retried. The window keeps a log up to 3 s old, which for a failure is likely missing the error. The old UI polled until BUILD_FINISHED. Fix: fetch once more on the terminal state.
• BuildInfoModal.tsx:121-123 — a PENDING build's Logs tab (opened by default from BuildsTable.tsx:71) shows a red "Failed to refresh logs", because the log endpoint 404s until the build starts. Treat 404 as "waiting".
• frontend/src/hooks/useConfigLoad.ts:245, 284 — a superseded config load resets the phase to idle and closes a newer load's conflict modal. handleFeatureConfigDrop (BuildForm.tsx:271-277) similarly applies its result after the async schema fetch without checking the board is unchanged. Check the load generation in both.
• frontend/src/api.ts:34, 70 — raw response bodies reach the error banner: a 429 shows {"detail":...} JSON, and a 502 shows nginx's HTML page as text. Show detail instead.
• frontend/src/App.tsx:58-65 — /?build_id=<id> is silently ignored. The old UI redirected there after submit, so it is the URL people bookmarked or shared. This is separate from rebuild_from.

Notes (non-blocking)

• frontend/nginx.conf:11-14 — Host $host drops the port and X-Forwarded-Proto $scheme overwrites an outer TLS proxy's value. Backend redirects, such as a trailing-slash redirect, therefore point at http://host/ with no port and no https. Use $http_host and pass the trusted proto through.
• Rate-limit follow-up: with nginx as a second hop, trusting only the nginx container would put every client behind the Docker gateway address, in one bucket. FORWARDED_ALLOW_IPS has to cover the whole proxy chain.
• For release notes: OpenAPI moved to /api/openapi.json with no alias, and the default port changed from 8080 to 11080. jinja2 and python-multipart are now unused.
• Icon-only buttons in BuildsTable.tsx (:276, 284, 296, 321, 328) have no aria-label. Build polling retries every 2 s forever on error. DoneStep offers download only on SUCCESS, while the table uses artifact_available.
• npm audit --omit=dev shows 2 high runtime advisories (js-yaml, fast-uri), both fixable within the existing ranges. node:20 and nginx:1.27 are past support.

[shiv-tyagi]

1, 2, 3 were minor bugs which I have addressed. Other points look valid but mostly additive and can be fixed iteratively instead of blocking this PR.

I have updated this and tested the fresh version on custom beta. Looks okay mostly.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-14)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head 2898b04724; my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_14_1714/devcall_pr_reviews.html#prCustomBuild-251

All three bugs from my previous comment are fixed, so the verdict moves from REQUEST CHANGES to COMMENT. The B3 fix brings in one small regression, described below. I agree the remaining issues and notes can be follow-ups.

Previous round

• B1 RESOLVED: backend/services/vehicles.py:78 now falls back to commit_ref.
  • All 256 backend tests pass.
  • With the fix reverted, the new test fails with the original VersionOut.name validation error.
• B2 RESOLVED: reset() now clears vehiclesLoading (frontend/src/components/BuildForm.tsx:141).
  • Reproduced in Chromium at the old head, and gone at this head, for all three paths: Vehicle "change", Start over, and cancelling a conflict.
• B3 RESOLVED: startBuild() now checks the generation (BuildForm.tsx:332-352).
  • Start over during an in-flight submit no longer blanks the app. The old head threw TypeError: Cannot read properties of null (reading 'name').
  • The orphaned build is not polled.
• I1–I5 and N1–N5 are still open (code unchanged). I agree with you that none of them has to block merge.
  • I'd take these first:
    • I2: every queued build's default Logs tab shows a red "Failed to refresh logs". On a busy queue that reads as "my build failed".
    • I1: a failed build's log can be missing the error tail.
    • I5: old /?build_id= links are silently ignored.
  • The independent Codex pass argued that I3 and I5 should block. I don't think they need to hold up the merge.
  • Please track the list in an issue so it isn't lost.

New: introduced by the B3 fix

• ISSUE, frontend/src/components/BuildForm.tsx:352: changing a pill or step while a submit is in flight leaves the Build button disabled on "Submitting…".
  • goToStep() (:190) bumps the load generation but never clears submitting. The guarded finally then skips setSubmitting(false).
  • After the user picks the board again, the button stays disabled until Start over.
  • The POST has already been sent. The queued build counts against the rate limit but is not tracked, and a submit error on this path is dropped silently.
  • Both passes reproduced it in Chromium, for the Vehicle, Version and Board pills, with both success and 429 responses.
  • It's still an improvement on the old head, which jumped to the building screen with the wrong board.
  • Fix: add setSubmitting(false) in goToStep(). Codex confirmed that fixes all six cases. The alternative is to disable the pills and step links while a submit is in flight.

Checks at this head:

• Backend: 256 passed, flake8 clean.
• Frontend: npm ci, lint and npm run build all clean.
• CI: 3 of 3 passing.

[shiv-tyagi]

the new regression pointed out is also resolved. rest all is good for the merge. PTAL.

[tridge]

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head 6f7e26a9f4; my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_14_AIReview/devcall_pr_reviews.html#prCustomBuild-251

The regression from my previous comment is fixed and nothing new blocks — no blockers to merge. The verdict stays COMMENT only because I1–I5 remain open as agreed follow-ups.

Previous round

• Build stuck on "Submitting…": RESOLVED. goToStep() now calls setSubmitting(false) (frontend/src/components/BuildForm.tsx:199). That one line is the entire tree diff from 2898b04724.
  • In Chromium with a delayed POST, for the Vehicle, Version and Board pills with both 200 and 429 responses, Build is re-enabled in 6/6 cases at this head. With only that line removed it stays stuck in 6/6.
  • An independent Codex pass also found the old head fails and this one passes.
• B1–B3: still resolved; the code is unchanged.
• I1–I5 and N1–N5: unchanged. Please track them in an issue. I'd still take I2, I1 and I5 first.

New note (follow-up)

• N6, BuildForm.tsx:343-353: after a pill change during an in-flight submit, the POST already sent still queues a build that is never tracked, and a 429 on it is dropped silently. Now that Build re-enables, a user can queue a second build with only that one tracked (inferred from the code). Disabling the pills and step links while submitting would close it.

Checks at this head: npm ci, lint and build are clean. The backend is byte-identical to the last head, where 256 tests passed.

CI: 3 of 3 passing.

[peterbarker]

My claude/Codex and tridge's. The redis issue looks serious.

● Codex and I ended up with mostly different findings. The one issue we both found is the most serious: builds already stored in Redis will crash the builds endpoints after upgrading. I
checked each of Codex's claims against the code. It modified no files, and it couldn't run the frontend lint/build because npm isn't installed here.

Confirmed issues

1. Builds saved before this branch crash the builds endpoints (found by both of us). Those saved builds don't have vehicle_name, board_name, version_name or version_type.
   backend/services/builds.py:326-350 and BuildInfo.to_dict() (build_manager/manager.py:99) read those fields directly, so GET /api/v1/builds and /builds/{id} return a 500 on a deployment
   with old builds in Redis. The fix is getattr fallbacks or a migration.
2. ?rebuild_from= links stopped working (Codex). The old UI (add_build.js:554) used /add_build?rebuild_from= to reload a previous build's settings. frontend/src/App.tsx only looks for
   vehicle_id, so existing bookmarks and the "Copy and re-build" links now open an empty form.
3. The Download button ignores artifact_available (Codex). The builds table and the details modal check artifact_available. frontend/src/components/DoneStep.tsx:99 shows Download as soon
   as the state is SUCCESS.
4. Missing custombuild.yaml only produces a log message (Codex). In builder/builder.py:299-303, any failure writing the config is logged and the archive ships without it. Builds already in
   the queue when the builder is upgraded will always hit this.

Issues Codex missed

5. An unknown release type breaks build listing. builds.py passes version_info.release_type straight into BuildVersionInfo.type, which only accepts beta, stable, latest, tag or custom. Any
   other value in remotes.json fails validation and the endpoint returns a 500. vehicles.py maps unknown types to custom, but builds.py doesn't.
6. A missing version_number breaks the config file. Releases that come from remotes.json can have no version_number (providers.py:271). The code then stores an empty version_name, so the
   YAML fails the schema check. The /config download returns 404 and the archive has no config, with only a log line to show for it. vehicles.py falls back to commit_ref in this case;
   builds.py doesn't.

Real, but partly older than this branch

7. Downloads can be truncated while the archive is being written (Codex, rated High). tarfile.open(archive_path, "w:gz") creates the file at its final path before it's finished. The
   progress updater treats "the file exists" as "the build is done". Both of those were already on main. What's new is that artifact_available and the Download button above rely on the
   same check, so users are more likely to hit it. The fix is to write to a temporary file and rename it when done.
8. Rate limit can be bypassed with a fake X-Forwarded-For header (Codex). nginx adds to whatever X-Forwarded-For the client sends ($proxy_add_x_forwarded_for), and the backend trusts
   forwarded headers from any address (FORWARDED_ALLOW_IPS=*). That setting was already on main, but the nginx layer is new. The fix is to set the header to $remote_addr in nginx, or trust
   only the frontend container. I haven't checked how the production reverse proxy sets this header.
9. /health on the published port returns the web page, not the backend status (Codex). nginx only forwards /api/, so /health gets index.html with a 200. This only matters if something
   monitors that port.

I haven't changed anything. I'd start with 1, 5 and 6, which are small backend fixes, then 2 and 3 in the frontend.

[shiv-tyagi]

1. This is not an issue worth spending time. We will flush redis while deploying. We have done this before and that is the simplest way to handle it.
2. This is fine. The api and frontend get upgraded together. So there would be nothing hitting /?rebuild_from=
3. ⁠This is also fine. The artifacts become available as soon as build succeeds. The issue raised is an rare edge case which can be taken in a follow up. Not worth blocking the PR.
4. ⁠This is also okay. As I said we will flush redis while deploying so the new builds will surely get the custombuild.yml. For other cases also, it is fine shipping without custombuild.yml if something catastrophic happens while writing it instead of not shipping anything. We can handle things more gracefully as a follow up.
5. It can't be hit under normal conditions.
6. build configs mostly don't make sense for such custom listings. Those are mostly for official ardupilot releases. Someone trying to self deploy the app and knowingly feeding empty version number for a custom listing itself is not a good configuration. Though the issue seems genuine, maybe we can take it up as a follow up as the changes increase the scope by a lot. The versions manager, backend and more is involved there.

[peterbarker]

LGTM. @shiv-tyagi says he clears the redis DB as part of the upgrade, so not problem

[shiv-tyagi]

Thanks for approving @peterbarker. Merging.