Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughNew unit test added for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
priscila-moneo
requested review from
martinquiroga-exo,
santipalenque and
smarcet
![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
🧹 Nitpick comments (2)
src/pages/sponsors/__tests__/sponsor-list-page.test.js (2)
53-60: Comment on Line 56 is misleading.
DEFAULT_CURRENT_PAGEis passed by the component regardless of the seededcurrentPage: 1— the fact that they happen to both equal1hides a potential bug where the effect ignores the preservedcurrentPage. Consider seedingcurrentPageto a non-default value (e.g.2) to make the test's intent explicit: the effect deliberately resets toDEFAULT_CURRENT_PAGEon mount. Or, if the desired behavior is to preserve the page too, the assertion (and the production code) should reflect that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/__tests__/sponsor-list-page.test.js` around lines 53 - 60, The test is misleading because it passes DEFAULT_CURRENT_PAGE implicitly (both are 1), masking whether the component effect resets or preserves currentPage; update the test for clarity by seeding the mocked preserved state with a non-default currentPage (e.g., currentPage: 2) and then assert the intended behavior: if the effect should reset, assert getSponsors was called with DEFAULT_CURRENT_PAGE, otherwise update the expectation to use the preserved currentPage value; reference the test helpers/state setup where currentPage is seeded and the getSponsors expectation in sponsor-list-page.test.js to change the seeded value or the expected argument accordingly.
49-51: Weak negative assertion.
queryByText("OTHER")passes trivially since no sponsor with that name was ever seeded into state — the assertion doesn't validate any filtering logic (and the component doesn't filter client-side anyway; filtering happens server-side viagetSponsors). Consider either removing this line, or seeding a second sponsor and verifying that the list only renders what the reducer provides, to avoid a misleading "only FNTECH is visible" comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/__tests__/sponsor-list-page.test.js` around lines 49 - 51, The negative assertion using screen.queryByText("OTHER") is misleading because no "OTHER" sponsor was ever seeded; update the test in sponsor-list-page.test.js to either remove that queryByText line or better: seed a second sponsor (e.g., an "OTHER" fixture) into the mocked getSponsors/reducer state and assert that only the reducer-provided sponsors are rendered (use screen.getByText("FNTECH") and expect(screen.queryByText("OTHER")).not.toBeInTheDocument()). Locate the seeding/mocking logic around the test setup (where getSponsors or the sponsors reducer is mocked) and add the second sponsor there so the negative assertion actually verifies filtering/rendering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/sponsors/sponsor-list-page.js`:
- Around line 63-67: The effect currently depends on currentSummit, term,
perPage, order, and orderDir which causes duplicate/conflicting fetches because
handlers like handleSearchDebounced, handlePerPageChange, and handleSort call
getSponsors and then Redux updates re-trigger the effect; change the effect to
only run on mount/summit change (e.g., the useEffect with useEffect(() => { if
(currentSummit) getSponsors(term, DEFAULT_CURRENT_PAGE, perPage, order,
orderDir); }, [currentSummit]) or add a firstLoad/summitChanged boolean gate) so
that handlers (handleSearchDebounced, handlePerPageChange, handleSort) remain
the single source of subsequent fetches and you avoid duplicate/contradictory
requests involving DEFAULT_CURRENT_PAGE and currentPage.
---
Nitpick comments:
In `@src/pages/sponsors/__tests__/sponsor-list-page.test.js`:
- Around line 53-60: The test is misleading because it passes
DEFAULT_CURRENT_PAGE implicitly (both are 1), masking whether the component
effect resets or preserves currentPage; update the test for clarity by seeding
the mocked preserved state with a non-default currentPage (e.g., currentPage: 2)
and then assert the intended behavior: if the effect should reset, assert
getSponsors was called with DEFAULT_CURRENT_PAGE, otherwise update the
expectation to use the preserved currentPage value; reference the test
helpers/state setup where currentPage is seeded and the getSponsors expectation
in sponsor-list-page.test.js to change the seeded value or the expected argument
accordingly.
- Around line 49-51: The negative assertion using screen.queryByText("OTHER") is
misleading because no "OTHER" sponsor was ever seeded; update the test in
sponsor-list-page.test.js to either remove that queryByText line or better: seed
a second sponsor (e.g., an "OTHER" fixture) into the mocked getSponsors/reducer
state and assert that only the reducer-provided sponsors are rendered (use
screen.getByText("FNTECH") and
expect(screen.queryByText("OTHER")).not.toBeInTheDocument()). Locate the
seeding/mocking logic around the test setup (where getSponsors or the sponsors
reducer is mocked) and add the second sponsor there so the negative assertion
actually verifies filtering/rendering behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56060623-e6b9-4e01-9a03-382a56c77a31
📒 Files selected for processing (2)
src/pages/sponsors/__tests__/sponsor-list-page.test.jssrc/pages/sponsors/sponsor-list-page.js
| useEffect(() => { | ||
| if (currentSummit) { | ||
| getSponsors(); | ||
| getSponsors(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir); | ||
| } | ||
| }, [currentSummit]); | ||
| }, [currentSummit, term, perPage, order, orderDir]); |
There was a problem hiding this comment.
Risk of duplicate/conflicting fetches from overlapping effect + handler calls.
Because term, perPage, order, and orderDir are now effect dependencies, and the REQUEST_SPONSORS reducer writes these back into Redux props (see src/reducers/sponsors/sponsor-list-reducer.js), every handler that calls getSponsors will also re-trigger this effect, causing a second fetch:
handleSearchDebounced(Line 71) callsgetSponsors(term, 1, perPage, order, orderDir)→ Redux updatesterm→ effect re-runs with the same args (duplicate request).handlePerPageChange(Line 97) → effect re-fires (duplicate request with same args).handleSort(Line 100) callsgetSponsors(term, currentPage, perPage, key, dir), but the effect then re-fires withDEFAULT_CURRENT_PAGE, clobbering the user's page and issuing a conflicting request.
Consider scoping the effect to only the mount/summit-change case (e.g., keep only currentSummit as a dependency, or gate on a "first load / summit changed" flag) and let the handlers remain the single source of truth for subsequent fetches. That preserves the original bug fix (seeding the list with the preserved term on mount) without introducing redundant/racing requests.
Proposed scoping of the effect
useEffect(() => {
if (currentSummit) {
getSponsors(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir);
}
- }, [currentSummit, term, perPage, order, orderDir]);
+ }, [currentSummit?.id]);📝 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.
| useEffect(() => { | |
| if (currentSummit) { | |
| getSponsors(); | |
| getSponsors(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir); | |
| } | |
| }, [currentSummit]); | |
| }, [currentSummit, term, perPage, order, orderDir]); | |
| useEffect(() => { | |
| if (currentSummit) { | |
| getSponsors(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir); | |
| } | |
| }, [currentSummit?.id]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors/sponsor-list-page.js` around lines 63 - 67, The effect
currently depends on currentSummit, term, perPage, order, and orderDir which
causes duplicate/conflicting fetches because handlers like
handleSearchDebounced, handlePerPageChange, and handleSort call getSponsors and
then Redux updates re-trigger the effect; change the effect to only run on
mount/summit change (e.g., the useEffect with useEffect(() => { if
(currentSummit) getSponsors(term, DEFAULT_CURRENT_PAGE, perPage, order,
orderDir); }, [currentSummit]) or add a firstLoad/summitChanged boolean gate) so
that handlers (handleSearchDebounced, handlePerPageChange, handleSort) remain
the single source of subsequent fetches and you avoid duplicate/contradictory
requests involving DEFAULT_CURRENT_PAGE and currentPage.
| useEffect(() => { | ||
| if (currentSummit) { | ||
| getSponsors(); | ||
| getSponsors(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir); |
There was a problem hiding this comment.
we should also preserve currentPage
priscila-moneo
force-pushed
the
fix/search-criteria-not-preserved
branch
from
d9c4979 to
a639fe3
Compare
priscila-moneo
force-pushed
the
fix/search-criteria-not-preserved
branch
from
a639fe3 to
3c921ae
Compare
2 participants
ref: https://app.clickup.com/t/86b9hhg65
Summary by CodeRabbit
Bug Fixes
Tests