Add confirm review button with auto-advance to rubric sidebar#695
Conversation
|
@LavishSphere is attempting to deploy a commit to the Pawtograder Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 26 minutes and 2 seconds. ⌛ 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: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughIntroduces an auto-advance workflow for completing reviews: adds Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Browser/Client
participant Hook as useNextIncompleteReviewUrl
participant Router as Next.js Router
participant Page as Submission Files Page
User->>Browser: Complete review checks / clicks "Mark as Complete"
Browser->>Hook: Request next incomplete review URL (reads params, assignments)
Hook-->>Browser: Return nextIncompleteUrl or null
alt Auto-Advance Enabled & URL Available
Browser->>Browser: Persist completion state
Browser->>Router: router.push(nextIncompleteUrl)
Router->>Page: Load target submission files with review_assignment_id
Page-->>User: Display next review
else Auto-Advance Disabled or No URL
Browser->>Browser: Persist completion state
Browser-->>User: Remain on current page
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (2)
components/ui/submission-review-toolbar.tsx (2)
350-360: Solid localStorage-backed toggle hook.The SSR safety check via
typeof localStorage === "undefined"is appropriate. One minor consideration: in some privacy-focused browsers or incognito modes,localStorage.getItemorsetItemcan throw exceptions. This is rare but could be wrapped in a try-catch for extra resilience.🛡️ Optional: Defensive localStorage access
function useAutoAdvance(): [boolean, (v: boolean) => void] { const [on, setOn] = useState(() => { - if (typeof localStorage === "undefined") return false; - return localStorage.getItem("pawtograder-auto-advance-review") === "true"; + try { + if (typeof localStorage === "undefined") return false; + return localStorage.getItem("pawtograder-auto-advance-review") === "true"; + } catch { + return false; + } }); const toggle = (v: boolean) => { - localStorage.setItem("pawtograder-auto-advance-review", String(v)); + try { + localStorage.setItem("pawtograder-auto-advance-review", String(v)); + } catch { + // Ignore storage errors in restricted environments + } setOn(v); }; return [on, toggle]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/submission-review-toolbar.tsx` around lines 350 - 360, The hook useAutoAdvance currently accesses localStorage directly which can throw in some environments; wrap the read in the initializer in a try-catch and return false on error, and wrap the localStorage.setItem call inside toggle in a try-catch to silently fail (or optionally console.warn) so the state still updates via setOn(v); this keeps the existing API ([on, toggle]) and uses the same symbols (useAutoAdvance, on, toggle, setOn) but makes both getItem and setItem defensively safe.
490-492: Consider whether navigation should close the popover first.When auto-advance triggers
router.push(nextIncompleteUrl), the popover may briefly remain visible during the navigation transition. This is likely fine given Next.js client-side routing speed, but if you notice a visual flicker, you could close the popover before navigating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/submission-review-toolbar.tsx` around lines 490 - 492, When autoAdvance triggers navigation, close the popover first to avoid a visual flicker: in the block where you check autoAdvance && nextIncompleteUrl (around the router.push call), invoke the popover closing handler (e.g., call the component's onClose, closePopover, or setOpen(false) that controls the Popover state) before calling router.push(nextIncompleteUrl); if the popover close is asynchronous, ensure you await or sequence it (close then push) so the popover is hidden prior to navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/ui/submission-review-toolbar.tsx`:
- Around line 350-360: The hook useAutoAdvance currently accesses localStorage
directly which can throw in some environments; wrap the read in the initializer
in a try-catch and return false on error, and wrap the localStorage.setItem call
inside toggle in a try-catch to silently fail (or optionally console.warn) so
the state still updates via setOn(v); this keeps the existing API ([on, toggle])
and uses the same symbols (useAutoAdvance, on, toggle, setOn) but makes both
getItem and setItem defensively safe.
- Around line 490-492: When autoAdvance triggers navigation, close the popover
first to avoid a visual flicker: in the block where you check autoAdvance &&
nextIncompleteUrl (around the router.push call), invoke the popover closing
handler (e.g., call the component's onClose, closePopover, or setOpen(false)
that controls the Popover state) before calling router.push(nextIncompleteUrl);
if the popover close is asynchronous, ensure you await or sequence it (close
then push) so the popover is hidden prior to navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d845c700-91a9-4e2e-af40-539a3a3d8991
📒 Files selected for processing (4)
components/ui/assignment-grading-toolbar.tsxcomponents/ui/rubric-sidebar.tsxcomponents/ui/submission-review-toolbar.tsxhooks/useNextIncompleteReview.ts
There was a problem hiding this comment.
Pull request overview
Adds a shared “next incomplete review” URL helper and extends the review completion UX so reviewers can confirm from the rubric sidebar and optionally auto-advance to the next pending review.
Changes:
- Introduced
useNextIncompleteReviewUrlhook to centralize next-incomplete navigation logic. - Added an auto-advance toggle to the “Complete Review” popover and navigates on completion when enabled.
- Rendered the “Complete Review” button at the bottom of the rubric sidebar when a review assignment is active.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hooks/useNextIncompleteReview.ts | New hook computing the next incomplete review URL with wrap-around behavior. |
| components/ui/submission-review-toolbar.tsx | Adds auto-advance preference storage and navigation on completing a review. |
| components/ui/rubric-sidebar.tsx | Adds a bottom-of-sidebar “Complete Review” entry point for active review assignments. |
| components/ui/assignment-grading-toolbar.tsx | Refactors Next Incomplete link to use the new shared hook. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
components/ui/assignment-grading-toolbar.tsx:396
- With the current
useNextIncompleteReviewUrl()behavior,nextIncompleteUrlcan benullwhilestats.allCompleteis stillfalse(e.g., when the only incomplete review is on the current submission). In that case this block renders nothing (neither “Next Incomplete” nor “All reviews completed”), which is confusing. Consider rendering a fallback message/state, or adjusting the hook sonextIncompleteUrlis non-null whenever!stats.allComplete.
{stats && !stats.allComplete && nextIncompleteUrl ? (
<Link href={nextIncompleteUrl}>
<FaArrowRight style={{ marginRight: "4px" }} />
Next Incomplete
</Link>
) : stats?.allComplete ? (
<Text color="fg.muted" fontSize="sm">
All reviews completed!
</Text>
) : null}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
Thanks for the contribution and sorry for the delay in review + merge! |
Resolves #308
Three major changes:
useNextIncompleteReviewUrlhook to share the "next incomplete review" URL logic that was previously only in the grading toolbarSummary by CodeRabbit
New Features
Improvements