Skip to content

fix: search & filter mismatch with pagination on Tracker pageagination #362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rathan2511 wants to merge 1 commit into from
+65 −91
Closed

fix: search & filter mismatch with pagination on Tracker pageagination #362

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 65 additions & 91 deletions src/pages/Tracker/Tracker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ const Home: React.FC = () => {
setUsername,
token,
setToken,
error: authError,
getOctokit,
} = useGitHubAuth();

Expand All @@ -82,14 +81,26 @@ const Home: React.FC = () => {
// Fetch data when username, tab, or page changes
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]);
Comment on lines 82 to +92
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd -t f 'useGitHubData' src/hooks
rg -nP -C5 '\b(fetchData|useGitHubData)\b' src/hooks

Repository: 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' src

Repository: 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' src

Repository: 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.tsx

Repository: GitMetricsLab/github_tracker

Length of output: 54

Avoid duplicate network requests when applying filters + fix deps lint intent

  • handleSubmit sets page to 0 and also calls fetchData directly; when the current page is not 0, the useEffect (which depends on page) fires and triggers the same fetch again → duplicate requests.
  • useEffect uses username (and fetchData) but they’re intentionally not in deps; since fetchData is already memoized via useCallback, keep the current “auth changes require Submit” behavior by suppressing react-hooks/exhaustive-deps with a brief comment (or include them only if you want auto-fetch on auth changes).
  • The merged state is already handled in useGitHubData by translating filters.state === 'merged' to is:merged for PR searches—no change needed there.
  • No authError exists in useGitHubAuth; dataError from useGitHubData is 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.


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,
});
};
Comment on lines 94 to 104
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


const handlePageChange = (_: unknown, newPage: number) => {
Expand All @@ -99,45 +110,6 @@ const Home: React.FC = () => {
const formatDate = (dateString: string): string =>
new Date(dateString).toLocaleDateString();

const filterData = (data: GitHubItem[], filterType: string): GitHubItem[] => {
let filtered = [...data];
if (["open", "closed", "merged"].includes(filterType)) {
filtered = filtered.filter((item) => {
if (filterType === "merged") {
return !!item.pull_request?.merged_at
}
else if (filterType === "closed") {
return item.state === "closed" && !item.pull_request?.merged_at
}
else {
//open
return item.state === "open"
}
});
}
if (searchTitle) {
filtered = filtered.filter((item) =>
item.title.toLowerCase().includes(searchTitle.toLowerCase())
);
}
if (selectedRepo) {
filtered = filtered.filter((item) =>
item.repository_url.includes(selectedRepo)
);
}
if (startDate) {
filtered = filtered.filter(
(item) => new Date(item.created_at) >= new Date(startDate)
);
}
if (endDate) {
filtered = filtered.filter(
(item) => new Date(item.created_at) <= new Date(endDate)
);
}
return filtered;
};

const getStatusIcon = (item: GitHubItem) => {

if (item.pull_request) {
Expand All @@ -158,16 +130,15 @@ const Home: React.FC = () => {
};


// Current data and filtered data according to tab and filters
const currentRawData = tab === 0 ? issues : prs;
const currentFilteredData = filterData(currentRawData, tab === 0 ? issueFilter : prFilter);
// Current data according to tab
const currentFilteredData = tab === 0 ? issues : prs;
const totalCount = tab === 0 ? totalIssues : totalPrs;

return (
<Container maxWidth="lg" sx={{ mt: 4, minHeight: "80vh", color: theme.palette.text.primary }}>
{/* Auth Form */}
<Paper elevation={1} sx={{ p: 2, mb: 4, backgroundColor: theme.palette.background.paper }}>
<form onSubmit={handleSubmit}>
<form onSubmit={handleSubmit}>
{/* Auth Form */}
<Paper elevation={1} sx={{ p: 2, mb: 4, backgroundColor: theme.palette.background.paper }}>
<Box sx={{ display: "flex", gap: 2, flexWrap: "wrap" }}>
<TextField
label="GitHub Username"
Expand Down Expand Up @@ -236,43 +207,43 @@ const Home: React.FC = () => {
alignSelf: "flex-start",
}}
>
Fetch Data
Fetch Data / Apply Filters
</Button>
</Box>
</form>
</Paper>

{/* Filters */}
<Box sx={{ mb: 2, display: "flex", flexWrap: "wrap", gap: 2 }}>
<TextField
label="Search Title"
value={searchTitle}
onChange={(e) => setSearchTitle(e.target.value)}
sx={{ minWidth: 200 }}
/>
<TextField
label="Repository"
value={selectedRepo}
onChange={(e) => setSelectedRepo(e.target.value)}
sx={{ minWidth: 200 }}
/>
<TextField
label="Start Date"
type="date"
value={startDate}
onChange={(e) => setStartDate(e.target.value)}
InputLabelProps={{ shrink: true }}
sx={{ minWidth: 150 }}
/>
<TextField
label="End Date"
type="date"
value={endDate}
onChange={(e) => setEndDate(e.target.value)}
InputLabelProps={{ shrink: true }}
sx={{ minWidth: 150 }}
/>
</Box>
</Paper>

{/* Filters */}
<Box sx={{ mb: 2, display: "flex", flexWrap: "wrap", gap: 2 }}>
<TextField
label="Search Title"
value={searchTitle}
onChange={(e) => setSearchTitle(e.target.value)}
sx={{ minWidth: 200 }}
/>
<TextField
label="Repository"
value={selectedRepo}
onChange={(e) => setSelectedRepo(e.target.value)}
sx={{ minWidth: 200 }}
/>
<TextField
label="Start Date"
type="date"
value={startDate}
onChange={(e) => setStartDate(e.target.value)}
InputLabelProps={{ shrink: true }}
sx={{ minWidth: 150 }}
/>
<TextField
label="End Date"
type="date"
value={endDate}
onChange={(e) => setEndDate(e.target.value)}
InputLabelProps={{ shrink: true }}
sx={{ minWidth: 150 }}
/>
</Box>
</form>

{/* Tabs + State Filter */}
<Box
Expand Down Expand Up @@ -300,11 +271,14 @@ const Home: React.FC = () => {
<InputLabel sx={{ fontSize: "14px" }}>State</InputLabel>
<Select
value={tab === 0 ? issueFilter : prFilter}
onChange={(e) =>
tab === 0
? setIssueFilter(e.target.value)
: setPrFilter(e.target.value)
}
onChange={(e) => {
setPage(0);
if (tab === 0) {
setIssueFilter(e.target.value);
} else {
setPrFilter(e.target.value);
}
}}
label="State"
sx={{
backgroundColor: theme.palette.background.paper,
Expand All @@ -324,9 +298,9 @@ const Home: React.FC = () => {
</FormControl>
</Box>

{(authError || dataError) && (
{dataError && (
<Alert severity="error" sx={{ mb: 3 }}>
{authError || dataError}
{dataError}
</Alert>
)}

Expand Down
Loading