fix(tabs): keep the sort and page of a restored tab you never opened - #2255
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in how a restored tab's view state round-trips. Found while investigating #2234; the second is why the first could not ship alone.
A tab you never clicked loses its sort and page
Restore parks a tab's saved sort and page in
pendingRestoredSort/restoredPagerather than applying them, because applying needs the schema. OnlyapplyPendingRestoredViewStateconsumes them, and it is reachable solely throughprepareTableTabFirstLoad, whose first guard istabManager.selectedTabId == tabId. So only the tab in front ever consumes its own state.Every save maps every tab through
toPersistedTab(), which re-derived the sort fromsortState.columnsand the page frompagination.currentPage. Both are still empty on a tab that has not been activated, so the very next autosave wrotenilover each of them.Leave five tabs sorted and on page 3, quit, relaunch, and only the selected tab keeps its state. Wait 30 seconds for the periodic save, or just quit again, and the other four have lost it permanently, without the user having touched them.
toPersistedTab()now falls back to the pending values when the live ones are empty, which is exactly whatrestoredCursorOffsetandcolumnWidthsalready do. The consumption side is untouched:applyPendingRestoredViewStatestays the only consumer.A restored page index was read in the wrong page size
This is why the first fix could not ship on its own. Restoring the page number makes the page restore actually happen, and the offset is recomputed as
(page - 1) * pageSizeagainstAppSettingsManager.shared.dataGrid.defaultPageSize, not against the size the page was counted in.PersistedTabhad no page size at all.Set a tab to 100 rows per page, go to page 12 (rows 1101-1200), quit. It comes back as page 12 of the 1,000 default: offset 11,000, an empty grid, and a pager reading 12 of 2. Fixing the loss alone would have taken this from rare to routine, which is what made it scope rather than a follow-up.
PersistedTabgainsrestoredPageSize, and restore reads the page index in the size it was counted in.Two things fall out of that framing, and both are deliberate:
currentPageis 1, so no page size is written and relaunching never re-fetches a million rows.It also preserves the existing contract.
PersistedTabRoundTripTests.paginationSeedsFromLivePageSizepins that a restored tab seeds its page size from the live default rather than from the persisted query text, and that test has a tab on page 1: no page size is persisted, the default still applies, and it passes unchanged. The new field is an explicit, trustworthy value rather than a parse of the query string, which is what that test was guarding against.restoredPageSizeis decoded withdecodeIfPresent, so a tab written by an older build restores its page exactly as before.Keeping the fallback from outliving the restore
Making the pending values survive a re-save means they now outlive a single activation, and three paths relied on them being transient. Self-review caught all three:
execution.lastExecutedAt == nil. Without that, a user who pages back to 1 and quits reopens on page 12 forever, and nothing can correct it: the only code that clears the pending fields is the first-load path.replaceTabContentalready resets sort, pagination, filters and layout when a tab is pointed at a different table, which is what clicking a sidebar table during a restored tab's schema load does. The pending fields now reset with them, ororders' page 12 and sort get persisted againstcustomersand applied to it next launch.tabType == .table, so a query tab would never clear one and a sort the user cleared would reappear in the persisted record permanently.restoredPageSizeis also clamped toSettingsValidationRules.defaultPageSizeRangeon decode, so a hand-edited or out-of-range value cannot make a restored tab issue an enormous first fetch.Verification
Run through
verify.shin an isolated worktree on752019a6e.generatePASS,buildPASStestPASS, 46 of 46: PersistedTabRoundTripTests, DefaultSortInitialQueryTests, TabPersistenceTests, QueryTabManagerTestsswiftlint --strictclean over every touched fileThe new tests pin the parts that failed silently: an unactivated tab re-emits its sort and its page, three consecutive save/restore cycles keep both, a persisted page carries the size it was counted in, page 1 carries no size (the Show All guard), and a tab written before the field existed still restores its page.
No UI automation. The flow is quit, relaunch and restore across three launches, driven by an autosave timer, which does not run deterministically in
TableProUITests. The behaviour is covered at the round-trip boundary instead, which is where both causes live.