Repository navigation
Give the action buttons states, and stop swallowing failures - #37
Merged
Merged
Conversation
Three defects found while checking #32 in the browser rather than in the diff. The .act buttons (ASSIGN, ASSIGN ALL, Track as new) had no hover, no active and no focus style at all — pointing at a button that creates a person looked exactly like not pointing at it. Measured rather than guessed: 1.66:1 from rest to hover, 1.50:1 from hover to active, with the dark label staying above 4.5:1 on both (7.3:1 and 4.9:1). The focus ring sits on the panel thanks to outline-offset and stands there at 13:1. Network and server failures were invisible: 56 'await j(...)' calls across 25 handlers, most without a catch, so a click that failed did nothing at all and said nothing. One 'unhandledrejection' listener covers every handler, including the ones written next. POST /api/unknowns/assign created the person and then returned HTTP 200 with {"assigned": 0} when no face could be assigned — stale ids, an empty selection, or a second tab that got there first. Through Track as new that left an empty Unnamed-xxxxxx in the gallery on every failed attempt, while the UI reported '0 face(s) → Unnamed-ab12cd' and looked like success. It now answers 409 and removes a person that was created for that very call; an existing person is never deleted. Measured against a second instance on port 8699 with an empty data directory, production untouched: the toast reads 'no face was assigned — Unnamed-ab12cd was not created' and the person list stays empty.
🧪 Automated PR ChecksProfile: ✅ python-syntax — 3 Python file(s) compile cleanly hermes-work · branch |
create_person() is deliberately idempotent: 'Juli', ' juli ' and a second tab all resolve to the existing slug. Checking body.person against the slug keys therefore took the else branch for a display name, got the pre-existing slug back, and on zero assignments deleted an existing person with all its stored faces — the exact case the comment claimed could not happen. The flag now comes from the resolved slug, and a test covers the display-name, casing and whitespace paths. Verified the other way round too: with the old condition that test fails. Two more from the same review. The prefers-reduced-motion block reset transition, which is never declared here, while the motion actually added — translateY(1px) on :active — stayed on; it now resets transform. And the global rejection handler called preventDefault() without logging, so a TypeError survived only as a 120-character toast; it logs the original reason to the console first. Release 0.25.1.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #32. The feature itself is sound — minting the name client-side against the
existing assign endpoint, leaving ASSIGN alone — but checking it in a browser instead of in
the diff turned up three things, and the third one quietly damages the gallery.
A failed assign created a person anyway
POST /api/unknowns/assigncreated the person first and assigned afterwards, so when noface could be assigned — stale ids, an empty selection, or a second tab that got there first
— it answered HTTP 200 with
{"assigned": 0}and the person stayed. Through "Track asnew" that means every failed attempt leaves an empty
Unnamed-xxxxxxin the gallery, whilethe UI reports
0 face(s) → Unnamed-ab12cdand looks like success.Measured on a second instance before the fix:
It now answers 409 and deletes a person that was created for that very call. An existing
person is never deleted — there is a test for that, because getting this wrong would be far
worse than the bug.
The action buttons had no states
.act— ASSIGN, ASSIGN ALL, Track as new — had no:hover, no:activeand no:focus-visiblerule. Only.ghost:hoverexisted. Pointing at a button that creates aperson looked exactly like not pointing at it.
Measured rather than guessed, via canvas pixels and the WCAG formula:
Below roughly 1.5:1 a state change is not visible, which is why the first attempt
(
oklch(78% …), 1.26:1) was thrown away. Active also shifts by 1px, so the press issignalled twice. The focus ring sits on the panel because of
outline-offset: 2px, which iswhere those 13:1 come from.
[disabled]gets 45 % opacity andnot-allowed.Failures were invisible
56
await j(...)calls across 25 handlers, most without acatch— so a click whoserequest failed did nothing and said nothing. That is the same defect class as the Settings
form in #36. One listener covers every handler, including the ones written next:
Verified end to end on the second instance: triggering the 409 now shows
Failed: no face was assigned — Unnamed-ab12cd was not created, and/api/personsstaysempty.
Not in here
The action buttons are 26 px tall against the 44 pt minimum for a touch target. That is true
of every button in the UI, not just these, so it is a separate change rather than something
to bolt on here.
104 tests green. Production was not touched: everything was measured against a second
instance on port 8699 with its own empty data directory.