From afce024cacec44cd0b96f7168a93d2cc166f9ecd Mon Sep 17 00:00:00 2001 From: Frederic Cabassut Date: Tue, 18 Aug 2026 12:05:29 +0200 Subject: [PATCH] refactor(wu-003b): one uniform icon per row action, and guard the reveal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to d5c7ab6 after visual review. The 44px Reset button sat beside ~24px siblings, and four text labels wrapped — Delete dropped to its own line at 900px, three lines at 360px. All four row actions are now identical 44x44 icon buttons on one line: shield-check/shield-off, key-round, log-out, trash-2. Each carries a tooltip for a mouse and an aria-label, since an icon alone has no accessible name. In-flight swaps to a spinning loader-circle and an "Resetting…" accessible name, so the e2e assertion reads the accessible name rather than visible text that no longer exists. `flex-wrap` on the strip is what keeps the squares square: flex compresses items only on a line that overflows, so breaking the line means no button is ever asked to shrink. Without it the four flatten to 40x44 at 320px — under the minimum, on a row whose Delete is irreversible. They hold one line down to 336px, exactly 4x44 plus the gaps, and take a second line below that. Correctly sized targets over two lines beat cramped ones on one. A first attempt also put `shrink-0` on each button and credited it in a comment. Measurement disproved that: strip the token and the geometry is byte-identical at every width from 280 to 900, because `flex-wrap` has already spared the buttons from ever shrinking. The dead tokens are gone and the comment now names the real mechanism, while noting that the container's identical-looking `shrink-0` DOES carry weight — from 640px the row turns sm:flex-row, where the truncating name block would otherwise squeeze the strip. Two things stay deliberately as they were. Copy and Dismiss remain text buttons: Dismiss irreversibly destroys a credential the server may already hold, and an icon-only cross beside Copy is the one place ambiguity could cost a password. The blocked-reset reason remains visible in-row text rather than moving into the tooltip, which needs hover the reporter's Android device cannot give. Also closes a gap in the reveal's regression cover. The rule that an unconfirmed outcome REVEALS the generated password protects the only copy of a credential the server may already hold, but only the network-abort trigger was tested. Two more scenarios now pin the lines that decide it: a 5xx carrying a parseable message (guarding `res.status < 500`) and a 4xx with an unreadable body (guarding `.json().catch()`), the latter being what a reverse proxy returns as an HTML error page. DoD items verified: - [x] 1-3. four identical >=44px icon buttons, no wrap at 360px+, per-icon subpath imports at 16-20px - [x] 4-5. Copy/Dismiss still text; reason still visible in-row text - [x] 6. spinner plus "Resetting…" accessible name, aria-disabled - [x] 7-9. no disabled attribute, focusable, no request when unavailable; Dismiss focus return; all 10 data-testids kept - [x] 10. CSS variables only, no hover-only visibility - [x] 11-12. 5xx-with-reason and unparseable-body both reveal - [x] 13. distinct victim emails, no new failed logins (suite: 3 of 5) - [x] 14. no regression in the 22 properties of d5c7ab6 Verified independently by the orchestrator: pnpm check 0 errors / 14 pre-existing warnings; pnpm test:unit 306 passed / 26 files; admin-users.spec.ts 31 passed; full e2e suite 129 passed. Removing flex-wrap fails the new scenario with "Expected: >= 44, Received: 40", reproduced here, so the guard covers the token that actually matters. Reviewed-by: adversarial-review (PASS on the icon work; a second reviewer failed the first attempt for crediting shrink-0 with an effect mutation testing showed it does not have) Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RbqJcFofZSbhbGwUwZP3wU --- src/routes/(app)/settings/users/+page.svelte | 65 ++++++-- tests/e2e/admin-users.spec.ts | 147 ++++++++++++++++++- 2 files changed, 198 insertions(+), 14 deletions(-) diff --git a/src/routes/(app)/settings/users/+page.svelte b/src/routes/(app)/settings/users/+page.svelte index dfcfbda..e2df474 100644 --- a/src/routes/(app)/settings/users/+page.svelte +++ b/src/routes/(app)/settings/users/+page.svelte @@ -3,6 +3,13 @@ import PasswordStrengthMeter from '$lib/components/PasswordStrengthMeter.svelte'; import { canResetPassword, passwordResetBlockedReason } from '$lib/utils/auth-methods.js'; import { generatePassword } from '$lib/utils/password.js'; + import { tooltip } from '$lib/utils/tooltip.js'; + import ShieldCheck from 'lucide-svelte/icons/shield-check'; + import ShieldOff from 'lucide-svelte/icons/shield-off'; + import KeyRound from 'lucide-svelte/icons/key-round'; + import LogOut from 'lucide-svelte/icons/log-out'; + import Trash2 from 'lucide-svelte/icons/trash-2'; + import LoaderCircle from 'lucide-svelte/icons/loader-circle'; let { data } = $props(); // svelte-ignore state_referenced_locally @@ -396,6 +403,8 @@ {#each users as user (user.id)} {@const reset = resets[user.id]} {@const control = resetControl(user)} + {@const roleAction = user.role === 'admin' ? 'Demote to user' : 'Promote to admin'} + {@const resetLabel = control.inFlight ? 'Resetting…' : 'Reset password'}

{user.email}

-
+ +
@@ -426,26 +454,37 @@ data-testid="reset-password-btn-{user.id}" aria-disabled={control.unavailable} aria-describedby={control.reason ? `reset-password-reason-${user.id}` : undefined} - class="inline-flex min-h-11 min-w-11 items-center justify-center rounded-sm border px-3 text-xs transition-colors duration-150 ease-out {control.unavailable - ? 'cursor-not-allowed border-dashed border-[var(--border-subtle)] text-[var(--text-muted)]' - : 'border-[var(--border-subtle)] text-[var(--text)] hover:border-[var(--primary)] hover:bg-[var(--hover-wash)]/10'}" + use:tooltip={resetLabel} + aria-label={resetLabel} + class="inline-flex size-11 items-center justify-center rounded-sm transition-colors duration-150 ease-out {control.unavailable + ? 'cursor-not-allowed border border-dashed border-[var(--border-subtle)] text-[var(--text-muted)]' + : 'text-[var(--text)] hover:bg-[var(--hover-wash)]/10'}" > - {control.inFlight ? 'Resetting…' : 'Reset password'} + {#if control.inFlight} + + + {:else} + + {/if} {#if user.id !== data.user?.id} {/if}
diff --git a/tests/e2e/admin-users.spec.ts b/tests/e2e/admin-users.spec.ts index ebbf0fd..8da1c4a 100644 --- a/tests/e2e/admin-users.spec.ts +++ b/tests/e2e/admin-users.spec.ts @@ -307,6 +307,17 @@ async function resetViaUi( return revealed; } +/** + * The four row action buttons, in DOM order, named by their accessible name. An + * eligible row that is not the signed-in admin's own offers all four. + */ +const ROW_ACTION_LABELS = [ + 'Promote to admin', + 'Reset password', + 'Revoke all sessions', + 'Delete user' +] as const; + test.describe('Admin — Password Reset UI', () => { test('Scenario: Every user row offers a reset control, and an unavailable one says why', async ({ authenticatedPage: page @@ -616,6 +627,65 @@ test.describe('Admin — Password Reset UI', () => { await page.request.delete(`/api/admin/users/${victimId}`); }); + test('Scenario: Row actions stay full-size square targets at every phone width', async ({ + authenticatedPage: page + }) => { + // Given a password-auth user, whose row therefore offers all four actions + const victimId = await createVictim(page, 'ui-strip@test.com', 'UI Strip'); + + // The strip needs 188px of room, so it holds one line to 336px and wraps + // below. 360px covers the fitting case; 320px covers the case that used to + // shrink; 300px is narrow enough that a strip which refused to wrap would be + // pushed clear of its row, which the containment check below then catches. + for (const [width, fitsOneLine] of [ + [360, true], + [320, false], + [300, false] + ] as const) { + // When the admin views the users list on a screen that wide + await page.setViewportSize({ width, height: 800 }); + await openUsersPage(page); + const row = page.getByTestId(`user-row-${victimId}`); + + const boxes = []; + for (const label of ROW_ACTION_LABELS) { + const box = await row.getByRole('button', { name: label, exact: true }).boundingBox(); + expect(box, `${label} at ${width}px`).not.toBeNull(); + boxes.push(box!); + } + + // Then every action is a square that clears the 44px minimum. One of them + // deletes an account irreversibly, so an action that shrank to fit its row + // would put a data-losing target under a thumb. + for (const [i, box] of boxes.entries()) { + const at = `${ROW_ACTION_LABELS[i]} at ${width}px`; + expect(Math.min(box.width, box.height), at).toBeGreaterThanOrEqual(44); + expect(Math.abs(box.width - box.height), at).toBeLessThan(1); + } + + // And they sit on one line wherever there is room for one + const lines = new Set(boxes.map((box) => Math.round(box.y))); + if (fitsOneLine) expect(lines.size, `lines at ${width}px`).toBe(1); + + // And where there is not, they wrap rather than spill out of their row or + // push the page sideways + const rowBox = await row.boundingBox(); + expect(rowBox, `row at ${width}px`).not.toBeNull(); + for (const [i, box] of boxes.entries()) { + expect(box.x + box.width, `${ROW_ACTION_LABELS[i]} at ${width}px`).toBeLessThanOrEqual( + rowBox!.x + rowBox!.width + ); + } + const overflows = await page.evaluate( + () => document.documentElement.scrollWidth > document.documentElement.clientWidth + ); + expect(overflows, `horizontal overflow at ${width}px`).toBe(false); + } + + // Clean up + await page.request.delete(`/api/admin/users/${victimId}`); + }); + test('Scenario: A refused reset reports the server reason and reveals no password', async ({ authenticatedPage: page }) => { @@ -691,6 +761,81 @@ test.describe('Admin — Password Reset UI', () => { await page.request.delete(`/api/admin/users/${victimId}`); }); + test('Scenario: A server-side failure keeps the generated password even when it gives a reason', async ({ + authenticatedPage: page + }) => { + // Given a user whose reset fails inside the server, with a reason attached — + // a reason says why the server is unhappy, never whether it already stored + // the password, so the outcome is unknown however readable the body is + const victimId = await createVictim(page, 'ui-5xx-reason@test.com', 'UI 5xx Reason'); + await openUsersPage(page); + await page.route(`**/api/admin/users/${victimId}`, async (route) => { + if (route.request().method() !== 'PATCH') return route.continue(); + await route.fulfill({ + status: 503, + contentType: 'application/json', + body: JSON.stringify({ message: 'Database is temporarily read-only.' }) + }); + }); + + // When the admin resets that account's password + await page.getByTestId(`reset-password-btn-${victimId}`).click(); + + // Then the uncertainty is stated rather than the server's reason being taken + // as a refusal + await expect(page.getByTestId(`reset-password-error-${victimId}`)).toContainText( + /may or may not/i + ); + + // And the generated password is kept, because the server may already hold it + const reveal = page.getByTestId(`generated-password-${victimId}`); + await expect(reveal).toBeVisible(); + expect(((await reveal.textContent()) ?? '').trim().length).toBeGreaterThanOrEqual(8); + + // Clean up + await page.unroute(`**/api/admin/users/${victimId}`); + await page.request.delete(`/api/admin/users/${victimId}`); + }); + + test('Scenario: A rejection nobody can read keeps the generated password', async ({ + authenticatedPage: page + }) => { + // Given a user whose reset comes back as an unreadable error page — what a + // reverse proxy returns when it answers instead of the app, so the request may + // have reached the server or may not have + const victimId = await createVictim(page, 'ui-html-error@test.com', 'UI Html Error'); + await openUsersPage(page); + await page.route(`**/api/admin/users/${victimId}`, async (route) => { + if (route.request().method() !== 'PATCH') return route.continue(); + await route.fulfill({ + status: 400, + contentType: 'text/html', + body: '

400 Bad Request

' + }); + }); + + // When the admin resets that account's password + await page.getByTestId(`reset-password-btn-${victimId}`).click(); + + // Then the uncertainty is stated rather than the status alone being read as a + // refusal + await expect(page.getByTestId(`reset-password-error-${victimId}`)).toContainText( + /may or may not/i + ); + + // And the generated password is kept, because nothing said it was not stored + const reveal = page.getByTestId(`generated-password-${victimId}`); + await expect(reveal).toBeVisible(); + expect(((await reveal.textContent()) ?? '').trim().length).toBeGreaterThanOrEqual(8); + + // And the control is not left explaining a reason it never received + await expect(page.getByTestId(`reset-password-reason-${victimId}`)).toHaveCount(0); + + // Clean up + await page.unroute(`**/api/admin/users/${victimId}`); + await page.request.delete(`/api/admin/users/${victimId}`); + }); + test('Scenario: A reset attempted offline reports that nothing was changed', async ({ authenticatedPage: page }) => { @@ -800,7 +945,7 @@ test.describe('Admin — Password Reset UI', () => { // When the admin activates the control twice in a row const btn = page.getByTestId(`reset-password-btn-${victimId}`); await btn.click(); - await expect(btn).toContainText(/Resetting/); + await expect(btn).toHaveAccessibleName(/Resetting/); await expect(btn).toHaveAttribute('aria-disabled', 'true'); await btn.click({ force: true });