feat(tabs): add CycleMostRecentTab as third Ctrl+Tab behavior option#9658
Conversation
|
I ran into an unexpected error while working on this. 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 posted feedback on 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 adds a CycleMostRecentTab Ctrl+Tab behavior with MRU tab tracking, a tabs-only command palette data source, action wiring, and settings UI support.
Concerns
- The selection-offset ordering change applies to the existing synchronous sessions Ctrl+Tab path as well as the new tabs path, causing
CycleMostRecentSessionto advance twice on first open. - Security pass found no additional concerns.
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 |
There was a problem hiding this comment.
Overview
This PR adds a new Ctrl+Tab behavior that should present tabs in most-recently-used order through the command palette UI, with supporting MRU tracking, a tabs data source, and settings UI wiring.
Concerns
- The new tabs palette path appears to run its synchronous query before the
Workspaceaction handler returns, so the weak workspace handle can fail to upgrade and the palette opens with no tab results. - After the tabs-only mixer is installed, switching the Ctrl+Tab setting back to session cycling does not restore the session data source for the Ctrl+Tab palette.
- Security pass: no security-specific issues found in the inlined diff.
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
There was a problem hiding this comment.
Overview
This PR adds a third Ctrl+Tab behavior that cycles tabs by workspace MRU order, with new tab navigation data, a tabs command-palette data source, and settings UI wiring. The supplemental security pass did not identify security findings tied to the diff.
Concerns
- The new tabs data source is queried synchronously while
Workspace::open_ctrl_tab_paletteis still executing inside a workspace update. Because it upgrades a weak handle back to the same workspace at that point, the initial tabs query can return empty results and the new Ctrl+Tab behavior fails to show/select tabs.
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 |
There was a problem hiding this comment.
Overview
This PR adds a third Ctrl+Tab behavior that opens a tabs-only navigation palette ordered by most-recently-used tab activation, plus settings UI and tab metadata rendering.
Concerns
- Tab palette selections are still enqueued into the shared command-palette recent-items store, but Tab summaries intentionally cannot be reconstructed for recent items. Repeated Ctrl+Tab use can therefore fill the bounded recent queue with invisible entries and evict useful command/session recents.
- Supplemental security pass: no security issues identified in the inlined diff.
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 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 adds a third Ctrl-Tab behavior that opens a tabs-only navigation palette backed by MRU tab tracking, plus the settings/UI/data-source plumbing needed to render and activate tab results.
Concerns
- The new tabs search item imports
SingletonEntitywithout using it, which should be removed before the presubmit clippy gate. - No security findings were identified in the changed diff.
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 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 adds a third Ctrl-Tab behavior that opens a tabs-only navigation palette ordered by most-recently-used tabs, along with settings UI, data-source wiring, MRU tracking, and activation handling.
Concerns
- UI evidence is missing for this user-visible change. The diff changes settings copy and adds a new two-line tab palette row, but the provided PR context has no screenshot or video demonstrating the settings option and Ctrl-Tab tab-switcher flow end to end.
- Security pass: no security concerns were identified in the changed lines.
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
Akeuuh
force-pushed
the
axel/ctrl-tab-mru-tabs
branch
from
4c6526c to
f4e1772
Compare
|
/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: @acarl005. 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 adds a Ctrl+Tab behavior that uses a tabs-only command palette backed by workspace tab MRU order, with settings UI wiring and tab activation support.
Concerns
- The tab palette displays each tab's working directory but only searches tab titles, so the visible subtitle cannot be used to disambiguate or find tabs.
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
|
/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: @acarl005. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
Adds a third Ctrl+Tab behavior that opens a tabs-only navigation palette backed by MRU tab ordering, plus the settings dropdown entry and tab palette rendering.
Concerns
- No blocking correctness or security concerns found in the annotated 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
There was a problem hiding this comment.
Thanks @Akeuuh, I dig this idea!
Before I review the code in depth, I want to discuss the UI. The only problem is that there are cases where the palette items can look identical. I'd like to propose a way to distinguish when tabs have the same title + subtitle. Can you prefix the title with something like "[Tab N] " where N is a 1-based index from left-to-right? That way tabs are always unambiguous.
I suppose that is also a problem with the session-based ctrl+tab, but those items don't have a straightforward numbering sequence so we'll just have to accept that.
Also, I appreciate the video you have posted. Can you also include a screenshot of the palette for quicker reference?
I left a couple of comments on code requesting changes but I'll do a more in-depth review once the above is addressed.
Akeuuh
force-pushed
the
axel/ctrl-tab-mru-tabs
branch
from
dc38523 to
0068345
Compare
|
Thanks for the thorough review @acarl005! All your comments have been addressed in the latest push:
Let me know if anything else needs adjusting! |
There was a problem hiding this comment.
Regarding this comment:
That wasn't the only instance. There are several other methods in the same file that have the same issue (redundant updates to self.tab_mru_order). We should rely on fn set_active_tab_index as the single source of truth for keeping self.tab_mru_order in sync. Please clean those up as well. After that (and dealing with any CI issues), this should be ready to merge.
Add a new CtrlTabBehavior::CycleMostRecentTab option that cycles through tabs by most-recently-used order via the existing ctrl_tab palette. - Track tab MRU order in Workspace via tab_mru_order Vec<EntityId> - Add QueryFilter::Tabs and tabs data source module (data_source, search_item) - Wire NavigateToTab action through mixer, palette view, and root_view - Show tab title (bold) + working directory subtitle (gray) in palette - Add CycleMostRecentTab to settings dropdown as third option - Fix palette pre-selection offset for sync data sources
- Guard select_next/prev to only fire when palette already open; on first open, set_initial_selection_offset(1) handles pre-selection - Assign descending scores by MRU rank (1000 - rank) so TopDown rendering preserves most-recently-used order - Update tab_mru_order in adopt_transferred_pane_group when placeholder pane group is replaced with transferred one
The offset-before-filter ordering is only needed for QueryFilter::Tabs (synchronous data source). Applying it unconditionally caused CycleMostRecentSession to double-advance on first Ctrl+Tab. Also applies cargo fmt to all new code.
Bug 1: tabs::DataSource held WeakViewHandle<Workspace> but run_query executes while workspace is borrowed (inside update_view), so upgrade() returns None and palette opens empty. Fix: pre-compute tab snapshot in open_ctrl_tab_palette and pass Vec<TabNavigationData> to DataSource. Bug 2: reset_ctrl_tab_mixer wipes all mixer sources via mixer.reset() then adds only tabs. Switching back to CycleMostRecentSession never restored sessions source. Fix: add restore_ctrl_tab_session_mixer that resets mixer and re-adds sessions_data_source.
NavigateToTab actions are transient Ctrl+Tab selections — recording them in SelectedItems evicts real recent items over repeated use.
- search_item: fix is_multiline for tabs without subtitle (e.g. settings) - search_item: add doc comment explaining ctrl-tab-only design - search_item: include tab title in accessibility_help_message - workspace: cfg(test) guard on tab_mru_order(), reduce pub visibility - workspace: merge CycleMostRecentSession/Tab arms, preserve behavior diff - workspace: remove redundant tab_mru_order push (set_active_tab_index handles it) - workspace: replace if/else with match on QueryFilter for exhaustiveness - workspace: remove extra blank line - view: reword transient actions comment per reviewer suggestion - data_source: replace TABS_FILTER_ATOM with NO_FILTER_ATOM (tabs not in main palette)
set_active_tab_index is the single source of truth for MRU tracking. Remove manual pushes that duplicated the update before activate_tab_internal. Also remove unused SingletonEntity import and apply formatting.
Akeuuh
force-pushed
the
axel/ctrl-tab-mru-tabs
branch
4 times, most recently
from
7dc7bde to
97f57ad
Compare
Akeuuh
force-pushed
the
axel/ctrl-tab-mru-tabs
branch
from
97f57ad to
cf8c606
Compare
|
Hey @acarl005, the CI hasn't been triggered yet on this PR (fork PR, needs manual approval). Could you kick it off? The presubmit issues that were flagged should now be fixed — the recent commits address the compilation errors and clippy warnings from the rebase. |
|
Ok CI is passing. I'm just going to run this locally again after the changes to make sure the behavior is still correct. |
|
@Akeuuh Thanks for the new feature! |
|
Thanks a lot @acarl005 for the thorough review and for going through multiple rounds with me on this! The back-and-forth really shaped the feature for the better — the |
|
This missed the cut to be included in this week's release but it should be in next week's assuming no issues are found in dogfood. |
…arpdotdev#9658) (cherry picked from commit 0320c79)
2 participants
Summary
Adds a new
CycleMostRecentTaboption to the Ctrl+Tab behavior settings, allowing users to cycle through tabs by most-recently-used order using the existing ctrl_tab palette UI.Validation
Enregistrement.de.l.ecran.2026-05-04.a.09.09.12.mov
What changed
CtrlTabBehaviorvariant:CycleMostRecentTabadded as a third option alongsideActivatePrevNextTabandCycleMostRecentSessionWorkspacetracks tab activation order viatab_mru_order: Vec<EntityId>, updated inset_active_tab_index()search/command_palette/tabs/module withDataSource(provides MRU-ordered tabs) andSearchItem(renders two-row layout: bold title + gray CWD subtitle)QueryFilter::Tabs,NavigateToTabaction,reset_ctrl_tab_mixer()for tabs-only palette moderoot_viewhandlesactivate_tab_by_pane_group_idaction to switch to the selected tabset_initial_selection_offsetnow called beforeset_active_query_filterto fix pre-selection for synchronous data sourcesFiles changed (16 files, +435/-7)
settings/mod.rsCycleMostRecentTabenum variantsettings_view/features_page.rssession_management.rsTabNavigationDatastruct with subtitleworkspace/view.rstab_navigation_data(), palette opening logicworkspace/view_test.rstest_tab_mru_orderunit testsearch/data_source.rsQueryFilter::Tabsvariantsearch/filter_chip_renderer.rssearch/command_palette/tabs/mod.rs,data_source.rs,search_item.rssearch/command_palette/data_sources.rsreset_ctrl_tab_mixer(),ItemSummary::Tabsearch/command_palette/mixer.rsNavigateToTabactionsearch/command_palette/view.rsNavigateToTabhandlersearch/command_palette/filter_chip_renderer.rsroot_view.rsactivate_tab_by_pane_group_idaction handlerHow to test