fix(ui): give the pending devices count a page to land on - #7048
Conversation
DeviceDetails paired useInstallKeys with resolveEnrollmentSource and repeated the page size the lookup depends on. A second caller was about to repeat both, so the pair moves behind useEnrollmentSource and the page size stops being a literal each caller has to agree on. The lookup is client-side over a single page of keys, so a namespace past that page resolves its tail to null rather than to a name. That limit is why the hook's doc comment states it: it is not visible from the call site, and a caller that assumes otherwise renders a dash. KeyValueChip moves to components/common for the same reason — a second page tree needs it, and reaching across into pages/install-keys/ to get it is not a dependency worth having.
Acceptance moved to the install keys area, which left no list of what is waiting: a pending registration was reachable only by opening each key's activity in turn and scanning it. The dashboard still counted them, so the count named work with nowhere to go. The queue lists by device status rather than by key, which is what lets it agree with the dashboard count — both now answer the same question of the same table. Each row names the key that enrolled it, so the context acceptance needs survives the change of axis. The route is /pending-devices rather than /devices/pending because NavLink matches by prefix: nested under /devices, the sidebar would light up both entries at once, and giving Devices an exact match instead would stop it highlighting on the device detail and add-device pages.
Both links pointed at /devices?status=pending. The devices list hides every status but accepted and forces the parameter back, so the click landed on the accepted list with no error and no explanation — the count on the card was real, the destination was not. AddDevice's link also called the destination a tab. There has not been one since acceptance moved to the install keys area, so the word was describing the page it failed to reach. Fixes: #7043
An effect forced the status back to accepted on every render, so the pending and rejected column sets, their tabs, the status constraint and the handler that switched between them could not be reached. The accept, reject and remove actions went with them: nothing could open the confirmation dialog, so the dialog and the two hooks behind it were dead as well. Keeping them was not free. The pending column set duplicated, line for line, the one the new review queue renders, so the same markup existed twice with only one copy reachable. The tab that used to switch status now links out: to the queue for what is waiting, and to install keys for how devices get admitted. The list is the accepted fleet and says so.
The count fed a dashboard card whose only destination was the device list, and that list excludes connectors. A pending container was therefore counted and then absent from the page the count sends you to, which is the one thing a count next to a link must not do. The comparison is <> rather than IS DISTINCT FROM, and the difference matters: the device list builds its filter from the ne operator, which emits <>, and platform is nullable. A null-safe comparison here would count a row the list does not show, restoring the disagreement this removes. Matching the list exactly is the point, quirk included. Only the pending count moves. The registered count answers the same question for licence and firewall evaluation, so narrowing it would move a limit rather than a label. Pending containers keep their own path: the containers list still has a tab for them.
…container The wizard shows when the device counts are all zero. Pending stopped counting containers one commit ago, and registered and rejected still do, so the counts can now read empty for a namespace whose only entity is a pending container — and the dashboard would offer to connect a first device to someone who already has one waiting. Containers are asked for only once the counts already say empty, so the common path keeps the single stats request it had.
Reviewing a pending enrollment described the key's activity page as the place it happens, "rather than in the device list". That was true when it was written and stops being true one commit earlier: the queue lists every device waiting on a decision across all keys, so the two are now alternatives rather than one correct answer. Troubleshooting still told readers to check a Pending tab, which has not existed since acceptance moved to install keys.
|
Claude finished @otavio's task in 4m 12s —— View job Code Review CompleteReviewed 24 files across code quality, security, testing, language patterns, A few things I specifically verified, since they're the subtle load-bearing parts of this change:
--- · |
|
Documentation preview: https://189ece20.shellhub-docs.pages.dev |
QA validation — run against a live instanceExercised this branch ( 28 checks executed: 28 pass, 0 fail. The server fix does what it claimsSeeded a namespace with 4 pending devices, one of them
So the count and the list it is read alongside disagreed before and agree now. Still consistent at 13/13 Review queue
Page 2 of the same queue, and the empty state in a namespace with nothing pending:
The welcome-screen guard, both directions
Also verified
The add-device page's link into the queue, on a non-codeless install method:
Devices list no longer status-filters (
|
| pending rows instance-wide (2 namespaces, 2 of them containers) | 15 |
pre-fix query (status='pending' alone) |
15 |
post-fix query (excluding connector) |
13 |
GET /admin/api/stats → pending_devices |
13 |
So the admin surface carries the same correction, not just the namespace-scoped one. No license was needed:
internal/admin/routes/routes.go lists StatsURL under licmiddleware.SkipLicense, and the sibling
/admin/api/auth/token returns 402 on the same unlicensed instance, which confirms the exemption is what
let stats through.
Install-key lifecycle, device removal, live SSH, and the repo's own Vitest/Go suites were out of scope
for this run.
|
#7070 merged and closed #7043, taking a different route: instead of a page listing pending devices across keys, the install key list now says how many devices each key has waiting, in the usage column, and the dashboard card lands there. Two things from this PR that were right and that #7070 did not carry: The dead code. The |
2 participants
What
Adds a review queue at
/pending-deviceslisting every device waiting on a decision across allinstall keys, and points the dashboard card at it. The card's count and the list are now the same
number from the same table.
Why
The Dashboard's Pending Devices card linked to
/devices?status=pending. The devices list hidesevery status but accepted and an effect forced the parameter back, so the click landed on the
accepted list with no error and no explanation — a real count with no destination.
Acceptance had moved to the install keys area, which left no list of what was waiting: a pending
registration was reachable only by opening each key's activity in turn and scanning it.
Closes #7043
Changes
dashboard count. Each row names the key that enrolled it, so the context acceptance needs
survives the change of axis. Route is
/pending-devices, not/devices/pending, becauseNavLinkmatches by prefix — nested, the sidebar would light up two entries at once, and anexact match on Devices would stop it highlighting on the device detail and add-device pages.
tab, which has not existed since acceptance moved.
the accept/reject/remove dialog unreachable, and the pending set duplicated the new queue line
for line. Removed; the tab now links out instead of switching status.
injects
platform ne connectorunconditionally. Only the pending count moved; the registeredcount answers the same question for licence and firewall evaluation.
container would have been offered a first device it already had.
"rather than in the device list"; the two are now alternatives.
Testing
The count-versus-list agreement rests on matching the list's operator exactly. The stats predicate
uses
<>rather thanIS DISTINCT FROMdeliberately:platformis nullable and the list'snefilter emits
<>, so a null-safe comparison here would count a row the list does not show andrestore the disagreement.
TestPgStore/StatsStorecovers a pending container; the NULL case is notexpressible through the fixture and rests on that reasoning.
Worth probing: a namespace whose only entity is a pending container should still get the dashboard,
not the wizard.