Skip to content

Work the review queue without the mouse #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MaXiMo000 merged 1 commit into from
Aug 25, 2026
Merged

Work the review queue without the mouse #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions HANDOFF.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,8 +549,36 @@ structurally sound and in the right unit.
- **The upload screen does not say what happens next.** A dropped file goes
`queued → extracting → mapping → needs_review`, and only the last one is
actionable. Say so on the screen rather than in status labels.
- **Keyboard path through the review queue.** Confirm/reject/skip without the
mouse is the difference between reviewing forty-four items and abandoning it.
- ~~**Keyboard path through the review queue.**~~ Done. `j`/`k` move a cursor,
`1`–`9` choose a candidate, `Enter` confirms, `x` rejects, `/` reaches the
search field and `Escape` leaves it. There is no skip key: moving the cursor
*is* skipping, and a second key for it would only be a second way to lose
your place.

**The cursor is an index into a list that shrinks underneath it**, which is
how it advances — settle the row it points at, the row after it takes that
position, and the cursor is already on the next thing to do. Clamped rather
than corrected, so settling the last row lands on the new last row instead of
running off the end. Focus follows it for real (`tabIndex={-1}` on the row),
not as a class, so a screen reader is told the cursor moved and the row is
scrolled into view without asking — but only once a key has been pressed,
because moving focus on mount yanks the page for somebody who arrived with a
mouse.

**`keyAction` is pure and tested; the listeners are not.** Same split as
`idleDelayMs`: two `addEventListener` calls and a cleanup that mirrors them
is what reading the file shows, while *which keystrokes this claims* is what
can be quietly wrong. The expensive failure is claiming one it should not
have — a `window` handler sees every keystroke on the screen, so typing
"flux" into a LOINC search must not reject the row on the `x`. Text fields,
modifier combinations, and Enter on an already-focused button are all
returned as "not ours", and each has a test.

Two bugs the screen had all along surfaced while driving it: the tally
counted rejections as **rules learned**, when `/reject` answers
`alias_written: false` and means it — the one counter whose job is to make
convergence visible rather than claimed was overstating it. And a queue of
one read "1 result need you".
- **`prefers-reduced-motion`.** The cascade scene animates unconditionally.
- **A real 404 route.** Unknown `/app/*` paths currently render the shell empty.

Expand Down
62 changes: 60 additions & 2 deletions frontend/src/screens/Review.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,38 @@
.review__lede { color: var(--ink-body); max-width: 58ch; margin-bottom: var(--s6); }
.review__lede { color: var(--ink-body); max-width: 58ch; margin-bottom: var(--s3); }

/* ---- the keyboard legend -------------------------------------------------
Set in the caption size and faint ink: it is a reference somebody reads
once and then stops seeing, so it must not compete with the queue it sits
above. Hidden where there is no keyboard — a phone showing key caps it
cannot press is describing somebody else's screen. */

.review__keys {
display: flex;
flex-wrap: wrap;
align-items: center;
gap: var(--s1);
margin: 0 0 var(--s6);
font-size: var(--text-caption);
color: var(--ink-faint);
}
.review__keys-sep { margin: 0 var(--s2); color: var(--rule-strong); }

.review__keys kbd {
font-family: var(--font-mono);
font-size: var(--text-caption);
line-height: 1;
padding: 3px 5px;
margin-right: 2px;
color: var(--ink-muted);
background: var(--paper-sunk);
border: 1px solid var(--rule);
border-bottom-width: 2px;
border-radius: var(--radius);
}

@media (hover: none) {
.review__keys { display: none; }
}

.ritems { list-style: none; margin: 0; padding: 0; display: grid; gap: var(--s5); }

Expand All @@ -9,6 +43,18 @@
border-radius: var(--radius-lg);
}

/* The cursor. A rule down the edge rather than a fill or a ring: several rows
are on screen at once, so it has to be findable at a glance without becoming
the loudest thing in a queue whose whole content is the evidence. It is also
the focus indicator — the row really does take focus — so the default
outline is dropped only because this replaces it. */
.ritem--on {
border-color: var(--rule-strong);
box-shadow: inset 3px 0 0 var(--accent);
}
.ritem:focus,
.ritem:focus-visible { outline: none; }

/* The printed line, verbatim and set apart: this is the evidence. */
.ritem__printed {
display: flex;
Expand All @@ -31,7 +77,7 @@

.cand {
display: grid;
grid-template-columns: 5.5rem 1fr auto;
grid-template-columns: 1.1rem 5.5rem 1fr auto;
gap: var(--s3);
width: 100%;
padding: var(--s3);
Expand All @@ -46,16 +92,28 @@
row and a filled state would read as already-confirmed. */
.cand--on { border-color: var(--accent); background: var(--accent-wash); }

/* The digit that picks this row. Faintest ink on the screen: it is an
accelerator, not part of the evidence, and it must not read as an ordering
the candidates have been ranked into. */
.cand__key { font-size: var(--text-caption); color: var(--rule-strong); }
.cand__code { font-size: var(--text-small); color: var(--ink); }
.cand__name { font-size: var(--text-small); color: var(--ink-body); }
.cand__why { font-size: var(--text-caption); color: var(--ink-faint); white-space: nowrap; }

/* No keyboard, no accelerators to advertise — same test the legend uses. */
@media (hover: none) {
.cand { grid-template-columns: 5.5rem 1fr auto; }
.cand__key { display: none; }
}

.ritem__actions { display: flex; flex-wrap: wrap; gap: var(--s3); margin-top: var(--s5); }

@media (max-width: 640px) {
.ritem { padding: var(--s4); }
.ritem__meta { margin-left: 0; width: 100%; }
.cand { grid-template-columns: 1fr; gap: 2px; }
/* Stacked, the digit would take a line of its own for one character. */
.cand__key { display: none; }
.cand__why { white-space: normal; }
}

Expand Down
152 changes: 142 additions & 10 deletions frontend/src/screens/Review.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
import { useState } from "react";
import { useEffect, useRef, useState } from "react";
import { Link } from "react-router-dom";
import { api, messageFor } from "../api/client";
import { usePatient } from "../patients/PatientContext";
import { keyAction } from "./reviewKeys";
import "./Review.css";

/**
Expand All @@ -12,16 +13,28 @@ import "./Review.css";
* it might be, with the reason each candidate surfaced. Confirming writes an
* alias, so the same printed name resolves without asking again — the counter
* at the top is there so that convergence is visible rather than claimed.
*
* It is also the one screen somebody works *down*. Forty-four rows at eight tab
* stops each is where a queue stops being reviewed and starts being dismissed,
* so the whole loop — move, choose, confirm — has keys. The queue keeps a
* cursor; the row under it owns the keys that settle a row, the list owns the
* keys that move between them.
*/

function Candidate({ c, chosen, onPick }) {
function Candidate({ c, chosen, onPick, ordinal }) {
return (
<li>
<button
className={`cand ${chosen ? "cand--on" : ""}`}
onClick={onPick}
aria-pressed={chosen}
>
{/* The digit that picks this one. Only the first nine get one, and a
row with more candidates than that still reads correctly — the
unnumbered ones are the ones the mouse is for. */}
<span className="cand__key num" aria-hidden="true">
{ordinal <= 9 ? ordinal : ""}
</span>
<span className="cand__code num">{c.loinc_code}</span>
<span className="cand__name">{c.display}</span>
<span className="cand__why num">{c.why}</span>
Expand All @@ -30,10 +43,12 @@ function Candidate({ c, chosen, onPick }) {
);
}

function Item({ item, onDone }) {
function Item({ item, focused, takeFocus, onDone }) {
const [picked, setPicked] = useState(item.proposed_loinc ?? null);
const [query, setQuery] = useState("");
const [error, setError] = useState(null);
const row = useRef(null);
const search = useRef(null);
const qc = useQueryClient();

const { data: hits } = useQuery({
Expand All @@ -48,11 +63,13 @@ function Item({ item, onDone }) {
action === "reject"
? api.post(`/review/item/${item.observation_id}/reject`)
: api.post(`/review/item/${item.observation_id}/confirm`, { loinc_code: picked }),
onSuccess: () => {
onSuccess: (_res, action) => {
["review", "panels", "documents"].forEach((k) =>
qc.invalidateQueries({ queryKey: [k] })
);
onDone();
// Which one it was, because only a confirm writes an alias — the reject
// endpoint answers `alias_written: false` and means it.
onDone(action);
},
onError: (err) => setError(messageFor(err)),
});
Expand All @@ -65,8 +82,59 @@ function Item({ item, onDone }) {
}))
: item.candidates;

// Held in a ref, like `useDialog` holds its close handler, so the effect
// below stays keyed on `focused` alone. Everything this reads — the pick,
// the current options, whether a save is in flight — changes on renders that
// have no business tearing down and re-attaching a window listener.
const handle = useRef(null);
handle.current = (e) => {
const action = keyAction(e);
if (!action) return;

if (action.type === "leaveSearch") {
// Back to the queue rather than out of the screen: the row is still the
// row, and the next keystroke should move the cursor, not the caret.
e.target.blur();
row.current?.focus();
return;
}
if (action.type === "search") {
// Or the "/" lands in the field as its first character.
e.preventDefault();
search.current?.focus();
return;
}
if (action.type === "pick") {
const chosen = options[action.index];
if (chosen) setPicked(chosen.loinc_code);
return;
}
if (settle.isPending) return;
if (action.type === "confirm" && picked) settle.mutate("confirm");
if (action.type === "reject") settle.mutate("reject");
};

useEffect(() => {
if (!focused) return undefined;
const onKey = (e) => handle.current(e);
window.addEventListener("keydown", onKey);
return () => window.removeEventListener("keydown", onKey);
}, [focused]);

// Real focus, not a class, so a screen reader is told the cursor moved and
// the row is scrolled into view without asking. Only once the keyboard is
// actually in use — moving focus on mount would yank the page for somebody
// who arrived with a mouse and has not pressed anything yet.
useEffect(() => {
if (takeFocus) row.current?.focus();
}, [takeFocus]);

return (
<li className="ritem">
<li
ref={row}
tabIndex={-1}
className={`ritem ${focused ? "ritem--on" : ""}`}
>
{/* The printed line, verbatim. This is the thing being judged, so it is
set as it appeared on the page, not reformatted. */}
<div className="ritem__printed">
Expand All @@ -87,6 +155,7 @@ function Item({ item, onDone }) {
<label className="ritem__search">
<span className="sr-only">Search all LOINC codes</span>
<input
ref={search}
className="field__input"
placeholder="Search all 58,252 codes…"
value={query}
Expand All @@ -95,10 +164,11 @@ function Item({ item, onDone }) {
</label>

<ul className="cands">
{options.map((c) => (
{options.map((c, i) => (
<Candidate
key={c.loinc_code}
c={c}
ordinal={i + 1}
chosen={picked === c.loinc_code}
onPick={() => setPicked(c.loinc_code)}
/>
Expand Down Expand Up @@ -136,6 +206,8 @@ function Item({ item, onDone }) {

export default function Review() {
const [confirmed, setConfirmed] = useState(0);
const [cursor, setCursor] = useState(0);
const [byKeyboard, setByKeyboard] = useState(false);
const { activeId } = usePatient();

const { data: items, isPending, error } = useQuery({
Expand All @@ -144,6 +216,36 @@ export default function Review() {
enabled: Boolean(activeId),
});

// Before the early returns: hooks cannot be conditional, and an empty queue
// is a perfectly ordinary state for this to run against.
const groups = groupQueue(items ?? []);
const order = groups.flatMap((g) => g.rows);
// The cursor is an index into a list that shrinks underneath it, and that is
// how it advances: settle the row it points at, the row after it takes that
// position, and the cursor is already on the next thing to do. Clamped
// rather than corrected, so settling the last row lands on the new last row
// instead of running off the end.
const at = Math.min(cursor, Math.max(order.length - 1, 0));
const focusedId = order[at]?.observation_id;

const step = useRef(null);
step.current = (e) => {
const action = keyAction(e);
if (action?.type !== "next" && action?.type !== "prev") return;
// Or ArrowDown scrolls the page out from under the row it just moved to.
e.preventDefault();
setByKeyboard(true);
setCursor(
Math.max(0, Math.min(at + (action.type === "next" ? 1 : -1), order.length - 1))
);
};

useEffect(() => {
const onKey = (e) => step.current(e);
window.addEventListener("keydown", onKey);
return () => window.removeEventListener("keydown", onKey);
}, []);

if (!activeId || isPending) return <p className="muted">Loading the queue…</p>;
if (error) return <p className="muted">Could not load the review queue.</p>;

Expand All @@ -167,7 +269,9 @@ export default function Review() {
<div>
<p className="eyebrow">Review</p>
<h1 className="screen__title">
{items.length} result{items.length > 1 ? "s" : ""} need you
{items.length === 1
? "1 result needs you"
: `${items.length} results need you`}
</h1>
</div>
{confirmed > 0 && (
Expand All @@ -187,7 +291,23 @@ export default function Review() {
from then on.
</p>

{groupQueue(items).map((g) => (
{/* Hidden where there is no keyboard to describe. A phone showing a row
of key caps it cannot press is telling the reader about somebody
else's screen. */}
<p className="review__keys">
<kbd>J</kbd>
<kbd>K</kbd> move
<span className="review__keys-sep">·</span>
<kbd>1</kbd>–<kbd>9</kbd> choose
<span className="review__keys-sep">·</span>
<kbd>↵</kbd> confirm
<span className="review__keys-sep">·</span>
<kbd>X</kbd> not a result
<span className="review__keys-sep">·</span>
<kbd>/</kbd> search
</p>

{groups.map((g) => (
<section key={g.key} className="rgroup">
<h2 className="rgroup__h">
<span className="rgroup__label">{g.label}</span>
Expand All @@ -200,7 +320,19 @@ export default function Review() {
<Item
key={item.observation_id}
item={item}
onDone={() => setConfirmed((n) => n + 1)}
focused={item.observation_id === focusedId}
takeFocus={byKeyboard && item.observation_id === focusedId}
onDone={(action) => {
// Rejecting teaches nothing — the endpoint writes no alias —
// so counting it here made the tally overstate exactly what
// it exists to keep honest.
if (action === "confirm") setConfirmed((n) => n + 1);
// Whatever settled this row — a key or a click — focus was
// inside a row that is about to be unmounted. Parking it on
// the row that takes its place beats dropping it to <body>,
// which is a keyboard user back at the top of the document.
setByKeyboard(true);
}}
/>
))}
</ul>
Expand Down
Loading
Loading