Skip to content

feat(devices): add device management#40

Merged
rowan-stein merged 1 commit into
mainfrom
noa/issue-39
Apr 11, 2026
Merged

feat(devices): add device management#40
rowan-stein merged 1 commit into
mainfrom
noa/issue-39

Conversation

@casey-brooks

Copy link
Copy Markdown
Contributor

Summary

  • regenerate buf-generated types for device APIs
  • add devices page and create dialog with enrollment JWT flow
  • add device status formatter/test plus routing/nav updates

Testing

  • npm run lint
  • npm run typecheck
  • npm test
  • npm run build

Fixes #39

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Test & Lint Summary

  • npm run lint (no errors)
  • npm run typecheck
  • npm test (Test Files: 8 passed; Tests: 35 passed; 0 failed; 0 skipped)
  • npm run build

@rowan-stein

Copy link
Copy Markdown
Contributor

Implementation complete. Requesting code review.

Changes (10 files, +979/-31):

  • New DevicesPage with list, search, sort, create, and delete flows
  • New CreateDeviceDialog with one-time enrollment JWT display and copy-to-clipboard
  • formatDeviceStatus utility + unit test
  • Route /devices + sidebar nav link (MonitorSmartphoneIcon)
  • Regenerated proto types with Device support

CI status: Build ✅ | E2E pending | Argos: visual changes expected (new page)

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean implementation that closely follows the existing ApiTokensPage / CreateApiTokenDialog patterns. All spec requirements from Issue #39 are addressed:

  • formatDeviceStatus utility with unit tests
  • CreateDeviceDialog with enrollment JWT flow (one-time display, copy, warning, blocked close)
  • DevicesPage with list, search, sort, delete confirmation
  • ✅ Route added as sibling to api-tokens
  • ✅ Sidebar NavLink with MonitorSmartphoneIcon, before API Tokens
  • ✅ Generated proto types regenerated

One minor comment on the React key fallback in DevicesPage. Otherwise, this is ready to merge.

Comment thread src/pages/DevicesPage.tsx
className="grid items-center gap-2 px-6 py-4 text-sm text-foreground md:grid-cols-[2fr_1fr_1fr_120px]"
data-testid="devices-row"
>
<span className="font-medium" data-testid="devices-name">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] Falling back to device.name as a React key when deviceId is undefined is fragile — device names aren't unique, so two devices with the same name would cause key collisions and rendering bugs.

The ApiTokensPage reference uses token.id directly (a required top-level field), so there's no fallback needed there. Here, since meta is an optional proto message field, consider asserting the ID is present rather than silently degrading:

const deviceId = device.meta?.id ?? '';
// ...
key={deviceId}

An empty string key is at least consistently degenerate and signals something is wrong, rather than silently producing plausible-looking-but-wrong keys.

@rowan-stein
rowan-stein merged commit 2ff00b3 into main Apr 11, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Devices section to user menu (Port Exposure — Stream 7)

3 participants