donghyeon.kang/tech-log-frontend
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

fix: restore focus to a stable anchor, not a removed row, after asset delete

Browse Source 
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 <body>. 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) <noreply@anthropic.com>
This commit is contained in:
DongHyeonka
2026-08-18 07:53:17 +09:00
co-authored by Claude Opus 5
parent b11aa94f1c
commit 3ab04a236d
3 changed files with 147 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/features/tech-log/presentation/studio/components/asset-library.tsx
+22 -3
View File
@@ -31,6 +31,7 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) {
const [notice, setNotice] = useState(""); const [notice, setNotice] = useState("");
const triggerRef = useRef<HTMLButtonElement | null>(null); const triggerRef = useRef<HTMLButtonElement | null>(null);
const detailHeadingRef = useRef<HTMLHeadingElement | null>(null); const detailHeadingRef = useRef<HTMLHeadingElement | null>(null);
const pageHeadingRef = useRef<HTMLHeadingElement | null>(null);
const openAssetId = useRef<string | null>(null); const openAssetId = useRef<string | null>(null);
useEffect(() => { useEffect(() => {
@@ -44,8 +45,10 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) {
}) })
.catch((error: unknown) => { .catch((error: unknown) => {
if (isAbortError(error)) return; 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"); setListStatus("ERROR");
setNotice("Asset 목록을 불러오지 못했습니다.");
}); });
return () => controller.abort(); return () => controller.abort();
}, [props.gateway]); }, [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() { function closeDetail() {
setSelected(null); setSelected(null);
queueMicrotask(() => triggerRef.current?.focus()); 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 `<body>`. 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) { async function archive(detail: AssetDetail) {
setPending(true); setPending(true);
try { try {
@@ -106,7 +125,7 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) {
}); });
setAssets((current) => current.filter((item) => item.id !== detail.asset.id)); setAssets((current) => current.filter((item) => item.id !== detail.asset.id));
setNotice("삭제했습니다."); setNotice("삭제했습니다.");
closeDetail(); closeDetailAfterRemoval();
} catch (error) { } catch (error) {
setNotice( setNotice(
isStudioGatewayError(error) ? error.problem.detail : "삭제하지 못했습니다.", isStudioGatewayError(error) ? error.problem.detail : "삭제하지 못했습니다.",
@@ -120,7 +139,7 @@ export function AssetLibrary(props: Readonly<{ gateway: StudioAssetGateway }>) {
<div className="studio-page studio-assets-page"> <div className="studio-page studio-assets-page">
<header className="studio-page-heading"> <header className="studio-page-heading">
<p className="studio-eyebrow">ASSET LIBRARY</p> <p className="studio-eyebrow">ASSET LIBRARY</p>
<h1>Asset</h1> <h1 ref={pageHeadingRef} tabIndex={-1}>Asset</h1>
<p> <p>
업로드한 Asset을 검색하고 사용처를 확인하며, 사용하지 않는 Asset을 업로드한 Asset을 검색하고 사용처를 확인하며, 사용하지 않는 Asset을
정리합니다. 정리합니다.
tests/features/tech-log/asset-library.test.tsx
+124
View File
@@ -30,6 +30,14 @@ const ASSET = {
updatedAt: "2026-08-14T01:00:00.000Z", updatedAt: "2026-08-14T01:00:00.000Z",
} as never; } 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) { function gatewayOf(detail: unknown, onDelete?: () => never) {
return { return {
async listAssets() { async listAssets() {
@@ -50,6 +58,37 @@ function gatewayOf(detail: unknown, onDelete?: () => never) {
} as 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", () => { test("offers hard delete only for an unused asset with no publication history", () => {
assert.equal( assert.equal(
canHardDelete({ asset: ASSET, usages: [], hasPublicationHistory: false } as never), 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), } as never),
false, 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 () => { 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은 삭제할 수 없습니다.")); 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(<AssetLibrary gateway={gateway} />);
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 <body>. 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(<AssetLibrary gateway={gateway} />);
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);
});
tests/features/tech-log/route-contract.test.ts
+1 -1
View File
@@ -74,7 +74,7 @@ const expectedTitles = {
} as const; } as const;
describe("TechLog route boundary contract", () => { 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( expect(
Object.values(TECH_LOG_ROUTE_REGISTRY).map((definition) => [ Object.values(TECH_LOG_ROUTE_REGISTRY).map((definition) => [
definition.routeId, definition.routeId,