fix: reload table after archive/unarchive rows#938
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR introduces a utility function to safely recalculate pagination page numbers after archiving or unarchiving list items, then applies it consistently across nine different page and tab components. The change prevents users from being left on invalid empty pages after removing items from the current view. ChangesSafe Pagination After Archive/Unarchive
Sequence DiagramsequenceDiagram
participant User
participant ListComponent
participant ArchiveAPI
participant getSafePageAfterRemove
participant ReloadAPI
User->>ListComponent: Click archive/unarchive item
ListComponent->>ArchiveAPI: archiveItem() or unarchiveItem()
ArchiveAPI-->>ListComponent: Promise resolves
ListComponent->>getSafePageAfterRemove: Calculate safe page from (totalCount-1, perPage, currentPage)
getSafePageAfterRemove-->>ListComponent: safePage (clamped to valid range)
ListComponent->>ReloadAPI: fetchItems(page = safePage)
ReloadAPI-->>ListComponent: Updated list
ListComponent-->>User: Display updated list on valid page
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
| const totalPages = Math.ceil(totalAfter / perPage); | ||
| const lastValidPage = Math.max(1, totalPages); | ||
| return Math.min(currentPage, lastValidPage); | ||
| }; |
There was a problem hiding this comment.
@tomrndom
Bug: safe-page logic fires unconditionally for both archive and unarchive, but the item only leaves the current view in one of the four direction × filter combinations.
showArchived |
Action | Item leaves view? | Decrement correct? |
|---|---|---|---|
false (default) |
Archive | ✅ Yes — archived items are hidden | ✅ Yes |
true |
Archive | ❌ No — still visible (grayed out) | ❌ No |
true |
Unarchive | ❌ No — still visible (un-grayed) | ❌ No |
Concrete example of the bug:
showArchived=true, totalCount=21, perPage=10, currentPage=3 (1 item on page 3).
User unarchives that item → getSafePageAfterRemove(21, 10, 3) returns 2 → user gets bounced to page 2, but the item is still visible (unarchiving never removes it from a showArchived=true list). Page 3 is not empty.
Suggested fix: move the "does this item leave the view?" decision inside this function, so the 10 call sites stay simple:
export const getSafePageAfterRemove = (
totalCount,
perPage,
currentPage,
isItemCurrentlyArchived, // item.is_archived — state BEFORE the action
showArchived // current filter value
) => {
// Item only disappears from view when it is being archived (isItemCurrentlyArchived=false)
// and the list is currently hiding archived items (showArchived=false).
const itemLeavesView = !isItemCurrentlyArchived && !showArchived;
if (!itemLeavesView) return currentPage;
const totalAfter = totalCount - 1;
const totalPages = Math.ceil(totalAfter / perPage);
const lastValidPage = Math.max(1, totalPages);
return Math.min(currentPage, lastValidPage);
};Then every call site just adds the two extra args — item.is_archived is already in scope at each handler since the item.is_archived ? unarchive() : archive() branch is right above the .then():
const safePage = getSafePageAfterRemove(
totalCount, perPage, currentPage, item.is_archived, showArchived
);One exception to verify: the
customizedPageshandler insponsor-forms-tab/index.jsdoes not receive anitemargument — confirm whatis_archivedvalue applies there, or whethershowArchivedgoverns that sub-list independently.
| (item.is_archived | ||
| ? unarchiveFormTemplate(item) | ||
| : archiveFormTemplate(item) | ||
| ).then(() => { |
There was a problem hiding this comment.
[MEDIUM] Missing .catch() — unhandled rejection on archive/unarchive failure
All 10 archive handlers in this PR follow the same pattern:
archiveAction(item.id).then(() => {
const safePage = ...;
getItems(...);
});
// no .catch()If the API call fails, the action's own authErrorHandler fires a snackbar — that part is fine. But the bare rejected promise propagates up with no handler, producing an unhandled promise rejection warning. More subtly, there is no explicit guarantee that the follow-up getItems reload is skipped on failure — that is only implicitly guaranteed because .then() is not called on rejection.
Suggested fix — add a no-op catch to all 10 handlers:
archiveAction(item.id)
.then(() => {
const safePage = getSafePageAfterRemove(...); getItems(term, safePage, perPage, order, orderDir, showArchived); })
.catch(() => {
// API error already handled by authErrorHandler; reload current page to keep list consistent
getItems(term, currentPage, perPage, order, orderDir, showArchived); }); ```
Reloading on failure in the catch also ensures the list stays in sync if the server rejected the action silently (e.g., a 403 the error handler swallowed).
|
[HIGH]
What the reducers do today: All case FORM_TEMPLATE_ARCHIVED: {
const updatedFormTemplates = state.formTemplates.map((item) =>
item.id === updatedFormTemplate.id ? { ...item, is_archived: true } : item
);
return { ...state, formTemplates: updatedFormTemplates };
// ↑ totalFormTemplates is untouched
}Compare with case FORM_TEMPLATE_DELETED:
return { ...state, formTemplates: [...], totalFormTemplates: state.totalFormTemplates - 1 };Why this matters for this PR:
Under normal single-click usage this is masked by the follow-up Correct fix: decrement case FORM_TEMPLATE_ARCHIVED: {
const updatedFormTemplates = state.formTemplates.map((item) =>
item.id === updatedFormTemplate.id ? { ...item, is_archived: true } : item
);
return {
...state,
formTemplates: updatedFormTemplates,
totalFormTemplates: state.totalFormTemplates - 1 // ← add this
};
}This applies to all domains: This finding is secondary to the direction×filter bug already commented on |
2 participants
ref: https://app.clickup.com/t/86b7v230m
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit