Repository navigation
fix: hand focus back when overlays close, and raise the Windows stack - #84
Conversation
Debug builds on Windows overflowed the main thread's default 1 MB stack about a second after a repository loaded. Link Windows MSVC binaries with /STACK:8388608, the figure Zed uses for the same reason. Only address space is reserved, so release builds pay nothing. The target is read from CARGO_CFG_TARGET_OS and CARGO_CFG_TARGET_ENV because build scripts run on the build host, where cfg! would describe the host. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every overlay and dialog focuses its own input when it opens and only hides itself when it closes. A hidden overlay draws nothing, so the focused input is missing from the frame and gpui dispatches keystrokes from the window root, above the workspace's handlers: every shortcut did nothing until a panel was clicked. Only the repo opener, clone dialog and global search restored focus, and only when dismissed, from a panel recorded when opened by keyboard. The workspace now records the focused element on each frame with no overlay open. On the frame the last overlay closes it schedules a check for after that frame is drawn, and if focus no longer reaches the workspace it goes back to the recorded element, or to the commit graph when that element is gone too. This covers every close path, including running a command from the palette, and waits while a chained overlay such as palette -> Create Branch is open. The transitions live in a window-free OverlayFocus with unit tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. |
![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: e160b691ff
ℹ️ 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".
| /// leave global shortcuts live, because every overlay takes focus when it | ||
| /// opens and each of them strands it the same way when it closes. | ||
| pub(super) fn any_overlay_visible(&self, cx: &Context<Self>) -> bool { | ||
| self.any_overlay_active(cx) |
There was a problem hiding this comment.
When Global Search is opened from the command palette, this predicate becomes false as soon as the palette closes because any_overlay_active checks the unused workspace-level overlays.global_search, while CommandId::GlobalSearch displays tab.global_search_view. After the user focuses that search input and presses Esc, the removed GlobalSearchViewEvent::Dismissed restoration no longer has a replacement, leaving focus on the hidden input and disabling workspace shortcuts until another panel is clicked. Include the active tab's search while bottom_panel_mode == BottomPanelMode::GlobalSearch or retain explicit restoration for this panel.
Useful? React with 👍 / 👎.
| /// opens and each of them strands it the same way when it closes. | ||
| pub(super) fn any_overlay_visible(&self, cx: &Context<Self>) -> bool { | ||
| self.any_overlay_active(cx) | ||
| || self.dialogs.stash_save_dialog.read(cx).is_visible() |
There was a problem hiding this comment.
After StashSave sets this dialog visible in a workspace with an open tab, any_overlay_visible remains true indefinitely: the active-workspace render chain in layout.rs never attaches stash_save_dialog (it is attached only on the no-tabs welcome path), so its UI cannot render and dismiss itself. Consequently OverlayFocus stays in its open state, and every subsequently closed palette or dialog strands focus instead of producing a Closed transition. Render the stash dialog in the active-workspace chain or do not count an entity that cannot close there.
Useful? React with 👍 / 👎.
Addresses both Codex findings on this PR. - Closing Global Search stranded focus. The overlay predicates checked `overlays.global_search`, a workspace-level entity nothing ever shows; the search that opens is the active tab's bottom-panel view, and this PR had removed the only focus hand-back its Esc handler had. Counting that view as an overlay, as suggested, would regress the palette: the search panel can stay open indefinitely, and while it did no overlay close would ever register, so closing the palette over it would strand focus again. The search panel is one case of a wider bug that was already on main: Esc in Reflog, File History, Blame, Submodules or Bisect swaps the focused view for the diff viewer, and switching tab stops drawing the focused panel at all; either leaves every shortcut dead. `track_focus` now also watches which tab and bottom-panel view are drawn. When they change with no overlay open, the same after-the-frame check runs, and stranded focus goes to the replacement: the bottom panel for a view swap, the new tab's graph for a tab switch. Focus that still reaches the workspace is left alone, so a view that takes focus as it opens keeps it. The transitions are a window-free `DrawnPanels<T>` with unit tests. `focus_panel(DiffViewer)` focused the diff viewer for every mode other than Blame, which is not drawn when another view fills the panel. It now focuses whichever view the mode draws (`BisectView` gains the `focus` its siblings already have). The unused `overlays.global_search` entity and its subscription are removed. - The stash dialog was never attached to the render tree once a tab was open, only on the welcome screen, so the Stash button and Ctrl+Z set it visible with no way to see or dismiss it. With this PR that also held `any_overlay_visible` true for the rest of the session and stopped every later overlay from handing focus back. It is now rendered with the other dialogs. The render's own overlay list duplicated `any_overlay_visible` entry for entry and now calls it, so the two cannot drift apart again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…draws Addresses the Codex finding on this PR, and a Tab-cycling case of the same bug. - Hiding the graph while it held focus handed focus to the diff viewer even when File History, Reflog, Submodules, Global Search or Bisect filled the bottom panel. The diff viewer is not drawn then, so focus was stranded and shortcuts went dead until a click. Merging main (#84) resolves it: `focus_panel(DiffViewer)` now focuses whichever view the bottom panel draws, and every route here goes through it - the render's hidden-graph redirect, Tab / Shift+Tab, Alt+4, initial focus, and the fallback when an overlay closes. - With the graph hidden and one of those views focused, Tab left focus where it was. `current_focused_panel` recognised only the diff viewer and blame, so the cycle started from nothing, at the graph, which a hidden graph resolves straight back to the bottom panel. A `ProjectTab::bottom_panel_focused` helper now covers every view the bottom panel can show, and the render's focus highlight uses it too, which also adds the Bisect view it had left out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Changelog entry for everything since 0.4.1: #78, #79, #80, #82, #83, #84, #86, #87 and the model-picker follow-up pushed to main. The version is bumped across the workspace, and the AppStream metainfo lists 0.5.0, which the Linux metadata test requires before the crate version can move. The AI work (#79, #80, #82) had no changelog entries; it now has them, with each fix checked against the 0.4.1 code so that only bugs that shipped are listed as fixed. The model-picker fixes in #82 are folded into the feature for that reason: the picker did not exist in 0.4.1. The drafts already under [Unreleased] are checked against the code and corrected where they were off: binary, symlink, submodule and modify/delete conflicts are the whole-file cases (add/add is resolved per region), Stage All skips conflicted files rather than refusing, double-clicking the commit the current branch is on shows a toast, and the diff viewer grows until the graph is down to 120px. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Summary
Fixes two problems already on
main:thread 'main' has overflowed its stackabout a second after a repository loaded. Windows gives the main thread 1 MB of stack; Linux and macOS give 8 MB.Changes
Focus (
rgitui_workspace)workspace/focus.rs.Workspace::renderrecords the focused element on every frame with no overlay open. On the frame the last overlay closes it schedules a check for after that frame is drawn: if focus no longer reaches the workspace's key context, it goes back to the recorded element, or to the commit graph when that element wasn't drawn either (for example, the command switched tab).any_overlay_visiblealso counts the stash, create-PR and clone dialogs. Themodalkey context still usesany_overlay_active, unchanged.save_focus,restore_focusandpending_focus_restore, and a stale#[allow(dead_code)]onrepo_clone_dialog.OverlayFocus<T>with unit tests.Stack (
crates/rgitui/build.rs)/STACK:8388608, the same figure Zed'sbuild.rsuses. Only address space is reserved. The target is read fromCARGO_CFG_TARGET_OS/CARGO_CFG_TARGET_ENV, because build scripts run on the build host.Testing
cargo test --workspace: 1722 passed, 0 failed)cargo clippy --workspace --all-targets -- -D warnings)cargo fmt --all)Focus, driven in a debug build on a throwaway repository with synthetic keystrokes. Each step presses Ctrl+Shift+P and checks whether the palette appears (its backdrop changes about 95% of the screen), confirmed against screenshots:
main+ stack fix)A Settings-window round trip was also attempted but is inconclusive: the driver's Esc left the Settings window in front, so its keystrokes never reached the main window. Clicking outside an overlay and each dialog's buttons go through the same close detection but weren't driven individually.
Stack: the debug executable's PE header now reserves 8 MB (
SizeOfStackReserve=8388608). It ran for 25 seconds on this repository, and through every keystroke run, without overflowing; unmodifiedmainoverflowed about a second after loading.Review fixes (609010e)
Both Codex findings were valid:
GlobalSearchViewthat is never shown. The real search is the active tab's bottom-panel view. Counting it as an overlay would have broken palette focus while the panel stays open, so the fix is broader:track_focusalso watches which tab and bottom-panel view are drawn, and when one is swapped out with no overlay open, stranded focus goes to its replacement. This also fixes Esc in Reflog / File History / Blame / Submodules / Bisect and switching tab, which stranded focus onmainalready.focus_panel(DiffViewer)now focuses whichever view the bottom panel draws. The unused entity is removed.Verified with
cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warningsandcargo test --workspace(all pass; 5 newDrawnPanelstests). Not re-driven with synthetic keystrokes: the desktop was in use.🤖 Generated with Claude Code