Repository navigation
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. WalkthroughCard components now accept and render an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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
| 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.
Co-authored-by: Kate Golovanova <kate@kgthreads.com>
Co-authored-by: Kate Golovanova <kate@kgthreads.com>
Co-authored-by: Kate Golovanova <kate@kgthreads.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/Card.tsx (1)
29-35: The hardcoded priority tags concern was previously raised.The maintainer kasya has already acknowledged concerns about hardcoded label values in the previous review thread. This should be addressed per that discussion before merging.
Additionally, the comment on line 31 could be more professional and specific.
Optional: improve comment clarity
- //tag priority to be set by the devs + // Priority tags - configure PRIORITY_TAGS constant to control display
🧹 Nitpick comments (1)
frontend/src/components/Card.tsx (1)
139-153: Clean up minor code quality issues.Several small improvements would enhance maintainability:
- Line 140: The
displayTags &&check is redundant sincedisplayTagsis always an array (guaranteed by the|| []fallback on line 35).- Line 141: Remove the extra leading space in
className=" flex".- Line 144: The comment doesn't add meaningful context—React key usage is self-explanatory.
- Lines 145-147: The multi-line className is difficult to read and maintain.
🔎 Suggested improvements
- {displayTags && displayTags.length > 0 && ( - <div className=" flex flex-wrap gap-2"> + {displayTags.length > 0 && ( + <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-${index}`} + 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>Note: These changes may help resolve the SonarCloud issues mentioned by the maintainer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Card.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
frontend/src/components/Card.tsx
📚 Learning: 2025-08-16T13:23:34.874Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2021
File: backend/apps/slack/models/google_auth.py:25-36
Timestamp: 2025-08-16T13:23:34.874Z
Learning: In the OWASP Nest project, encryption and decryption for Google OAuth tokens stored in BinaryField are being handled in a separate PR, not in the initial Google Auth setup implementation.
Applied to files:
frontend/src/components/Card.tsx
🧬 Code graph analysis (1)
frontend/src/components/Card.tsx (1)
frontend/src/types/card.ts (1)
CardProps(18-36)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/components/Card.tsx (1)
29-41: Address the hardcoded priority tags and matching logic.This filtering logic has been discussed in previous reviews. The concerns remain:
- Priority tags are hardcoded and include placeholders ("tag-4", "backend")
- Using
includes()for matching is overly broad and could match unintended substrings- Redundant fallback operators (
|| []on both lines 39 and 41)- Line 32: Comment should have a space after
//Per the past discussion, this needs coordination with maintainers on the final priority tag list and should use exact case-insensitive matching with a configurable constant.
🧹 Nitpick comments (1)
frontend/src/components/Card.tsx (1)
149-149: Improve comment clarity.The comment
//key =tag-1 ,2, 3 ...is unclear and has formatting issues. Either make it more descriptive or remove it since the code is self-explanatory.🔎 Suggested fix
- key={`tag-${index}`} //key =tag-1 ,2, 3 ... + key={`tag-${index}`}Or if keeping the comment:
- key={`tag-${index}`} //key =tag-1 ,2, 3 ... + key={`tag-${index}`} // Unique key for each tag element
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/tests/apps/core/utils/match_test.pyfrontend/__tests__/unit/components/Card.test.tsxfrontend/src/components/Card.tsxfrontend/src/types/card.tsfrontend/src/types/issue.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/types/issue.ts
- frontend/tests/unit/components/Card.test.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
frontend/src/components/Card.tsx
📚 Learning: 2025-08-16T13:23:34.874Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2021
File: backend/apps/slack/models/google_auth.py:25-36
Timestamp: 2025-08-16T13:23:34.874Z
Learning: In the OWASP Nest project, encryption and decryption for Google OAuth tokens stored in BinaryField are being handled in a separate PR, not in the initial Google Auth setup implementation.
Applied to files:
frontend/src/components/Card.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/tests/apps/core/utils/match_test.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/tests/apps/core/utils/match_test.py
🧬 Code graph analysis (1)
frontend/src/components/Card.tsx (1)
frontend/src/types/card.ts (1)
CardProps(18-36)
🔇 Additional comments (3)
backend/tests/apps/core/utils/match_test.py (1)
21-21: LGTM! Test expectation correctly updated.The addition of
idx_tagsto the expectedattributesToRetrievelist for the issues index is correct and maintains alphabetical ordering. This aligns with the broader PR changes that add idx_tags to issue indexing and retrieval throughout the backend.Note: The chapters index (line 48) already includes
idx_tags, so this change brings consistency across index types.frontend/src/types/card.ts (1)
27-27: Clarify and apply the property ordering convention.A previous review comment indicates the property order should be updated. Please clarify the project's convention for ordering properties in type definitions (e.g., alphabetical, logical grouping, required-then-optional) and reorder accordingly.
frontend/src/components/Card.tsx (1)
27-27: LGTM!The
tagsprop is correctly added to the component signature, matching the updatedCardPropstype.
| {/* Tags Section */} | ||
| {displayTags && displayTags.length > 0 && ( | ||
| <div className="flex flex-wrap gap-2"> | ||
| {displayTags.slice(0, 3).map((tag, index) => ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the hardcoded tag display limit to a named constant.
The magic number 3 should be defined as a named constant at the top of the file for maintainability and to make the responsive behavior (2 on mobile, 3 on desktop) more explicit.
🔎 Proposed refactor
+// Maximum number of tags to display (2 visible on mobile, 3 on desktop)
+const MAX_DISPLAY_TAGS = 3
+
const Card = ({Then use the constant:
- {displayTags.slice(0, 3).map((tag, index) => (
+ {displayTags.slice(0, MAX_DISPLAY_TAGS).map((tag, index) => (📝 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.
| {displayTags.slice(0, 3).map((tag, index) => ( | |
| // Maximum number of tags to display (2 visible on mobile, 3 on desktop) | |
| const MAX_DISPLAY_TAGS = 3 | |
| const Card = ({ | |
| // ... component props ... | |
| // Later in the render section: | |
| {displayTags.slice(0, MAX_DISPLAY_TAGS).map((tag, index) => ( |
🤖 Prompt for AI Agents
In frontend/src/components/Card.tsx around line 147, the tag display slice uses
a magic number (3); define a named constant (e.g., TAG_DISPLAY_LIMIT_DESKTOP = 3
and/or a responsive TAG_DISPLAY_LIMIT) at the top of the file and replace the
hardcoded 3 with that constant in the slice call; if responsive behavior is
required, add a small helper or derive the constant based on viewport/isMobile
prop and use that value here so the 2-on-mobile / 3-on-desktop intent is
explicit and maintainable.
There was a problem hiding this comment.
@Mr-Rahul-Paul also these comments by Rabbit seems reasonable.
It's required for you to address all comments before we review. I can see this one (and the one above) shows as outdated, but the code is the same 🤔
Do you think you could address these?
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!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/src/components/Card.tsx (3)
133-133: Magic number should be extracted to a named constant.As noted in previous reviews, the hardcoded
3should be extracted to a named constant for maintainability.Please refer to the previous review suggestion to define a constant like
MAX_DISPLAY_TAGS = 3at the top of the file.
131-144: Add semantic HTML and ARIA attributes for accessibility.As flagged in previous reviews, the tags section lacks semantic structure and ARIA attributes for screen reader users.
Please refer to the previous review suggestion to wrap tags in a semantic container (e.g.,
<ul>withrole="list"andaria-label="Issue tags").
135-135: Fix: Use index-based key to prevent React warnings with duplicate tags.Using the tag value as the key will cause React to throw warnings and potentially break rendering if the tags array contains duplicates. Keys should be unique and stable.
🔎 Proposed fix
{tags.slice(0, 3).map((tag, index) => ( <span - key={tag} + key={`tag-${index}`} className={`flex items-center justify-center gap-2 rounded-md border border-1 bg-transparent px-2 py-1 whitespace-nowrap text-zinc-300 transition-all hover:text-white ${
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/Card.test.tsx (1)
491-513: Consider adding edge case tests for tag handling.The current tests cover basic rendering and truncation, but miss important edge cases that would improve coverage:
Suggested additional test cases
it('does not render tags section when tags is undefined', () => { const propsWithoutTags = { ...baseProps, tags: undefined, } render(<Card {...propsWithoutTags} />) // Verify tags section doesn't appear expect(screen.queryByText('good first issue')).not.toBeInTheDocument() }) it('does not render tags section when tags array is empty', () => { const propsWithEmptyTags = { ...baseProps, tags: [], } render(<Card {...propsWithEmptyTags} />) // Verify no tags are rendered expect(screen.queryByText('tag1')).not.toBeInTheDocument() }) it('renders single tag correctly', () => { const propsWithOneTag = { ...baseProps, tags: ['bug'], } render(<Card {...propsWithOneTag} />) expect(screen.getByText('bug')).toBeInTheDocument() })Note: Testing the responsive behavior (2 tags on mobile, 3 on desktop) would require additional viewport mocking and is acceptable to defer to integration/E2E tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/Card.test.tsxfrontend/src/components/Card.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
frontend/src/components/Card.tsx
📚 Learning: 2025-08-16T13:23:34.874Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2021
File: backend/apps/slack/models/google_auth.py:25-36
Timestamp: 2025-08-16T13:23:34.874Z
Learning: In the OWASP Nest project, encryption and decryption for Google OAuth tokens stored in BinaryField are being handled in a separate PR, not in the initial Google Auth setup implementation.
Applied to files:
frontend/src/components/Card.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/src/components/Card.tsx
🔇 Additional comments (1)
frontend/src/components/Card.tsx (1)
27-27: LGTM!The
tagsprop is correctly added to the component signature and properly typed inCardProps.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Hi @Mr-Rahul-Paul !
Left requests ⬇️
| @property | ||
| def idx_tags(self) -> list[str]: | ||
| """Return tags for indexing (from project).""" | ||
| return self.project.idx_tags if self.project else [] | ||
|
|
There was a problem hiding this comment.
@Mr-Rahul-Paul do we really need this? Isn't it identical to idx_project_tags() above?
| {/* Tags Section */} | ||
| {tags && tags.length > 0 && ( | ||
| <div className="flex flex-wrap gap-2"> | ||
| {tags.slice(0, 3).map((tag, index) => ( | ||
| <span | ||
| key={tag} | ||
| className={`flex items-center justify-center gap-2 rounded-md border border-1 bg-transparent px-2 py-1 whitespace-nowrap text-zinc-300 transition-all hover:text-white ${ | ||
| index >= 2 ? 'hidden sm:flex' : '' | ||
| }`} | ||
| > | ||
| {tag} | ||
| </span> | ||
| ))} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
@Mr-Rahul-Paul have you checked if this works? I see no labels at all - not even those coming from a project tags that you added on the backend.
The issue.tags is undefined
| body?: string | ||
| createdAt: string | number | ||
| hint?: string | ||
| labels?: string[] |
There was a problem hiding this comment.
I believe this is the property you need.
Unless you really wanted to show project tags instead 🤔
|
Hey @kasya Yes, the tags will come up as undefined because idx_tags wasn't populated in the Algolia index. I manually added them using a script to demonstrate the idea. 
In the mock data we have by default:
I made a test.js script that runs and populates all the db objects with hard coded tags , This is mentioned in the Issue
|
I understand that and that was exactly my point. Where would the idx_tags you suggest to use come from then? Clearly we won't be manually adding them to every issue - they should come from somewhere. I'm just trying to understand how are they different from labels that we have and how you envision us using it in general. As I mentioned above I did think this would come from the issue labels on GitHub 🤔 For example, here's a screenshot from the issue obj in Algolia for one of the Next issues:
I don't see any problem with updating the props if needed 🤔 As long as you have data you can use it wherever you want. |
|
It makes perfect sense now
I’ll update the code to pass labels into the Card props and use those on the cards in contribute page |
For objects with non-empty 
The Sonar suggestion is unrelated to this issue and breaks tests, so I’m leaving it as-is. |
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
hi @Mr-Rahul-Paul !
I updated your code to use LabelList component instead that we already had.
Thank you!
|
Thank you for the help with the |
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.