feat(recruiter): add advance candidate confirmation dialog#1405
Conversation
github-actions
Bot
added
quality:exceptional
github-actions
Bot
added
bug
|
Hi @YAXH64, thanks for contributing to InternHack! 🎉 I have automatically:
Our workflows will now analyze your changes to classify:
Tip Ensure your PR description references the issue it resolves (e.g. Happy coding! 🚀 |
github-actions
Bot
added
type:design
📝 WalkthroughWalkthrough
ChangesApplication Advancement Confirmation
Sequence Diagram(s)sequenceDiagram
participant User
participant AdvanceButton as Row Advance Button
participant ConfirmDialog
participant HandleAdvance as handleAdvance()
participant API as PATCH /advance
User->>AdvanceButton: Click Advance
AdvanceButton->>ConfirmDialog: Set pendingAdvanceApp
ConfirmDialog->>ConfirmDialog: Focus cancel, show dialog
User->>ConfirmDialog: Click Confirm
ConfirmDialog->>HandleAdvance: Trigger for pending app
HandleAdvance->>API: PATCH request
API-->>HandleAdvance: Response
HandleAdvance->>ConfirmDialog: Clear pendingAdvanceApp
ConfirmDialog->>User: Close dialog, show toast
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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. ✨ 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.
🧹 Nitpick comments (3)
client/src/components/ui/ConfirmDialog.tsx (2)
37-46: ⚡ Quick winOptimize effect dependencies to prevent unnecessary listener re-registration.
The Escape key effect depends on
onCancel, which will likely be passed as an inline arrow function by callers (as seen inApplicationsList.tsxline 88). This causes the event listener to be removed and re-added on every parent render while the dialog is open, even whenopenandloadinghaven't changed.♻️ Proposed fix using useCallback pattern
Document that callers should memoize handlers, or wrap the handler internally:
// Keyboard navigation & Escape key dismiss useEffect(() => { if (!open) return; const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === "Escape" && !loading) { - onCancel(); - } + if (e.key === "Escape" && !loading) onCancel(); }; document.addEventListener("keydown", handleKeyDown); return () => document.removeEventListener("keydown", handleKeyDown); - }, [open, loading, onCancel]); + }, [open, loading, onCancel]);Alternatively, add a note in the component documentation that
onCancelandonConfirmshould be memoized withuseCallbackin parent components to avoid performance overhead.🤖 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 `@client/src/components/ui/ConfirmDialog.tsx` around lines 37 - 46, The effect currently includes onCancel in its dependency array causing the keydown listener to be re-registered whenever a parent passes an inline handler; fix this by storing the latest onCancel in a ref (e.g. onCancelRef) and update that ref when onCancel changes, then have the useEffect that registers handleKeyDown depend only on open and loading and call onCancelRef.current() inside handleKeyDown; this keeps the listener stable while still invoking the up-to-date onCancel handler, referencing useEffect, handleKeyDown, onCancel, open, and loading to locate the code to change.
64-70: 💤 Low valueConsider validating that content is provided.
Since both
descriptionandchildrenare now optional, the component could render an empty<p>tag if a caller mistakenly passes neither. While TypeScript doesn't prevent this, a runtime check or dev-time warning could improve developer experience.🛡️ Proposed validation
<div id="confirm-desc" className="mt-2 text-sm text-stone-600 dark:text-stone-400"> {children ? ( children ) : ( - <p className="leading-relaxed"> - {description} - </p> + <p className="leading-relaxed"> + {description || "Are you sure you want to proceed?"} + </p> )} </div>Or add a prop validation/type constraint to require at least one.
🤖 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 `@client/src/components/ui/ConfirmDialog.tsx` around lines 64 - 70, The ConfirmDialog component can render an empty <p> when both children and description are missing; add a validation at the start of the ConfirmDialog function to detect when both props are falsy and either (a) emit a clear dev-time console.warn (e.g., "ConfirmDialog requires children or description") or (b) throw an error in strict mode, and optionally strengthen the prop typing by changing the props type to a discriminated union requiring at least one of children or description; reference the ConfirmDialog component and its children and description props when adding the check.client/src/module/recruiter/applications/ApplicationsList.tsx (1)
222-227: ⚡ Quick winConsider migrating to the shared Button component.
The Advance button uses inline classes instead of the reusable
Buttoncomponent fromclient/src/components/ui/button.tsx. While this button structure pre-dates this PR (only theonClickhandler was updated here), it would improve consistency to refactor it to use the shared component pattern.♻️ Proposed refactor using shared Button
Import the Button component:
+import { Button } from "../../../components/ui/button";Then replace the inline button:
- <button onClick={() => setPendingAdvanceApp(app)} - disabled={isAdvancing} - className={`inline-flex min-w-21.5 items-center justify-center gap-1.5 text-xs px-3 py-1.5 bg-black dark:bg-white text-white dark:text-gray-950 rounded-lg hover:bg-gray-800 dark:hover:bg-gray-200 transition-colors ${isAdvancing ? "cursor-not-allowed opacity-70" : ""}`}> - {isAdvancing && <Loader2 className="h-3 w-3 animate-spin" />} - {isAdvancing ? "Advancing" : "Advance"} - </button> + <Button + onClick={() => setPendingAdvanceApp(app)} + disabled={isAdvancing} + variant="primary" + size="sm" + > + {isAdvancing && <Loader2 className="h-3 w-3 animate-spin" />} + {isAdvancing ? "Advancing" : "Advance"} + </Button>As per coding guidelines: Use the reusable
Buttoncomponent fromclient/src/components/ui/button.tsxfor all buttons with variants (primary, secondary, mono, ghost, danger), modes (button, icon, link), and sizes (sm, md, lg).🤖 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 `@client/src/module/recruiter/applications/ApplicationsList.tsx` around lines 222 - 227, Replace the inline <button> in ApplicationsList with the shared Button component from client/src/components/ui/button.tsx: import Button, then call <Button onClick={() => setPendingAdvanceApp(app)} disabled={isAdvancing} size="sm" variant="primary" ...> and move the existing Loader2 and conditional text inside the Button so the spinner shows when isAdvancing and the label toggles between "Advancing" and "Advance"; preserve the onClick handler (setPendingAdvanceApp), the disabled behavior (isAdvancing) and any extra className needed for spacing, and remove the old inline classes that are now handled by the shared Button.
🤖 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.
Nitpick comments:
In `@client/src/components/ui/ConfirmDialog.tsx`:
- Around line 37-46: The effect currently includes onCancel in its dependency
array causing the keydown listener to be re-registered whenever a parent passes
an inline handler; fix this by storing the latest onCancel in a ref (e.g.
onCancelRef) and update that ref when onCancel changes, then have the useEffect
that registers handleKeyDown depend only on open and loading and call
onCancelRef.current() inside handleKeyDown; this keeps the listener stable while
still invoking the up-to-date onCancel handler, referencing useEffect,
handleKeyDown, onCancel, open, and loading to locate the code to change.
- Around line 64-70: The ConfirmDialog component can render an empty <p> when
both children and description are missing; add a validation at the start of the
ConfirmDialog function to detect when both props are falsy and either (a) emit a
clear dev-time console.warn (e.g., "ConfirmDialog requires children or
description") or (b) throw an error in strict mode, and optionally strengthen
the prop typing by changing the props type to a discriminated union requiring at
least one of children or description; reference the ConfirmDialog component and
its children and description props when adding the check.
In `@client/src/module/recruiter/applications/ApplicationsList.tsx`:
- Around line 222-227: Replace the inline <button> in ApplicationsList with the
shared Button component from client/src/components/ui/button.tsx: import Button,
then call <Button onClick={() => setPendingAdvanceApp(app)}
disabled={isAdvancing} size="sm" variant="primary" ...> and move the existing
Loader2 and conditional text inside the Button so the spinner shows when
isAdvancing and the label toggles between "Advancing" and "Advance"; preserve
the onClick handler (setPendingAdvanceApp), the disabled behavior (isAdvancing)
and any extra className needed for spacing, and remove the old inline classes
that are now handled by the shared Button.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63532fcc-4aee-4be1-b619-443a910da03a
📒 Files selected for processing (2)
client/src/components/ui/ConfirmDialog.tsxclient/src/module/recruiter/applications/ApplicationsList.tsx
Code Review — PR #1405: Add advance candidate confirmation dialogHi @YAXH64, the confirmation dialog implementation is clean and the warning text in the dialog body is helpful. One coordination note. 🟡 Merge conflict:
|
Sachinchaurasiya360
merged commit 9faa5fc
into
Sachinchaurasiya360:main
2 participants
Pull Request
Description
Adds a confirmation step before recruiters can advance candidates to the next hiring stage.
Previously, clicking the Advance button immediately triggered the advancement action, which could lead to accidental candidate progression and incorrect hiring pipeline states. This PR introduces a confirmation dialog that requires explicit user confirmation before the candidate is advanced.
Changes Made
Added a confirmation modal before candidate advancement.
Introduced
pendingAdvanceAppstate to manage the selected candidate awaiting confirmation.Refactored the recruiter applications workflow so advancement only occurs after explicit confirmation.
Enhanced the reusable
ConfirmDialogcomponent:children.Prevented duplicate submissions through UI and handler-level guards.
Improved error handling by keeping the dialog open when advancement fails, allowing recruiters to retry.
User Impact
Recruiters can no longer accidentally advance candidates with a single misclick. Candidate progression now requires an explicit confirmation step, improving data integrity and hiring workflow reliability.
Related Issue
Fixes #1134
Type of Change
Testing
Manual Testing
Open Recruiter Dashboard.
Navigate to a job's Applications page.
Click Advance on any candidate.
Verify a confirmation dialog appears.
Click Cancel and confirm no status change occurs.
Reopen the dialog and click Confirm Advance.
Verify the candidate advances successfully.
Verify success toast appears.
Simulate an API failure and verify:
Rapidly click Confirm and verify only a single request is sent.
Press Escape while the dialog is open and verify it closes correctly (when not loading).
Screenshots / Video
Before
After
Checklist
.env, credentials, ornode_modulescommittedSummary by CodeRabbit