Repository navigation
🤖 fix: show the selected sidebar tab whole on a narrow pane under the window-controls inset - #7174
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. |
|
Codex Review: Didn't find any major issues. 👍 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". |
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 30 model calls · 207.0k tokens (72% cached) All 14 tests in 9 files
e2e 0.20.0 · 3m 12s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.0k tokens (21% cached) All 3 tests in 3 files
e2e 0.20.0 · 23.8s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 6.6s · 2 targets (web, phone) · run artifacts |
dbf0cb1 to
c616c12
Compare
|
@codex review |
|
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. |
c616c12 to
6742cdf
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: Didn't find any major issues. Breezy! 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". |
|
Perf re-clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
6742cdf to
f5131b6
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: Didn't find any major issues. 🎉 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". |
f5131b6 to
0e44316
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 |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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. |
0e44316 to
1875c45
Compare
|
Perf re-clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
1875c45 to
efe0eab
Compare
|
Perf re-clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
|
Perf re-clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
…r under a full-screen panel (coder#7170) ## 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 coder#7114 Stack 1 of 4 (bottom). The other layers: coder#7171 (dialog Escape, coder#7141), coder#7173 (tab arrow keys, coder#7102), coder#7174 (selected tab fit, coder#7103). ## Background `useBubbleFocus` (`TutorialTooltip.tsx`) called `bubble.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()`: 1. `isEditableElement(document.activeElement)`: leave focus in a text field. 2. `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 coder#5701 The coder#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 coder#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 - Two new tests in `TutorialTooltip.test.tsx` (composer focus kept, covered bubble). Both fail on `main` and pass here. The covered case stubs `document.elementFromPoint`, because happy-dom computes no layout. - A third new test: after focus moves into the bubble (a click on Skip), closing the bubble returns focus to the composer. It fails without the always-registered cleanup. - The existing focus tests now start from a focused header button instead of the composer, since the composer case changed on purpose. - `make test-bugbash-repros` with `--grep tutorial` (mock AI): the updated coder#5701 repro passes on web and phone. ## Follow-up coder#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 (coder#5701 behavior). --- _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 -->
51c9a01 to
9b857bc
Compare
|
Perf re-clearance (perf owner 8e18f4aada): CLEARED at head
Generated with |
1 participant
Summary
On a narrow right sidebar under the Windows/Linux window-controls inset, the selected tab now shows whole. The selected tab is capped at the tab row's width, so its label truncates and its close X stays in view. Before, the selected tab was cut off at the row's end, and its badge and X were hidden.
Fixes #7103
Stack 4 of 4 (top), on #7173. The coordinator chose this option (cap the tab) over hiding "+" on narrow panes, which alone did not free enough room.
Background
.titlebar-safe-right-cappedleaves 10rem for the strip's content. At the 300 px minimum sidebar width with the 138 px fallback inset, the row gets 73 px. The selected Stats tab with its cost badge is 111 px wide. Raising the cap is not an option: at 300 px it puts strip controls under the window controls.Implementation
max-w-fullon the selected tab's wrapper. The tab keeps its fixedmax-w-[240px]. Only the selected tab gets the cap: a capped terminal tab is mostly its pop-out and close buttons, so a click at the center of an unselected one hit "Open terminal in new window" (the first CI run failedsidebarDragDrop.spec.tsandterminal.spec.tsthat way). I first put a percentage cap on the tab itself. That broke the wide layout: during intrinsic sizing the browser ignores a percentage max-width, so the row was sized for the uncapped tab and kept 71 px of empty space after the last tab (theWideplay caught it). The wrapper's cap resolves against the row, which has a definite width.Validation
TabsReachableUnderWideTitlebarInsetis now one of two stories fromtitlebarInsetStory(insetPx, sidebarWidthPx). It has the restoredexpectSelectedTabVisiblecheck, and the cost badge on Stats. The newSelectedTabFitsUnderTitlebarInsetcovers the 138 px inset at 300 px. Both fail onmain("selected tab 9-123 is outside its strip 9-82") and pass here.Stable tab controlsplays (Narrow, Wide, Cramped, ArrowKeys) andApp.seededWorkspaceA11y/App.titlebarDragpass.300 px sidebar, 138 px inset (red hatch: window-controls area). Before:
After:
375 px window (no inset on phones; the strip is unchanged there):
Risks
Low. The cap only changes a selected tab wider than the row. Wider rows keep the 240 px cap.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$47.52