Improve search stability: debounce input and cancel in-flight GitHub requests#338
Open
ABHImaybeJEET wants to merge 5 commits into
Open
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
📝 WalkthroughWalkthroughThe PR extends the GitHub tracker to support a third "Commits" tab by introducing a commit classification utility, refactoring the data hook to debounce and concurrently fetch commits with issues and PRs, and updating the tracker page UI to render and filter commit data alongside existing items. ChangesCommit Tracking Feature
Sequence Diagram(s)sequenceDiagram
participant Component
participant Hook as useGitHubData
participant Debounce as Debouncer
participant Controller as AbortController
participant API as GitHub API
Component->>Hook: fetchData(username, page, perPage)
Hook->>Debounce: schedule debounced call
Debounce->>Debounce: wait 300-500ms
alt prior request in flight
Hook->>Controller: abort()
end
Controller->>API: cancel pending request
Debounce->>API: search issues, PRs, commits concurrently
API-->>Hook: issues, prs, commits results
Hook->>Hook: classify commits via classifyCommit()
Hook->>Hook: update issues, prs, commits, totals, error, rateLimited
flowchart LR
subgraph TabSelection["Tab Selection"]
direction LR
Issues["Issues Tab"]
PRs["PRs Tab"]
Commits["Commits Tab"]
end
subgraph DataSource["Data Source"]
direction LR
IssuesData["issues / totalIssues"]
PRsData["prs / totalPrs"]
CommitsData["commits / totalCommits"]
end
subgraph FilterType["Filter Options"]
direction LR
StateFilter["All/Open/Closed/Merged"]
ImportanceFilter["All/High/Medium/Low/Unknown"]
end
subgraph Rendering["Row Rendering"]
direction LR
Title["First line of message or title"]
Status["Importance pill or state badge"]
Icon["GitCommitIcon or StateIcon"]
end
Issues --> IssuesData
PRs --> PRsData
Commits --> CommitsData
Issues --> StateFilter
PRs --> StateFilter
Commits --> ImportanceFilter
IssuesData --> Rendering
PRsData --> Rendering
CommitsData --> Rendering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![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
+4
to
8
| 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'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/hooks/useGitHubData.ts"
echo "Checking duplicate declarations in ${FILE}..."
rg -n "export const useGitHubData|const fetchPaginated = async|^\\s*async\\s*\\(" "$FILE"
echo
echo "Expected: each declaration should appear once in final code."
echo "If multiple hits appear for the same declaration, remove the legacy block."Repository: GitMetricsLab/github_tracker
Length of output: 553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/hooks/useGitHubData.ts"
echo "---- file head (1-90) ----"
nl -ba "$FILE" | sed -n '1,90p'
echo
echo "---- middle (90-160) ----"
nl -ba "$FILE" | sed -n '90,160p'
echo
echo "---- tail (160-260) ----"
nl -ba "$FILE" | sed -n '160,260p'Repository: GitMetricsLab/github_tracker
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/hooks/useGitHubData.ts"
echo "---- file head (1-90) ----"
cat -n "$FILE" | sed -n '1,90p'
echo
echo "---- middle (90-160) ----"
cat -n "$FILE" | sed -n '90,160p'
echo
echo "---- tail (160-260) ----"
cat -n "$FILE" | sed -n '160,260p'Repository: GitMetricsLab/github_tracker
Length of output: 9642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/hooks/useGitHubData.ts"
echo "---- tail (260-420) ----"
cat -n "$FILE" | sed -n '260,420p'
echo
echo "---- very end (last 120 lines) ----"
# print last 120 lines without assuming total line count
TOTAL=$(wc -l < "$FILE")
START=$(( TOTAL>120 ? TOTAL-119 : 1 ))
cat -n "$FILE" | sed -n "${START},${TOTAL}p"Repository: GitMetricsLab/github_tracker
Length of output: 8072
Fix broken module structure in src/hooks/useGitHubData.ts (duplicate hook + stray code blocks break compilation)
useGitHubDatais exported twice (starts at line 4 and again at line 31) without properly closing the first export.- An
import { useState, useCallback, useRef } from 'react';appears inside the hook body (line 8), which is invalid syntax in TS/JS modules. fetchPaginatedis declared twice and the earlier declaration is left in an incomplete state before the later one (lines ~43-56).fetchDatacontains a stray legacyasync (...) => { ... }block starting around line 210, corrupting theuseCallbackcallback structure.
Remove the leftover legacy implementation so the file contains exactly one export const useGitHubData and one coherent fetchPaginated/fetchData implementation.
🧰 Tools
🪛 Biome (2.4.15)
[error] 8-8: Illegal use of an import declaration not at the top level
(parse)
🤖 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 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.
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.
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.
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