From 9b7cae1fec887e36f19e8f955f52ba7ea01050fb Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 10 Aug 2026 14:37:53 +0000 Subject: [PATCH] feat: stop reloading the page out from under people MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "we just need to make the page not refresh and use ajax/realtime event updates and logging and status bars or spinners and shit. its hard to tell when i should wait or take action." There is no client router and there should not be one: the pages are server-rendered and the project commits to working without JavaScript. What there was instead was `window.location.reload()` — fired 1.5s after any job finished, twice more after uploads — and a "Analysis is running. Refresh for progress." link for the gaps in between. Four hard reloads and an instruction to do a fifth by hand. That is what made it impossible to tell whether to wait or to click. It also raced the user: a click landing before a reload arrived with no athlete loaded, and the server minted another one. Seven duplicate athletes came from that race, so this and the identify fix are the same bug seen from two ends. `client/live.ts` fetches the URL the user is already on and swaps only the regions marked `data-live` — the footage list, the athlete table, the suggested moments. The server stays the single source of truth, no-JavaScript keeps working unchanged, nothing scrolls, and the islands that own state are outside every region: the job log keeps its EventSource, the identify grid keeps a half-finished selection. `#moment-review` sits inside one deliberately, because it re-reads its data from an attribute, and is re-mounted after a swap. Identifying an athlete no longer navigates at all. It reports what happened in place, refreshes its own grid, and lets the rest of the page catch up. apps/web/src/liveregions.test.ts pins the invariant that makes this safe: the job log, the identify grid and the uploader are outside every swappable region. Move one inside and the test fails rather than the feed dying silently in production. Co-Authored-By: Claude Opus 5 (1M context) --- apps/web/src/client/identify.tsx | 28 ++++++++-- apps/web/src/client/jobs.tsx | 13 ++++- apps/web/src/client/live.ts | 86 +++++++++++++++++++++++++++++ apps/web/src/client/main.tsx | 3 + apps/web/src/client/upload.tsx | 9 ++- apps/web/src/liveregions.test.ts | 94 ++++++++++++++++++++++++++++++++ apps/web/src/views/Layout.tsx | 3 + apps/web/src/views/pages.tsx | 17 +++++- 8 files changed, 242 insertions(+), 11 deletions(-) create mode 100644 apps/web/src/client/live.ts create mode 100644 apps/web/src/liveregions.test.ts 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