fix(tab-configs): gate Space toggle binding so text params accept spaces (#11138)#11247
fix(tab-configs): gate Space toggle binding so text params accept spaces (#11138)#11247maxmilian wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
external-contributor
…ces (warpdotdev#11138) Root cause: the modal-level Space and Enter `FixedBinding`s were scoped to `id!("TabConfigParamsModal") & !id!("EditorView")` in an attempt to suppress them while a descendant text editor is focused. But keystroke dispatch in `crates/warpui_core/src/core/app.rs` evaluates each layer's `ContextPredicate` against that layer's own `keymap_context` only — it does not merge ancestors and descendants. The modal layer's context never contains `"EditorView"`, so the `!id!("EditorView")` guard is always true on that layer, the binding always matches, and `ToggleDropdown` fires before the focused text editor can swallow the space. Fix: introduce a `TabConfigParamsModalDropdownOnly` marker that the modal adds to its own `keymap_context` when no `Text` param is present, and scope the Space binding to `id!(DROPDOWN_ONLY_FLAG)`. Dropdown-only modals continue to support Space-to-toggle; mixed configs no longer steal the space, so it lands in the focused text editor as a literal character. Enter binding is left alone: the focused editor emits `EditorEvent::Enter` through the existing subscription, so submit still works. 4 new unit tests cover both `keymap_context` outputs and the `ContextPredicate` behavior in both branches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
maxmilian
force-pushed
the
fix/11138-tab-params-space-key
branch
from
0f49e19 to
2709059
Compare
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR narrows the tab-config params modal Space binding so mixed text/dropdown configs can type literal spaces in text params while preserving the dropdown-only shortcut.
Concerns
- For this user-facing keyboard interaction change, please include screenshots or a short screen recording demonstrating the mixed-config modal accepting spaces end to end.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
pr11247-before-after.mp4Visual evidence — end-to-end recording (per Oz review request)Side-by-side comparison using a tab config that mixes a
Tab config used in both runs: name = "PR 11247 repro"
[[panes]]
id = "main"
type = "terminal"
directory = "{{repo}}"
commands = []
[params.repo]
type = "repo"
description = "Repository to open"
[params.tab_name]
type = "text"
description = "Tab name"
default = "my tab"/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR changes the tab-config params modal so the modal-level Space binding only fires for dropdown-only configs, allowing text params in mixed configs to receive literal spaces. It also adds unit coverage for the new keymap-context behavior.
Concerns
- For this user-facing behavior change, please include screenshots or a short screen recording demonstrating it working end to end. The PR description describes local tests, but it does not include visual evidence for the changed modal behavior.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed stale reviews from themself
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR scopes the tab config params modal's Space fixed binding to dropdown-only modals by adding a dedicated keymap-context marker when no text params are present. It also adds focused unit coverage for the keymap context and the binding predicate behavior, and the attached visual evidence demonstrates the mixed repo/text-param regression before and after the fix.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the changed diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
moirahuang
left a comment
moirahuang
left a comment
There was a problem hiding this comment.
a higher level comment, i think a better fix would be to move the space handling out of the params modal and into the dropdown picker instead. the one thing we'd want to ensure is that if there's only 1 item in the modal, we're focused on that item.
the semantics of what i'd recommend is something like "if the dropdown is focused, the dropdown owns Space"
2 participants
Summary
Fixes #11138. In a tab config that mixes a
repoparam with atextparam, pressing Space while the text field is focused was opening the repo dropdown instead of inserting a literal space. This made multi-word text params impossible to enter.Visual evidence
End-to-end recording with a tab config that mixes a
repodropdown and atextparam. Same tab config TOML is used in both runs.pr11247-before-after.mp4
v0.2026.05.18.05.32.stable_02): right-click+→ "PR 11247 repro" → modal opens with therepodropdown +tab_nametext field. Clicking intotab_nameand pressing Space pops therepodropdown open instead of inserting a space — the modal-levelToggleDropdownbinding fires even withEditorViewfocused.fix/11138-tab-params-space-key): same config (copied to~/.warp-oss/tab_configs/), same modal. Pressing Space intab_nameinserts a literal space (my tab→my tab→my tab); the dropdown stays closed. The Space binding is now scoped toid!(TabConfigParamsModalDropdownOnly), which is only emitted bykeymap_contextwhen the config has noTextparam.Tab config used in both runs:
Root cause
app/src/tab_configs/params_modal.rsregistered the modal-level Space binding with the scope predicateid!("TabConfigParamsModal") & !id!("EditorView"), on the assumption that the!id!("EditorView")clause would suppress the binding while a descendant text editor is focused.But keystroke dispatch (
crates/warpui_core/src/core/app.rs:1998-2034) evaluates each layer'sContextPredicateagainst that layer's ownkeymap_contextonly — it never merges ancestors with descendants. When the focused view isEditorView:EditorViewlayer the context contains"EditorView"but not"TabConfigParamsModal", so the predicate is false. The editor itself has no Space FixedBinding, so dispatch returns no match for the editor layer and continues up the chain.TabConfigParamsModallayer the context contains"TabConfigParamsModal"but never"EditorView"(the modal isn't an editor), so!id!("EditorView")is always true. The predicate matches,ToggleDropdownfires, the editor never sees the space.Oz's triage on the issue flagged this exact concern: "whether fixed-binding scope matching is not recognizing the focused text input as an
EditorViewdescendant." It isn't — by design.(
!id!("EditorView")was used nowhere else in the codebase; only the two bindings in this file relied on it.)Fix
TabConfigParamsModalDropdownOnlymarker.TabConfigParamsModal::keymap_contextto insert that marker only whenhas_text_fields()is false (i.e. the config has noTextparam at all).id!(DROPDOWN_ONLY_FLAG).Dropdown-only modals (repo-only / branch-only configs) keep Space-to-toggle. Mixed configs no longer steal the space — it falls through to the focused editor as a literal character, which is what users expect.
Tests
4 new unit tests in
app/src/tab_configs/params_modal_tests.rs:keymap_context_omits_dropdown_only_flag_when_text_field_present— the modal's emitted context.keymap_context_includes_dropdown_only_flag_when_no_text_fields— complement.space_binding_predicate_blocks_when_text_field_present— the load-bearing regression test for pressing space on tab params switches input field #11138: with a text param present, the predicate must NOT match.space_binding_predicate_fires_in_dropdown_only_modal— preserves the dropdown-only shortcut.All 7 module tests pass locally (
cargo test -p warp --lib tab_configs::params_modal::).cargo fmt --checkandcargo clippy -p warp --libare clean.Scope
Deliberately not changed:
!id!("EditorView")scope. The guard is also dead (same per-layer evaluation reason), but Enter is not reported as broken. The dispatch loop walks the responder chain in reverse, so the editor layer is hit first —EditorViewhas its own Enter binding that matches and short-circuits the loop, never reaching the modal-layer Enter binding. The modal still receives a separateEditorEvent::Entervia the subscription inhandle_editor_event, which callstry_submit. Result: single submit, no double-fire. Touching the Enter binding here would expand the surface for non-reported behavior.!id!("EditorView")framework semantics are left as-is. Fixing the framework so chain-wide negative scopes work is a much larger change.Follow-up
!id!("EditorView")from the Enter binding too — it's misleading dead code now that the mechanism is understood.!id!(...)against a descendant view'sui_name; this codebase only has the two inparams_modal.rs, but the pitfall is easy to hit.EditorViewcannot accept a literal Space either, because the modal-layer Space binding swallows it. Same root cause; out of scope for this PR.