UI/UX issue and issue details page #2398
Conversation
Summary by CodeRabbit
WalkthroughAdds GitHub comment model and syncing; links pull requests to issues; introduces issue-level and module-issue relationships and user-interest tracking; expands GraphQL fields and mentorship mutations; adds management commands, migrations, frontend issue list/detail pages, components, types, and admin tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-07-28T14:51:14.736ZApplied to files:
🧬 Code graph analysis (1)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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: 11
🧹 Nitpick comments (28)
backend/apps/mentorship/utils.py (1)
4-6: Consider adding type hints for clarity.The normalization logic is correct, but adding type hints would improve code documentation and IDE support.
Apply this diff to add type hints:
-def normalize_name(name): +def normalize_name(name: str | None) -> str: """Normalize a string by stripping whitespace and converting to lowercase.""" return (name or "").strip().casefold()backend/apps/mentorship/management/commands/sync_issue_levels.py (3)
20-38: Clarify label collision handling behavior.When multiple TaskLevels within the same module share the same normalized label (lines 35-37), the last TaskLevel processed will overwrite previous mappings. This "last-wins" behavior might be intentional, but it could lead to unexpected results if label collisions are not anticipated.
Consider:
- Adding a check to detect and warn about label collisions across TaskLevels in the same module.
- Documenting this behavior explicitly in the method's docstring.
Example implementation to detect collisions:
def _build_module_level_maps(self, all_levels): """Build a mapping from module ID to a dictionary of data. The dictionary contains a 'label_to_level_map' for normalized label/level names to TaskLevel objects. + + Note: If multiple TaskLevels in the same module have the same normalized label, + the last one processed will be used (last-wins behavior). """ module_data_map = {} for level in all_levels: module_id = level.module_id level_map_container = module_data_map.setdefault(module_id, {"label_to_level_map": {}}) level_map = level_map_container["label_to_level_map"] normalized_level_name = normalize_name(level.name) level_map[normalized_level_name] = level for label_name in level.labels: normalized_label = normalize_name(label_name) + if normalized_label in level_map: + self.stdout.write( + self.style.WARNING( + f"Label collision: '{label_name}' maps to multiple TaskLevels " + f"in module {level.module.name}" + ) + ) level_map[normalized_label] = level return module_data_map
40-53: Rename method to reflect "first match" behavior.The method name
_find_best_match_levelsuggests a prioritization or scoring mechanism, but it actually returns the first matching TaskLevel found. The result depends on:
- The order of modules in
issue_mentorship_modules.- The order of labels in
issue_labels_normalized.This could lead to non-deterministic behavior if an issue belongs to multiple modules with different matching TaskLevels.
Consider renaming to
_find_first_match_levelfor accuracy:- def _find_best_match_level( + def _find_first_match_level( self, issue_labels_normalized, issue_mentorship_modules, module_data_map, ): - """Find the best matching TaskLevel for an issue based on its labels and modules.""" + """Find the first matching TaskLevel for an issue based on its labels and modules. + + Returns the first TaskLevel that matches any of the issue's labels within any + of the issue's modules. The order depends on module and label iteration order. + """And update the call site:
- best_match_level = self._find_best_match_level( + best_match_level = self._find_first_match_level(
72-88: Consider implementing batch processing for scalability.The current implementation loads all issues into memory at once (lines 72-88). For repositories with thousands of issues, this could lead to memory pressure and slow performance.
Consider implementing batch processing using Django's
iterator()with chunking:# Process issues in batches to reduce memory usage BATCH_SIZE = 500 issues_to_update = [] issues_query = Issue.objects.prefetch_related( Prefetch("labels", queryset=Label.objects.only("name")), "mentorship_modules", ).select_related("level").iterator(chunk_size=BATCH_SIZE) for issue in issues_query: issue_labels_normalized = {normalize_name(label.name) for label in issue.labels.all()} best_match_level = self._find_first_match_level( issue_labels_normalized, list(issue.mentorship_modules.all()), module_data_map, ) if issue.level != best_match_level: issue.level = best_match_level issues_to_update.append(issue) # Bulk update when batch is full if len(issues_to_update) >= BATCH_SIZE: Issue.objects.bulk_update(issues_to_update, ["level"]) self.stdout.write(f"Updated batch of {len(issues_to_update)} issues...") issues_to_update = [] # Update remaining issues if issues_to_update: Issue.objects.bulk_update(issues_to_update, ["level"])Note: Be aware that
iterator()with prefetch may require Django 4.1+ for optimal performance. Test this change with your Django version.backend/apps/github/models/comment.py (4)
30-32: Consider indexing for GenericForeignKey performance.The
content_typeandobject_idfields form a generic foreign key, which is frequently queried when accessing related comments. Without an index onobject_id, queries filtering by content object may suffer performance degradation as the comments table grows.Apply this diff to add a compound index:
class Meta: db_table = "github_comments" verbose_name = "Comment" verbose_name_plural = "Comments" ordering = ("-nest_created_at",) + indexes = [ + models.Index(fields=["content_type", "object_id"]), + ]
20-20: Consider indexinggithub_idfor lookup performance.The
github_idfield is used inupdate_datafor lookups (line 73) and is marked as unique. While uniqueness provides an implicit index in most databases, explicitly addingdb_index=Trueimproves code clarity and ensures consistent behavior across database backends.Apply this diff:
-github_id = models.BigIntegerField(unique=True, verbose_name="Github ID") +github_id = models.BigIntegerField(unique=True, db_index=True, verbose_name="Github ID")
38-51: Add defensive attribute checking infrom_github.The method uses
getattr(gh_comment, gh_field, None)with a default, but doesn't validate thatgh_commentis the expected type or has the required structure. If called with an invalid object, silent failures could occur.Consider adding a type check or validation at the method entry:
def from_github(self, gh_comment, author=None): """Populate fields from a GitHub API comment object.""" if not hasattr(gh_comment, 'body'): raise ValueError("Invalid GitHub comment object") field_mapping = { "body": "body", "created_at": "created_at", "updated_at": "updated_at", } # ... rest of method
72-75: Verifygh_comment.idattribute exists before access.Line 73 accesses
gh_comment.iddirectly without validation. If the GitHub API returns an unexpected object structure, this could raise anAttributeError.Apply this diff to add defensive checks:
+if not hasattr(gh_comment, 'id') or gh_comment.id is None: + raise ValueError("GitHub comment object missing required 'id' field") + try: comment = Comment.objects.get(github_id=gh_comment.id) except Comment.DoesNotExist: comment = Comment(github_id=gh_comment.id)backend/apps/mentorship/admin/__init__.py (1)
3-3: Consider alphabetically ordering imports.The import of
IssueUserInterestis placed between existing imports but doesn't follow alphabetical ordering. For consistency and maintainability, consider ordering all imports alphabetically.Apply this diff:
-from .issue_user_interest import IssueUserInterest from .mentee import MenteeAdmin from .mentee_program import MenteeProgramAdmin from .mentor import MentorAdmin from .module import ModuleAdmin from .program import ProgramAdmin from .task import TaskAdmin from .task_level import TaskLevelAdmin +from .issue_user_interest import IssueUserInterestAdminNote: Ensure the imported class name matches the actual export from
issue_user_interest.py(likelyIssueUserInterestAdminrather thanIssueUserInterest).backend/apps/mentorship/models/__init__.py (1)
1-1: Consider maintaining alphabetical import order.The
IssueUserInterestimport is placed at the top but breaks alphabetical ordering. For consistency with the rest of the imports in this file, consider placing it in alphabetical order.Apply this diff:
-from .issue_user_interest import IssueUserInterest from .mentee import Mentee from .mentee_module import MenteeModule from .mentee_program import MenteeProgram from .mentor import Mentor from .mentor_module import MentorModule from .module import Module from .program import Program from .task import Task from .task_level import TaskLevel +from .issue_user_interest import IssueUserInterestThis places the import between
MentorModuleandModulein alphabetical order.frontend/src/components/CardDetailsPage.tsx (1)
92-92: Consider using Next.js router pathname instead ofwindow.location.pathname.Using
window.location.pathnamedirectly can be fragile and is not the idiomatic Next.js approach. TheuseRouterhook fromnext/navigationprovides apathnameproperty that should be used for path construction.However, note that in the Next.js App Router (next/navigation),
useRouter()doesn't expose apathnameproperty. If you need the current path, you should use theusePathname()hook:import { useRouter, usePathname } from 'next/navigation' // Inside component: const router = useRouter() const pathname = usePathname() // Then use pathname instead of window.location.pathname: <button type="button" onClick={() => { router.push(`${pathname}/issues`) }} > View Issues </button>Also applies to: 101-101
backend/apps/mentorship/admin/issue_user_interest.py (1)
11-11: Consider adding user to list_display for better admin UX.The
list_displaycurrently shows onlymoduleandissue, but thesearch_fieldsincludesuser__login. For better admin usability, consider addingusertolist_displayso admins can see which user is interested in each issue at a glance.Apply this diff:
- list_display = ("module", "issue") + list_display = ("module", "issue", "user")backend/apps/github/models/pull_request.py (1)
64-69: Add guardrails to prevent cross‑repository relations in related_issues.Nothing stops linking an Issue from a different repository. Add a validation hook (e.g., m2m_changed receiver) to enforce issue.repository_id == pull_request.repository_id.
Example (place in a signals module or apps.py-ready hook):
from django.db.models.signals import m2m_changed from django.dispatch import receiver from apps.github.models import PullRequest, Issue @receiver(m2m_changed, sender=PullRequest.related_issues.through) def validate_related_issues(sender, instance: PullRequest, action, pk_set, **kwargs): if action == "pre_add" and instance.repository_id and pk_set: invalid = Issue.objects.filter(pk__in=pk_set).exclude(repository_id=instance.repository_id).exists() if invalid: raise ValueError("All related issues must belong to the same repository as the pull request.")backend/apps/github/admin/comment.py (1)
8-19: Optimize admin list rendering for performance.Select related author to cut N+1 queries when listing.
class CommentAdmin(admin.ModelAdmin): @@ list_filter = ("nest_created_at", "nest_updated_at") search_fields = ("body", "author__login") + list_select_related = ("author",)backend/apps/github/management/commands/github_update_pull_requests.py (1)
24-33: Remove redundant bulk_save and prefetch related_issues to avoid N+1.
- pr.related_issues.add() writes the M2M through table; no need to bulk update PRs.
- Prefetch related_issues to avoid a query per PR when computing existing_ids.
- linked = 0 - updated_prs = [] + linked = 0 @@ - queryset = PullRequest.objects.select_related("repository").all() + queryset = ( + PullRequest.objects.select_related("repository") + .prefetch_related("related_issues") + .all() + ) @@ - if new_ids: - pr.related_issues.add(*new_ids) - linked += len(new_ids) - updated_prs.append(pr) + if new_ids: + pr.related_issues.add(*new_ids) + linked += len(new_ids) @@ - if updated_prs: - PullRequest.bulk_save(updated_prs) - self.stdout.write(f"Linked: {linked}")Also applies to: 47-60
backend/apps/github/api/internal/nodes/issue.py (2)
55-59: Avoid N+1 in interested_users; add select_related + ordering.Use select_related("user") and deterministic ordering.
- def interested_users(self) -> list[UserNode]: + def interested_users(self) -> list[UserNode]: """Return all users who have expressed interest in this issue.""" - return [interest.user for interest in IssueUserInterest.objects.filter(issue=self)] + interests = ( + IssueUserInterest.objects.select_related("user") + .filter(issue=self) + .order_by("user__login") + ) + return [i.user for i in interests]
50-53: Return labels in a stable order.Current order is DB-dependent; sort for consistent UI.
- return list(self.labels.values_list("name", flat=True)) + return list(self.labels.order_by("name").values_list("name", flat=True))backend/apps/mentorship/models/issue_user_interest.py (1)
9-14: Prefer UniqueConstraint over unique_together.Future-proof the uniqueness rule and name the constraint explicitly.
class IssueUserInterest(models.Model): @@ class Meta: db_table = "mentorship_issue_user_interests" verbose_name = "Issue User Interest" verbose_name_plural = "Issue User Interests" - unique_together = ("module", "issue", "user") + constraints = [ + models.UniqueConstraint( + fields=("module", "issue", "user"), + name="uniq_interest_module_issue_user", + ) + ]frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (2)
29-36: Call useParams once and destructure.Avoid duplicate hooks and conversions.
- const { issueId } = useParams() as { issueId: string } - - const { programKey, moduleKey } = useParams() as { - programKey: string - moduleKey: string - issueId: string - } + const { programKey, moduleKey, issueId } = useParams() as { + programKey: string + moduleKey: string + issueId: string + }
133-141: Use non-interactive element and stable key for label chips.Buttons without handlers hinder a11y; avoid index keys.
- {visibleLabels.map((label, index) => ( - <button key={index} className={labelButtonClassName}> - {label} - </button> - ))} + {visibleLabels.map((label) => ( + <span key={label} className={labelButtonClassName}> + {label} + </span> + ))} {remainingLabels > 0 && ( - <button className={labelButtonClassName}>+{remainingLabels} more</button> + <span className={labelButtonClassName}>+{remainingLabels} more</span> )}backend/apps/mentorship/api/internal/mutations/module.py (2)
146-149: Use case-insensitive login lookup for GitHub users.GitHub logins are case-insensitive. Align with
resolve_mentors_from_loginsto avoid false negatives.- gh_user = GithubUser.objects.filter(login=user_login).first() + gh_user = GithubUser.objects.filter(login__iexact=user_login).first()Apply in both assign and unassign.
Also applies to: 188-191
17-17: Import consistency (optional).Elsewhere you import models via
from apps.mentorship.models import IssueUserInterest. Consider the same here for consistency.backend/apps/mentorship/management/commands/mentorship_update_comments.py (2)
17-19: Anchor the interest command to avoid false positives.Current
r"/interested"matches "not /interested". Anchor and use word boundary.-INTEREST_PATTERNS = [ - re.compile(r"/interested", re.IGNORECASE), -] +INTEREST_PATTERNS = [ + re.compile(r"(?m)^\s*/interested\b", re.IGNORECASE), +]
70-76: Scope issues to module-linked ones to reduce load.Filtering by all open issues in project repos may process unrelated issues. Prefer the M2M if it’s maintained.
- relevant_issues = Issue.objects.filter( - repository_id__in=module_repos, state=Issue.State.OPEN - ).distinct() + relevant_issues = module.issues.filter(state=Issue.State.OPEN).distinct()If Module.issues isn’t fully populated yet, keep current logic; otherwise adopt the M2M for accuracy/perf. Confirm intended scope.
backend/apps/mentorship/api/internal/nodes/module.py (3)
61-73: Deduplicate interested users when multiple repos share the same issue number.If a module links issues from different repos with the same number, users may appear twice.
- interests = ( - IssueUserInterest.objects.select_related("user") - .filter(module=self, issue_id__in=issue_ids) - .order_by("user__login") - ) - return [i.user for i in interests] + interests = ( + IssueUserInterest.objects.select_related("user") + .filter(module=self, issue_id__in=issue_ids) + ) + users = {i.user for i in interests} + return sorted(users, key=lambda u: u.login)Alternatively, add repo as an argument or return interests per specific IssueNode to avoid cross-repo ambiguity.
74-86: Disambiguate by issue ID(s), not just number, to avoid cross-repo mix.Numbers collide across repos. Align with the approach used above.
- return ( - Task.objects.filter( - module=self, - issue__number=issue_number, - deadline_at__isnull=False, - ) + issue_ids = list(self.issues.filter(number=issue_number).values_list("id", flat=True)) + return ( + Task.objects.filter( + module=self, + issue_id__in=issue_ids, + deadline_at__isnull=False, + ) .order_by("deadline_at") .values_list("deadline_at", flat=True) .first() )
88-101: Same cross-repo ambiguity for assigned_at; filter by issue IDs.- return ( - Task.objects.filter( - module=self, - issue__number=issue_number, - assigned_at__isnull=False, - ) + issue_ids = list(self.issues.filter(number=issue_number).values_list("id", flat=True)) + return ( + Task.objects.filter( + module=self, + issue_id__in=issue_ids, + assigned_at__isnull=False, + ) .order_by("assigned_at") .values_list("assigned_at", flat=True) .first() )backend/apps/github/migrations/0037_issue_level_comment.py (1)
27-73: Add composite index for GenericRelation lookups.
issue.commentsqueries hitcontent_typeandobject_id. A composite index significantly improves performance at scale.options={ "verbose_name": "Comment", "verbose_name_plural": "Comments", "db_table": "github_comments", "ordering": ("-nest_created_at",), + "indexes": [ + models.Index(fields=["content_type", "object_id"], name="github_comments_ct_obj_idx"), + ], },If you prefer a separate migration, use migrations.AddIndex on Comment after this migration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
backend/Makefile(1 hunks)backend/apps/github/Makefile(1 hunks)backend/apps/github/admin/__init__.py(1 hunks)backend/apps/github/admin/comment.py(1 hunks)backend/apps/github/admin/issue.py(1 hunks)backend/apps/github/admin/pull_request.py(1 hunks)backend/apps/github/api/internal/nodes/issue.py(2 hunks)backend/apps/github/common.py(2 hunks)backend/apps/github/management/commands/github_update_pull_requests.py(1 hunks)backend/apps/github/migrations/0037_issue_level_comment.py(1 hunks)backend/apps/github/migrations/0038_pullrequest_related_issues.py(1 hunks)backend/apps/github/models/__init__.py(1 hunks)backend/apps/github/models/comment.py(1 hunks)backend/apps/github/models/issue.py(3 hunks)backend/apps/github/models/pull_request.py(1 hunks)backend/apps/mentorship/Makefile(1 hunks)backend/apps/mentorship/admin/__init__.py(1 hunks)backend/apps/mentorship/admin/issue_user_interest.py(1 hunks)backend/apps/mentorship/admin/module.py(1 hunks)backend/apps/mentorship/admin/task.py(1 hunks)backend/apps/mentorship/api/internal/mutations/module.py(3 hunks)backend/apps/mentorship/api/internal/nodes/module.py(2 hunks)backend/apps/mentorship/management/commands/mentorship_update_comments.py(1 hunks)backend/apps/mentorship/management/commands/sync_issue_levels.py(1 hunks)backend/apps/mentorship/management/commands/sync_module_issues.py(1 hunks)backend/apps/mentorship/migrations/0005_remove_task_level_module_issues_and_more.py(1 hunks)backend/apps/mentorship/models/__init__.py(1 hunks)backend/apps/mentorship/models/issue_user_interest.py(1 hunks)backend/apps/mentorship/models/managers/__init__.py(1 hunks)backend/apps/mentorship/models/managers/module.py(1 hunks)backend/apps/mentorship/models/module.py(2 hunks)backend/apps/mentorship/models/task.py(1 hunks)backend/apps/mentorship/utils.py(1 hunks)cspell/custom-dict.txt(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)frontend/src/components/ModuleCard.tsx(2 hunks)frontend/src/components/ProgramCard.tsx(0 hunks)frontend/src/server/queries/issueQueries.ts(1 hunks)frontend/src/server/queries/moduleQueries.ts(1 hunks)frontend/src/types/issue.ts(2 hunks)frontend/src/types/pullRequest.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/ProgramCard.tsx
🧰 Additional context used
🧬 Code graph analysis (25)
backend/apps/mentorship/models/module.py (2)
backend/apps/mentorship/models/managers/module.py (1)
PublishedModuleManager(8-13)backend/apps/mentorship/api/internal/nodes/module.py (1)
issues(43-49)
backend/apps/mentorship/models/managers/__init__.py (1)
backend/apps/mentorship/models/managers/module.py (1)
PublishedModuleManager(8-13)
backend/apps/mentorship/models/__init__.py (1)
backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)
backend/apps/github/admin/__init__.py (1)
backend/apps/github/admin/comment.py (1)
CommentAdmin(8-18)
backend/apps/mentorship/management/commands/mentorship_update_comments.py (6)
backend/apps/common/utils.py (1)
truncate(181-193)backend/apps/github/common.py (1)
sync_issue_comments(237-299)backend/apps/github/models/issue.py (1)
Issue(19-234)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/mentorship/models/module.py (1)
Module(17-104)backend/apps/github/models/generic_issue_model.py (1)
State(18-20)
backend/apps/mentorship/models/managers/module.py (1)
backend/apps/mentorship/models/program.py (1)
ProgramStatus(25-28)
backend/apps/github/admin/comment.py (1)
backend/apps/github/models/comment.py (1)
Comment(11-85)
backend/apps/mentorship/utils.py (1)
backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)
backend/apps/github/api/internal/nodes/issue.py (3)
backend/apps/github/api/internal/nodes/pull_request.py (1)
PullRequestNode(17-42)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/mentorship/api/internal/nodes/module.py (1)
interested_users(62-72)
backend/apps/mentorship/api/internal/nodes/module.py (3)
backend/apps/github/api/internal/nodes/issue.py (2)
IssueNode(23-63)interested_users(56-58)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/mentorship/models/task.py (1)
Task(10-80)
backend/apps/mentorship/api/internal/mutations/module.py (3)
backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/mentorship/api/internal/nodes/module.py (2)
ModuleNode(17-100)issues(43-49)backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)
backend/apps/mentorship/management/commands/sync_issue_levels.py (4)
backend/apps/github/models/issue.py (1)
Issue(19-234)backend/apps/github/models/label.py (1)
Label(9-77)backend/apps/mentorship/models/task_level.py (1)
TaskLevel(8-61)backend/apps/mentorship/utils.py (1)
normalize_name(4-6)
backend/apps/github/migrations/0037_issue_level_comment.py (1)
backend/apps/mentorship/migrations/0005_remove_task_level_module_issues_and_more.py (1)
Migration(7-79)
backend/apps/mentorship/models/issue_user_interest.py (2)
backend/apps/github/models/issue.py (1)
Meta(25-31)backend/apps/mentorship/models/module.py (1)
Meta(23-31)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (5)
frontend/src/server/queries/moduleQueries.ts (1)
GET_MODULE_ISSUES(79-120)frontend/src/app/global-error.tsx (2)
handleAppError(66-86)ErrorDisplay(28-51)backend/apps/mentorship/api/internal/nodes/module.py (1)
issues(43-49)backend/apps/github/api/internal/nodes/issue.py (2)
labels(51-53)assignees(46-48)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
backend/apps/mentorship/admin/__init__.py (1)
backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)
backend/apps/github/models/__init__.py (1)
backend/apps/github/models/comment.py (1)
Comment(11-85)
frontend/src/types/issue.ts (1)
frontend/src/types/pullRequest.ts (1)
PullRequest(3-11)
backend/apps/mentorship/management/commands/sync_module_issues.py (5)
backend/apps/github/models/issue.py (2)
Issue(19-234)save(187-196)backend/apps/mentorship/models/module.py (2)
Module(17-104)save(97-104)backend/apps/mentorship/models/task.py (2)
Task(10-80)Status(19-25)backend/apps/mentorship/utils.py (1)
normalize_name(4-6)backend/apps/github/models/repository.py (1)
path(154-156)
backend/apps/github/management/commands/github_update_pull_requests.py (1)
backend/apps/github/models/pull_request.py (2)
PullRequest(11-152)bulk_save(114-116)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (3)
frontend/src/server/queries/issueQueries.ts (3)
GET_MODULE_ISSUE_VIEW(3-53)ASSIGN_ISSUE_TO_USER(55-71)UNASSIGN_ISSUE_FROM_USER(73-89)frontend/src/app/global-error.tsx (1)
ErrorDisplay(28-51)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
backend/apps/github/common.py (5)
backend/apps/github/models/comment.py (3)
Comment(11-85)update_data(59-85)bulk_save(54-56)backend/apps/github/models/issue.py (4)
latest_comment(99-106)update_data(210-234)save(187-196)bulk_save(199-201)backend/apps/github/models/repository.py (2)
path(154-156)update_data(307-349)backend/apps/github/models/user.py (3)
User(19-159)update_data(130-159)bulk_save(110-112)backend/apps/common/models.py (1)
bulk_save(19-34)
backend/apps/mentorship/admin/issue_user_interest.py (1)
backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)
backend/apps/mentorship/migrations/0005_remove_task_level_module_issues_and_more.py (1)
backend/apps/github/migrations/0037_issue_level_comment.py (1)
Migration(7-74)
backend/apps/github/models/comment.py (3)
backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)backend/apps/common/utils.py (1)
truncate(181-193)backend/apps/github/models/issue.py (5)
Meta(25-31)from_github(108-147)bulk_save(199-201)update_data(210-234)save(187-196)
🪛 checkmake (0.2.2)
backend/apps/mentorship/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 GitHub Actions: Run CI/CD
frontend/src/components/ModuleCard.tsx
[warning] 15-15: ESLint import/order warning: 'next/navigation' import should occur before import of 'react'. (Step: pnpm run lint:check)
[warning] 18-18: ESLint import/order warning: 'next/dist/shared/lib/hooks-client-context.shared-runtime' import should occur before import of 'next/link'. (Step: pnpm run lint:check)
[error] 18-18: ESLint error: 'PathnameContext' is defined but never used. (no-unused-vars) (Step: pnpm run lint:check)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (26)
frontend/src/components/ModuleCard.tsx (1)
73-73: LGTM! Correct migration to Next.js navigation hook.The replacement of
window.location.pathnamewithusePathname()follows Next.js best practices and correctly constructs the module link path. This change properly integrates with the new module/issue navigation flows introduced in this PR.Also applies to: 77-77
backend/Makefile (1)
3-3: LGTM!The include directive correctly integrates the mentorship Makefile into the backend build workflow, following the established pattern for other app-specific Makefiles.
cspell/custom-dict.txt (1)
123-123: LGTM!The addition of "unassigning" to the custom dictionary correctly supports the new unassignment functionality introduced in this PR.
backend/apps/mentorship/models/managers/__init__.py (1)
1-1: LGTM!The package initialization correctly exposes
PublishedModuleManageras part of the managers module's public API, following standard Python conventions.backend/apps/mentorship/admin/task.py (1)
28-28: LGTM!The ordering by
-assigned_atappropriately displays the most recently assigned tasks first in the admin interface. Django will place tasks with nullassigned_atvalues at the end, which is the expected behavior.backend/apps/mentorship/Makefile (1)
1-9: LGTM!All three Makefile targets follow the established
exec-backend-commandpattern consistently and correctly integrate with the corresponding Django management commands.Note: The static analysis warnings about missing phony targets (
all,clean,test) are false positives. These are not required in an included Makefile that's meant to extend the parentbackend/Makefile.backend/apps/mentorship/models/task.py (1)
27-31: Verify nullableassigned_atusage and handling
Ensure all Task creation paths explicitly setassigned_at, no code (views, serializers, filters, etc.) assumes it’s non-null, and the migration backfills or otherwise handles existing records.backend/apps/mentorship/migrations/0005_remove_task_level_module_issues_and_more.py (1)
38-78: LGTM!The
IssueUserInterestmodel is well-designed with:
- Appropriate foreign key relationships with CASCADE deletion behavior
unique_togetherconstraint preventing duplicate interest records- Clear verbose names for admin display
- Proper db_table naming convention
The model structure aligns with the requirements for tracking user interest in issues within modules.
backend/apps/github/admin/issue.py (1)
22-22: LGTM!Adding
leveltolist_displayprovides visibility into issue difficulty levels in the admin interface, which aligns with the newIssue.levelfield added in migration 0037.backend/apps/github/admin/__init__.py (1)
3-3: LGTM!The
CommentAdminimport follows the established pattern for admin registrations in the GitHub app and maintains alphabetical ordering with other admin imports.backend/apps/mentorship/admin/module.py (1)
16-16: LGTM!Adding
autocomplete_fieldsfor theissuesM2M relationship improves the admin UI experience when linking issues to modules. TheIssueAdminalready hassearch_fields = ("title",)configured, which enables the autocomplete functionality to work correctly.frontend/src/types/issue.ts (2)
1-1: LGTM!The import is necessary for the new
pullRequestsfield and follows TypeScript conventions.
13-13: LGTM!The optional
pullRequestsfield correctly models the many-to-many relationship between issues and pull requests established in the backend.backend/apps/github/models/__init__.py (1)
3-3: LGTM!The Comment import follows the established pattern for exposing models publicly.
backend/apps/github/admin/pull_request.py (1)
17-17: LGTM!Adding
related_issuestoautocomplete_fieldsenables efficient admin selection of related issues and is consistent with the existing pattern for other relational fields.frontend/src/server/queries/moduleQueries.ts (1)
79-120: LGTM!The
GET_MODULE_ISSUESquery is well-structured and fetches all necessary fields for displaying module issues with their related pull requests, assignees, and labels. The nested structure aligns with the type definitions and backend schema.backend/apps/github/migrations/0038_pullrequest_related_issues.py (1)
1-19: LGTM!The migration correctly adds a many-to-many relationship between
PullRequestandIssue. Theblank=Trueconfiguration is appropriate since not all pull requests are linked to issues, and therelated_name="pull_requests"establishes a clear reverse relationship.backend/apps/github/Makefile (1)
21-23: Management command exists: Verifiedgithub_update_pull_requestsis implemented inbackend/apps/github/management/commands/github_update_pull_requests.py.backend/apps/mentorship/models/managers/module.py (1)
11-13: LGTM.Clean queryset override targeting published programs.
backend/apps/mentorship/models/module.py (1)
20-22: LGTM.Manager exposure and issues M2M look consistent with the API nodes using select/prefetch.
Also applies to: 72-78
frontend/src/server/queries/issueQueries.ts (1)
3-53: LGTM.Queries and mutations align with the described schema and UI needs.
Also applies to: 55-71, 73-89
backend/apps/github/models/issue.py (1)
98-107: Ordering by nest_created_at is valid.Commentinheritsnest_created_atviaTimestampedModel.backend/apps/mentorship/management/commands/sync_module_issues.py (1)
128-169: Create/update tasks for all assignees, not just the first.Using first() drops additional assignees; iterate over all.
- for issue in issues: - assignee = issue.assignees.first() - if not assignee: - continue - - status = self._get_status(issue, assignee) - task, created = Task.objects.get_or_create( - issue=issue, - assignee=assignee, - defaults={"module": module, "status": status}, - ) + for issue in issues: + for assignee in issue.assignees.all(): + status = self._get_status(issue, assignee) + task, created = Task.objects.get_or_create( + issue=issue, + assignee=assignee, + defaults={"module": module, "status": status}, + ) @@ - if (created or task.assigned_at is None) and issue.repository: + if (created or task.assigned_at is None) and issue.repository: @@ - if created: - num_tasks_created += 1 - self.stdout.write( - self.style.SUCCESS( - f"Task created for user '{assignee.login}' on issue " - f"{issue.repository.name}#{issue.number} " - f"in module '{module.name}'" - ) - ) + if created: + num_tasks_created += 1 + self.stdout.write( + self.style.SUCCESS( + f"Task created for user '{assignee.login}' on issue " + f"{issue.repository.name}#{issue.number} " + f"in module '{module.name}'" + ) + ) @@ - if updates: - for field, value in updates.items(): - setattr(task, field, value) - task.save(update_fields=list(updates.keys())) + if updates: + for field, value in updates.items(): + setattr(task, field, value) + task.save(update_fields=list(updates.keys()))⛔ Skipped due to learnings
Learnt from: Rajgupta36 PR: OWASP/Nest#2000 File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145 Timestamp: 2025-08-17T11:55:55.990Z Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.backend/apps/github/api/internal/nodes/issue.py (1)
60-63: Resolver pull_requests is valid PullRequest model declares its ManyToManyField to Issue with related_name="pull_requests", so this resolver will work as intended.backend/apps/mentorship/api/internal/nodes/module.py (2)
42-50: Good resolver with prefetch/select_related.
51-59: Single issue lookup looks good.
backend/apps/mentorship/migrations/0005_remove_task_level_module_issues_and_more.py
Show resolved
Hide resolved
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Outdated
Show resolved
Hide resolved
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
frontend/src/components/ModuleCard.tsx (1)
11-12: Fix import order to comply with linting rules.The
next/navigationimport on line 11 should be placed before thereactimport on line 12, as flagged in the previous review and indicated by the linting error.Apply this diff to fix the import order:
import Link from 'next/link' +import { usePathname } from 'next/navigation' import { useState } from 'react' import type { Module } from 'types/mentorship' import { formatDate } from 'utils/dateFormatter' import { TextInfoItem } from 'components/InfoItem' -import { usePathname } from 'next/navigation' import SingleModuleCard from 'components/SingleModuleCard'
🧹 Nitpick comments (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (2)
174-174: Prefer displaying name over login for better readability.Line 174 displays
a.login || a.name, which prefers the technical login over the more human-readable name. Consider reversing the precedence toa.name || a.loginfor improved user experience.Apply this diff:
- <span className="text-sm font-medium">{a.login || a.name}</span> + <span className="text-sm font-medium">{a.name || a.login}</span>
110-118: Consider using consistent date formatting with locale options.The date formatting uses
toLocaleDateString()without arguments, which relies on the browser's default locale. For consistent presentation and better internationalization support, consider passing explicit locale and format options.Example with consistent formatting:
const formatDate = (dateString: string) => { return new Date(dateString).toLocaleDateString('en-US', { year: 'numeric', month: 'short', day: 'numeric' }) } // Then use: {data?.getModule?.taskAssignedAt ? formatDate(data.getModule.taskAssignedAt) : 'Not assigned'}Alternatively, you could use a library like
date-fnsordayjsfor more robust date formatting with i18n support.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/__tests__/unit/components/ItemCardList.test.tsx(1 hunks)frontend/__tests__/unit/components/RecentPullRequests.test.tsx(2 hunks)frontend/__tests__/unit/data/mockOrganizationData.ts(2 hunks)frontend/__tests__/unit/data/mockProjectDetailsData.ts(2 hunks)frontend/__tests__/unit/data/mockRepositoryData.ts(2 hunks)frontend/__tests__/unit/data/mockUserDetails.ts(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)frontend/src/components/ModuleCard.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/tests/unit/data/mockProjectDetailsData.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (3)
frontend/src/server/queries/issueQueries.ts (3)
GET_MODULE_ISSUE_VIEW(3-53)ASSIGN_ISSUE_TO_USER(55-71)UNASSIGN_ISSUE_FROM_USER(73-89)frontend/src/app/global-error.tsx (1)
ErrorDisplay(28-51)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (7)
frontend/src/components/ModuleCard.tsx (1)
72-76: Correct migration to Next.jsusePathnamehook.The replacement of
window.location.pathnamewith the Next.jsusePathnamehook is the correct approach for constructing module links in Next.js 15. The hook is properly called at the top level of the component, and the href construction is accurate.frontend/__tests__/unit/data/mockOrganizationData.ts (1)
79-79: LGTM! Test data augmentation is consistent.The addition of
idfields to the pull request mock data aligns with the updated PullRequest type shape and is consistent with similar changes across other test files in this PR.Also applies to: 89-89
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
521-521: LGTM! Test data updated correctly.The
idfield addition to mock pull request data is consistent with the PullRequest type changes and aligns with similar updates across the test suite.frontend/__tests__/unit/components/ItemCardList.test.tsx (1)
151-151: LGTM! Mock data aligned with type definition.The
idfield addition to the mock pull request object is a straightforward update that aligns with the PullRequest type augmentation.frontend/__tests__/unit/data/mockUserDetails.ts (1)
48-48: LGTM! Consistent test data update.The
idfield addition to the pull request mock data is consistent with the pattern established across all test fixtures in this PR.frontend/__tests__/unit/data/mockRepositoryData.ts (1)
65-65: LGTM! Test data properly augmented.The addition of
idfields to both pull request entries is consistent with the updated PullRequest type and follows the same descriptive naming pattern used in other test files.Also applies to: 75-75
frontend/__tests__/unit/components/RecentPullRequests.test.tsx (1)
43-43: LGTM! Test fixtures updated correctly.The
idfield additions to the test data objects align with the PullRequest type changes. Both objects using the sameidvalue is acceptable since they're used in separate, isolated test cases.Also applies to: 55-55
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/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
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Show resolved
Hide resolved
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/apps/mentorship/api/internal/mutations/module.py (2)
139-196: Stop passingmsg=into Django exceptionsEvery
raise ObjectDoesNotExist(msg=...)/raise PermissionDenied(msg=...)here will immediately crash withTypeError: __init__() got an unexpected keyword argument 'msg'. Pass the message positionally (and add one where it’s currently missing). Example fix:- if module is None: - raise ObjectDoesNotExist(msg="Module not found.") + if module is None: + raise ObjectDoesNotExist("Module not found.") ... - if mentor is None: - raise PermissionDenied(msg="Only mentors can assign issues.") + if mentor is None: + raise PermissionDenied("Only mentors can assign issues.") ... - if gh_user is None: - raise ObjectDoesNotExist(msg="Assignee not found.") + if gh_user is None: + raise ObjectDoesNotExist("Assignee not found.") ... - if issue is None: - raise ObjectDoesNotExist(msg="Issue not found in this module.") + if issue is None: + raise ObjectDoesNotExist("Issue not found in this module.")Please update the remaining occurrences in this file the same way.
212-223: Samemsg=bug in update flow
raise ObjectDoesNotExist(msg="Module not found.")andraise PermissionDenied(msg="Only mentors can edit modules.")will also throwTypeErrorat runtime. Switch to positional strings:- except Module.DoesNotExist as e: - raise ObjectDoesNotExist(msg="Module not found.") from e + except Module.DoesNotExist as e: + raise ObjectDoesNotExist("Module not found.") from e ... - except Mentor.DoesNotExist as err: - ... - raise PermissionDenied(msg="Only mentors can edit modules.") from err + except Mentor.DoesNotExist as err: + ... + raise PermissionDenied("Only mentors can edit modules.") from err
🧹 Nitpick comments (2)
frontend/src/server/queries/moduleQueries.ts (1)
80-121: Consider adding pagination to the issues query.The GET_MODULE_ISSUES query fetches all issues for a module without pagination. For modules with many issues, this could lead to large response sizes and slower query performance.
Consider adding pagination parameters (e.g.,
first,after) to the query:query GetModuleIssues( $programKey: String! $moduleKey: String! $first: Int $after: String ) { getModule(moduleKey: $moduleKey, programKey: $programKey) { id name key issues(first: $first, after: $after) { edges { node { # ... issue fields } } pageInfo { hasNextPage endCursor } } } }backend/apps/mentorship/api/internal/nodes/module.py (1)
43-50: Consider adding pagination to the issues resolver.The issues() resolver returns all issues for a module without pagination. For modules with many issues, this could lead to performance issues and large response sizes.
Consider implementing cursor-based or limit/offset pagination:
@strawberry.field def issues(self, first: int | None = None, after: str | None = None) -> list[IssueNode]: """Return issues linked to this module.""" queryset = ( self.issues.select_related("repository", "author") .prefetch_related("assignees", "labels") .order_by("-created_at") ) if first: queryset = queryset[:first] return list(queryset)Or return a connection type with page info for full pagination support.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/apps/mentorship/api/internal/mutations/module.py(5 hunks)backend/apps/mentorship/api/internal/nodes/module.py(5 hunks)frontend/__tests__/unit/components/SingleModuleCard.test.tsx(1 hunks)frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx(2 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/create/page.tsx(2 hunks)frontend/src/components/CardDetailsPage.tsx(3 hunks)frontend/src/components/LabelList.tsx(1 hunks)frontend/src/components/ModuleCard.tsx(3 hunks)frontend/src/components/ModuleForm.tsx(2 hunks)frontend/src/server/mutations/moduleMutations.ts(2 hunks)frontend/src/server/queries/moduleQueries.ts(2 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/mentorship.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/ModuleCard.tsx
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
backend/apps/mentorship/api/internal/mutations/module.py (7)
backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/github/api/internal/nodes/issue.py (2)
labels(51-53)assignees(46-48)backend/apps/nest/api/internal/permissions.py (1)
IsAuthenticated(10-21)backend/apps/mentorship/api/internal/nodes/module.py (2)
ModuleNode(17-101)issues(44-50)backend/apps/github/api/internal/queries/user.py (1)
user(40-53)backend/apps/mentorship/models/module.py (1)
Module(17-104)backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)
frontend/src/app/my/mentorship/programs/[programKey]/modules/create/page.tsx (1)
frontend/src/utils/parser.ts (1)
parseCommaSeparated(1-6)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx (1)
frontend/src/utils/parser.ts (1)
parseCommaSeparated(1-6)
frontend/src/components/LabelList.tsx (2)
backend/apps/github/models/label.py (1)
Label(9-77)backend/apps/github/api/internal/nodes/issue.py (1)
labels(51-53)
backend/apps/mentorship/api/internal/nodes/module.py (3)
backend/apps/github/api/internal/nodes/issue.py (3)
IssueNode(23-63)labels(51-53)interested_users(56-58)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/mentorship/models/task.py (1)
Task(10-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (10)
frontend/src/components/ModuleForm.tsx (1)
19-19: LGTM!The labels field is correctly added to the form data structure and the UI input follows the same pattern as the existing domains and tags fields.
Also applies to: 189-202
frontend/src/server/mutations/moduleMutations.ts (1)
15-15: LGTM!The labels field is correctly added to both mutation selection sets, consistent with other fields like tags and domains.
Also applies to: 39-39
frontend/src/app/my/mentorship/programs/[programKey]/modules/create/page.tsx (1)
42-42: LGTM!The labels field initialization and parsing follows the same pattern as domains and tags, maintaining consistency across the codebase.
Also applies to: 92-92
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
68-68: LGTM!The labels prop is correctly passed to DetailsCard, following the same pattern as tags and domains.
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx (1)
85-85: LGTM!The labels field is correctly initialized by joining the array into a comma-separated string and parsed back into an array for the mutation, following the same pattern as domains and tags. The null/undefined handling with
|| []is appropriate.Also applies to: 107-107
frontend/src/server/queries/moduleQueries.ts (1)
63-63: LGTM!The labels field is correctly added to the query selection set.
backend/apps/mentorship/api/internal/nodes/module.py (4)
27-27: LGTM!The labels field is correctly added to ModuleNode and both input types, with appropriate default values.
Also applies to: 113-113, 133-133
52-60: LGTM!The issue_by_number resolver correctly fetches a single issue with proper query optimization using select_related and prefetch_related.
62-73: LGTM!The interested_users resolver correctly handles the case where a module might have issues from multiple repositories with the same issue number. The logic appropriately fetches all matching issue IDs and returns the interested users.
75-87: LGTM!Both task_deadline and task_assigned_at resolvers correctly fetch the earliest timestamp for tasks linked to the module and issue number, with proper null filtering.
Also applies to: 89-101
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
Outdated
Show resolved
Hide resolved
| </button> | ||
| </div> | ||
| ))} | ||
| {assignees.length === 0 && <span className="text-sm text-gray-400">Unassigned</span>} |
There was a problem hiding this comment.
Let's just not show anything if the issue is not assigned - I feel like that will be clearer visually.
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Show resolved
Hide resolved
| <SecondaryCard title={<AnchorTitle title="Task Timeline" />}> | ||
| <div className="space-y-1 text-sm text-gray-700 dark:text-gray-300"> | ||
| <div> | ||
| <span className="font-medium">Assigned:</span>{' '} |
There was a problem hiding this comment.
This is confusing...
So right now for one of the issues I see Assignee and at the same time this block says Not Assigned. 🤔
Did you mean to show when the issue was assigned? If so, why does it not show anything even though we show Assignee.
There was a problem hiding this comment.
@Rajgupta36 so about this... Will the new synced data help with this too? Or was it addressed in changes you pushed since review?
There was a problem hiding this comment.
I updated the command so that it always syncs the latest assignee date, ensuring that both the assignee and the assigned date are included if an assignee is present
There was a problem hiding this comment.
How are these ordered right now? Can we show the most recent ones at the top?
There was a problem hiding this comment.
Right now, they are ordered by the created_at attribute. Do we want to use another field?
There was a problem hiding this comment.
@Rajgupta36 as I can see right now the oldest issues are at the top, which is not the best idea.
Also, not really relevant, but I can't open images you attach. When I click on them - they lead to the address for some private-user and fallback to 404. Not a huge deal and not sure if you can attach them differently. But that would be good to be able to click into these.
I've never had this issue before 🤔
There was a problem hiding this comment.
Actually, I synced everything again and I see a different order now.
Not sure how these are ordered tbh.
There was a problem hiding this comment.
Something is off here.
Look at issue #1766
Current assignee is @trucodd. Your page shows assignee as the first person who was assigned for the issue. Since then they have been removed as an assignee and the issue was reassigned.
There was a problem hiding this comment.
Same for #1385
Page shows no assignee at all and shows PR from first assignee of the issue. That PR was closed and never merged in, though this is not the point.
After that the issue was reassigned and another person completed the task - but it's never shown on this page, not in Assignee nor in the PR section.
This needs to be fixed:
Also this case shows one important thing - we should probably add a label to the PR view about if the PR was merged or closed - that's important.
There was a problem hiding this comment.
Right now, it loads old data using the make load-data command.
If you want to see the current data, please use this command:
make github-update-owasp-organization --repository Nest
My personal access token got rate-limited halfway 😭 through the syncing.
There was a problem hiding this comment.
Assignee date is wrong in second image. got rate limited while fetching
There was a problem hiding this comment.
This is better 👌🏼 Thanks!
Can we change the merged color for traditional GitHub color for merge (which is purple shade)? I do think those colors are something people are already used to and understand without even needing to read words like merged or closed.
I would keep green shade for Open PRs.
There was a problem hiding this comment.
I'm syncing new data locally and will try that out when it's done 👍🏼
There was a problem hiding this comment.
ok, everything synced properly. I did not run into any rate limiting issues, so I assume everything was synced and should show up.
I see Assigned at timestamp now, but I still don't see Deadline date.
For example for issue #1385 :
I do see correct PRs (with a proper state) and and correct assignee now 👍🏼 . So the only issue would be the deadline date here.
There was a problem hiding this comment.
Regarding this matter, I have spoken with Ark. Could you please review it and share your feedback?
https://owasp.slack.com/archives/C08RHMHHZ5K/p1755798724749389?thread_ts=1755720622.747399&cid=C08RHMHHZ5K
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
Show resolved
Hide resolved
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Show resolved
Hide resolved
| <div className="flex flex-row items-center gap-2"> | ||
| <FontAwesomeIcon icon={faUsers} className="mr-2 h-5 w-5" /> | ||
| </div> | ||
| <span>Interested Users</span> |
There was a problem hiding this comment.
UPD: Realized we need to have something like /interested on the issue comment to mark user as interested.
How did you test this locally?
I'm yet to find an issue that would show some interested users.. 🤔
Do you have any examples? I can see we have issues from gssoc25 label that were not taken yet.
This one for example - #2177
I don't even see this issue in the list we pull for some reason 🤔 This is also a problem.
There was a problem hiding this comment.
There was a problem hiding this comment.
Also, suppose we have issue #2177 in our project locally.
Just try commenting /interested on that issue.
Then, run this command
make mentorship-update-comments
to update the comment locally — you’ll get the list of interested users in the array.
There was a problem hiding this comment.
Thanks for pointing that out. This should have been added to the steps to test though - I'm not always checking all of the backend logic to know which commands to run to check if this works.
Could you update the description with the rest of the steps required to fully test the implementation? Thanks!
|
Also, can we add a way to view Issues from this dropdown for a |
done |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/__tests__/unit/components/ItemCardList.test.tsx(2 hunks)frontend/__tests__/unit/components/RecentPullRequests.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/unit/components/RecentPullRequests.test.tsx
- frontend/tests/unit/components/ItemCardList.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
| )} | ||
| </div> | ||
| ) : ( | ||
| <span className="text-gray-400">Unassigned</span> |
There was a problem hiding this comment.
This was not resolved as per by prev. request.
I can still see Unassigned if the issue is not assigned instead an of empty space there.
There was a problem hiding this comment.
ohh.sorry for that
| id | ||
| number | ||
| title | ||
| summary |
There was a problem hiding this comment.
I do believe we'd better show an actual description of the issue, instead of the summary. Is this the same summary we show on the Contribute page?
Do we have actual description available? That would make more sense to show the full thing here.
There was a problem hiding this comment.
It's same summary as contribute page.
this is how body looks
| issues(limit: $limit, offset: $offset, label: $label) { | ||
| id | ||
| number | ||
| createdAt | ||
| title | ||
| summary | ||
| url | ||
| author { | ||
| id | ||
| avatarUrl | ||
| login | ||
| name | ||
| } | ||
| assignees { | ||
| id | ||
| avatarUrl | ||
| login | ||
| name | ||
| } | ||
| labels | ||
| pullRequests { | ||
| id | ||
| title | ||
| url | ||
| createdAt | ||
| author { | ||
| id | ||
| login | ||
| name | ||
| avatarUrl |
There was a problem hiding this comment.
I don't think we need ALL of this info at this level. Noticed because of the summary which we never use on this page, but I think there is a lot of other properties as well that we don't need.
Can we clean this up?
| className="block max-w-xl cursor-pointer text-left hover:underline" | ||
| > | ||
| <TruncatedText | ||
| text={`${issue.title.slice(0, 50)}${issue.title.length > 50 ? '…' : ''}`} |
There was a problem hiding this comment.
I do believe we better show the full title on hover in Tooltip/title property. Otherwise there's no sense in showing truncated text on truncated text...
| <ActionButton url={pr.url} tooltipLabel="View PR"> | ||
| <FontAwesomeIcon icon={faLink} /> | ||
| <span className="hidden sm:inline">View PR</span> | ||
| </ActionButton> |
There was a problem hiding this comment.
I think this is redundant and can be removed completely. The link tot he PR in the title leads to the same place. Plus it will look nicer, since styles for PR state and this button don't really look nice together:
There was a problem hiding this comment.
There was a problem hiding this comment.
I think these are old ones 🤔
You should be able to get the colors from inspecting issues pages, then you can transform that into hex code:
There was a problem hiding this comment.
I'm syncing new data locally and will try that out when it's done 👍🏼
| <SecondaryCard title={<AnchorTitle title="Task Timeline" />}> | ||
| <div className="space-y-1 text-sm text-gray-700 dark:text-gray-300"> | ||
| <div> | ||
| <span className="font-medium">Assigned:</span>{' '} |
There was a problem hiding this comment.
@Rajgupta36 so about this... Will the new synced data help with this too? Or was it addressed in changes you pushed since review?
There was a problem hiding this comment.
ok, everything synced properly. I did not run into any rate limiting issues, so I assume everything was synced and should show up.
I see Assigned at timestamp now, but I still don't see Deadline date.
For example for issue #1385 :
I do see correct PRs (with a proper state) and and correct assignee now 👍🏼 . So the only issue would be the deadline date here.
There was a problem hiding this comment.
Can we also add a label tag to show current state of the issue? Is it open or closed.
We could show that next to a title on the Issue page.
Or even underneath the title, where we have the Org/repo and the Issue ID. Whatever you prefer.
There was a problem hiding this comment.
does it looks good ?
There was a problem hiding this comment.
@Rajgupta36 Sorry, I still can't open your images for some reason. Are those the same colors GitHub uses? If so, they look great 👍🏼
Also, it's hard to tell from the small image, but do both have the same border radius? The one on the issue page looks a bit more pill-shaped - can we make them consistent?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (2)
30-36: Remove duplicate useParams destructuring.Extract all params once.
Apply this diff:
- const { issueId } = useParams() as { issueId: string } - - const { programKey, moduleKey } = useParams() as { - programKey: string - moduleKey: string - issueId: string - } + const { programKey, moduleKey, issueId } = useParams() as { + programKey: string + moduleKey: string + issueId: string + }
303-327: Add fallback for missing login and aria-label to Assign button.Prevents rendering “@undefined” and improves accessibility.
Apply this diff:
- <span className="text-sm font-medium">@{u.login}</span> + <span className="text-sm font-medium">@{u.login || u.name || 'Unknown'}</span> @@ - <button + <button type="button" + aria-label={`Assign issue to @${u.login || u.name || 'Unknown'}`} disabled={!issueId || assigning}
🧹 Nitpick comments (4)
backend/apps/github/api/internal/nodes/issue.py (1)
45-48: Consider prefetch optimization for nested field access.If nested UserNode fields are frequently accessed (e.g., user login or email), consider prefetching to prevent potential N+1 queries.
Apply this diff to add prefetch optimization if needed:
@strawberry.field def assignees(self) -> list[UserNode]: """Resolve assignees list.""" - return list(self.assignees.all()) + return list(self.assignees.prefetch_related('assignees'))Note: Only apply if profiling shows N+1 queries in production.
frontend/src/types/issue.ts (1)
6-11: Align Issue.number and timestamps with backend types (or split types).GraphQL Issue.number is Int, and timestamps are typically ISO strings. Here, number?: string and createdAt/updatedAt: number can cause subtle bugs if reused for GraphQL data. Either:
- Update to number?: number and createdAt/updatedAt: string, or
- Keep this as an Algolia/search shape and define a separate GraphQL Issue type.
Please verify usages and adjust accordingly.
Also applies to: 16-18
frontend/src/server/queries/moduleQueries.ts (1)
80-106: Prefer cursor-based pagination and include assignee id.
- Offset pagination can duplicate/skip items when the list changes; consider cursor-based pagination for stability.
- Include assignees.id to support stable React keys downstream.
Example (shape only):
- issues(after: $cursor, first: $first) { edges { node { …, assignees { id login name avatarUrl } } pageInfo { endCursor hasNextPage } } }
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
118-134: Hide Task Timeline when unassigned (per maintainer feedback).Only render when assignedAt or deadline exists.
Apply this diff:
- <SecondaryCard title={<AnchorTitle title="Task Timeline" />}> - <div className="space-y-1 text-sm text-gray-700 dark:text-gray-300"> - <div> - <span className="font-medium">Assigned:</span>{' '} - {data?.getModule?.taskAssignedAt - ? new Date(data.getModule.taskAssignedAt).toLocaleDateString() - : 'Not assigned'} - </div> - <div> - <span className="font-medium">Deadline:</span>{' '} - {data?.getModule?.taskDeadline - ? new Date(data.getModule.taskDeadline).toLocaleDateString() - : 'No deadline set'} - </div> - </div> - </SecondaryCard> + {(data?.getModule?.taskAssignedAt || data?.getModule?.taskDeadline) && ( + <SecondaryCard title={<AnchorTitle title="Task Timeline" />}> + <div className="space-y-1 text-sm text-gray-700 dark:text-gray-300"> + {data?.getModule?.taskAssignedAt && ( + <div> + <span className="font-medium">Assigned:</span>{' '} + {new Date(data.getModule.taskAssignedAt).toLocaleDateString()} + </div> + )} + {data?.getModule?.taskDeadline && ( + <div> + <span className="font-medium">Deadline:</span>{' '} + {new Date(data.getModule.taskDeadline).toLocaleDateString()} + </div> + )} + </div> + </SecondaryCard> + )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/issueQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleMutations.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/programsQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (9)
backend/.env.example(0 hunks)backend/apps/github/Makefile(2 hunks)backend/apps/github/api/internal/nodes/issue.py(2 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(2 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx(1 hunks)frontend/src/server/queries/issueQueries.ts(1 hunks)frontend/src/server/queries/moduleQueries.ts(2 hunks)frontend/src/types/issue.ts(2 hunks)
💤 Files with no reviewable changes (1)
- backend/.env.example
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/apps/github/Makefile
- frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
- frontend/src/server/queries/issueQueries.ts
- frontend/tests/unit/components/CardDetailsPage.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
PR: OWASP/Nest#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/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
🧬 Code graph analysis (3)
backend/apps/github/api/internal/nodes/issue.py (5)
backend/apps/github/api/internal/nodes/pull_request.py (1)
PullRequestNode(19-44)frontend/src/types/__generated__/graphql.ts (2)
PullRequestNode(557-569)UserNode(927-948)frontend/src/types/issue.ts (1)
Issue(4-21)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/mentorship/api/internal/nodes/module.py (1)
interested_users(90-100)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (4)
frontend/src/server/queries/issueQueries.ts (3)
GET_MODULE_ISSUE_VIEW(3-48)ASSIGN_ISSUE_TO_USER(50-66)UNASSIGN_ISSUE_FROM_USER(68-84)frontend/src/app/global-error.tsx (1)
ErrorDisplay(28-51)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
frontend/src/types/issue.ts (2)
frontend/src/types/pullRequest.ts (1)
PullRequest(3-13)frontend/src/types/user.ts (1)
RepositoryDetails(5-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
backend/apps/github/api/internal/nodes/issue.py (1)
60-63: LGTM! Good use ofselect_relatedfor performance.The resolver correctly uses
select_related("author", "repository")to prevent N+1 queries when accessing nested PullRequestNode fields.frontend/src/server/queries/moduleQueries.ts (1)
63-63: Adding labels looks good.This enables filtering/rendering on the module page. No issues.
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
176-186: Good fix: guarded Next/Image src usage.Avatar src is now conditionally rendered with a fallback placeholder across assignees, PR authors, and interested users. This prevents Next/Image crashes when URLs are missing.
Also applies to: 224-237, 292-302
...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
🧹 Nitpick comments (1)
frontend/src/types/issue.ts (1)
16-16: Clarify the distinction betweenbodyandsummary.Both
bodyandsummaryfields are present, which could be confusing without clear documentation of their purposes. Based on the PR objectives mentioning truncating descriptions after 10–15 rows, it appearsbodymay hold the full description whilesummaryholds a truncated version—but this should be explicitly documented.Consider adding JSDoc comments to clarify:
- What content each field contains
- When to use one vs. the other
- Whether they're mutually exclusive or complementary
Example documentation:
projectUrl: string pullRequests?: PullRequest[] repository?: RepositoryDetails repositoryLanguages?: string[] + /** Full issue description/body text from GitHub */ body?: string title: string state?: string + /** Truncated version of the issue description for list/preview display */ summary?: string updatedAt: numberAlso applies to: 19-19
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/manage.py(1 hunks)backend/tests/apps/github/api/internal/nodes/issue_test.py(1 hunks)frontend/src/types/issue.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/github/api/internal/nodes/issue_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/types/issue.ts (2)
frontend/src/types/pullRequest.ts (1)
PullRequest(3-13)frontend/src/types/user.ts (1)
RepositoryDetails(5-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
frontend/src/types/issue.ts (3)
1-1: LGTM!The import statement correctly brings in the PullRequest type, which is used for the new
pullRequestsfield below.
13-13: LGTM!The
pullRequestsfield addition aligns with the PR objectives to link pull requests with issues. The optional array type is appropriate for representing zero or more related PRs.
18-18: LGTM!The
statefield is a standard addition for tracking issue status (e.g., "open", "closed"). The optional type is appropriate for backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/apps/github/api/internal/nodes/issue.py (1)
54-62: Past review comment properly addressed!The
interested_users()resolver now correctly uses the reverse relationshipparticipant_interestswithselect_related("user")and appliesorder_by("user__login")for consistency. This eliminates the N+1 query issue flagged in the previous review.frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (3)
31-37: Remove redundant destructuring ofissueId.
issueIdis extracted twice: once on line 31 and again on line 36. Consolidate into a single destructuring call.Apply this diff:
- const { issueId } = useParams() as { issueId: string } - - const { programKey, moduleKey } = useParams() as { + const { programKey, moduleKey, issueId } = useParams() as { programKey: string moduleKey: string issueId: string }
347-347: Add fallback for missing login.Displaying
@{u.login}without a fallback will render "@undefined" when login is missing.Apply this diff:
- <span className="text-sm font-medium">@{u.login}</span> + <span className="text-sm font-medium">@{u.login || u.name || 'Unknown'}</span>
349-374: Addaria-labelto assign button for accessibility.The assign button lacks an
aria-labelattribute. The unassign button (line 231) properly includes one.Apply this diff:
<button type="button" + aria-label={ + !issueId ? 'Loading issue…' : assigning ? 'Assigning…' : `Assign to @${u.login}` + } disabled={!issueId || assigning}
🧹 Nitpick comments (1)
backend/apps/github/api/internal/nodes/issue.py (1)
44-52: Consider query-level prefetching for list views.The
assignees()andlabels()resolvers are appropriate for single-issue access. However, if these resolvers are called when fetching multiple issues in list views, consider adding query-level prefetching (e.g., using strawberry-django's query optimization features or dataloaders) to avoid N+1 queries.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/github/api/internal/nodes/issue.py(3 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
PR: OWASP/Nest#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/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
🧬 Code graph analysis (2)
backend/apps/github/api/internal/nodes/issue.py (2)
backend/apps/github/api/internal/nodes/pull_request.py (1)
PullRequestNode(19-44)backend/apps/mentorship/api/internal/nodes/module.py (1)
interested_users(90-100)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (4)
frontend/src/server/queries/issueQueries.ts (3)
GET_MODULE_ISSUE_VIEW(3-48)ASSIGN_ISSUE_TO_USER(50-66)UNASSIGN_ISSUE_FROM_USER(68-84)frontend/src/app/global-error.tsx (1)
ErrorDisplay(28-51)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
🔇 Additional comments (1)
backend/apps/github/api/internal/nodes/issue.py (1)
64-67: Well-optimized resolver!The
pull_requests()resolver correctly usesselect_related("author", "repository")to prefetch related objects, preventing N+1 queries when accessing pull request details.
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/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: 3
♻️ Duplicate comments (6)
backend/apps/mentorship/api/internal/mutations/module.py (4)
121-161: Fix exception instantiation: use positional args, notmsg=keyword.Django's
ObjectDoesNotExistandPermissionDenieddo not accept amsg=keyword argument. Passing it raisesTypeErrorat runtime. Pass the message as the first positional argument, and add explicit messages to all bare raises.Apply this diff:
if module is None: - raise ObjectDoesNotExist(msg="Module not found.") + raise ObjectDoesNotExist("Module not found.") mentor = Mentor.objects.filter(nest_user=user).first() if mentor is None: - raise PermissionDenied(msg="Only mentors can assign issues.") + raise PermissionDenied("Only mentors can assign issues.") if not module.program.admins.filter(id=mentor.id).exists(): - raise PermissionDenied + raise PermissionDenied("Only program admins can assign issues.") gh_user = GithubUser.objects.filter(login=user_login).first() if gh_user is None: - raise ObjectDoesNotExist(msg="Assignee not found.") + raise ObjectDoesNotExist("Assignee not found.") issue = module.issues.filter(number=issue_number).first() if issue is None: - raise ObjectDoesNotExist(msg="Issue not found in this module.") + raise ObjectDoesNotExist("Issue not found in this module.")
163-201: Fix exception instantiation: use positional args and add missing messages.Same issue as
assign_issue_to_user: use positional arguments instead ofmsg=keyword, and add explicit messages to all bare raises.Apply this diff:
if module is None: - raise ObjectDoesNotExist + raise ObjectDoesNotExist("Module not found.") mentor = Mentor.objects.filter(nest_user=user).first() if mentor is None: - raise PermissionDenied + raise PermissionDenied("Only mentors can unassign issues.") if not module.program.admins.filter(id=mentor.id).exists(): - raise PermissionDenied + raise PermissionDenied("Only program admins can unassign issues.") gh_user = GithubUser.objects.filter(login=user_login).first() if gh_user is None: - raise ObjectDoesNotExist(msg="Assignee not found.") + raise ObjectDoesNotExist("Assignee not found.") issue = module.issues.filter(number=issue_number).first() if issue is None: - raise ObjectDoesNotExist(msg=f"Issue {issue_number} not found in this module.") + raise ObjectDoesNotExist(f"Issue {issue_number} not found in this module.")
214-236: Fix exception instantiation: use positional args and add missing message.Same critical issue: use positional arguments instead of
msg=keyword, and add explicit message to the bare raise.Apply this diff:
if module is None: - raise ObjectDoesNotExist(msg="Module not found.") + raise ObjectDoesNotExist("Module not found.") mentor = Mentor.objects.filter(nest_user=user).first() if mentor is None: - raise PermissionDenied(msg="Only mentors can set deadlines.") + raise PermissionDenied("Only mentors can set deadlines.") if not module.program.admins.filter(id=mentor.id).exists(): - raise PermissionDenied + raise PermissionDenied("Only program admins can set deadlines.") issue = ( module.issues.select_related("repository").prefetch_related("assignees") .filter(number=issue_number) .first() ) if issue is None: - raise ObjectDoesNotExist(msg="Issue not found in this module.") + raise ObjectDoesNotExist("Issue not found in this module.")
275-285: Fix exception instantiation inupdate_module: use positional args.Same critical issue in the
update_modulemethod: use positional arguments instead ofmsg=keyword.Apply this diff:
except Module.DoesNotExist as e: - raise ObjectDoesNotExist(msg="Module not found.") from e + raise ObjectDoesNotExist("Module not found.") from e try: creator_as_mentor = Mentor.objects.get(nest_user=user) except Mentor.DoesNotExist as err: logger.warning( "User '%s' is not a mentor and cannot edit modules.", user.username, exc_info=True, ) - raise PermissionDenied(msg="Only mentors can edit modules.") from err + raise PermissionDenied("Only mentors can edit modules.") from errfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (2)
457-457: Add fallback for missing login.Line 457 displays
@{u.login}without a fallback. Ifu.loginis missing or undefined, this will render "@undefined", which is confusing.Apply this diff to add a fallback:
- <span className="text-sm font-medium">@{u.login}</span> + <span className="text-sm font-medium">@{u.login || u.name || 'Unknown'}</span>
459-481: Addaria-labelto assign button for accessibility.The assign button lacks an
aria-labelattribute, which is important for screen reader users. The unassign button on line 345 properly includes anaria-label.Apply this diff to add the
aria-label:<button type="button" + aria-label={`Assign issue to @${u.login}`} disabled={!issueId || assigning}
🧹 Nitpick comments (5)
backend/manage.py (1)
14-14: Add trailing newline.Static analysis (Ruff W292) flags that the file is missing a newline at the end. This is a minor style issue but aligns with Python conventions.
Apply this diff to add the trailing newline:
execute_from_command_line(sys.argv) +frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (4)
100-102: Consider timezone implications for deadline initialization.The deadline input is initialized using
.slice(0, 10)to extract the date portion. While this works for ISO strings, it assumes thetaskDeadlineis always in ISO format and doesn't account for timezone conversions that might be needed for user-local display.Consider adding a comment or using a more explicit date formatting utility:
const [deadlineInput, setDeadlineInput] = useState<string>( - taskDeadline ? new Date(taskDeadline).toISOString().slice(0, 10) : '' + // Extract YYYY-MM-DD from ISO timestamp for date input + taskDeadline ? new Date(taskDeadline).toISOString().split('T')[0] : '' )
247-272: Extract complex deadline display logic to a helper function.The inline deadline display logic is 25 lines of nested conditionals and calculations. This reduces readability and makes the JSX harder to scan.
Extract to a helper function:
const formatDeadlineDisplay = (deadline: string | undefined): { text: string; className: string } => { if (!deadline) { return { text: 'No deadline set', className: 'text-xs font-medium text-gray-600 dark:text-gray-300' } } const d = new Date(deadline) const today = new Date() const isPast = d.setHours(0, 0, 0, 0) < today.setHours(0, 0, 0, 0) const diffDays = Math.ceil((d.getTime() - today.getTime()) / (1000 * 60 * 60 * 24)) const suffix = isPast ? '(overdue)' : diffDays > 0 ? `(in ${diffDays} days)` : '(today)' return { text: `${d.toLocaleDateString()} ${suffix}`, className: 'text-xs font-medium text-red-700 dark:text-red-300' } }Then use it in the JSX:
{(() => { const { text, className } = formatDeadlineDisplay(taskDeadline) return <span className={className}>{text}</span> })()}
257-257: Mutating date object in comparison affects subsequent logic.Line 257 uses
d.setHours(0, 0, 0, 0)which mutates the date objectd. This mutation affects the subsequentd.toLocaleDateString()call on line 264, though in this case it only resets the time component so the date display should still be correct.For clarity and to avoid potential bugs, use a separate date object for comparison:
- const isPast = d.setHours(0, 0, 0, 0) < today.setHours(0, 0, 0, 0) + const dNormalized = new Date(d).setHours(0, 0, 0, 0) + const todayNormalized = new Date(today).setHours(0, 0, 0, 0) + const isPast = dNormalized < todayNormalized
32-491: Consider breaking down this large component.The component is 460 lines long and handles multiple concerns: data fetching, mutations, state management, and rendering multiple UI sections. This makes it harder to test, maintain, and reuse.
Consider extracting sections into smaller components:
IssueHeader(lines 159-182)IssueDescription(lines 184-188)TaskTimeline(lines 190-287)IssueLabels(lines 289-308)IssueAssignees(lines 310-367)IssuePullRequests(lines 369-428)InterestedUsers(lines 430-487)Each component could receive the necessary data and callbacks as props, improving testability and following the Single Responsibility Principle. Based on learnings from the OWASP/Nest project, you may want to defer this refactoring until these sections are reused elsewhere to follow the YAGNI principle.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/mentorship/api/internal/mutations/module.py(5 hunks)backend/manage.py(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(1 hunks)frontend/src/server/queries/issueQueries.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/server/queries/issueQueries.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
PR: OWASP/Nest#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/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
🧬 Code graph analysis (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (4)
frontend/src/server/queries/issueQueries.ts (4)
GET_MODULE_ISSUE_VIEW(3-48)ASSIGN_ISSUE_TO_USER(50-66)UNASSIGN_ISSUE_FROM_USER(68-84)SET_TASK_DEADLINE(86-102)frontend/src/app/global-error.tsx (1)
ErrorDisplay(28-51)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
backend/apps/mentorship/api/internal/mutations/module.py (4)
backend/apps/mentorship/models/task.py (1)
Task(10-80)backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/mentorship/api/internal/nodes/module.py (2)
ModuleNode(18-128)issues(45-56)backend/apps/mentorship/models/module.py (1)
Module(17-104)
🪛 GitHub Actions: Run CI/CD
backend/apps/mentorship/api/internal/mutations/module.py
[error] 205-205: ruff: Missing docstring in public method (D102) in set_task_deadline. Add a docstring to the public method.
[warning] 1-1: ruff-format: 1 file reformatted during pre-commit; code style updated. Run 'pre-commit run --all-files' or re-commit to apply changes.
🪛 Ruff (0.14.0)
backend/apps/mentorship/api/internal/mutations/module.py
205-205: Missing docstring in public method
(D102)
backend/manage.py
14-14: No newline at end of file
Add trailing newline
(W292)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
backend/manage.py (1)
14-14: Inconsistency: AI summary mentions dotenv integration, but no dotenv code is present.The AI-generated summary states that this file "adds dotenv integration: imports load_dotenv from dotenv and calls load_dotenv() inside the main entry point," but the provided code contains no dotenv imports or
load_dotenv()calls. The past review comment also referencedload_dotenv()usage around lines 8-11, yet the current code shows only standard Django setup withos.environ.setdefaultcalls.Please clarify:
- Was the dotenv integration removed in this change?
- Is the AI summary describing changes from a different commit?
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (3)
185-187: Good use of Markdown component for issue description.The description now uses the
Markdowncomponent with theline-clamp-[15]class for truncation, addressing the feedback to support markdown rendering.The implementation correctly sanitizes and renders markdown content while limiting the display to approximately 15 lines as specified.
168-176: Issue state label properly implemented.The issue state (Open/Closed) is now displayed as a colored badge next to the issue number, using GitHub-style colors (green for open, red for closed). This addresses the feedback to show the current state of the issue.
408-420: PR state colors align with GitHub conventions.The PR states use appropriate colors: purple for merged, red for closed, and green for open. This follows GitHub's familiar color scheme and improves user recognition without needing to read the labels.
This addresses the feedback to use traditional GitHub colors for PR states, making the interface more intuitive for users familiar with GitHub.
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Outdated
Show resolved
Hide resolved
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
backend/apps/mentorship/api/internal/mutations/module.py (4)
141-141: Critical: Removemsg=keyword and add missing exception messages.Django's
ObjectDoesNotExistandPermissionDenieddo not accept amsg=keyword argument. This will raiseTypeErrorat runtime. Additionally, Line 147 raisesPermissionDeniedwithout a message.Apply this diff:
- raise ObjectDoesNotExist(msg="Module not found.") + raise ObjectDoesNotExist("Module not found.") - raise PermissionDenied(msg="Only mentors can assign issues.") + raise PermissionDenied("Only mentors can assign issues.") if not module.program.admins.filter(id=mentor.id).exists(): - raise PermissionDenied + raise PermissionDenied("Only program admins can assign issues.") - raise ObjectDoesNotExist(msg="Assignee not found.") + raise ObjectDoesNotExist("Assignee not found.") - raise ObjectDoesNotExist(msg="Issue not found in this module.") + raise ObjectDoesNotExist("Issue not found in this module.")Also applies to: 145-145, 147-147, 151-151, 155-155
183-183: Critical: Removemsg=keyword and add missing exception messages.Same issue as in
assign_issue_to_user:ObjectDoesNotExistandPermissionDenieddo not acceptmsg=keyword arguments. Lines 183, 187, and 189 raise exceptions without messages.Apply this diff:
- raise ObjectDoesNotExist + raise ObjectDoesNotExist("Module not found.") mentor = Mentor.objects.filter(nest_user=user).first() if mentor is None: - raise PermissionDenied + raise PermissionDenied("Only mentors can unassign issues.") if not module.program.admins.filter(id=mentor.id).exists(): - raise PermissionDenied + raise PermissionDenied("Only program admins can unassign issues.") - raise ObjectDoesNotExist(msg="Assignee not found.") + raise ObjectDoesNotExist("Assignee not found.") - raise ObjectDoesNotExist(msg=f"Issue {issue_number} not found in this module.") + raise ObjectDoesNotExist(f"Issue {issue_number} not found in this module.")Also applies to: 187-187, 189-189, 193-193, 197-197
223-223: Critical: Removemsg=keyword and add missing exception message.Same
msg=keyword issue present in this mutation. Line 229 also raisesPermissionDeniedwithout a message.Apply this diff:
- raise ObjectDoesNotExist(msg="Module not found.") + raise ObjectDoesNotExist("Module not found.") - raise PermissionDenied(msg="Only mentors can set deadlines.") + raise PermissionDenied("Only mentors can set deadlines.") if not module.program.admins.filter(id=mentor.id).exists(): - raise PermissionDenied + raise PermissionDenied("Only program admins can set deadlines.") - raise ObjectDoesNotExist(msg="Issue not found in this module.") + raise ObjectDoesNotExist("Issue not found in this module.")Also applies to: 227-227, 229-229, 238-238
277-277: Critical: Removemsg=keyword from exceptions.Same
msg=keyword issue inupdate_module.Apply this diff:
- raise ObjectDoesNotExist(msg="Module not found.") from e + raise ObjectDoesNotExist("Module not found.") from e - raise PermissionDenied(msg="Only mentors can edit modules.") from err + raise PermissionDenied("Only mentors can edit modules.") from errAlso applies to: 287-287
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (3)
33-34: Consolidate params destructuring.Lines 33-34 perform redundant destructuring. Extract all three keys in a single statement.
Apply this diff:
- const params = useParams() as { programKey: string; moduleKey: string; issueId: string } - const { programKey, moduleKey, issueId } = params + const { programKey, moduleKey, issueId } = useParams() as { + programKey: string + moduleKey: string + issueId: string + }
465-465: Add fallback for missing login.If
u.loginis undefined, line 465 will render "@undefined". Provide a fallback value.Apply this diff:
- <span className="text-sm font-medium">@{u.login}</span> + <span className="text-sm font-medium">@{u.login || u.name || 'Unknown'}</span>
467-489: Add aria-label to assign button for accessibility.The assign button lacks an
aria-label, while the unassign button on line 344 properly includes one. Add a descriptive label for screen reader users.Apply this diff:
<button type="button" + aria-label={`Assign issue to @${u.login || u.name || 'user'}`} disabled={!issueId || assigning}
🧹 Nitpick comments (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (2)
174-189: Simplify tooltip isDisabled logic.The ternary on line 180 can be simplified. A tooltip should be disabled when the title is short enough to display fully (≤50 chars).
Apply this diff:
- isDisabled={issue.title.length > 50 ? false : true} + isDisabled={issue.title.length <= 50}
228-251: Simplify assignee data access.The assignee lookup on lines 249-250 searches through
data?.getModule?.issuesto find matching issue byobjectID. Since you're already mappingmoduleData?.issuesintomoduleIssues, consider including assignees in that normalized structure (lines 43-62) to avoid this nested lookup.If assignees are available in the server response, add them to the Issue mapping:
const moduleIssues: Issue[] = useMemo(() => { return (moduleData?.issues || []).map((i) => ({ author: i.author, createdAt: i.createdAt, hint: '', labels: i.labels || [], number: i.number, organizationName: i.organizationName, projectName: moduleData?.projectName || '', projectUrl: '', repository: undefined, repositoryLanguages: [], body: i.body || '', title: i.title, state: i.state, updatedAt: i.createdAt, url: i.url, objectID: i.id, + assignees: i.assignees || [], })) }, [moduleData])Then access assignees directly from
issue.assigneesin the table row.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/mentorship/api/internal/mutations/module.py(6 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
PR: OWASP/Nest#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/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
🧬 Code graph analysis (3)
backend/apps/mentorship/api/internal/mutations/module.py (4)
backend/apps/mentorship/models/issue_user_interest.py (1)
IssueUserInterest(6-31)backend/apps/mentorship/models/task.py (1)
Task(10-80)backend/apps/mentorship/api/internal/nodes/module.py (2)
ModuleNode(18-128)issues(45-56)backend/apps/mentorship/models/mentor.py (1)
Mentor(11-40)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (3)
frontend/src/server/queries/moduleQueries.ts (1)
GET_MODULE_ISSUES(80-106)frontend/src/app/global-error.tsx (2)
handleAppError(66-86)ErrorDisplay(28-51)frontend/src/types/issue.ts (1)
Issue(4-23)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (4)
frontend/src/server/queries/issueQueries.ts (4)
GET_MODULE_ISSUE_VIEW(3-48)ASSIGN_ISSUE_TO_USER(50-66)UNASSIGN_ISSUE_FROM_USER(68-84)SET_TASK_DEADLINE(86-102)frontend/src/app/global-error.tsx (1)
ErrorDisplay(28-51)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (1)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
298-305: Labels correctly rendered as non-interactive elements.The labels are now
<span>elements rather than<button>elements. Since this is the single-issue detail view and labels don't need to be interactive here (unlike in a list view where they might filter), this is the appropriate semantic choice.
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (2)
170-186: Simplify inverted boolean logic.The
isDisabledprop uses inverted ternary logic that can be simplified for better readability.Apply this diff:
<Tooltip closeDelay={100} delay={100} showArrow content={issue.title} placement="bottom" - isDisabled={issue.title.length > 50 ? false : true} + isDisabled={issue.title.length <= 50} >
224-248: Simplify assignee rendering after including assignees in ModuleIssueRow.After adding assignees to the ModuleIssueRow type (as suggested in the previous comment), this complex lookup can be replaced with direct access to
issue.assignees.After applying the previous fix, replace this entire cell content with:
<td className="px-6 py-4 text-sm whitespace-nowrap text-gray-700 dark:text-gray-300"> - {((assignees) => - assignees?.length ? ( + {issue.assignees?.length ? ( <div className="flex items-center gap-2"> <div className="flex items-center gap-2"> <Image height={24} width={24} - src={assignees[0].avatarUrl} - alt={assignees[0].login} + src={issue.assignees[0].avatarUrl} + alt={issue.assignees[0].login} className="rounded-full" /> - <span>{assignees[0].login || assignees[0].name}</span> + <span>{issue.assignees[0].login || issue.assignees[0].name}</span> </div> - {assignees.length > 1 && ( + {issue.assignees.length > 1 && ( <div className="flex h-6 w-6 items-center justify-center rounded-full bg-gray-200 text-xs font-medium text-gray-600 dark:bg-gray-700 dark:text-gray-300"> - +{assignees.length - 1} + +{issue.assignees.length - 1} </div> )} </div> - ) : null)( - (data?.getModule?.issues || []).find((i) => i.id === issue.objectID) - ?.assignees - )} + ) : null} </td>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (3)
frontend/src/server/queries/moduleQueries.ts (1)
GET_MODULE_ISSUES(80-106)frontend/src/app/global-error.tsx (2)
handleAppError(66-86)ErrorDisplay(28-51)backend/apps/github/api/internal/nodes/issue.py (2)
labels(50-52)assignees(45-47)
🪛 GitHub Actions: Run CI/CD
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
[warning] 51-51: Unexpected any. Specify a different type (no-explicit-any).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (1)
172-187: LGTM! Tooltip implementation addresses previous feedback.The tooltip correctly displays the full title on hover for truncated text, addressing the previous review comment.
Consider these minor simplifications:
isDisabled={issue.title.length > 50 ? false : true} + isDisabled={issue.title.length <= 50}- className="block max-w-md cursor-pointer truncate text-left hover:underline" + className="block max-w-md truncate text-left hover:underline"Note: Buttons already have
cursor: pointerby default, so the class is redundant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (2)
frontend/src/server/queries/moduleQueries.ts (1)
GET_MODULE_ISSUES(80-106)frontend/src/app/global-error.tsx (2)
handleAppError(66-86)ErrorDisplay(28-51)
🔇 Additional comments (12)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (12)
1-16: LGTM! Clean imports and constants.The imports are appropriate and the constants (LABEL_ALL, ITEMS_PER_PAGE, MAX_VISIBLE_LABELS) are well-defined for managing the issues list page.
18-24: LGTM! State management initialized correctly.The component properly initializes state from URL query parameters, enabling shareable filtered URLs.
25-35: LGTM! GraphQL query configured correctly.The query variables are properly computed, the skip condition prevents unnecessary requests, and the cache-and-network fetch policy is appropriate for potentially changing data.
37-39: LGTM! Error handling implemented correctly.The error effect properly delegates to the centralized
handleAppErrorutility, which handles toast notifications and Sentry logging.
41-60: LGTM! Data transformation improved based on previous reviews.The ModuleIssueRow type now includes the assignees field, the map function is properly typed (no longer using
any), and the mapping correctly extracts only the fields returned by the GraphQL query.
62-75: LGTM! Robust label and pagination computation.The code correctly calculates pagination and provides a solid fallback for label extraction when
availableLabelsis not provided by the server.
77-95: LGTM! Event handlers implemented correctly.The handlers properly manage state, synchronize URL parameters, and reset pagination when filters change. Navigation follows Next.js patterns.
97-99: LGTM! Proper loading and error state handling.The component correctly shows a loading spinner during data fetch and displays a 404 error page when the module is not found.
107-135: LGTM! HeroUI Select component fixed correctly.The Select component now properly uses
selectedKeysas a Set andonSelectionChangeto handle selection changes, addressing the critical issue from the previous review.
200-224: LGTM! Label rendering with overflow indicator.The labels are correctly displayed with a "+N more" indicator when there are more than MAX_VISIBLE_LABELS, providing good UX for issues with many labels.
226-246: LGTM! Assignee display fixed per previous feedback.The assignee column now correctly displays nothing when there are no assignees (instead of showing "Unassigned"), addressing the previous review comment. Multiple assignees are handled well with the count badge.
249-270: LGTM! Empty state and pagination implemented.The empty state provides clear feedback when no issues match the filter. The Pagination component addresses the previous review request for pagination support on this page.
|
|
||
| <div className="flex flex-col gap-2"> | ||
| <div className="flex flex-wrap items-center gap-2"> | ||
| <span className="font-medium">Deadline:</span> |
There was a problem hiding this comment.
Can we simplify this logic so that the edit form opens up as soon as the user clicks the "No deadline set" or Current deadline date? Meaning that whole thing would be like a button trigger.
We won't need edit button this way.
We also won't need close button for editing. To cancel the change user can click out of the "calendar" view, use Clear on it or edit manually.
And we will accept the edit as soon as user selects the date. This should set it to deadline automatically, without having to click Save.
This will simplify the view a lot!
| : 'bg-[#cb2431] text-white' | ||
| }`} | ||
| > | ||
| {issue.state === 'open' ? 'Open' : 'Closed'} |
| : 'bg-[#cb2431] text-white' | ||
| }`} | ||
| > | ||
| {issue.state === 'open' ? 'Open' : 'Closed'} |
There was a problem hiding this comment.
Also I do believe that we should be able to show the Merged state as well in case if the attached PR was merged 🤔
It would make sense to differentiate these 2 views, since Closed more of a negative label, where Merged is definitely a positive one.
| alt={issue.assignees[0].login} | ||
| className="rounded-full" | ||
| /> | ||
| <span>{issue.assignees[0].login || issue.assignees[0].name}</span> |
There was a problem hiding this comment.
Can we truncate the name in case it's longer?
| issue.state === 'open' | ||
| ? 'bg-[#2cbe4e] text-white' | ||
| : 'bg-[#cb2431] text-white' | ||
| }`} |
There was a problem hiding this comment.
Also I don't think these are the color that GitHub uses for these states. I wanted these to be exactly GitHub shades so that there's instant recognition of the state.
I inspected the GitHub labels for these and colors look different (GitHub shades on the right):
These would need to be updated on single Issue page too. and for Merged state I'd also prefer the GiHub shade.
There was a problem hiding this comment.
I updated the hex code.
| <ActionButton url={pr.url} tooltipLabel="View PR"> | ||
| <FontAwesomeIcon icon={faLink} /> | ||
| <span className="hidden sm:inline">View PR</span> | ||
| </ActionButton> |
There was a problem hiding this comment.
I think these are old ones 🤔
You should be able to get the colors from inspecting issues pages, then you can transform that into hex code:
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (2)
455-455: Add fallback for missing login.Line 455 displays
@{u.login}without a fallback. Ifu.loginis missing or undefined, this will render "@undefined", which is confusing for users.Apply this diff to add a fallback:
- <span className="text-sm font-medium">@{u.login}</span> + <span className="text-sm font-medium">@{u.login || u.name || 'Unknown'}</span></invoke_comment_end -->
457-479: Addaria-labelto assign button for accessibility.The assign button lacks an
aria-labelattribute, which is important for screen reader users. The unassign button properly includes anaria-label(line 334).Apply this diff to add the
aria-label:<button type="button" + aria-label={ + !issueId ? 'Loading issue…' : assigning ? 'Assigning…' : `Assign to ${u.login || u.name || 'user'}` + } disabled={!issueId || assigning}</invoke_comment_end -->
🧹 Nitpick comments (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (1)
174-191: Consider alignment between truncation and tooltip threshold.The Tooltip is enabled when
title.length > 50, but the visual truncation is controlled bymax-w-mdwidth on the button. These two thresholds may not align—a 40-character title with wide characters might truncate visually, while a 55-character title with narrow characters might fit. This could result in truncated titles without tooltips or non-truncated titles with unnecessary tooltips.Consider one of these approaches:
- Use a ref and measure actual overflow to conditionally enable the tooltip.
- Use a more generous character threshold (e.g., 30-40) to reduce false negatives.
- Always show the tooltip (remove
isDisabled) since it only appears on hover anyway.</invoke_comment_end -->
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
36-54: Non-overdue deadlines are styled the same as overdue ones.The
formatDeadlinefunction returnscolor: 'text-[#DA3633]'(red) for all deadlines, regardless of whether they're overdue, today, or in the future. This may confuse users—red typically signals urgency or a problem.Consider using different colors based on deadline status:
- Overdue: red (
text-[#DA3633])- Today: yellow/orange
- Future: gray or neutral
Apply this diff:
return { text: `${deadlineDate.toLocaleDateString()} ${statusText}`, - color: 'text-[#DA3633]', + color: isOverdue + ? 'text-[#DA3633]' + : daysLeft === 0 + ? 'text-yellow-600 dark:text-yellow-500' + : 'text-gray-600 dark:text-gray-300', }</invoke_comment_end -->
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/github/api/internal/nodes/issue.py(3 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(1 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx(1 hunks)frontend/src/server/queries/issueQueries.ts(1 hunks)frontend/src/server/queries/moduleQueries.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/server/queries/issueQueries.ts
- frontend/src/server/queries/moduleQueries.ts
- backend/apps/github/api/internal/nodes/issue.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
PR: OWASP/Nest#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/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
🧬 Code graph analysis (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (4)
frontend/src/server/queries/issueQueries.ts (4)
GET_MODULE_ISSUE_VIEW(3-49)ASSIGN_ISSUE_TO_USER(51-67)UNASSIGN_ISSUE_FROM_USER(69-85)SET_TASK_DEADLINE(87-103)frontend/src/app/global-error.tsx (1)
ErrorDisplay(28-51)frontend/src/components/MarkdownWrapper.tsx (1)
Markdown(4-17)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (2)
frontend/src/server/queries/moduleQueries.ts (1)
GET_MODULE_ISSUES(80-107)frontend/src/app/global-error.tsx (2)
handleAppError(66-86)ErrorDisplay(28-51)
🪛 GitHub Actions: Run CI/CD
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
[error] 274-274: Unstaged changes detected. Run 'make check' and use 'git add' to address it. Diff shows a className order change in the IssuesPage component.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
...nd/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
Show resolved
Hide resolved
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx
Outdated
Show resolved
Hide resolved
|
2 participants
Proposed change
Resolves #2239
Tasks
Steps
make mentorship-update-commentscmd to sync latest comments update