feat(web): per-page-size selector on Devices list (Discussion #684)#705
Conversation
ToddHebebrand
left a comment
There was a problem hiding this comment.
Two small follow-ups from a comprehensive automated review (code-reviewer + test-analyzer + silent-failure-hunter, parallel). Both shippable, neither blocking — but #1 is a user-visible regression introduced by this PR.
1. Stale currentPage now renders visibly when filters shrink the result set
The pagination-bar visibility guard changed from totalPages > 1 → sortedDevices.length > 0. Pre-PR, if a user was on page 3 (10/page) of 30 devices and filtered down to 5 results, totalPages became 1 and the entire pagination block hid, masking the stale currentPage=3. With the new guard, the bar renders and shows Showing 21 to 5 of 5 / Page 3 of 1 until the user clicks Prev (which snaps things back).
Fix is a one-line useEffect next to where totalPages is computed:
useEffect(() => {
if (totalPages > 0 && currentPage > totalPages) {
setCurrentPage(1);
}
}, [totalPages, currentPage]);2. pageSize prop is now silently a one-time fallback
After mount, effectivePageSize comes entirely from localStorage. The pageSize prop only seeds the initial useState lazy initializer and is ignored on subsequent re-renders if the caller changes it. Worth a docstring on the prop so future callers don't expect it to remain authoritative.
Combined patch
Verified locally:
npx tsc --noEmit(apps/web) — cleannpx astro check— 0 errors, 0 warningsnpx vitest run pageSizePreference.test.ts— 12 passed
diff --git a/apps/web/src/components/devices/DeviceList.tsx b/apps/web/src/components/devices/DeviceList.tsx
@@ -53,6 +53,9 @@ type DeviceListProps = {
onSelect?: (device: Device) => void;
onAction?: (action: string, device: Device) => void;
onBulkAction?: (action: string, devices: Device[]) => void;
+ // Initial page size if the user has no stored preference for this browser.
+ // Once the component mounts, the live page size comes from localStorage
+ // (see pageSizePreference.ts); subsequent changes to this prop are ignored.
pageSize?: number;
serverFilter?: FilterConditionGroup | null;
};
@@ -277,6 +280,17 @@ export default function DeviceList({
const moreFiltersCount = [roleFilter, orgFilter, siteFilter].filter(f => f !== 'all').length + (groupFilter.length > 0 ? 1 : 0);
const totalPages = Math.ceil(sortedDevices.length / effectivePageSize);
+
+ // Clamp currentPage when filters/search shrink the result set below the
+ // current page. Without this, a user on page 3 who narrows the filter to
+ // fit on one page sees "Page 3 of 1" until they click Prev. Pre-PR this
+ // was masked because the pagination bar was hidden when totalPages === 1.
+ useEffect(() => {
+ if (totalPages > 0 && currentPage > totalPages) {
+ setCurrentPage(1);
+ }
+ }, [totalPages, currentPage]);
+
const startIndex = (currentPage - 1) * effectivePageSize;
const paginatedDevices = sortedDevices.slice(startIndex, startIndex + effectivePageSize);Out of scope (filed mentally as follow-up)
- First-ever
DeviceList.test.tsxsmoke test asserting "size change resets to page 1" — worth a separate issue, requires non-trivial mock scaffolding forfetchWithAuthandConnectDesktopButton. The page-reset behavior here is otherwise verified only by your manual test plan.
Otherwise this is a clean PR — pure-utility split, the jsdom + Node 22 localStorage stub doc-comment is exactly the kind of thing a future reader would otherwise strip, and the silent-failure-hunter found the localStorage swallows appropriate at this layer.
ToddHebebrand
left a comment
There was a problem hiding this comment.
Quick correction to my previous comment — switching the clamp from a useEffect to a check-during-render. Same behavior, but no flash of stale UI (the effect would briefly paint "Page 3 of 1" before the post-render setState re-paints).
Setting state during render is React's documented pattern for correcting derived state — React discards the in-progress render and re-runs with the corrected value. Strictly better than useEffect for this case.
Updated combined patch:
diff --git a/apps/web/src/components/devices/DeviceList.tsx b/apps/web/src/components/devices/DeviceList.tsx
@@ -53,6 +53,9 @@ type DeviceListProps = {
onSelect?: (device: Device) => void;
onAction?: (action: string, device: Device) => void;
onBulkAction?: (action: string, devices: Device[]) => void;
+ // Initial page size if the user has no stored preference for this browser.
+ // Once the component mounts, the live page size comes from localStorage
+ // (see pageSizePreference.ts); subsequent changes to this prop are ignored.
pageSize?: number;
serverFilter?: FilterConditionGroup | null;
};
@@ -277,6 +280,16 @@ export default function DeviceList({
const moreFiltersCount = [roleFilter, orgFilter, siteFilter].filter(f => f !== 'all').length + (groupFilter.length > 0 ? 1 : 0);
const totalPages = Math.ceil(sortedDevices.length / effectivePageSize);
+
+ // Adjust currentPage during render when filters/search shrink the result
+ // set below it. Setting state during render is React's documented way to
+ // correct derived state without a flash of stale UI — React discards the
+ // in-progress render and re-runs with the corrected value. Pre-PR this
+ // case was masked because the pagination bar hid when totalPages === 1.
+ if (totalPages > 0 && currentPage > totalPages) {
+ setCurrentPage(1);
+ }
+
const startIndex = (currentPage - 1) * effectivePageSize;
const paginatedDevices = sortedDevices.slice(startIndex, startIndex + effectivePageSize);Verified: npx tsc --noEmit clean, npx astro check 0/0, pageSizePreference.test.ts 12 passed.
Sorry for the noise — please apply this version, not the useEffect one from my prior comment.
…Ops#684) Lets users pick how many device rows to render per page on /devices, persisted to localStorage per browser. Default stays 10 so existing users see no surprise. Options: 10/25/50/100/200. - New apps/web/src/components/devices/pageSizePreference.ts exports PAGE_SIZE_OPTIONS + readPageSizePreference + writePageSizePreference. Allowed-set guard: stored values that aren't one of the 5 options resolve to the supplied fallback (further fall back to DEFAULT_PAGE_SIZE if the fallback itself is invalid). SSR-safe (typeof window guard). Throw-safe (Safari private mode / quota errors swallowed). - DeviceList.tsx adds an effectivePageSize state initialized from readPageSizePreference(pageSize prop). The selector lives in the pagination bar next to "Showing X to Y of Z" so it is visible whenever there is at least one device — even when only one page is needed — per Todd's spec in Discussion LanternOps#684. Switching size resets currentPage to 1 so the user does not land on an out-of-range page. Selector styling matches the existing filter selects on the page (h-10 w-full sm:w-32, same border + focus ring). - 12-case unit test for the helper covering: every allowed option, malformed stored values, out-of-set stored values, out-of-set caller fallback, missing fallback, getItem throwing (Safari private mode), setItem throwing (quota), invalid write rejected without clobbering. Scope per Todd's review: - Default stays 10. - localStorage only for v1; lifting to users.settings is a follow-up. - Other list pages (Sites/Users/Alerts/Scripts) NOT in this PR. - Server-side pagination / virtualization NOT in this PR. Local checks: - npx tsc --noEmit 0 errors - npx astro check 0 errors / 0 warnings - npx vitest run pageSizePreference.test.ts 12 passed
602d774 to
c525a9e
Compare
|
Following up on my review from 2026-05-14 — both items are still unaddressed (no follow-up commit was pushed after the review; branch is still at 1. if (totalPages > 0 && currentPage > totalPages) {
setCurrentPage(1);
}2. Happy to re-review as soon as these land. |
Two items from the re-review on LanternOps#705. 1. currentPage clamp regression: now that the pagination bar renders whenever sortedDevices.length > 0 (vs. only totalPages > 1 pre-PR), a stale currentPage paints visibly when filters/search shrink the result set below it (e.g. "Showing 21 to 5 of 5 / Page 3 of 1"). Apply the check-during-render fix per Todd's suggestion — setting state during render is React's documented pattern for correcting derived state and avoids the useEffect flash-of-stale-UI. 2. pageSize prop docstring: pageSize is now effectively a one-time fallback (after mount, effectivePageSize comes from localStorage). Added a short block comment so future callers don't expect it to stay authoritative. Verification: - npx tsc --noEmit clean. - npx astro check 0 errors / 0 warnings. - npx vitest run pageSizePreference.test.ts — 12 passed.
|
Both items addressed in Blocking — const totalPages = Math.ceil(sortedDevices.length / effectivePageSize);
+
+ // Adjust currentPage during render when filters/search shrink the result
+ // set below it. Setting state during render is React's documented way to
+ // correct derived state without a flash of stale UI — React discards the
+ // in-progress render and re-runs with the corrected value.
+ if (totalPages > 0 && currentPage > totalPages) {
+ setCurrentPage(1);
+ }
+
const startIndex = (currentPage - 1) * effectivePageSize;Nit — Verification:
Ready for re-review. |
ToddHebebrand
left a comment
There was a problem hiding this comment.
Re-review of 61216e06 — both items from the prior round are genuinely and correctly resolved. Approving.
1. currentPage clamp (blocking) — resolved. It's the check-during-render form, not a useEffect, exactly as requested:
const totalPages = Math.ceil(sortedDevices.length / effectivePageSize);
if (totalPages > 0 && currentPage > totalPages) {
setCurrentPage(1);
}Verified safe: sits after all (unconditional) hook declarations with no early returns before it, so hooks order is never violated and the render-phase setState is legal (Lint/Type Check green confirm no rules-of-hooks break). Edge cases hold — filter/search shrink and page-size-shrink both reset cleanly with no stale "Page 3 of 1" flash, and the totalPages > 0 guard makes it self-terminating on empty sets (no infinite render loop). Pagination math (startIndex, Math.ceil, Math.min upper-bound clamp) has no off-by-one.
2. pageSize prop docstring — resolved.
Non-blocking note: persistence is localStorage, not window.location.hash. CLAUDE.md's hash convention targets transient UI state (tabs/selections); a cross-navigation user preference is a different category and hash wouldn't survive navigating to device detail and back. Reasonable documented v1 decision — tracked in #714 only in case a server-side users.settings.devicesPageSize follow-up lands.
CI fully green (25/25).
…) guard (#710) (#713) Closes/implements [Discussion #710](#710). Green-lit by @ToddHebebrand with the role-id-by-name correction folded in. ## Root cause \`updateUserSchema\` (\`apps/api/src/routes/users.ts:73-76\`) was not \`.strict()\`, so the Edit dialog's PATCH body \`{ email, name, roleId }\` silently dropped \`roleId\` before the handler ever saw it. Handler returned 200 with \`name\` as a no-op, \`fetchUsers()\` re-rendered the unchanged row, user appeared to "save" without error. Role only persists via \`POST /users/:id/role\`, which the Edit flow never called. ## Changes 1. **\`.strict()\` on \`updateUserSchema\`** — unknown keys surface as 400 instead of silently dropped. Defense in depth so the next silently-dropped field doesn't reproduce the same class of bug. 2. **\`handleEditSubmit\` in \`UsersPage.tsx\`** splits into: - PATCH \`/users/:id\` with \`{ name }\` only (schema-valid) - Conditional POST \`/users/:id/role\` with \`{ roleId }\` **only when the role actually changed**. \`selectedUser\` carries the role *name*, not the id — resolved via \`roles.find(r => r.name === selectedUser.role)?.id\` per your correction. 3. **PATCH and POST are independent** — role POST errors surface to the user; the name PATCH is not silently swallowed. ## Tests - **\`apps/web/src/components/settings/UsersPage.test.tsx\` (new)**: - "POSTs \`/users/:id/role\` with the new roleId when the role is changed" - "does NOT POST \`/users/:id/role\` when the role is unchanged (name-only edit)" - **\`apps/api/src/routes/users.test.ts\` (added)** under a new \`PATCH /users/:id (admin update)\` describe: - "rejects unknown top-level fields including roleId (strict schema)" — asserts 400 - "rejects an arbitrary extra field (strict schema, defense in depth)" — asserts 400 ## Verification - \`apps/api\`: \`npx tsc --noEmit\` clean. \`vitest run src/routes/users.test.ts\` — **14/14 passed** (12 existing + 2 new). - \`apps/web\`: \`npx tsc --noEmit\` clean. \`vitest run UsersPage.test.tsx\` — **2/2 passed**. - \`pnpm audit --audit-level=critical\` — 1 moderate + 1 high pre-existing on \`main\` (same cross-PR upstream-image blocker tracked in #680/#700/#705/#712); not introduced by this branch.
… client-side (#714) (#1162) ## Summary Resolves the still-valid nits from the #680 remote-desktop launcher review pass (issue #714). Per the issue owner's [triage comment](#714 (comment)), the `#700` `preflight.sh` items are moot and `#705` is tracking-only, so this covers the two open `#680` items — which are coupled (the client reuses the shared validator). ## Changes **1. Allowlist-first scheme guard** — `packages/shared/src/validators/remoteAccessLauncherScheme.ts` - Introduces an explicit, exported `ALLOWED_LAUNCHER_SCHEMES` set (`https`, `http`, `rustdesk`, `teamviewer`, `anydesk`, `splashtop`, `screenconnect`). The guard now allowlists these first, and any off-allowlist scheme is still accepted **only** if it clears the dangerous denylist (`javascript:`, `data:`, `vbscript:`, `file:`, …). - Chosen for **no regression**: partners configure their own custom protocol handlers (the tests include e.g. `bdunn-rustremote://`), so a strict allowlist would break legitimate setups. Accept/reject behaviour is unchanged vs. the old denylist — the value is the explicit, reusable allowlist and clearer intent. **2. Client mirrors the server guard** — `apps/web/src/components/settings/PartnerRemoteAccessTab.tsx` - The inline URL-template validation previously only checked for a scheme's *presence*. It now reuses `isAllowedLauncherScheme` via an extracted, unit-tested `urlTemplateError()` helper, so a partner admin sees a blocked scheme (`javascript:`, etc.) **immediately** instead of only when the server rejects it on save. ## Items already addressed / moot (verified) - `makeProviderId()` already uses `crypto.randomUUID()` (a prior commit, with a `#714` comment). - `EffectiveOrgSettings.remoteAccessProviders` **does not exist** — the field lives on `PartnerSettings` and is actively used; nothing dead to remove. - `#700` `scripts/security/preflight.sh` nits — struck by the issue owner (script not on `main`). - `#705` localStorage persistence — tracking-only by design. ## Tests - Shared: `ALLOWED_LAUNCHER_SCHEMES` is exported and every entry passes; custom off-allowlist non-dangerous schemes still pass; dangerous schemes still rejected. - Web: new `urlTemplateError` suite (empty → no error, missing scheme, dangerous scheme rejected inline, allowlisted/custom accepted). - Suites green: shared **595**, web **806**, API remote-access-launch **6** + orgs **93**; `astro check` 0 errors, lint clean. Closes #714 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes/implements Discussion #684. Green-lit by @ToddHebebrand on 2026-05-14.
What changed
Lets the user pick how many device rows the
/deviceslist renders per page. Default stays at 10 so existing users see no surprise on first render. The chosen size persists tolocalStorageper browser, so the next visit remembers it.Options: 10 / 25 / 50 / 100 / 200.
The selector lives in the pagination bar at the bottom of the table, next to "Showing X to Y of Z". It is visible whenever there's at least one device — even when only one page is needed — so users can pick 25/page even when 10 fit on screen.
Switching size resets
currentPageto 1 so the user doesn't land on an out-of-range page.Files
apps/web/src/components/devices/pageSizePreference.ts(new, ~50 lines):PAGE_SIZE_OPTIONS,isValidPageSize(),readPageSizePreference(fallback?),writePageSizePreference(size). SSR-safe (typeof windowguard). Throw-safe (Safari private-mode and quota errors swallowed). Allowed-set guard: stored values not inPAGE_SIZE_OPTIONSresolve to the supplied fallback, which is itself validated and falls back toDEFAULT_PAGE_SIZE(10) if invalid.apps/web/src/components/devices/pageSizePreference.test.ts(new, 12 cases): every allowed option accepted; out-of-set rejected; missing localStorage entry → fallback; malformed stored value → fallback; out-of-set stored → fallback; out-of-set caller fallback → DEFAULT_PAGE_SIZE;getItemthrows (Safari private mode) → fallback; default fallback when not supplied;setItempersists string; invalid write does not clobber existing valid value;setItemthrows (quota) → swallowed.apps/web/src/components/devices/DeviceList.tsx(modified): addseffectivePageSizestate initialized viareadPageSizePreference(pageSize), replaces the three uses ofpageSizein pagination math, adds ahandlePageSizeChangethat writes through to localStorage and resets the page. The pagination bar now renders wheneversortedDevices.length > 0(previously only whentotalPages > 1) so the selector is reachable even on lists that fit on one page.Scope (matches @ToddHebebrand's review of #684)
users.settings.devicesPageSizefor cross-browser persistence is a follow-up, not in this PR.Selector styling
Matches the existing filter selects above the table:
h-10 w-full rounded-md border bg-background px-3 text-sm focus:outline-none focus:ring-2 focus:ring-ring sm:w-32— same border, focus ring, height, and responsive width.Verification
npx tsc --noEmit(apps/web) — 0 errors.npx astro check— 0 errors, 0 warnings.npx vitest run apps/web/src/components/devices/pageSizePreference.test.ts— 12 passed.breeze.bdunn.com) on top of0.65.15. Verified: selector renders next to the pagination bar at the matching half-width on lg+, switching size re-paginates and persists across reload, picking a value that no longer divides cleanly into the list count resets to page 1, default-10 behavior preserved for users with no stored value.Test plan
/deviceson a fresh browser (no localStorage entry) → 10 rows/page, default unchanged.localStorage['breeze.devices.pageSize']to'7'(out of allowed set) → falls back to 10 silently, no console errors.