feat: add skeleton loaders for Organizations and Members Details pages (#2852) #2890
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces spinner loading UIs with new member and organization skeleton components, registers them in SkeletonsBase, updates member and organization page loading branches to render skeletons (with data-testid), and adjusts unit tests to assert skeleton presence and absence of sponsor blocks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (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: 2
🧹 Nitpick comments (2)
frontend/src/components/skeletons/OrganizationDetailsPageSkeleton.tsx (1)
3-220: Consider adding accessibility attributes for screen reader users.The skeleton component provides visual loading feedback but lacks ARIA attributes to communicate loading state to assistive technologies. Consider wrapping the skeleton in a container with
role="status"andaria-live="polite"oraria-busy="true"to announce the loading state to screen reader users.Apply this diff to improve accessibility:
const OrganizationDetailsPageSkeleton = () => { return ( - <div className="min-h-screen bg-white p-8 text-gray-600 dark:bg-[#212529] dark:text-gray-300"> + <div className="min-h-screen bg-white p-8 text-gray-600 dark:bg-[#212529] dark:text-gray-300" role="status" aria-label="Loading organization details"> <div className="mx-auto max-w-6xl">frontend/src/components/skeletons/MemberDetailsPageSkeleton.tsx (1)
4-232: Consider accessibility attributes and consistent iteration patterns.Similar to
OrganizationDetailsPageSkeleton, this component would benefit from accessibility attributes to communicate loading state to screen reader users (e.g.,role="status",aria-label).Additionally, the component uses mixed iteration patterns:
[1, 2, 3, 4].map((i) => ...)in some places andArray.from({ length: 5 }, (_, i) => ...)in others. While both work correctly, using a consistent pattern throughout would improve code maintainability.For accessibility, apply a similar diff as suggested for OrganizationDetailsPageSkeleton:
const MemberDetailsPageSkeleton: React.FC = () => { return ( - <div className="min-h-screen bg-white p-8 text-gray-600 dark:bg-[#212529] dark:text-gray-300"> + <div className="min-h-screen bg-white p-8 text-gray-600 dark:bg-[#212529] dark:text-gray-300" role="status" aria-label="Loading member details"> <div className="mx-auto max-w-6xl">For consistency, consider standardizing on one iteration approach (either array literals for fixed small counts or
Array.fromthroughout).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/__tests__/unit/pages/OrganizationDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/app/organizations/[organizationKey]/page.tsx(2 hunks)frontend/src/components/SkeletonsBase.tsx(2 hunks)frontend/src/components/skeletons/MemberDetailsPageSkeleton.tsx(1 hunks)frontend/src/components/skeletons/OrganizationDetailsPageSkeleton.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/UserDetails.test.tsx
🔇 Additional comments (4)
frontend/src/app/members/[memberKey]/page.tsx (1)
28-28: LGTM! Clean integration of skeleton loading state.The import and usage of
MemberDetailsPageSkeletonproperly replaces the loading spinner with a skeleton UI, improving the user experience during data loading.Also applies to: 102-104
frontend/src/app/organizations/[organizationKey]/page.tsx (1)
17-17: LGTM! Consistent skeleton implementation.The skeleton loading state is properly integrated, matching the pattern used in the member details page.
Also applies to: 49-51
frontend/src/components/SkeletonsBase.tsx (1)
5-6: LGTM! Skeleton routing properly extended.The new skeleton components are correctly imported and integrated into the switch statement. The dual mapping of both
'member-details'and'members'toMemberDetailsPageSkeletonappears intentional for handling different route contexts.Also applies to: 72-76
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
546-555: LGTM! Appropriate test coverage for sponsor block.The test correctly verifies that the sponsor block does not render on the user details page, matching the similar test added to OrganizationDetails.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)
102-104: Consider adding minimal loading a11y semantics on the skeleton wrapper.
If not already handled insideMemberDetailsPageSkeleton, addingaria-busy="true"(and optionallyrole="status") to the wrapper helps assistive tech.- return <div data-testid="user-loading-skeleton"><MemberDetailsPageSkeleton /></div> + return ( + <div data-testid="user-loading-skeleton" aria-busy="true" role="status"> + <MemberDetailsPageSkeleton /> + </div> + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/pages/OrganizationDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/app/organizations/[organizationKey]/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/app/organizations/[organizationKey]/page.tsx
- frontend/tests/unit/pages/UserDetails.test.tsx
- frontend/tests/unit/pages/OrganizationDetails.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
🔇 Additional comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)
26-29: Import change looks good; keep skeletons import path consistent with the repo’s pattern.
No concerns with switching fromLoadingSpinnertoMemberDetailsPageSkeletonat the import level.
|
Hi @arkid15r |
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@dhirukumar This looks good (with minor required updates), but I am concerned about this approach in general.
With current approach OrganizationDetailsPageSkeleton (and Members skeleton page as well) hardcodes the layout. If we ever add new components or rearrange the layout we'd also need to remember to update the skeletons to match.
I feel like it makes much more sense to add skeletons for specific components (like RecentReleases, RecentPullRequests, etc.) and render them while loading the page.
This can be handled as all or nothing with a global loading state passed in from the parent component to DetailsCard component or any other component we will work with. 🤔
However, since switching to this new model with separate components skeletons will take awhile to implement (work on adding skeletons for all components, update pages where we use them) - I think it makes sense to still work on this and merge this in for now 👍🏼
I left a couple of requests to address, but overall this looks good ⬇️
| {[1, 2, 3, 4, 5].map((i) => ( | ||
| <div | ||
| key={`pr-${i}`} | ||
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
There was a problem hiding this comment.
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" | |
| className="mb-4 rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
| {[1, 2, 3, 4, 5].map((i) => ( | ||
| <div | ||
| key={`issue-${i}`} | ||
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
There was a problem hiding this comment.
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" | |
| className="mb-4 rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
| {Array.from({ length: 2 }, (_, i) => ( | ||
| <div | ||
| key={`milestone-${i}`} | ||
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
There was a problem hiding this comment.
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" | |
| className="mb-4 rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
| {Array.from({ length: 5 }, (_, i) => ( | ||
| <div | ||
| key={`issue-${i}`} | ||
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
There was a problem hiding this comment.
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" | |
| className="mb-4 rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
| </div> | ||
| </div> | ||
|
|
||
| {/* Recent Releases */} |
There was a problem hiding this comment.
The skeletons for Releases doesn't match at all to what we show on the loaded component 🤔 I believe this should be very similar to the rest of the columns we have:
| {Array.from({ length: 5 }, (_, i) => ( | ||
| <div | ||
| key={`pr-${i}`} | ||
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
There was a problem hiding this comment.
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" | |
| className="mb-4 rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
| </div> | ||
| </div> | ||
|
|
||
| {/* Recent Milestones - Empty State */} |
| {[1, 2, 3, 4, 5].map((i) => ( | ||
| <div | ||
| key={`release-${i}`} | ||
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
There was a problem hiding this comment.
| className="rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" | |
| className="mb-4 rounded-lg border-1 border-gray-200 bg-white p-4 shadow-xs dark:border-gray-700 dark:bg-gray-900" |
2 participants
Proposed change
Resolves #2852
This PR introduces skeleton loading components for the following pages:
🟦 Organization Details Page( Home ->Organizations ->OWASP)
OrganizationDetailsPageSkeletoncomponent🟩 Member Details Page( Home -> Members ->Björn Kimminich )
MemberDetailsPageSkeletoncomponent🔧 Additional Updates
/organizations/[organizationKey]/page.tsx/members/[memberKey]/page.tsxSkeletonsBase.tsxto support additional height variants used in detail viewsThese changes modernize the loading state experience and align both pages with Nest’s skeleton design standards.
Checklist
make check-testlocally and all tests passed