From 9874f0ec9c145783c047366c5e0ae2f81d864a6e Mon Sep 17 00:00:00 2001
From: Ritishsaini06 <ritish.s@wizcommerce.com>
Date: Tue, 25 Aug 2026 23:28:44 +0530
Subject: [PATCH] Work the review queue without the mouse

Forty-four rows at roughly eight tab stops each is where a queue stops
being reviewed and starts being dismissed. The whole loop now has keys:
j/k move a cursor, 1-9 choose a candidate, Enter confirms, x rejects,
/ reaches the search field and Escape leaves it.

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
rather than as a class, so a screen reader is told the cursor moved and
the row scrolls 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. 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, each with 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".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 HANDOFF.md                              |  32 ++++-
 frontend/src/screens/Review.css         |  62 +++++++++-
 frontend/src/screens/Review.jsx         | 152 ++++++++++++++++++++++--
 frontend/src/screens/reviewKeys.js      |  57 +++++++++
 frontend/src/screens/reviewKeys.test.js |  77 ++++++++++++
 5 files changed, 366 insertions(+), 14 deletions(-)
 create mode 100644 frontend/src/screens/reviewKeys.js
 create mode 100644 frontend/src/screens/reviewKeys.test.js

diff --git a/HANDOFF.md b/HANDOFF.md
index c3ce223..b23e79c 100644
--- a/HANDOFF.md
+++ b/HANDOFF.md
@@ -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.
 
diff --git a/frontend/src/screens/Review.css b/frontend/src/screens/Review.css
index e9e2777..35216fa 100644
--- a/frontend/src/screens/Review.css
+++ b/frontend/src/screens/Review.css
@@ -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); }
 
@@ -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;
@@ -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);
@@ -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; }
 }
 
diff --git a/frontend/src/screens/Review.jsx b/frontend/src/screens/Review.jsx
index dfd187a..8d74289 100644
--- a/frontend/src/screens/Review.jsx
+++ b/frontend/src/screens/Review.jsx
@@ -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";
 
 /**
@@ -12,9 +13,15 @@ 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
@@ -22,6 +29,12 @@ function Candidate({ c, chosen, onPick }) {
         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>
@@ -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({
@@ -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)),
   });
@@ -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">
@@ -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}
@@ -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)}
           />
@@ -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({
@@ -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>;
 
@@ -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 && (
@@ -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>
@@ -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>
diff --git a/frontend/src/screens/reviewKeys.js b/frontend/src/screens/reviewKeys.js
new file mode 100644
index 0000000..16f3fbd
--- /dev/null
+++ b/frontend/src/screens/reviewKeys.js
@@ -0,0 +1,57 @@
+/**
+ * One keypress to one intent, for the review queue.
+ *
+ * Pure, and separate from the queue itself, for the same reason `idleDelayMs`
+ * is separate from `useIdleSignOut`: the wiring is two `addEventListener`
+ * calls and a cleanup that mirrors them, which reading the file shows. *Which
+ * key means what, and when a key means nothing at all* is the part that can be
+ * quietly wrong, and being wrong here is not cosmetic — a shortcut that fires
+ * while somebody is typing into the LOINC search sends `x` to "not a lab
+ * result" instead of into the field.
+ *
+ * Returning `null` is the common answer and the important one. This handler is
+ * on `window`, so every keystroke anywhere on the screen passes through it.
+ */
+
+const TEXT_FIELD = /^(input|textarea|select)$/;
+
+export function keyAction(e) {
+  // A modifier means the browser or the OS is being addressed, not us.
+  if (e.ctrlKey || e.metaKey || e.altKey) return null;
+
+  const target = e.target ?? {};
+  const tag = (target.tagName ?? "").toLowerCase();
+
+  // Typing beats every shortcut. The search field accepts 58k codes' worth of
+  // letters and every one of them is also an accelerator here.
+  if (TEXT_FIELD.test(tag) || target.isContentEditable) {
+    return e.key === "Escape" ? { type: "leaveSearch" } : null;
+  }
+
+  switch (e.key) {
+    case "j":
+    case "ArrowDown":
+      return { type: "next" };
+    case "k":
+    case "ArrowUp":
+      return { type: "prev" };
+    case "x":
+      return { type: "reject" };
+    case "/":
+      return { type: "search" };
+    case "Enter":
+      // A focused button already answers to Enter, and on this screen that
+      // button is a candidate code. Claiming Enter as well would settle the
+      // row on the keypress that only meant "choose this one".
+      return tag === "button" ? null : { type: "confirm" };
+    default: {
+      // 1–9 pick the nth candidate as listed. Not 0: there is no tenth row
+      // worth a shortcut, and a queue where the digits are off by one is
+      // worse than a queue with no digits.
+      const n = Number(e.key);
+      return Number.isInteger(n) && n >= 1 && n <= 9
+        ? { type: "pick", index: n - 1 }
+        : null;
+    }
+  }
+}
diff --git a/frontend/src/screens/reviewKeys.test.js b/frontend/src/screens/reviewKeys.test.js
new file mode 100644
index 0000000..a8411d4
--- /dev/null
+++ b/frontend/src/screens/reviewKeys.test.js
@@ -0,0 +1,77 @@
+import { describe, expect, it } from "vitest";
+import { keyAction } from "./reviewKeys.js";
+
+/**
+ * Only the mapping. The listeners are two `addEventListener` calls and a
+ * cleanup that mirrors them; what can be quietly wrong is which keystrokes
+ * this claims, and the expensive failure is claiming one it should not have.
+ */
+
+const press = (key, extra = {}) => ({
+  key,
+  ctrlKey: false,
+  metaKey: false,
+  altKey: false,
+  target: { tagName: "LI" },
+  ...extra,
+});
+
+const inField = (key, tagName = "INPUT") =>
+  press(key, { target: { tagName } });
+
+describe("keyAction", () => {
+  it("moves down and up on both the letters and the arrows", () => {
+    for (const k of ["j", "ArrowDown"]) {
+      expect(keyAction(press(k))).toEqual({ type: "next" });
+    }
+    for (const k of ["k", "ArrowUp"]) {
+      expect(keyAction(press(k))).toEqual({ type: "prev" });
+    }
+  });
+
+  it("maps 1-9 to a zero-based candidate index", () => {
+    expect(keyAction(press("1"))).toEqual({ type: "pick", index: 0 });
+    expect(keyAction(press("9"))).toEqual({ type: "pick", index: 8 });
+    // 0 is not a tenth candidate, and an off-by-one queue is worse than none.
+    expect(keyAction(press("0"))).toBeNull();
+  });
+
+  it("keeps its hands off a text field", () => {
+    // The bug this exists to stop: typing "flux" into the LOINC search
+    // rejecting the row on the x, and jumping the cursor on the j.
+    for (const k of ["x", "j", "k", "1", "/", "Enter"]) {
+      expect(keyAction(inField(k))).toBeNull();
+      expect(keyAction(inField(k, "TEXTAREA"))).toBeNull();
+      expect(keyAction(inField(k, "SELECT"))).toBeNull();
+    }
+    expect(
+      keyAction(press("x", { target: { tagName: "DIV", isContentEditable: true } }))
+    ).toBeNull();
+  });
+
+  it("leaves a field on Escape, and only on Escape", () => {
+    expect(keyAction(inField("Escape"))).toEqual({ type: "leaveSearch" });
+    expect(keyAction(press("Escape"))).toBeNull();
+  });
+
+  it("lets a focused button keep Enter", () => {
+    // Enter on a candidate means "choose this one". If it also confirmed, the
+    // row would settle on the keypress that was only meant to select.
+    expect(keyAction(press("Enter", { target: { tagName: "BUTTON" } }))).toBeNull();
+    expect(keyAction(press("Enter"))).toEqual({ type: "confirm" });
+  });
+
+  it("yields every combination to the browser and the OS", () => {
+    for (const mod of ["ctrlKey", "metaKey", "altKey"]) {
+      // ⌘K is a browser search bar, not a cursor move.
+      expect(keyAction(press("k", { [mod]: true }))).toBeNull();
+      expect(keyAction(press("Enter", { [mod]: true }))).toBeNull();
+    }
+  });
+
+  it("ignores everything it was not given a meaning for", () => {
+    for (const k of [" ", "q", "Tab", "F5", "Shift", "ArrowLeft"]) {
+      expect(keyAction(press(k))).toBeNull();
+    }
+  });
+});