Improve search stability: debounce input and cancel in-flight GitHub requests#338
Closed
ABHImaybeJEET wants to merge 5 commits into
Closed
Improve search stability: debounce input and cancel in-flight GitHub requests#338ABHImaybeJEET wants to merge 5 commits into
ABHImaybeJEET wants to merge 5 commits into
Conversation
❌ Deploy Preview for github-spy failed.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds commit classification and commits as a new data source, refactors the GitHub data hook to debounce and cancel concurrent fetches, and extends the Tracker UI with a "Commits" tab that shows commit messages, classification badges, and importance-based filtering. ChangesCommit Tracking Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant useGitHubData
participant Debouncer
participant AbortController
participant GitHubAPI
participant Classifier as classifyCommit
Client->>useGitHubData: fetchData(username, page, perPage)
useGitHubData->>Debouncer: schedule debounced call
Debouncer->>Debouncer: wait debounce interval
alt prior request exists
useGitHubData->>AbortController: abort()
AbortController->>GitHubAPI: cancel pending requests
end
Debouncer->>GitHubAPI: search/issues (author query)
Debouncer->>GitHubAPI: search/issues (prs author query)
Debouncer->>GitHubAPI: search/commits (author query with preview header)
GitHubAPI-->>useGitHubData: issues, prs, commits
useGitHubData->>Classifier: classifyCommit(commit.message, files, adds, dels)
Classifier-->>useGitHubData: classifiedInfo
useGitHubData->>useGitHubData: set issues/prs/commits and totals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/hooks/useGitHubData.tsParsing error: Declaration or statement expected. 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 |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Tracker/Tracker.tsx (1)
97-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop refetching all datasets on tab switches.
Since
fetchDatanow loads all tabs’ data in one call, keepingtabin this effect causes avoidable API churn.Suggested fix
useEffect(() => { if (username) { fetchData(username, page + 1, ROWS_PER_PAGE); } - }, [tab, page]); + }, [page]);🤖 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 97 - 102, The effect is refetching everything on tab changes even though fetchData now loads all tabs; update the useEffect that calls fetchData(username, page + 1, ROWS_PER_PAGE) to remove tab from its dependency list so it only runs when username or page changes (e.g., change the dependency array to [username, page]), keeping the existing username check inside the effect.
🤖 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/hooks/useGitHubData.ts`:
- Around line 205-208: In the finally block of the fetch flow in
useGitHubData.ts, guard the setLoading(false) so a stale aborted request cannot
clear a newer active loading state: only call setLoading(false) (and null-out
abortControllerRef.current) if abortControllerRef.current === controller;
otherwise leave loading unchanged. Update the finally block that currently
references abortControllerRef, controller and setLoading to perform this
equality check before mutating state.
- Around line 4-8: The file has duplicated and misplaced code: move the import
"useState, useCallback, useRef" to the top of the module, remove the
stray/duplicate export block so there is exactly one export const useGitHubData,
delete the earlier incomplete fetchPaginated declaration and keep only the
coherent fetchPaginated implementation, and remove the legacy stray async (...)
=> { ... } block inside fetchData so that fetchData is a single valid
useCallback function; reference the exports/useGitHubData, fetchPaginated, and
fetchData symbols while making these edits.
In `@src/pages/Tracker/Tracker.tsx`:
- Around line 136-138: The importance filter currently only checks for
["High","Medium","Low"] so selecting "Unknown" in the UI has no effect; update
the predicate in the Tracker component where filterType is applied (the block
that sets filtered = filtered.filter(...)) to include "Unknown" in the allowed
list OR normalize classifiedInfo?.importance to treat undefined/null as
"Unknown" before comparing so that classifiedInfo?.importance === filterType
will match "Unknown"; make the same change in the second occurrence of this
logic (the other filtered filter block referenced around lines 356-363) to keep
behavior consistent.
---
Outside diff comments:
In `@src/pages/Tracker/Tracker.tsx`:
- Around line 97-102: The effect is refetching everything on tab changes even
though fetchData now loads all tabs; update the useEffect that calls
fetchData(username, page + 1, ROWS_PER_PAGE) to remove tab from its dependency
list so it only runs when username or page changes (e.g., change the dependency
array to [username, page]), keeping the existing username check inside the
effect.
🪄 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: 2f0b66df-e421-46d7-91c9-1ea3bdb35b07
📒 Files selected for processing (3)
src/hooks/useGitHubData.tssrc/pages/Tracker/Tracker.tsxsrc/utils/commitClassifier.ts
Comment on lines
+205
to
+208
| } finally { | ||
| setLoading(false); | ||
| if (abortControllerRef.current === controller) abortControllerRef.current = null; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard setLoading(false) so stale aborted requests don’t clear active loading state.
Only the currently active controller should finalize loading state.
Suggested fix
} finally {
- setLoading(false);
- if (abortControllerRef.current === controller) abortControllerRef.current = null;
+ if (abortControllerRef.current === controller) {
+ setLoading(false);
+ abortControllerRef.current = null;
+ }
}📝 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
| } finally { | |
| setLoading(false); | |
| if (abortControllerRef.current === controller) abortControllerRef.current = null; | |
| } | |
| } finally { | |
| if (abortControllerRef.current === controller) { | |
| setLoading(false); | |
| abortControllerRef.current = null; | |
| } | |
| } |
🤖 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/hooks/useGitHubData.ts` around lines 205 - 208, In the finally block of
the fetch flow in useGitHubData.ts, guard the setLoading(false) so a stale
aborted request cannot clear a newer active loading state: only call
setLoading(false) (and null-out abortControllerRef.current) if
abortControllerRef.current === controller; otherwise leave loading unchanged.
Update the finally block that currently references abortControllerRef,
controller and setLoading to perform this equality check before mutating state.
Comment on lines
+136
to
+138
| if (["High", "Medium", "Low"].includes(filterType)) { | ||
| filtered = filtered.filter(item => item.classifiedInfo?.importance === filterType); | ||
| } |
Contributor
There was a problem hiding this comment.
Include Unknown in the importance filter predicate.
The UI offers Unknown, but filtering logic currently ignores it.
Suggested fix
- if (["High", "Medium", "Low"].includes(filterType)) {
+ if (["High", "Medium", "Low", "Unknown"].includes(filterType)) {
filtered = filtered.filter(item => item.classifiedInfo?.importance === filterType);
}Also applies to: 356-363
🤖 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 136 - 138, The importance filter
currently only checks for ["High","Medium","Low"] so selecting "Unknown" in the
UI has no effect; update the predicate in the Tracker component where filterType
is applied (the block that sets filtered = filtered.filter(...)) to include
"Unknown" in the allowed list OR normalize classifiedInfo?.importance to treat
undefined/null as "Unknown" before comparing so that classifiedInfo?.importance
=== filterType will match "Unknown"; make the same change in the second
occurrence of this logic (the other filtered filter block referenced around
lines 356-363) to keep behavior consistent.
mehul-m-prajapati
added
onhold
and removed
level:intermediate
quality:clean
gssoc:approved
labels
Contributor
Author
|
I have done it, please check once as there have been various changes in the
codebase since the last commit.
…On Sat, May 23, 2026 at 5:24 PM coderabbitai[bot] ***@***.***> wrote:
***@***.***[bot]* commented on this pull request.
------------------------------
In src/hooks/useGitHubData.ts
<#338 (comment)>
:
> +export const useGitHubData = (getOctokit: () => any) => {
+ const [issues, setIssues] = useState<any[]>([]);
+ const [prs, setPrs] = useState<any[]>([]);
+ const [commits, setCommits] = useState<any[]>([]);
import { useState, useCallback, useRef } from 'react';
Seems like the humans are having a chat. I'll hop back into my burrow for
now. If you need me again, just tag @coderabbitai
<https://github.com/coderabbitai> in a new comment, and I'll come hopping
out!
—
Reply to this email directly, view it on GitHub
<#338 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOV2PZQ643J3KEE62EODP7L44GGRFAVCNFSM6AAAAACZE3SIMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGNJQGU4TEMRSGE>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Contributor
|
(◕‿◕)っ 🐇 ✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useGitHubData.ts (1)
115-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't turn commit API failures into a fake "0 commits" state.
Any non-abort error here is converted to
{ items: [], total: 0 }. The tracker renders that exactly like a real empty result set, so users can't tell whether the author has no commits or the commits endpoint failed. Bubble the error or track a partial-failure state instead of silently swallowing it.🤖 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/hooks/useGitHubData.ts` around lines 115 - 119, The current catch on fetchCommitsPaginated in useGitHubData.ts swallows all non-abort errors and returns { items: [], total: 0 }, which hides real API failures; change the handler so that only AbortError returns the empty fallback and all other errors are propagated (re-throw) or converted into a distinct error/partial-failure signal that your hook can surface (e.g., set an error state or a partialFailure flag) instead of returning a fake zero-commit result; update the call site around fetchCommitsPaginated(...) to either throw the caught error for upstream handling or return a clearly labelled failure object so the UI can distinguish "no commits" from "fetch failed."
🤖 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/hooks/useGitHubData.ts`:
- Around line 76-77: Short-circuit when the trimmed username is empty to avoid
constructing an invalid GitHub query: after computing trimmedUsername in the
function (the anonymous async fetch/search function that uses trimmedUsername
and builds q = `author:`), return early (or reset hook state) if trimmedUsername
=== "" so you never set q = "author:" or call the GitHub search; update the
logic around trimmedUsername and q to bail out before building or executing the
request and ensure the hook state is cleared/reset appropriately.
- Around line 80-90: Move cancellation/cleanup before returning when octokit is
falsy: clear any pending debounce timeout (e.g., debounceTimeoutRef.current) and
abort/reset abortControllerRef.current before the early return in the !octokit
branch so no queued callback or in-flight request can mutate state. Also,
instead of resolving getOctokit() earlier, defer calling/awaiting getOctokit()
inside the delayed debounce callback immediately before making the request so
the callback uses a fresh client. Update the logic around octokit,
abortControllerRef.current, and getOctokit() accordingly.
---
Outside diff comments:
In `@src/hooks/useGitHubData.ts`:
- Around line 115-119: The current catch on fetchCommitsPaginated in
useGitHubData.ts swallows all non-abort errors and returns { items: [], total: 0
}, which hides real API failures; change the handler so that only AbortError
returns the empty fallback and all other errors are propagated (re-throw) or
converted into a distinct error/partial-failure signal that your hook can
surface (e.g., set an error state or a partialFailure flag) instead of returning
a fake zero-commit result; update the call site around
fetchCommitsPaginated(...) to either throw the caught error for upstream
handling or return a clearly labelled failure object so the UI can distinguish
"no commits" from "fetch failed."
🪄 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: 0fc6fbf0-ed15-49fa-abe1-3c32325adfe4
📒 Files selected for processing (1)
src/hooks/useGitHubData.ts
Collaborator
|
why state is blank ? |
mehul-m-prajapati
requested changes
May 23, 2026
| signal?: AbortSignal | ||
| ) => { | ||
| let q = `author:${username} is:${type}`; | ||
|
|
Collaborator
There was a problem hiding this comment.
why did you removed this code block? @ABHImaybeJEET
Contributor
Author
|
Really sorry, I staged something else and pushed it . I would correct it as
whole and raise a new PR .
…On Sat, 23 May 2026, 20:00 Mehul Prajapati, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/hooks/useGitHubData.ts
<#338 (comment)>
:
> ) => {
- let q = `author:${username} is:${type}`;
-
why did you removed this code block? @ABHImaybeJEET
<https://github.com/ABHImaybeJEET>
—
Reply to this email directly, view it on GitHub
<#338 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOV2PZWZSUTWELUMJLMLKC344GYWZAVCNFSM6AAAAACZE3SIMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGNJQHA3TCNBWGY>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
ABHImaybeJEET
force-pushed
the
single-commit/debounce
branch
from
e2d6ec3 to
3b33d5c
Compare
Contributor
Author
|
Hello @mehul-m-prajapati |
Contributor
Author
|
Otherwise, worst case, I would fork newly and again solve this issue, if thats what you feel is needed. |
Collaborator
|
Get latest commit and make new PR |
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
2 participants
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.
THIS PR CLOSES #337 .
Description
request: { signal }).Changes
fetchData.AbortControllercancellation for previous requests.signalintooctokit.requestcalls for both issues and commits endpoints.octokit).Why
Testing
DEBOUNCE_MS) if you prefer shorter/longer delay.Summary by CodeRabbit
New Features
Bug Fixes