fix(admin): point the attention card at what is actually wrong - #167
Merged
Merged
Conversation
The card was a stat tile showing `summary.failedServers` while its caption counted failed backups and servers mid-delete, so it could read "0" with backups broken. Either way its link went to the unfiltered server list, which told an operator nothing they did not already know. Closes #163. The overview endpoint now carries the records behind each row, not just the counts, each with the route key its own destination takes: a server id for the server groups, the owning server's uuid_short for a backup (the backups tab is the only page a backup appears on, and ServerPolicy::before lets an admin open any server's). Groups are capped at 25 records so a broken fleet cannot turn the dashboard into a full table read; the count beside the row stays the authority for how many there really are. The card lists those records. One record is named outright and links straight at itself; several collapse into a disclosure whose summary previews the names and whose panel links each record separately. Nothing wrong at all gets an "all clear" state rather than a wall of zeros. Deleting servers are gone from the card: mid-delete is a transient state, not a failure, and the server-state card already counts it. Dropping it is what removes the ambiguity the issue is named for. The attention tile leaves the top row to three stat cards; the card itself takes the column beside Capacity, and Server State moves to full width. The overview tests now flush the cache between cases. OverviewService caches its payload for 15s, and whether that survives between tests depends on the ambient cache driver -- an array store is per-process and hides it, a shared redis does not.
5.x carries a checked-in ddev project and this branch does not, so working on 4.x meant hand-rolling one. This mirrors 5.x's setup where the two branches agree and diverges only where 4.x genuinely differs. Same shape as 5.x: nginx-fpm, a pinned host_db_port so GUI clients keep their saved connections across restarts, the ddev-redis add-on (byte-identical to 5.x's), horizon and scheduler as web_extra_daemons in place of the compose `workers` service, and the Vite dev server exposed. Tailored to this branch: PHP 8.2 and MySQL 8.0 per composer.json and docker-compose.yml, Node 20 per the release workflow, Vite on 1234 per vite.config.js, and CACHE_DRIVER rather than 5.x's CACHE_STORE, since this branch runs Laravel 11 against the pre-11 config/cache.php. php8.2-gmp is not optional: composer.json requires ext-gmp, and without it `composer install` fails its platform check while `ddev composer` still exits 0, so vendor/ silently stays stale and the next artisan call dies in platform_check.php. No db_test database, unlike 5.x. tests/Pest.php uses DatabaseTransactions rather than RefreshDatabase, so the suite runs against the dev database and rolls its own writes back -- meaning seeded dev data is visible to it and will fail anything asserting on a fleet-wide aggregate. The config says so where someone will hit it.
Same three files 5.x carries, so working on this branch in a Docker Sandbox
does not mean hand-rolling the provisioning each time. The sandbox plumbing is
identical to 5.x's and deliberately left alone: the ddev install, the NO_PROXY
entry that keeps *.ddev.site on this sandbox instead of resolving on the host
and driving the real app, the pinned Playwright under /opt/sbx-e2e rather than
in the repo, the writable /etc/hosts overmount ddev needs to register its
names, and the certs overmount that stops a sandbox-signed cert landing in the
host checkout.
The kit keeps 5.x's name, `convoy-dev`. Kits are referenced by path
(`--kit .sbx/dev`), so two branches naming theirs the same collide over
nothing. What does have to differ is the ddev project: `convoy4x`, since a 4.x
and a 5.x sandbox would otherwise fight over one project name. The suggested
template name is scoped for the same reason -- the saved image bakes in this
branch's PHP and database versions.
Setup is composer + npm + key:generate + migrate + ServerSeeder, which needs no
Proxmox credentials, rather than 5.x's DevNodeSeeder which does.
browser.mjs carries two fixes on top of the port, both found by running it
against this app:
- `login()` waited for the URL to leave /auth/login with the default
`waitUntil: 'load'`. This app leaves that route by a client-side
react-router redirect that fires no second load event, so the helper hung
for the full 30s timeout on a login that had already succeeded. Waiting on
'commit' returns in under a second, and the URL is the only thing the
predicate asks about anyway.
- `capture()` did `return page.evaluate(...)` inside a try whose finally
closes the page, so the close raced the pending evaluate and the call died
with "Target page, context or browser has been closed". Awaiting the
evaluate before returning fixes it.
Both bugs exist in 5.x's copy too. Whether to fix them there is a separate
change against that branch.
The agentContext carries the three things that cost real time here: assets are
prebuilt so a frontend edit is invisible until `npx vite build` (and a stale
public/hot renders the page blank), ext-gmp is required while `ddev composer
install` still exits 0 without it, and the suite runs on DatabaseTransactions
against the dev database so seeded data breaks any aggregate assertion.
5.x's kit points at docs/docker-sandbox.md for the proxy-isolation background.
That file does not exist on this branch and porting the whole document is not
this change, so the kit states the check inline instead of linking somewhere
that isn't there.
The files are force-added: .sbx is in the operator's global gitignore, which is
why 5.x's copies are tracked the same way.
sswang36
self-requested a review
September 9, 2026 03:18
Three follow-ups to the attention card, all of them about a row that promised
something it did not deliver.
The server rows linked at the primary key. Nothing in the panel routes on it:
RouteServiceProvider binds {server} as uuid_short for an 8-character value and
uuid for anything else, and ServerBuildTransformer deliberately ships the short
uuid as `id` with the primary key as `internal_id`. Every server link in the
admin UI is built that way, so the card's were the odd ones out and every click
died on "No query results for model [Convoy\Models\Server]". All three groups
now carry the same key and differ only in destination. A test walks the link
rather than restating the value -- it takes the id off the endpoint and fetches
the server page behind it.
Suspended servers are gone from the card. A suspension is an administrative
decision someone made on purpose; it is the panel working as asked, not a thing
to be fixed, and listing it beside real failures turns the card into a status
feed. The count stays in Server State, which is where a deliberate state
belongs. That leaves one tone, so the tone lookup goes with it.
The overflow link is gone. It pointed at the unfiltered server list -- the exact
dead end this card was written to remove, reintroduced for the leftovers -- and
there is nowhere honest to send them: the servers index allows no status filter,
and no page in 4.x lists backups. The sheet now says what it is not showing
instead of pretending to.
Modal.Body capped itself at 60vh, but nothing bounded the panel around it, so a header and a footer on top of that could exceed the screen. The scroll container is overflow-hidden, so the excess was not scrolled to -- it was clipped, and a long enough list took the title with it. The panel now caps at the viewport and lays out as a column, so the body is what gives: min-h-0 lets it shrink below its content and its own overflow-y-auto absorbs the rest, with the header and actions held at their natural size. It is a cap rather than a height, so a modal that already fit renders exactly as before.
DevNodeSeeder is a port of 5.x's, reading PROXMOX_* out of the gitignored .env. The 4.x Node splits what 5.x kept in one field -- `cluster` is the PVE name in the API path, `name` is a free label -- and the seeder probes /status and /storage for the host's real capacity rather than leaving the factory's invented 64GiB, falling back to those defaults when the node cannot be reached. AttentionSeeder covers both shapes the card renders: one failed server for the row that names its record outright, and 28 failed backups for the row that opens a sheet and runs past the 25-record cap. Suspended and mid-delete servers come along as negative checks, since both belong in Server State and neither should appear on the card. Fixtures hang off a placeholder node with a .invalid fqdn, never the live one.
Two faults in the same panel, both reachable from any modal in the panel.
The panel caps itself and hides its overflow, and Modal.Body sizes itself
with `flex-1` -- but 22 of the 23 modals put a <form> between the two so the
footer can submit. A block-level form is a flex item that will not shrink
below its content, so Modal.Body had no bounded parent to measure `flex-1`
against, grew to its full height, and pushed the submit row out through the
panel's `overflow-hidden` with nothing left to scroll it back. Create Server
and Create Node could not be submitted at all at a 900px-tall viewport: no
scrollbar, no wheel response, and no Cancel/Create on screen. Extending the
column through a direct-child form puts Modal.Body back under the cap.
FormProvider and FormikProvider render no DOM, so the form really is a
direct child in every case.
Separately, `initialFocus` pointed at an `<input type='hidden'>`, which
cannot take focus at all. Opening a dialog therefore left focus on the
trigger behind the overlay and the first few Tabs walked the page
underneath. A tabIndex={-1} sentinel is focusable programmatically without
joining the tab order, so it parks focus inside the panel while preserving
what the hidden input was for -- not stealing focus from the first field.
…tion The collapsed summary built one string -- every name it had, then "and 3 more" -- and left the trimming to `truncate`. But CSS truncates from the end, so the suffix was always the first thing cut, and the suffix is the only part the count beside it does not already say. With 25 backup names the row read "daily-1, daily-2, daily-3, daily-4, d..." and the three unlisted records went unmentioned. Returning the names and the overflow separately lets the names shrink around a pinned suffix. Also in the card: guard `subject.detail` in the single-record branch the way SubjectRow already does, so a subject without one does not render an empty line; key subject rows on their index as well, since one server can own two failed backups sharing a name and id+label then collides; add `aria-expanded` alongside `aria-haspopup`; and drop the reference to a 60vh cap on Modal.Body that this branch itself removed.
Dropping the attention stat tile left three cards behind, still laid out `sm:col-span-6 xl:col-span-4`. Three cards two-to-a-row means the third sits alone against a half-width gap, and that held from 640px all the way to 1279px -- most laptop widths included. Stacking them until `md` and fitting all three from 768px up gives one clean row or a clean stack, never 2+1. The skeleton follows the same breakpoints so the placeholder does not jump to a different shape when the data lands. Formatting is prettier's; the file was already failing `prettier --check` on this branch and the import it added was part of why.
The create-server form renders its address and template pickers before a node is selected. The select binds to a string and coerces with `Number`, and one caller defaults to -1, so the node id reaching these hooks is '', NaN or -1 -- and the request went out regardless, as /api/admin/nodes//template-groups and /api/admin/nodes/NaN/addresses. Both 404 on every open of the modal. A null SWR key tells SWR not to fetch, which is the intended way to express "no subject yet". Callers already handle `data` being undefined while loading, so nothing downstream changes.
Modal.Title rendered a bare h3, so Headless UI never saw a Dialog.Title and the dialog went out with no `aria-labelledby` at all. A screen reader announced "dialog" and stopped, with the visible heading sitting right there unused. Rendering Dialog.Title instead wires the dialog to it, and `as='h3'` keeps the heading level the markup already had. Verified on the attention sheet: the panel now carries aria-labelledby -> "28 backups failed".
Filed under Unreleased rather than a version heading: no tag is being cut yet, and Keep a Changelog keeps pending entries there until one is. Rename the heading when the release goes out. Only net changes since v4.6.1 are listed. The ragged stat-card row and the truncated overflow count were both introduced and fixed inside this same unreleased branch, so they never reached anyone and are not user-visible changes.
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.
Closes #163.
The bug
The attention card was a stat tile showing
summary.failedServerswhile its caption counted failed backups and servers mid-delete, so it could read 0 with backups broken. Either way its link went to the unfiltered server list, which told an operator nothing they did not already know.What it does now
The overview endpoint carries the records behind each row, not just the counts, each with the route key its own destination takes: a server id for the server groups, the owning server's
uuid_shortfor a backup (the backups tab is the only page a backup appears on, andServerPolicy::beforelets an admin open any server's). Groups are capped at 25 records so a broken fleet cannot turn the dashboard into a full table read; the count beside the row stays the authority for how many there really are.The card lists those records. One record is named outright and links straight at itself; several collapse into a disclosure whose summary previews the names and whose panel links each record separately. Nothing wrong at all gets an "all clear" state rather than a wall of zeros.
Deleting servers are gone from the card: mid-delete is a transient state, not a failure, and the server-state card already counts it. Dropping it is what removes the ambiguity the issue is named for.
Layout: the top row is three stat cards, the attention card takes the column beside Capacity, and Server State moves to full width.
Verified
77 tests pass. Clicked through against a real install: a failed backup lands on `/servers/<uuid_short>/backups`, a record inside an expanded group on `/admin/servers/`.
The overview tests now flush the cache between cases. `OverviewService` caches its payload for 15s, and whether that survives between tests depends on the ambient cache driver — an array store is per-process and hides it, a shared redis does not.
The two chore commits
`chore(ddev)` and `chore(sbx)` add a dev environment and sandbox kit to this branch, mirroring 5.x where the branches agree and diverging where 4.x differs (PHP 8.2, MySQL 8.0, Node 20, `CACHE_DRIVER`). The sbx commit also fixes two bugs in `browser.mjs` that 5.x's copy shares. They are independent of the fix and can be split into their own PR.