Skip to content

Commit a5c323a

Browse files
committed
fix: report the document reference map from slot state
onChange was emitted at the end of each upload, which had two problems: - it read `slots` from a closure captured before the await, so a slot finishing could emit a map missing another slot's reference - it never fired when a pending scan later resolved, so the parent stayed on "scan_pending" after the scan had finished The map is now derived from slot state and reported via an effect keyed on its content, so progress ticks don't emit and scan transitions do. Also guards the deliberately un-awaited scan poll against an unhandled rejection.
1 parent 58ffbc2 commit a5c323a

3 files changed

Lines changed: 166 additions & 32 deletions

File tree

__tests__/documents/DocumentUpload.test.jsx

Lines changed: 119 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -423,9 +423,12 @@ describe("replace and remove", () => {
423423
`/api/educators/applications/documents/${SIGNED_TARGET.documentId}`
424424
);
425425

426-
// The referenced object now points at the new document.
427-
const latest = onChange.mock.calls.at(-1)[0];
428-
expect(latest[ID].documentId).toBe("doc_second");
426+
// The referenced object now points at the new document. Waited for rather
427+
// than read once: the filename renders as soon as validation starts, which
428+
// is before the replacement upload has finalised.
429+
await waitFor(() =>
430+
expect(onChange.mock.calls.at(-1)[0][ID].documentId).toBe("doc_second")
431+
);
429432
});
430433

431434
it("remove deletes the document and clears the slot", async () => {
@@ -462,6 +465,109 @@ describe("replace and remove", () => {
462465

463466
// ---------------------------------------------------------------------------
464467

468+
describe("onChange reports the full reference map", () => {
469+
const CERT = DOCUMENT_TYPES.TEACHING_CERTIFICATE;
470+
471+
const TWO_SLOTS = [
472+
{ ...SINGLE_SLOT[0] },
473+
{
474+
type: CERT,
475+
label: "Teaching certificate",
476+
description: "Ijazah or teaching licence.",
477+
required: true,
478+
allowCamera: false,
479+
},
480+
];
481+
482+
/** Give each slot its own documentId so the map can be told apart. */
483+
function mockPerSlot() {
484+
axiosInstance.post.mockImplementation((url, body) => {
485+
if (url.endsWith("/upload-url")) {
486+
return Promise.resolve({
487+
data: { ...SIGNED_TARGET, documentId: `doc_${body.documentType}` },
488+
});
489+
}
490+
if (url.endsWith("/complete")) {
491+
const documentId = url.split("/documents/")[1].split("/complete")[0];
492+
return Promise.resolve({
493+
data: {
494+
documentId,
495+
documentType: body.documentType,
496+
status: "accepted",
497+
},
498+
});
499+
}
500+
return Promise.resolve({ data: {} });
501+
});
502+
axios.request.mockResolvedValue({ data: {}, status: 200 });
503+
}
504+
505+
function dropInto(type, name) {
506+
fireEvent.drop(screen.getByTestId(`dropzone-${type}`), {
507+
dataTransfer: {
508+
files: [makeFile(PDF_MAGIC, { name, type: "application/pdf" })],
509+
},
510+
});
511+
}
512+
513+
it("keeps earlier slots when a later slot finishes", async () => {
514+
mockPerSlot();
515+
const onChange = vi.fn();
516+
517+
render(
518+
<DocumentUpload
519+
slots={TWO_SLOTS}
520+
uploadOptions={{ pollIntervalMs: 5, maxPollAttempts: 5 }}
521+
onChange={onChange}
522+
/>
523+
);
524+
525+
dropInto(ID, "passport.pdf");
526+
await waitFor(() =>
527+
expect(screen.getByTestId(`document-slot-${ID}`)).toHaveAttribute(
528+
"data-state",
529+
"accepted"
530+
)
531+
);
532+
533+
dropInto(CERT, "ijazah.pdf");
534+
await waitFor(() =>
535+
expect(screen.getByTestId(`document-slot-${CERT}`)).toHaveAttribute(
536+
"data-state",
537+
"accepted"
538+
)
539+
);
540+
541+
// The second slot completing must not drop the first slot's reference.
542+
const latest = onChange.mock.calls.at(-1)[0];
543+
expect(Object.keys(latest).sort()).toEqual([CERT, ID].sort());
544+
expect(latest[ID].documentId).toBe(`doc_${ID}`);
545+
expect(latest[CERT].documentId).toBe(`doc_${CERT}`);
546+
});
547+
548+
it("reports the status change when a pending scan resolves", async () => {
549+
mockHappyPath({ finalStatus: "scan_pending", pollStatus: "accepted" });
550+
const onChange = vi.fn();
551+
renderUpload({ onChange });
552+
553+
dropInto(ID, "passport.pdf");
554+
555+
await waitFor(
556+
() =>
557+
expect(screen.getByTestId("slot-status")).toHaveAttribute(
558+
"data-state",
559+
"accepted"
560+
),
561+
{ timeout: 3000 }
562+
);
563+
564+
// The parent must see the resolved status, not the stale scan_pending one.
565+
expect(onChange.mock.calls.at(-1)[0][ID].status).toBe("accepted");
566+
});
567+
});
568+
569+
// ---------------------------------------------------------------------------
570+
465571
describe("no public asset URL is ever produced or stored", () => {
466572
it("never surfaces a cloudinary or other public URL, and stores only an id", async () => {
467573
mockHappyPath({ finalStatus: "accepted" });
@@ -483,13 +589,17 @@ describe("no public asset URL is ever produced or stored", () => {
483589
);
484590

485591
// The stored reference carries an opaque id and a status — no URL field.
486-
const reference = onChange.mock.calls.at(-1)[0][ID];
487-
expect(reference).toEqual(
488-
expect.objectContaining({
489-
documentId: "doc_abc123",
490-
status: "accepted",
491-
})
592+
// Waited for: the reference map is reported from an effect, which flushes
593+
// a tick after the slot's DOM state settles.
594+
await waitFor(() =>
595+
expect(onChange.mock.calls.at(-1)?.[0]?.[ID]).toEqual(
596+
expect.objectContaining({
597+
documentId: "doc_abc123",
598+
status: "accepted",
599+
})
600+
)
492601
);
602+
const reference = onChange.mock.calls.at(-1)[0][ID];
493603
expect(JSON.stringify(reference)).not.toMatch(/https?:\/\//);
494604
expect(reference.secure_url).toBeUndefined();
495605
expect(reference.url).toBeUndefined();

components/organisms/educator-onboarding/DocumentUpload.jsx

Lines changed: 43 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,14 @@
1818
* - No component in this tree ever receives or renders a public asset URL.
1919
*/
2020

21-
import { useCallback, useId, useRef, useState } from "react";
21+
import {
22+
useCallback,
23+
useEffect,
24+
useId,
25+
useMemo,
26+
useRef,
27+
useState,
28+
} from "react";
2229
import {
2330
AlertCircle,
2431
Camera,
@@ -373,35 +380,49 @@ export default function DocumentUpload({
373380
useDocumentUpload(uploadOptions);
374381

375382
const handleSelectFile = useCallback(
376-
async (documentType, file) => {
383+
(documentType, file) => {
377384
const existing = slots[documentType]?.reference?.documentId;
378385
const run = existing ? replaceDocument : uploadDocument;
379-
380-
const result = await run(documentType, file);
381-
382-
if (result?.ok && onChange) {
383-
onChange({
384-
...collectReferences(slots),
385-
[documentType]: result.reference,
386-
});
387-
}
388-
return result;
386+
return run(documentType, file);
389387
},
390-
[onChange, replaceDocument, slots, uploadDocument]
388+
[replaceDocument, slots, uploadDocument]
391389
);
392390

393391
const handleRemove = useCallback(
394-
async (documentType) => {
395-
const result = await removeDocument(documentType);
396-
if (result?.ok && onChange) {
397-
const next = collectReferences(slots);
398-
delete next[documentType];
399-
onChange(next);
400-
}
401-
},
402-
[onChange, removeDocument, slots]
392+
(documentType) => removeDocument(documentType),
393+
[removeDocument]
403394
);
404395

396+
// ── Report the reference map upward ──────────────────────────────────────
397+
// Derived from slot state rather than emitted at the end of each upload.
398+
// Emitting per-upload read `slots` from a closure captured before the await,
399+
// and never fired at all when a pending scan later resolved — so the parent
400+
// could hold a reference map that was missing a slot, or stuck on
401+
// "scan_pending" after the scan had finished.
402+
const references = useMemo(() => collectReferences(slots), [slots]);
403+
404+
// Compare by content: slot state also changes on every progress tick, and
405+
// those must not be reported as reference changes.
406+
const referencesKey = JSON.stringify(references);
407+
408+
// Held in a ref so a parent passing an inline arrow doesn't re-fire this.
409+
const onChangeRef = useRef(onChange);
410+
useEffect(() => {
411+
onChangeRef.current = onChange;
412+
}, [onChange]);
413+
414+
const hasReportedRef = useRef(false);
415+
useEffect(() => {
416+
// Skip the initial empty map so mounting doesn't look like a change.
417+
if (!hasReportedRef.current) {
418+
hasReportedRef.current = true;
419+
if (referencesKey === "{}") return;
420+
}
421+
onChangeRef.current?.(references);
422+
// `references` is recreated per render; `referencesKey` is the real trigger.
423+
// eslint-disable-next-line react-hooks/exhaustive-deps
424+
}, [referencesKey]);
425+
405426
const requiredTypes = definitions
406427
.filter((d) => d.required)
407428
.map((d) => d.type);

hooks/useDocumentUpload.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,10 @@ export function useDocumentUpload(options = {}) {
266266
});
267267

268268
if (!isTerminalStatus(reference.status)) {
269-
pollScanStatus(documentType, reference.documentId);
269+
// Deliberately not awaited — the upload is done and the caller
270+
// shouldn't block on the scan. Errors are swallowed inside the poll
271+
// loop; the catch here only guards against an unhandled rejection.
272+
pollScanStatus(documentType, reference.documentId).catch(() => {});
270273
}
271274

272275
return { ok: true, reference };

0 commit comments

Comments
 (0)