[UX] Confirmation Dialog System #256
Conversation
- Replaces `window.confirm` with a custom, accessible `ConfirmDialog` component. - Implements `ConfirmContext` for asynchronous confirmation handling using Promises. - Adds `ConfirmProvider` to `App.tsx` to enable global usage. - Updates `GroupDetails.tsx` to use `useConfirm` for destructive actions (Delete Group, Delete Expense, Leave Group, Remove Member). - Enhances `Modal.tsx` accessibility by adding `role="dialog"`, `aria-modal="true"`, and `aria-labelledby`. - Supports both Neobrutalism and Glassmorphism themes.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for split-but-wiser ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughIntroduces a promise-based Confirmation Dialog System replacing window.confirm with an accessible modal. Adds ConfirmDialog component and ConfirmContext provider with useConfirm hook. Enhances Modal component with ARIA attributes. Integrates confirmations into GroupDetails for destructive actions and updates documentation accordingly. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pages/GroupDetails.tsx (1)
366-393: Unsettled balance check happens after confirmation.The check for unsettled balances (lines 379-385) occurs after the user confirms removal. Consider validating this before showing the confirmation dialog to avoid a confusing experience where the user confirms but the action still fails.
Move validation before confirmation
const handleKickMember = async (memberId: string, memberName: string) => { if (!id || !isAdmin) return; if (memberId === user?._id) return; + const hasUnsettled = settlements.some( + s => (s.fromUserId === memberId || s.toUserId === memberId) && (s.amount || 0) > 0 + ); + if (hasUnsettled) { + addToast('Cannot remove: This member has unsettled balances in the group.', 'error'); + return; + } + const confirmed = await confirm({ title: 'Remove Member', description: `Are you sure you want to remove ${memberName} from the group?`, confirmText: 'Remove', variant: 'danger' }); if (confirmed) { try { - const hasUnsettled = settlements.some( - s => (s.fromUserId === memberId || s.toUserId === memberId) && (s.amount || 0) > 0 - ); - if (hasUnsettled) { - alert('Cannot remove: This member has unsettled balances in the group.'); - return; - } await removeMember(id, memberId); fetchData(); addToast(`Removed ${memberName} from group`, 'success'); } catch (err: any) { addToast(err.response?.data?.detail || "Failed to remove member", 'error'); } } };
🤖 Fix all issues with AI agents
In `@web/components/ui/ConfirmDialog.tsx`:
- Around line 31-32: The destructured but unused variable `mode` from useTheme()
should be removed to eliminate the unused variable warning: update the
destructuring in the ConfirmDialog component (where useTheme() is called) to
only pull `style` (e.g., change `const { style, mode } = useTheme()` to `const {
style } = useTheme()`), leaving the rest of the logic that uses `style`/`isNeo`
(`THEMES.NEOBRUTALISM`, `isNeo`) untouched.
- Around line 71-79: The ConfirmDialog currently sets autoFocus on the Confirm
button which can cause accidental destructive confirmations; update the footer
so that autoFocus is applied conditionally: if getButtonVariant() returns a
destructive variant (e.g., "danger" or "warning") move the autoFocus prop from
the confirm Button (onConfirm) to the cancel Button (onCancel), otherwise keep
autoFocus on the confirm Button; reference ConfirmDialog, getButtonVariant,
onConfirm, onCancel, cancelText and confirmText when making this change.
In `@web/components/ui/Modal.tsx`:
- Around line 68-70: The close button in Modal.tsx currently can act as a submit
button when placed inside forms; update the <button> element that calls onClose
(the button rendering the X icon) to include an explicit type="button" attribute
so it never triggers form submission, keeping the onClose handler and existing
classes/aria-label unchanged.
- Line 46: The Modal component currently hardcodes id="modal-title", which can
collide when multiple modals render; update the Modal (in
web/components/ui/Modal.tsx) to generate a unique id (e.g., call React's useId()
at top of the Modal component), assign that generated id to the dialog's
aria-labelledby attribute and to the modal title element (the element that
currently has id="modal-title"); add the necessary import for useId and replace
the hardcoded string with the generated id so each Modal instance gets a unique,
matching aria-labelledby/title id pair.
In `@web/contexts/ConfirmContext.tsx`:
- Around line 26-32: The current confirm function overwrites the previous
pending promise's resolver, leaving it unresolved; update confirm to first check
for an existing resolver (e.g., resolveRef.current or the existing value held by
setResolveRef) and settle it (call the previous resolve with false or call a
stored reject) before storing the new resolve and returning the new Promise;
specifically, in confirm, before setResolveRef(() => resolve) call the existing
resolver if present to resolve(false) (or reject) so the previous promise is not
left hanging, then proceed to setOptions, setIsOpen, and set the new resolver.
| const { style, mode } = useTheme(); | ||
| const isNeo = style === THEMES.NEOBRUTALISM; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused mode variable.
The mode variable is destructured from useTheme() but is never used in this component.
♻️ Suggested fix
- const { style, mode } = useTheme();
+ const { style } = useTheme();📝 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.
| const { style, mode } = useTheme(); | |
| const isNeo = style === THEMES.NEOBRUTALISM; | |
| const { style } = useTheme(); | |
| const isNeo = style === THEMES.NEOBRUTALISM; |
🤖 Prompt for AI Agents
In `@web/components/ui/ConfirmDialog.tsx` around lines 31 - 32, The destructured
but unused variable `mode` from useTheme() should be removed to eliminate the
unused variable warning: update the destructuring in the ConfirmDialog component
(where useTheme() is called) to only pull `style` (e.g., change `const { style,
mode } = useTheme()` to `const { style } = useTheme()`), leaving the rest of the
logic that uses `style`/`isNeo` (`THEMES.NEOBRUTALISM`, `isNeo`) untouched.
| footer={ | ||
| <> | ||
| <Button variant="ghost" onClick={onCancel}> | ||
| {cancelText} | ||
| </Button> | ||
| <Button variant={getButtonVariant()} onClick={onConfirm} autoFocus> | ||
| {confirmText} | ||
| </Button> | ||
| </> |
There was a problem hiding this comment.
Consider focusing the Cancel button for destructive actions.
The autoFocus on the Confirm button (line 76) may lead to accidental confirmations for danger and warning variants when users press Enter. Accessibility best practices recommend focusing the safer action (Cancel) for destructive dialogs.
🛡️ Suggested fix
footer={
<>
- <Button variant="ghost" onClick={onCancel}>
+ <Button variant="ghost" onClick={onCancel} autoFocus={variant === 'danger' || variant === 'warning'}>
{cancelText}
</Button>
- <Button variant={getButtonVariant()} onClick={onConfirm} autoFocus>
+ <Button variant={getButtonVariant()} onClick={onConfirm} autoFocus={variant === 'info'}>
{confirmText}
</Button>
</>
}📝 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.
| footer={ | |
| <> | |
| <Button variant="ghost" onClick={onCancel}> | |
| {cancelText} | |
| </Button> | |
| <Button variant={getButtonVariant()} onClick={onConfirm} autoFocus> | |
| {confirmText} | |
| </Button> | |
| </> | |
| footer={ | |
| <> | |
| <Button variant="ghost" onClick={onCancel} autoFocus={variant === 'danger' || variant === 'warning'}> | |
| {cancelText} | |
| </Button> | |
| <Button variant={getButtonVariant()} onClick={onConfirm} autoFocus={variant === 'info'}> | |
| {confirmText} | |
| </Button> | |
| </> | |
| } |
🤖 Prompt for AI Agents
In `@web/components/ui/ConfirmDialog.tsx` around lines 71 - 79, The ConfirmDialog
currently sets autoFocus on the Confirm button which can cause accidental
destructive confirmations; update the footer so that autoFocus is applied
conditionally: if getButtonVariant() returns a destructive variant (e.g.,
"danger" or "warning") move the autoFocus prop from the confirm Button
(onConfirm) to the cancel Button (onCancel), otherwise keep autoFocus on the
confirm Button; reference ConfirmDialog, getButtonVariant, onConfirm, onCancel,
cancelText and confirmText when making this change.
| <AnimatePresence> | ||
| {isOpen && ( | ||
| <div className="fixed inset-0 z-50 flex items-center justify-center p-4"> | ||
| <div className="fixed inset-0 z-50 flex items-center justify-center p-4" role="dialog" aria-modal="true" aria-labelledby="modal-title"> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded id="modal-title" may conflict if multiple modals render simultaneously.
If two Modal instances are open at the same time, both will have elements with id="modal-title", violating the HTML requirement that IDs be unique. Consider generating a unique ID (e.g., using useId() from React 19) and passing it to both aria-labelledby and the title element.
Suggested approach using React's useId
+import { useId } from 'react';
export const Modal: React.FC<ModalProps> = ({ isOpen, onClose, title, children, footer }) => {
const { style, mode } = useTheme();
+ const titleId = useId();
// ... variants ...
return (
<AnimatePresence>
{isOpen && (
- <div className="fixed inset-0 z-50 flex items-center justify-center p-4" role="dialog" aria-modal="true" aria-labelledby="modal-title">
+ <div className="fixed inset-0 z-50 flex items-center justify-center p-4" role="dialog" aria-modal="true" aria-labelledby={titleId}>
{/* ... */}
- <h3 id="modal-title" className={...}>{title}</h3>
+ <h3 id={titleId} className={...}>{title}</h3>🤖 Prompt for AI Agents
In `@web/components/ui/Modal.tsx` at line 46, The Modal component currently
hardcodes id="modal-title", which can collide when multiple modals render;
update the Modal (in web/components/ui/Modal.tsx) to generate a unique id (e.g.,
call React's useId() at top of the Modal component), assign that generated id to
the dialog's aria-labelledby attribute and to the modal title element (the
element that currently has id="modal-title"); add the necessary import for useId
and replace the hardcoded string with the generated id so each Modal instance
gets a unique, matching aria-labelledby/title id pair.
| <button onClick={onClose} className="hover:rotate-90 transition-transform duration-200" aria-label="Close modal"> | ||
| <X size={24} /> | ||
| </button> |
There was a problem hiding this comment.
Add type="button" to prevent unintended form submission.
The close button lacks an explicit type attribute. Buttons default to type="submit", which can cause unintended form submissions when the Modal wraps form content.
Proposed fix
- <button onClick={onClose} className="hover:rotate-90 transition-transform duration-200" aria-label="Close modal">
+ <button type="button" onClick={onClose} className="hover:rotate-90 transition-transform duration-200" aria-label="Close modal">
<X size={24} />
</button>📝 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.
| <button onClick={onClose} className="hover:rotate-90 transition-transform duration-200" aria-label="Close modal"> | |
| <X size={24} /> | |
| </button> | |
| <button type="button" onClick={onClose} className="hover:rotate-90 transition-transform duration-200" aria-label="Close modal"> | |
| <X size={24} /> | |
| </button> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 68-68: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
In `@web/components/ui/Modal.tsx` around lines 68 - 70, The close button in
Modal.tsx currently can act as a submit button when placed inside forms; update
the <button> element that calls onClose (the button rendering the X icon) to
include an explicit type="button" attribute so it never triggers form
submission, keeping the onClose handler and existing classes/aria-label
unchanged.
| const confirm = useCallback((options: ConfirmOptions) => { | ||
| setOptions(options); | ||
| setIsOpen(true); | ||
| return new Promise<boolean>((resolve) => { | ||
| setResolveRef(() => resolve); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider handling concurrent confirm() calls.
If confirm() is called while a dialog is already open, the previous promise's resolve function is overwritten, leaving the earlier promise unresolved indefinitely. While unlikely in typical usage, this could cause issues if triggered programmatically.
Optional: Reject previous promise on new confirm call
const confirm = useCallback((options: ConfirmOptions) => {
+ // Resolve any pending promise as false before opening new dialog
+ if (resolveRef) {
+ resolveRef(false);
+ setResolveRef(null);
+ }
setOptions(options);
setIsOpen(true);
return new Promise<boolean>((resolve) => {
setResolveRef(() => resolve);
});
- }, []);
+ }, [resolveRef]);🤖 Prompt for AI Agents
In `@web/contexts/ConfirmContext.tsx` around lines 26 - 32, The current confirm
function overwrites the previous pending promise's resolver, leaving it
unresolved; update confirm to first check for an existing resolver (e.g.,
resolveRef.current or the existing value held by setResolveRef) and settle it
(call the previous resolve with false or call a stored reject) before storing
the new resolve and returning the new Promise; specifically, in confirm, before
setResolveRef(() => resolve) call the existing resolver if present to
resolve(false) (or reject) so the previous promise is not left hanging, then
proceed to setOptions, setIsOpen, and set the new resolver.
|
@jules In 366-393: Unsettled balance check happens after confirmation. The check for unsettled balances (lines 379-385) occurs after the user confirms removal. Consider validating this before showing the confirmation dialog to avoid a confusing experience where the user confirms but the action still fails. |
This PR introduces a complete Confirmation Dialog system to the Web application, replacing the native and intrusive
window.confirmdialogs with a custom, accessible, and themable UI.Changes:
web/components/ui/ConfirmDialog.tsx- A specialized modal for confirmations with support for 'danger', 'warning', and 'info' variants.web/contexts/ConfirmContext.tsx- Provides auseConfirmhook that returns a Promise, allowing for cleaner async/await usage in components.ConfirmProviderwithinweb/App.tsx.web/pages/GroupDetails.tsxto use the new system for all destructive actions.Modalcomponent to include proper ARIA attributes, ensuring it is correctly identified by screen readers.Verification:
Modalstyles works for both themes.PR created automatically by Jules for task 12669270586709539879 started by @Devasy23
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.