From e8915d0b1ed0d872e0d478fda8f82da9114e79aa Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 10 Aug 2026 14:27:47 +0000 Subject: [PATCH] fix: marking the same child twice should leave you with one child MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user marking their kid on the footage — play, pause, click, play, pause, click — ended up with eight athletes, seven of them named "My athlete", created inside three minutes, each bound to exactly one fragment. Scoring reads the focal flag, so a dozen careful selections collapsed to whichever click happened last. Their words: "it created each focal selection into a duplicate player and in the end it just uses one selection not all of them". `new` in the identify route means "I have not told you who this is", and it was creating an athlete unconditionally. The client sends `new` whenever its own athlete list has not loaded yet, which is every click made faster than a page reload — so the faster you worked, the more duplicates you got. It now prefers the athlete already being followed, and only creates one when there genuinely is none. The second half is which fragments survive. A caller that said `new` did not know who this was and cannot have sent the fragments already bound, so replacing the set silently discarded every earlier pick; those calls now add. The picker, which holds the whole selection and names the athlete, still replaces — that is what makes unticking a crop work. apps/web/src/identifytwice.test.ts drives the real route: three clicks with no loaded state give three athletes before this change and one athlete with three fragments after it. Co-Authored-By: Claude Opus 5 (1M context) --- apps/web/src/actions.ts | 44 +++++++--- apps/web/src/identifytwice.test.ts | 131 +++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 10 deletions(-) create mode 100644 apps/web/src/identifytwice.test.ts diff --git a/apps/web/src/actions.ts b/apps/web/src/actions.ts index 31fb56b..4fa7d6f 100644 --- a/apps/web/src/actions.ts +++ b/apps/web/src/actions.ts @@ -705,26 +705,50 @@ export const registerActions = (app: Hono): void => { if (trackId.length === 0) throw new UploadError('INVALID_INPUT', 'Choose a track first.'); /** - * `new` creates the athlete on the spot. + * `new` means "I have not told you who this is", not "make me another + * one". * - * Identifying an athlete is the one step scoring cannot proceed without, - * and it used to require having already created an athlete record — a - * prerequisite the UI hid until you had satisfied it. Someone who had - * never added an athlete saw a collapsed panel offering nothing to click, - * and every run they made was mathematically incapable of producing a - * moment. Picking a face is now the whole of the setup. + * It created an athlete unconditionally, and the client sends it whenever + * its own athlete list has not loaded yet — which is every click made + * faster than a page reload. A user marking their child on the footage, + * pausing and clicking again a dozen times, produced a dozen athletes + * named "My athlete", each bound to exactly one fragment, each in turn + * made the focal one. Scoring reads the focal flag, so all of that work + * collapsed to whichever click happened last: production ended up with + * eight athletes, seven of them duplicates created less than three + * minutes apart, and one selection in use. + * + * Identifying is still the one step that cannot be skipped, so this + * still creates an athlete when there genuinely is none. It just prefers + * the one already being followed. */ const requested = c.req.param('id') ?? ''; + const existing = requested === 'new' ? await listAthletes(root) : []; + const reusable = existing.find((candidate) => candidate.isFocal) ?? existing[0]; const athlete = requested === 'new' - ? await addAthlete(root, { name: 'My athlete' }) + ? (reusable ?? (await addAthlete(root, { name: 'My athlete' }))) : await getAthlete(root, requested); const athleteId = athlete.id; // Following and being bound to a track are different things; a picked // athlete is obviously the one to follow. await updateAthlete(root, athleteId, { focalTrackId: trackId, focal: true }); - // Every fragment the user picked, not only the first. - const assigned = await assignTracksToAthlete(root, athleteId, requestedIds); + + /** + * Add to the athlete, or set them, depending on what the caller knew. + * + * The picker holds the whole selection and posts all of it, so unticking + * a crop has to be able to remove it — that call names the athlete and + * replaces the set. A caller that said `new` did not know who this was + * and cannot have sent the existing fragments, so replacing would silently + * discard every earlier pick. Marking the same child at six moments on the + * footage should leave them marked at six moments. + */ + const adding = requested === 'new' && reusable !== undefined; + const finalIds = adding + ? [...new Set([...(await tracksForAthlete(root, athleteId)), ...requestedIds])] + : requestedIds; + const assigned = await assignTracksToAthlete(root, athleteId, finalIds); startAnalysis(root, { preset: 'balanced', scoreOnly: true }); diff --git a/apps/web/src/identifytwice.test.ts b/apps/web/src/identifytwice.test.ts new file mode 100644 index 0000000..66107fa --- /dev/null +++ b/apps/web/src/identifytwice.test.ts @@ -0,0 +1,131 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { Hono } from 'hono'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +/** + * Marking the same child twice should leave you with one child. + * + * `new` created an athlete unconditionally, and the client sends `new` whenever + * its own athlete list has not loaded — which is every click made faster than a + * page reload. A user marking their kid on the footage, pausing, and clicking + * again produced one athlete per click, each bound to a single fragment, each in + * turn made focal. Scoring reads the focal flag, so a dozen careful selections + * collapsed to whichever happened last. Production reached eight athletes, seven + * of them duplicates created inside three minutes, with one selection in use. + */ + +let home: string; +let root: string; +let app: Hono; + +/** + * The route fires a re-score and does not wait for it. It fails here, loudly + * and harmlessly, because the fixture video is a path that does not exist — + * that is the "1 source file(s) are no longer where they were imported from" + * on stderr, and it is expected. + */ +beforeAll(async () => { + home = mkdtempSync(path.join(tmpdir(), 'reeleel-identify-')); + process.env['REELEEL_HOME'] = home; + + const { createProject } = await import('@reeleel/core'); + const created = await createProject({ + name: 'twice', + path: path.join(home, 'projects', 'twice'), + sport: 'basketball', + }); + root = created.path ?? created.root; + + const { execute, projectDb, createTrack } = await import('@reeleel/core'); + const db = await projectDb(root); + const now = new Date().toISOString(); + await execute( + db, + 'INSERT INTO source_videos (id, project_id, path, created_at, updated_at) VALUES (?, ?, ?, ?, ?)', + ['vid_a', 'prj_test', '/tmp/vid_a.mp4', now, now], + ); + for (let i = 0; i < 4; i += 1) { + await createTrack(root, { + videoId: 'vid_a', + className: 'player', + confidence: 0.9, + samples: [ + { ts: i * 10, frame: i * 300, x: 1, y: 2, w: 3, h: 4, confidence: 0.9 }, + { ts: i * 10 + 5, frame: i * 300 + 150, x: 1, y: 2, w: 3, h: 4, confidence: 0.9 }, + ], + }); + } + + const { registerActions } = await import('./actions.js'); + app = new Hono(); + registerActions(app); +}); + +afterAll(async () => { + const { resetDbCache } = await import('@reeleel/core'); + resetDbCache(); + rmSync(home, { recursive: true, force: true }); + delete process.env['REELEEL_HOME']; +}); + +const trackIds = async (): Promise => { + const { listTracks } = await import('@reeleel/core'); + return (await listTracks(root, 'vid_a')).map((track) => track.id); +}; + +/** What the client posts when its athlete list has not loaded yet. */ +const identify = async (ids: string[]): Promise => + app.request(`/projects/${encodeURIComponent(root)}/athletes/new/track`, { + method: 'POST', + headers: { accept: 'application/json', 'content-type': 'application/json' }, + body: JSON.stringify({ trackId: ids[0], trackIds: ids }), + }); + +describe('marking the same athlete over and over', () => { + it('keeps one athlete and accumulates the fragments', async () => { + const tracks = await trackIds(); + const { listAthletes, tracksForAthlete } = await import('@reeleel/core'); + + // Six clicks, exactly as a user scrubbing the footage would make them. + for (const id of tracks.slice(0, 3)) { + const response = await identify([id]); + expect(response.status).toBe(200); + } + + const athletes = await listAthletes(root); + expect(athletes).toHaveLength(1); + + // Every pick kept, not merely the last one. + const bound = await tracksForAthlete(root, athletes[0]!.id); + expect(bound.sort()).toEqual(tracks.slice(0, 3).sort()); + expect(athletes[0]!.isFocal).toBe(true); + }); + + it('still lets a named athlete replace their set, so unticking works', async () => { + const tracks = await trackIds(); + const { listAthletes, tracksForAthlete } = await import('@reeleel/core'); + const athlete = (await listAthletes(root))[0]!; + + // The picker knows who it is talking about and sends the whole selection. + const response = await app.request( + `/projects/${encodeURIComponent(root)}/athletes/${athlete.id}/track`, + { + method: 'POST', + headers: { accept: 'application/json', 'content-type': 'application/json' }, + body: JSON.stringify({ trackId: tracks[0], trackIds: [tracks[0]] }), + }, + ); + expect(response.status).toBe(200); + + expect(await tracksForAthlete(root, athlete.id)).toEqual([tracks[0]]); + expect(await listAthletes(root)).toHaveLength(1); + }); + + it('creates exactly one athlete for a project that has none', async () => { + const { listAthletes } = await import('@reeleel/core'); + expect(await listAthletes(root)).toHaveLength(1); + }); +});