Optimize GitHub UserNode#5041
Merged
arkid15r merged 5 commits intoJun 29, 2026
Merged
Conversation
Contributor
WalkthroughAdds user dataloaders, registers them in the GitHub dataloader map, changes ChangesGitHub user dataloaders and fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/graphql-dataloaders #5041 +/- ##
============================================================
Coverage 98.74% 98.74%
============================================================
Files 543 544 +1
Lines 17138 17160 +22
Branches 2425 2425
============================================================
+ Hits 16923 16945 +22
Misses 123 123
Partials 92 92
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/github/api/internal/nodes/user.py (1)
85-102: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
distinct=Trueto both count annotations.Line 85 and Line 99 can return inflated values when a query asks for
issues_countandreleases_counttogether. Django will join both reverse relations on the sameUserqueryset, so the counts multiply each other (for example, 2 issues × 3 releases becomes 6/6).Suggested fix
- `@strawberry_django.field`(annotate={"issues_count": Count("created_issues")}) + `@strawberry_django.field`( + annotate={"issues_count": Count("created_issues", distinct=True)} + ) def issues_count(self, root: User) -> int: """Resolve issues count.""" return root.issues_count @@ - `@strawberry_django.field`(annotate={"releases_count": Count("created_releases")}) + `@strawberry_django.field`( + annotate={"releases_count": Count("created_releases", distinct=True)} + ) def releases_count(self, root: User) -> int: """Resolve releases count.""" return root.releases_count🤖 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 `@backend/apps/github/api/internal/nodes/user.py` around lines 85 - 102, Add distinct counting to the annotated fields in the User node so `issues_count` and `releases_count` do not multiply when both are requested together. Update the `strawberry_django.field` annotations on `issues_count` and `releases_count` in `User` to use `Count(..., distinct=True)` for the `created_issues` and `created_releases` relations, keeping the existing resolver methods unchanged.
🤖 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 `@backend/apps/github/api/internal/dataloaders/release.py`:
- Around line 12-20: The release URL mapping in the dataloader duplicates the
URL construction logic instead of reusing the existing Release.url property.
Update the async loop in the release dataloader to use release.url with the same
empty-string fallback, and keep the select_related("repository__owner") prefetch
so Repository.url can still resolve owner.login without extra queries.
---
Outside diff comments:
In `@backend/apps/github/api/internal/nodes/user.py`:
- Around line 85-102: Add distinct counting to the annotated fields in the User
node so `issues_count` and `releases_count` do not multiply when both are
requested together. Update the `strawberry_django.field` annotations on
`issues_count` and `releases_count` in `User` to use `Count(..., distinct=True)`
for the `created_issues` and `created_releases` relations, keeping the existing
resolver methods unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5c217fb-f0df-4160-85f9-113a4ba8a338
📒 Files selected for processing (18)
backend/apps/common/api/internal/dataloaders/utils.pybackend/apps/github/api/internal/dataloaders/__init__.pybackend/apps/github/api/internal/dataloaders/release.pybackend/apps/github/api/internal/dataloaders/repository.pybackend/apps/github/api/internal/dataloaders/user.pybackend/apps/github/api/internal/nodes/release.pybackend/apps/github/api/internal/nodes/user.pybackend/apps/mentorship/api/internal/dataloaders/__init__.pybackend/apps/mentorship/api/internal/dataloaders/interested_users.pybackend/settings/graphql_context.pybackend/tests/unit/apps/common/api/internal/dataloaders/utils_test.pybackend/tests/unit/apps/github/api/internal/dataloaders/__init__.pybackend/tests/unit/apps/github/api/internal/dataloaders/release_test.pybackend/tests/unit/apps/github/api/internal/dataloaders/repository_test.pybackend/tests/unit/apps/github/api/internal/dataloaders/user_test.pybackend/tests/unit/apps/github/api/internal/nodes/release_test.pybackend/tests/unit/apps/github/api/internal/nodes/user_test.pybackend/tests/unit/apps/mentorship/api/internal/dataloaders/interested_users_test.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 18 files
Confidence score: 4/5
- In
backend/apps/github/api/internal/dataloaders/release.py, using""as a missing-value sentinel instead ofNoneis inconsistent with sibling loaders and can cause subtle downstream truthiness/typing bugs in API consumers if merged as-is — align missing release/repository values toNone(or document and enforce a single sentinel) before merging. backend/tests/unit/apps/github/api/internal/dataloaders/repository_test.pydoes not cover theOWASP_ORGANIZATION_NAMEprefix-removal mapping path, so a regression there could ship unnoticed and return incorrect project names to clients — add a focused unit test for that mapping behavior before merge.- In
backend/apps/github/api/internal/dataloaders/release.py, rebuilding the release URL inline duplicatesRelease.url, which raises drift risk if URL rules change later and can lead to inconsistent links — switch toRelease.urlto keep URL generation single-sourced.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/github/api/internal/dataloaders/release.py">
<violation number="1" location="backend/apps/github/api/internal/dataloaders/release.py:17">
P3: The inline URL construction `f"{release.repository.url}/releases/tag/{release.tag_name}"` duplicates the logic already defined in `Release.url`. Since `select_related("repository__owner")` is already applied (which `Repository.url` needs to resolve `owner.login`), you can simplify this to `release.url` with the empty-string fallback. This keeps the URL format defined in one place.</violation>
<violation number="2" location="backend/apps/github/api/internal/dataloaders/release.py:19">
P2: DataLoader uses empty string sentinel for missing releases/repositories instead of None, inconsistent with sibling fields and codebase patterns</violation>
</file>
<file name="backend/tests/unit/apps/github/api/internal/dataloaders/repository_test.py">
<violation number="1" location="backend/tests/unit/apps/github/api/internal/dataloaders/repository_test.py:140">
P2: Tests do not cover the OWASP_ORGANIZATION_NAME prefix-removal behavior in project name mapping.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| mapping[release.pk] = ( | ||
| f"{release.repository.url}/releases/tag/{release.tag_name}" | ||
| if release.repository | ||
| else "" |
Contributor
There was a problem hiding this comment.
P2: DataLoader uses empty string sentinel for missing releases/repositories instead of None, inconsistent with sibling fields and codebase patterns
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/github/api/internal/dataloaders/release.py, line 19:
<comment>DataLoader uses empty string sentinel for missing releases/repositories instead of None, inconsistent with sibling fields and codebase patterns</comment>
<file context>
@@ -0,0 +1,31 @@
+ mapping[release.pk] = (
+ f"{release.repository.url}/releases/tag/{release.tag_name}"
+ if release.repository
+ else ""
+ )
+
</file context>
| """Returns project names in order of release_ids.""" | ||
| release_ids = [1, 2, 3] | ||
| project_1 = MagicMock() | ||
| project_1.name = "Project Alpha" |
Contributor
There was a problem hiding this comment.
P2: Tests do not cover the OWASP_ORGANIZATION_NAME prefix-removal behavior in project name mapping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/unit/apps/github/api/internal/dataloaders/repository_test.py, line 140:
<comment>Tests do not cover the OWASP_ORGANIZATION_NAME prefix-removal behavior in project name mapping.</comment>
<file context>
@@ -0,0 +1,249 @@
+ """Returns project names in order of release_ids."""
+ release_ids = [1, 2, 3]
+ project_1 = MagicMock()
+ project_1.name = "Project Alpha"
+ project_2 = MagicMock()
+ project_2.name = "Project Beta"
</file context>
| mapping: dict[int, str] = {} | ||
| async for release in releases: | ||
| mapping[release.pk] = ( | ||
| f"{release.repository.url}/releases/tag/{release.tag_name}" |
Contributor
There was a problem hiding this comment.
P3: The inline URL construction f"{release.repository.url}/releases/tag/{release.tag_name}" duplicates the logic already defined in Release.url. Since select_related("repository__owner") is already applied (which Repository.url needs to resolve owner.login), you can simplify this to release.url with the empty-string fallback. This keeps the URL format defined in one place.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/github/api/internal/dataloaders/release.py, line 17:
<comment>The inline URL construction `f"{release.repository.url}/releases/tag/{release.tag_name}"` duplicates the logic already defined in `Release.url`. Since `select_related("repository__owner")` is already applied (which `Repository.url` needs to resolve `owner.login`), you can simplify this to `release.url` with the empty-string fallback. This keeps the URL format defined in one place.</comment>
<file context>
@@ -0,0 +1,31 @@
+ mapping: dict[int, str] = {}
+ async for release in releases:
+ mapping[release.pk] = (
+ f"{release.repository.url}/releases/tag/{release.tag_name}"
+ if release.repository
+ else ""
</file context>
ahmedxgouda
force-pushed
the
dataloaders/github-user
branch
from
5df7421 to
5f44fca
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/github/api/internal/nodes/user.py (1)
85-102: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
distinct=Trueon bothCount(...)annotations.
issues_countandreleases_countare reverse FK counts, so selecting them together can multiply rows through the shared join and inflate both totals.distinct=Trueavoids the overcount.🤖 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 `@backend/apps/github/api/internal/nodes/user.py` around lines 85 - 102, Add distinct counting to both reverse relation annotations in the User resolver: update the issues_count and releases_count fields in the User class so their Count("created_issues") and Count("created_releases") annotations use distinct=True. This fixes the row multiplication when both annotated counts are selected together, while keeping the existing resolver methods unchanged.
🤖 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.
Outside diff comments:
In `@backend/apps/github/api/internal/nodes/user.py`:
- Around line 85-102: Add distinct counting to both reverse relation annotations
in the User resolver: update the issues_count and releases_count fields in the
User class so their Count("created_issues") and Count("created_releases")
annotations use distinct=True. This fixes the row multiplication when both
annotated counts are selected together, while keeping the existing resolver
methods unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 113f5d7a-71e1-4889-bb92-b39abb05662a
📒 Files selected for processing (5)
backend/apps/github/api/internal/dataloaders/__init__.pybackend/apps/github/api/internal/dataloaders/user.pybackend/apps/github/api/internal/nodes/user.pybackend/tests/unit/apps/github/api/internal/dataloaders/user_test.pybackend/tests/unit/apps/github/api/internal/nodes/user_test.py
Contributor
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
arkid15r
approved these changes
Jun 29, 2026
arkid15r
merged commit b2cbe07
into
OWASP:feature/graphql-dataloaders
31 of 32 checks passed
|
ahmedxgouda
added a commit
to ahmedxgouda/Nest
that referenced
this pull request
Jun 29, 2026
* Add optimization hints * Add dataloader * Add and update tests * Replace optimization hints with dataloaders and optimize author field in IssueNode
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.
Proposed change
Resolves #4600
GitHub.UserNode.mp4
Checklist
make check-testlocally: all warnings addressed, tests passed