Improve loading and error states with skeletons and retry actions#4351
Open
satyammishra8-1 wants to merge 1 commit into
Open
Improve loading and error states with skeletons and retry actions#4351satyammishra8-1 wants to merge 1 commit into
satyammishra8-1 wants to merge 1 commit into
Conversation
|
CodeAnt AI is reviewing your PR. |
|
@satyammishra8-1 is attempting to deploy a commit to the Anurag Mishra's projects Team on Vercel. A member of the Team first needs to authorize it. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughAdds skeleton loading placeholders to ChangesUX Improvements: Skeleton Loading, Bug Reporting, Sidebar Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
codeant-ai
Bot
added
the
size:M
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+2263
to
+2269
| {error && ( | ||
| <div className="p-4 rounded-xl bg-red-500/10 border border-red-500/20"> | ||
| <p className="text-3xl font-bold text-red-400"> | ||
| {totalFillerWords} | ||
| </p> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Suggestion: The Filler Words metric is now gated behind error, so in successful feedback flows (where error is empty) users never see their filler-word count. This ties a core result card to unrelated error state and hides expected feedback data; render this metric unconditionally in the feedback summary. [logic error]
Severity Level: Major ⚠️
- ⚠️ Communication analysis hides filler-words metric on success.
- ⚠️ Detailed feedback inconsistent with summary filler-words card.
- ⚠️ InterviewPrep feedback less informative for user communication practice.Steps of Reproduction ✅
1. Open the Interview Prep flow implemented in `frontend/src/pages/InterviewPrep.jsx` and
complete a full interview end-to-end so that `completeInterview()` (around lines
1025–1061) succeeds and sets `overallResults` then `step` to `'feedback'`.
2. On the feedback page (rendered when `step === 'feedback' && overallResults` at lines
1941+ in `InterviewPrep.jsx`), observe the top-level performance summary cards around
lines 2211–2217 where `{totalFillerWords}` is displayed unconditionally in the “Filler
Words Used” card.
3. Scroll down to the “Communication & Confidence Analysis” section (grid at lines
2234–2281) and note the JSX fragment at diff lines 2260–2269: the filler-words metric is
wrapped in `{error && ( ... {totalFillerWords} ... )}`, so only the label “Filler Words”
is rendered when `error` is falsy.
4. Trace `error` usage in `InterviewPrep.jsx`: before reaching feedback, flows such as
`stopRecording` (lines 884–985) and `submitTextAnswer` (lines 987–1021) explicitly call
`setError('')` on success, and `completeInterview` only sets `error` in its catch block;
in successful interviews `error` is an empty string, so the `{error && (...)}` condition
at line 2263 is false and the detailed filler-words metric card never appears in normal
feedback runs.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/pages/InterviewPrep.jsx
**Line:** 2263:2269
**Comment:**
*Logic Error: The Filler Words metric is now gated behind `error`, so in successful feedback flows (where `error` is empty) users never see their filler-word count. This ties a core result card to unrelated error state and hides expected feedback data; render this metric unconditionally in the feedback summary.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 131: The /shared/:shareToken route renders the lazy SharedResumeView
without a Suspense boundary, which will break navigation. Update the route setup
in App so SharedResumeView is wrapped in the same Suspense loading fallback used
by the other lazy routes, keeping the lazy import and the route element under a
Suspense ancestor.
In `@frontend/src/components/AppSidebar.jsx`:
- Around line 110-113: The `Logo` and `UserSection` blocks in `AppSidebar` still
gate `display` on `animate`, so the non-animated closed state leaves invisible
content in layout instead of collapsing. Update the `motion.div` logic for those
sections to follow the same open-state-only behavior used by the button rows,
making `display` depend on `open` rather than `animate` while preserving opacity
transitions where needed.
In `@frontend/src/pages/InterviewPrep.jsx`:
- Around line 2259-2269: The filler-words metric card is incorrectly wrapped in
the page-level error check, so successful runs hide the metric and leave the
label disconnected from its value. Update the InterviewPrep JSX block around the
filler-words display to render the card unconditionally, or condition it on the
actual filler-word data state instead of the generic error state, keeping the
label and value together in the same card.
🪄 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: 879c3965-6244-4f1d-b00e-b86ebc25d5c4
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
frontend/src/App.jsxfrontend/src/components/AppSidebar.jsxfrontend/src/pages/InterviewPrep.jsxfrontend/src/pages/Outreach.jsxfrontend/src/pages/hubs/ResumeHub.jsx
| import MichelinStarChefPlating from './components/portfolio/templates/Michelin_Star_Chef_Plating/index.jsx'; | ||
| import SommelierWineCellarRacks from './components/portfolio/templates/Sommelier_Wine_Cellar_Racks/index.jsx'; | ||
| import SharedResumeView from './pages/SharedResumeView.jsx' | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm SharedResumeView is lazy-only and the /shared route lacks a Suspense wrapper.
rg -nP 'SharedResumeView' frontend/src/App.jsx
echo "---- route + surrounding context ----"
rg -nP -C3 '/shared/:shareToken' frontend/src/App.jsx
echo "---- any outer Suspense around <Routes>? ----"
rg -nP -B2 -A1 '<Routes>' frontend/src/App.jsxRepository: anurag3407/career-pilot
Length of output: 852
Wrap /shared/:shareToken in Suspense.
<SharedResumeView /> is still lazy, but this route has no <Suspense> ancestor, so navigating here will throw. Use the same loading boundary as the other lazy routes.
🤖 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/App.jsx` at line 131, The /shared/:shareToken route renders the
lazy SharedResumeView without a Suspense boundary, which will break navigation.
Update the route setup in App so SharedResumeView is wrapped in the same
Suspense loading fallback used by the other lazy routes, keeping the lazy import
and the route element under a Suspense ancestor.
Comment on lines
110
to
+113
| <motion.div | ||
| animate={{ | ||
| display: animate ? (open ? "flex" : "none") : "flex", | ||
| opacity: animate ? (open ? 1 : 0) : 1, | ||
| }} | ||
| opacity: open ? 1 : 0, }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Finish the open-only migration for Logo and UserSection.
These two blocks still keep display tied to animate, so open={false}, animate={false} leaves the text in layout with opacity: 0 instead of fully collapsing. That makes the non-animated path inconsistent with the button rows below and with the stated open-state-only behavior.
Suggested fix
<motion.div
animate={{
- display: animate ? (open ? "flex" : "none") : "flex",
- opacity: open ? 1 : 0, }}
+ display: open ? "flex" : "none",
+ opacity: open ? 1 : 0,
+ }}
transition={{ duration: 0.2 }}
className="flex items-center gap-2"
> <motion.div
animate={{
- display: animate ? (open ? "block" : "none") : "block",
- opacity: open ? 1 : 0, }}
+ display: open ? "block" : "none",
+ opacity: open ? 1 : 0,
+ }}
transition={{ duration: 0.2 }}
className="flex-1 min-w-0"
>Also applies to: 157-160
🤖 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/AppSidebar.jsx` around lines 110 - 113, The `Logo`
and `UserSection` blocks in `AppSidebar` still gate `display` on `animate`, so
the non-animated closed state leaves invisible content in layout instead of
collapsing. Update the `motion.div` logic for those sections to follow the same
open-state-only behavior used by the button rows, making `display` depend on
`open` rather than `animate` while preserving opacity transitions where needed.
Comment on lines
+2259
to
+2269
|
|
||
| <p className="text-sm text-muted-foreground"> | ||
| Filler Words | ||
| </p> | ||
| {error && ( | ||
| <div className="p-4 rounded-xl bg-red-500/10 border border-red-500/20"> | ||
| <p className="text-3xl font-bold text-red-400"> | ||
| {totalFillerWords} | ||
| </p> | ||
| </div> | ||
| )} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the filler-words metric card.
Line 2263 gates this block on the page-level error state, so successful feedback runs hide the filler-words metric entirely and leave the label orphaned in the grid. Render this card unconditionally, or key it off actual filler-word data instead of error.
Proposed fix
-
- <p className="text-sm text-muted-foreground">
- Filler Words
- </p>
- {error && (
- <div className="p-4 rounded-xl bg-red-500/10 border border-red-500/20">
- <p className="text-3xl font-bold text-red-400">
- {totalFillerWords}
- </p>
- </div>
- )}
+ <div className="p-4 rounded-xl bg-amber-500/10 border border-amber-500/20">
+ <p className="text-sm text-muted-foreground">
+ Filler Words
+ </p>
+ <p className="text-3xl font-bold text-amber-400">
+ {totalFillerWords}
+ </p>
+ </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <p className="text-sm text-muted-foreground"> | |
| Filler Words | |
| </p> | |
| {error && ( | |
| <div className="p-4 rounded-xl bg-red-500/10 border border-red-500/20"> | |
| <p className="text-3xl font-bold text-red-400"> | |
| {totalFillerWords} | |
| </p> | |
| </div> | |
| )} | |
| <div className="p-4 rounded-xl bg-amber-500/10 border border-amber-500/20"> | |
| <p className="text-sm text-muted-foreground"> | |
| Filler Words | |
| </p> | |
| <p className="text-3xl font-bold text-amber-400"> | |
| {totalFillerWords} | |
| </p> | |
| </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/pages/InterviewPrep.jsx` around lines 2259 - 2269, The
filler-words metric card is incorrectly wrapped in the page-level error check,
so successful runs hide the metric and leave the label disconnected from its
value. Update the InterviewPrep JSX block around the filler-words display to
render the card unconditionally, or condition it on the actual filler-word data
state instead of the generic error state, keeping the label and value together
in the same card.
Author
|
[UX] No empty states / skeleton screens for ResumeHub, InterviewPrep, Outreach## Description This PR addresses issue #4251 by improving loading and error states. Changes
Testing
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses issue #4251 by improving loading and error states.
Changes
Testing
npm run lintpasses