fix: reload table after archive/unarchive rows#938
Open
tomrndom wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds getSafePageAfterRemove and uses it in archive/unarchive handlers across multiple list components to compute a pagination-safe page after a remove-like mutation; reducers and tests were updated so totals decrement when items are archived/unarchived. ChangesSafe Pagination After Archive/Unarchive
Sequence DiagramsequenceDiagram
participant User
participant ListComponent
participant ArchiveAPI
participant Methods_getSafePageAfterRemove
participant ReloadAPI
User->>ListComponent: Click archive/unarchive item
ListComponent->>ArchiveAPI: archiveItem() / unarchiveItem()
ArchiveAPI-->>ListComponent: Promise resolves
ListComponent->>Methods_getSafePageAfterRemove: getSafePageAfterRemove(totalCount, perPage, currentPage)
Methods_getSafePageAfterRemove-->>ListComponent: safePage
ListComponent->>ReloadAPI: fetchList(page=safePage, perPage, sort, filters)
ReloadAPI-->>ListComponent: Updated list + totals
ListComponent-->>User: Render updated page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
smarcet
reviewed
May 15, 2026
| 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.
Author
There was a problem hiding this comment.
This is not necesary since the current table and filter only display all the archived or unarchived rows, so every time a row is archived/unarchived is gonna be removed from the grid
smarcet
requested changes
May 15, 2026
smarcet
reviewed
May 15, 2026
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.js (1)
311-311: ⚡ Quick winAdd a regression test for no-op archive/unarchive count behavior.
Good update on expected decrements; please also add a case where the target item is missing or already in the target state and assert
totalCountdoes not change.Also applies to: 380-380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.js` at line 311, Add regression tests to assert totalCount is unchanged when archive/unarchive is a no-op: in the sponsor-form-items-list-reducer test suite add two cases calling the reducer (via the same action creators used elsewhere in the file) where the target item is either missing or already in the target state (already archived when archiving, already unarchived when unarchiving) and assert that sponsorFormItemsListReducer returns state with the same totalCount value as the input; reuse the existing test helpers/setup and the same action types used around the existing totalCount decrement tests so the new cases mirror those scenarios but expect no change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/reducers/sponsors_inventory/form-template-item-list-reducer.js`:
- Around line 109-113: The reducer is unconditionally decrementing
totalFormTemplateItems when returning the new state, which causes undercounts on
no-op archive/unarchive actions; update the logic in the form-template-item-list
reducer (the code that builds updatedFormTemplatesItems and sets
totalFormTemplateItems) to detect whether the target item's archived state
actually changed (compare the previous item's archived flag to the new value on
the item in updatedFormTemplatesItems) and only decrement/increment
totalFormTemplateItems when a real transition occurred, leaving
totalFormTemplateItems unchanged for no-op/replayed actions.
In `@src/reducers/sponsors_inventory/form-template-list-reducer.js`:
- Around line 108-112: The reducer currently decrements totalFormTemplates
unconditionally when returning the new state; change it to only decrement when
the template was actually removed from the current set by comparing lengths (or
checking existence) of updatedFormTemplates versus state.formTemplates or by
verifying the removed id from action.payload existed; update the return so
totalFormTemplates = state.totalFormTemplates - 1 only if removal occurred,
otherwise leave totalFormTemplates unchanged (apply the same guard for both
occurrences where totalFormTemplates is decremented).
In `@src/reducers/sponsors_inventory/inventory-item-list-reducer.js`:
- Around line 204-208: The reducer currently unconditionally decrements
totalInventoryItems by 1 (at the return that spreads state and sets
inventoryItems: updatedInventoryItems and totalInventoryItems:
state.totalInventoryItems - 1), which can desync counts on no-ops or duplicate
actions; change it to compute the actual change (e.g., const delta =
state.inventoryItems.length - updatedInventoryItems.length or check whether the
removed item was found) and set totalInventoryItems: Math.max(0,
state.totalInventoryItems - delta) or directly set totalInventoryItems:
updatedInventoryItems.length; apply the same conditional update logic to the
other similar return (lines around the second occurrence using
updatedInventoryItems) so totals only change when inventoryItems length actually
changes.
In `@src/reducers/sponsors_inventory/page-template-list-reducer.js`:
- Around line 112-116: The reducer is unconditionally decrementing
totalPageTemplates which undercounts when no row actually changed; instead
compute how many items were removed/changed by comparing state.pageTemplates and
updatedPageTemplates (e.g., removedCount = state.pageTemplates.length -
updatedPageTemplates.length or detect the specific id change) and subtract only
that removedCount from totalPageTemplates (or leave it unchanged if removedCount
is 0); update the return that currently sets totalPageTemplates:
state.totalPageTemplates - 1 to use the computed removedCount (and apply the
same guard in the other similar block that updates totalPageTemplates).
In `@src/reducers/sponsors/show-pages-list-reducer.js`:
- Around line 116-117: The reducer updates totalCount by subtracting 1 which can
go negative; change the assignment(s) that set totalCount (the object returned
in the reducer around the lines updating totalCount, e.g., where totalCount:
state.totalCount - 1 is used) to clamp the result to a minimum of 0 (use
Math.max(0, state.totalCount - 1) or equivalent) and apply the same fix to the
other occurrence mentioned (the second totalCount update at lines ~127-128).
In `@src/reducers/sponsors/sponsor-page-pages-list-reducer.js`:
- Around line 243-244: When removing a page you currently always decrement
state.customizedPages.totalItems; change the reducer logic so totalItems is only
decremented when a page was actually removed. Concretely, compute the new pages
array (e.g., newPages = pages.filter(...)) and compare lengths (or check a found
flag) — if newPages.length < pages.length then set totalItems =
state.customizedPages.totalItems - 1 (clamped to >= 0); otherwise leave
totalItems unchanged. Apply this change to both places that update pages and
totalItems (the blocks that build pages and set totalItems currently using
pages: [...pages], totalItems: state.customizedPages.totalItems - 1) so
duplicate/out-of-order actions do not drift the count.
---
Nitpick comments:
In `@src/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.js`:
- Line 311: Add regression tests to assert totalCount is unchanged when
archive/unarchive is a no-op: in the sponsor-form-items-list-reducer test suite
add two cases calling the reducer (via the same action creators used elsewhere
in the file) where the target item is either missing or already in the target
state (already archived when archiving, already unarchived when unarchiving) and
assert that sponsorFormItemsListReducer returns state with the same totalCount
value as the input; reuse the existing test helpers/setup and the same action
types used around the existing totalCount decrement tests so the new cases
mirror those scenarios but expect no change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 761db084-fe43-4aab-b9a6-d7df7025054a
📒 Files selected for processing (21)
src/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/form-templates/form-template-list-page.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/pages/sponsors-global/page-templates/page-template-list-page.jssrc/pages/sponsors/show-pages-list-page/index.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-forms-list-page/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.jssrc/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.jssrc/reducers/sponsors/show-pages-list-reducer.jssrc/reducers/sponsors/sponsor-customized-form-items-list-reducer.jssrc/reducers/sponsors/sponsor-form-items-list-reducer.jssrc/reducers/sponsors/sponsor-forms-list-reducer.jssrc/reducers/sponsors/sponsor-page-forms-list-reducer.jssrc/reducers/sponsors/sponsor-page-pages-list-reducer.jssrc/reducers/sponsors_inventory/form-template-item-list-reducer.jssrc/reducers/sponsors_inventory/form-template-list-reducer.jssrc/reducers/sponsors_inventory/inventory-item-list-reducer.jssrc/reducers/sponsors_inventory/page-template-list-reducer.js
🚧 Files skipped from review as they are similar to previous changes (7)
- src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/index.js
- src/pages/sponsors/sponsor-form-item-list-page/index.js
- src/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.js
- src/pages/sponsors-global/form-templates/form-template-item-list-page.js
- src/pages/sponsors/show-pages-list-page/index.js
- src/pages/sponsors/sponsor-forms-list-page/index.js
- src/pages/sponsors-global/page-templates/page-template-list-page.js
Comment on lines
+109
to
+113
| return { | ||
| ...state, | ||
| formTemplateItems: updatedFormTemplatesItems, | ||
| totalFormTemplateItems: state.totalFormTemplateItems - 1 | ||
| }; |
There was a problem hiding this comment.
Prevent totalFormTemplateItems undercount on no-op archive/unarchive.
Line 112 and Line 126 should only decrement when the target item actually transitions state; otherwise totals can drift after duplicate/replayed actions.
Proposed fix
case FORM_TEMPLATE_ITEM_ARCHIVED: {
const updatedFormTemplateItem = payload.response;
+ const shouldDecrement = state.formTemplateItems.some(
+ (item) =>
+ item.id === updatedFormTemplateItem.id && item.is_archived !== true
+ );
const updatedFormTemplatesItems = state.formTemplateItems.map((item) =>
item.id === updatedFormTemplateItem.id
? { ...item, is_archived: true }
: item
);
return {
...state,
formTemplateItems: updatedFormTemplatesItems,
- totalFormTemplateItems: state.totalFormTemplateItems - 1
+ totalFormTemplateItems: shouldDecrement
+ ? Math.max(0, state.totalFormTemplateItems - 1)
+ : state.totalFormTemplateItems
};
}
case FORM_TEMPLATE_ITEM_UNARCHIVED: {
const updatedFormTemplateItemId = payload;
+ const shouldDecrement = state.formTemplateItems.some(
+ (item) => item.id === updatedFormTemplateItemId && item.is_archived !== false
+ );
const updatedFormTemplatesItems = state.formTemplateItems.map((item) =>
item.id === updatedFormTemplateItemId
? { ...item, is_archived: false }
: item
);
return {
...state,
formTemplateItems: updatedFormTemplatesItems,
- totalFormTemplateItems: state.totalFormTemplateItems - 1
+ totalFormTemplateItems: shouldDecrement
+ ? Math.max(0, state.totalFormTemplateItems - 1)
+ : state.totalFormTemplateItems
};
}Also applies to: 123-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/reducers/sponsors_inventory/form-template-item-list-reducer.js` around
lines 109 - 113, The reducer is unconditionally decrementing
totalFormTemplateItems when returning the new state, which causes undercounts on
no-op archive/unarchive actions; update the logic in the form-template-item-list
reducer (the code that builds updatedFormTemplatesItems and sets
totalFormTemplateItems) to detect whether the target item's archived state
actually changed (compare the previous item's archived flag to the new value on
the item in updatedFormTemplatesItems) and only decrement/increment
totalFormTemplateItems when a real transition occurred, leaving
totalFormTemplateItems unchanged for no-op/replayed actions.
Comment on lines
+108
to
+112
| return { | ||
| ...state, | ||
| formTemplates: updatedFormTemplates, | ||
| totalFormTemplates: state.totalFormTemplates - 1 | ||
| }; |
There was a problem hiding this comment.
Decrement totalFormTemplates only when a template actually leaves the current set.
Line 111 and Line 125 currently decrement unconditionally; a no-op action can skew totals and safe-page outcomes.
Proposed fix
case FORM_TEMPLATE_ARCHIVED: {
const updatedFormTemplate = payload.response;
+ const shouldDecrement = state.formTemplates.some(
+ (item) => item.id === updatedFormTemplate.id && item.is_archived !== true
+ );
const updatedFormTemplates = state.formTemplates.map((item) =>
item.id === updatedFormTemplate.id
? { ...item, is_archived: true }
: item
);
return {
...state,
formTemplates: updatedFormTemplates,
- totalFormTemplates: state.totalFormTemplates - 1
+ totalFormTemplates: shouldDecrement
+ ? Math.max(0, state.totalFormTemplates - 1)
+ : state.totalFormTemplates
};
}
case FORM_TEMPLATE_UNARCHIVED: {
const updatedFormTemplateId = payload;
+ const shouldDecrement = state.formTemplates.some(
+ (item) => item.id === updatedFormTemplateId && item.is_archived !== false
+ );
const updatedFormTemplates = state.formTemplates.map((item) =>
item.id === updatedFormTemplateId
? { ...item, is_archived: false }
: item
);
return {
...state,
formTemplates: updatedFormTemplates,
- totalFormTemplates: state.totalFormTemplates - 1
+ totalFormTemplates: shouldDecrement
+ ? Math.max(0, state.totalFormTemplates - 1)
+ : state.totalFormTemplates
};
}Also applies to: 122-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/reducers/sponsors_inventory/form-template-list-reducer.js` around lines
108 - 112, The reducer currently decrements totalFormTemplates unconditionally
when returning the new state; change it to only decrement when the template was
actually removed from the current set by comparing lengths (or checking
existence) of updatedFormTemplates versus state.formTemplates or by verifying
the removed id from action.payload existed; update the return so
totalFormTemplates = state.totalFormTemplates - 1 only if removal occurred,
otherwise leave totalFormTemplates unchanged (apply the same guard for both
occurrences where totalFormTemplates is decremented).
Comment on lines
+204
to
+208
| return { | ||
| ...state, | ||
| inventoryItems: updatedInventoryItems, | ||
| totalInventoryItems: state.totalInventoryItems - 1 | ||
| }; |
There was a problem hiding this comment.
totalInventoryItems update should be transition-aware, not unconditional.
On Line 207 and Line 219, unconditional - 1 can desync totals when the action is a no-op or duplicated.
Proposed fix
case INVENTORY_ITEM_ARCHIVED: {
const updatedItem = payload.response;
+ const shouldDecrement = state.inventoryItems.some(
+ (item) => item.id === updatedItem.id && item.is_archived !== true
+ );
const updatedInventoryItems = state.inventoryItems.map((item) =>
item.id === updatedItem.id ? { ...item, is_archived: true } : item
);
return {
...state,
inventoryItems: updatedInventoryItems,
- totalInventoryItems: state.totalInventoryItems - 1
+ totalInventoryItems: shouldDecrement
+ ? Math.max(0, state.totalInventoryItems - 1)
+ : state.totalInventoryItems
};
}
case INVENTORY_ITEM_UNARCHIVED: {
const updatedItemId = payload;
+ const shouldDecrement = state.inventoryItems.some(
+ (item) => item.id === updatedItemId && item.is_archived !== false
+ );
const updatedInventoryItems = state.inventoryItems.map((item) =>
item.id === updatedItemId ? { ...item, is_archived: false } : item
);
return {
...state,
inventoryItems: updatedInventoryItems,
- totalInventoryItems: state.totalInventoryItems - 1
+ totalInventoryItems: shouldDecrement
+ ? Math.max(0, state.totalInventoryItems - 1)
+ : state.totalInventoryItems
};
}Also applies to: 216-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/reducers/sponsors_inventory/inventory-item-list-reducer.js` around lines
204 - 208, The reducer currently unconditionally decrements totalInventoryItems
by 1 (at the return that spreads state and sets inventoryItems:
updatedInventoryItems and totalInventoryItems: state.totalInventoryItems - 1),
which can desync counts on no-ops or duplicate actions; change it to compute the
actual change (e.g., const delta = state.inventoryItems.length -
updatedInventoryItems.length or check whether the removed item was found) and
set totalInventoryItems: Math.max(0, state.totalInventoryItems - delta) or
directly set totalInventoryItems: updatedInventoryItems.length; apply the same
conditional update logic to the other similar return (lines around the second
occurrence using updatedInventoryItems) so totals only change when
inventoryItems length actually changes.
Comment on lines
+112
to
+116
| return { | ||
| ...state, | ||
| pageTemplates: updatedPageTemplates, | ||
| totalPageTemplates: state.totalPageTemplates - 1 | ||
| }; |
There was a problem hiding this comment.
Avoid unconditional decrement of totalPageTemplates.
Line 115 and Line 127 can undercount when no row actually transitioned state, which can reintroduce pagination errors.
Proposed fix
case PAGE_TEMPLATE_ARCHIVED: {
const updatedFormTemplate = payload.response;
+ const shouldDecrement = state.pageTemplates.some(
+ (item) => item.id === updatedFormTemplate.id && item.is_archived !== true
+ );
const updatedPageTemplates = state.pageTemplates.map((item) =>
item.id === updatedFormTemplate.id
? { ...item, is_archived: true }
: item
);
return {
...state,
pageTemplates: updatedPageTemplates,
- totalPageTemplates: state.totalPageTemplates - 1
+ totalPageTemplates: shouldDecrement
+ ? Math.max(0, state.totalPageTemplates - 1)
+ : state.totalPageTemplates
};
}
case PAGE_TEMPLATE_UNARCHIVED: {
const { pageTemplateId } = payload;
+ const shouldDecrement = state.pageTemplates.some(
+ (item) => item.id === pageTemplateId && item.is_archived !== false
+ );
const updatedPageTemplates = state.pageTemplates.map((item) =>
item.id === pageTemplateId ? { ...item, is_archived: false } : item
);
return {
...state,
pageTemplates: updatedPageTemplates,
- totalPageTemplates: state.totalPageTemplates - 1
+ totalPageTemplates: shouldDecrement
+ ? Math.max(0, state.totalPageTemplates - 1)
+ : state.totalPageTemplates
};
}Also applies to: 124-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/reducers/sponsors_inventory/page-template-list-reducer.js` around lines
112 - 116, The reducer is unconditionally decrementing totalPageTemplates which
undercounts when no row actually changed; instead compute how many items were
removed/changed by comparing state.pageTemplates and updatedPageTemplates (e.g.,
removedCount = state.pageTemplates.length - updatedPageTemplates.length or
detect the specific id change) and subtract only that removedCount from
totalPageTemplates (or leave it unchanged if removedCount is 0); update the
return that currently sets totalPageTemplates: state.totalPageTemplates - 1 to
use the computed removedCount (and apply the same guard in the other similar
block that updates totalPageTemplates).
Comment on lines
+116
to
117
| totalCount: state.totalCount - 1 | ||
| }; |
There was a problem hiding this comment.
Clamp totalCount to avoid negative values.
state.totalCount - 1 can go below 0 under repeated/stale archive events, which can break pagination math downstream. Please guard with a floor.
Suggested fix
- totalCount: state.totalCount - 1
+ totalCount: Math.max(0, state.totalCount - 1)
...
- totalCount: state.totalCount - 1
+ totalCount: Math.max(0, state.totalCount - 1)Also applies to: 127-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/reducers/sponsors/show-pages-list-reducer.js` around lines 116 - 117, The
reducer updates totalCount by subtracting 1 which can go negative; change the
assignment(s) that set totalCount (the object returned in the reducer around the
lines updating totalCount, e.g., where totalCount: state.totalCount - 1 is used)
to clamp the result to a minimum of 0 (use Math.max(0, state.totalCount - 1) or
equivalent) and apply the same fix to the other occurrence mentioned (the second
totalCount update at lines ~127-128).
Comment on lines
+243
to
+244
| pages: [...pages], | ||
| totalItems: state.customizedPages.totalItems - 1 |
There was a problem hiding this comment.
Guard totalItems decrement to actual state transitions only.
On Line 244 and Line 258, totalItems is decremented even when no row actually changed (e.g., duplicate/out-of-order action), which can drift counts and break pagination math.
Proposed fix
case SPONSOR_CUSTOMIZED_PAGE_ARCHIVED: {
const { pageId } = payload;
+ const shouldDecrement = state.customizedPages.pages.some(
+ (page) => page.id === pageId && page.is_archived !== true
+ );
const pages = state.customizedPages.pages.map((page) =>
page.id === pageId ? { ...page, is_archived: true } : page
);
return {
...state,
customizedPages: {
...state.customizedPages,
pages: [...pages],
- totalItems: state.customizedPages.totalItems - 1
+ totalItems: shouldDecrement
+ ? Math.max(0, state.customizedPages.totalItems - 1)
+ : state.customizedPages.totalItems
}
};
}
case SPONSOR_CUSTOMIZED_PAGE_UNARCHIVED: {
const { pageId } = payload;
+ const shouldDecrement = state.customizedPages.pages.some(
+ (page) => page.id === pageId && page.is_archived !== false
+ );
const pages = state.customizedPages.pages.map((page) =>
page.id === pageId ? { ...page, is_archived: false } : page
);
return {
...state,
customizedPages: {
...state.customizedPages,
pages: [...pages],
- totalItems: state.customizedPages.totalItems - 1
+ totalItems: shouldDecrement
+ ? Math.max(0, state.customizedPages.totalItems - 1)
+ : state.customizedPages.totalItems
}
};
}Also applies to: 257-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/reducers/sponsors/sponsor-page-pages-list-reducer.js` around lines 243 -
244, When removing a page you currently always decrement
state.customizedPages.totalItems; change the reducer logic so totalItems is only
decremented when a page was actually removed. Concretely, compute the new pages
array (e.g., newPages = pages.filter(...)) and compare lengths (or check a found
flag) — if newPages.length < pages.length then set totalItems =
state.customizedPages.totalItems - 1 (clamped to >= 0); otherwise leave
totalItems unchanged. Apply this change to both places that update pages and
totalItems (the blocks that build pages and set totalItems currently using
pages: [...pages], totalItems: state.customizedPages.totalItems - 1) so
duplicate/out-of-order actions do not drift the count.
smarcet
reviewed
May 20, 2026
smarcet
requested changes
May 20, 2026
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@tomrndom please review comment
#938 (comment)
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
fix/reload-table-on-archive
branch
from
cd7b812 to
b8545c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/reducers/sponsors/sponsor-page-forms-list-reducer.js (1)
234-252:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid unconditional decrement in
SPONSOR_CUSTOMIZED_FORM_ARCHIVED_CHANGED.Line 251 should decrement only if the targeted form actually changes
is_archived.Proposed fix
case SPONSOR_CUSTOMIZED_FORM_ARCHIVED_CHANGED: { const { formId, isArchived } = payload; + const shouldDecrement = state.customizedForms.forms.some( + (form) => form.id === formId && form.is_archived !== isArchived + ); const forms = state.customizedForms.forms.map((form) => { if (form.id === formId) { return { ...form, is_archived: isArchived }; } return form; }); return { ...state, customizedForms: { ...state.customizedForms, forms, - totalCount: state.customizedForms.totalCount - 1 + totalCount: shouldDecrement + ? Math.max(0, state.customizedForms.totalCount - 1) + : state.customizedForms.totalCount } }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/sponsors/sponsor-page-forms-list-reducer.js` around lines 234 - 252, The reducer handling SPONSOR_CUSTOMIZED_FORM_ARCHIVED_CHANGED unconditionally subtracts 1 from state.customizedForms.totalCount; change it to first detect whether the matched form's is_archived actually changes by locating the form with form.id === formId (use the existing map or a prior find), compare its current is_archived to payload.isArchived, and only decrement totalCount when the value changed and payload.isArchived is true (leave totalCount unchanged otherwise); update the return to use this conditional delta when constructing customizedForms.totalCount.src/reducers/sponsors/sponsor-form-items-list-reducer.js (1)
118-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent
totalCountdrift on no-op archive/unarchive actions.Line 125 and Line 134 always decrement. If the item is absent or already in target state, pagination totals become inconsistent.
Proposed fix
case SPONSOR_FORM_ITEM_ARCHIVED: { const { id: itemId } = payload.response; + const shouldDecrement = state.items.some( + (item) => item.id === itemId && item.is_archived !== true + ); const items = state.items.map((item) => item.id === itemId ? { ...item, is_archived: true } : item ); - return { ...state, items, totalCount: state.totalCount - 1 }; + return { + ...state, + items, + totalCount: shouldDecrement + ? Math.max(0, state.totalCount - 1) + : state.totalCount + }; } case SPONSOR_FORM_ITEM_UNARCHIVED: { const { itemId } = payload; + const shouldDecrement = state.items.some( + (item) => item.id === itemId && item.is_archived !== false + ); const items = state.items.map((item) => item.id === itemId ? { ...item, is_archived: false } : item ); - return { ...state, items, totalCount: state.totalCount - 1 }; + return { + ...state, + items, + totalCount: shouldDecrement + ? Math.max(0, state.totalCount - 1) + : state.totalCount + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/sponsors/sponsor-form-items-list-reducer.js` around lines 118 - 135, In the SPONSOR_FORM_ITEM_ARCHIVED and SPONSOR_FORM_ITEM_UNARCHIVED reducer cases, avoid blindly changing totalCount — determine whether an item was actually found and its archived state changed before adjusting totals: for SPONSOR_FORM_ITEM_ARCHIVED (use payload.response.id) map state.items and detect if an item with that id existed and its is_archived transitioned from false to true, then decrement totalCount by 1 only in that case; for SPONSOR_FORM_ITEM_UNARCHIVED (use payload.itemId) map state.items and detect if an item existed and its is_archived transitioned from true to false, then increment totalCount by 1 only in that case; if no item is found or no state change occurred leave totalCount unchanged.src/reducers/sponsors/sponsor-forms-list-reducer.js (1)
151-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
totalCountupdates conditional on actual state change.Line 158 and Line 167 should only decrement when the matched form actually transitions archive state.
Proposed fix
case SPONSOR_FORM_ARCHIVED: { const { id: formId } = payload.response; + const shouldDecrement = state.sponsorForms.some( + (item) => item.id === formId && item.is_archived !== true + ); const sponsorForms = state.sponsorForms.map((item) => item.id === formId ? { ...item, is_archived: true } : item ); - return { ...state, sponsorForms, totalCount: state.totalCount - 1 }; + return { + ...state, + sponsorForms, + totalCount: shouldDecrement + ? Math.max(0, state.totalCount - 1) + : state.totalCount + }; } case SPONSOR_FORM_UNARCHIVED: { const { formId } = payload; + const shouldDecrement = state.sponsorForms.some( + (item) => item.id === formId && item.is_archived !== false + ); const sponsorForms = state.sponsorForms.map((item) => item.id === formId ? { ...item, is_archived: false } : item ); - return { ...state, sponsorForms, totalCount: state.totalCount - 1 }; + return { + ...state, + sponsorForms, + totalCount: shouldDecrement + ? Math.max(0, state.totalCount - 1) + : state.totalCount + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/sponsors/sponsor-forms-list-reducer.js` around lines 151 - 168, The reducer currently always adjusts totalCount when handling SPONSOR_FORM_ARCHIVED and SPONSOR_FORM_UNARCHIVED; change both branches to only mutate totalCount when the matched form actually changes archive state: for SPONSOR_FORM_ARCHIVED (use payload.response.id as formId) find the item in state.sponsorForms, if item.exists and item.is_archived is false then set is_archived: true and decrement totalCount by 1, otherwise leave totalCount unchanged; for SPONSOR_FORM_UNARCHIVED (use payload.formId) similarly only set is_archived: false and increment totalCount by 1 if the item was previously archived, otherwise keep totalCount as-is; return the updated sponsorForms array and the conditionally updated totalCount.src/reducers/sponsors/sponsor-customized-form-items-list-reducer.js (1)
123-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
totalCountdecrement to real archive-state transitions only.Line 130 and Line 139 decrement unconditionally. If the target row is missing/already in target state,
totalCountdrifts.Proposed fix
case SPONSOR_CUSTOMIZED_FORM_ITEM_ARCHIVED: { const { id: itemId } = payload.response; + const shouldDecrement = state.items.some( + (item) => item.id === itemId && item.is_archived !== true + ); const items = state.items.map((item) => item.id === itemId ? { ...item, is_archived: true } : item ); - return { ...state, items, totalCount: state.totalCount - 1 }; + return { + ...state, + items, + totalCount: shouldDecrement + ? Math.max(0, state.totalCount - 1) + : state.totalCount + }; } case SPONSOR_CUSTOMIZED_FORM_ITEM_UNARCHIVED: { const { itemId } = payload; + const shouldDecrement = state.items.some( + (item) => item.id === itemId && item.is_archived !== false + ); const items = state.items.map((item) => item.id === itemId ? { ...item, is_archived: false } : item ); - return { ...state, items, totalCount: state.totalCount - 1 }; + return { + ...state, + items, + totalCount: shouldDecrement + ? Math.max(0, state.totalCount - 1) + : state.totalCount + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/sponsors/sponsor-customized-form-items-list-reducer.js` around lines 123 - 140, The SPONSOR_CUSTOMIZED_FORM_ITEM_ARCHIVED and SPONSOR_CUSTOMIZED_FORM_ITEM_UNARCHIVED cases unconditionally change totalCount; instead, first locate the target item from state.items (use payload.response.id for ARCHIVED and payload.itemId for UNARCHIVED), check its current is_archived flag, and only modify totalCount when the flag actually flips: for ARCHIVED, set is_archived true and decrement totalCount by 1 only if the item existed and was previously false; for UNARCHIVED, set is_archived false and increment totalCount by 1 only if the item existed and was previously true; return updated items and the conditionally adjusted totalCount.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js`:
- Around line 154-185: The handler handleArchiveItem currently uses
getSafePageAfterRemove for both archive and unarchive flows; change it so
getSafePageAfterRemove(...) is used only when performing a removal
(archiveSponsorCustomizedFormItem) and for the unarchive path call
getSponsorCustomizedFormItems with the existing currentPage (no safe-page
adjustment), keeping the catch behavior consistent by reloading the appropriate
page for the attempted action; update calls around
archiveSponsorCustomizedFormItem(formId, item.id) and
unarchiveSponsorCustomizedFormItem(formId, item.id) to branch the then() logic
so the page argument passed to getSponsorCustomizedFormItems is safePage only
for archive and currentPage for unarchive.
---
Outside diff comments:
In `@src/reducers/sponsors/sponsor-customized-form-items-list-reducer.js`:
- Around line 123-140: The SPONSOR_CUSTOMIZED_FORM_ITEM_ARCHIVED and
SPONSOR_CUSTOMIZED_FORM_ITEM_UNARCHIVED cases unconditionally change totalCount;
instead, first locate the target item from state.items (use payload.response.id
for ARCHIVED and payload.itemId for UNARCHIVED), check its current is_archived
flag, and only modify totalCount when the flag actually flips: for ARCHIVED, set
is_archived true and decrement totalCount by 1 only if the item existed and was
previously false; for UNARCHIVED, set is_archived false and increment totalCount
by 1 only if the item existed and was previously true; return updated items and
the conditionally adjusted totalCount.
In `@src/reducers/sponsors/sponsor-form-items-list-reducer.js`:
- Around line 118-135: In the SPONSOR_FORM_ITEM_ARCHIVED and
SPONSOR_FORM_ITEM_UNARCHIVED reducer cases, avoid blindly changing totalCount —
determine whether an item was actually found and its archived state changed
before adjusting totals: for SPONSOR_FORM_ITEM_ARCHIVED (use
payload.response.id) map state.items and detect if an item with that id existed
and its is_archived transitioned from false to true, then decrement totalCount
by 1 only in that case; for SPONSOR_FORM_ITEM_UNARCHIVED (use payload.itemId)
map state.items and detect if an item existed and its is_archived transitioned
from true to false, then increment totalCount by 1 only in that case; if no item
is found or no state change occurred leave totalCount unchanged.
In `@src/reducers/sponsors/sponsor-forms-list-reducer.js`:
- Around line 151-168: The reducer currently always adjusts totalCount when
handling SPONSOR_FORM_ARCHIVED and SPONSOR_FORM_UNARCHIVED; change both branches
to only mutate totalCount when the matched form actually changes archive state:
for SPONSOR_FORM_ARCHIVED (use payload.response.id as formId) find the item in
state.sponsorForms, if item.exists and item.is_archived is false then set
is_archived: true and decrement totalCount by 1, otherwise leave totalCount
unchanged; for SPONSOR_FORM_UNARCHIVED (use payload.formId) similarly only set
is_archived: false and increment totalCount by 1 if the item was previously
archived, otherwise keep totalCount as-is; return the updated sponsorForms array
and the conditionally updated totalCount.
In `@src/reducers/sponsors/sponsor-page-forms-list-reducer.js`:
- Around line 234-252: The reducer handling
SPONSOR_CUSTOMIZED_FORM_ARCHIVED_CHANGED unconditionally subtracts 1 from
state.customizedForms.totalCount; change it to first detect whether the matched
form's is_archived actually changes by locating the form with form.id === formId
(use the existing map or a prior find), compare its current is_archived to
payload.isArchived, and only decrement totalCount when the value changed and
payload.isArchived is true (leave totalCount unchanged otherwise); update the
return to use this conditional delta when constructing
customizedForms.totalCount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5ea9f81-7c69-43d0-b4fc-710abd128e57
📒 Files selected for processing (23)
src/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/form-templates/form-template-list-page.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/pages/sponsors-global/page-templates/page-template-list-page.jssrc/pages/sponsors/show-pages-list-page/index.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-forms-list-page/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.jssrc/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.jssrc/reducers/sponsors/show-pages-list-reducer.jssrc/reducers/sponsors/sponsor-customized-form-items-list-reducer.jssrc/reducers/sponsors/sponsor-form-items-list-reducer.jssrc/reducers/sponsors/sponsor-forms-list-reducer.jssrc/reducers/sponsors/sponsor-page-forms-list-reducer.jssrc/reducers/sponsors/sponsor-page-pages-list-reducer.jssrc/reducers/sponsors_inventory/form-template-item-list-reducer.jssrc/reducers/sponsors_inventory/form-template-list-reducer.jssrc/reducers/sponsors_inventory/inventory-item-list-reducer.jssrc/reducers/sponsors_inventory/page-template-list-reducer.jssrc/utils/__tests__/methods.test.jssrc/utils/methods.js
Comment on lines
154
to
+185
| const handleArchiveItem = (item) => | ||
| item.is_archived | ||
| (item.is_archived | ||
| ? unarchiveSponsorCustomizedFormItem(formId, item.id) | ||
| : archiveSponsorCustomizedFormItem(formId, item.id); | ||
| : archiveSponsorCustomizedFormItem(formId, item.id) | ||
| ) | ||
| .then(() => { | ||
| const safePage = getSafePageAfterRemove( | ||
| totalCount, | ||
| perPage, | ||
| currentPage | ||
| ); | ||
| getSponsorCustomizedFormItems( | ||
| formId, | ||
| term, | ||
| safePage, | ||
| perPage, | ||
| order, | ||
| orderDir, | ||
| showArchived | ||
| ); | ||
| }) | ||
| .catch(() => | ||
| getSponsorCustomizedFormItems( | ||
| formId, | ||
| term, | ||
| currentPage, | ||
| perPage, | ||
| order, | ||
| orderDir, | ||
| showArchived | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Safe-page logic is applied to both directions, but removal is filter-dependent.
getSafePageAfterRemove is remove-specific, yet this handler uses it for both archive and unarchive paths unconditionally. That can still produce wrong page selection for one direction depending on showArchived filtering (the direction×filter case called out in the PR discussion).
Suggested adjustment
const handleArchiveItem = (item) =>
(item.is_archived
? unarchiveSponsorCustomizedFormItem(formId, item.id)
: archiveSponsorCustomizedFormItem(formId, item.id)
)
.then(() => {
- const safePage = getSafePageAfterRemove(
- totalCount,
- perPage,
- currentPage
- );
+ const isRemoveLikeMutation = !showArchived; // adjust based on actual filter semantics
+ const safePage = isRemoveLikeMutation
+ ? getSafePageAfterRemove(totalCount, perPage, currentPage)
+ : currentPage;
getSponsorCustomizedFormItems(
formId,
term,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js`
around lines 154 - 185, The handler handleArchiveItem currently uses
getSafePageAfterRemove for both archive and unarchive flows; change it so
getSafePageAfterRemove(...) is used only when performing a removal
(archiveSponsorCustomizedFormItem) and for the unarchive path call
getSponsorCustomizedFormItems with the existing currentPage (no safe-page
adjustment), keeping the catch behavior consistent by reloading the appropriate
page for the attempted action; update calls around
archiveSponsorCustomizedFormItem(formId, item.id) and
unarchiveSponsorCustomizedFormItem(formId, item.id) to branch the then() logic
so the page argument passed to getSponsorCustomizedFormItems is safePage only
for archive and currentPage for unarchive.
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
2 participants
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.
ref: https://app.clickup.com/t/86b7v230m
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests