Move the pagination control into the AQL: Performance section - #163
Conversation
…ove the standalone component for pagination.
There was a problem hiding this comment.
🟡 Changes recommended
The moved pagination control currently isn’t correctly gated by allowedControls and is unintentionally coupled to enable_caching via the component-level early return.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves the “Disable pagination” UI from the main Advanced Query Settings panel into the “AQL: Performance Controls” panel, aligning pagination behavior with other performance-related toggles.
Changes:
- Removes the standalone
PaginationTogglecontrol fromsrc/variations/controls.js. - Inlines the pagination toggle into
PerformanceControls(ToolsPanel) alongside caching. - Deletes the now-unused
src/components/pagination-toggle.jscomponent.
File summaries
| File | Description |
|---|---|
| src/variations/controls.js | Stops rendering the standalone pagination toggle in the main inspector panel. |
| src/components/performance-controls.js | Adds pagination toggle to the performance ToolsPanel and reads disable_pagination from attributes. |
| src/components/pagination-toggle.js | Removes the standalone pagination toggle component. |
Review details
Suppressed comments (1)
src/components/performance-controls.js:82
- The pagination toggle was moved here, but unlike the previous
PaginationToggleit is no longer gated byallowedControls.includes( 'pagination' ). This can expose a disabled control in the UI and allow writingquery.disable_paginationeven whenaql_allowed_controlsremovespagination. It should be conditionally rendered (or otherwise prevented from mutating attributes) the same way other controls are gated.
<ToolsPanelItem
hasValue={ () => !! disablePagination }
label={ __( 'Pagination', 'advanced-query-loop' ) }
onDeselect={ () =>
setAttributes( {
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…rom the allowed list.
There was a problem hiding this comment.
🟡 Changes recommended
The current resetAll implementation can mutate disallowed query attributes and the pagination toggle may become hidden-by-default due to ToolsPanel behavior, creating a UX/test regression.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/components/performance-controls.js:88
- Moving pagination into
ToolsPanelItemlikely makes the “Disable pagination” toggle hidden by default (similar to Caching, which requires enabling via the panel options menu in existing e2e tests). This appears to change UX compared to the previous always-visiblePaginationToggleand may break the existing pagination e2e test that expects the checkbox to be visible immediately.
<ToolsPanelItem
hasValue={ () => !! disablePagination }
label={ __( 'Pagination', 'advanced-query-loop' ) }
onDeselect={ () =>
setAttributes( {
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: ryanwelcher <1259027+ryanwelcher@users.noreply.github.com>
The pagination control seemed in a weird spot. And it makes more sense to be inside of the performance section, so I've moved it there.