Skip to content

feat: Use Store API pagination for remodels table#5681

Merged
steverydz merged 1 commit intomainfrom
WD-36146-update-pagination-for-remodels-table
Apr 29, 2026
Merged

feat: Use Store API pagination for remodels table#5681
steverydz merged 1 commit intomainfrom
WD-36146-update-pagination-for-remodels-table

Conversation

@steverydz
Copy link
Copy Markdown
Contributor

@steverydz steverydz commented Apr 27, 2026

Done

Updated the remodels table to use the next-cursor pagination supplied by the Store API. This involved removing the ordering by date so as to retain the order returned from the API.

How to QA

Testing

  • This PR has tests
  • No testing required (explain why):

Security

  • Security considerations for review (list them):
    • Examples:
    • Access control: users can only access their own data
    • Input: user input is validated and sanitised
    • Sensitive data: secret or private data is not exposed in any way
    • ...
  • This PR has no security considerations (explain why): No user input

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-36146

Screenshots

n/a

UX Approval

  • This PR does not require UX approval
  • This PR does require UX approval (add context):

Copilot AI review requested due to automatic review settings April 27, 2026 13:48
@webteam-app
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the publisher “Remodels” view to use Store API–backed pagination (cursor + page-size) instead of paginating/filtering entirely client-side, aligning the UI with the API’s paging model.

Changes:

  • Pass pagination/filter query params through the Flask /models/remodel-allowlist endpoint to PublisherGW.get_remodel_allowlist.
  • Replace client-side TablePagination with explicit cursor-based pagination controls and a page-size selector in the React UI.
  • Update useRemodels to build a URL with query params and expand tests around these parameters; bump canonicalwebteam.store-api to 7.8.2.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
webapp/endpoints/models.py Forwards page, from-model, and page-size query params (mapping page → Store API cursor) to the Store API gateway call.
tests/endpoints/tests_models.py Adds endpoint tests asserting that pagination/filter query parameters are forwarded to the gateway call.
static/js/publisher/pages/Remodel/RemodelTable.tsx Switches to explicit Pagination + Select controls (instead of TablePagination) and adds truncation styling for table cells.
static/js/publisher/pages/Remodel/Remodel.tsx Introduces cursor state/history, reads page-size from URL search params, and wires pagination handlers into the table.
static/js/publisher/hooks/useRemodels.ts Builds request URL with optional page, page-size, from-model search params and expands the query key.
static/js/publisher/hooks/tests/useRemodels.test.tsx Adds tests intended to validate query-string behavior for useRemodels.
requirements.txt Bumps canonicalwebteam.store-api dependency version.

Comment thread static/js/publisher/hooks/useRemodels.ts
Comment thread static/js/publisher/hooks/__tests__/useRemodels.test.tsx Outdated
Comment thread webapp/endpoints/models.py Outdated
Comment thread tests/endpoints/tests_models.py
Comment thread static/js/publisher/pages/Remodel/Remodel.tsx Outdated
Comment thread static/js/publisher/pages/Remodel/Remodel.tsx Outdated
Comment thread static/js/publisher/hooks/useRemodels.ts
@bartaz
Copy link
Copy Markdown
Member

bartaz commented Apr 28, 2026

Page number (or the next cursor) doesn't seem to be added to URL, is this expected?
Seems a bit weird to have page size there, but not pointer to the page itself.

@steverydz
Copy link
Copy Markdown
Contributor Author

@bartaz I agree, however we can't reliably know the page number - for example if someone loaded the page with a cursor in the URL, we don't know what the previous cursor is and won't have generated a history unless they have navigated there from the first page. Not ideal I know, but for this iteration this is all we can really do. Having previous_cursor in the API response would solve this but right now we don't have this.

Copy link
Copy Markdown
Contributor

@edisile edisile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, left some minor suggestions

const brandId = useAtomValue(brandIdState);
const [currentCursor, setCurrentCursor] = useState<string | null>(null);
const [nextCursor, setNextCursor] = useState<string | null>(null);
const [cursorHistory, setCursorHistory] = useState<Array<string | null>>([]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about using a ref for cursorHistory? I think it would make the history updates clearer (just .push(...) on page forward and .pop() on page back)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edisile Good idea. Done

Comment on lines +81 to +115
<Row>
<Col size={6} medium={3}>
<Pagination
onForward={() => {
handlePageForward();
}}
onBack={() => {
handlePageBack();
}}
showLabels
forwardLabel="Next"
forwardDisabled={forwardDisabled}
backLabel="Prev"
backDisabled={backDisabled}
/>
</Col>
<Col size={6} medium={3} className="u-align--right">
<Select
label="Number of remodels per page"
labelClassName="u-off-screen"
defaultValue={pageSize}
style={{ maxWidth: "150px" }}
onChange={(e: ChangeEvent<HTMLSelectElement>) => {
setSearchParams({ "page-size": e.target.value });
}}
options={[
{ label: "10/page", value: 10 },
{ label: "25/page", value: 25 },
{ label: "50/page", value: 50 },
{ label: "100/page", value: 100 },
{ label: "200/page", value: 200 },
]}
/>
</Col>
</Row>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't the TablePagination component already implement this logic? or is it a bad fit for this kind of cursor-based pagination?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edisile You're right. I thought it might be a bit complicated, but along with using the ref for the history this makes more sense. Done

@steverydz steverydz force-pushed the WD-36146-update-pagination-for-remodels-table branch from 5e3ca17 to b3df19b Compare April 28, 2026 14:04
@steverydz
Copy link
Copy Markdown
Contributor Author

@edisile I've addressed your comments

@edisile
Copy link
Copy Markdown
Contributor

edisile commented Apr 28, 2026

@steverydz thanks! I took a look at the page again and I think there are some missing styles:
immagine
when comparing to the example linked in the comment I see some .pagination rules that are missing on our side

@steverydz
Copy link
Copy Markdown
Contributor Author

@edisile That's really strange? Those styles are there when I run locally, and other instances of pagination e.g. https://snapcraft-io-5681.demos.haus/admin/ahnuP3quahti9vis8aiw/signing-keys seem to be working? I'll investigate.

@edisile
Copy link
Copy Markdown
Contributor

edisile commented Apr 28, 2026

I think that visiting that page after you've visited one where the pagination loads correctly hides the issue because the styles are already loaded (e.g. go to the "Policies" tab, then come back to the "Remodels" and the pagination looks fine)

@steverydz
Copy link
Copy Markdown
Contributor Author

@edisile Makes sense, although that's still not the case locally as I've been going directly to the remodel page and the styles are there?

@steverydz steverydz force-pushed the WD-36146-update-pagination-for-remodels-table branch 2 times, most recently from e2e1bd5 to c5e94a4 Compare April 29, 2026 09:51
@steverydz
Copy link
Copy Markdown
Contributor Author

@edisile I've fixed it. The issue was that the TablePaginationControls component doesn't import the styles itself, but for some reason the styles are imported locally, probably because the TablePagination component is used elsewhere and the tree shaking removes it in the product build. That's my theory at least :)

@steverydz steverydz force-pushed the WD-36146-update-pagination-for-remodels-table branch from c5e94a4 to 5118213 Compare April 29, 2026 11:43
@steverydz steverydz merged commit 5194b4a into main Apr 29, 2026
14 checks passed
@steverydz steverydz deleted the WD-36146-update-pagination-for-remodels-table branch April 29, 2026 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants