Code review caught a race + a missing None guard; designer review
caught a window.confirm regression and a button-hierarchy issue.
Backend:
- Race fix (bug-2): replace set_skill_readonly+create_skill_version
with a single atomic unlock_skill(template_id, snapshot, changed_by)
-> int|None on the storage protocol (sqlite + postgres). Snapshot
insert + readonly flip happen in one transaction; the next version
number is computed via SELECT MAX(version)+1 inside the txn rather
than len(list)+1 outside, closing the (skill_id, version)
collision window where two concurrent admin actions could both pick
the same version.
- None guard (bug-3): check the post-flip get_prompt_template re-read;
return 404 instead of letting _skill_to_response(None) raise.
- Audit body: also record snapshot_version, and harden None-vs-empty
with `or ""` on the existing.get(...) calls.
Frontend:
- D-1: replace window.confirm with the existing showConfirmModal
(admin.js:2350) — themed dialog, focus-trap, can render the source
URL with consistent typography. The native dialog could collapse
the multi-paragraph copy depending on browser.
- D-2: mutate-in-place on success rather than hide → reload → reopen.
loadGovSkills now returns its fetch promise so unlockSkill can
chain showEditTemplateModal after the cache refresh — no flicker,
no focus bounce, and it kills bug-1 (the reopen was reading stale
_govSkills before loadGovSkills resolved). showEditTemplateModal
is idempotent when already open: it skips the trigger-element
capture and the focus-trap reinstall.
- D-3: button hierarchy. Drop flex:1 from .modal-secondary so the
Save button keeps a stable width whether or not Customize is
rendered; insert a flex-spacer between Customize and Save so the
destructive-ish detach groups left next to Cancel and the primary
action floats right.
- D-4: NBSP normalized to match the existing escape pattern
on the sibling badge line (was an actual NBSP byte).
- D-5: success toast now reads "Skill unlocked — fields are now
editable" so the operator gets a positive affirmation that the
edit affordance is live.
- D-10: aria-describedby="etm-origin-badge" on disabled spec inputs
so screen-reader users get the same "this came from upstream"
context that sighted users see in the cyan badge.
Tests: + test_unlock_skill_versions_after_existing_history seeds an
out-of-order version (3) and asserts unlock picks 4, defending
against the len()-based version computation regressing.
Patrick Buckley
2cbd926b9d