fix: search & filter mismatch with pagination on Tracker pageagination#362
Open
rathan2511 wants to merge 1 commit into
Open
fix: search & filter mismatch with pagination on Tracker pageagination#362rathan2511 wants to merge 1 commit into
rathan2511 wants to merge 1 commit into
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughTracker.tsx refactored to move search and filter logic server-side. Filter parameters (search, repo, date range, state) are now passed to ChangesServer-Side Filtering Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @rathan2511 for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/pages/Tracker/Tracker.tsx`:
- Around line 94-104: handleSubmit currently calls setPage(0) and then
fetchData(...) directly, causing a duplicate fetch because the effect that
depends on page will run after the state update; remove the synchronous
fetchData call from handleSubmit and let the existing useEffect that watches
page (and the filter/search state) own fetching when page changes. Ensure the
effect's dependency array includes searchTitle, selectedRepo, startDate,
endDate, tab, issueFilter, prFilter and page so setting page to 0 triggers the
single, correct fetch.
- Around line 82-92: handleSubmit currently sets page to 0 and then calls
fetchData directly which causes duplicate requests because the useEffect (which
depends on page) will also call fetchData; remove the direct fetchData call from
handleSubmit and rely on setting page to 0 to trigger the existing useEffect
that calls fetchData (keep fetchData memoized via useCallback). In the useEffect
that calls fetchData, explicitly omit username and fetchData from the dependency
array by adding a brief eslint-disable comment for react-hooks/exhaustive-deps
and a comment explaining we only want manual/authed submits to trigger fetches;
do not add authError (it doesn’t exist) and continue to use dataError from
useGitHubData, and note that merged handling is already done inside
useGitHubData so no change is needed there.
🪄 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: cd85765a-853e-4443-9a9c-b2f98651079d
📒 Files selected for processing (1)
src/pages/Tracker/Tracker.tsx
Comment on lines
82
to
+92
| useEffect(() => { | ||
| if (username) { | ||
| fetchData(username, page + 1, ROWS_PER_PAGE); | ||
| fetchData(username, page + 1, ROWS_PER_PAGE, tab === 0 ? "issue" : "pr", { | ||
| search: searchTitle, | ||
| repo: selectedRepo, | ||
| startDate, | ||
| endDate, | ||
| state: tab === 0 ? issueFilter : prFilter, | ||
| }); | ||
| } | ||
| }, [tab, page]); | ||
| }, [tab, page, issueFilter, prFilter]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'useGitHubData' src/hooks
rg -nP -C5 '\b(fetchData|useGitHubData)\b' src/hooksRepository: GitMetricsLab/github_tracker
Length of output: 1662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant parts of Tracker.tsx around the reported lines and nearby logic.
sed -n '1,220p' src/pages/Tracker/Tracker.tsx | nl -ba | sed -n '60,130p'
sed -n '220,380p' src/pages/Tracker/Tracker.tsx | nl -ba
# Inspect hooks for error shapes and "merged" handling.
sed -n '1,240p' src/hooks/useGitHubData.ts | nl -ba | sed -n '1,220p'
sed -n '220,340p' src/hooks/useGitHubData.ts | nl -ba
sed -n '1,240p' src/hooks/useGitHubAuth.ts | nl -ba
# Search for how "merged" is used/translated across the repo.
rg -n --hidden --no-ignore-vcs '\bmerged\b' srcRepository: GitMetricsLab/github_tracker
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Tracker.tsx: print with line numbers using cat -n (nl missing in this sandbox)
cat -n src/pages/Tracker/Tracker.tsx | sed -n '60,170p'
cat -n src/pages/Tracker/Tracker.tsx | sed -n '170,340p'
# Hooks
cat -n src/hooks/useGitHubData.ts | sed -n '1,220p'
cat -n src/hooks/useGitHubData.ts | sed -n '220,310p'
cat -n src/hooks/useGitHubAuth.ts | sed -n '1,260p'
# Search for "merged" translation
rg -n --hidden --no-ignore-vcs '\bmerged\b' srcRepository: GitMetricsLab/github_tracker
Length of output: 19844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '\bauthError\b' src/pages/Tracker/Tracker.tsx
rg -n '\buseGitHubAuth\b' src/pages/Tracker/Tracker.tsxRepository: GitMetricsLab/github_tracker
Length of output: 54
Avoid duplicate network requests when applying filters + fix deps lint intent
handleSubmitsetspageto0and also callsfetchDatadirectly; when the currentpageis not0, theuseEffect(which depends onpage) fires and triggers the same fetch again → duplicate requests.useEffectusesusername(andfetchData) but they’re intentionally not in deps; sincefetchDatais already memoized viauseCallback, keep the current “auth changes require Submit” behavior by suppressingreact-hooks/exhaustive-depswith a brief comment (or include them only if you want auto-fetch on auth changes).- The
mergedstate is already handled inuseGitHubDataby translatingfilters.state === 'merged'tois:mergedfor PR searches—no change needed there. - No
authErrorexists inuseGitHubAuth;dataErrorfromuseGitHubDatais what’s displayed.
🤖 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 `@src/pages/Tracker/Tracker.tsx` around lines 82 - 92, handleSubmit currently
sets page to 0 and then calls fetchData directly which causes duplicate requests
because the useEffect (which depends on page) will also call fetchData; remove
the direct fetchData call from handleSubmit and rely on setting page to 0 to
trigger the existing useEffect that calls fetchData (keep fetchData memoized via
useCallback). In the useEffect that calls fetchData, explicitly omit username
and fetchData from the dependency array by adding a brief eslint-disable comment
for react-hooks/exhaustive-deps and a comment explaining we only want
manual/authed submits to trigger fetches; do not add authError (it doesn’t
exist) and continue to use dataError from useGitHubData, and note that merged
handling is already done inside useGitHubData so no change is needed there.
Comment on lines
94
to
104
| const handleSubmit = (e: React.FormEvent<HTMLFormElement>): void => { | ||
| e.preventDefault(); | ||
| setPage(0); | ||
| fetchData(username, 1, ROWS_PER_PAGE); | ||
| fetchData(username, 1, ROWS_PER_PAGE, tab === 0 ? "issue" : "pr", { | ||
| search: searchTitle, | ||
| repo: selectedRepo, | ||
| startDate, | ||
| endDate, | ||
| state: tab === 0 ? issueFilter : prFilter, | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Potential double fetch when submitting from a non-first page.
setPage(0) schedules a state update, then fetchData(...) is invoked synchronously. When the current page is not 0, the subsequent re-render will trigger the useEffect (because page is in its dep array), firing a second fetchData call with the same arguments. On page 0 only the direct call runs. The result is an inconsistent number of network requests depending on the user's pagination state.
♻️ Suggested fix — let the effect own the fetch
const handleSubmit = (e: React.FormEvent<HTMLFormElement>): void => {
e.preventDefault();
- setPage(0);
- fetchData(username, 1, ROWS_PER_PAGE, tab === 0 ? "issue" : "pr", {
- search: searchTitle,
- repo: selectedRepo,
- startDate,
- endDate,
- state: tab === 0 ? issueFilter : prFilter,
- });
+ if (page !== 0) {
+ setPage(0); // effect will refetch with page=0
+ } else {
+ fetchData(username, 1, ROWS_PER_PAGE, tab === 0 ? "issue" : "pr", {
+ search: searchTitle,
+ repo: selectedRepo,
+ startDate,
+ endDate,
+ state: tab === 0 ? issueFilter : prFilter,
+ });
+ }
};🤖 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 `@src/pages/Tracker/Tracker.tsx` around lines 94 - 104, handleSubmit currently
calls setPage(0) and then fetchData(...) directly, causing a duplicate fetch
because the effect that depends on page will run after the state update; remove
the synchronous fetchData call from handleSubmit and let the existing useEffect
that watches page (and the filter/search state) own fetching when page changes.
Ensure the effect's dependency array includes searchTitle, selectedRepo,
startDate, endDate, tab, issueFilter, prFilter and page so setting page to 0
triggers the single, correct fetch.
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.
🛠️ Related Issue
Closes: #360
📌 Description
This PR fixes a critical data-flow bug in the Tracker page (
/track) where search filters, repository scopes, and date ranges were being applied client-side on a paginated subset of only 10 items. This was causing a pagination count mismatch and prevented users from searching their complete GitHub history.This PR also fixes a pre-existing TypeScript type-checking build error in the hook destructuring block of
Tracker.tsx.✨ Changes Made
useEffectandhandleSubmitroutines inTracker.tsxto pass the state-driven search/filter inputs directly to thefetchDatacall. The GitHub Search API now handles all filtering server-side.filterDataclient-side helper function completely since the API returns already-filtered data.authErrorparameter from theuseGitHubAuthhook and updating the errorAlertrendering logic to only readdataError.<form>container to wrap both authentication and filter blocks so users can apply filters by pressing "Enter" inside any filter text field.onChangeevent to callsetPage(0)so pagination resets back to the first page when changing status filters.📷 Screenshots
🧪 Type of Change
✔️ Checklist
npm run build)Summary by CodeRabbit