From 3ab04a236d024b0a030ea5af71cf758b3e57c497 Mon Sep 17 00:00:00 2001 From: DongHyeonka Date: Tue, 18 Aug 2026 07:53:17 +0900 Subject: [PATCH] fix: restore focus to a stable anchor, not a removed row, after asset delete Fix round 1 for the Task 11 asset library review. I1 (Important): a successful delete removed the row from state, so remove()'s reuse of closeDetail() queued .focus() on a now-detached button -- a silent no-op that stranded focus on . Splits closeDetail() (cancel/close, row still on screen, restores focus to the trigger) from a new closeDetailAfterRemoval() (post-delete, focuses the page heading, the one anchor guaranteed to survive any list change) so the two paths stop sharing a helper that only one of them can safely use. Also closes four Minors from the same review round: - route-contract.test.ts's "27-route inventory" test title corrected to 28. - canHardDelete's usageCount clause gets its own isolating assertion (every prior case used usageCount: 0, so that clause was never independently falsified). - Dropped a duplicate role="status" announcement on a listAssets load failure; the existing role="alert" paragraph is now the sole announcement. - Added success-path tests for archive() and remove() via a new recordingGateway() test helper that pins the exact gateway call shape (expectedVersion, managementStatus: ARCHIVED, idempotency keys), not just the resulting UI text; the delete-success test also pins the I1 focus fix so a regression back to the removed trigger fails loudly. See task-11-report.md's "Fix round 1" section for the RED/GREEN evidence (the pre-fix code reliably crashes the test worker rather than failing the assertion cleanly -- explained there) and the correction to this task's original claim about matching publication-list.tsx's focus pattern. Co-Authored-By: Claude Opus 5 (1M context) --- .../studio/components/asset-library.tsx | 25 +++- .../features/tech-log/asset-library.test.tsx | 124 ++++++++++++++++++ .../features/tech-log/route-contract.test.ts | 2 +- 3 files changed, 147 insertions(+), 4 deletions(-) diff --git a/src/features/tech-log/presentation/studio/components/asset-library.tsx b/src/features/tech-log/presentation/studio/components/asset-library.tsx index 16e3ab5..a795c8e 100644 --- a/src/features/tech-log/presentation/studio/components/asset-library.tsx +++ b/src/features/tech-log/presentation/studio/components/asset-library.tsx @@ -31,6 +31,7 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) { const [notice, setNotice] = useState(""); const triggerRef = useRef(null); const detailHeadingRef = useRef(null); + const pageHeadingRef = useRef(null); const openAssetId = useRef(null); useEffect(() => { @@ -44,8 +45,10 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) { }) .catch((error: unknown) => { if (isAbortError(error)) return; + // The `role="alert"` paragraph below is the sole announcement for a + // load failure -- also routing it through `notice`'s `role="status"` + // paragraph would announce the same sentence twice. setListStatus("ERROR"); - setNotice("Asset 목록을 불러오지 못했습니다."); }); return () => controller.abort(); }, [props.gateway]); @@ -71,11 +74,27 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) { } } + // Dismissing the panel without changing the list (cancel/close): the row + // that opened it is still on screen, so restore focus there. function closeDetail() { setSelected(null); queueMicrotask(() => triggerRef.current?.focus()); } + // Fix round 1 (I1). A successful delete removes the row from `assets`, so + // by the time this runs `triggerRef.current` -- still set, since React + // never nulls a plain ref -- points at a button that is no longer attached + // to the document. `.focus()` on a detached element is a silent no-op, so + // reusing `closeDetail` here left focus stranded on ``. The page + // heading is the one thing in this screen guaranteed to survive any list + // change (the list itself can disappear into the empty state), so a + // deletion moves focus there instead of trying to reuse a node it just + // destroyed. + function closeDetailAfterRemoval() { + setSelected(null); + queueMicrotask(() => pageHeadingRef.current?.focus()); + } + async function archive(detail: AssetDetail) { setPending(true); try { @@ -106,7 +125,7 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) { }); setAssets((current) => current.filter((item) => item.id !== detail.asset.id)); setNotice("삭제했습니다."); - closeDetail(); + closeDetailAfterRemoval(); } catch (error) { setNotice( isStudioGatewayError(error) ? error.problem.detail : "삭제하지 못했습니다.", @@ -120,7 +139,7 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) {

ASSET LIBRARY

-

Asset

+

Asset

업로드한 Asset을 검색하고 사용처를 확인하며, 사용하지 않는 Asset을 정리합니다. diff --git a/tests/features/tech-log/asset-library.test.tsx b/tests/features/tech-log/asset-library.test.tsx index be56a80..2910981 100644 --- a/tests/features/tech-log/asset-library.test.tsx +++ b/tests/features/tech-log/asset-library.test.tsx @@ -30,6 +30,14 @@ const ASSET = { updatedAt: "2026-08-14T01:00:00.000Z", } as never; +// `ASSET` is typed `never` (see above) so it can stand in for any generated +// contract shape without fighting the type checker across this file's other +// fixtures. That means its fields can't be dereferenced directly; these two +// mirror the literals in `ASSET` above for the fix-round-1 tests that need +// to assert on them. +const ASSET_ID = "11111111-1111-4111-8111-111111111111"; +const ASSET_VERSION = 1; + function gatewayOf(detail: unknown, onDelete?: () => never) { return { async listAssets() { @@ -50,6 +58,37 @@ function gatewayOf(detail: unknown, onDelete?: () => never) { } as never; } +/** + * Fix round 1 (Minor 4). Unlike `gatewayOf`, this records exactly what + * `archive()`/`remove()` send the gateway, so a test can pin the command + * shape (`expectedVersion`, `managementStatus`) and the idempotency-key + * plumbing, not just the resulting UI text. + */ +function recordingGateway(detail: unknown) { + const archiveCalls: Array<{ assetId: string; command: unknown; options: unknown }> = []; + const deleteCalls: Array<{ assetId: string; options: unknown }> = []; + const archivedAsset = { ...(ASSET as object), managementStatus: "ARCHIVED" } as never; + const gateway = { + async listAssets() { + return { items: [ASSET], nextCursor: null } as never; + }, + async getAsset() { + return detail as never; + }, + async uploadAsset() { + throw new Error("not used"); + }, + async updateAssetMetadata(assetId: string, command: unknown, options: unknown) { + archiveCalls.push({ assetId, command, options }); + return archivedAsset; + }, + async deleteAsset(assetId: string, options: unknown) { + deleteCalls.push({ assetId, options }); + }, + } as never; + return { gateway, archiveCalls, deleteCalls }; +} + test("offers hard delete only for an unused asset with no publication history", () => { assert.equal( canHardDelete({ asset: ASSET, usages: [], hasPublicationHistory: false } as never), @@ -67,6 +106,19 @@ test("offers hard delete only for an unused asset with no publication history", } as never), false, ); + // Fix round 1 (Minor 2). Every assertion above uses `usageCount: 0`, so + // the `asset.usageCount === 0` clause was never independently exercised -- + // dropping or inverting it would still pass. `usages: []` and + // `hasPublicationHistory: false` isolate it: only the usage-count clause + // can make this `false`. + assert.equal( + canHardDelete({ + asset: { ...(ASSET as object), usageCount: 1 }, + usages: [], + hasPublicationHistory: false, + } as never), + false, + ); }); test("shows archive instead of delete for an asset in use", async () => { @@ -103,3 +155,75 @@ test("surfaces ASSET_IN_USE when the server rejects a delete", async () => { assert.ok(await screen.findByText("사용 중인 Asset은 삭제할 수 없습니다.")); }); + +// Fix round 1 (Minor 4). The archive success path -- the exact command sent +// to the gateway, the merged list state, and the success notice -- was +// previously verified only by reading the source. +test("archives an asset by sending the expected command and merging the server's response into the list", async () => { + const user = userEvent.setup(); + const { gateway, archiveCalls } = recordingGateway({ + asset: { ...(ASSET as object), usageCount: 1 }, + usages: [{ documentId: "d", documentKind: "CASE", title: "사용 중 문서", published: true }], + hasPublicationHistory: true, + }); + render(); + + const row = (await screen.findByRole("button", { name: "boundary" })).closest("li"); + assert.ok(row); + await user.click(await screen.findByRole("button", { name: "boundary" })); + await user.click(await screen.findByRole("button", { name: "보관" })); + + assert.ok(await screen.findByText("보관했습니다.")); + assert.equal(archiveCalls.length, 1); + assert.equal(archiveCalls[0].assetId, ASSET_ID); + assert.deepEqual(archiveCalls[0].command, { + expectedVersion: ASSET_VERSION, + managementStatus: "ARCHIVED", + }); + const options = archiveCalls[0].options as { idempotencyKey?: unknown }; + assert.equal(typeof options.idempotencyKey, "string"); + assert.ok((options.idempotencyKey as string).length > 0); + // The row reflects the server's returned asset (now ARCHIVED), not a + // client-guessed status -- proving the list state actually merged the + // response instead of just leaving the old row in place. `findByText` + // above already settled on the post-update render, so this can assert + // directly instead of polling. + assert.ok(row.textContent?.includes("ARCHIVED")); +}); + +// Fix round 1 (I1 + Minor 4). Combines the delete success path's gateway +// call shape with the focus-restoration regression this round fixed: the +// row that opened the panel is removed from the DOM by a successful delete, +// so restoring focus to it (the pre-fix behaviour) is a silent no-op that +// strands focus on . This test pins the fix -- focus must land on the +// page heading, the one anchor guaranteed to still exist -- so a future +// regression back to the removed trigger fails loudly here. +test("removes the asset and restores focus to the page heading, not the detached row button, after a successful delete", async () => { + const user = userEvent.setup(); + const { gateway, deleteCalls } = recordingGateway({ + asset: ASSET, + usages: [], + hasPublicationHistory: false, + }); + render(); + + await user.click(await screen.findByRole("button", { name: "boundary" })); + await user.click(await screen.findByRole("button", { name: "삭제" })); + + assert.ok(await screen.findByText("삭제했습니다.")); + assert.equal(deleteCalls.length, 1); + assert.equal(deleteCalls[0].assetId, ASSET_ID); + const options = deleteCalls[0].options as { idempotencyKey?: unknown }; + assert.equal(typeof options.idempotencyKey, "string"); + assert.ok((options.idempotencyKey as string).length > 0); + assert.equal(screen.queryByRole("button", { name: "boundary" }), null); + + // `findByText` above already settled on the post-delete render -- the + // `queueMicrotask`-scheduled focus call has necessarily run by then, since + // `findByText` only resolves after yielding through at least one macrotask + // (its polling uses `setTimeout`/`MutationObserver`), and microtasks always + // drain before the next macrotask runs. So this asserts directly rather + // than polling. + const heading = screen.getByRole("heading", { name: "Asset", level: 1 }); + assert.equal(document.activeElement, heading); +}); diff --git a/tests/features/tech-log/route-contract.test.ts b/tests/features/tech-log/route-contract.test.ts index 4ecb5c0..97c7539 100644 --- a/tests/features/tech-log/route-contract.test.ts +++ b/tests/features/tech-log/route-contract.test.ts @@ -74,7 +74,7 @@ const expectedTitles = { } as const; describe("TechLog route boundary contract", () => { - it("freezes the standalone 27-route inventory before runtime installation", () => { + it("freezes the standalone 28-route inventory before runtime installation", () => { expect( Object.values(TECH_LOG_ROUTE_REGISTRY).map((definition) => [ definition.routeId,