Conversation
📝 WalkthroughWalkthroughThis PR introduces GitHub Actions CI/CD workflows, refactors pagination and search result mapping logic from app.js into a new helpers module, establishes testing infrastructure with Jest and Babel configuration, and adds comprehensive unit tests for the new helpers module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
app/app.js (1)
102-117:⚠️ Potential issue | 🟠 MajorReset the requested page before issuing a fresh search.
doSearch(that, true)still uses the previous page on Line 104 and only resets pagination on Line 117 after the response. If the user starts a new query from page> 1, the request fetches the wrong slice while the UI shows page 1 / results 1-10.Proposed fix
function doSearch(that, reset) { var term = that.get('term'); - var page = that.get('page'); + var page = reset ? 1 : that.get('page'); var perPage = that.get('perPage'); var url = 'https://'+that.baseUrl+'/api/public/v1/search/'+that.context;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app.js` around lines 102 - 117, The search function doSearch currently builds the request URL using the old page value then calls helpers.resetPagination(that) after the AJAX completes; move/reset the page before composing the URL when reset is true so the request uses page 1. Specifically, in doSearch(where term,page,perPage,url are read) check the reset flag and either set that.set('page', 1) or override the local page variable to 1 before constructing url and performing $.ajax; keep helpers.resetPagination(that) for updating UI state after the response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/helpers.js`:
- Around line 22-39: In resetPagination, explicitly handle the total === 0 case:
at the top of the function (inside resetPagination) check if total === 0 and
then set pagesToShow to an empty array, set page to 0 (or another sentinel for
“no page”), and set fromResult and toResult both to 0, then return; otherwise
continue the existing logic that computes totalPages, lastPage, newPagesToShow,
etc.; update references to pagesToShow, page, fromResult, and toResult
accordingly so the UI never shows an impossible "1-0 of 0" state.
---
Outside diff comments:
In `@app/app.js`:
- Around line 102-117: The search function doSearch currently builds the request
URL using the old page value then calls helpers.resetPagination(that) after the
AJAX completes; move/reset the page before composing the URL when reset is true
so the request uses page 1. Specifically, in doSearch(where
term,page,perPage,url are read) check the reset flag and either set
that.set('page', 1) or override the local page variable to 1 before constructing
url and performing $.ajax; keep helpers.resetPagination(that) for updating UI
state after the response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff28e8cc-8f1b-4b7c-9a19-45ac9b5eb99a
⛔ Files ignored due to path filters (2)
embed.min.jsis excluded by!**/*.min.jsyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/tests.ymlapp/app.jsapp/helpers.jsbabel.config.jsjest.config.jspackage.jsontests/helpers.test.js
Comment on lines
+22
to
+39
| function resetPagination(that) { | ||
| var total = that.get('total'); | ||
| var perPage = that.get('perPage'); | ||
| var totalPages = Math.ceil(total / perPage); | ||
| var lastPage = (totalPages < 5) ? totalPages : 5; | ||
| var newPagesToShow = []; | ||
|
|
||
| for (var i = 1; i <= lastPage; i++) { | ||
| newPagesToShow.push(i); | ||
| } | ||
|
|
||
| that.set('page', 1); | ||
| that.set('pagesToShow', newPagesToShow); | ||
|
|
||
| var newResultLimit = perPage; | ||
| newResultLimit = (newResultLimit > total) ? total : newResultLimit; | ||
| that.set('fromResult', 1); | ||
| that.set('toResult', newResultLimit); |
There was a problem hiding this comment.
Handle the zero-results case explicitly.
When total is 0, Line 38 sets fromResult to 1 while Line 39 sets toResult to 0, and pagesToShow is left empty. That produces an impossible 1-0 of 0 state for empty searches.
Proposed fix
function resetPagination(that) {
var total = that.get('total');
var perPage = that.get('perPage');
+
+ if (total === 0) {
+ that.set('page', 1);
+ that.set('pagesToShow', []);
+ that.set('fromResult', 0);
+ that.set('toResult', 0);
+ return;
+ }
+
var totalPages = Math.ceil(total / perPage);
var lastPage = (totalPages < 5) ? totalPages : 5;
var newPagesToShow = [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function resetPagination(that) { | |
| var total = that.get('total'); | |
| var perPage = that.get('perPage'); | |
| var totalPages = Math.ceil(total / perPage); | |
| var lastPage = (totalPages < 5) ? totalPages : 5; | |
| var newPagesToShow = []; | |
| for (var i = 1; i <= lastPage; i++) { | |
| newPagesToShow.push(i); | |
| } | |
| that.set('page', 1); | |
| that.set('pagesToShow', newPagesToShow); | |
| var newResultLimit = perPage; | |
| newResultLimit = (newResultLimit > total) ? total : newResultLimit; | |
| that.set('fromResult', 1); | |
| that.set('toResult', newResultLimit); | |
| function resetPagination(that) { | |
| var total = that.get('total'); | |
| var perPage = that.get('perPage'); | |
| if (total === 0) { | |
| that.set('page', 1); | |
| that.set('pagesToShow', []); | |
| that.set('fromResult', 0); | |
| that.set('toResult', 0); | |
| return; | |
| } | |
| var totalPages = Math.ceil(total / perPage); | |
| var lastPage = (totalPages < 5) ? totalPages : 5; | |
| var newPagesToShow = []; | |
| for (var i = 1; i <= lastPage; i++) { | |
| newPagesToShow.push(i); | |
| } | |
| that.set('page', 1); | |
| that.set('pagesToShow', newPagesToShow); | |
| var newResultLimit = perPage; | |
| newResultLimit = (newResultLimit > total) ? total : newResultLimit; | |
| that.set('fromResult', 1); | |
| that.set('toResult', newResultLimit); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/helpers.js` around lines 22 - 39, In resetPagination, explicitly handle
the total === 0 case: at the top of the function (inside resetPagination) check
if total === 0 and then set pagesToShow to an empty array, set page to 0 (or
another sentinel for “no page”), and set fromResult and toResult both to 0, then
return; otherwise continue the existing logic that computes totalPages,
lastPage, newPagesToShow, etc.; update references to pagesToShow, page,
fromResult, and toResult accordingly so the UI never shows an impossible "1-0 of
0" state.
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
1 participant
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.
Summary by CodeRabbit
Chores
Tests