feat: implement loading context and skeleton components for improved …#78
feat: implement loading context and skeleton components for improved …#78anish27863 wants to merge 7 commits into
Conversation
🚀 PR Received SuccessfullyHello @anish27863, Thank you for taking the initiative to contribute to this project. Please ensure that your PR follows all project guidelines properly before requesting review.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR refactors loading state handling across the frontend by introducing page fade-in animations, shared skeleton placeholder components, and per-page loading UI while simplifying route guards. The backend server startup also disables database connection initialization. ChangesPage Fade-in Animations and Loading State Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 4
🧹 Nitpick comments (2)
frontend/src/components/shared/skeletons/PageSkeletons.jsx (1)
33-41: ⚡ Quick winPrefer SkeletonBlock for consistent skeleton styling.
Lines 35, 37-38 manually replicate skeleton styling instead of using the
SkeletonBlockhelper. For maintainability, consider usingSkeletonBlockwith custom classes or extracting these as a variant.♻️ Suggested refactor
<div className="mb-12 p-6 border-[4px] border-black bg-black"> <div className="flex items-center gap-5"> - <div className="w-16 h-16 border-[3px] border-white bg-gray-300 animate-pulse" /> + <SkeletonBlock className="w-16 h-16 border-[3px] border-white bg-gray-300" /> <div className="w-full"> - <div className="h-7 w-60 bg-gray-300 border-[2px] border-white animate-pulse mb-2" /> - <div className="h-4 w-72 bg-gray-300 border-[2px] border-white animate-pulse" /> + <SkeletonBlock className="h-7 w-60 bg-gray-300 border-[2px] border-white mb-2" /> + <SkeletonBlock className="h-4 w-72 bg-gray-300 border-[2px] border-white" /> </div> </div> </div>Note:
SkeletonBlockalready includesanimate-pulse, so it can be removed from the custom classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/shared/skeletons/PageSkeletons.jsx` around lines 33 - 41, Replace the manually styled skeleton divs in the PageSkeletons component with the shared SkeletonBlock helper: swap the avatar div (w-16 h-16 border-[3px] border-white bg-gray-300 animate-pulse) for <SkeletonBlock className="w-16 h-16 border-[3px] border-white bg-gray-300" /> and replace the two text bars (h-7 w-60 ... and h-4 w-72 ...) with <SkeletonBlock className="h-7 w-60 border-[2px] border-white mb-2 bg-gray-300" /> and <SkeletonBlock className="h-4 w-72 border-[2px] border-white bg-gray-300" /> respectively; remove the explicit animate-pulse classes because SkeletonBlock already provides the animation and adjust import/usage of SkeletonBlock in PageSkeletons.jsx accordingly.frontend/src/components/shared/GlobalLoadingOverlay.jsx (1)
7-10: ⚡ Quick winAdd live loading semantics for assistive technologies.
The overlay is visually clear, but it doesn’t announce loading state to screen readers. Add status semantics and hide the decorative spinner.
Proposed patch
- <div className="fixed inset-0 z-[80] bg-white/85 flex items-center justify-center"> + <div + role="status" + aria-live="polite" + aria-busy="true" + className="fixed inset-0 z-[80] bg-white/85 flex items-center justify-center" + > <div className="flex flex-col items-center gap-4"> - <div className="w-12 h-12 border-[4px] border-black border-t-transparent animate-spin" /> + <div aria-hidden="true" className="w-12 h-12 border-[4px] border-black border-t-transparent animate-spin" /> <p className="text-xs font-black uppercase tracking-[0.22em] text-black">Loading</p> </div> </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/shared/GlobalLoadingOverlay.jsx` around lines 7 - 10, The overlay lacks screen-reader semantics — add a live status and hide the decorative spinner: give the loading text element (the <p>Loading</p>) role="status" and aria-live="polite" (or aria-live="assertive" if needed for urgency) so assistive tech announces the state, and mark the decorative spinner div with aria-hidden="true" to prevent it from being read; keep the visible text for sighted users but ensure the status attributes are applied on the element rendering "Loading" in GlobalLoadingOverlay.jsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/shared/RouteFrame.jsx`:
- Around line 10-19: The loading effect in RouteFrame.jsx only depends on
location.pathname so same-path navigations that change query/hash don't trigger
the loading overlay; update the useEffect dependency to include the router
location key (or include location.search and location.hash) so the effect runs
for those navigations, i.e. change the dependency array referenced in the
useEffect that uses firstRender.current and setLoading to include location.key
(or location.search and location.hash) so the timer logic and clearing (timer,
setLoading, firstRender) run on query/hash changes as well.
In `@frontend/src/pages/AccountCenterPage.jsx`:
- Line 334: The component AccountCenterPage currently uses an artificial 320ms
delay via setTimeout to set pageReady (state variable) which unnecessarily slows
rendering; remove the timeout and instead setPageReady(true) immediately when
authLoading is false and user is present (or implement a true minimum-time
pattern only if there was an actual prior loading phase). Locate the
useEffect(s) referencing pageReady/setPageReady and authLoading/user and replace
the setTimeout-based logic with an immediate state update (or conditional
minimum-time logic) so the UI renders as soon as authentication data is ready.
In `@frontend/src/pages/DashboardPage.jsx`:
- Line 14: Remove the artificial 320ms delay by deleting the setTimeout that
sets pageReady and instead set pageReady directly when loading becomes false;
update the DashboardPage component (references: pageReady, setPageReady,
loading) so that once loading is false you call setPageReady(true) immediately
(or derive readiness directly from loading) and remove the timeout logic; if
skeleton flashing must be addressed, implement it in the data-fetching layer or
use a minimal debounce/conditional render there rather than a hardcoded timeout
in the component.
In `@frontend/src/pages/ExplorePage.jsx`:
- Line 18: The artificial 360ms boot delay should be removed: change the initial
state for isBootLoading to false (useState(false)) and delete the
useEffect/setTimeout block that sets isBootLoading after 360ms (remove any
references to the 360ms timer), or if you must keep parity with other pages
replace the hardcoded timeout with the same behavior used in
Dashboard/AccountCenter; update usages of isBootLoading/setIsBootLoading
accordingly so ExplorePage renders immediately with no artificial delay.
---
Nitpick comments:
In `@frontend/src/components/shared/GlobalLoadingOverlay.jsx`:
- Around line 7-10: The overlay lacks screen-reader semantics — add a live
status and hide the decorative spinner: give the loading text element (the
<p>Loading</p>) role="status" and aria-live="polite" (or aria-live="assertive"
if needed for urgency) so assistive tech announces the state, and mark the
decorative spinner div with aria-hidden="true" to prevent it from being read;
keep the visible text for sighted users but ensure the status attributes are
applied on the element rendering "Loading" in GlobalLoadingOverlay.jsx.
In `@frontend/src/components/shared/skeletons/PageSkeletons.jsx`:
- Around line 33-41: Replace the manually styled skeleton divs in the
PageSkeletons component with the shared SkeletonBlock helper: swap the avatar
div (w-16 h-16 border-[3px] border-white bg-gray-300 animate-pulse) for
<SkeletonBlock className="w-16 h-16 border-[3px] border-white bg-gray-300" />
and replace the two text bars (h-7 w-60 ... and h-4 w-72 ...) with
<SkeletonBlock className="h-7 w-60 border-[2px] border-white mb-2 bg-gray-300"
/> and <SkeletonBlock className="h-4 w-72 border-[2px] border-white bg-gray-300"
/> respectively; remove the explicit animate-pulse classes because SkeletonBlock
already provides the animation and adjust import/usage of SkeletonBlock in
PageSkeletons.jsx accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca7c39a3-b007-4137-ad08-879e3e52f49f
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
frontend/src/App.jsxfrontend/src/components/shared/GlobalLoadingOverlay.jsxfrontend/src/components/shared/RouteFrame.jsxfrontend/src/components/shared/skeletons/PageSkeletons.jsxfrontend/src/context/LoadingContext.jsxfrontend/src/index.cssfrontend/src/pages/AccountCenterPage.jsxfrontend/src/pages/DashboardPage.jsxfrontend/src/pages/ExplorePage.jsx
|
please consider this for my gssoc26 contribution |
|
@anish27863 provide a video for your work and showing those changes in interface |
Screen.Recording.2026-05-20.153405.mp4@kunalverma2512 please checkout this Ui demo |
|
@coderabbitai Please provide a complete and detailed review of this PR. Specifically review:
Please do a strict full PR review instead of only nitpick comments. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 40279
Full PR Review — feat: Loading Context, Skeletons & Route Transitions1. What This PR Actually ChangesThe PR introduces:
Architectural impact: 2. Critical Issues🔴
|
| Component | Quality | Issue |
|---|---|---|
DashboardPageSkeleton |
✅ Good | Layout roughly matches real page |
AccountCenterSkeleton |
✅ Good | Adheres to brutalist style |
ExplorePageSkeleton |
❌ Dead code | Never used in ExplorePage |
Style concern: SkeletonBlock applies border-[2px] border-black animate-pulse — the black border pulses with the background, which looks unusual. Standard skeletons use borderless blocks. This may be intentional for the brutalist style but reads as visual noise.
Inconsistent coverage: Many pages have no skeletons at all — CodeforcesPage, GitHubIntelligencePage, PracticePage, ApexAIPage, AlgoVersePage, etc. The improvement is partial and inconsistent.
5. AccountCenterPage — Simplified, But Correct
The final implementation is clean:
if (authLoading || !user) {
return <AccountCenterSkeleton />;
}No artificial pageReady timer — this correctly gates on real auth state. ✅
6. DashboardPage — Also Clean
if (loading) {
return <DashboardPageSkeleton />;
}Replaces the old LoaderSwitcher with a skeleton. This is the best use of skeletons in the PR. ✅
7. Existing LoaderSwitcher Inconsistency
LoaderSwitcher still exists and is still used in ProtectedRoute.jsx and PublicRoute.jsx:
frontend/src/components/shared/ProtectedRoute.jsx:3: import Loader from "./loaders/LoaderSwitcher";
frontend/src/components/shared/PublicRoute.jsx:3: import Loader from "./loaders/LoaderSwitcher";
The PR introduces a new loading system but doesn't clean up the old one. This leaves two parallel loading UIs in the codebase.
8. Code Quality & Structure
| Item | Status |
|---|---|
Folder structure (components/shared/skeletons/) |
✅ Good |
Naming (GlobalLoadingOverlay, RouteFrame) |
✅ Clear |
Accessibility (aria-hidden, role="status", aria-live) on overlay |
✅ Good |
useLoading error boundary (throw new Error) |
✅ Good pattern |
CSS keyframe in index.css |
✅ Appropriate placement |
| Brutalist style adherence | ✅ Consistent |
| No TypeScript — consistent with existing project | ✅ |
9. Summary Verdict
| Aspect | Verdict |
|---|---|
DashboardPage skeleton (auth-gated) |
✅ Merge-ready |
AccountCenterPage skeleton (auth-gated) |
✅ Merge-ready |
| CSS fade animation | ✅ Merge-ready |
ExplorePageSkeleton |
❌ Dead code, wire it up or remove |
GlobalLoadingOverlay + RouteFrame artificial timers |
❌ Not recommended — misleading UX |
LoadingContext infrastructure |
|
Full subtree remount via key |
❌ Performance concern |
showLoading/hideLoading unused helpers |
❌ Remove or use |
Recommended Actions Before Merge
- Wire up or remove
ExplorePageSkeleton— it's currently dead code. - Remove or rethink
RouteFrame— artificial delays are not real loading signals. If a fade-in animation is desired, use a CSS-only approach (e.g.,<Outlet>withanimateclass) without the global state machinery. - Remove
showLoading/hideLoadingfrom context if unused, or document/use them. - Consolidate with
LoaderSwitcher— either replace it with the skeleton system or keep only one loading primitive. - Don't use
keyon the inner div ofRouteFrame— it causes unnecessary remounts. Apply the CSS class without a key-based remount strategy.
The skeleton components themselves are well-crafted and the auth-gated usage in DashboardPage and AccountCenterPage is a genuine improvement. The main concerns are the over-engineered context, artificial delays, and dead code.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/App.jsx`:
- Line 38: The wrapper div in App.jsx uses key={location.key}, which forces a
full remount of the entire routed subtree (including <Routes>) on every
navigation; change this to avoid remounting page components by replacing
key={location.key} with a key that only changes on actual path changes (e.g.,
key={location.pathname}) or remove the key entirely and apply the CSS keyframe
to an inner element that you intentionally want to remount/animate (for example,
move the key to the element that wraps the page content or to the Routes child),
ensuring you update the JSX where the div with className "page-fade-in w-full
flex-1 flex flex-col" is declared.
In `@frontend/src/components/shared/ProtectedRoute.jsx`:
- Around line 7-9: The guard currently returns null while auth resolves in
ProtectedRoute.jsx (if (loading) return null;), producing a blank frame; replace
that null with a shared guard fallback component (e.g., render an
<AuthGuardFallback /> or <GuardSkeleton />) so the UI shows a consistent loading
state during authentication resolution, keeping the existing redirect behavior
(if (!isAuthenticated) return <Navigate ... />) and wiring the fallback into the
same component where loading and isAuthenticated are checked.
In `@server/server.js`:
- Line 8: Re-enable the fail-fast DB initialization by calling connectDB()
before starting the HTTP listener and only call server/app listen after
connectDB resolves; if connectDB throws or rejects, log the error and exit
process with non-zero status. Locate the connectDB call and the server start
(e.g., connectDB and app.listen or server.listen in server/server.js) and
move/restore the call so the listener is invoked inside the successful-connect
branch or after await connectDB(), with proper try/catch that logs the error and
calls process.exit(1) on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 570c897f-39e8-4d50-9fac-7f7b93813a48
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
frontend/src/App.jsxfrontend/src/components/shared/ProtectedRoute.jsxfrontend/src/components/shared/PublicRoute.jsxfrontend/src/components/shared/skeletons/PageSkeletons.jsxfrontend/src/pages/AccountCenterPage.jsxfrontend/src/pages/DashboardPage.jsxfrontend/src/pages/ExplorePage.jsxserver/server.js
💤 Files with no reviewable changes (1)
- frontend/src/pages/ExplorePage.jsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai Please provide a complete and detailed review of this PR. Specifically review: What this PR actually changes in the project architecture and user experience |
|
Tip For best results, initiate chat on the files or code changes.
✅ Parts That Are Merge-Ready
❌ Issues You Must Fix
Recommended Minimal Fix PathThe simplest path to a clean, mergeable PR:
This keeps the genuine UX improvements (skeletons) while removing the over-engineered and problematic parts. |
|
@kunalverma2512 please check it out |
2 participants
Navigation UX Fix Summary
I improved route navigation feedback across the frontend by adding:
animate-pulseto reduce visual jumps while loading.All new UI follows the existing responsive brutalist black-and-white style (no rounded or colorful elements).
Summary by CodeRabbit
New Features
Visual Improvements