Add skeleton loading states for About, Organization, and Snapshot pages #2757
Conversation
…shot pages and updated corresponding page structures and all tests
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces LoadingSpinner with skeleton placeholders on About and Snapshots pages, adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (5)
frontend/__tests__/unit/pages/About.test.tsx (1)
414-416: Loading-state assertion works but is tightly coupled to Tailwind classesThe new check correctly verifies the skeleton-based loading state, but querying via
document.querySelectorAll('.bg-gray-100.dark\\:bg-gray-800')is brittle and bypasses Testing Library’s querying patterns. Consider exposing a more semantic handle fromAboutSkeleton(e.g.,role="status"on the main containers or adata-testid) and then usingscreen.getAllByRole('status')orgetAllByTestId(...)here. That would also avoid the awkward escaped selector and keep the test resilient to styling changes.frontend/__tests__/unit/pages/Snapshots.test.tsx (1)
62-63: Good switch to role-based loading assertionUsing
screen.getAllByRole('status')aligns nicely withSnapshotSkeleton’srole="status"and is more robust than image alt-text checks. If you want to tighten clarity, you could also rename the test description from “renders loading spinner” to mention skeletons, but that’s purely cosmetic.frontend/src/app/snapshots/page.tsx (1)
9-9: Snapshot skeleton grid and loading guard are wired correctlyThe new
SnapshotSkeletonimport, “View Snapshot” label, and theisLoading-guarded grid (skeletons vs. real cards / empty state) match the intended UX and the updated tests. If you ever revisit this, you could consider leaning directly on Apollo’sloadingflag instead of a separateisLoadingstate to cut a bit of bookkeeping, but the current approach is functionally sound.Also applies to: 44-44, 63-79
frontend/src/components/skeletons/SnapshotSkeleton.tsx (1)
10-36: SnapshotSkeleton structure looks solid; consider adding an accessible nameThe component gives a clear, flexible skeleton layout for snapshot cards, and
role="status"works well with the tests and intent of signaling a loading region. Right now, though, that status region has no accessible name or text, so screen readers may not convey what’s happening. Consider adding something likearia-label="Loading snapshot"(or a visually hidden descriptive text) on the root<div>so assistive tech users get meaningful feedback while the skeleton is shown.frontend/src/components/skeletons/AboutSkeleton.tsx (1)
3-152: Comprehensive AboutSkeleton matches the page layout; optional a11y/test improvementsThis skeleton does a good job mirroring all the major About sections (mission, features, leaders, contributors, tech, roadmap, story, timeline, stats) so the layout remains stable while data loads. To make it both more accessible and less tied to Tailwind classes in tests, you might consider giving the main skeleton cards a semantic marker—e.g., a wrapper with
role="status"/aria-busy="true"or a dedicateddata-testid—and then updating the About tests to query by role or test id instead of CSS selectors. That would keep the implementation free to evolve its exact styling without breaking tests while also giving assistive tech a clearer signal that content is loading.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/__tests__/unit/pages/About.test.tsx(1 hunks)frontend/__tests__/unit/pages/Snapshots.test.tsx(1 hunks)frontend/src/app/about/page.tsx(2 hunks)frontend/src/app/snapshots/page.tsx(3 hunks)frontend/src/components/SkeletonsBase.tsx(2 hunks)frontend/src/components/skeletons/AboutSkeleton.tsx(1 hunks)frontend/src/components/skeletons/SnapshotSkeleton.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Snapshots.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/About.test.tsx
🧬 Code graph analysis (1)
frontend/src/app/snapshots/page.tsx (1)
frontend/src/types/snapshot.ts (1)
Snapshot(15-20)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/SkeletonsBase.tsx
[warning] 23-23: Do not use Array index in keys
frontend/src/app/snapshots/page.tsx
[warning] 66-66: Do not use Array index in keys
frontend/__tests__/unit/pages/About.test.tsx
[warning] 415-415: String.raw should be used to avoid escaping \.
frontend/src/components/skeletons/SnapshotSkeleton.tsx
[warning] 16-19: Use instead of the "status" role to ensure accessibility across all devices.
🔇 Additional comments (2)
frontend/src/app/about/page.tsx (1)
42-42: AboutSkeleton integration for loading state looks correctImporting
AboutSkeletonand returning it whenprojectMetadataLoading || topContributorsLoading || leadersLoadingis true cleanly replaces the old spinner and keeps the rest of the control flow (404 handling, main render) unchanged. This should give a stable layout during load without altering data or error semantics.Also applies to: 100-104
frontend/src/components/SkeletonsBase.tsx (1)
3-6: Extended SkeletonsBase mappings are consistent and fix the special cases cleanlyFactoring user and snapshot skeleton grids into
userCardRender/snapshotCardRender, reusing the user grid fororganizations, and adding explicit cases forsnapshotsandaboutkeeps SkeletonsBase cohesive and avoids the previous “generic card” mismatch. The early returns for these index names also make the control flow clearer and sidestep the old unreachable-wrapper issue.Also applies to: 7-27, 62-69
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 (5)
frontend/__tests__/unit/pages/About.test.tsx(1 hunks)frontend/src/app/about/page.tsx(5 hunks)frontend/src/app/snapshots/page.tsx(3 hunks)frontend/src/components/SkeletonsBase.tsx(2 hunks)frontend/src/components/skeletons/SnapshotSkeleton.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/skeletons/SnapshotSkeleton.tsx
- frontend/tests/unit/pages/About.test.tsx
- frontend/src/app/snapshots/page.tsx
🔇 Additional comments (3)
frontend/src/components/SkeletonsBase.tsx (1)
3-70: LGTM! Clean implementation of new skeleton loading states.The changes follow existing patterns consistently:
- Updated key generation using template strings improves clarity over raw indices
- New
snapshotCardRenderproperly mirrorsuserCardRenderstructure- Switch cases for 'organizations', 'snapshots', and 'about' correctly return early, maintaining consistency with the 'users' case
- No control flow issues or unreachable code
frontend/src/app/about/page.tsx (2)
42-42: LGTM! AboutSkeleton successfully replaces LoadingSpinner.The integration correctly addresses the PR objective by providing a skeleton-based loading state instead of the generic spinner, maintaining consistent UX across the application.
Also applies to: 123-123
53-71: LGTM! Helper functions improve code organization.The new
getMilestoneStatusandgetMilestoneIconfunctions cleanly extract milestone rendering logic, making the code more maintainable and testable. The mapping logic is correct and usage is consistent.Also applies to: 241-241, 248-248
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/app/about/page.tsx (2)
53-71: Milestone helpers are clear and align with backend progress semantics
getMilestoneStatusandgetMilestoneIconnicely centralize milestone-display logic and match the 0–100% progress contract from the backend snippet, making the roadmap UI easier to reason about and test. As an optional robustness tweak, you could use>= 100instead of=== 100if you ever change how progress is computed, but it isn’t required given the current backend implementation.
260-262: Index key on staticprojectStorylist is acceptable despite Sonar warningUsing the array index as the key here is reasonable because
projectStoryis static, ordered content that isn’t re-ordered or dynamically mutated, and it avoids potential duplicate-key issues you’d get from using the paragraph text itself. The Sonar warning is generic; in this specific case you can likely treat it as a false positive. If you ever need to make this list dynamic, consider changingprojectStoryto include explicit IDs and keying on those, but that feels out of scope for this PR.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/about/page.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/about/page.tsx (1)
backend/apps/github/api/internal/nodes/milestone.py (1)
progress(40-45)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/app/about/page.tsx
[warning] 261-261: Do not use Array index in keys
🔇 Additional comments (4)
frontend/src/app/about/page.tsx (4)
42-42: AboutSkeleton import cleanly integrates page-specific loading UIImporting
AboutSkeletonhere keeps the About page’s loading state consistent with the new skeleton system and avoids coupling to a generic spinner. No issues from this change.
120-124: Switch to AboutSkeleton for loading state is correct and on-briefUsing
<AboutSkeleton />when any of the queries are loading keeps layout stable and aligns the About page with the platform’s skeleton-based loading pattern, without touching the data-fetching logic.
239-249: Refactoring milestone tooltip to use helpers improves consistencyWiring the tooltip content and icon through
getMilestoneStatus(milestone.progress)andgetMilestoneIcon(milestone.progress)removes duplicated status logic and keeps text and icon state in sync for roadmap milestones. This is a solid cleanup with no apparent edge-case regressions.
275-278: Timeline dot offset change is a harmless Tailwind normalizationChanging from a hard-coded
top-[10px]totop-2.5keeps the visual alignment while relying on Tailwind’s spacing scale, which is more consistent with utility-class conventions. No functional or accessibility concerns from this tweak.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@dhirukumar Thanks for working on this!
The PR description and title says that this should cover About, Organization and Snapshot pages, but I only see skeletons on About and Snapshot pages.
Do you plan to add Organization page too?
Also left a request ⬇️ and there are conflicts that you need to fix ⚙️
There was a problem hiding this comment.
The skeleton state is narrower than the final page layout, which causes a visible jump on reload.
Could you update this to match the Snapshot page layout?
Snapshots-.-OWASP-Nest.mp4
2 participants
Proposed change
Resolves #2673
Summary
This PR implements skeleton loading components for the About, Organization, and Snapshots pages to improve user experience during data fetching. Users now see structured loading states instead of generic spinners, making the application feel more responsive and modern.
Before & After
Before:
After:
Snapshots(Before)
Snapshots(After)
Organizations(Before)
Organizations(After)
About(Before)
About(After)
Changes Made
1. Created AboutSkeleton Component (
src/components/skeletons/AboutSkeleton.tsx)2. Created SnapshotSkeleton Component (
src/components/skeletons/SnapshotSkeleton.tsx)3. Updated About Page (
src/app/about/page.tsx)LoadingSpinnerwithAboutSkeletonfor better loading UXAboutSkeletoncomponent4. Updated Snapshots Page (
src/app/snapshots/page.tsx)SnapshotSkeletoncomponent for loading state5. Updated SkeletonBase Component (
src/components/SkeletonsBase.tsx)'about'case to the switch statement to returnAboutSkeleton'snapshots'case to returnSnapshotSkeleton6. Updated About Page Tests (
__tests__/unit/pages/About.test.tsx)'renders LoadingSpinner when project data is loading'testdocument.querySelectorAllto find skeleton container elements with specific CSS classes7. Updated Snapshots Page Tests (
__tests__/unit/pages/Snapshots.test.tsx)SnapshotSkeletoncomponentTechnical Details
Files Created:
frontend/src/components/skeletons/AboutSkeleton.tsx(new component)frontend/src/components/skeletons/SnapshotSkeleton.tsx(new component)Files Modified:
frontend/src/components/SkeletonsBase.tsx(added new cases, fixed linting issue)frontend/src/app/about/page.tsx(integrated AboutSkeleton)frontend/src/app/snapshots/page.tsx(integrated SnapshotSkeleton)frontend/__tests__/unit/pages/About.test.tsx(updated tests, removed duplicates)frontend/__tests__/unit/pages/Snapshots.test.tsx(updated tests)Benefits
Testing
make check-testruns successfullyChecklist
make check-testlocally; all checks and tests passed.