Populate hit counts in JSON results format across load-more feature settings - #457
Populate hit counts in JSON results format across load-more feature settings#457matt-bernhardt wants to merge 5 commits into
Conversation
❌ 4 blocking issues (4 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
Coverage Report for CI Build 33206635348Coverage increased (+0.004%) to 98.304%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
Restores the pagination.hits attribute in the JSON /results response so external relevance tooling (Quepid) can continue to read total hit counts regardless of whether the load-more pagination feature flag is enabled.
Changes:
- Adds controller tests asserting that JSON responses include
pagination.hits. - Updates
SearchControllerJSON rendering to populatepaginationbased on the active pagination mode (legacy vs. load-more).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/controllers/search_controller_test.rb |
Adds assertions (and a new case) to ensure JSON responses include pagination.hits, including when load-more is enabled. |
app/controllers/search_controller.rb |
Switches JSON pagination payload to be derived from either @pagination or @load_more[:total_hits] via a helper method. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # This returns the total result count from whichever instance variable is present | ||
| def hit_count | ||
| return @pagination unless pagination_load_more_enabled? | ||
|
|
||
| { hits: @load_more[:total_hits] } | ||
| end |
There was a problem hiding this comment.
I see this suggestion, and am rejecting the specific change while trying to respond to the observation underlying it. I've refactored the hit_count method to explicitly only return the value we care about, in a way that is more robust to the conditions the data might take running through the method.
This is not the agent's suggested change, but it does respond to what I think the agent's criticism was - inconsistent shape in the response, and a lack of robustness to different data conditions. My response is to focus on the one value that Quepid needs - the number of hits - and strip out all the other parameters that could be found between the @Pagination and @load_more instance variables. The new hit_count method is also more robust to nulls by using the &.[]() syntax, and falls back to 0 if a value cannot be extracted.
| format.json { render json: { results: @results, hits: hit_count, errors: @errors } } | ||
| format.turbo_stream { render :results } | ||
| format.html { render :results } | ||
| end |
There was a problem hiding this comment.
jazairi
left a comment
jazairi
left a comment
There was a problem hiding this comment.
This looks good! Could we add a test catching the fallback to 0 you added in response to Copilot's review?
d0f8336 to
a58637b
Compare
|
Okay @jazairi - I've added a test to exercise the |
jazairi
left a comment
jazairi
left a comment
There was a problem hiding this comment.
I think this confirms the fallback behavior well. Thanks!
This restores an attribute in our search results - specific to the JSON format response - upon which our relevance platform depends, but which is going to go away with the next release of the application.
The change follows a TDD pattern of development: the first commit expands our test suite to reveal the problem (the loss of "hit counts" in the response, which will cause a problem with our Quepid platform). The second commit resolves the problem by restoring the value in the two conditions across which the application will likely operate.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing