diff --git a/apps/web/src/client/identify.tsx b/apps/web/src/client/identify.tsx index f742b19..bd25f83 100644 --- a/apps/web/src/client/identify.tsx +++ b/apps/web/src/client/identify.tsx @@ -1,6 +1,8 @@ /** @jsxImportSource hono/jsx/dom */ import { render, useEffect, useState } from 'hono/jsx/dom'; +import { emitChanged } from './live.js'; + /** * "Which one is yours?" * @@ -92,6 +94,8 @@ const Identify = ({ base }: { base: string }) => { const [scores, setScores] = useState>({}); const [finding, setFinding] = useState(false); const [found, setFound] = useState(null); + /** What just happened, said here rather than via a redirect and a flash. */ + const [saved, setSaved] = useState(null); const load = async (): Promise => { try { @@ -190,20 +194,33 @@ const Identify = ({ base }: { base: string }) => { const target = athleteId === '' ? 'new' : athleteId; setBusy(picked[0] ?? 'saving'); setError(null); + setSaved(null); try { const response = await fetch(`${base}/athletes/${target}/track`, { method: 'POST', headers: { accept: 'application/json', 'content-type': 'application/json' }, body: JSON.stringify({ trackId: picked[0], trackIds: picked }), }); - const body = (await response.json()) as { ok: boolean; error?: string }; + const body = (await response.json()) as { ok: boolean; athleteId?: string; error?: string }; if (!response.ok || !body.ok) throw new Error(body.error ?? 'Could not save that choice.'); - // Re-scoring runs in the background; the job log shows it finishing. - window.location.assign( - `${base}?ok=${encodeURIComponent(`Athlete identified across ${picked.length} track(s) — re-scoring`)}`, - ); + + /** + * Stay on the page. + * + * This navigated, which meant every save cost a full reload — losing the + * grid, the scroll position and any sense of what had just happened. It + * also raced the user: a click made before the reload landed arrived with + * no athlete loaded, and the server minted another one. Seven duplicates + * came from exactly this. + */ + if (body.athleteId !== undefined) setAthleteId(body.athleteId); + setSaved(`Identified across ${picked.length} track(s) — re-scoring now`); + // Reload this island's own data, then let the rest of the page catch up. + await load(); + emitChanged(); } catch (cause) { setError(cause instanceof Error ? cause.message : String(cause)); + } finally { setBusy(null); } }; @@ -252,6 +269,7 @@ const Identify = ({ base }: { base: string }) => { )} {error === null ? null :

{error}

} + {saved === null ? null :

{saved}

} {candidates.length === 0 ? (

diff --git a/apps/web/src/client/jobs.tsx b/apps/web/src/client/jobs.tsx index 275ee8c..8d9c235 100644 --- a/apps/web/src/client/jobs.tsx +++ b/apps/web/src/client/jobs.tsx @@ -1,6 +1,8 @@ /** @jsxImportSource hono/jsx/dom */ import { render, useEffect, useRef, useState } from 'hono/jsx/dom'; +import { refreshLive } from './live.js'; + /** * The live analysis log. * @@ -104,9 +106,16 @@ const JobLog = ({ base }: { base: string }) => { if (next.some((job) => job.status === 'running' || job.status === 'queued')) { reloadWhenDone.current = true; } else if (reloadWhenDone.current && next.some((job) => job.status === 'completed')) { - // Analysis writes moments; the page around this island is stale now. + /** + * Analysis writes moments, so the page around this island is stale — + * but reloading it was a blunt instrument. It threw away scroll + * position, collapsed whatever the user had open, and interrupted a + * selection in progress; worse, it raced their clicks, which is how + * one athlete became seven. Swapping the server-rendered regions + * leaves everything else exactly where it was. + */ reloadWhenDone.current = false; - window.setTimeout(() => window.location.reload(), 1500); + void refreshLive(); } }); diff --git a/apps/web/src/client/live.ts b/apps/web/src/client/live.ts new file mode 100644 index 0000000..9d9b730 --- /dev/null +++ b/apps/web/src/client/live.ts @@ -0,0 +1,86 @@ +import { mountReview } from './review.js'; + +/** + * Bringing the page up to date without reloading it. + * + * The pages are server-rendered and must keep working with no JavaScript, so + * the server stays the single source of truth — there is no client router and + * no client-side view model to keep in sync. What there was instead was + * `window.location.reload()`, fired 1.5 seconds after any job finished, plus a + * "Analysis is running. Refresh for progress." link for the rest of the time. + * + * That is what made the app feel broken to a new user: the page moved under + * them without being asked, they lost their place, and between refreshes there + * was no way to tell whether to wait or to click again. Clicking again is + * exactly what produced seven duplicate athletes — the reloads were racing the + * clicks. + * + * So: fetch the same URL the user is already on, and swap only the regions + * marked `data-live`. The server renders them exactly as it always did, the + * islands that own state (the job log holding an EventSource, the identify + * grid holding a half-finished selection) are never inside one, and nothing + * scrolls. + */ + +/** Islands that live inside a swappable region and must be re-mounted after it. */ +const REMOUNT: Record void> = { + moments: mountReview, +}; + +let inFlight: Promise | null = null; + +/** + * Replaces every `data-live` region with the server's current version. + * + * Coalesced: several finishing jobs, or a save and a job completing together, + * should cost one fetch and one repaint rather than three. + */ +export const refreshLive = async (): Promise => { + if (inFlight !== null) return inFlight; + + inFlight = (async () => { + try { + const response = await fetch(window.location.href, { + headers: { accept: 'text/html' }, + credentials: 'same-origin', + }); + if (!response.ok) return; + const parsed = new DOMParser().parseFromString(await response.text(), 'text/html'); + + // Array.from rather than iterating the NodeList: the DOM lib this project + // compiles against does not give NodeListOf an iterator. + for (const current of Array.from(document.querySelectorAll('[data-live]'))) { + const key = current.dataset['live']; + if (key === undefined) continue; + const fresh = parsed.querySelector(`[data-live="${key}"]`); + // A region that has gone away entirely is left alone rather than + // blanked: an empty page is a worse lie than a stale one. + if (fresh === null) continue; + if (fresh.innerHTML === current.innerHTML) continue; + current.innerHTML = fresh.innerHTML; + REMOUNT[key]?.(); + } + } catch { + // Offline, or the session expired. The page is stale, which is survivable; + // throwing here would take an island down with it. + } finally { + inFlight = null; + } + })(); + + return inFlight; +}; + +type Listener = () => void; +const listeners = new Set(); + +/** Something changed the project; islands that show its data should catch up. */ +export const emitChanged = (): void => { + for (const listener of listeners) listener(); + void refreshLive(); +}; + +export const onChanged = (listener: Listener): (() => void) => { + listeners.add(listener); + return () => listeners.delete(listener); +}; diff --git a/apps/web/src/client/main.tsx b/apps/web/src/client/main.tsx index 8948cbd..84650ad 100644 --- a/apps/web/src/client/main.tsx +++ b/apps/web/src/client/main.tsx @@ -105,6 +105,9 @@ const MomentReview = ({ projectId, initial }: { projectId: string; initial: Mome }; const mount = (): void => { + // Proof the bundle ran, so the stylesheet can hide instructions that only + // make sense without it. + document.documentElement.classList.add('js'); // Independent of the review island, and present on pages that have no moments. mountUploads(); mountJobLog(); diff --git a/apps/web/src/client/upload.tsx b/apps/web/src/client/upload.tsx index 648f65b..51f74c6 100644 --- a/apps/web/src/client/upload.tsx +++ b/apps/web/src/client/upload.tsx @@ -1,6 +1,8 @@ /** @jsxImportSource hono/jsx/dom */ import { render, useEffect, useRef, useState } from 'hono/jsx/dom'; +import { refreshLive } from './live.js'; + /** * The realtime uploader. * @@ -301,9 +303,10 @@ const Uploader = ({ base }: { base: string }) => { name: finished.upload?.fileName ?? item.name, }); - // Everything settled: bring the page's own video list up to date. + // Everything settled: bring the page's own video list up to date. Swapped + // in place, so a second upload queued behind this one is not interrupted. if (items.current.every((entry) => entry.phase === 'done' || entry.phase === 'canceled')) { - window.setTimeout(() => window.location.reload(), 1200); + void refreshLive(); } } catch (error) { // A canceled request is a user action, not a failure to report. @@ -402,7 +405,7 @@ const Uploader = ({ base }: { base: string }) => { try { await api(`${base}/uploads/${dto.id}/finish`, { method: 'POST' }); await refresh(); - window.setTimeout(() => window.location.reload(), 800); + void refreshLive(); } catch (error) { setListError(error instanceof Error ? error.message : String(error)); } diff --git a/apps/web/src/liveregions.test.ts b/apps/web/src/liveregions.test.ts new file mode 100644 index 0000000..babf9ee --- /dev/null +++ b/apps/web/src/liveregions.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it } from 'vitest'; + +import { ProjectPage } from './views/pages.js'; +import type { ProjectView } from './views/pages.js'; + +/** + * The page updates itself by fetching its own URL and swapping the regions + * marked `data-live`. That only works while the islands holding state stay + * outside them. + * + * The job log owns an EventSource; swapping it would drop the analysis feed and + * silently reconnect. The identify grid holds a selection the user is part-way + * through making; swapping it would discard their clicks — which is close to + * the original bug, where reloads raced clicks into seven duplicate athletes. + * + * `#moment-review` is the exception: it is inside `data-live="moments"` on + * purpose, because it re-reads everything from a `data-moments` attribute, and + * live.ts re-mounts it after a swap. + */ + +const view: ProjectView = { + project: { id: 'prj_test', name: 'Smoke', sport: 'basketball' } as ProjectView['project'], + videos: [], + athletes: [], + moments: [], + clips: [], + jobs: [], + exports: [], + music: [], + flash: {}, +}; + +const render = async (extra: Partial = {}): Promise => + String(await ProjectPage({ ...view, ...extra })); + +/** Crude but sufficient: is `needle` inside a `data-live` block in `html`? */ +const insideLiveRegion = (html: string, needle: string): boolean => { + const at = html.indexOf(needle); + if (at < 0) return false; + const before = html.slice(0, at); + const opens = (before.match(/

/g)) { + if (match[0] === '
0) depth += 1; + else if (match[0] === '
' && depth > 0) { + depth -= 1; + if (depth === 0) closed += 1; + } + } + return opens > closed; +}; + +describe('the regions the page swaps in place', () => { + it('marks the ones that go stale after a job', async () => { + const html = await render(); + for (const key of ['videos', 'athletes', 'moments']) { + expect(html).toContain(`data-live="${key}"`); + } + }); + + it('keeps the job log outside them, so the analysis feed survives', async () => { + // An EventSource inside a swapped region is dropped and reconnected on + // every update, losing the log the user is reading. + const html = await render({ jobs: [] }); + expect(html).toContain('id="job-log"'); + expect(insideLiveRegion(html, 'id="job-log"')).toBe(false); + }); + + it('keeps the identify grid outside them, so a half-made selection survives', async () => { + const html = await render(); + expect(html).toContain('id="identify-athlete"'); + expect(insideLiveRegion(html, 'id="identify-athlete"')).toBe(false); + }); + + it('keeps the uploader outside them, so an upload in flight is not interrupted', async () => { + const html = await render(); + const at = html.indexOf('id="uploads"'); + if (at >= 0) expect(insideLiveRegion(html, 'id="uploads"')).toBe(false); + }); + + it('stops telling a user to refresh once the page can update itself', async () => { + const running = [ + { id: 'job_x', kind: 'detection', status: 'running', progress: 0.5 }, + ] as unknown as ProjectView['jobs']; + const html = await render({ jobs: running }); + // Still present for the no-JavaScript case, but hidden once the bundle runs. + expect(html).toContain('no-js-only'); + const at = html.indexOf('Analysis is running'); + expect(html.slice(Math.max(0, at - 200), at)).toContain('no-js-only'); + }); +}); diff --git a/apps/web/src/views/Layout.tsx b/apps/web/src/views/Layout.tsx index bf4cc11..916d7b1 100644 --- a/apps/web/src/views/Layout.tsx +++ b/apps/web/src/views/Layout.tsx @@ -37,6 +37,9 @@ const STYLES = ` h1 { font-size: 1.4rem; margin: 0 0 .25rem; } h2 { font-size: 1.05rem; margin: 2rem 0 .75rem; } .muted { color: var(--muted); } + /* Advice that only applies without JavaScript — telling someone to refresh + is actively wrong once the page updates itself. */ + html.js .no-js-only { display: none; } .card { border: 1px solid var(--line); border-radius: .6rem; padding: .9rem 1rem; margin-bottom: .6rem; background: var(--card); diff --git a/apps/web/src/views/pages.tsx b/apps/web/src/views/pages.tsx index 7fdcae4..f73d937 100644 --- a/apps/web/src/views/pages.tsx +++ b/apps/web/src/views/pages.tsx @@ -156,6 +156,8 @@ export const ProjectPage: FC = ({

Footage

+ {/* The uploader below posts into this list; swapped rather than reloaded. */} +
{videos.length === 0 ? (

Nothing imported yet.

) : ( @@ -191,6 +193,8 @@ export const ProjectPage: FC = ({ )} +
+
Import footage @@ -226,6 +230,10 @@ export const ProjectPage: FC = ({

Athlete to follow

+ {/* Swapped in place when anything changes it — see client/live.ts. The + identify grid below is deliberately outside: it holds a selection the + user is part-way through making. */} +
{athletes.length === 0 ? (

Nobody yet. Analysis needs someone to follow.

) : ( @@ -288,6 +296,7 @@ export const ProjectPage: FC = ({ +
{/* The one irreducibly manual step: a detector cannot know which player is yours. Until an athlete is bound to a track, scoring has no focal @@ -416,14 +425,19 @@ export const ProjectPage: FC = ({ )} + {/* The no-JavaScript fallback only. With the bundle loaded the job log + below streams progress and the page updates itself, so telling a + user to refresh — the thing that loses their place and races their + clicks — would be advice to make it worse. */} {jobs.some((job) => job.status === 'running' || job.status === 'queued') ? ( -

+

Analysis is running. Refresh for progress.

) : null}

Suggested moments

+
{/* The honest answer to "is it even following the right kid" — and the place to fix it when it is not. */}

@@ -513,6 +527,7 @@ export const ProjectPage: FC = ({

)} +

Reel