Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 51 additions & 8 deletions src/hooks/useGitHubData.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useState, useCallback, useRef } from 'react';
import { useState, useCallback, useRef, useEffect } from 'react';
import { Octokit } from '@octokit/core';

interface GitHubItem {
Expand Down Expand Up @@ -34,14 +34,16 @@ export const useGitHubData = (

// Prevent stale responses overwriting latest data
const lastRequestId = useRef(0);
const abortControllerRef = useRef<AbortController | null>(null);

const fetchPaginated = async (
octokit: Octokit,
username: string,
type: 'issue' | 'pr',
page = 1,
perPage = 10,
filters: FetchFilters = {}
filters: FetchFilters = {},
signal?: AbortSignal
) => {
let q = `author:${username} is:${type}`;

Expand Down Expand Up @@ -77,6 +79,9 @@ export const useGitHubData = (
order: 'desc',
per_page: perPage,
page,
request: {
signal,
},
}
);

Expand All @@ -100,6 +105,14 @@ export const useGitHubData = (
return;
}

// Cancel any active in-flight request before triggering a new one
if (abortControllerRef.current) {
abortControllerRef.current.abort();
}

const controller = new AbortController();
abortControllerRef.current = controller;

const requestId = ++lastRequestId.current;

setLoading(true);
Expand All @@ -122,7 +135,8 @@ export const useGitHubData = (
'issue',
page,
perPage,
filters
filters,
controller.signal
)
);
}
Expand All @@ -135,15 +149,16 @@ export const useGitHubData = (
'pr',
page,
perPage,
filters
filters,
controller.signal
)
);
}

const results = await Promise.allSettled(requests);

// Ignore stale requests
if (requestId !== lastRequestId.current) {
// Ignore stale or aborted requests
if (requestId !== lastRequestId.current || abortControllerRef.current !== controller) {
return;
}

Expand All @@ -156,6 +171,10 @@ export const useGitHubData = (
setIssues(issueResult.value.items);
setTotalIssues(issueResult.value.total);
} else {
const reason = issueResult.reason;
if (reason && reason.name === 'AbortError') {
return;
}
setIssues([]);
setTotalIssues(0);
}
Expand All @@ -170,6 +189,10 @@ export const useGitHubData = (
setPrs(prResult.value.items);
setTotalPrs(prResult.value.total);
} else {
const reason = prResult.reason;
if (reason && reason.name === 'AbortError') {
return;
}
setPrs([]);
setTotalPrs(0);
}
Expand All @@ -180,22 +203,33 @@ export const useGitHubData = (
);

if (hasRejected) {
const wasAborted = results.some(
(result) => result.status === 'rejected' && result.reason?.name === 'AbortError'
);
if (wasAborted) {
return;
}
setError(
'Some GitHub data could not be fetched completely.'
);
}

setRateLimited(false);
} catch (err: unknown) {
if (requestId !== lastRequestId.current) {
if (requestId !== lastRequestId.current || abortControllerRef.current !== controller) {
return;
}

const error = err as {
status?: number;
message?: string;
name?: string;
};

if (error.name === 'AbortError') {
return;
}

const errorMessage =
error.message?.toLowerCase() || '';

Expand Down Expand Up @@ -231,14 +265,23 @@ export const useGitHubData = (
);
}
} finally {
if (requestId === lastRequestId.current) {
if (requestId === lastRequestId.current && abortControllerRef.current === controller) {
setLoading(false);
}
}
},
[getOctokit, rateLimited]
);

// Cleanup abort controller on component unmount
useEffect(() => {
return () => {
if (abortControllerRef.current) {
abortControllerRef.current.abort();
}
};
}, []);

return {
issues,
prs,
Expand Down
40 changes: 23 additions & 17 deletions src/pages/Tracker/Tracker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,29 +81,35 @@ const Home: React.FC = () => {
const [startDate, setStartDate] = useState("");
const [endDate, setEndDate] = useState("");

// Prefill from user context on mount
useEffect(() => {
const user = userContext?.user;
if (user?.username) setUsername(user.username);
if (user?.token) setToken(user.token);
}, []);

const debouncedUsername = useDebounce(username, 800);
const [debouncedUsername, setDebouncedUsername] = useState(username);

// Auto-fetch when debounced username or token changes
useEffect(() => {
if (debouncedUsername) {
setPage(0);
fetchData(debouncedUsername, 1, ROWS_PER_PAGE);
if (!username) {
setDebouncedUsername("");
return;
}
}, [debouncedUsername, token]);
const handler = setTimeout(() => {
setDebouncedUsername(username);
}, 500);

return () => {
clearTimeout(handler);
};
}, [username]);

// Fetch when tab or page changes (username already set)
// Fetch data when debouncedUsername, tab, or page changes
useEffect(() => {
if (username) {
fetchData(username, page + 1, ROWS_PER_PAGE);
if (debouncedUsername && debouncedUsername.trim().length >= 1) {
fetchData(debouncedUsername, page + 1, ROWS_PER_PAGE);
}
}, [tab, page]);
}, [debouncedUsername, tab, page]);

const handleSubmit = (e: React.FormEvent<HTMLFormElement>): void => {
e.preventDefault();
setPage(0);
setDebouncedUsername(username);
fetchData(username, 1, ROWS_PER_PAGE);
Comment on lines 107 to 111
Copy link
Copy Markdown
Contributor

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

Submit still triggers the same search twice.

Updating debouncedUsername already schedules the effect-based fetch. The extra fetchData(...) here sends a second identical request whenever the submitted username differs from the current debounced value or page resets, so the hook immediately aborts one of the two calls. Keep a single trigger path here; if the button must refresh the same username/page, drive the effect with a submit counter instead of calling fetchData directly.

🤖 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 105 - 109, handleSubmit currently
both sets state (setPage, setDebouncedUsername) and calls fetchData directly,
causing duplicate requests; remove the direct fetchData(username, 1,
ROWS_PER_PAGE) call and instead drive the effect that performs fetching from
changes to debouncedUsername/page by introducing a submit counter (e.g.,
submitCount state and setSubmitCount(c => c+1)) that the effect also depends on
so the button can force refresh the same username/page without calling fetchData
manually; update the effect to include submitCount alongside debouncedUsername
and page and call fetchData from there.

};

const handlePageChange = (_: unknown, newPage: number) => {

Expand Down
Loading