Repository navigation
Show gallery reference face on Unknown "looks like" cues - #34
Conversation
🧪 Automated PR ChecksProfile: ✅ python-syntax — 12 Python file(s) compile cleanly hermes-work · branch |
|
CI is red — one missed spot, and it is only the test fixture.
class FakeGallery:
def __init__(self, match=(None, None, 0.0), ignored=0.0): # -> (None, None, 0.0, None)Both the default and the tuples the three tests pass in need the fourth element. Three errors, all from that one line. I can push the fix to your branch if you prefer — Two things I noticed while reading, neither blocking:
And The feature itself is a good call — with the hex-suffixed names from #32, a thumbnail is what makes those IDs usable at all. |
Persist guess_top_photo (best single-photo match) with each guess and render a thumb + hover enlarge on rest clusters and suggestion headers so role+hex stranger IDs are visually identifiable during review.
0223ef1 to
b233885
Compare
|
Rebased onto main and fixed the Thanks for the catch — CI should be green on |
![the-codemole[bot]](https://avatars.githubusercontent.com/in/4150723?s=60&v=4){kind=small}
No longer clean — approval withdrawn.
Store the winning person slug next to guess_top_photo so the Unknown UI does not re-derive the gallery folder from the display name. Only write those keys when a photo exists (same convention as refresh_guesses), and hide a broken thumb if the file is gone. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the two open findings on
@codemole recheck |
|
Recheck is running — the Both fixes read correctly in the diff: Two small things:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-ups from the recheck note:
@codemole recheck |
|
Recheck is running — the On the stale-slug case: hiding the thumb is the right call, and it falls out of the design rather than needing a special path. If the person folder is gone, One thing I can't verify from the diff: whether a rename keeps the slug stable. If renaming re-slugifies into a new folder, the old slug goes stale the same way a delete does — self-healing on the next |
Call refresh_guesses after person/photo delete, set-aside, restore, and non-dry dedupe so stored guess_top_slug/photo pointers do not dangle. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Third finding (dangling pointer): also wired @codemole recheck |
|
Recheck is running — the Two things on the
Also worth stating in the changelog if it isn't already: Rename staying on the display name matches |
|
On the wiring notes:
@codemole recheck |
|
Recheck is running — the All three notes resolve the open points for me:
Residual, non-blocking: |
request_refresh_guesses coalesces delete/set-aside/dedupe bursts into one background refresh_guesses pass, and each metadata rewrite uses temp+replace so a crash cannot truncate an unknown's embedding JSON. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the sync-refresh finding:
@codemole recheck |
|
Recheck is running — the The debounce itself looks correct: One race the atomic write doesn't close, though: the tmp path is derived only from the target ( Minor and non-blocking: |
No longer clean — approval withdrawn.
Two findings from the review of the debounced refresh. The temp file was named after its target, so every writer of the same unknown shared one path — and nothing stopped a request handler's direct refresh_guesses() from running while the background worker was in one. Two passes then interleaved as 'A writes half, B replaces', publishing the truncated file as the real metadata: exactly what the atomic replace is there to prevent. The pass now runs under its own lock (not self._lock, which match() already holds), and each write goes through tempfile.mkstemp() so no two writers can collide. A failed write unlinks its temp file instead of leaving it behind. The debounce was trailing-only, so a steady stream closer than the delay — bulk deletion, a dedupe cleanup loop — deferred the refresh for the whole burst while the UI kept showing cues for photos that were already gone. The deadline is now also capped at first_request + max_delay (2 s default). Verified the other way round: with the shared temp name and the trailing-only deadline restored, both new tests fail, five runs out of five — the concurrency one on the FileNotFoundError from two threads racing for the same temp path. 108 tests green with the fix.
|
@Alien10140 — the CI fix and the debounce both look right, and the sync-refresh rework is a Two concurrency findings came back on the new code and I pushed the fixes to your branch
Both have tests, and I checked them the other way round — with the old code restored they Nothing else from my side. Once the bot is satisfied I will take it to the maintainer for the |
Two differences from the plain write_text() this replaced. mkstemp() creates with 0600 and os.replace() carries that mode onto the target, so every refresh would have narrowed each unknown's JSON to the service user. The mode of the existing file is now copied onto the temp file before the replace, falling back to 0644. And the comment promised more than the code delivered: without flush+fsync the rename can land ahead of the data, leaving a zero-length file after a power cut — the silent loss the atomic replace is there to prevent. The write is now fsynced, and the directory once per pass rather than per file. Measured before claiming it is cheap: 200 unknowns 43.4 ms with fsync against 41.6 ms without (0.009 ms per file), 2000 unknowns 426 ms against 428 ms — inside the noise. The mode test fails with 384 != 420 when the chmod is removed.
|
All four findings are addressed on the branch ( @codemole recheck |
|
Recheck is running — the What I can confirm from the diff:
Two things I can't verify from here: the |
os.replace() recreates its target unconditionally, so an unknown that was assigned or discarded while the refresh was mid-pass came back as a JSON file with stale metadata and no crop — and reappeared in the review queue. The debounced background worker widened that window by design: the pass now runs beside the handlers instead of inside one. The target is stat'ed once (its mode is needed anyway) and a missing file means skip rather than write, with a second existence check immediately before the replace. The window shrinks from the whole write to two calls; closing it fully would need the handlers to take the refresh lock, which is a bigger change than this PR should carry. Reverting the skip makes the new test fail with 'True is not false: ein geloeschtes Unknown darf nicht zurueckkommen'. 110 tests green.
|
Head is @codemole recheck |
|
Recheck is running — the What the diff confirms:
The test results (110 green, race 5/5, |
The looks-like preview from #34, plus the debounced refresh and the safe metadata write that came out of its review. The debounce is the one visible change for users: cues update a moment later instead of instantly, capped at 2 s after the first request in a burst.
|
Released as v0.26.0 — thanks, Credited in the changelog, including the part you were not asked for: turning the The release notes call out the one visible behaviour change for anyone reading them — cues |
3 participants
Summary
guess_top_photo(highest single-photo similarity for the winning person) whenever an unknown getsguess/guess_score.Makes role+hex stranger IDs (e.g. after Track as new) visually identifiable without jumping to the Persons tab.
Test plan
refresh_guesses, remaining unknowns gainguess_top_photoMade with Cursor