feat: Enable Pagination for Pull Requests on Issue and Module Pages#3608
feat: Enable Pagination for Pull Requests on Issue and Module Pages#3608HarshitVerma109 wants to merge 10 commits intoOWASP:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughBackend GraphQL resolvers were updated to support paginated pull request retrieval (limit + offset, normalized and clamped); frontend queries, pages, components, types, and tests were updated to use fetchMore-based incremental loading and new DetailsCard callbacks for loading/resetting PR lists. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@backend/apps/github/api/internal/nodes/issue.py`:
- Around line 28-31: The pull_requests resolver (def pull_requests) currently
slices using limit/offset without validation; add guards at the start of
pull_requests to ensure limit and offset are non-negative integers (e.g., if
limit < 0 or offset < 0 raise ValueError or return an empty list) and
normalize/clip limit if desired before calling
self.pull_requests.all().order_by("-created_at")[offset:offset+limit]; this
prevents invalid ranges and unexpected behavior when slicing the queryset.
In `@backend/apps/mentorship/api/internal/nodes/module.py`:
- Around line 164-171: In recent_pull_requests, validate and sanitize the
pagination inputs before performing the slice: ensure limit and offset are
integers and non-negative (coerce or raise), return an empty list if limit is 0
or negative, and clamp limit to a safe maximum (e.g., 100) to avoid unbounded
queries; then proceed to use issue_ids and the existing
PullRequest.objects.filter(...).select_related("author").distinct().order_by("-created_at")[offset:offset+limit]
slice with the sanitized values.
In
`@frontend/src/app/mentorship/programs/`[programKey]/modules/[moduleKey]/page.tsx:
- Around line 35-42: The effect that sets hasMorePRs uses
data.getModule.recentPullRequests and compares prCount to limit but runs on
every cache-and-network update, causing hasMorePRs to stay false if the cached
page is short; change the effect (useEffect) to only recalculate setHasMorePRs
when on the first page of results (e.g., when your page or cursor state
indicates the initial page — check whatever state you use to track pagination)
so that a later network response can flip hasMorePRs back to true when prCount
=== limit; keep the same prCount/limit logic but guard it with an "is first
page" condition before calling setHasMorePRs.
In
`@frontend/src/app/my/mentorship/programs/`[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx:
- Around line 88-95: The effect only turns hasMorePRs to false but never back to
true when fresher data arrives; update the useEffect that reads
data.getModule.issueByNumber.pullRequests to compute prCount and call
setHasMorePRs(prCount >= limit) (or add an else branch that calls
setHasMorePRs(true) when prCount >= limit), ensuring you still set false when
there are no PRs; reference the useEffect, setHasMorePRs,
data.getModule.issueByNumber.pullRequests and limit symbols when making the
change.
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
Outdated
Show resolved
Hide resolved
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@frontend/src/app/mentorship/programs/`[programKey]/modules/[moduleKey]/page.tsx:
- Line 43: The line creating programModule uses an unsafe double cast ("const
programModule = data?.getModule as unknown as Module"); remove the "as unknown
as Module" and either: 1) use the generated getModule type directly (e.g. const
programModule = data?.getModule) if that suffices, 2) map the GraphQL getModule
result to a Module by explicitly transforming fields like mentors, mentees, and
recentPullRequests into Contributor[] and PullRequest[] shapes, or 3)
align/update the Module type to include the GraphQL-specific fields (__typename,
projectId, projectName) so a direct assignment is valid—choose one approach and
implement the corresponding change in the code that references
getModule/programModule so TypeScript catches real mismatches rather than
bypassing them.
🧹 Nitpick comments (4)
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
126-150: Reset optimistically setshasMorePRstotruebefore fetch completes.If the reset fetch returns fewer than
limitPRs, there's a brief moment wherehasMorePRsis incorrectlytrue. The useEffect will correct it, but consider movingsetHasMorePRs(true)into theupdateQuerycallback for more accurate state.Suggested improvement
onResetPullRequests={ hasMorePRs ? undefined : () => { - setHasMorePRs(true) fetchMore({ variables: { programKey, moduleKey, offset: 0, limit, }, updateQuery: (prevResult, { fetchMoreResult }) => { if (!fetchMoreResult) return prevResult + const newPRs = fetchMoreResult.getModule?.recentPullRequests || [] + setHasMorePRs(newPRs.length >= limit) return { ...prevResult, getModule: { ...prevResult.getModule, - recentPullRequests: fetchMoreResult.getModule?.recentPullRequests || [], + recentPullRequests: newPRs, }, } }, }) } }frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (3)
389-389: Uselimitconstant instead of hardcoded4.The condition uses a hardcoded
4instead of thelimitconstant defined on line 32, which could lead to inconsistencies if the limit value changes.Suggested fix
- {!hasMorePRs && (issue.pullRequests || []).length > 4 && ( + {!hasMorePRs && (issue.pullRequests || []).length > limit && (
348-387: Consider adding loading state during pagination operations.The Show more/Show less buttons have no visual feedback while
fetchMoreis in progress. Users may click multiple times thinking the action didn't register.Consider tracking a
loadingMorestate to disable the button and/or show a spinner.Example approach
const [loadingMore, setLoadingMore] = useState(false) // In the button click handler: onClick={async () => { setLoadingMore(true) try { await fetchMore({ ... }) } finally { setLoadingMore(false) } }} // In the button: <button disabled={loadingMore} ...> {loadingMore ? 'Loading...' : 'Show more'} </button>Also applies to: 389-423
350-385: Addtype="button"attribute to prevent default form submission behavior.While these buttons aren't currently inside a form, adding
type="button"is a defensive best practice that prevents accidental form submissions if the component structure changes.Suggested fix
<button + type="button" onClick={() => {Also applies to: 391-421
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
Outdated
Show resolved
Hide resolved
|
@cubic-dev-ai review this pull request |
@HarshitVerma109 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx">
<violation number="1" location="frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx:351">
P2: The "Show less" button unnecessarily re-fetches data from the server when the first 4 PRs are already available locally. Consider maintaining a local display state and slicing the existing array instead of making an API call:
```javascript
const [showAllPRs, setShowAllPRs] = useState(false)
// In render:
{(issue.pullRequests || []).slice(0, showAllPRs ? undefined : limit).map(...)}
This avoids the network round-trip when collapsing the list.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Outdated
Show resolved
Hide resolved
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx
Outdated
Show resolved
Hide resolved
|
@cubic-dev-ai review this pull request |
@HarshitVerma109 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 16 files
Confidence score: 3/5
- Potential performance risk:
backend/apps/github/api/internal/nodes/issue.pyclampslimitonly to >=0, so unbounded requests could trigger very large DB queries - Score reflects a medium-severity, user-impacting risk if clients request huge result sets; otherwise change seems contained
- Pay close attention to
backend/apps/github/api/internal/nodes/issue.py- add an upper bound (e.g., sharednormalize_limit/MAX_LIMIT) to prevent heavy queries
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/apps/github/api/internal/nodes/issue.py">
<violation number="1" location="backend/apps/github/api/internal/nodes/issue.py:31">
P2: The new resolver only clamps `limit` to >= 0; without an upper bound, clients can request extremely large result sets and trigger heavy DB queries. Consider using the shared `normalize_limit`/`MAX_LIMIT` pattern (as in RepositoryNode) to cap pagination limits.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai review this pull request |
@HarshitVerma109 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 16 files
Confidence score: 3/5
- Pagination errors in
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsxare swallowed whenfetchMorefails, so users may see a stuck or incomplete list with no feedback - This is a concrete user-facing reliability gap (medium severity) which introduces some regression risk despite being localized
- Pay close attention to
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx- add error handling or retry for failed pagination.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx">
<violation number="1" location="frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx:390">
P2: Missing error handling for `fetchMore`. If the pagination request fails, the error will be silently swallowed with no user feedback. Add a `.catch()` handler to display an error or retry.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Outdated
Show resolved
Hide resolved
|
@cubic-dev-ai review this pull request |
@HarshitVerma109 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 16 files
Confidence score: 4/5
- Minor UX risk: in
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsxthe “load more PRs” button doesn’t disable or show loading state whileisFetchingMoreis true, which can allow repeated clicks during fetches - Overall risk is low since this is a UI feedback issue rather than core logic or data integrity
- Pay close attention to
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx- ensure the button disables or indicates loading while fetching more PRs.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx">
<violation number="1" location="frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx:355">
P2: Button should be disabled and show loading feedback while fetching more PRs. Currently `isFetchingMore` is only checked inside onClick but the button has no `disabled` attribute or loading indicator, leaving users without feedback during network requests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Outdated
Show resolved
Hide resolved
|
@cubic-dev-ai review this pull request |
@HarshitVerma109 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 16 files
Confidence score: 4/5
- Overall risk looks low-to-moderate; the issues are mid-severity (5/10) and mostly about UX/perf rather than correctness, so this feels safe to merge with awareness.
frontend/src/components/CardDetailsPage.tsxcould allow a “Show less” click duringisFetchingMore, potentially creating confusing UI or racey resets under active fetch.backend/apps/github/api/internal/nodes/issue.pymay incur N+1 queries withoutprefetch_related, which could degrade performance under load.- Pay close attention to
frontend/src/components/CardDetailsPage.tsxandbackend/apps/github/api/internal/nodes/issue.py- avoid fetch race UX and possible N+1 query behavior.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/CardDetailsPage.tsx">
<violation number="1" location="frontend/src/components/CardDetailsPage.tsx:402">
P2: The "Show less" button should be disabled while `isFetchingMore` is true to prevent race conditions or confusing UX when resetting during an active fetch operation.</violation>
</file>
<file name="backend/apps/github/api/internal/nodes/issue.py">
<violation number="1" location="backend/apps/github/api/internal/nodes/issue.py:31">
P2: Missing `prefetch_related` may cause N+1 query performance issues. Other methods in this file (e.g., `is_merged`) that access `pull_requests` use `@strawberry_django.field(prefetch_related=["pull_requests"])`. Consider using the same pattern for consistency and optimization.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai review this pull request |
@HarshitVerma109 I have started the AI code review. It will take a few minutes to complete. |
|
@arkid15r , please review it and let me know if any changes are required. |
HarshitVerma109
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
a68560d
|
|
@arkid15r, I’ve resolved the merge conflicts. Please review the changes and let me know if any further adjustments are needed. |
1 participant
Proposed change
Resolves #3573
This PR implements pagination for Pull Requests to fix visibility limits on the Issue Details Page and Public Module Page.
Previously, these lists were capped at a fixed number (4 or 5) with no way to view the rest.
Key Changes:
fetchMoreto load additional PRs on demand without page refreshes.limitandoffsetarguments for efficient data fetching.Checklist
make check-testlocally: all warnings addressed, tests passed