fix(dashboard): stop a bad payload from making a server undeletable - #560
Open
AbdullahM07 wants to merge 1 commit into
Open
fix(dashboard): stop a bad payload from making a server undeletable#560AbdullahM07 wants to merge 1 commit into
AbdullahM07 wants to merge 1 commit into
Conversation
A server could only ever be deleted from one screen — the detail page's ⋯ menu — so anything that stopped that page rendering also made the server impossible to remove. Reported against an offline box, which is the worst case: the one you most want to delete is the one whose page does the most work. Two render throws found by mounting the page with a real DOM: - RateLimitSettings guarded its config with `!== null`, which admits `undefined`. That check runs on every render BEFORE the component's own `!currentConfig` bail-out, so a 2xx body without `config` poisoned state and threw mid-render. The card is mounted by the server page, so one odd payload took the whole screen to the error boundary — and the only remove action with it. The shape is now validated at the boundary: a bad payload is a load error, not a crash. - useInfraFleet normalised with `?? []`, which admits any non-array, and then called `.some(...)` during render. Same class, bigger blast radius: that hook drives the servers LIST, so a non-array left no route to any server at all. Neither is reachable from the API's normal responses (an unreachable host answers 502/500, which the callers already handle) — they need a proxy answering for the API or a version skew. But the failure mode is a dead page either way, and both guards were one token from correct. Removal no longer depends on any of it: the servers list grows a per-row ⋯ menu with Remove Server. DELETE /system/servers/:id was always record-only and opens no SSH, so it worked fine while the host was down; the only thing missing was a way to ask for it. Reuses the detail page's confirm copy and i18n keys, so the two surfaces can't describe removal differently and no locale drifts. Testing: the dashboard had no DOM harness at all — every test used renderToStaticMarkup, which runs no effects and so can never reach a state that depends on a failed fetch. Adds happy-dom and mounts the real page inside the real (dashboard) provider stack, stubbing only the network. Covers the six failure shapes system/check actually returns (unreachable, auth, no_server, host channel blocked, host channel unprovisioned, a non-JSON gateway 502), every ?tab= the URL can restore, reaching Remove Server while the box is down, and the list-level delete end to end. Verified the suite catches the regression: with the rate-limit fix reverted, the security-tab case fails on the original TypeError.
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
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.
The problem
A server can only be deleted from one place — the detail page's ⋯ menu. So anything that stops that page rendering also makes the server impossible to remove. It was reported against an offline box, which is the worst case: the server you most want to delete is the one whose page does the most work.
The symptom is the
(dashboard)error boundary ("This page hit an error") on every visit to the server's id page.Two render throws
Found by mounting the page with a real DOM (see Testing):
RateLimitSettingsguarded its config with!== null, which admitsundefined:That line runs on every render before the component's own
if (loadError || !currentConfig) returnbail-out, so a 2xx body withoutconfigpoisons state and throws mid-render. The card is mounted by the server page, so one odd payload takes the whole screen to the error boundary — and the only remove action with it. The response shape is now validated at the boundary: a bad payload becomes a load error, not a crash.useInfraFleetnormalised with?? [], which admits any non-array, then called.some(...)during render:Same class, bigger blast radius — that hook drives the servers list, so a non-array leaves no route to any server at all.
Neither is reachable from the API's normal responses: an unreachable host answers 502/500 and both callers already handle that. They need a proxy answering for the API, or a version skew. But the failure mode is a dead page either way, and both guards were one token from correct.
Removal no longer depends on any of it
The servers list grows a per-row ⋯ menu with Remove Server.
DELETE /system/servers/:idwas always a record-only delete that opens no SSH, so it worked fine while the host was down — the only thing missing was a way to ask for it.<Link>, not nested inside it (abuttoninside anais invalid, and every menu click would navigate).Testing
The dashboard had no DOM harness at all. Every existing test used
renderToStaticMarkup, which runs no effects and therefore can never reach a state that depends on a failed fetch — which is why this class of bug was invisible. This addshappy-dom(one devDependency) and mounts the real page inside the real(dashboard)provider stack, stubbing only the network, so every component runs its own code.Coverage:
offline-server.render.test.tsxsystem/checkactually returns (unreachable, auth,no_server, host channel blocked, host channel unprovisioned, a non-JSON gateway 502); every?tab=the URL can restore (changeTabpersists it, so a security-tab visit sticks — that was the crashing one); reaching Remove Server while the box is downlist-remove.render.test.tsxDELETEfired with the right idThe suite provably catches the regression: with the rate-limit fix reverted, the security-tab case fails on the original
TypeError.745 passed (745)·tsc --noEmitclean.Note on formatting
servers/page.tsxis not prettier-clean onmain, so the re-indentation from wrapping the row was done by hand rather than running prettier and producing a large diff in unrelated code.