NAS-143108 / 27.0.0-BETA.1 / Keep the snapshot list's page and selection across reloads and page turns

[AlexKarpov98]

Note

This is not the fix for the bug as reported — that one is #14056, against stable/26. On master the line that snapped the page back went with the tn-table migration, and the user confirmed master behaves correctly. What follows is a set of defects found while chasing it that are still live on master. Splitting these onto their own ticket is fine by me if you'd rather; opening it here to avoid losing the work.

What this PR fixes

Two live regressions on master, both in Datasets → Snapshots:

1. A background reload throws the user back to page 1. getSnapshots() re-ran the search filter on every store emission, and setFilter unconditionally reset pagination — so a periodic snapshot task firing was enough to lose the user's place mid-task. setFilter now takes keepPage, and the reload path passes it; a query the user actually typed still starts at page 1.

2. Selecting rows across pages doesn't work. The ticket's expected result asks for "selections from both pages retained". The pre-migration UI held a selected flag on the row objects, so it did; tn-table clears its selection on any dataSource change, so the component was explicitly clearing on every emission and the selection died on a page turn or a reload. tn-table now takes [selectionKey], snapshots key on name, and the explicit clear is gone.

Each fix opens a hole that this PR also closes, so neither lands half-done:

3. Keyed selection can outlive the snapshot. Retaining a selection across reloads means a snapshot selected on one page and destroyed elsewhere stays selected — the batch toolbar would offer to delete a snapshot that no longer exists. The selection is pruned against the store's list wherever that list is rebuilt, plus on the live selectionChange path. doBatchDelete also moves to clearSelection(); selection.clear() only drops the rows on screen and would let off-screen picks come back on the next reconcile.

4. keepPage can hold a page that no longer exists. Skipping the reset means a reload that shrinks the list below the current page would slice past the end. setFilter clamps pageNumber to the last page with rows before slicing.

Not fixed here

The pager echo-guard bug — pushToProvider() handed the provider the same object it kept as lastPushedPagination, so a provider that re-paginates in place mutated the pager's own guard and the label sat on "51 – 100" over page 1's rows — is a library fix, shipped in 0.7.5. It's consumed here by the pin bump, not present in this diff.

Library dependency

Shipped in @truenas/ui-components 0.7.5; the pin here moves ~0.7.4 → ~0.7.5 (master, now merged in, had already taken it to ~0.7.4).

• tn-table gains selectionKey — selection tracked by key, surviving dataSource changes, with selectionChange carrying rows from pages that aren't on screen. Select-all and isAllSelected stay scoped to the visible page, so the header never claims rows the user can't see. New clearSelection() also drops the retained rows; plain selection.clear() would let them come back on the next reconcile.
• tn-table-pager pushes a copy to the provider.
• Specs for both, plus an argType and a SelectionAcrossPages story. Library suite green (4528), lint clean.

Changed here

File	Why
base-data-provider.ts	setFilter(filter, { keepPage }) + clampPaginationToLastPage()
async-data-provider.ts	forwards the new options through its override
snapshot-list.component.{ts,html}	[selectionKey], setSnapshots() as the single writer, clearSelection(), keepPage on the reload path
package.json / yarn.lock	pin bump

keepPage defaults to false, so the other ~30 setFilter call sites are untouched.

Review round

The clamp, and a correction. The review asked for the clamp on the grounds that this template renders <tn-table-pager> inside the @else of the empty-state check, so an empty page would unmount the pager and strand the user. That isn't reachable: the 0.7.5 pager self-corrects in syncFromProvider when currentPage() > totalPages(), synchronously inside the currentPage$ subscription, before change detection can unmount it. The clamp stays anyway — it belongs next to the flag, it makes keepPage correct for a provider with no pager bound, and it removes the transient empty emission. The setFilter doc comment, which had credited the pager with the correction, was updated to match.

The prune moved off selectionChange. It previously ran only there, which made correctness depend on tn-table emitting for a row it never rendered — a snapshot selected on a page the user has since left, then destroyed elsewhere, changes nothing about the visible selection. It now runs in setSnapshots() as well, so the invariant holds whoever emits.

snapshots has one writer. setSnapshots() writes the list, the snapshotNames index and the pruned selection together, so the index can't drift. Done without converting snapshots to a signal — it's read from buildSearchFilter and from the specs, and none of that needed to churn.

Testing

Three paging tests plus two rewritten selection tests on the component, and three new ArrayDataProvider tests covering keepPage, clamp-to-last-page and clamp-when-empty. The clamp is tested at the provider rather than through the component: the pager corrects itself too, so a component-level assertion passes either way and wouldn't catch a regression. The off-screen prune test was verified to fail with the fix reverted.

16 snapshot-list specs and 9 array-data-provider specs pass, lint is clean, and yarn build:prod exits 0 against the released pin — the [selectionKey] binding now type-checks under AOT.

Verified on hardware

Against a real appliance with 120 snapshots on one dataset (page size 50), instrumenting the live ArrayDataProvider:

Check	Result
Tick a checkbox on page 2	stays 51 – 100 of 120, row unmoved, batch toolbar appears
Cross-page selection	pick on page 2 → page 1 → pick there → back to page 2: first pick still checked
Background reload	snapshot created externally, total 120 → 121, pager stays 51 – 100, selection intact
Off-screen prune	on page 3, both selected (off-screen) rows destroyed externally → selection empties, toolbar clears, page held at 3
Clamp	on page 3, list shrunk 112 → 82 so page 3 stops existing → lands on 51 – 82 of 82, 32 rows, no empty state, pager still mounted

keepPage: true was logged on every reload-driven setFilter, and the page held in 5/5 controlled trials (single delete, three rapid deletes, one and two selected off-screen deletes, and the clamp run).

One false alarm worth recording: the pager was once seen jumping 101 – 121 → 1 – 50. That is setSorting's page reset, reproducible by clicking any column header (101 – 112 → 1 – 50 of 112), and is pre-existing intended behaviour this PR does not touch — not the reload path.

Known adjacent issue, not touched here

bootenv-list, container-list, docker-images-list and installed-apps-list call table.selection.clear(), which doesn't reset the table's selection count — the header checkbox can be left stuck indeterminate until the next reload corrects it. clearSelection() is the right call there now, but it's out of scope for this PR.

https://claude.ai/code/session_01MnsWWxBSkRM6QrgbK74fmG

[bugclerk]

Jira URL: https://ixsystems.atlassian.net/browse/NAS-143108

[github-actions]

Five findings: five LOW. Nothing here blocks the merge. 🎉

The core of this is genuinely well done: keepPage is opt-in with a false default so the ~30 other setFilter call sites are untouched, the clamp runs before setRows so the pager syncs against the page the rows were actually sliced for, and choosing the dataset branch up front instead of applying-and-undoing removes both the spurious currentPage$ emission and the clamp-against-a-discarded-result that was the reported symptom on /datasets/snapshots/tank. Making snapshots private so the compiler holds the single-writer invariant, rather than a comment, is the right instinct — and the hardware verification table is the kind of evidence that makes this reviewable at all. 👏 The jobs-list side is a net deletion: expansionKey retires lastRows/awaitingTableReset exactly as their own TODO asked.

What's left is all small.

The one worth a second look is hasSnapshotsInDataset — it re-implements filterTableRows's exact comparison instead of deriving it, and nothing in the suite fails if the two ever drift. Calling filterTableRows(datasetFilter) directly and branching on its length keeps the single setFilter call and the up-front choice while leaving one definition of "matches this dataset"; suggestion inline.

On the selection prune: selectedSnapshots is always correct thanks to the double prune, but tn-table's own keyed set never hears about it, so it keeps the dropped names — mostly cosmetic, except that a snapshot recreated under the same name comes back pre-selected.

In jobs-list, the !expandedRow early return no longer resets lastSyncedExpandedId, which master did on that path. It only bites when something navigates to ?jobId= for a job on another page while a row is open, but the recovery afterwards clears the parameter instead of honouring it.

The remaining two are cosmetic: the clampPaginationToLastPage comment credits the pager with holding a reference to pagination, which is the opposite of the 0.7.5 copy-on-push fix this PR consumes; and { keepPage?: boolean } is spelled out inline in three signatures where an exported alias would do. That last one overlaps the still-open thread on async-data-provider.ts:33.

One housekeeping note, not a finding: the description still says the pin moves ~0.7.4 → ~0.7.5 and its "Changed here" table doesn't mention jobs-list.component.{ts,html} — the diff is ~0.7.5 → ~0.7.6 and does touch the jobs list. Worth a refresh so the next reader isn't hunting selectionKey in the wrong release.

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.80%. Comparing base (c24d76c) to head (3016022).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/app/pages/jobs/jobs-list.component.ts	80.00%	2 Missing ⚠️
...snapshots/snapshot-list/snapshot-list.component.ts	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14057      +/-   ##
==========================================
+ Coverage   88.76%   88.80%   +0.03%     
==========================================
  Files        1894     1902       +8     
  Lines       71463    71630     +167     
  Branches     9331     9344      +13     
==========================================
+ Hits        63434    63608     +174     
+ Misses       8029     8022       -7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aervin]

works as described

[bugclerk]

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.