Review workspace has no next/previous navigation or queue context #95
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Severity: HIGH - the single most consequential UX gap
The problem
The review workspace has no next or previous control, no queue position, no filmstrip, and no
advance after a decision. After clicking Approve you stay on the same photo with only a badge
change as feedback, then must click "← Back", re-find your place in a grid that has reset its
scroll, and click the next tile.
docs/circa-wireframes.md§3.2 specifies Prev/Next and a queue count. None of it was built. Theonly tracked coverage, #45 (keyboard shortcuts), is scheduled in the final milestone and
presupposes navigation targets that nothing creates.
Multiply by thousands of photos over months. This is the difference between a tool that gets used
and one that gets abandoned.
Recommended architecture
Rather than bolting "Next" onto the grid, define a queue as
(filter, sort, cursor)that theworkspace consumes:
itself via
new Image().src.This same substrate is what the filmstrip, the disputes queue (#23), and the duplicates queue
(#17) all need. Building it once avoids three ad-hoc navigation models.
Scope
reviewers will want to stay and add a comment).
keythe DecisionForm byphoto.id— itsuseState-from-props initialization will otherwiseshow stale drafts once in-place navigation exists.
Note the current "queue" — the needs_review filter — reorders as statuses change, so reviewed
photos drop out and the list shifts under you. The persisted cursor must be robust to that.
Done when
References
frontend/src/pages/ReviewWorkspacePage.tsx(toolbar 452-473, only exit is "← Back" at 441)docs/circa-wireframes.md§3.2Blocks: #45 (keyboard shortcuts) needs these targets to bind to.
Picking this up. Probed first; the architecture the issue recommends is right, but two details in it do not survive contact with the API, and both were settled with @rbrooks before starting.
1. "Position 214 of 1,830" cannot be built, and the wireframe does not ask for it. The issue says the position indicator "falls out naturally". It does not:
ListMetahas no total, deliberately — #83 removed it because it puts aCOUNTover the whole collection on the hot list path, and its docstring says so.GET /photos/statscounts by status only, so it cannot answer the date-range, undated or flag filters #94 just added, which is exactly what a queue is now defined by.docs/circa-wireframes.md§3.2 — the reference this issue itself cites — saysQueue: 32 left, not a position and total. That is both cheaper and the more useful thing for someone working a session, so the wireframe wins and no count endpoint is being added.Where the queue is longer than the snapshot, the count is rendered
N+ left. The+is the point: the software has not established the true remainder, and printing a number it cannot stand behind is the failure #78 is about.2. The queue is a snapshot, taken on entry. The issue flags that "the current queue — the
needs_reviewfilter — reorders as statuses change, so reviewed photos drop out and the list shifts under you", and asks for a cursor robust to that. A live re-query cannot be made robust to it: approving photo 214 removes it from its own queue, so every subsequent photo renumbers andNexteither skips or repeats depending on timing.So the queue is a frozen ordered list of ids captured when the reviewer enters the workspace. A photo just decided stays in place, marked done, rather than vanishing underneath the reviewer. Position never jumps, prefetch is trivial, and resume-across-sessions is just persisting the list. The cost, stated plainly: photos ingested mid-session do not appear until the queue is rebuilt.
The snapshot is bounded rather than exhaustive. Building it by paging a 30,000-photo queue would be 150 requests at the API's
limitceiling of 200, which is not a page load. It is capped, andcappedis what turns32 leftinto32+ left.Scope being built — the issue's list, unchanged except as above:
(filter, sort)plus a frozen id list, handed from the browser as router state and persisted for resume.N left/N+ left.new Image().keytheDecisionFormbyphoto.id. The issue is right that this is load-bearing and it is worth being explicit about why: the form seedsuseStatefrom props, and today the workspace is only ever entered by a fresh mount, so nothing has exposed it. In-place navigation reuses the component instance, and without the key the previous photograph's draft dates would sit in the form over the next photograph — a reviewer could approve one photo with another's date and nothing on screen would say so.Deliberately not in scope: the filmstrip and histogram in §3.2's wireframe, which are their own work, and keyboard shortcuts, which are #45 and are blocked on exactly the navigation targets this creates.
Done in
0afccbf(auto-closed by the commit trailer). CI run #57 green on all three jobs.Done when:
N leftper the wireframe, not214 of 1,830; see my earlier comment for why the total is not buildable121 frontend tests (was 87) and 7 e2e journeys.
The
key={photo.id}fix deserves a note, because the test for it was wrong first and that is the interesting part.The issue predicted this defect exactly, and it is real. What neither of us anticipated is that prefetching is what makes it reachable. When the reviewer moves to the next photograph, the workspace's
useQueryfor the new id has no cached data, so the page early-returnsLoading photo…— which unmountsDecisionFormand gives it fresh state whether it is keyed or not. The stale draft cannot occur.Prefetch removes that. With the next photograph already in the cache there is no loading state, the component instance is reused, and the previous photograph's dates sit in the form. So the two halves of this issue's scope interact: the prefetch requested in one bullet is what arms the bug named in another.
The first version of the test moved between photographs without seeding the cache, so it passed with
key={photo.id}deleted — a test asserting the right thing about the wrong path. It now seeds the query cache the way the prefetch does, and fails correctly when the key is removed. Three other mutations were checked and each failed exactly the test written for it:cappedignoring the overshoot case, auto-advance ignoring its own preference, and advancing past a decision the server refused.One limit worth stating rather than leaving to be discovered. The snapshot is capped at 500 ids, so a queue longer than that reports
N+ leftand Next stops at the 500th. Reaching the end means going back to the grid to take a fresh snapshot. That is a deliberate trade — the alternative was 150 round trips before first render on a 30,000-photo queue — but it is a real edge a reviewer working a very long session will hit, and if it becomes annoying the fix is to extend the snapshot in the background rather than to raise the cap.Three existing photo-browser tests had to change:
navigatenow takes queue state as a second argument. Not a regression, but worth knowing the call signature moved. I added an assertion that the actual current filter travels, rather than only that some state does.#45 (keyboard shortcuts) is now unblocked —
previousandnextinuseReviewQueueare the targets it needs to bind to.