Repository navigation
🤖 fix: keep the tutorial bubble from taking focus from a text field or under a full-screen panel - #7170
Conversation
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. |
ThomasK33
changed the title
|
@codex review |
|
Perf clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 29893ba68c
ℹ️ 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".
🟢 e2e stories (Haiku): 8 passed21 agent steps · 12 replayed from cache · 9 model calls · 35.2k tokens (22% cached) All 8 tests in 8 files
e2e 0.20.0 · 1m 7s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.1k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 23.7s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 6.4s · 2 targets (web, phone) · run artifacts |
29893ba to
fd86217
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
fd86217 to
496f7ff
Compare
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Perf re-clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
496f7ff to
759f141
Compare
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
759f141 to
1308faa
Compare
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1308faa32e
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Perf re-clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
…r under a full-screen panel (#7114)
1308faa to
3e4714a
Compare
|
Perf re-clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…r#7171) ## Summary An Escape pressed in the first moments after a dialog opens now closes it. Before, Radix dropped that key: the dialog stayed open and kept focus. The fix is in our shared `DialogContent`, so it covers every modal built on it. Fixes coder#7141 Stack 2 of 4, on coder#7170. ## Background `@radix-ui/react-dismissable-layer` 1.1.11 computes its layer index during render. It registers the layer in an effect and then asks for a re-render. Until that re-render runs, its capture-phase Escape handler sees `index = -1` and returns without doing anything. Focus is already inside the dialog at that point (FocusScope moves it in a mount effect). I checked upstream first. The latest `@radix-ui/react-dismissable-layer` (1.2.1) has the same code path: the listener still reads the index from the stale render, and the changelog does not mention this. An upgrade does not fix it. ## Implementation 1. `Dialog` is now a thin wrapper around `DialogPrimitive.Root`. It holds an uncontrolled root's open state itself (Radix keeps that state private), passes `open` and `onOpenChange` to Radix, and puts its setter into a context. So the fallback can close every dialog: controlled, `defaultOpen`, or opened by `DialogTrigger` (Codex review). 2. `DialogContent` gets an `onKeyDown` fallback. An Escape that Radix handled never arrives unprevented: our `onEscapeKeyDown` stops it, or Radix prevents its default when it dismisses. So an unprevented Escape whose target is inside this content node was dropped. The fallback runs the same Escape logic (`allowEditableEscape`, the caller's `onEscapeKeyDown`) and then calls `onOpenChange(false)`. 3. A key from a portaled child layer (a popover inside the dialog) bubbles to `DialogContent` through React, but its DOM target is outside the content node. The fallback ignores it, so that layer keeps the key. ## Validation - New plays `Components/Dialog/EscapeRightAfterOpen` (controlled) and `EscapeRightAfterOpenUncontrolled` (`DialogTrigger`, no `open` or `onOpenChange`). It dispatches Escape from the `focusin` event that marks focus entering the dialog. That is the exact window: the layer is registered, and its index is not updated yet. A timer was not reliable (in a page probe, an Escape after `setTimeout(0)` was dropped in 1 of 3 runs). Both fail on `main` ("the dialog is still open after Escape"). The controlled one passed 4 of 4 runs here. The uncontrolled one also failed with only the first version of this fix, which closed through the caller's `onOpenChange`. - In a page probe on `main`, an Escape sent from `focusin` was dropped in 6 of 6 runs. - React Compiler audit of `Dialog.tsx` (babel-plugin-react-compiler with `reactCompilerConfig`): ok=14 err=0. On `main` it is ok=13 err=0. The extra component is the new `Dialog` wrapper. ## Risks Medium-low: every dialog renders through this file. The fallback acts only on an Escape that nothing else handled, from inside the content node. The Radix path does not change. Uncontrolled dialogs now keep their open state in the wrapper instead of in Radix, with the same `defaultOpen` start value. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$47.52`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=47.52 -->
1 participant
Summary
The tutorial bubble no longer takes keyboard focus in two cases: when the user's focus is already in a text field (the composer), and when a full-screen layer such as the Artifacts or App View panel covers the bubble. In both cases Escape went to the bubble and skipped a tutorial the user never chose to act on. The bubble stays clickable.
Fixes #7114
Stack 1 of 4 (bottom). The other layers: #7171 (dialog Escape, #7141), #7173 (tab arrow keys, #7102), #7174 (selected tab fit, #7103).
Background
useBubbleFocus(TutorialTooltip.tsx) calledbubble.focus()as soon as the bubble was positioned. It checked neither the element that had focus nor what was drawn over the bubble. The full-screen panels (z-1500) sit above the tutorial and are not modal, so nothing trapped focus away from the bubble. The related finding in the issue comments: the creation tutorial took focus from a composer the user had already focused.Implementation
One guard before
bubble.focus():isEditableElement(document.activeElement): leave focus in a text field.isBubbleCovered(bubble): a hit test at the bubble's center. If the top element there is not inside the bubble, something covers it. A hit test needs no list of overlay layers. An unknown result (no layout, center off screen) counts as uncovered, so the old behavior holds there.The restore cleanup is registered in every case. A click on Skip or Next moves focus from the composer into the bubble, and focus must go back to the composer when the bubble closes (Codex review).
Contract change for #5701
The #5701 repro (
tests/bugbash/repros/tutorialEscapeOverSettings.e2e.ts) said the bubble "owns focus while nothing covers it". The app opens on the creation form with the composer focused, so with this PR the bubble no longer takes focus there. The new contract: the bubble takes focus only while nothing covers it and no text field has it. Escape on the bubble still closes it. Reason: the #7114 comment shows the bubble taking focus from a composer the user had focused, and Escape then skipped the tutorial. The coordinator chose this over keeping the old contract.The repro now clicks the bubble's title first, as a keyboard user would Tab to it. Its other steps, assertions and tags do not change.
Validation
TutorialTooltip.test.tsx(composer focus kept, covered bubble). Both fail onmainand pass here. The covered case stubsdocument.elementFromPoint, because happy-dom computes no layout.make test-bugbash-reproswith--grep tutorial(mock AI): the updated 🤖 Escape does not close the tutorial tooltip #5701 repro passes on web and phone.Follow-up
#7198 tracks two Codex findings deferred from review round 5: hand focus to the bubble when a covering panel closes, and make the bubble the next Tab destination from a focused composer.
Risks
Low. Only the first focus move of a bubble changes. Keyboard users who start from a button still get focus in the bubble (#5701 behavior).
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$47.52