agent_ui: Fix agent panel focus stealing from modals#50511
Conversation
…tore When opening a modal (e.g. "Open Recent" via cmd-alt-o) with no active window, the AgentPanel async thread restoration would call set_active_view with focus: true, stealing keyboard focus from the modal. This is a regression of zed-industries#43180, where the focus guard was lost during subsequent refactoring. Thread a focus parameter through external_thread and _external_thread so that AgentPanel::load() passes false during startup restoration, while all user-initiated actions continue to pass true. Closes zed-industries#49336
The previous fix only addressed the 'load' path (restoring a saved thread), but missed the 'set_active' path where dock restoration activates the panel and creates a new thread with focus: true.
When the agent choice is already specified, call _external_thread synchronously instead of spawning an async task. This ensures focus is applied in the same frame as the panel activation, preventing race conditions where the async resolution would apply focus too late after a modal had already taken it. The async path is preserved only for the case where no agent is specified and a KVP store lookup is needed.
|
Reworked fix for the agent panel stealing keyboard focus from modals during workspace restoration. This is a successor to Root cause: When a user had no active window and triggered a modal (e.g., "Open Recent" via Cmd-Alt-O / Ctrl-Alt-O), multiple
What changedcrates/zed/src/zed.rs
crates/agent_ui/src/agent_panel.rs
Impact
How to reproduce and testWhy timing matters The focus-stealing is a race condition between async workspace restoration and modal focus. When Zed reopens a window, several Test 1: Modal focus preserved during workspace restore
Test 2: Agent panel focuses correctly on explicit open
Test 3: User-initiated agent interactions still focus
|
|
Hi @SomeoneToIgnore, I submitted a new PR on the same branch addressing the issues you pointed out in the last review. |
|
Sorry for the delay: I was partially available last week and finally back to full capacity. I have tried building |
All good, @SomeoneToIgnore, thanks for your time invested! |
|
Hmm, I'm looking to the CI errors again.... |
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Great, I have tested things and they seem to work, thank you for pushing through to this.
Before merging, let's understand why the certain changes are made and whether they are needed at all.
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Looks nice by now, thank you.
Another merge conflict resolution before the final re-test and merge is needed, though.
|
@SomeoneToIgnore, adjusted the conflicts again. |
Closes #49336
Before you mark this PR as ready for review, make sure that you have:
checklist
Video:
https://drive.google.com/file/d/1qAwAoDr4wr8cs1dosvLocU-a4pngJZvr/view?usp=sharing
Release Notes: