fix(web): UniFi collector-agent pickers — real device names, drift + truncation surfaced (#3121)

[ToddHebebrand]

Problem

Both Collector agent dropdowns in the UniFi integration settings — the self-hosted controller registration form (unifi-controller-agent) and the per-console collector rows under Site mapping (unifi-collector-agent) — listed agents by raw device UUID (e.g. 6eae0f70-8da9-49ff-9e18-c241698975f3) instead of a human-readable name. There was no way to tell which agent you were picking.

Root cause

UnifiIntegration.tsx declared a local AgentDevice interface with name: string | null and rendered {a.name ?? a.id}. But GET /devices returns hostname and displayName — there is no name field in the response. Because that interface is a structural assertion over an untyped JSON body rather than a checked contract, the mismatch typechecked silently and the ?? a.id fallback fired for every row.

Scope

This started as a label-only fix. Two review rounds surfaced further defects on the same code path — all the same failure family, the picker showing the operator something other than the truth — so they were folded in rather than filed separately.

1. Labels (the reported bug)

AgentDevice corrected to mirror the API. agentLabel() renders displayName?.trim() || hostname?.trim() || id, matching the primary device surfaces (DeviceList, DeviceDetails, NetworkChangesPanel).

|| with a trim, not ??: displayName: "" survives both device write schemas (no .min(1), no trim), and a blank <option> is worse than the UUID it replaced — indistinguishable from the — Select agent — placeholder and from every other blank row.

Option value is unchanged (still the device id), so existing saved collector assignments are unaffected.

2. The list was capped at one page — now it walks the cursor

apps/web/src/lib/devicesFetch.ts already exports fetchAllDevices(), the reviewed keyset-cursor walker from #742/#778. This picker was instead taking a single ?limit=500 page.

That matters because the picker filters by site client-side: a partner past the ceiling could not reach their agent at all. An earlier revision of this PR added a "list may be incomplete" notice — but CLAUDE.md targets 10,000+ agents, so that notice would have been permanently lit on exactly the fleets where the picker was useless. A label on a broken control is not a fix. The walk now runs to completion; "truncated" means only the walker's 40,000-row safety ceiling.

Fallout, all improvements:

• AGENT_LIST_LIMIT deleted — it could not prevent the copy drift it was introduced to prevent (the server clamps at DEVICES_LIST_HARD_MAX=1000).
• The { devices: [...] } envelope branch is gone. devicesFetch.ts:139-141 records that no deployed shape returns that key (dropped per feat(web): DevicesPage walks /devices cursor (#742 PR 3b — web) #778 review); accepting it meant a genuine future drift to that key was the one shape the guard waved through. The test that pinned it as valid now pins it as drift.
• includeDecommissioned: false preserves the route's own default — a decommissioned box is not a collector candidate.

3. Response-shape drift was swallowed

The body was unwrapped with an inline ?? chain that collapsed any unrecognized shape to [] while res.ok was true, pushing nothing to failed — an empty dropdown with zero diagnostics, strictly worse than the UUIDs. validateAgentRows() now returns a discriminated union:

Body	Outcome
No rows and no pagination total (error body served with HTTP 200, renamed envelope)	Error banner + warning
Any row lacking id/hostname/displayName — the #3121 shape, arriving as data instead of as a type error	Error banner + warning
Genuinely empty fleet (total: 0)	Silent, on purpose

Two things review corrected here: the check was originally all-or-nothing, so partial drift dropped the bad rows silently (a shorter list and no signal — harder to spot than a total failure); and id was never validated even though it is what the collector save actually submits.

4. AgentDevice.status was declared but never consulted

An offline agent was selectable as a collector with no indication — and a collector polls the controller from the agent, so that silently never polls. Non-online agents are now annotated (Closet (Offline)), translated out of the devices namespace so a German operator doesn't get an English (offline) inside otherwise-localized copy.

Annotate rather than filter: an agent that is down right now is still a legitimate thing to configure, the operator just has to be able to see it.

Tests

23 cases in the file, each pinned by revert:

Revert	Fails
a.name ?? a.id (original)	both label tests
displayName ?? hostname (no trim/||)	both label tests, on the empty-string fixture
flattened row validator	6 drift tests
single capped page	2 cursor-walk tests
flattened agentOptionLabel	the status test

The empty-fleet case keeps passing under all of those reverts — which is what proves it guards against false positives rather than just re-asserting the fix.

The label tests also select a site first, so the select is actually enabled and the a.siteId === siteId filter branch is exercised. A pre-existing mock used a fictional { id, name, siteId } device shape — corrected to the real API shape; that fake mock is why the suite never caught this.

Verification: vitest run src/components/integrations/ src/lib/ → 72 files / 1072 tests passed (includes localeParity, translationCoverage, keyUsage, no-envelope-fallthrough). tsc --noEmit clean, eslint clean. No .astro files touched.

Closes #3121

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying breeze with    Cloudflare Pages

Latest commit:	04ac6cf
Status:	✅  Deploy successful!
Preview URL:	https://e938197b.breeze-9te.pages.dev
Branch Preview URL:	https://fix-3121-unifi-agent-dropdow.breeze-9te.pages.dev

View logs

[ToddHebebrand]

Review run: /pr-review-toolkit:review-pr — code-reviewer, pr-test-analyzer, silent-failure-hunter (parallel).

Findings: 3 raised → all addressed in f0579cf; 0 outstanding.

1. ?? rendered a blank option for displayName: "" — raised independently by all three reviewers, the one consequential finding. ?? only guards null/undefined, and empty string survives both device write paths (updateDeviceSchema / provisionDeviceSchema have no .min(1) or trim, schemas.ts:101,119). A blank <option> is worse than the UUID this PR removes — it's indistinguishable from the — Select agent — placeholder, and the same device reads as edge-01 in DeviceList (which uses ||) and blank here. Now displayName?.trim() || hostname?.trim() || id.
2. The ?? id last resort was itself a silent mask. devices.hostname is NOT NULL and always selected, so that branch is unreachable for a well-formed row — reaching it means response-shape drift, i.e. a recurrence of this very bug, silently. It now warns.
3. Tests asserted against a select the user cannot reach. No site was ever chosen, so the control stayed disabled and the a.siteId === siteId filter branch never evaluated — the fixtures' siteId implied coverage that didn't exist. Both label tests now select the site first (the real journey from the issue), assert the select is enabled, and assert an agent at another site is filtered out.

Regression pinning verified by revert: the two label tests fail against the original name ?? id behavior and against the interim displayName ?? hostname behavior (2 failed / 10 passed each time), and pass on f0579cf.

Tests: vitest run src/components/integrations/ src/lib/__tests__/ src/lib/i18n/ → 24 files / 392 passed. tsc --noEmit clean, eslint clean. No .astro files touched, so astro check not applicable.

Deliberately out of scope (pre-existing, flagged for follow-up rather than widening a label fix):

• The untyped envelope unwrap at UnifiIntegration.tsx (~L417-423, ~L501-507) casts an unvalidated body to AgentDevice[]; on envelope drift it yields [] with res.ok true and no error surfaced — an empty dropdown with zero diagnostics. This is the surface where the next instance of this bug will land; it needs a runtime validator, not a label change.
• /devices?limit=500 truncates with no "showing first 500" notice — same user-visible symptom ("my agent isn't in the list") on large partners.
• AgentDevice.status is declared but neither filter consults it, so an offline agent is selectable as a collector with no indication.

Status: review-clean, awaiting maintainer merge. Not merged and #3121 left open per handoff rules.

[ToddHebebrand]

Scope widened (approved): the three follow-ups I'd flagged as out-of-scope are now folded into this PR rather than filed separately. It is no longer a label-only change — see the rewritten description.

Review run: /pr-review-toolkit:review-pr — code-reviewer + silent-failure-hunter on the widened diff (round 2; the label fix was reviewed in round 1 by code-reviewer, pr-test-analyzer, silent-failure-hunter).

Findings: 8 raised → all addressed in 04ac6cf; 0 outstanding.

The one that changed the design: the truncation notice I'd added was a label on a broken control, not a fix. apps/web/src/lib/devicesFetch.ts already exports fetchAllDevices() — the reviewed keyset-cursor walker from #742/#778 — and this picker was taking a single capped page instead. Since it filters by site client-side, a partner past the ceiling could not reach their agent at all, and at the 10,000+ agent target in CLAUDE.md the notice would have been permanently lit on exactly the fleets where the picker was useless. Now it walks the cursor to completion and "truncated" means only the walker's 40,000-row safety ceiling.

That cascaded usefully: AGENT_LIST_LIMIT is gone (it couldn't prevent the copy drift it existed to prevent — the server clamps at 1000), and the { devices: [...] } branch is gone per the #778 decision recorded in devicesFetch.ts:139-141. My earlier test had pinned that envelope as valid; it now pins it as drift, which is the correct contract.

Others addressed:

• Partial drift passed silently — the row check was all-or-nothing, so a mixed body dropped the bad rows with no signal. Any unusable row is now drift.
• id was never validated despite being what the collector save submits.
• Object.keys(rows[0]) threw on {data:[null]} into a bare catch, losing the diagnostic and aborting unrelated sections of the load.
• Stale truncation notice could outlive a failed reload and assert "cut short" about a fetch that loaded nothing.
• Raw pgEnum in localized UI — status now translated from the devices namespace, with the /* i18n-dynamic */ marker the keyUsage gate requires.

Not taken (deliberate): routing the drift diagnostic to Sentry as apps/web/src/lib/i18n/index.ts:296-307 does. It's a fair point — the console warning only reaches the operator's devtools, not us — but it's telemetry scope beyond the three findings I was asked to fold in, and I'd rather land these well than keep widening. Worth a follow-up if you want drift to page us rather than wait for a ticket.

Tests: 23 in the file (from 9 originally). Every fix pinned by revert — flattening the row validator fails 6, reverting to a single capped page fails 2, flattening the status label fails 1, and the two original label reverts fail 2 each. The empty-fleet case keeps passing under all of them, which is what proves it guards against false positives rather than re-asserting the fix.

vitest run src/components/integrations/ src/lib/ → 72 files / 1072 passed (incl. localeParity, translationCoverage, keyUsage, no-envelope-fallthrough). tsc --noEmit clean, eslint clean.

Status: review-clean, awaiting maintainer merge. Not merged; #3121 left open.