fix(web): settings permissions are sticky, so the owner-only tab survives the /me window (BUG-2978)

Deep-linking `/{user}/{ws}/settings#danger` landed on General for a workspace
OWNER — 0/10 loads, at both 390px and 1280px, while `#storage` and `#members`
were 10/10.

The hash-restoration effect was not the fault, which is where I looked first.
Instrumenting it showed the effect applying `danger` correctly at 219ms and
losing it at 244ms. `workspaceStore.setCurrent` clears `currentMembership` to
null before `/me` resolves and the permission helpers treat unknown as
no-access by design; this route calls `setCurrent` TWICE per load, once from
the workspace layout and once from the page's own `load()`. So
`canEditWorkspace` reads true -> false -> true, the owner-only tab drops out of
the tab set during the false window, the effect's snap-back branch moves
`activeTab` off the now-invalid `danger`, and `pendingHash` was already
consumed — nothing restores it when the permission returns. Only the owner-only
tab could hit this, which is exactly why `#storage` never did.

The page now reads its permissions through sticky state that updates only when
membership is definitively known, reset on a real workspace switch — the same
two-effect shape the dashboard already uses for its owner-gated CTA, and for
the same reason (CONVE-606). `isOwner` and `canExport` get the same treatment,
because they flicker identically and gate ~15 controls on this page: read
straight from the store, an ordinary owner load makes the Save buttons, the
invite form and the delete controls go readonly and then come back.

One reset effect owns all three. Two effects testing the same
`wsSlug !== lastPermSlug` could never both fire, since whichever ran first
would have already updated the marker — an error in my first draft of this fix.

Default stays false, so owner-only chrome still never flashes before `/me`
confirms; the server-side owner check remains the enforcement boundary.

Measured after the fix: 40/40 deep links land across `#danger`, `#storage`,
`#members` and `#collections` at both widths, against 0/10 for `#danger` on
main.

The regression spec settles and then asserts ONCE, rather than using an
auto-retrying assertion: the pre-fix failure is "correct, then reverted", so a
retrying matcher can observe the correct intermediate state and pass on a
broken build. The owner leg asserts again after a further wait so a later
revert is still caught, and carries a non-vacuity check that the fixture user
really is an owner.

Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm
This commit is contained in:
xarmian
2026-09-09 19:09:06 +00:00
parent 0b16be492b
commit e6ed6fdd11
2 changed files with 132 additions and 8 deletions
@@ -0,0 +1,83 @@
import { expect, type Page } from '@playwright/test';
import { test } from './fixtures';
/**
* BUG-2978 — deep-linking `/{user}/{ws}/settings#danger` landed on General for
* a workspace OWNER: 0/10 loads before the fix, while `#storage` and
* `#members` were 10/10.
*
* Mechanism, measured rather than guessed (the trail carries the probe). The
* hash-restoration effect works: it applied `danger` correctly ~219ms in. What
* broke it is that `workspaceStore.setCurrent` clears `currentMembership` to
* null before `/me` resolves, and the permission helpers treat unknown as
* no-access by design — and this route calls `setCurrent` TWICE per load, once
* from the workspace layout and once from the page's own `load()`. So
* `canEditWorkspace` reads true -> false -> true, the owner-only tab leaves the
* tab set during the false window, the effect's snap-back branch moves
* `activeTab` off the now-invalid `danger`, and `pendingHash` has already been
* consumed — so nothing restores it when the permission comes back.
*
* Only the owner-only tab could hit this, which is why `#storage` never did:
* an always-valid tab is never snapped away from.
*
* TIMING MATTERS IN THIS SPEC. The failure is "correct, then reverted", so an
* auto-retrying assertion (`toPass`, or a bare `toHaveAttribute` with its
* default timeout) can observe the CORRECT intermediate state and pass on a
* broken build. Both legs therefore settle first, then assert once, and the
* owner leg asserts a second time after a further wait so a later revert is
* still caught.
*/
const SETTLE_MS = 2500;
async function openSettings(page: Page, username: string, workspace: string, hash: string) {
await page.goto(`/${username}/${workspace}/settings${hash}`);
// The owner-only tab arrives with `/me`; waiting for the full set is what
// makes this a measurement of the settled tab bar rather than of the
// pre-permission one.
await expect(page.locator('.tab-bar .tab')).toHaveCount(5);
await page.waitForTimeout(SETTLE_MS);
}
function activeTabLabel(page: Page) {
return page.evaluate(() => {
const active = [...document.querySelectorAll('.tab')].find((t) =>
t.classList.contains('active'),
);
return (active?.textContent ?? '').trim();
});
}
test('BUG-2978: deep-linking the owner-only settings tab lands on it and stays', async ({
page,
fixture,
}) => {
await openSettings(page, fixture.adminUsername, fixture.workspaceSlug, '#danger');
// Non-vacuous: the fixture user must actually be an owner, or "Danger Zone
// is not selected" would be the correct answer and the leg would pass for
// the wrong reason.
await expect(page.locator('.tab', { hasText: 'Danger Zone' })).toHaveCount(1);
expect(await activeTabLabel(page), 'deep-linked owner-only tab not selected').toContain(
'Danger Zone',
);
// The pre-fix build SELECTED it and then reverted, so hold and look again.
await page.waitForTimeout(1500);
expect(await activeTabLabel(page), 'owner-only tab was selected and then lost').toContain(
'Danger Zone',
);
});
test('BUG-2978 control: a tab valid without /me deep-links on both builds', async ({
page,
fixture,
}) => {
// Storage is visible to every member, so it is never removed from the tab
// set by the permission window. This leg passed before the fix too — it is
// here to show the failure was specific to the owner-only tab rather than
// to hash restoration in general.
await openSettings(page, fixture.adminUsername, fixture.workspaceSlug, '#storage');
expect(await activeTabLabel(page)).toContain('Storage');
});
@@ -84,7 +84,55 @@
{ id: 'storage', label: 'Storage', icon: '\uD83D\uDCBE', ownerOnly: false },
{ id: 'danger', label: 'Danger Zone', icon: '\u26A0\uFE0F', ownerOnly: true },
];
let tabs = $derived(allTabs.filter(t => !t.ownerOnly || workspaceStore.canEditWorkspace));
// BUG-2978: gate the owner-only tab on a STICKY read of the permission, not
// on `workspaceStore.canEditWorkspace` directly.
//
// `setCurrent` clears `currentMembership` to null before `/me` resolves, and
// the permission helpers treat unknown as no-access by design. This route
// calls it twice per load — once from the workspace layout, once from this
// page's own `load()` — so `canEditWorkspace` reads true -> false -> true on
// an ordinary owner page load. Measured on the trail: the effect below
// applied `#danger` correctly at 219ms and the false window at 244ms snapped
// it back to General, with `pendingHash` already consumed, so the tab was
// lost for good and 0/10 deep links landed.
//
// Same two-effect shape the dashboard uses for its owner-gated CTA, and for
// the same reason (CONVE-606: reset on a real workspace switch, update only
// when membership is definitively known). Default false so owner-only chrome
// never flashes before `/me` confirms; the server-side owner check remains
// the enforcement boundary, this is a stability fix.
let canEditWs = $state(false);
// Same treatment, same reason: read straight from the store these flip false
// during that window too, so on an ordinary owner load the Save buttons, the
// invite form and the delete controls go readonly and then come back.
let isOwner = $state(false);
let canExport = $state(false);
let lastPermSlug: string | null = null;
$effect(() => {
if (wsSlug !== lastPermSlug) {
lastPermSlug = wsSlug;
// ONE reset for every sticky permission on this page. A second
// effect testing the same `wsSlug !== lastPermSlug` could never
// fire — whichever ran first would have already updated the marker.
canEditWs = false;
isOwner = false;
canExport = false;
}
});
$effect(() => {
// Read through the store's getters rather than re-deriving the cascade —
// they mirror the server's ResolveUserPermission and must not be forked.
if (workspaceStore.currentMembership !== null) {
canEditWs = workspaceStore.canEditWorkspace;
isOwner = workspaceStore.isOwner;
// Editor-or-owner predicate for affordances outside the strict
// owner-only line (Export bundle is a read-side action gated to
// editor+ per project policy).
const role = workspaceStore.currentRole;
canExport = role === 'owner' || role === 'editor';
}
});
let tabs = $derived(allTabs.filter(t => !t.ownerOnly || canEditWs));
let validTabIds = $derived(tabs.map(t => t.id));
// Hash-driven tab restoration. The hash is captured once on mount, but
@@ -421,13 +469,6 @@
}
}
let isOwner = $derived(workspaceStore.isOwner);
// Editor-or-owner predicate for affordances that fall outside the
// strict canEditWorkspace owner-only line (e.g. Export bundle is a
// read-side action that we still gate to editor+ per project policy).
let canExport = $derived(
workspaceStore.currentRole === 'owner' || workspaceStore.currentRole === 'editor'
);
let confirmDelete = $state(false);
let deleting = $state(false);