Users: move flock-assignment into the shared dialog; keep capture forms inline (#133)

web/src/routes/HelpPage.tsx

@@ -165,9 +165,10 @@ export function HelpPage() {
         <li>
           Drill-downs work the same way. Open a flock&apos;s{" "}
           <strong>birds</strong> ledger to record a cull, an inventory item to
-          record a purchase, feed usage, or a stock correction, or an order to{" "}
-          <strong>record a payment</strong> — the ledger stays where it is and
-          the form comes to you.
+          record a purchase, feed usage, or a stock correction, an order to{" "}
+          <strong>record a payment</strong>, or a worker&apos;s{" "}
+          <strong>flocks</strong> to manage their access — the ledger stays
+          where it is and the form comes to you.
         </li>
         <li>
           <strong>Cancel</strong>, Escape, or a click outside closes the popup

web/src/routes/UsersPage.test.tsx

@@ -195,14 +195,14 @@ describe("UsersPage flock scoping", () => {
     });
 
     expect(mockListAssignments).toHaveBeenCalledWith("u-w");
-    expect(await screen.findByText("Assigned flocks")).toBeInTheDocument();
+    // F133: the per-worker panel now opens in the shared dialog, titled with the
+    // worker's email (its accessible name).
+    const panel = await screen.findByRole("dialog", { name: /Flock access — worker@farm.test/ });
     // The assignment's flock id resolves to a name via the loaded flocks list.
     // Scope to the <li> so it doesn't collide with the dropdown's "Coop A" option.
-    const item = screen.getByRole("listitem");
+    const item = within(panel).getByRole("listitem");
     expect(within(item).getByText("Coop A")).toBeInTheDocument();
     expect(within(item).getByRole("button", { name: "remove" })).toBeInTheDocument();
-    // Toggle flips to "hide flocks".
-    expect(within(workerRow).getByRole("button", { name: "hide flocks" })).toBeInTheDocument();
   });
 
   it("shows the empty-assignments hint (account-wide access) and lists only ACTIVE flocks to assign", async () => {
@@ -294,18 +294,81 @@ describe("UsersPage flock scoping", () => {
     expect(await screen.findByText(/No assignments — account-wide access/)).toBeInTheDocument();
   });
 
-  it("collapses the panel again on 'hide flocks'", async () => {
+  it("resets the assign dropdown to the first active flock each time the dialog opens", async () => {
+    mockListAssignments.mockResolvedValue([]);
+    await renderReady(ADMIN);
+
+    const workerRow = screen.getByRole("row", { name: /worker@farm.test/ });
+    // Open, switch the pick off the default (fl1 -> fl2), then close.
+    await act(async () => {
+      fireEvent.click(within(workerRow).getByRole("button", { name: "flocks" }));
+    });
+    await screen.findByRole("dialog", { name: /Flock access/ });
+    fireEvent.change(flockSelect(), { target: { value: "fl2" } });
+    expect(flockSelect()).toHaveValue("fl2");
+    fireEvent.click(screen.getByRole("button", { name: "Done" }));
+
+    // Reopen — the dropdown is back on the first active flock, not the stale fl2,
+    // so a distracted admin can't assign the previous worker's pick by accident.
+    await act(async () => {
+      fireEvent.click(within(workerRow).getByRole("button", { name: "flocks" }));
+    });
+    await screen.findByRole("dialog", { name: /Flock access/ });
+    expect(flockSelect()).toHaveValue("fl1");
+  });
+
+  it("closes the flock dialog on Done", async () => {
     mockListAssignments.mockResolvedValue([ASSIGN_1]);
     await renderReady(ADMIN);
 
-    const workerRow = () => screen.getByRole("row", { name: /worker@farm.test/ });
+    const workerRow = screen.getByRole("row", { name: /worker@farm.test/ });
+    await act(async () => {
+      fireEvent.click(within(workerRow).getByRole("button", { name: "flocks" }));
+    });
+    const panel = await screen.findByRole("dialog", { name: /Flock access/ });
+
+    fireEvent.click(within(panel).getByRole("button", { name: "Done" }));
+    expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
+  });
+
+  it("discards a stale refresh from a worker whose dialog was closed and reopened for another", async () => {
+    const WORKER_2: User = { id: "u-w2", email: "worker2@farm.test", displayName: "Walt", role: "Worker" };
+    mockListUsers.mockResolvedValue([WORKER_USER, WORKER_2, ADMIN_USER]);
+    // Call 1: open A (has Coop A). Call 2: A's post-remove refresh — hung.
+    // Call 3: open B (empty). If the guard fails, A's refresh overwrites B.
+    let resolveStale!: (v: FlockAssignment[]) => void;
+    mockListAssignments
+      .mockResolvedValueOnce([ASSIGN_1])
+      .mockImplementationOnce(() => new Promise<FlockAssignment[]>((r) => { resolveStale = r; }))
+      .mockResolvedValueOnce([]);
+    mockUnassignFlock.mockResolvedValue(undefined);
+
+    renderWithProviders(<UsersPage />, { token: ADMIN });
+    await screen.findByText("worker@farm.test");
+
+    // Open worker A, remove its assignment — the refresh now hangs (busy).
+    const rowA = screen.getByRole("row", { name: /worker@farm\.test/ });
+    await act(async () => {
+      fireEvent.click(within(rowA).getByRole("button", { name: "flocks" }));
+    });
+    const panelA = await screen.findByRole("dialog", { name: /Flock access — worker@farm\.test/ });
+    await act(async () => {
+      fireEvent.click(within(panelA).getByRole("button", { name: "remove" }));
+    });
+
+    // Close A and open worker B while A's refresh is still in flight.
+    fireEvent.click(within(panelA).getByRole("button", { name: "Done" }));
+    const rowB = screen.getByRole("row", { name: /worker2@farm\.test/ });
     await act(async () => {
-      fireEvent.click(within(workerRow()).getByRole("button", { name: "flocks" }));
+      fireEvent.click(within(rowB).getByRole("button", { name: "flocks" }));
     });
-    expect(await screen.findByText("Assigned flocks")).toBeInTheDocument();
+    const panelB = await screen.findByRole("dialog", { name: /Flock access — worker2@farm\.test/ });
+    expect(within(panelB).getByText(/No assignments — account-wide access/)).toBeInTheDocument();
 
-    fireEvent.click(within(workerRow()).getByRole("button", { name: "hide flocks" }));
-    expect(screen.queryByText("Assigned flocks")).not.toBeInTheDocument();
+    // A's refresh finally resolves — it must NOT splice A's list into B's dialog.
+    await act(async () => { resolveStale([ASSIGN_1]); });
+    expect(within(panelB).queryByRole("listitem")).toBeNull();
+    expect(within(panelB).getByText(/No assignments — account-wide access/)).toBeInTheDocument();
   });
 
   it("surfaces an error when an assign fails, keeping the panel open", async () => {
@@ -323,7 +386,7 @@ describe("UsersPage flock scoping", () => {
     });
 
     expect(await screen.findByText(/already assigned|Conflict/)).toBeInTheDocument();
-    expect(screen.getByText("Assigned flocks")).toBeInTheDocument(); // panel stayed open
+    expect(screen.getByRole("dialog")).toBeInTheDocument(); // dialog stayed open
   });
 });
 

web/src/routes/UsersPage.tsx

@@ -30,6 +30,12 @@ export function UsersPage() {
   // #103 flock scoping: expand a worker row to manage assignments.
   const [openUser, setOpenUser] = useState<string | null>(null);
   const [assignments, setAssignments] = useState<FlockAssignment[]>([]);
+  // The user the dialog is CURRENTLY for, tracked synchronously (before state
+  // commits). An assign/remove refresh, or a load, that resolves after the
+  // dialog was closed and reopened for a different worker must not splice the
+  // old worker's list or error into the new one (#154 review) — post-await
+  // writes commit only while this still matches the request's target.
+  const activeUser = useRef<string | null>(null);
   const [flocks, setFlocks] = useState<Flock[]>([]);
   const [assignFlockId, setAssignFlockId] = useState("");
 
@@ -59,47 +65,66 @@ export function UsersPage() {
       .catch((err) => setError(errText(err)));
   }, []);
 
-  async function toggleAssignments(userId: string) {
-    if (openUser === userId) {
-      setOpenUser(null);
-      return;
-    }
+  // F133: flock scoping is a per-worker action, so it opens in the shared dialog
+  // like the other per-row surfaces (#131) — the row button opens it, the dialog
+  // closes it. Load the assignments before opening so the panel is never empty
+  // mid-flight; a load failure surfaces on the page and the dialog stays shut.
+  async function openAssignments(userId: string) {
+    activeUser.current = userId;
     try {
-      setAssignments(await listFlockAssignments(userId));
-      setOpenUser(userId);
+      const list = await listFlockAssignments(userId);
+      if (activeUser.current !== userId) return; // superseded by another open/close
+      setAssignments(list);
+      // Start every worker's dialog on the first active flock. Without this the
+      // dropdown keeps the last worker's pick — open A, choose fl2, close, open
+      // B, and B shows fl2 — so a distracted admin could assign the wrong flock.
+      setAssignFlockId(flocks[0]?.id ?? "");
       setError(null);
+      setOpenUser(userId);
     } catch (err) {
+      if (activeUser.current !== userId) return;
+      activeUser.current = null; // load failed → no dialog; surface on the page
       setError(errText(err));
     }
   }
 
+  function closeAssignments() {
+    activeUser.current = null;
+    setOpenUser(null);
+    setError(null);
+  }
+
   async function onAssign() {
-    if (!openUser || !assignFlockId || busy) return;
+    const target = openUser;
+    if (!target || !assignFlockId || busy) return;
     setBusy(true);
     setError(null);
-    const scope = `assign:${openUser}:${assignFlockId}`;
+    const scope = `assign:${target}:${assignFlockId}`;
     try {
-      await assignFlock(openUser, assignFlockId, keyFor(scope));
-      setAssignments(await listFlockAssignments(openUser));
+      await assignFlock(target, assignFlockId, keyFor(scope));
+      const fresh = await listFlockAssignments(target);
       clearKey(scope);
+      if (activeUser.current === target) setAssignments(fresh);
     } catch (err) {
-      setError(errText(err));
+      if (activeUser.current === target) setError(errText(err));
     } finally {
       setBusy(false);
     }
   }
 
   async function onUnassign(assignmentId: string) {
-    if (!openUser || busy) return;
+    const target = openUser;
+    if (!target || busy) return;
     setBusy(true);
     setError(null);
     const scope = `unassign:${assignmentId}`;
     try {
-      await unassignFlock(openUser, assignmentId, keyFor(scope));
-      setAssignments(await listFlockAssignments(openUser));
+      await unassignFlock(target, assignmentId, keyFor(scope));
+      const fresh = await listFlockAssignments(target);
       clearKey(scope);
+      if (activeUser.current === target) setAssignments(fresh);
     } catch (err) {
-      setError(errText(err));
+      if (activeUser.current === target) setError(errText(err));
     } finally {
       setBusy(false);
     }
@@ -178,8 +203,8 @@ export function UsersPage() {
         </form>
       </Dialog>
 
-      {/* The dialog carries its own copy while it is up. */}
-      {error && !creating && <p className="error">{error}</p>}
+      {/* Each dialog carries its own error copy while it is up. */}
+      {error && !creating && openUser === null && <p className="error">{error}</p>}
       {message && <p className="success">{message}</p>}
 
       <table className="data">
@@ -194,8 +219,8 @@ export function UsersPage() {
               <td>{u.role}</td>
               <td>
                 {u.role === "Worker" && (
-                  <button className="link" onClick={() => void toggleAssignments(u.id)}>
-                    {openUser === u.id ? "hide flocks" : "flocks"}
+                  <button className="link" onClick={() => void openAssignments(u.id)}>
+                    flocks
                   </button>
                 )}
               </td>
@@ -204,37 +229,42 @@ export function UsersPage() {
         </tbody>
       </table>
 
-      {openUser !== null && (
-        <>
-          <h3>Assigned flocks</h3>
-          <p className="muted">
-            No assignments = the worker can record for any flock. The first
-            assignment narrows them to the listed flocks only.
-          </p>
-          {assignments.length === 0 ? (
-            <p className="muted">No assignments — account-wide access.</p>
-          ) : (
-            <ul>
-              {assignments.map((a) => (
-                <li key={a.id}>
-                  {flockName(a.flockId)}{" "}
-                  <button className="link" disabled={busy} onClick={() => void onUnassign(a.id)}>
-                    remove
-                  </button>
-                </li>
-              ))}
-            </ul>
-          )}
-          <div className="inline-form">
-            <select value={assignFlockId} onChange={(e) => setAssignFlockId(e.target.value)}>
-              {flocks.map((f) => <option key={f.id} value={f.id}>{f.name}</option>)}
-            </select>
-            <button disabled={busy || !assignFlockId} onClick={() => void onAssign()}>
-              Assign flock
-            </button>
-          </div>
-        </>
-      )}
+      <Dialog
+        open={openUser !== null}
+        title={`Flock access — ${users.find((u) => u.id === openUser)?.email ?? ""}`}
+        onClose={closeAssignments}
+      >
+        <p className="muted">
+          No assignments = the worker can record for any flock. The first
+          assignment narrows them to the listed flocks only.
+        </p>
+        {assignments.length === 0 ? (
+          <p className="muted">No assignments — account-wide access.</p>
+        ) : (
+          <ul>
+            {assignments.map((a) => (
+              <li key={a.id}>
+                {flockName(a.flockId)}{" "}
+                <button className="link" disabled={busy} onClick={() => void onUnassign(a.id)}>
+                  remove
+                </button>
+              </li>
+            ))}
+          </ul>
+        )}
+        <div className="inline-form">
+          <select value={assignFlockId} onChange={(e) => setAssignFlockId(e.target.value)}>
+            {flocks.map((f) => <option key={f.id} value={f.id}>{f.name}</option>)}
+          </select>
+          <button disabled={busy || !assignFlockId} onClick={() => void onAssign()}>
+            Assign flock
+          </button>
+        </div>
+        {error && <p className="error">{error}</p>}
+        <div className="dialog-foot">
+          <button type="button" className="link" onClick={closeAssignments}>Done</button>
+        </div>
+      </Dialog>
     </section>
   );
 }