jakob48/crumbs
1
0
Fork 0
forked from mirror/crumbs-original
Code Pull Requests Activity

refactor(wu-003b): one uniform icon per row action, and guard the reveal

Browse Source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RbqJcFofZSbhbGwUwZP3wU
This commit is contained in:
Frederic Cabassut
2026-08-18 12:31:12 +02:00
committed by Fred Cabassut
co-authored by Claude Opus 5
parent ffde151b75
commit afce024cac
2 changed files with 198 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/routes/(app)/settings/users/+page.svelte
+52 -13
View File
@@ -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'}
<div
class="flex flex-col gap-3 rounded-sm border border-[var(--border-subtle)] p-4"
data-testid="user-row-{user.id}"
@@ -410,13 +419,32 @@
</p>
<p class="truncate text-sm text-[var(--text-muted)]">{user.email}</p>
</div>
<div class="flex flex-wrap items-center gap-2">
<!-- One uniform 44x44 square per action, replacing four text labels that
wrapped onto three lines at phone width. `flex-wrap` 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. Drop it and
the four flatten to 40x44 at 320px — under the minimum, on a row whose
Delete is irreversible. They hold one line down to 336px, which is
exactly 4x44 plus the gaps, and take a second line below that: correctly
sized targets over two lines beat cramped ones on one. The container's
own `shrink-0` is a different mechanism and does carry weight — from
640px the row turns `sm:flex-row`, where the truncating name block would
otherwise squeeze the strip. On the buttons themselves that same token
would do nothing, `flex-wrap` having already spared them. Each icon
carries both a tooltip, for a mouse, and an aria-label, since an icon
alone has no accessible name. -->
<div class="flex shrink-0 flex-wrap items-center gap-1">
<button
onclick={() => toggleRole(user)}
class="rounded-sm px-3 py-1 text-xs text-[var(--text-muted)] hover:bg-[var(--border-subtle)]/50"
title={user.role === 'admin' ? 'Demote to user' : 'Promote to admin'}
class="inline-flex size-11 items-center justify-center rounded-sm text-[var(--text-muted)] transition-colors duration-150 ease-out hover:bg-[var(--hover-wash)]/10"
use:tooltip={roleAction}
aria-label={roleAction}
>
{user.role === 'admin' ? 'Make user' : 'Make admin'}
{#if user.role === 'admin'}
<ShieldOff class="h-4 w-4" />
{:else}
<ShieldCheck class="h-4 w-4" />
{/if}
</button>
<!-- Offered on every row, the admin's own included: a control that
vanishes is the confusion this replaces. -->
@@ -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}
<!-- The label went with the text, so in-flight has to be carried by
the icon and by the accessible name above. -->
<LoaderCircle class="h-4 w-4 animate-spin" />
{:else}
<KeyRound class="h-4 w-4" />
{/if}
</button>
{#if user.id !== data.user?.id}
<button
onclick={() => revokeSessions(user)}
class="rounded-sm px-3 py-1 text-xs text-[var(--text-muted)] hover:bg-[var(--border-subtle)]/50"
title="Force logout"
class="inline-flex size-11 items-center justify-center rounded-sm text-[var(--text-muted)] transition-colors duration-150 ease-out hover:bg-[var(--hover-wash)]/10"
use:tooltip={'Revoke all sessions'}
aria-label="Revoke all sessions"
>
Revoke
<LogOut class="h-4 w-4" />
</button>
<button
onclick={() => deleteUser(user.id)}
data-testid="delete-user-btn-{user.id}"
class="rounded-sm px-3 py-1 text-xs text-[var(--destructive)] hover:bg-[var(--error-bg)]"
class="inline-flex size-11 items-center justify-center rounded-sm text-[var(--destructive)] transition-colors duration-150 ease-out hover:bg-[var(--error-bg)]"
use:tooltip={'Delete user'}
aria-label="Delete user"
>
Delete
<Trash2 class="h-4 w-4" />
</button>
{/if}
</div>
tests/e2e/admin-users.spec.ts
+146 -1
View File
@@ -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: '<html><body><h1>400 Bad Request</h1></body></html>'
});
});
// 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 });