Repository navigation
fix: recover project pagination after deletion - #3771
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: recover project pagination after deletion
Nice, well-scoped fix. The clamping logic is correct across all the cases I traced, the guards (page.value <= 1, Math.max(1, …)) make it one-directional and idempotent (no infinite loop), and the four new tests cover the key scenarios (empty-page recovery, stepping back, keeping a populated page, and the empty state), asserting both the route p param and the resulting fetch pageIndex. The fix is also correctly scoped to projects.vue only — sibling pages (likes/followers/following) have no deletable items, so they don't need it.
No blocking issues and no security concerns. A few optional polish suggestions are inline below.
Summary of non-inline notes
- (Optional) Test gap — multi-page clamp: Every recovery test drops
totalby exactly one, so the clamp target happens to equalpage - 1. A case where, e.g.,p=5must clamp directly top=2would lock in the intended "jump to the new last page" behavior and guard against a future regression to a naivepage--.
| } | ||
| }) | ||
|
|
||
| watch([() => queryRet.data.value, pageSize], ([data]) => { |
There was a problem hiding this comment.
The pageSize watch source is effectively redundant and slightly misleading. pageSize already feeds listParams, so any pageSize change triggers a refetch which updates queryRet.data.value and re-runs this watch anyway. It's also misleading because the callback destructures only [data] — the new pageSize value is never read from the args; the body reads pageSize.value directly. Consider dropping pageSize from the sources (relying on the data change), or add a brief comment explaining why it's kept.
Separately, a one-line comment on this block would help a lot: the intent (clamp back to the new last page after a deletion shrinks total) is only recorded in the PR title, and it's non-obvious that this clamps to lastPage rather than page - 1 — in the tested cases those coincide, which could tempt a future reader to "simplify" it to page.value--.
| watch([() => queryRet.data.value, pageSize], ([data]) => { | ||
| if (data == null || page.value <= 1) return | ||
|
|
||
| const lastPage = Math.max(1, Math.ceil(data.total / pageSize.value)) |
There was a problem hiding this comment.
Minor DRY: this duplicates pageTotal (line 42, Math.ceil((…total ?? 0) / pageSize.value)) with the only difference being the Math.max(1, …) floor. You could derive one from the other, e.g. const lastPage = Math.max(1, pageTotal.value), which reads more clearly and keeps the two sites from drifting if page-size/total semantics ever change.
| if (data == null || page.value <= 1) return | ||
|
|
||
| const lastPage = Math.max(1, Math.ceil(data.total / pageSize.value)) | ||
| if (page.value > lastPage) page.value = lastPage |
There was a problem hiding this comment.
Low-priority / informational: deleting the last remaining item on a non-first page triggers two sequential list requests — @removed refetches at the stale pageIndex (returning an empty page + reduced total), then this watch sets page.value, which changes listParams and fires a second fetch for the corrected page (plus a brief empty-page flash in legacy query mode). It converges and is bounded, so it's acceptable given how rare the trigger is; if you want to avoid the wasted request you could clamp the page in the @removed handler before refetching. A short comment acknowledging the intentional refetch-then-clamp would help future readers either way.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3771 +/- ##
==========================================
+ Coverage 59.96% 60.01% +0.04%
==========================================
Files 772 772
Lines 27728 27732 +4
Branches 5231 5233 +2
==========================================
+ Hits 16627 16642 +15
+ Misses 9586 9582 -4
+ Partials 1515 1508 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Summary
When the last project on a later page was deleted, the projects list remained on the now-empty page and displayed “No projects” even though projects still existed on previous pages.
This change recalculates the last available page after a project deletion and navigates to it when necessary.
Changes
Testing
Fixes #3734