Add tag display to issue cards on contribute page #3158
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR implements tag display functionality on issue cards in the contribute page. Backend changes add Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
|
![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
🧹 Nitpick comments (3)
backend/apps/github/models/mixins/issue.py (1)
72-75: Suggest refactoring to reduce code duplication.The new
idx_tagsproperty is identical toidx_project_tags(lines 68-70). While this alias may improve clarity for frontend consumers, consider refactoring to avoid duplication:🔎 Proposed refactor
@property def idx_project_tags(self) -> list[str]: """Return project tags for indexing.""" return self.project.idx_tags if self.project else [] @property def idx_tags(self) -> list[str]: - """Return tags for indexing (from project).""" - return self.project.idx_tags if self.project else [] + """Return tags for indexing (alias for idx_project_tags).""" + return self.idx_project_tagsfrontend/src/components/Card.tsx (2)
142-154: Use tag value as key instead of index and fix formatting.The current implementation has a few refinements needed:
- Index as key: Using
indexas a key can cause React rendering issues if tags are reordered or filtered. Use the tag value itself as the key.- Inaccurate comment: The comment states "key =tag-1 ,2, 3 ..." but indices start at 0, so it would be "tag-0", "tag-1", "tag-2".
- Formatting inconsistency: The className string has irregular spacing and line breaks.
🔎 Proposed fixes
{/* Tags Section */} {displayTags && displayTags.length > 0 && ( - <div className=" flex flex-wrap gap-2"> + <div className="flex flex-wrap gap-2"> {displayTags.slice(0, 3).map((tag, index) => ( <span - key={`tag-${index}`} //key =tag-1 ,2, 3 ... - className={`flex items-center gap-2 px-2 py-1 rounded-md border border-1 transition-all whitespace-nowrap justify-center bg-transparent text-zinc-300 hover:text-white - ${index >= 2 ? 'hidden sm:flex' : '' - }`} + key={tag} + className={`flex items-center gap-2 px-2 py-1 rounded-md border border-1 transition-all whitespace-nowrap justify-center bg-transparent text-zinc-300 hover:text-white ${ + index >= 2 ? 'hidden sm:flex' : '' + }`} > {tag} </span>
177-177: Fix spacing in closing tag.Minor formatting issue: there's an extra space before the closing angle bracket.
- </div > + </div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/apps/core/utils/index.pybackend/apps/github/index/registry/issue.pybackend/apps/github/models/mixins/issue.pybackend/apps/owasp/index/search/issue.pyfrontend/__tests__/unit/components/Card.test.tsxfrontend/src/app/contribute/page.tsxfrontend/src/components/Card.tsxfrontend/src/types/card.tsfrontend/src/types/issue.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/Card.test.tsx
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/apps/github/index/registry/issue.pybackend/apps/core/utils/index.pybackend/apps/github/models/mixins/issue.pybackend/apps/owasp/index/search/issue.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/apps/github/index/registry/issue.pybackend/apps/core/utils/index.pybackend/apps/github/models/mixins/issue.pybackend/apps/owasp/index/search/issue.py
📚 Learning: 2026-01-01T18:57:05.007Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/video.py:189-215
Timestamp: 2026-01-01T18:57:05.007Z
Learning: In the OWASP backend area, maintain the established pattern: when dealing with sponsors, include all entries from Sponsor.objects.all() (including NOT_SPONSOR) and perform in-memory sorting using the same criteria/pattern used by the GraphQL sponsor query implemented in backend/apps/owasp/api/internal/queries/sponsor.py. Apply this behavior consistently to files in backend/apps/owasp (not just video.py), and ensure code paths that render sponsor lists follow this in-code sorting approach rather than pre-filtering NOT_SPONSOR entries before sorting.
Applied to files:
backend/apps/owasp/index/search/issue.py
🧬 Code graph analysis (3)
frontend/src/app/contribute/page.tsx (1)
backend/tests/apps/github/models/issue_test.py (1)
issue(26-28)
frontend/src/components/Card.tsx (1)
frontend/src/types/card.ts (1)
CardProps(18-36)
backend/apps/github/models/mixins/issue.py (2)
backend/apps/owasp/models/mixins/common.py (1)
idx_tags(33-35)backend/apps/github/models/repository.py (1)
project(162-164)
🔇 Additional comments (7)
backend/apps/core/utils/index.py (1)
123-123: LGTM! Clean addition of idx_tags to issues index.The addition of
"idx_tags"to theattributesToRetrievelist is consistent with the existing pattern (chapters index already has this field at line 142) and maintains alphabetical ordering.backend/apps/owasp/index/search/issue.py (1)
45-45: LGTM! Consistent addition of idx_tags to search defaults.The addition of
"idx_tags"to the defaultattributesToRetrievelist maintains alphabetical ordering and aligns with the corresponding change inbackend/apps/core/utils/index.py.frontend/src/types/issue.ts (1)
24-24: LGTM! Clean type extension for tags support.The addition of the optional
tags?: string[]property to theIssuetype is correct and maintains type safety. The property name aligns with the backendidx_tagsfield (camelized as per thedeep_camelizeutility).frontend/src/types/card.ts (1)
35-35: LGTM! Consistent type extension for tags support.The addition of the optional
tags?: string[]property toCardPropsis correct and aligns with theIssuetype extension. Note thatDetailsCardPropsalready hastags?: string[]at line 79, so this change brings consistency across card-related types.backend/apps/github/index/registry/issue.py (1)
33-33: LGTM! Theidx_tagsproperty is properly implemented.The addition of
"idx_tags"to theIssueIndex.fieldstuple is correct and maintains alphabetical ordering. Theidx_tagsproperty is properly implemented onIssueIndexMixinwith the correct type annotation (list[str]) and returnsself.project.idx_tagswith a sensible fallback to an empty list if no project is associated.frontend/src/app/contribute/page.tsx (1)
54-54: LGTM!The tags prop is correctly passed from the issue data to the Card component.
frontend/__tests__/unit/components/Card.test.tsx (1)
491-527: LGTM!The test coverage for tag functionality is comprehensive and properly validates:
- Basic tag rendering
- Priority tag filtering behavior
- Display limit truncation to 3 tags
All assertions correctly verify the expected behavior.
| const filteredTags = tags?.filter(tag => { | ||
| const lowerTag = tag.toLowerCase() | ||
| //tag priority to be set by the devs | ||
| return lowerTag.includes('good first issue') || lowerTag.includes('help wanted') || lowerTag.includes('tag-4') || lowerTag.includes('backend') | ||
| }) || [] | ||
| // If no filtered tags, use tags prop directly | ||
| const displayTags = filteredTags.length > 0 ? filteredTags : (tags || []) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor hardcoded priority tags to a configurable constant and use exact matching.
The current implementation has several concerns:
- Hardcoded priority list: The priority tags are embedded in the component logic, making them difficult to maintain and configure.
- Overly broad matching: Using
includes()could match unintended tags (e.g., "good first issue" would match "not a good first issue"). - Placeholder values: "tag-4" and "backend" appear to be temporary placeholders based on the PR description.
- Redundant fallback: Line 35 has
|| []but line 33 already provides the same fallback.
🔎 Proposed refactor
Define a constant at the top of the file:
+// Priority tags configuration - update this list to control which tags are displayed
+const PRIORITY_TAGS = ['good first issue', 'help wanted', 'backend'] as const
+
const Card = ({Then use exact matching with the constant:
- const filteredTags = tags?.filter(tag => {
- const lowerTag = tag.toLowerCase()
- //tag priority to be set by the devs
- return lowerTag.includes('good first issue') || lowerTag.includes('help wanted') || lowerTag.includes('tag-4') || lowerTag.includes('backend')
- }) || []
- // If no filtered tags, use tags prop directly
- const displayTags = filteredTags.length > 0 ? filteredTags : (tags || [])
+ const filteredTags = tags?.filter(tag =>
+ PRIORITY_TAGS.some(priority => tag.toLowerCase() === priority.toLowerCase())
+ ) || []
+
+ // If no priority tags matched, show all tags
+ const displayTags = filteredTags.length > 0 ? filteredTags : tagsNote: Coordinate with maintainers on the final priority tag list before merging, as mentioned in the PR objectives.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/Card.tsx around lines 29 to 35, the component uses
hardcoded priority tags and broad includes() matching; extract a configurable
constant (e.g., PRIORITY_TAGS) at the top of the file and replace the inline
literals with that constant, perform exact case-insensitive matching by
normalizing tag to lower-case and checking equality against the set (avoid
includes()), remove the redundant || [] fallback (keep a single fallback when
computing filteredTags) and ensure placeholder tags like "tag-4"/"backend" are
easily editable or removed per maintainer guidance.
There was a problem hiding this comment.
@Mr-Rahul-Paul This seems like a valid point. I'm not sure if I like the idea of hardcoding label values that we check with. These could be anything set by the project owners.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@Mr-Rahul-Paul hi! Requested changes.
And please run make check-test locally and address found issues - this is required for all PRs.
| const filteredTags = tags?.filter(tag => { | ||
| const lowerTag = tag.toLowerCase() | ||
| //tag priority to be set by the devs | ||
| return lowerTag.includes('good first issue') || lowerTag.includes('help wanted') || lowerTag.includes('tag-4') || lowerTag.includes('backend') | ||
| }) || [] | ||
| // If no filtered tags, use tags prop directly | ||
| const displayTags = filteredTags.length > 0 ? filteredTags : (tags || []) |
There was a problem hiding this comment.
@Mr-Rahul-Paul This seems like a valid point. I'm not sure if I like the idea of hardcoding label values that we check with. These could be anything set by the project owners.
There was a problem hiding this comment.
There are also 2 new issues introduced in this PR
Please resolve those before requesting a review in the future
| </div> | ||
| </div> | ||
| </div> | ||
| </div > |
There was a problem hiding this comment.
| </div > | |
| </div> |
| ))} | ||
| </div> | ||
|
|
||
|
|
|
|
||
|
|
| title: string | ||
| updatedAt?: string | number | ||
| url: string | ||
| tags?: string[] |
There was a problem hiding this comment.
That's not the best place to have this property listed. Notice the order of items ⏫
| tooltipLabel?: string | ||
| topContributors?: Contributor[] | ||
| url: string | ||
| tags?: string[] |
There was a problem hiding this comment.
Same here - update the order.
2 participants
Fixes #2962
Summary
This PR implements the display of GitHub issue labels #2962 on issue cards within the
/contributepage. Tags are fetched from the Algolia index and rendered as badge-like elements, improving issue discoverability for contributors.Feature:
screenshots
pc:
mobile:
Implementation Details
Frontend: Extended Card component to support priority-based tag rendering with a fallback to raw tags. Includes test coverage for tag filtering, sorting, and display limits.
Backend:
backend/apps/github/models/mixins/issue.py: Addedidx_tagsproperty method to return tags from the associated projectbackend/apps/github/index/registry/issue.py: Addedidx_tagsto thefieldstuple for Algolia indexingbackend/apps/core/utils/index.py: Addedidx_tagstoattributesToRetrievelist for the issues indexbackend/apps/owasp/index/search/issue.py: Addedidx_tagsto the defaultattributesToRetrievelist inget_issuesfunctionDiscussion Points
Tag Display Implementation
Tags are fully implemented and functional. The current implementation includes:
I have worked on sorting and filtering functionality that can be enabled if maintainers want. The priority tags can be manually selected based on project requirements.
Current placeholder priority tags:
good first issuehelp wantedbackendtag-4(placeholder for testing)Decision needed: Which tags should be prioritized for display? [TBD!]
Responsiveness
Current behavior shows 2 tags on mobile and 3 (or 4) on larger screens. Please confirm if this meets the design requirements or if adjustments are needed.
Migration Required
For production deployment, the following migration steps will be required:
idx_tagsattribute to thelocal_issuesindex schemaidx_tagsfor all existing issuesThis PR prepares the frontend and backend code but does not execute the migration. Migration can be coordinated with maintainers to avoid index disruption.
I can also work on the reindexing process and GitHub REST API integration for fetching real labels, if required, under guidance from maintainers.