Skip to content

Commit 58c616d

Browse files
lidge-junlidge-jun
committed
fix(gui): give restore focus somewhere to land when its trigger is gone
The reporter's diagnosis is wrong and their experience is real, which is why this is a fix rather than a close. What #3059 describes -- onRestored() -> refresh() clears status, if (!status) unmounts the page -- cannot happen. refresh() keeps cached data: runFetch only shows loading when data === undefined or forceLoading is set (gui/src/client-resource.ts:339-341), and FileIntegrationPage's restore passes neither, so useDataSurface classifies it loading-with-stale-data and the if (!status) branch at :175 is cold-load only. The focus failure underneath it is real, and RestoreDialog said so itself: // The row's button is gone from the DOM in the collapsed case, so this is // a best effort: focus returns only if the trigger survived the close. A restore that consumes its snapshot re-renders the row as an expired badge with no button (RollbackHistory.tsx:44-46), so the remembered element is detached by cleanup time. Calling .focus() on a detached node succeeds silently and focus stays on <body> -- a keyboard user is dropped at the top of the document with nothing announced. The dialog now also remembers the enclosing region and falls back to it when the trigger did not survive, made programmatically focusable with tabindex=-1 so it never joins the Tab order. isConnected is the load-bearing check. Mutation-checked: collapsing the cleanup back to trigger?.focus?.() fails exactly the new region test (33 pass / 1 fail), restored to 34/0. Closes #3059.
1 parent 6123be3 commit 58c616d

2 files changed

Lines changed: 114 additions & 3 deletions

File tree

‎gui/src/pages/integrations/RestoreDialog.tsx‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ export default function RestoreDialog({
3232
const t = useT();
3333
const dialogRef = useRef<HTMLDialogElement>(null);
3434
const restoreFocusRef = useRef<HTMLElement | null>(null);
35+
const restoreFallbackRef = useRef<HTMLElement | null>(null);
3536
const [drift, setDrift] = useState(false);
3637
const [pending, setPending] = useState(false);
3738
const [failure, setFailure] = useState<string | null>(null);
@@ -43,12 +44,31 @@ export default function RestoreDialog({
4344
// focus-restore uses the same tagName check.
4445
const active = document.activeElement;
4546
restoreFocusRef.current = active?.tagName === "BUTTON" ? active as HTMLElement : null;
47+
// The trigger may not survive the restore. A row whose snapshot is consumed
48+
// re-renders as an `expired` badge with no button (RollbackHistory.tsx:44-46),
49+
// so the element captured above is detached by the time the cleanup runs and
50+
// focus lands on <body> — a keyboard user dropped at the top of the document
51+
// with nothing announced (#3059). Remember the enclosing region too, so there
52+
// is somewhere to return to that cannot disappear.
53+
restoreFallbackRef.current =
54+
(active?.closest?.("section, [role='region'], main") as HTMLElement | null) ?? null;
4655
if (dialog && !dialog.open) dialog.showModal();
4756
return () => {
4857
if (dialog?.open) dialog.close();
49-
// The row's button is gone from the DOM in the collapsed case, so this is
50-
// a best effort: focus returns only if the trigger survived the close.
51-
restoreFocusRef.current?.focus?.();
58+
// Prefer the trigger; fall back to its region when the restore removed it.
59+
// `isConnected` is the check that matters: a detached node accepts .focus()
60+
// silently and focus stays on <body>, which is the reported symptom.
61+
const trigger = restoreFocusRef.current;
62+
if (trigger?.isConnected) {
63+
trigger.focus?.();
64+
return;
65+
}
66+
const fallback = restoreFallbackRef.current;
67+
if (!fallback?.isConnected) return;
68+
// A region is not focusable by default; -1 makes it programmatically
69+
// focusable without adding it to the Tab order.
70+
if (!fallback.hasAttribute("tabindex")) fallback.setAttribute("tabindex", "-1");
71+
fallback.focus?.();
5272
};
5373
}, []);
5474

‎gui/tests/integrations-surfaces.test.tsx‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -673,6 +673,97 @@ test("a drifted restore asks a second time instead of failing", async () => {
673673
expect((posts[1] as { confirmDrift?: boolean }).confirmDrift).toBe(true);
674674
});
675675

676+
/**
677+
* #3059: focus had nowhere to go when the restore consumed the row that owned the
678+
* trigger. A consumed snapshot re-renders as an `expired` badge with no button, so
679+
* the remembered element is detached by the time the dialog closes — and calling
680+
* .focus() on a detached node succeeds silently while focus stays on <body>. A
681+
* keyboard user ends up at the top of the document with nothing announced.
682+
*
683+
* The reporter blamed a page unmount, which the tree does not do: refresh() keeps
684+
* stale data (client-resource.ts:339-341), so `if (!status)` is cold-load only. The
685+
* mechanism is wrong and the experience is real, which is why this is a fix rather
686+
* than a close.
687+
*/
688+
test("focus returns to a stable region when the restore removed its trigger", async () => {
689+
const [{ createRoot }, { LanguageProvider }, { default: RestoreDialog }] = await Promise.all([
690+
import("react-dom/client"),
691+
import("../src/i18n/provider"),
692+
import("../src/pages/integrations/RestoreDialog"),
693+
]);
694+
695+
// The shape RollbackHistory renders: a region holding the row's trigger.
696+
const region = testWindow.document.createElement("section");
697+
const trigger = testWindow.document.createElement("button");
698+
region.appendChild(trigger);
699+
testWindow.document.body.appendChild(region);
700+
trigger.focus();
701+
expect(testWindow.document.activeElement).toBe(trigger);
702+
703+
const row = {
704+
opId: "op-consumed",
705+
clientId: "hermes" as const,
706+
kind: "apply" as const,
707+
at: "2026-08-02T09:00:00.000Z",
708+
configPath: "/tmp/home/.hermes/config.yaml",
709+
snapshot: "stored" as const,
710+
undoable: false,
711+
};
712+
await act(async () => {
713+
root = createRoot(container);
714+
root.render(
715+
<LanguageProvider>
716+
<RestoreDialog apiBase={apiBase} row={row} onClose={() => {}} onRestored={() => {}} />
717+
</LanguageProvider>,
718+
);
719+
});
720+
721+
// The restore consumes the snapshot, so the row re-renders without its button.
722+
trigger.remove();
723+
await act(async () => { root!.unmount(); root = null; });
724+
725+
expect(testWindow.document.activeElement).toBe(region);
726+
expect(testWindow.document.activeElement).not.toBe(testWindow.document.body);
727+
region.remove();
728+
});
729+
730+
test("focus returns to the trigger itself when it survived", async () => {
731+
const [{ createRoot }, { LanguageProvider }, { default: RestoreDialog }] = await Promise.all([
732+
import("react-dom/client"),
733+
import("../src/i18n/provider"),
734+
import("../src/pages/integrations/RestoreDialog"),
735+
]);
736+
737+
const region = testWindow.document.createElement("section");
738+
const trigger = testWindow.document.createElement("button");
739+
region.appendChild(trigger);
740+
testWindow.document.body.appendChild(region);
741+
trigger.focus();
742+
743+
const row = {
744+
opId: "op-kept",
745+
clientId: "hermes" as const,
746+
kind: "apply" as const,
747+
at: "2026-08-02T09:00:00.000Z",
748+
configPath: "/tmp/home/.hermes/config.yaml",
749+
snapshot: "stored" as const,
750+
undoable: true,
751+
};
752+
await act(async () => {
753+
root = createRoot(container);
754+
root.render(
755+
<LanguageProvider>
756+
<RestoreDialog apiBase={apiBase} row={row} onClose={() => {}} onRestored={() => {}} />
757+
</LanguageProvider>,
758+
);
759+
});
760+
await act(async () => { root!.unmount(); root = null; });
761+
762+
// The fallback must not preempt a trigger that is still there.
763+
expect(testWindow.document.activeElement).toBe(trigger);
764+
region.remove();
765+
});
766+
676767
test("a card toggles its own client without a trip to the sub-page", async () => {
677768
// Same rule as the client page: off means disable, for `stale` too.
678769
stateResponse = () => json({ clients: [status({ state: "stale" })] });

0 commit comments

Comments
 (0)