feat: Add pagination in merchants page#1965
Conversation
📝 WalkthroughWalkthroughThis PR separates total merchant counts (unpaginated scopes) from paginated table results: the controller builds ChangesFamily and Provider Merchant Pagination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d72de03545
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/views/family_merchants/index.html.erb (2)
114-116: 💤 Low valueConsider conditionally rendering pagination controls.
The pagination controls render even when there's only one page of results. While functional, this may clutter the UI when pagination isn't needed.
🎨 Proposed refinement to hide pagination when not needed
- <div class="pt-4"> - <%= render "shared/pagination", pagy: `@pagy_provider_merchants` %> - </div> + <% if `@pagy_provider_merchants.pages` > 1 %> + <div class="pt-4"> + <%= render "shared/pagination", pagy: `@pagy_provider_merchants` %> + </div> + <% end %>🤖 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 `@app/views/family_merchants/index.html.erb` around lines 114 - 116, Wrap the pagination partial render in a conditional that only shows it when there is more than one page; specifically, change the block that calls render "shared/pagination", pagy: `@pagy_provider_merchants` inside an if `@pagy_provider_merchants.pages` > 1 (or equivalent check) so the "shared/pagination" partial is only rendered when `@pagy_provider_merchants.pages` > 1.
56-58: 💤 Low valueConsider conditionally rendering pagination controls.
The pagination controls render even when there's only one page of results. While functional, this may clutter the UI when pagination isn't needed.
🎨 Proposed refinement to hide pagination when not needed
- <div class="pt-4"> - <%= render "shared/pagination", pagy: `@pagy_family_merchants` %> - </div> + <% if `@pagy_family_merchants.pages` > 1 %> + <div class="pt-4"> + <%= render "shared/pagination", pagy: `@pagy_family_merchants` %> + </div> + <% end %>🤖 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 `@app/views/family_merchants/index.html.erb` around lines 56 - 58, Wrap the existing pagination render so it only shows when there are multiple pages: check the Pagy object `@pagy_family_merchants` (for example using `@pagy_family_merchants.pages` > 1) and only call render "shared/pagination", pagy: `@pagy_family_merchants` when that condition is true; this will keep the UI clean by hiding pagination controls for single-page results.
🤖 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.
Nitpick comments:
In `@app/views/family_merchants/index.html.erb`:
- Around line 114-116: Wrap the pagination partial render in a conditional that
only shows it when there is more than one page; specifically, change the block
that calls render "shared/pagination", pagy: `@pagy_provider_merchants` inside an
if `@pagy_provider_merchants.pages` > 1 (or equivalent check) so the
"shared/pagination" partial is only rendered when `@pagy_provider_merchants.pages`
> 1.
- Around line 56-58: Wrap the existing pagination render so it only shows when
there are multiple pages: check the Pagy object `@pagy_family_merchants` (for
example using `@pagy_family_merchants.pages` > 1) and only call render
"shared/pagination", pagy: `@pagy_family_merchants` when that condition is true;
this will keep the UI clean by hiding pagination controls for single-page
results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d78346a6-fc9d-4189-9cd7-3be804c3289e
📒 Files selected for processing (2)
app/controllers/family_merchants_controller.rbapp/views/family_merchants/index.html.erb
|
Pagination on a potentially long list is a good addition. A couple of things: Redundant visibility condition <% if @pagy_family_merchants.count > 0 || @family_merchants.any? %>
<% if @all_family_merchants.any? %>Same applies to the provider section. Pagination shown even when there's only one page The No tests for the pagination behavior The PR adds no controller tests asserting that the paginated variables are assigned correctly, that page 2 works, or that the total count is preserved. Given the project's convention of writing tests for critical paths, a basic test that Generated by Claude Code |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/views/family_merchants/index.html.erb (2)
114-116: ⚡ Quick winConsider conditionally rendering pagination controls.
Same concern as the family merchants pagination above: the pagination block renders even when there's only one page of provider merchants. Wrap it in a conditional:
<% if `@pagy_provider_merchants.pages` > 1 %> <div class="pt-4"> <%= render "shared/pagination", pagy: `@pagy_provider_merchants` %> </div> <% end %>🤖 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 `@app/views/family_merchants/index.html.erb` around lines 114 - 116, The pagination partial is rendered even when there's only one page; wrap the block that renders "shared/pagination" with a conditional check on the Pagy object (use `@pagy_provider_merchants.pages` > 1) so that the div and render call only execute when more than one page exists, keeping the reference to `@pagy_provider_merchants` and the render "shared/pagination" invocation.
56-58: ⚡ Quick winConsider conditionally rendering pagination controls.
The pagination block renders even when there's only one page of results. Since the
shared/paginationpartial doesn't guard itself (see context snippet 2), you may want to wrap this in a conditional to avoid showing unnecessary controls:<% if `@pagy_family_merchants.pages` > 1 %> <div class="pt-4"> <%= render "shared/pagination", pagy: `@pagy_family_merchants` %> </div> <% end %>This prevents extra DOM elements and visual clutter when all family merchants fit on a single page.
🤖 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 `@app/views/family_merchants/index.html.erb` around lines 56 - 58, Wrap the pagination render in a conditional that checks the Pagy object so controls are only shown when there is more than one page: guard the render of "shared/pagination" using `@pagy_family_merchants.pages` > 1 (or the equivalent Pagy helper) so the <div class="pt-4"> and the render call for the partial only appear when `@pagy_family_merchants` has multiple pages.
🤖 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.
Nitpick comments:
In `@app/views/family_merchants/index.html.erb`:
- Around line 114-116: The pagination partial is rendered even when there's only
one page; wrap the block that renders "shared/pagination" with a conditional
check on the Pagy object (use `@pagy_provider_merchants.pages` > 1) so that the
div and render call only execute when more than one page exists, keeping the
reference to `@pagy_provider_merchants` and the render "shared/pagination"
invocation.
- Around line 56-58: Wrap the pagination render in a conditional that checks the
Pagy object so controls are only shown when there is more than one page: guard
the render of "shared/pagination" using `@pagy_family_merchants.pages` > 1 (or the
equivalent Pagy helper) so the <div class="pt-4"> and the render call for the
partial only appear when `@pagy_family_merchants` has multiple pages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee920a10-9990-4d87-939d-5f9dadea484e
📒 Files selected for processing (1)
app/views/family_merchants/index.html.erb
This PR introduces a pagination feature in the “Merchants” settings page, applicable to both family and provider merchants.
Summary by CodeRabbit
New Features
Bug Fixes