Repository navigation
fix(gui): give restore focus somewhere to land when its trigger is gone - #3113
Conversation
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.
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe restore dialog now preserves focus after a successful restore removes its trigger. Cleanup focuses the trigger when it remains mounted and otherwise focuses a connected containing region. Regression tests cover both outcomes. ChangesRestore focus management
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized GUI change improves where keyboard focus lands after a restore dialog closes without changing restore behavior or access boundaries; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The PR addresses issue ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
✅ READY
UI screenshot waived by the Hygiene✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58c616ddd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lidge-jun
added
the
gui-screenshot-waived
|
Applying This PR changes where keyboard focus lands after a dialog closes. The rendered pixels are identical before and after — a screenshot of the fixed build and a screenshot of the broken build would be the same image, which is worse than no screenshot because it implies verification that did not happen. The equivalent evidence is the rendered-DOM assertion, and it is in the tests against a real happy-dom render of the production Mutation-checked: collapsing the cleanup back to Also noting the unrelated CI failure on this head: |
리뷰 · 우선순위 58 / 80설명 이 PR은 이슈 #3059를 지금 다만 포커스가 사라지는 경험은 진짜입니다. 지금 체크아웃의 이 PR은 트리거가 속한 round-2 prio≥70 과는 무관한 GUI 접근성 수정입니다. types.ts/config.ts 분할과도 무관합니다. 파일 2개·+114/−3이라 충돌면이 작습니다. 라인 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Administrator bypass record\n\nOwner authorization was explicit in Codex session 01a05a34-1e3a-73f2-8607-15e517cbec11. Exact head: 9daba00. Current rollup: 0 failing, 0 pending. The focus-race review blocker was repaired with the real close-then-refresh-removal ordering; focused GUI tests passed and the thread is resolved. Using the MAINTAINERS.md maintain/admin bypass because the author cannot self-approve. |
…ne (lidge-jun#3113) * 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 lidge-jun#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 lidge-jun#3059. * fix(gui): retain restore focus through history refresh
…ne (lidge-jun#3113) * 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 lidge-jun#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 lidge-jun#3059. * fix(gui): retain restore focus through history refresh
1 participant
Summary
Closes #3059. The reporter's diagnosis is wrong and their experience is real, which is why this is a fix rather than a close.
What the report describes cannot happen.
onRestored()→refresh()clearsstatus,if (!status)unmounts the page. Butrefresh()keeps cached data:runFetchshows loading only whendata === undefinedorforceLoadingis set (gui/src/client-resource.ts:339-341), andFileIntegrationPage's restore passes neither, souseDataSurfaceclassifies itloading-with-stale-dataand theif (!status)branch at:175is cold-load only. That branch was already cold-load only at the commit the report names.The focus failure underneath it is real, and
RestoreDialogdocumented it against itself:A restore that consumes its snapshot re-renders the row as an
expiredbadge 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.
isConnectedis the load-bearing check; a detached node is exactly what looks fine and does nothing.Screenshot
There is no visual change to screenshot. This alters where keyboard focus lands after a dialog closes; the rendered pixels are identical before and after. The equivalent evidence is the rendered-DOM assertion, which is what the tests capture:
Both are asserted against a real happy-dom render of the production
RestoreDialog, not a mock.Verification
Mutation: collapsing the cleanup back to
trigger?.focus?.()gives 33 pass / 1 fail, exactlyfocus returns to a stable region when the restore removed its trigger. Restored to 34/0.focus returns to the trigger itself when it survivedis the control — the fallback must not preempt a trigger that is still in the document.Checklist
bun x tsc --noEmitcleanbun run lint:guicleanTriaged in the 2026-08-31 non-priority-70 bug round.
Summary by CodeRabbit
Bug Fixes
Tests