[ac-pplx]

Summary

Follow-up to #1278, fixing the two low findings its review left open.

• shortHash now strips any <algorithm>: prefix (/^[a-z0-9]+:/) before keeping 12 characters. It removed only sha256:, so a blake3:<64 hex> digest, the default for new releases, showed as blake3:01234 (5 hex characters). The Templates page's Hash and releases columns, its sort, and the rebuild panel share the helper, so they all show 12 hex characters now.
• The rebuild panel's exclusive helper now catches a QuotaExceededError or SecurityError from the localStorage fallback lock (used where Web Locks is unavailable), and runs work() without the lock. Any other error from taking the lock still propagates. Before, the throw escaped before persist or store could report "could not save ...", so the click did nothing visible and the promote dialog kept spinning. It does not retry the lock after that error, since retrying cannot store it. With blocked storage, work()'s own writes fail too and report through the existing messages. Full storage still accepts removals and writes that do not grow the stored run, so those land without the lock.

To test exclusive, its body moved into ui/src/lib/exclusive.ts, unchanged except for the new catch, taking its lock functions as parameters. The fallback lock's TTL moved with it, beside the loop it depends on. The component passes the real Web Locks and view-refresh functions.

Validation

At this head, in ui/: npm ci, npm test (125 pass, 0 fail), npx tsc --noEmit -p ., and npm run build all exit 0.

• template-format.test.mjs: the blake3: case fails on main (blake3:01234 instead of 0123456789ab) and passes here; sha256:, no-prefix, and fallback cases are also covered.
• exclusive.test.mjs: covers Web Locks, hold and release in the fallback, waiting on a held lock, the throwing acquisition (work runs once, no waiting, no release), errors from work propagating, an unrelated acquisition error propagating without running work, and the TTL the fallback asks for. I did not run the throwing-acquisition test against the old exclusive, which lived in the component and could not be imported; the old code let the throw escape.

Not validated: a real browser with full or blocked storage; the dialog behavior is covered by the unit test of the helper only.

Severity and risk

Severity: low. A cosmetic digest display and an unreported failure in an admin UI edge case (storage full or blocked, no Web Locks).
Risk: digests display differently, only for prefixes other than sha256:. Where storage refuses the lock and Web Locks is unavailable, writes run unlocked. With full storage, a dismiss or a write that does not grow the stored run can then land and be overwritten by another tab's write in the same moment; before, both clicks threw. Roll back by reverting the PR.

AI assistance: Claude Code drafted the code, tests, and this description.

[github-actions]

Thanks for opening this pull request! A SPACE maintainer will take a look.

CONTRIBUTING.md explains how we review outside contributions.

[the-gigi-pplx]

Review — 2 findings

Medium

1. ui/src/lib/exclusive.ts:26-28 — full storage doesn't refuse every write, so the unlocked work() can save without the lock.
   The comment says work's "own write fails the same way and reports it", and the description says "storage refuses every write". That holds for blocked storage (SecurityError) but not for full storage. A new lock key needs extra bytes and throws QuotaExceededError. Rewriting the existing run key at the same size or smaller, and removeItem, still succeed. I filled Node's Web Storage to its quota and checked: the new ~80-byte lock key throws, while same-size and smaller rewrites of an existing 2 KB key and removeItem all succeed.
   So with full storage and no Web Locks, dismiss (a removeItem) and an update that doesn't grow the record both run unlocked and commit. One way that goes wrong: tab A dismisses while tab B's poll has already read the run, and B's write brings the dismissed run back. Before this PR both clicks threw, so this lost update is new, though it needs a narrow overlap.
   I think running unlocked is still the right call. Retrying the lock forever was the original hang, and refusing to run brings back the silent click. If you want the middle, catch QuotaExceededError and SecurityError separately. Then either refuse only the writes that need the lock on quota (an explicit "storage is full" notice), or keep the current behavior and narrow the comment and the risk note to what's true: on full storage, writes that don't grow the record and removals still land, unlocked. Narrowing the bare catch {} to those two errors also keeps an unrelated bug in tryAcquireViewRefreshLock from turning cross-tab locking off without a trace.

Low

2. ui/src/app/dashboard/templates/system-template-rebuild.tsx:54-56 — the fallback lock's contract now spans two files.
   FALLBACK_LOCK_TTL_MS and its rationale ("held only while work runs, which is synchronous") stay in the component. The loop that makes that true, and its retry constant, moved to lib/exclusive.ts. A later change to exclusiveWith (async work, longer holds) won't show that a 2-second TTL depends on it. Exporting a ready-wired exclusive with the view-refresh and timer defaults from lib/exclusive.ts would keep the TTL next to the loop, drop the component wrapper, and merge the two imports at lines 48-49.

Verified against 630fb9870c53778d73428cf6b652748de26709c4. Reproduced locally, in ui/: npm ci, npm test (123 pass), npx tsc --noEmit -p . and npm run build all exit 0. I mutated both fixes: putting back the sha256:-only replace in shortHash fails the blake3: test, and removing the acquire catch fails 2 of the 6 exclusive tests. Not checked: behavior in a real browser. The quota check above used Node's Web Storage implementation.

[the-gigi-pplx]

Approved. 2 non-blocking findings in the review comment above.

[ac-pplx]

Thanks, both fixed in 7a7bb59.

1. The fallback now runs work() unlocked only for QuotaExceededError or SecurityError; any other error from taking the lock propagates, with a test for each. I kept the unlocked behavior, and the comment and the description's risk note now say what full storage still accepts: removals and writes that do not grow the stored run, which land unlocked and can be overwritten by another tab's write in the same moment.
2. The fallback TTL now lives in lib/exclusive.ts beside the loop, and exclusiveWith passes it to acquire, with a test. The component keeps a three-line wiring wrapper rather than a ready-wired export: the unit tests load lib/*.ts directly under Node, which does not resolve the extensionless ./view-refresh import, and no unit-tested module imports a sibling at runtime.

At 7a7bb59 and on its merge with main, in ui/: npm ci, npm test (125 and 141 pass), npx tsc --noEmit -p . and npm run build all exit 0.

[the-gigi-pplx]

Review follow-up — both findings resolved

I checked 7a7bb59c289503892ab17f394e89bacd08a05d63 against my earlier review at 630fb987.

1. Unlocked fallback: exclusiveWith now runs work() unlocked only when acquire fails with QuotaExceededError or SecurityError, and rethrows anything else. The comment now says what full storage still accepts. I mutated the guard so every acquisition error falls through to the unlocked path, and the new "not a storage refusal propagates" test fails on that tree.
2. TTL placement: FALLBACK_LOCK_TTL_MS lives beside the loop in lib/exclusive.ts, and exclusiveWith passes it to acquire. The reason you gave for keeping the component wrapper (Node's test loader doesn't resolve the extensionless sibling import) is fine by me.

Reproduced in ui/: npm ci, npm test (125 pass) and npx tsc --noEmit -p . all exit 0. My approval stands.