Enforce pinning invariant and group contiguity on tab restoration#12677
Conversation
|
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 updates closed-tab restoration so ungrouped restored tabs are clamped to the correct pinned/unpinned region and avoid landing in the middle of an existing group. The PR includes user-facing visual evidence via Loom links, and I found no security-specific concerns or spec drift because no approved spec context was provided.
Concerns
- No blocking concerns found.
- One inline suggestion asks for automated coverage of the new restore-index paths so the pinned-region and group-contiguity invariants do not regress.
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
| /// If the tab was pinned, it must land at it's original index or the end of | ||
| /// the pinned region. If the tab is not pinned, it must land at it's original | ||
| /// index or after all pinned items. | ||
| fn index_for_restored_tab(&self, tab: &TabData, original_index: usize) -> usize { |
There was a problem hiding this comment.
💡 [SUGGESTION] Add automated coverage for the new restore-index paths (pinned tab restored outside the pinned prefix, unpinned tab restored inside it, and restoration that would split an existing group). These are distinct behavior paths and currently only have manual/Loom coverage.
There was a problem hiding this comment.
you could just Warp this but idrc, non-blocking
vkodithala
left a comment
vkodithala
left a comment
There was a problem hiding this comment.
makes sense, nice improvement 🚀
| /// If the tab was pinned, it must land at it's original index or the end of | ||
| /// the pinned region. If the tab is not pinned, it must land at it's original | ||
| /// index or after all pinned items. | ||
| fn index_for_restored_tab(&self, tab: &TabData, original_index: usize) -> usize { |
There was a problem hiding this comment.
you could just Warp this but idrc, non-blocking
vkodithala
left a comment
vkodithala
left a comment
There was a problem hiding this comment.
forgot to click approve ;)
johnturcoo
deleted the
johnturco/app-4746-restoring-a-pinned-tab-should-land-in-pinned-region
branch
2 participants
Description
There are a few bugs with tab restoration (closing a tab and then using "restore session") that this PR solves:
With the changes in this PR, restored tabs now respect the pinned invariant (pinned items appear before unpinned items) and will not land within a group.
How it works
Modified
restore_closed_tablogic as followsindex_for_restored_tabLinked Issue
https://linear.app/warpdotdev/issue/APP-4746/restoring-a-pinned-tab-should-land-in-pinned-region
Testing
./script/runScreenshots / Videos
Demo of bug
Demo of fix