feat: Filter PRs by status in LatestMergedPRs#427
Conversation
📝 WalkthroughWalkthroughPR retrieval now includes open, closed, and merged states, with GraphQL and REST fallback paths. The shared PR type carries state metadata, and ChangesPR status retrieval and presentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProfileData
participant githubGraphQL
participant GitHubAPI
participant LatestMergedPRs
ProfileData->>githubGraphQL: Search PRs across states
githubGraphQL->>GitHubAPI: Submit GraphQL request
GitHubAPI-->>ProfileData: Return PR nodes or failure
ProfileData->>GitHubAPI: Search merged, open, and closed PRs through REST fallback
GitHubAPI-->>ProfileData: Return mapped MergedPR records
ProfileData-->>LatestMergedPRs: Provide PRs with state
LatestMergedPRs->>LatestMergedPRs: Filter and render selected state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. 🔧 Biome (2.5.1)src/lib/profile-data.tsFile contains syntax errors that prevent linting: Line 332: Illegal use of an export declaration not at the top level; Line 346: Illegal use of an export declaration not at the top level; Line 373: Illegal use of an export declaration not at the top level; Line 396: expected 🔧 ESLint
src/lib/profile-data.tsParsing error: '}' 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 |
Welcome to OSSfolio, @Aditya8369! 🎉Thank you for opening this pull request and contributing to the open-source community! 🚀 To ensure a smooth review process, please make sure you have:
We will review your PR as soon as possible. Happy coding! 💻✨ |
github-actions
Bot
added
frontend
PRODHOSH
added
ELUSOC
|
fix the build error here @Aditya8369 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/lib/profile-data.ts (1)
295-325: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove dead old implementation left inside catch block — causes syntax error.
The old
fetchMergedPRsbody (lines 296–324) was not removed when the new REST fallback was added. It now sits inside thecatchblock at line 295. Tracing brace depth: the}at line 325 closes the catch block, but the function opened at line 181 is never closed. TheGitHubUserinterface at line 332 would be parsed as nested insidefetchMergedPRs, which is invalid TypeScript and will fail to compile.Additionally, the dead code returns objects without the required
stateproperty, violating the updatedMergedPRinterface.🐛 Proposed fix: remove dead code and close function properly
return [...merged, ...open, ...closed]; - } catch { - const query = `search/issues?q=author:${encodeURIComponent(username)}+type:pr+is:merged&sort=updated&order=desc&per_page=${limit}`; - try { - const res = await fetchWithTimeout( - `https://api.github.com/${query}`, - { - headers: { Accept: "application/vnd.github.v3+json" }, - cache: "no-store", - }, - 10_000 - ); - - if (!res.ok) { - await throwIfRateLimited(res); - return []; - } - const json = await res.json(); - if (!Array.isArray(json.items)) return []; - return json.items.map((item: any) => ({ - title: item.title, - url: item.html_url, - repoName: item.repository_url.split('/').slice(-1)[0], - mergedAt: item.closed_at, - })); - } catch (e) { - // A rate limit must not be flattened into "this user has none" — the result is - // persisted now, so that would cache an empty list as fresh for a full hour. - if (e instanceof Error && e.message === "RateLimit") throw e; - return []; - } + } catch (e) { + if (e instanceof Error && e.message === "RateLimit") throw e; + return []; } }🤖 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/lib/profile-data.ts` around lines 295 - 325, Remove the obsolete GitHub REST fallback implementation nested inside the catch block of fetchMergedPRs, then add the missing closing brace for fetchMergedPRs before the GitHubUser interface. Keep the active implementation and its rate-limit handling unchanged, and eliminate the dead mapping that omits the required MergedPR.state field.
🤖 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/components/profile/LatestMergedPRs.tsx`:
- Line 56: In the onChange handler for LatestMergedPRs, replace the any
assertion on e.target.value with an assertion to the component’s defined
filter-value union type, preserving the existing setFilter update while
retaining compile-time validation of allowed values.
In `@src/lib/profile-data.ts`:
- Line 263: Update the REST fallback fetch options in fetchForQuery to use
cache: "no-store" instead of next: { revalidate: 3600 }, matching the existing
convention in fetchGitHubUser and fetchGitHubRepos so the database snapshot
remains the sole freshness source.
- Around line 256-294: Restore rate-limit propagation in fetchForQuery and the
surrounding fetchMergedPRs flow: call throwIfRateLimited(res) before returning
[] for non-OK responses, re-throw errors whose message is "RateLimit" instead of
swallowing them in both catch blocks, and preserve empty-list behavior for other
failures so syncProfileSnapshot’s rateLimited guard can detect throttling.
---
Outside diff comments:
In `@src/lib/profile-data.ts`:
- Around line 295-325: Remove the obsolete GitHub REST fallback implementation
nested inside the catch block of fetchMergedPRs, then add the missing closing
brace for fetchMergedPRs before the GitHubUser interface. Keep the active
implementation and its rate-limit handling unchanged, and eliminate the dead
mapping that omits the required MergedPR.state field.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4835715f-56e1-436d-b6e6-8a55bfaa59de
📒 Files selected for processing (4)
src/components/profile/LatestMergedPRs.tsxsrc/lib/github.tssrc/lib/profile-data.tssrc/types/index.ts
| </h2> | ||
| <select | ||
| value={filter} | ||
| onChange={(e) => setFilter(e.target.value as any)} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace as any with a proper type assertion.
Using as any bypasses type checking on the select value. Cast directly to the union type to catch invalid values at compile time.
🔧 Proposed fix
- onChange={(e) => setFilter(e.target.value as any)}
+ onChange={(e) => setFilter(e.target.value as 'merged' | 'open' | 'closed')}📝 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.
| onChange={(e) => setFilter(e.target.value as any)} | |
| onChange={(e) => setFilter(e.target.value as 'merged' | 'open' | 'closed')} |
🤖 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/components/profile/LatestMergedPRs.tsx` at line 56, In the onChange
handler for LatestMergedPRs, replace the any assertion on e.target.value with an
assertion to the component’s defined filter-value union type, preserving the
existing setFilter update while retaining compile-time validation of allowed
values.
| const fetchForQuery = async (q: string, state: "open" | "closed" | "merged"): Promise<MergedPR[]> => { | ||
| const query = `search/issues?q=${q}&sort=updated&order=desc&per_page=${limit}`; | ||
| try { | ||
| const res = await fetchWithTimeout( | ||
| `https://api.github.com/${query}`, | ||
| { | ||
| headers: { Accept: "application/vnd.github.v3+json" }, | ||
| next: { revalidate: 3600 }, | ||
| }, | ||
| 10_000 | ||
| ); | ||
| if (!res.ok) return []; | ||
| const json = await res.json(); | ||
| if (!Array.isArray(json.items)) return []; | ||
| return json.items.map((item: any) => { | ||
| let date = item.created_at; | ||
| if (state === "merged" || state === "closed") { | ||
| date = item.closed_at || item.created_at; | ||
| } | ||
| return { | ||
| title: item.title, | ||
| url: item.html_url, | ||
| repoName: item.repository_url.split("/").slice(-1)[0], | ||
| mergedAt: date, | ||
| state, | ||
| }; | ||
| }); | ||
| } catch { | ||
| return []; | ||
| } | ||
| }; | ||
|
|
||
| try { | ||
| const [merged, open, closed] = await Promise.all([ | ||
| fetchForQuery(`author:${u}+type:pr+is:merged`, "merged"), | ||
| fetchForQuery(`author:${u}+type:pr+is:open`, "open"), | ||
| fetchForQuery(`author:${u}+type:pr+is:closed+is:unmerged`, "closed"), | ||
| ]); | ||
| return [...merged, ...open, ...closed]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore rate-limit detection in the REST fallback.
The new fetchForQuery helper silently swallows all errors (catch { return []; }), including rate-limit responses. The old code called throwIfRateLimited(res) on non-OK responses and re-threw "RateLimit" errors so the caller could distinguish "no PRs" from "API throttled." Since results are persisted by the snapshot sync, a swallowed rate limit would cache an empty list as fresh.
Two changes are needed:
- In
fetchForQuery: callthrowIfRateLimited(res)when!res.ok, and re-throw"RateLimit"in the catch block. - In the outer
try/catch(line 288): re-throw"RateLimit"so it propagates tosyncProfileSnapshot'srateLimitedguard.
🔒 Proposed fix for rate-limit handling
const fetchForQuery = async (q: string, state: "open" | "closed" | "merged"): Promise<MergedPR[]> => {
const query = `search/issues?q=${q}&sort=updated&order=desc&per_page=${limit}`;
try {
const res = await fetchWithTimeout(
`https://api.github.com/${query}`,
{
headers: { Accept: "application/vnd.github.v3+json" },
cache: "no-store",
},
10_000
);
- if (!res.ok) return [];
+ if (!res.ok) {
+ await throwIfRateLimited(res);
+ return [];
+ }
const json = await res.json();
if (!Array.isArray(json.items)) return [];
return json.items.map((item: any) => {
let date = item.created_at;
if (state === "merged" || state === "closed") {
date = item.closed_at || item.created_at;
}
return {
title: item.title,
url: item.html_url,
repoName: item.repository_url.split("/").slice(-1)[0],
mergedAt: date,
state,
};
});
- } catch {
+ } catch (e) {
+ if (e instanceof Error && e.message === "RateLimit") throw e;
return [];
}
};
try {
const [merged, open, closed] = await Promise.all([
fetchForQuery(`author:${u}+type:pr+is:merged`, "merged"),
fetchForQuery(`author:${u}+type:pr+is:open`, "open"),
fetchForQuery(`author:${u}+type:pr+is:closed+is:unmerged`, "closed"),
]);
return [...merged, ...open, ...closed];
- } catch {
+ } catch (e) {
+ if (e instanceof Error && e.message === "RateLimit") throw e;
return [];
}
}Based on learnings, fetchMergedPRs is expected to detect rate limiting reliably for the snapshot sync's rateLimited guard.
📝 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.
| const fetchForQuery = async (q: string, state: "open" | "closed" | "merged"): Promise<MergedPR[]> => { | |
| const query = `search/issues?q=${q}&sort=updated&order=desc&per_page=${limit}`; | |
| try { | |
| const res = await fetchWithTimeout( | |
| `https://api.github.com/${query}`, | |
| { | |
| headers: { Accept: "application/vnd.github.v3+json" }, | |
| next: { revalidate: 3600 }, | |
| }, | |
| 10_000 | |
| ); | |
| if (!res.ok) return []; | |
| const json = await res.json(); | |
| if (!Array.isArray(json.items)) return []; | |
| return json.items.map((item: any) => { | |
| let date = item.created_at; | |
| if (state === "merged" || state === "closed") { | |
| date = item.closed_at || item.created_at; | |
| } | |
| return { | |
| title: item.title, | |
| url: item.html_url, | |
| repoName: item.repository_url.split("/").slice(-1)[0], | |
| mergedAt: date, | |
| state, | |
| }; | |
| }); | |
| } catch { | |
| return []; | |
| } | |
| }; | |
| try { | |
| const [merged, open, closed] = await Promise.all([ | |
| fetchForQuery(`author:${u}+type:pr+is:merged`, "merged"), | |
| fetchForQuery(`author:${u}+type:pr+is:open`, "open"), | |
| fetchForQuery(`author:${u}+type:pr+is:closed+is:unmerged`, "closed"), | |
| ]); | |
| return [...merged, ...open, ...closed]; | |
| const fetchForQuery = async (q: string, state: "open" | "closed" | "merged"): Promise<MergedPR[]> => { | |
| const query = `search/issues?q=${q}&sort=updated&order=desc&per_page=${limit}`; | |
| try { | |
| const res = await fetchWithTimeout( | |
| `https://api.github.com/${query}`, | |
| { | |
| headers: { Accept: "application/vnd.github.v3+json" }, | |
| cache: "no-store", | |
| }, | |
| 10_000 | |
| ); | |
| if (!res.ok) { | |
| await throwIfRateLimited(res); | |
| return []; | |
| } | |
| const json = await res.json(); | |
| if (!Array.isArray(json.items)) return []; | |
| return json.items.map((item: any) => { | |
| let date = item.created_at; | |
| if (state === "merged" || state === "closed") { | |
| date = item.closed_at || item.created_at; | |
| } | |
| return { | |
| title: item.title, | |
| url: item.html_url, | |
| repoName: item.repository_url.split("/").slice(-1)[0], | |
| mergedAt: date, | |
| state, | |
| }; | |
| }); | |
| } catch (e) { | |
| if (e instanceof Error && e.message === "RateLimit") throw e; | |
| return []; | |
| } | |
| }; | |
| try { | |
| const [merged, open, closed] = await Promise.all([ | |
| fetchForQuery(`author:${u}+type:pr+is:merged`, "merged"), | |
| fetchForQuery(`author:${u}+type:pr+is:open`, "open"), | |
| fetchForQuery(`author:${u}+type:pr+is:closed+is:unmerged`, "closed"), | |
| ]); | |
| return [...merged, ...open, ...closed]; | |
| } catch (e) { | |
| if (e instanceof Error && e.message === "RateLimit") throw e; | |
| return []; | |
| } |
🤖 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/lib/profile-data.ts` around lines 256 - 294, Restore rate-limit
propagation in fetchForQuery and the surrounding fetchMergedPRs flow: call
throwIfRateLimited(res) before returning [] for non-OK responses, re-throw
errors whose message is "RateLimit" instead of swallowing them in both catch
blocks, and preserve empty-list behavior for other failures so
syncProfileSnapshot’s rateLimited guard can detect throttling.
Source: Learnings
| `https://api.github.com/${query}`, | ||
| { | ||
| headers: { Accept: "application/vnd.github.v3+json" }, | ||
| next: { revalidate: 3600 }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use cache: "no-store" instead of next: { revalidate: 3600 } in the REST fallback.
fetchForQuery uses next: { revalidate: 3600 }, but fetchGitHubUser (line 354) and fetchGitHubRepos (line 378) in the same file deliberately use cache: "no-store" with an explicit comment: "The snapshot TTL in the DB is now the single source of freshness. A second, independent Next Data Cache in front of it would let a 'stale' sync be served an hour-old response." The REST fallback should follow the same convention to avoid serving stale PR data.
🤖 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/lib/profile-data.ts` at line 263, Update the REST fallback fetch options
in fetchForQuery to use cache: "no-store" instead of next: { revalidate: 3600 },
matching the existing convention in fetchGitHubUser and fetchGitHubRepos so the
database snapshot remains the sole freshness source.
|
fix the build error and coderabbit comments @Aditya8369 |
2 participants
Summary
I have implemented the filter dropdown for Pull Request
Related Issue
Closes #295
Type of Change
Changes Made
Checklist
mainconsole.logleft insrc/schema.sqland a new migration file are includedDESIGN.mdand followed the design system (colors, spacing, typography, components)feat:,fix:,docs:, etc.)Summary by CodeRabbit
New Features
Bug Fixes