fix(admin): point the attention card at what is actually wrong - #166
Closed
ericwang401 wants to merge 843 commits into
Closed
fix(admin): point the attention card at what is actually wrong#166ericwang401 wants to merge 843 commits into
ericwang401 wants to merge 843 commits into
Conversation
Splitting the meter and its figure into two columns aligned the bars, but left the figure column as `auto`, so it absorbed the table's leftover width. Being text-right, that surfaced as a wide gap between each bar and its figure. It only showed up with short node names: long ones give the Node column enough max-content to soak up the slack first, which is exactly what the seeded test data had. With two short-named nodes at 1700px the figure column came out 285px wide for 100-119px of text, a ~190px gap. `w-px` collapses the column to the widest figure, so the slack goes to the Node column where it belongs. The column now measures a constant 127px from 1280px to 2200px, and the bars stay aligned and still grow with the viewport.
The four IPAM collections were the last DataTables with no mobileRow, so below @md they fell back to horizontally scrolling desktop tables. Each now renders the established muted Item row with the same actions as its desktop counterpart. Hoist the actionsColumn callbacks into shared renderActions helpers so the two representations cannot drift. This also moves AddressBlockTab's useAddressBlockModal call out of the cell render callback and up to the component body. Fix a truncation bug found while verifying: buttonVariants is inline-flex shrink-0, so `truncate` on a title Link neither shrinks it inside ItemTitle's flex row nor ellipsises its text. Long names escaped the row and were clipped mid-word by ItemContent's overflow-x-hidden, with no page overflow to make it obvious. Admin Servers, admin Nodes, node Servers, and node IPAM were already affected; all are corrected and the trap is written up in the audit. Verified at desktop and 390px against real seeded data: mobile renders Item rows and desktop keeps its table, no horizontal overflow, row menus carry the correct actions and return focus to the trigger on Escape, and a deliberately long node name now ellipsises.
The quota sidebar was entirely mocked: every figure was hardcoded (3 of 10 backups, 24 GiB of 24.5 GiB) and the fetched server was unused. Wire it to the server's real backup limits and to the count/size of its non-failed backups, handle the -1 unlimited sentinel on both limits, and give the icon-only trigger an accessible name. The index endpoint already returned a server-wide backupCount for this; add the matching size total next to it. backups.size is persisted in MiB but read back as bytes via StorageSizeCast, and a SQL aggregate bypasses the cast, so the sum is scaled through ByteUnit rather than returned raw -- otherwise the figure is off by 1048576x. The new test pins that round-trip. Also move the quota trigger into the standard responsive heading row, give Graphs its own "Resource usage" heading instead of reusing Overview's server-identity Header, and normalize the residual gap-5 page grids to the established gap-2/@md:gap-4 rhythm. Pest 266, PHPStan zero, tc and build green.
Eloquent is already the data-access layer, so wrapping it in repositories bought nothing -- EloquentRepository was ~280 lines re-implementing create/find/update/delete, and two of its methods already carried "@deprecated Just use the model". Query logic moves into the models it belongs to: - Backup::scopeNonFailed / scopeCreatedWithinSeconds, alongside the scopeSuccessful and scopeRunning already there. - Server::nonFailedBackupSize(), keeping the backups.size MiB-vs-bytes cast trap encapsulated in one place. - Server::isUniqueVmId / isUniqueUuidCombo. Call sites now read as plain Eloquent: $server->backups()->nonFailed(). ActivityRepository was dead -- its only apparent consumer was the unrelated ProxmoxActivityRepository -- so it goes, and RepositoryServiceProvider existed solely to bind it. RecordNotFoundException was thrown only by EloquentRepository and was RepositoryException's only child, so both are now orphaned; the Proxmox exceptions extend \Exception and ServiceUnavailableHttpException, and are untouched. ServerRepository::getByUuid had no callers and is not carried over. The Proxmox classes are deliberately left alone: they are HTTP clients for a remote API, not a wrapper over our own database, so none of this applies to them. They move to app/Services/Proxmox/ separately. Pest 266, PHPStan zero.
These were never repositories in the sense the previous commit removed: they are HTTP clients for Proxmox's remote API, not a wrapper over our own database, so the "Eloquent already is the data layer" argument never applied to them. Only the name was wrong, and it was actively misleading -- it implied a data store and invited the question of whether they should die too. app/Services/ is both the common Laravel placement and this repo's own precedent: app/Services/Metrics/VictoriaMetrics.php is already a "thin client" for a remote HTTP API living there. (Laravel prescribes nothing here; the one real convention, Saloon's app/Http/Integrations/, does not apply since these use the Http facade directly.) app/Repositories/Proxmox/*Repository.php -> app/Services/Proxmox/*Client.php app/Exceptions/Repository/Proxmox/* -> app/Exceptions/Proxmox/* Property and variable names follow the types ($serverRepository -> $serverClient, bare $repository -> $client) so the rename is not left half done. No logic changes. app/Repositories/ and app/Contracts/ are now gone entirely. The fluent setNode()/setServer() mutable state is deliberately left as-is -- that is a design change, not a rename. Pest 266, PHPStan zero.
…n recipe Records the no-repository-over-Eloquent decision and its Proxmox carve-out as a design constraint, corrects the exceptions constraint (RepositoryException is gone), and flags the backups quota as built-but-not-browser-verified.
Also records the stale-build trap: vite does not typecheck, so build-before-tc can leave a bundle that contradicts the source and sends you debugging a bug that no longer exists.
The empty state's "Create Backup" button was dead and there was no create-backup UI anywhere, so this is the feature, not a wiring fix. Shape per the maintainer's call: name up front, with mode/compression/lock behind an Advanced disclosure. mode and compression_type are both required by StoreBackupRequest with no config default, so the form always sends them -- snapshot (no downtime) + zstd (Proxmox's modern default) are what a user who never opens Advanced gets. Mode copy names the consequence, since "Kill" stops a running guest. Adds a Base UI Collapsible primitive rather than reusing the Radix Accordion: "Advanced" is a single disclosure, not a multi-item accordion, so Collapsible is the better fit and it avoids adding a new Radix consumer against the hard Base UI requirement.⚠️ Base UI's attributes are NOT Radix's -- the trigger gets `data-panel-open` (nothing when closed), the panel `data-open`/ `data-closed`. Radix's `data-state=open` matches nothing, and Tailwind never errors on a class that matches nothing. Verified against the rendered DOM. Surface the API's curated message on failure instead of a generic toast: these errors are actionable (the throttle names its window; a node with no backup-capable storage says so) and handleFormErrors only maps 422s. Follows the existing NameserversCard/DeleteBlockGroupModal idiom. The page-level create action hides when the collection is empty so it does not duplicate the empty state's, mirroring node Storages. Verified in-browser at desktop + 390px: POST body is {name, mode: snapshot, compression_type: zstd, is_locked: false} with Advanced untouched; the disclosure toggles data-panel-open and rotates its chevron; exactly one create action in each of the empty and populated states; the drawer renders with no overflow; and a real backend 409 surfaces as "No backup-capable storage is configured for this node." Pest 266, PHPStan zero, tc and build green.
Rebuild was the last server subpage with its own layout model. It wrapped itself in `flex flex-col gap-y-6` -- which made the whole page one flex child, so AppLayout's gap-2/@md:gap-4 never applied between the heading and the card, and the page then imposed its own spacing. It also capped its card with a bespoke max-w-xl. Use the same one-column-of-two grid as every sibling instead: the card now measures 560px on desktop and 358px at 390px -- identical to the Security page's cards -- with no magic number and no wrapper. Rebuild was also the ONLY server subpage missing route `meta`, so its document title never resolved; it now reads "Rebuild | <server> | Convoy". Skeletons: the submit and password rows drop to the nova h-8 control height, but the OS-family and template rows deliberately stay h-10 -- those triggers are `h-auto` rich selectors, which the audit lists as an intentional exception, so a blanket h-8 would have made the skeleton less accurate, not more. Browser-verified at desktop + 390px, no overflow and no console errors. (The Security page's 500s seen while comparing are the documented fake-node limitation -- settings tabs read live Proxmox -- not from this change.)
The audit's rule is to update this primitive BEFORE it gains consumers, so a future screen inherits something correct rather than the old chrome: it still had `border-primary`, `shadow-xs`, and a one-pixel focus ring. Chrome now mirrors the shared Checkbox — `border-input` at rest, no shadow, `ring-3` focus, invalid/disabled treatment, and the same `after:` hit-target expansion. Being dormant, it had no screen to verify on, and "compiles clean" is not evidence for Base UI — a ported Radix selector fails silently. Verified through a throwaway route instead (removed again in this commit): `role=radiogroup` / `role=radio`, correct `aria-checked`, `data-checked`/`data-unchecked` (NOT Radix's `data-state`), primary fill and border resolving to the blue primary when checked vs `border-input` when not, `data-disabled`, indicator present only when checked, plus click selection and ArrowDown roving. No console errors. `@radix-ui/react-radio-group` had no consumers left and is uninstalled. Note TanStack Router ignores `__`-prefixed files, so the probe route had to be named without that prefix to register at all.
Two Select-inside-dialog bugs were pre-existing and reproduced on /admin/tokens,
a screen this work never touched (both token UIs were build/type-verified only,
never clicked, which is why they were missed):
- Escape closed the whole dialog, discarding the user's form.
- Keyboard nav was dead: focus never entered the popup, so arrows and
typeahead did nothing. Mouse-only.
One root cause: Radix Dialog and Base UI Select are two dismissal/focus systems.
Base UI portals its popup outside the dialog, so Radix's focus trap pulls focus
back; and both listen for Escape on document in the CAPTURE phase, Radix first,
whose only cancel lever (defaultPrevented) Base UI also respects. No shim leaves
both libraries' handlers intact -- the fix is to stop mixing them.
So: Dialog, Drawer, and Sheet all move to Base UI, and both bugs are fixed at the
root with ZERO shim code. Verified on the previously-broken screen: focus now
enters the popup, ArrowDown highlights, Escape closes only the select, and a
second Escape closes the dialog.
vaul is gone -- Base UI ships a first-party Drawer (stable since 1.3.0; we were
already on 1.6.0 with it unused), which is also where shadcn's own drawer went.
No need for the vaul-base community port or a hand-rolled one.
Credenza -> ResponsiveDialog. The concept was right (it is what shadcn documents:
Dialog on desktop, Drawer on mobile) but the implementation called useMediaQuery
in all eight parts -- eight subscriptions to one breakpoint. It now resolves once
at the root and shares via context, and because both families are Base UI every
part is a straight alias with no adapter. asChild -> render across 47 files,
per shadcn's own migration guidance.
@radix-ui/react-dialog and vaul are uninstalled. Main JS 378.75 -> 359.42 kB
(gzip 115.92 -> 109.18).
Verified at desktop + 390px: dialog, drawer (responsive swap picks Drawer at
390px), sheet with a render= trigger, and the Ctrl+K command dialog all open,
close on Escape, and show no console errors or overflow.
Base UI deliberately renders NO backdrop for a nested dialog so the parent stays cleanly visible underneath — which means the parent itself has to signal depth. It now scales back once per open descendant, reading Base UI's --nested-dialogs count. Groundwork for rebuilding the security auth flow on real nested dialogs instead of the modal store's unmount/remount queue.
The modal store simulated a stack: opening a second modal set activeModal to null, waited a hardcoded setTimeout(250), then mounted the next. That unmount/remount IS the backdrop flash, and the delay existed only to hide the seam. On top of that it hand-rolled a modalQueue, a middleware (shouldContinue + fallback id), backOutFromMiddleware and pushToQueue purely to gate the security page behind an auth prompt. All of it is gone. The gate is now a real nested dialog: <AuthDialog> renders INSIDE the dialog it guards and opens itself while identity is unconfirmed. Base UI deliberately renders no backdrop for a nested child, so the parent stays mounted and visible underneath and there is nothing to flash; the parent signals depth by scaling back per --nested-dialogs instead. Confirming identity simply flips the child closed and reveals the parent. createModalStore keeps only activeModal/modalData/openModal/closeModal, which is all the six row-modal stores (ipam x3, nodes x2, template-groups, locations) ever used — they inherit the no-flash behaviour for free. pushToQueue callers become a plain openModal, since openModal already replaces the active step. Verified by driving the real gate on /security with the identity store cleared: opening Authenticator gives 2 dialogs but ONE backdrop, titles ["Authenticator", "Authorization Required"], parent data-nested-dialog-open with --nested-dialogs: 1. After confirming: 1 dialog, --nested-dialogs: 0, parent never unmounted. The backdrop count stays 1 across the whole transition — it was 1 -> 0 -> 1 before, which is what flashed. No console errors.
Two useEffects each auto-submitted the passkey ceremony — one keyed on the dialog opening, one on the type changing. Both of their *initial* invocations run when the gate mounts already-open, so the ceremony fired twice and the second attempt 403'd on an already-consumed challenge. The first attempt won and the error toast was swallowed, which is why it looked fine. This was survivable while AuthDialog was a page-level sibling that mounted with the gate closed (the guard blocked both initial runs, and only the open-keyed effect fired later). Nesting it inside the dialog it guards means it now mounts open — so the previous commit is what made this actually fire. Collapsing the two into a single effect keyed on both values fixes it: React coalesces a simultaneous open+type change into one run. Verified with a CDP virtual authenticator, gate opened with Passkey preselected: exactly one GET passkey-authentication-options and one POST identity/confirm (was two of each, one 403), no API failures, gate closes and reveals the parent.
Ticks only what is actually established, with how it was verified. The three left open say why, and the accessible-names check was done against the real DOM rather than by grep.
Attaching a storage to a node 500'd:
SQLSTATE[42703]: Undefined column: column "id" does not exist
... ("storage_id", "node_id", "backup_order") values ($1,$2,$3) returning "id"
storage_to_node is a composite pivot with no `id` column, but StorageToNode
extends the standard Eloquent Model, so $incrementing defaulted to true and
Eloquent appended `returning "id"`. The same assumption broke save() in the
update path, which resolves the row by storage_id and then saves against a
primary key that does not exist.
Postgres-only, which is why v10's move off MySQL exposed it, and reachable only
against a LIVE node — the store endpoint runs right after fetching real storages
from PVE, and seeders write the pivot directly, so nothing else exercised it.
storage_id is the key column updateBackupOrder() already passes to
setNewOrder(), so the model now says so.
Found by driving the audit's outstanding "verify Disks against a live seeded
node" item against the real us-southeast-2 PVE 9.2.2 node. Verified there:
attach now returns 201 with live figures (size 107467505664 = the node's real
100GB `local`). Pest 266, PHPStan zero.
…casing Several strands that had accumulated in the tree together. They do not share a subject, so they are described one at a time. An account can now edit its own profile and upload an avatar. Nothing the browser sent is ever served back: every upload is decoded, cropped square, scaled to at most 512px and re-encoded as WebP by GD, so the bytes on disk are produced by the panel rather than the uploader and a polyglot that is both a valid PNG and a valid script does not survive the round trip. Decoding refuses anything past 50 megapixels, because the dimension is what costs memory and a 200KB PNG can claim 30000x30000. Stored files are named after the hash of their own bytes under the account's uuid, so replacing an avatar changes the URL and the old one can be cached forever. Admins get a read-and-revoke view of everything that can be used to sign in as an account -- passkeys, SSH keys, API tokens, OAuth connections, 2FA. Nothing there mints a credential on someone else's behalf, since an admin who could add an SSH key to another account could take it over silently, which is a different power from being able to take access away. The routes are scope-bound to the user, so a key belonging to another account 404s before the controller runs, and revocations record as the existing account.* audit events rather than a parallel admin.* catalog that would split one account's security history in two. IPAM learns to say how full something is. One AddressCapacityData reading is shared by the pool row, the pool header and the block page so the three cannot disagree about the same block, with the drawing left to features/ipam/capacity.ts the way the storage figures already work. Sparse blocks report a null total rather than zero -- 2^64 units does not fit in a PHP int and a percentage of it would mean nothing -- and a null means "draw no meter". Addresses also gain bulk reserve, release and delete, capped at a page of the table; a hand-made selection routinely contains rows the action does not apply to, so those are skipped and counted rather than failing the batch, and the toast says how many. storages stops storing what it holds twice. The seven stores_* booleans were a projection of pve_content written by the same code at the same moment, so adding the import type meant touching a migration, an enum case, a cast, two DTO fields, a controller mapping and a factory, any of which could have drifted. They are accessors over pve_content now, with querying through Storage::scopeStores(), and the API still exposes storesIso and friends. Rows registered by hand are backfilled before the columns go, so flags an operator set before discovery ever ran are not lost. image_versions.size becomes size_bytes. Every other size column in the schema is mebibytes on disk and bytes through StorageSizeCast; this one is raw bytes on purpose, because it is compared against the byte length of a downloaded file and its sibling virtual_size is the floor a plan's disk has to clear, which the cast's flooring would under-report by up to a mebibyte. Opting out of the convention is fine; doing it silently under a name identical to its neighbours is not. ISO stops being spelled Iso. The acronym is uppercase everywhere else it is written, so the classes, namespaces and components follow -- ISOController, ISOService, ISOData, CreateISOModal and the rest. Wayfinder generates its action files from the PHP class names, so features/isos/api.ts had to follow the rename; it was still importing Admin/Isos/IsoController, which stale generated output masked until the tree was regenerated and the build broke. Also includes TmpChainSeeder and TmpInstallSeeder, scratch seeders for driving a server through deployment states by hand.
Both lines were registered twice. Laravel keeps the last definition, so the routes resolved and nothing broke, but route:list showed each of them twice and the next reader would have had to work out which copy was authoritative.
Three variants were added to give auth its own typographic voice, and all three are removed again. `Input variant="underline"` was designed against a 316px mockup with a placeholder in every field. At the size the app actually renders it — a 480px card with the fields empty — a 1px border-input rule sat 40px below its label (FormItem's gap-2 plus the input's own h-8) with no box to show where the field began, and read as a blank line on a paper form rather than a control. `FormLabel tone="mono"` and `CardTitle size="display"` followed it out. Each was defensible alone; together they made a screen that no longer read as the same app, which is the thing the consistency rule exists to stop. What stays is structural rather than stylistic: - the auth shell drops lg:w-[30rem] and stays at sm:w-96, since the card holds two 32px fields and a button and the extra 96px was dead width under any design - the login footer holds the submit and the passkey button, rather than the submit, a divider, the passkey button and every provider, which is what made the tinted band taller than the fields above it - providers move above the fields, so an account that signs in through one never reads past a form it cannot submit - the title stops reaching for text-3xl by hand CardTitle is hand-reverted rather than restored from f8058f6: its `as` prop landed separately, and a checkout of that revision would have taken it back out and broken every caller passing one.
The dot only ever carried the state in its colour, and the ring around it animated forever to report a value that changes twice a month -- motion belongs on a transition, not a resting state. Play and stop are the shapes already sitting on the power controls directly above the tile, so the state now reads by form as well as by hue.
`Model::boot()` registers a `saving` listener that validates, and it returned `true`. Eloquent dispatches model events with `until()`, so a non-null return halts the chain -- and this listener is registered first, by every model in the app. Anything a subclass hung off `saving` afterwards silently never ran. Not a warning, not a failure: the code simply did not execute. `ImageVersion` was the only model that had done so, and it lost two things. Its `size` column was never written, and neither was the `version_major/minor/patch` triple that `latestVersion()` sorts on -- so "the newest build of this image" was returning an arbitrary row. Both now hang off `creating`/`updating`, the pattern every other model here already uses, with a note saying why. Validation still refuses a bad save by throwing, which is the only signal the return value was ever standing in for. That path had no test at all, so it has one now, along with a guard that a subclass listener registered after it fires. Also puts `image_versions.size` back on the convention its siblings follow -- mebibytes stored, bytes read, via StorageSizeCast. It was briefly `size_bytes` on the grounds that its exact value mattered; it does not. The column feeds a progress bar's total, the verification that needs exactness is the sha256 that Proxmox checks, and residency is answered by a hash-derived filename. The floor that genuinely must stay exact is `virtual_size`, which lives in the disks JSON where no cast reaches anyway.
…et cropped Profile and Security now sit in the client sidebar under an "Account" group rather than behind a drilled-in section of their own. The account is the same account in both workspaces, so a section with its own shell needed a back link with no fixed destination -- and the rail it left behind held a single item. Grouping them here is what keeps "Profile" from reading as a sibling of "Servers"; the admin console reaches the same pages through the avatar menu, the one control both workspaces share. `/security` still redirects. An uploaded picture is now framed before it is sent. The dialog works out which square of the original the user chose and posts those coordinates, so the panel cuts and re-encodes once -- cropping in the browser would encode it twice, for a worse result. A crop that lands outside the picture is pulled back inside rather than refused: the browser measures a scaled copy and can disagree with the decoded original by a pixel at the edge. Sending no crop still centres it. The profile page is one card of settings rows instead of two cards of mostly nothing, built from nova's own Field primitives. `FormItem` and `InputForm` take nova's `orientation`, reusing its exported `fieldVariants` rather than a second copy of them; `vertical` keeps the classes it had, because nova's add `*:w-full` and CheckboxForm and SwitchForm put a checkbox and a switch in that slot. Also: the avatar fills its header button rather than sitting inset from it.
Three boxed checkboxes holding one word each spent a whole content column saying nothing, and a switch beside a bare "Display name" never said who was being allowed what. Divided switch rows instead: the card title names the audience, each row carries the consequence of turning it off, and the control sits at the edge the eye already scans to. No card nested inside a card.
…nd a table Both address surfaces drew a bordered table with a tinted header strip inside a card that is already a bordered surface — three boxes to hold two columns, with a header band spending a row to label a gateway and an em dash. The frame goes; the columns stay, because a stack of records is what columns are for. `CardTable` is that shape as a component: unpadded content so rows bleed to the card's edges, `pl-4`/`pr-4` edge cells, no last-row rule, no fill behind the header, columns hugging their content with one taking the slack, an optional sticky scroll cap, and a scroll box so machine values never overhang the card. Half a dozen cards had derived those details separately; five more can move onto it (see docs/card-design.md). What the two address cards now decide is only which columns exist: - A fact every address shares is said once under the list and loses its column, per fact rather than per card. A server handed a run of addresses off one block showed one gateway seven times; it now shows it once. - What varies keeps its own cell. Joining gateway and MAC into `via x · y` looks like a column and isn't — every MAC starts wherever its gateway ended. MAC group headings go away for the same reason hoisting exists. - The version chip yields to real data: an address announces its own version, so it earns a column only when nothing else competes for the width. The overview cuts its list at five rows and links to the tab, which keeps the filter (now in `CardAction`), the count (now the card description) and the sticky cap that stops a ninth address pushing the nameservers form down.
The block page could tell you what an address is; it could not tell you that
.88 through .90 came free in the middle of an otherwise full /24. That is the
question a subnet is usually opened with, and twenty rows a page is the wrong
instrument for it.
The map is one cell per allocatable unit, in address order, coloured by the
same four states the list and the counts use. Cells are placed by unitIndexOf
rather than by row order: generation writes in address order, but a single
deletion would shift every later cell if position were inferred from the
sequence, and a map that puts a hole in the wrong place is worse than no map.
Selection is the point of it being interactive. Clicking takes an address,
shift-clicking takes the run between, and the selection drives the same bulk
route the table's checkboxes do -- so "reserve .2 through .10 for
infrastructure" is two clicks rather than nine trips through a row menu. Only
materialised units can be selected; there is nothing to act on until an address
row exists, which is also why ungenerated cells are drawn fainter than free
ones. Free is an address the allocator can hand out now; ungenerated is one that
does not exist yet, and collapsing that difference would have the map promise
capacity the block cannot deliver.
The server refuses to draw what it cannot draw honestly. A sparse block has no
bounded run of units, and above 4,096 the grid stops being a picture, so both
come back flagged and the view switch is not offered at all -- a toggle that
leads to an explanation is worse than no toggle. Cells are a fixed 14px rather
than a share of the card: stretched to fill, a /24 renders 40px tiles that the
eye reads one at a time, which is what the list already does well.
AddressBulkActions now takes {id, kind} rather than whole address records. The
table and the map select the same things by different routes, and neither had a
reason to hand over full rows so the component could re-derive a state it had
already been told.
…he design Four things the redesign specified and the implementation had not done. Assignment is inverted. Attaching a server was an edit on a row you first had to find: open the block, page to the address, open its menu, pick a server, save. The question is almost never "what shall I do with .86" — it is "this server needs an address", and the block already knows which one is next. The dialog opens on the next free address and asks only for the server, with the remaining free addresses behind a select for the case where a particular one is wanted. No new endpoint: the list already sorts by address and filters by state, so the next free address is its first row. The map and the list are one filter now, not two. The table owns the state as a react-table column filter (which useDataTable already folds into filter[state] on the request) and the map reads and writes that same entry, so switching views never silently changes what is being shown. System reservations state their rule instead of implying a gap. The menu on those rows used to open with Edit and Delete and no Unreserve, which reads as a missing feature; it is now closed, and says why — network, broadcast and gateway exist so nothing else can take them. `Actions` grew a `disabledReason` for this, since a menu that is shut for a reason is a general case and the reason belongs on the trigger as its tooltip and label. Those cells are hatched on the map for the same reason: the texture says "not a colour you can act on" before anyone has hovered anything. Drag-select works, which is what the design asked for; shift-click stays, because it is the accessible equivalent and costs nothing to keep. The release is watched on the window, so a drag that ends outside the grid still commits, and the click that follows a drag is suppressed so it cannot undo a cell that the drag just took.
…es them Five faults, four of them in the map I had just added. Bulk actions rejected anything over 200 ids. That ceiling was written for the table, which selects twenty rows a page; the map then shipped with a drag that takes up to MAX_UNITS in one gesture and broke it on first use. The cap now belongs to the widest surface that can select rather than the narrowest. A batch that large would also have written every address into the audit entry, so past fifty it records the range instead — the entry is there to be read. Capacity never refreshed after a bulk action: delete a run and the header still claimed every address was generated, with Generate disabled, so there was no way to put them back. The invalidations were correct and simply never matched. Every IPAM hook cast `useParams` to `number` while the router hands back strings, so the queries were keyed "1" and the invalidations keyed 1. The casts now coerce. Ungenerated cells had no address, so the map labelled whole rows "#32" — an offset, which is not a thing anyone can act on. A unit is a real position in the block whether or not a row exists for it, so the server now derives its address either way (`unitAddressAt`, the inverse of `unitIndexOf`) and the labels and hover text read as addresses throughout. A selected cell's ring is drawn outside the cell, so the scroll container shaved it off at the edges. `clip-slack` is what the codebase already has for this; card-design.md says not to re-solve it at the call site, and this is the call site that should have used it. Dismissing a combobox inside a dialog closed the dialog with it. The list portals out of the dialog and lays an interaction layer over it, so the press that shuts the list reads to the dialog as a press outside itself. Dialogs now ignore outside presses while a descendant popover is open — the innermost layer owns that gesture. Escape was already correct and is untouched. This fixes every combobox-in-dialog in the app, not just the assign one. Also: a selection of only-assigned addresses supports none of the three actions, and rendered an empty toolbar next to "6 selected". It says what it is now.
Three faults, all in what the map and the meters do to the page around them. The utilisation cell was pinned to w-40, so "253 not generated" broke across two lines mid-phrase in a column that had room to spare. The column declares its own width now, and the reading is a row of whole facts — each one non-breaking — so a narrow viewport wraps between them instead of inside one. Generating addresses left the grid showing the block as it was. The counts were invalidated but the map was not, and generation is the one action whose entire point is to turn ungenerated cells into addresses — the grid is what the reader is watching while it runs. Dragging a selection highlighted the row labels and the hint text under it. The grid was only `select-none` once React knew a drag had started, which is a frame or two after the pointer has already crossed something. It never wants a text selection, so it never asks for one.
Fixing the utilisation column by pinning it to 260px just moved the problem: it took its room out of every other column, so "In use" split across two lines and "2 / 254" read as two numbers stacked. The column has a floor now rather than a fixed width, and headers and atomic values never wrap — when the columns really do not fit, the table's own container scrolls, which is legible in a way a broken-up number is not. That still left six columns on the block table, two of which were repeating the other four. "Hands out" stated the unit total that "In use" was already dividing by, and Gateway is stated in the block's own header a click away — between them they pushed utilisation off the edge at around 1000px, which is exactly the column an operator opened the page for. The geometry now sits under the CIDR it describes and the gateway column is gone: four columns, and at 1000px nothing wraps and nothing scrolls.
Every screen in the panel is Nova, and then mail arrives looking like stock
Laravel: the markdown mailer renders through a theme that knows nothing about
`app.css`, so the one surface a user sees outside the app is the one surface
that does not match it. Restyling that theme by hand would have meant a second
palette to keep in step, which is the arrangement that drifts.
So the tokens are generated instead. `emails/scripts/build-tokens.mjs` reads
`resources/scripts/app.css`, follows the `var()` chains, and emits an `@theme`
for Maizzle. Three things have to happen on the way, none of which are
cosmetic. OKLCH is unsupported outside WebKit clients, so every value is
resolved to hex. Custom properties are stripped by Gmail, so nothing may
reference one at runtime. And the tokens that carry alpha are composited
against the surface they sit on, because Outlook renders `rgba()` fully
opaque: `ring-foreground/10` would arrive at full strength and draw a black
box where the card's edge should be.
Dark mode cannot be done the way the app does it, and this took a build that
silently emitted nothing to notice. In the app, `.dark` swaps the value behind
`--card` and the utilities follow, because they read the variable at runtime.
Maizzle resolves those variables at build time, which is the entire reason the
palette is usable in an inbox at all, so by the time the email exists there is
no variable left to swap and a media query redefining `--color-card` compiles
away. The dark palette therefore ships as a parallel set of tokens applied
through `dark:`, which lands in a `<style>` block because media queries cannot
be inlined.
Two things deliberately do not port. Body text goes from 14px to 16px, and the
card padding follows from 16px to 24px: iOS Mail and Gmail silently upscale
anything smaller, so holding Nova's absolute numbers loses the scale either
way, and holding the ratio is the closest thing to keeping it. The button
grows from 32px to about 46px, because `h-8` is a pointer target in a dense
admin panel and 44px is the iOS minimum for a thumb. Its fill, radius and
weight are unchanged.
The build writes `.blade.php` straight into `resources/views/mail`, so there is
no intermediate output and nothing to keep in sync: the file Laravel renders is
the build output, and it is committed so a deploy never needs Node. Blade
survives compilation as ordinary text, including the `@if` that decides whether
the password notice shows an address, on the condition that every binding is a
flat scalar. `{{ $server->name }}` would put a `>` inside a Vue interpolation
and reach the recipient entity-encoded, so the callers flatten their view data.
`UserInvited` and `PasswordChanged` move off MailMessage's line/action builder
for the same reason the theme had to go, and the connection test now echoes the
settings it used, since the useful answer is not that something arrived but
that these credentials are the ones the relay accepted.
The new tests render the views for real. Every other mail test in the suite
runs under `Mail::fake()` and never touches a template, so a view with a
typo'd variable passes all of them, and since these views are build output the
thing most likely to break them is a rebuild. They also assert that no
`oklch()` or `var(--` reaches a view, which is what a regression in the token
pipeline looks like from the outside.
The account policy from #140 governed name, email and password but left the profile picture self-service, which is the one field a deployment mirroring an upstream directory would most visibly end up showing twice. Removal is gated as the same capability as upload, not a lesser one: with the switch off, what the account shows is the operator's to decide, and clearing it is as much a change as replacing it. DELETE /account/avatar was taking a bare Request, so it needed a form request of its own to be gated at all.
Mail is the one dependency an operator cannot debug from inside the panel. A wrong password or a rejected From address fails on a queue worker, hours later, silently, and the only way to find out was to change a variable and redeploy. So the credentials move onto a settings screen that can test them against the real relay before saving them, which is the whole reason for moving them at all. MAIL_* is read exactly once, by a migration that imports it. The first draft cascaded instead -- panel settings, then the environment -- and that produced a permanent banner on the screen explaining which of the two was winning, which is the architecture leaking into the UI. Importing collapses the tiers: the form is the truth, so an empty host means nothing is set rather than that something might be set somewhere else, and there is nothing left to explain. An install that upgrades finds its existing values already in the form and its delivery unchanged. The trade is that MAIL_* stops being consulted afterwards; the migration skips non-SMTP transports and Laravel's own 127.0.0.1:2525 fallback so an install that never configured mail does not come out looking configured. The test sends a real message through the submitted body, synchronously, and passes the transport's own words back on failure. 535 auth rejected, 550 sender refused and a TLS negotiation failure are each a different afternoon, and none of them surface on a socket connect -- the last needs a real From address to provoke at all. Testing the unsaved form is what makes it worth pressing. The password is write-only across the API and encrypted at rest, the first secret to live in the settings table. Omitting the key keeps the stored value and sending an empty string clears it, so changing a port does not mean retyping a credential the screen never showed. Nothing about it is ever written to the audit log. The override hangs off the mail manager rather than running at boot. It is a database read and almost no request sends anything, and resolving settings during boot pins them before the rest of the process has finished setting the database up -- which is how the first test in a suite ended up holding values from before its own migrations ran. One trap is documented in place. A docblock without an @var tag on a settings property resolves that property's cast to null, because PropertyReflector reads the type only from the tag and never falls back to the native one. The raw string then hits the typed property during fill(), and the TypeError aborts hydration for the whole group while naming whichever property happened to be first.
#55 asked for credentials to be emailed, the way Pterodactyl does it. This is the answer to that request, and it is deliberately not what was asked for: an emailed password sits in a mailbox, a provider's storage and a relay's logs long after it has been changed, and it cannot be revoked. A link expires, can be recalled, and leaves nothing behind that still works once it has been used. Creating a user with a blank password invites them instead. The account is made with 64 random characters nobody has ever seen, so it exists but is unreachable until the invite is redeemed, and the person on the other end chooses a password the panel has never held. The row stores the sha256 of the token and never the token, for the same reason Sanctum does -- a leaked database should not hand out working sign-in links. One live invite per account, enforced by a unique index rather than by the service, so re-issuing invalidates the previous link even when two admins press the button at the same moment. That is what makes "resend" and "revoke a forwarded link" the same operation. The response carries the link even when the panel also emailed it. Mail is not proof of delivery, plenty of self-hosted installs have no relay at all, and an invite that could only ever be emailed would make user creation impossible on those. The admin gets a copyable link and is never blocked by a mail configuration, which is what keeps this strictly better than emailing a password rather than merely different. Unknown, spent and expired tokens all answer identically, and the screen shows one explanation for the three, because telling them apart would confirm to whoever guessed a token that it once meant something. Redeeming signs them in, since the alternative is bouncing someone who has just proved they hold the invite to a login form to retype what they typed a second ago. The invited password is held to the same policy as any other, so an account an operator hands out is not held to a weaker bar than one whose owner changes it later. The routes sit outside the guest group on purpose. The token is the credential, so it decides what happens rather than the session: inside guest, an admin opening the link they had just minted to check it would be bounced to the dashboard, and a second attempt to spend a link would answer with a redirect instead of saying plainly that it is dead. The warning that mail is off lives on this screen rather than on the mail settings screen. Nobody is harmed by unconfigured mail while looking at the form that configures it -- an empty host already says so there -- and this is where an action quietly does less than the operator expects.
The bug and feature forms carry GitHub's type property instead of the bug and enhancement labels; not confirmed stays on the bug form as a triage label. Placeholders, the host OS field and the log instructions catch up with the container deployment, and every field gains a stable id.
Brings the 4.x line's ten unmerged commits onto next ahead of next becoming the trunk. Three of the four fixes turned out to be already answered by next's own architecture, so this merge records that rather than porting code that would contradict it. ISO node scoping (b6d1e27) does not apply. next replaced the per-node ISO rows with a panel-wide library: the row is the ISO, iso_library has no node_id, and getting the file onto a node happens at mount time. The vulnerability main closed -- mounting an ISO belonging to a node you have no server on -- has no referent here, and Server::isos() has no foreign key to hang off. So next keeps its getMedia(), its MediaRequest (which carries the hidden-flag gate that does still apply, covered both ways in SettingsControllerTest), and the scoped-binding opt-out on the two /hardware/isos/{iso} routes, which is now load-bearing rather than gratuitous: with no Server::isos() for Str::plural('iso') to resolve, scoping those routes raises BadMethodCallException. MountMediaRequest stays deleted, and main's cross-node cases are dropped -- they assert a 404 that next deliberately answers 204. Bandwidth null check (37d5ead, a111b1c) is likewise moot. main models unlimited as null; next uses the -1 sentinel, and Server::isOverBandwidth Quota() already excludes it, as does the admin form's -1 handling in settings.lazy.tsx. ServerBandwidthTest covers it directly. Horizon (ce9d371): next is on v5.47.1 with laravel/sentinel already present, past the version main had to upgrade to, so composer.lock keeps next's Laravel 12 resolution wholesale. What does carry over is the scoping test suite, adapted: - RouteScopingTest arrives with the two ISO routes as documented opt-outs and its reasoning rewritten around the panel-wide library. Its backup test now filters to api/client/, since next's admin backup endpoint addresses a backup directly and has no parent to scope through. - BackupControllerTest's cross-tenant cases are real on next and now pass against its schema: is_successful became error_code, and Backup no longer soft-deletes, so the assertions check the row is still there. tests.yml runs on next, main and the new 4.x branch; develop is dropped, having been renamed to main. CHANGELOG takes main's, which is current through v4.6.1. 900 tests pass. The 40 phpstan findings are pre-existing on next, all in files this merge does not touch.
The entry is an accurate record of what shipped on 4.x, but it now sits in
a tree where the code it describes is absent by design: the ISO library is
panel-wide, so nothing scopes {iso} through {server} and the mount routes
opt out on purpose. Left the entry alone and said which model it belongs to,
rather than editing released history to match current code.
…tright The trunk image was tagged `canary`, which is a name nothing else in the stack knows. CONVOY_VERSION is one variable doing three jobs: compose.yml reads it as an image tag, the Dockerfile takes it as the version stamped into config/app.php, and docker/install.sh hands it to raw.githubusercontent as a git ref to fetch the matching compose.yml and env template. Only the third is fussy about the name, and it already carries an escape hatch for `latest` resolving that through the releases API because `latest` "is a published image tag, not a git ref". `canary` has the same problem and no such hatch, so `install.sh --version canary` would have 404'd. A branch name is a real ref, so type=ref,event=branch satisfies all three at once, and it matches the version the build job stamps in, which is derived from the same ref and so already said `main` inside an image tagged `canary`. Also pin flavor=latest=false. metadata-action defaults to latest=auto, which was assigning `latest` by itself; the explicit type=raw line below it looked like the thing in control while being redundant with a rule it does not state. Editing that line would have changed nothing. No image has ever been published from this repo -- the workflow, Dockerfile and compose.yml live only on next, so pushes to main cannot see them and pushes to next do not match the trigger. Nothing to migrate, and the first build will happen when next becomes main. The same absence is why 4.x needs no exclusion here: it was branched from main, so a v4 tag has no workflow to run and cannot claim `latest`.
…again The workflow-status badge queried ?branch=develop. That branch was renamed to main in 6f7b504, so the badge has been rendering "no status" rather than a result. Point it at main, which is what it will mean once next lands there and is already the branch tests.yml runs on. The FOSSA badges were deleted in c9fef60 and are here anyway: that commit is an ancestor of this branch, so they came back through a merge that kept this side of README rather than through anyone re-adding them. Removing them restores the intent instead of leaving the deletion silently undone.
The next release is v5. "v10" was the working name while the rewrite lived on next, and it had reached a filename, a doc filename and one string an operator actually reads: install.sh suggested `--version v10.1.0` when it cannot resolve the latest release, which would have sent someone looking for a tag that is never going to exist. That example is now v5.0.0, a release that will. Renamed v4-to-v10.load and v10-next-handoff.md along with their references in verify.sh, RUNBOOK.md, frontend-overhaul-audit.md and .ddev/config.yaml, so nothing points at a path that no longer exists. The anchor enrollment plan staged itself over "v10.x", now v5.x. No behaviour changes: every hit was prose, a comment, or a path to one of the two renamed files. Both scripts still parse and verify.sh's LOAD_TEMPLATE resolves.
tests.yml still triggered on `3.0-develop`, which is not a branch on the remote; only `archive/3.x` survives from that line. A trigger for a branch nobody can push to never fires. The cutover runbook told the operator to reconcile `develop`'s newer commits before cutover. That branch was renamed to main, and after this reorganization the line it means is 4.x, so the instruction named something that has not existed for two renames. Point it at the maintenance line, and record that the reconciliation is already done through v4.6.1, so the remaining obligation is only for whatever lands on 4.x from here.
Added it in the merge on the assumption that listing a branch here covers it. It does not: GitHub reads the workflow from the commit being pushed, so this file only ever runs for pushes to a branch that contains it, and it will never live on 4.x. The entry read as coverage while doing nothing, which is worse than its absence. 4.x has its own copy of this workflow and now triggers on itself, in 2b2e54e on that branch.
next is being deleted once it fast-forwards onto main, so naming it here would leave the same dead trigger that 4.x had. Doing it before the swap rather than after costs nothing. Direct pushes to next stop running tests from here, but the fast-forward is a push to main, so this tree gets its CI run at the moment it becomes the trunk, which is the run that matters. Pull requests are unaffected either way; that trigger has no branch filter.
Collaborator
Author
|
Opened by mistake: gh inferred the head branch from a checkout sitting on main. Superseded by the fix/attention-card PR. |
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.