specs(GH12306): tab transfer between standard and dedicated hotkey windows - #12986
specs(GH12306): tab transfer between standard and dedicated hotkey windows#12986danieltsam wants to merge 6 commits into
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @danieltsam on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
Every PR must be linked to a same-repo issue before Oz can review it. Next step: open or find a same-repo issue describing this change, then link it to this PR by adding See the contribution guidelines for the full readiness model. Powered by Oz |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
Next step: open or find a same-repo issue describing this change, then link it to this PR by adding Closes #123 to the PR description (or using the "Development" sidebar on GitHub). A maintainer will mark the issue ready-to-spec when it is ready. Once it is marked, comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
|
@cla-bot check |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @danieltsam on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
/oz-review |
|
I'm re-reviewing this spec-only 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 spec adds product and technical plans for moving active or selected tabs between standard Warp windows and the dedicated hotkey window via command-palette actions. The scope is aligned with the linked issue's first proposal, and there were no separate high-level security concerns for this UI/window-management change.
Concerns
- The target standard window selection is unspecified when multiple standard windows exist, so the command behavior could be unpredictable.
- The hotkey last-tab lifecycle remains an open question while the technical flow and validation imply a concrete close-window behavior.
- The multi-selection transfer algorithm can reverse tab order if tabs are removed in descending order and appended immediately.
- The product requires keybindings UI hiding, but the tech plan only describes view-context predicates and does not specify how the keybindings UI is gated.
Verdict
Found: 0 critical, 4 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
|
|
||
| **`workspace:move_active_tab_to_standard_window`** | ||
| 1. User invokes from the hotkey window. | ||
| 2. If another standard window is open, tabs are moved there. |
There was a problem hiding this comment.
|
|
||
| ## Open questions | ||
|
|
||
| - **Hotkey window lifecycle on last-tab-moved-out:** When the last tab is moved out of the hotkey window to a standard window, should the hotkey window close automatically (same as standard window behavior), or should it remain open as an empty quake overlay? An empty overlay is unusual for a quake-style window but keeps the hotkey available for immediate reuse. |
There was a problem hiding this comment.
| 7. `move_active_tab_to_window(target_window_id, ctx)` is called. | ||
| 8. `selected_tab_indices()` gathers multi-selected indices, or falls back to `active_tab_index`. | ||
| 9. For each selected tab: `get_tab_transfer_info_for_attach` → `prepare_for_transferred_tab_attach` → `ctx.transfer_view_tree_to_window`. | ||
| 10. Tabs are removed from the source (descending index via `remove_tab_without_undo`, or `close_window_for_content_transfer` if it was the last tab). |
There was a problem hiding this comment.
| - **3 — Toggle moves to opposite type:** Invoke toggle from both window types. | ||
| - **4 — Multi-selection:** Enable `FeatureFlag::GroupedTabs`, select 2+ tabs, invoke each action. | ||
| - **5 — Last tab closes window:** Move the only tab out of a window. | ||
| - **6 — Hidden when setting disabled:** Disable quake mode, search command palette. |
There was a problem hiding this comment.
… lifecycle, tab order, keybindings UI gating
|
Pushed fixes for all 4 review concerns:
/oz-review |
|
I'm re-reviewing this spec-only 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 spec PR adds product and tech specs for command-palette actions that move active or multi-selected tabs between standard Warp windows and the dedicated hotkey window. The scope is aligned with the linked issue's tab-transfer direction and no design-level security findings were identified.
Concerns
- The tech spec selects the destination standard window with
ctx.window_ids()even though that iterator is not MRU ordered, conflicting with the product requirement to move to the most recently focused standard window. - The no-standard-window path is underspecified for multi-selected tabs because the existing new-window transfer primitive creates a window from one transferred tab, while the shared transfer routine expects a target window for all selected tabs.
- Validation does not cover the keybindings UI visibility requirement.
Verdict
Found: 0 critical, 2 important, 1 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
|
|
||
| **New handler methods** (~line 27093): | ||
| - `move_active_tab_to_dedicated_hotkey_window` — Verifies the hotkey window is open (opens it via the newly-`pub(crate)` `toggle_quake_mode_window` if needed), then calls `move_active_tab_to_window`. | ||
| - `move_active_tab_to_standard_window` — Finds an existing standard window via `ctx.window_ids()` (excluding self and quake) — the first non-self, non-quake ID corresponds to the most recently focused standard window. If none exists, calls `create_transferred_window`. Calls `move_active_tab_to_window` in either case. |
There was a problem hiding this comment.
ctx.window_ids() iterates window map keys, not most-recently-focused order, so the first non-self/non-quake ID will not reliably satisfy the product requirement to target the most recently focused standard window; specify an MRU/active-window source or change the target-selection behavior.
| - `move_active_tab_to_dedicated_hotkey_window` — Verifies the hotkey window is open (opens it via the newly-`pub(crate)` `toggle_quake_mode_window` if needed), then calls `move_active_tab_to_window`. | ||
| - `move_active_tab_to_standard_window` — Finds an existing standard window via `ctx.window_ids()` (excluding self and quake) — the first non-self, non-quake ID corresponds to the most recently focused standard window. If none exists, calls `create_transferred_window`. Calls `move_active_tab_to_window` in either case. | ||
| - `toggle_active_tab_window_type` — Dispatches to the appropriate directional method based on current window. | ||
| - `move_active_tab_to_window(target_window_id, ctx)` — Core logic: gathers selected indices (multi-selection from `selected_tab_indices()` or fallback to `active_tab_index`), collects transfer infos in ascending order, removes tabs from source in descending order (to avoid index shift), then inserts collected tabs at end of target in original ascending order, focuses target window. |
There was a problem hiding this comment.
create_transferred_window seeds a new window from one TransferredTab, while this shared helper assumes an existing target_window_id for all selected tabs; define first-tab seeding plus appending remaining tabs, or add a multi-tab window-creation primitive.
| - **3 — Toggle moves to opposite type:** Invoke toggle from both window types. | ||
| - **4 — Multi-selection:** Enable `FeatureFlag::GroupedTabs`, select 2+ tabs, invoke each action. | ||
| - **5 — Last tab closes window:** Move the only tab out of a window. | ||
| - **6 — Hidden when setting disabled:** Disable quake mode, search command palette. |
There was a problem hiding this comment.
💡 [SUGGESTION] The success criteria require hiding the commands from both the command palette and keybindings UI, but this validation only checks command palette visibility; add a keybindings settings check for the enabled-predicate path.
…ab new-window flow, keybindings UI validation
|
Second round of fixes addressing all 3 concerns:
/oz-review |
|
I'm re-reviewing this spec-only 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 spec PR defines product and tech specs for command-palette tab transfer between standard windows and the dedicated hotkey window. The scope is aligned with the linked issue's tab-transfer proposal and intentionally defers non-dedicated hotkey mode.
Concerns
- The tech spec relies on
EditableBinding::with_enabledfor a user setting, but that predicate is afn() -> boolwith noAppContext, so the keybindings UI hiding requirement is not implementable as written. - The hotkey-window open check only verifies
ctx.is_window_open(id), which does not distinguish a hidden quake window from a visible/open one. - The core transfer flow calls
prepare_for_transferred_tab_attachwithout specifying when to clearsuppress_detach_panes_on_window_closeif the source window remains alive.
Verdict
Found: 0 critical, 3 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
| | `workspace:move_active_tab_to_standard_window` | `Workspace & Quake_Mode_Editor & Workspace_InQuakeWindow` | `quake_mode_enabled` | | ||
| | `workspace:toggle_active_tab_window_type` | `Workspace & Quake_Mode_Editor` | `quake_mode_enabled` | | ||
|
|
||
| The `Quake_Mode_Editor` flag (set by `root_view.rs` when `quake_mode_enabled` is true) and the enabled predicate both gate on the same setting. The context predicate hides from the command palette and dispatch; the enabled predicate hides from the keybindings settings UI (following the `TOGGLE_VERTICAL_TABS_PANEL` pattern in the codebase). `Workspace_InQuakeWindow` (new) differentiates standard from hotkey windows. |
There was a problem hiding this comment.
with_enabled cannot gate on quake_mode_enabled because enabled predicates are fn() -> bool and cannot read KeysSettings; either make keybindings visibility setting-aware or remove that success criterion.
| 2. The `EditableBinding` matches via context predicate (`Workspace & Quake_Mode_Editor & !Workspace_InQuakeWindow`). | ||
| 3. `WorkspaceAction::MoveActiveTabToDedicatedHotkeyWindow` is dispatched to `TypedActionView`. | ||
| 4. `move_active_tab_to_dedicated_hotkey_window` is called. | ||
| 5. `quake_mode_window_id()` retrieves the hotkey window ID, verified with `ctx.is_window_open(id)`. |
There was a problem hiding this comment.
ctx.is_window_open(id) treats hidden quake windows as open, so this flow can move tabs into a hidden overlay without showing it; require checking QUAKE_STATE/WindowState or explicitly showing/focusing the quake window before transfer.
| - `move_active_tab_to_dedicated_hotkey_window` — Verifies the hotkey window is open (opens it via the newly-`pub(crate)` `toggle_quake_mode_window` if needed), then calls `move_active_tab_to_window`. | ||
| - `move_active_tab_to_standard_window` — Finds an existing standard window via `ctx.ordered_window_ids()` (z-order front-to-back, skipping self and quake). If none exists, calls `create_transferred_window` for the first tab, then transfers remaining tabs to that new window. If one exists, calls `move_active_tab_to_window` with that window as target. | ||
| - `toggle_active_tab_window_type` — Dispatches to the appropriate directional method based on current window. | ||
| - `move_active_tab_to_window(target_window_id, ctx)` — Core logic: gathers selected indices (multi-selection from `selected_tab_indices()` or fallback to `active_tab_index`), collects transfer infos in ascending order, removes tabs from source in descending order (to avoid index shift), then inserts collected tabs at end of target in original ascending order, focuses target window. |
There was a problem hiding this comment.
suppress_detach_panes_on_window_close when the source window is not closed; prepare_for_transferred_tab_attach documents that flag as not auto-restored, so leaving it set can break later normal closes.
…, quake visibility check, suppress_detach cleanup, const flag, handler line fix
|
Third round of fixes:
/oz-review |
|
I'm re-reviewing this spec-only 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 spec PR defines product and technical plans for moving active or multi-selected tabs between standard windows and the dedicated hotkey window through command-palette/keybinding actions.
Concerns
- The product spec claims to cover the context-menu half of the issue, but the UX and tech specs only define command-palette/keybinding actions.
- The no-standard-window transfer path for multi-selected tabs is underspecified and could lead to an implementation that loses selection/order or mishandles source-window closure.
Verdict
Found: 0 critical, 2 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
|
|
||
| ## Summary | ||
|
|
||
| Three command-palette actions that let users move active tabs (or multi-selections) between standard terminal windows and the dedicated hotkey (quake) window. This covers the keyboard/context-menu half of the issue's first proposal; the drag-and-drop half is being shipped concurrently by the Warp team. |
There was a problem hiding this comment.
|
|
||
| **New handler methods** (~line 26703, alongside existing transfer functions): | ||
| - `move_active_tab_to_dedicated_hotkey_window` — Verifies the hotkey window is open and visible via `quake_mode_window_is_open()` (opens it via the newly-`pub(crate)` `toggle_quake_mode_window` if closed or hidden), then calls `move_active_tab_to_window`. | ||
| - `move_active_tab_to_standard_window` — Finds an existing standard window via `ctx.ordered_window_ids()` (z-order front-to-back, skipping self and quake). If none exists, calls `create_transferred_window` for the first tab, then transfers remaining tabs to that new window. If one exists, calls `move_active_tab_to_window` with that window as target. |
There was a problem hiding this comment.
…detail multi-tab no-window flow
|
Fourth round of fixes:
/oz-review |
|
This is your last |
|
I'm re-reviewing this spec-only 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 no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This spec PR adds product and technical coverage for command-palette actions that transfer active or multi-selected tabs between standard windows and the dedicated hotkey window. The specs define the intended commands, context gating, transfer flow, multi-selection behavior, validation plan, risks, and follow-ups.
Concerns
- No blocking completeness, feasibility, issue-alignment, or security concerns found.
- One suggestion: clarify the hotkey last-tab lifecycle follow-up so it does not appear to conflict with the product success criterion that the source window closes when its last tab is moved.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| - **Drag-and-drop standard→hotkey** — The team is shipping hotkey→standard drag this cycle. The reverse direction would extend the cross-window drag system (`Workspace::drag_tab_over`, `tab_insertion_index_for_cursor`) to accept `WindowStyle::Pin` windows as valid drop targets. | ||
| - **Non-dedicated hotkey mode** — Deferred per maintainer signal. Would require a new `GlobalHotkeyMode` variant, different window creation path, changes to quake state lifecycle. | ||
| - Consider auto-hide on last-tab-moved-out from hotkey window. |
There was a problem hiding this comment.
💡 [SUGGESTION] This follow-up reopens hotkey last-tab lifecycle even though product.md makes closing the source window a success criterion. Clarify whether auto-hide is an alternative to closing, a post-close behavior, or remove this follow-up so implementers have one target behavior.
There was a problem hiding this comment.
Removed the conflicting auto-hide follow-up per Oz's suggestion. The last-tab lifecycle is consistent: source window always closes on last-tab-out.
peicodes
left a comment
peicodes
left a comment
There was a problem hiding this comment.
Cross-window tab drag is now shipped for MacOS and Windows. I think the feature doesn't work when dragging into a hotkey window.
I think the ideal solution would be to extend the functionality of cross-window tab drag to support dragging into a hotkey window. Could you explore that path and update the spec?
|
Appreciate you shipping hotkey→standard drag. That direction works well since the overlay is on top. For standard→hotkey though, the overlay sits on the same window, so dragging requires rearranging or guessing where to drop. With the default setup (pin to top, autohide on kb focus loss) the overlap makes it even worse and you'd lose the overlay as soon as you reach for the standard window. The command-palette actions in this spec avoid that entirely since they're keyboard-driven. Happy to be corrected if there's a UX workaround I'm not seeing but to me the keyboard transfers feel like the more natural primary path here. I thought about this initially but should've flagged this earlier, sorry. |
|
Hi @danieltsam — a reviewer requested changes on this PR and it hasn't had activity from you in 9 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 14 days of inactivity. |
danieltsam
left a comment
danieltsam
left a comment
There was a problem hiding this comment.
Appreciate you shipping hotkey→standard drag. That direction works well since the overlay is on top. For standard→hotkey though, the overlay sits on the same window, so dragging requires rearranging or guessing where to drop. With the default setup (pin to top, autohide on kb focus loss) the overlap makes it even worse and you'd lose the overlay as soon as you reach for the standard window. The command-palette actions in this spec avoid that entirely since they're keyboard-driven.
Happy to be corrected if there's a UX workaround I'm not seeing but to me the keyboard transfers feel like the more natural primary path here. I thought about this initially but should've flagged this earlier, sorry.
|
Hi @danieltsam — final reminder: a reviewer requested changes on this PR and it has been inactive for 10 days. It will be automatically closed in about 4 day(s) unless you push updates or reply. Maintainers can apply the |
|
Closing this pull request because the requested changes have gone unaddressed for over 14 days. If you'd like to continue, push your updates and reopen the PR (or comment to ask a maintainer to reopen) — we'd be glad to pick it back up. |
danieltsam
left a comment
danieltsam
left a comment
There was a problem hiding this comment.
Appreciate you shipping hotkey→standard drag. That direction works well since the overlay is on top. For standard→hotkey though, the overlay sits on the same window, so dragging requires rearranging or guessing where to drop. With the default setup (pin to top, autohide on kb focus loss) the overlap makes it even worse and you'd lose the overlay as soon as you reach for the standard window. The command-palette actions in this spec avoid that entirely since they're keyboard-driven.
Happy to be corrected if there's a UX workaround I'm not seeing but to me the keyboard transfers feel like the more natural primary path here. I thought about this initially but should've flagged this earlier, sorry.
|
@peicodes would like to reopen this pr and hear your take on the concern i shared earlier in the previous comment |
2 participants
Description
Product spec and tech spec for issue #12306 (tab transfer between standard and dedicated hotkey windows).
Adds three command-palette actions (
workspace:move_active_tab_to_dedicated_hotkey_window,workspace:move_active_tab_to_standard_window,workspace:toggle_active_tab_window_type) that let users move active tabs between standard windows and the quake overlay. This covers the keyboard/context-menu half of the issue's first proposal; the Warp team is shipping cross-window tab drag (hotkey→standard) concurrently.The spec evaluates the scope decision: command-palette actions as a keyboard-driven complement to drag-and-drop, sharing the same transfer primitives. The second proposal from the issue (non-dedicated hotkey mode) is deferred per maintainer signal.
Open questions in the spec cover hotkey window lifecycle when its last tab is moved out.
Linked Issue
Closes #12306
ready-to-spec.Testing
N/A — spec-only change. The validation plan for the implementation lives in the
Testing and validationsection oftech.md.Agent Mode