Fix: moved Github ID & Token to environmental variable #199
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughConditional UI update in Tracker page: if the filtered list is empty, render a centered, tab-specific empty-state message; otherwise render the existing table with pagination. Also standardizes import quotes to single quotes. No changes to exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant TrackerPage as Tracker Page
participant UI as UI Renderer
User->>TrackerPage: Open tab (Issues/PRs)
TrackerPage->>TrackerPage: Compute currentFilteredData
alt currentFilteredData.length == 0
TrackerPage->>UI: Render centered empty-state (tab-specific text)
else
TrackerPage->>UI: Render Table (Title, Repository, State, Created, Pagination)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @ali-0507 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}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
173-179: UI Token Field Still Required; Implement Env Fallback or RemoveWe ran searches across all TS/TSX files and confirmed there is no usage of
import.meta.env.VITE_GITHUB_TOKENorprocess.env.REACT_APP_GITHUB_TOKEN/ID. TheuseGitHubAuthhook unconditionally manages token via React state, and the Tracker page always renders a required PAT input. This conflicts with the PR’s stated goal of “moved GitHub ID & Token to environmental variable.”Recommendations:
- Gate or hide the Personal Access Token field when an environment token is present. For example:
// src/pages/Tracker/Tracker.tsx const envToken = import.meta.env.VITE_GITHUB_TOKEN || ''; … return ( <> {!envToken && ( <TextField label="Personal Access Token" value={token} onChange={e => setToken(e.target.value)} type="password" required sx={{ flex: 1, minWidth: 150 }} /> )} {/* use envToken || token when calling Octokit */} </> );- Or remove the front-end token input entirely and implement a backend-mediated auth flow (OAuth App or GitHub App with server exchange).
- Security reminder: Vite/CRA env vars are baked into the client bundle and are not secure storage for real PATs. A backend proxy or OAuth exchange is required for any production-grade authentication.
Please update src/hooks/useGitHubAuth.ts (lines 5–7) and src/pages/Tracker/Tracker.tsx (around lines 173–179) to reflect one of the above approaches.
🧹 Nitpick comments (7)
src/pages/Tracker/Tracker.tsx (7)
282-282: Use theme palette instead of hard-coded gray for better theming accessibility.Hard-coding “gray” can clash in dark mode or custom themes. Prefer palette tokens.
- <Box textAlign="center" my={4} color="gray"> + <Box textAlign="center" my={4} sx={{ color: theme.palette.text.secondary }}>
317-318: Show owner/repo for clarity and parity with ContributorProfile.This currently shows only the repo name. Displaying owner/repo is more informative and matches
ContributorProfile.tsx.- {item.repository_url.split("/").slice(-1)[0]} + {item.repository_url.split('/').slice(-2).join('/')}
38-46: Unify item type to reflect both Issues Search and Pulls API shapes.Add an optional
merged_atto accommodate PR items coming from the Pulls API and avoidanycasts downstream.interface GitHubItem { id: number; title: string; state: string; created_at: string; - pull_request?: { merged_at: string | null }; + // For PRs from Pulls API + merged_at?: string | null; + // For items originating from Search Issues API + pull_request?: { merged_at?: string | null }; repository_url: string; html_url: string; }
284-287: Clarify empty-state copy when client-side filters are active.When filters are applied, “No X found for this user.” can be misleading if other pages might contain matches. Consider a page-aware message.
- {tab === 0 - ? "No issues found for this user." - : "No pull requests found for this user."} + {filtersActive + ? (tab === 0 + ? "No matching issues on this page. Adjust filters or try a different page." + : "No matching pull requests on this page. Adjust filters or try a different page.") + : (tab === 0 + ? "No issues found for this user." + : "No pull requests found for this user.")}Add once near your state hooks:
const filtersActive = (tab === 0 ? issueFilter !== 'all' : prFilter !== 'all') || !!searchTitle || !!selectedRepo || !!startDate || !!endDate;
74-80: Reset to the first page when filters change to reduce “empty current page” confusion.Without this, narrowing filters while on a later page can yield an empty-state even if earlier pages have matches.
const [endDate, setEndDate] = useState(""); + // Jump back to page 0 whenever filters change + useEffect(() => { + setPage(0); + }, [issueFilter, prFilter, searchTitle, selectedRepo, startDate, endDate]);
327-334: Pagination count vs. client-side filtering can mislead users.
count={totalCount}reflects server totals, but rows are filtered per-page on the client. Consider:
- Hiding pagination when
filtersActive && currentFilteredData.length === 0, or- Keeping pagination but acknowledging page-level emptiness in the empty-state message (see prior comment), or
- Moving filters server-side so
countreflects filtered totals.
88-93: Minor: avoid potential double fetch on submit when page != 0.
handleSubmitcallsfetchDataand also setspage(0), which triggers theuseEffectfetch—leading to two requests when the current page is not 0.- const handleSubmit = (e: React.FormEvent<HTMLFormElement>): void => { + const handleSubmit = (e: React.FormEvent<HTMLFormElement>): void => { e.preventDefault(); - setPage(0); - fetchData(username, 1, ROWS_PER_PAGE); + const was = page; + setPage(0); + if (was === 0) { + fetchData(username, 1, ROWS_PER_PAGE); + } };Alternative: centralize all fetches in event handlers (submit/page change) and remove the
useEffectfetch, or track asubmittedUsernameto trigger the effect intentionally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/pages/Tracker/Tracker.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Tracker/Tracker.tsx (1)
src/pages/ContributorProfile/ContributorProfile.tsx (2)
pr(74-88)ContributorProfile(11-95)
🔇 Additional comments (1)
src/pages/Tracker/Tracker.tsx (1)
281-289: Empty-state UX is a clear improvement.Conditional rendering avoids an empty table/pagination and gives users immediate, contextual feedback. Nicely done.
Also applies to: 290-337
| {item.pull_request?.merged_at ? "merged" : item.state} | ||
| </TableCell> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Merged status detection may be unreliable depending on the source API.
If PRs come from GitHub’s Search Issues API, pull_request.merged_at is not included; merged_at exists on PR objects from the Pulls API. Current logic could under-report “merged”.
- {item.pull_request?.merged_at ? "merged" : item.state}
+ {(item as any).merged_at || item.pull_request?.merged_at ? 'merged' : item.state}Follow-up:
- If you’d rather avoid
any, extend the type (see next comment) and useitem.merged_at ?? item.pull_request?.merged_at. - Please verify where
prsare fetched/normalized.
Run this to locate data shaping and confirm the available fields:
🏁 Script executed:
#!/bin/bash
# Inspect how PRs are fetched and whether merged_at is present or mapped.
rg -nP -C3 --type=ts --type=tsx '\b(search/issues|pulls)\b|merged_at|pull_request' srcLength of output: 116
🏁 Script executed:
#!/bin/bash
# Locate the Tracker.tsx file
fd --extension tsx Tracker.tsx
# Find all occurrences of merged_at with context
grep -R -n -C3 "merged_at" src
# Find all occurrences of pull_request with context
grep -R -n -C3 "pull_request" src
# Print code around the relevant snippet in Tracker.tsx (lines 250-360)
sed -n '250,360p' src/pages/Tracker/Tracker.tsxLength of output: 6937
🏁 Script executed:
#!/bin/bash
# Locate where data is fetched/normalized in Tracker.tsx
grep -R -n -C2 "useEffect" src/pages/Tracker/Tracker.tsx
grep -R -n -C2 "fetch(" src/pages/Tracker/Tracker.tsx
grep -R -n -C2 "axios" src/pages/Tracker/Tracker.tsx
grep -R -n -C2 "graphql" src/pages/Tracker/Tracker.tsxLength of output: 468
🏁 Script executed:
#!/bin/bash
# Locate the fetchData function and inspect its implementation
grep -R -n -C3 "fetchData" src/pages/Tracker/Tracker.tsx
# Print the top of the file to view data fetching logic (lines 1–150)
sed -n '1,150p' src/pages/Tracker/Tracker.tsxLength of output: 4316
🏁 Script executed:
#!/bin/bash
# Locate useGitHubData hook file
fd useGitHubData src/hooks
# Print first 200 lines of useGitHubData implementation
sed -n '1,200p' src/hooks/useGitHubData.tsLength of output: 2033
Ensure merged status detection is accurate by fetching PRs via the Pull Requests API
The current useGitHubData.fetchData implementation calls GitHub’s GET /search/issues endpoint for both issues and PRs. That endpoint’s pull_request objects do not include merged_at — only url, html_url, diff_url, and patch_url — so
{item.pull_request?.merged_at ? "merged" : item.state}will always fall back to item.state, under-reporting merged PRs (github.com).
Action items:
- src/hooks/useGitHubData.ts
• For PRs (type === 'pr'), replace or augmentfetchPaginatedwith a call tooctokit.rest.pulls.list(oroctokit.pulls.list) so each PR record includes its realmerged_atvalue (github.com). - src/pages/Tracker/Tracker.tsx
• Extend theGitHubItemtype to match the Pull Requests API shape (e.g. addmerged_at: string | nullat top level or underpull_request).
• Update the status cell and filter logic to use that actualmerged_atfield for detecting merged PRs.
Once PRs are fetched through the Pulls API (or via a GraphQL query for mergedAt), item.pull_request?.merged_at will be populated correctly and merged PRs will render as “merged.”
🤖 Prompt for AI Agents
In src/pages/Tracker/Tracker.tsx around lines 320-321 and
src/hooks/useGitHubData.ts, the UI checks item.pull_request?.merged_at but
search/issues does not return merged_at; update fetching for PRs to use the
Pulls API so each PR includes merged_at (e.g. in src/hooks/useGitHubData.ts,
when type === 'pr' call octokit.rest.pulls.list or equivalent paginated pulls
endpoint instead of GET /search/issues), then extend the GitHubItem type in
Tracker.tsx to include merged_at (either at top-level for PR items or under
pull_request) and change the status cell and any filter logic to read that
merged_at field to determine "merged" status.
1 participant
Solve Issue no #197
Summary
This PR fixes the empty state issue in the Issues/PRs tab.
Now, instead of showing an empty table with pagination, the UI displays
a clear message when no data is available.
Changes Made
Testing
.env.Summary by CodeRabbit