feat: display related milestones on public program page#3441
feat: display related milestones on public program page#3441kasya merged 9 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a ProgramNode.recent_milestones GraphQL field; extends program query and Program type with recentMilestones; passes recentMilestones to DetailsCard; implements Recent Milestones UI with show-more toggle and unit tests. (28 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/apps/mentorship/api/internal/nodes/program.py`:
- Around line 37-44: Update the recent_milestones resolver: change its return
type to list["MilestoneNode"] (no None), add an optional limit parameter with
default 5, order results by "-created_at", eager-load relations via
select_related("repository__organization", "author") and
prefetch_related("labels") to avoid N+1, and preserve .distinct() (apply
select_related/prefetch before calling distinct()) when filtering
Milestone.open_milestones by project_ids in recent_milestones so duplicates from
repositories linked to multiple projects are prevented.
|
@kasya, please review it and let me know if any changes are required. |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@HarshitVerma109 looks good on the UI!
Left some comments ⬇️
| @strawberry.field | ||
| def recent_milestones(self, limit: int = 5) -> list["MilestoneNode"]: | ||
| """Get the list of recent milestones for the program.""" | ||
| from apps.github.models.milestone import Milestone |
There was a problem hiding this comment.
Move imports to the top of the module
| {type === 'program' ? ( | ||
| recentMilestones && ( | ||
| <div className="[&_.grid]:!grid-cols-1 md:[&_.grid]:!grid-cols-2"> | ||
| <Milestones | ||
| data={recentMilestones} | ||
| showAvatar={showAvatar} | ||
| showSingleColumn={false} | ||
| /> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
I believe this should be under Modules block.
| return self.admins.all() | ||
|
|
||
| @strawberry.field | ||
| def recent_milestones(self, limit: int = 5) -> list["MilestoneNode"]: |
There was a problem hiding this comment.
Do you think we should limit to 5 on this view?
I can see how 5 is suitable for projects page where we have 2 columns with 5 elements.
But on the program page 5 items look kinda unfinished:
There was a problem hiding this comment.
I was a bit confused, so I just set the limit to 5. Additionally, I suggest we add a 'Recent Issues' column, similar to the one on the project page
There was a problem hiding this comment.
Or should I limit this to 4 and add a "Show More" button? What do you think? Let me know if I'm wrong.
There was a problem hiding this comment.
@HarshitVerma109 either 4 or 6 would work just fine! 👍🏼
As for recent issues - I don't think we need to add that, at least not right now. Issues are available per module view.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/components/CardDetailsPage.tsx`:
- Around line 409-416: The Link currently uses href={milestone?.url || ''} which
makes an empty href navigate to the current page; update the JSX in
CardDetailsPage (the Link wrapping TruncatedText and the usage of milestone?.url
and milestone.title) to conditionally render: if milestone?.url is truthy render
the Link with target="_blank", otherwise render non-navigable plain text (e.g.,
a span/div with the same styling or TruncatedText alone) so clicks don’t trigger
unintended navigation.
- Around line 389-406: In CardDetailsPage, guard the avatar/link block against
missing author fields: check milestone?.author and required props (author.login
and author.avatarUrl) before rendering the Link/Image/Tooltip; if author is
missing render a safe fallback (e.g., plain text or a default avatar image URL
and no /members/ link) so you never create href={`/members/${undefined}`} or
pass undefined to Next/Image; update the block that currently uses Link, Image
and Tooltip to conditionally render based on those checks (or supply a
defaultAvatar constant) to prevent invalid hrefs and undefined src.
🧹 Nitpick comments (4)
frontend/src/components/CardDetailsPage.tsx (3)
65-72: Consider simplifying the type check logic.Adding
'program'toshowIssuesAndMilestonesbut then immediately excluding it at line 342 withtype !== 'program'is somewhat confusing. The function now returnstruefor programs, but programs never use the standard issues/milestones block.Consider either:
- Removing
'program'from this function since programs have their own milestone section, or- Renaming/documenting to clarify the intent
This keeps the code self-documenting and avoids confusion for future maintainers.
Option 1: Remove 'program' from showIssuesAndMilestones
const showIssuesAndMilestones = (type: CardType): boolean => - ['organization', 'program', 'project', 'repository', 'user'].includes(type) + ['organization', 'project', 'repository', 'user'].includes(type)Then simplify line 342:
-{showIssuesAndMilestones(type) && type !== 'program' && ( +{showIssuesAndMilestones(type) && (
373-376: Key fallback may cause collisions.Using
milestone.url || milestone.titleas the key could produce duplicates if multiple milestones share the same title and lack URLs. This would cause React reconciliation issues.Consider incorporating the index for uniqueness:
♻️ Suggested fix
- <div - key={milestone.url || milestone.title} + <div + key={milestone.url || `${milestone.title}-${index}`}
432-441: Minor: Inconsistent icon sizing.
FaFolderOpenusesh-5 w-4while other icons in the same row useh-4 w-4. This creates a subtle visual inconsistency.✨ Suggested fix for consistency
- <FaFolderOpen className="mr-2 h-5 w-4 shrink-0" /> + <FaFolderOpen className="mr-2 h-4 w-4 shrink-0" />frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
1711-1787: Good test coverage for the milestone toggle feature.The tests effectively cover:
- Initial rendering with milestone limit
- Expansion via "Show more"
- Collapse via "Show less"
- No toggle button at exactly the limit
Consider adding a test for the edge case when
recentMilestonesis empty or undefined for program type to ensure the section doesn't render:📋 Optional: Add edge case test
it('does not render milestones section when recentMilestones is empty for program type', () => { const programProps: DetailsCardProps = { ...defaultProps, type: 'program' as const, recentMilestones: [], modules: [], } render(<CardDetailsPage {...programProps} />) expect(screen.queryByText('Recent Milestones')).not.toBeInTheDocument() })
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/CardDetailsPage.tsx`:
- Around line 436-446: The Link href can produce "/organizations/undefined/..."
because milestone.organizationName is used without a guard even when
milestone.repositoryName exists; update the conditional rendering to require
both milestone.repositoryName and milestone.organizationName (or fallback to a
safe value) before rendering the Link, or build the href using a safe accessor
(e.g., only render Link when milestone.organizationName is truthy) so the Link
component and its TruncatedText remain inside a block that guarantees a valid
organizationName.
🧹 Nitpick comments (2)
frontend/src/components/CardDetailsPage.tsx (2)
65-66: Simplify the type guard logic.Adding
'program'toshowIssuesAndMilestonesbut then excluding it withtype !== 'program'on line 342 is contradictory. If programs use a custom milestones section, don't include'program'here.♻️ Suggested fix
const showIssuesAndMilestones = (type: CardType): boolean => - ['organization', 'program', 'project', 'repository', 'user'].includes(type) + ['organization', 'project', 'repository', 'user'].includes(type)Then remove the redundant guard on line 342:
- {showIssuesAndMilestones(type) && type !== 'program' && ( + {showIssuesAndMilestones(type) && (
374-376: Use a more unique key to avoid potential duplicates.If two milestones share the same title and both lack URLs, the key will collide. Consider using the index as a fallback or a composite key.
♻️ Suggested fix
<div - key={milestone.url || milestone.title} + key={milestone.url || `${milestone.title}-${index}`} className="mb-4 w-full rounded-lg bg-gray-200 p-4 dark:bg-gray-700" >
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/CardDetailsPage.tsx`:
- Around line 425-432: The Link rendering for milestone URLs (the JSX block that
checks milestone?.url and renders <Link ... href={milestone?.url}
target="_blank"> with <TruncatedText /> inside) should include rel="noopener
noreferrer" to prevent reverse-tabnabbing; update the Link element that opens
external URLs in a new tab to add rel="noopener noreferrer" whenever
target="_blank" is used and ensure the href uses milestone?.url as before.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@HarshitVerma109 Looks good 👍🏼 Thanks!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3441 +/- ##
==========================================
- Coverage 85.56% 85.56% -0.01%
==========================================
Files 461 461
Lines 14227 14249 +22
Branches 1895 1905 +10
==========================================
+ Hits 12174 12192 +18
Misses 1680 1680
- Partials 373 377 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
2 participants
Proposed change
Resolves #3245
This PR adds a "Related Milestones" section to the public program details page. It aggregates and displays milestone cards for all repositories associated with a program's modules, providing better visibility into ongoing work.
Key Changes:
recent_milestonesfield to theProgramNodeinprogram.py. This field aggregates open milestones from all repositories linked to the program's modules.recentMilestonesincluding URL and author details.CardDetailsPage.Checklist
make check-testlocally: all warnings addressed, tests passed