Open
Conversation
📝 WalkthroughWalkthroughAdds a new Confirmation component and Storybook stories; modernizes Alert and Modal stories to args-based renders with additional variants; enhances Modal runtime behavior (focus trap, aria wiring, portal/theme handling) and styling tweaks (alert/notice padding, modal slot borders); exposes getThemeStyles helper. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Storybook
participant Confirmation
participant Modal
participant AppCallbacks
User->>Storybook: click "Open Confirmation" (story UI)
Storybook->>Confirmation: render with props (open=true)
Confirmation->>Modal: open(modalProps)
Modal->>Modal: trap focus, apply theme vars, lock body scroll
User->>Modal: click "Confirm"
Modal->>Confirmation: onConfirm()
Confirmation->>AppCallbacks: invoke onConfirm callback
Confirmation->>Modal: close()
Modal->>Modal: release focus trap, restore scroll, cleanup portal
Modal->>Storybook: closed state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/modal.tsx (1)
395-406:⚠️ Potential issue | 🟡 MinorEscape handler will close all stacked modals simultaneously.
The
keydownlistener is added ondocument, so if two modals are open at the same time, a single Escape press triggersonClosefor both. Consider stopping propagation or checking if this modal is the topmost before closing.
🤖 Fix all issues with AI agents
In `@src/components/ui/confirmation.tsx`:
- Around line 86-92: The fallback ModalDescription currently renders only
whitespace when body is null, leaving aria-describedby pointing at an empty
element; update the component that renders ModalDescription (look for the
Confirmation dialog render where body and title are used) to either omit the
ModalDescription entirely when body is null (and ensure aria-describedby is not
set) or render a meaningful default (e.g., repeat the title text) instead of a
blank space; modify the logic around body and the aria-describedby prop so it
only references ModalDescription when it contains real content.
In `@src/components/ui/Modal.stories.tsx`:
- Line 97: The Countries field is displaying the wrong helper text: replace the
usage of ZONE_NAME_HELPER inside the FieldDescription for the Countries field
with the appropriate helper (e.g., COUNTRIES_HELPER) or remove the
FieldDescription entirely; locate the FieldDescription component instance that
currently renders ZONE_NAME_HELPER and update it to reference the correct
constant (or an inline string) so the Countries field shows relevant copy
instead of the zone-name helper.
🧹 Nitpick comments (4)
src/components/ui/Modal.stories.tsx (1)
81-96: Dropdown is display-only with no selection state.The Countries dropdown renders static text ("Bangladesh, United States") and clicking items does nothing. While this is a Storybook demo, a minimal
useStatefor selection would make the story more useful for demonstrating the component's interactive behavior, consistent with therestOfWorldtoggle above.src/components/ui/confirmation.tsx (1)
51-59:onClose()is called beforeonConfirm/onCancelcallbacks.This means the modal always closes immediately, before the consumer's callback runs. If a consumer needs to perform an async operation (e.g., API call) and keep the dialog open on failure, this ordering prevents that. Consider letting the consumer control closing, or at minimum document this behavior.
src/components/ui/modal.tsx (2)
408-440: Focus trap uses a stale list of focusable elements.The focusable elements are queried once when the layout effect runs (on
[portalContainer, open]changes). If modal content changes dynamically after mount (e.g., a loading spinner replaced by form fields), the Tab-trap will reference stale elements. Consider re-querying focusables inside thehandleKeyDowncallback instead.Proposed fix: query focusables at Tab-time
useLayoutEffect(() => { if (!portalContainer || !open) return; const container = contentRef.current; if (!container) return; const focusables = container.querySelectorAll<HTMLElement>( FOCUSABLE_SELECTOR, ); const first = focusables[0]; - const last = focusables[focusables.length - 1]; (first ?? container).focus(); const handleKeyDown = (e: KeyboardEvent) => { if (e.key !== "Tab") return; + const currentFocusables = container.querySelectorAll<HTMLElement>( + FOCUSABLE_SELECTOR, + ); + const firstEl = currentFocusables[0]; + const lastEl = currentFocusables[currentFocusables.length - 1]; + if (e.shiftKey) { - if (document.activeElement === first) { + if (document.activeElement === firstEl) { e.preventDefault(); - last?.focus(); + lastEl?.focus(); } } else { - if (document.activeElement === last) { + if (document.activeElement === lastEl) { e.preventDefault(); - first?.focus(); + firstEl?.focus(); } } };
297-297: Module-levelbodyScrollLockCountmay leak across tests.Since this is a module-scoped variable, it persists across test cases. If a test fails mid-render or doesn't unmount properly, the count drifts and subsequent tests may see
overflow: hiddenstuck ondocument.body. Consider resetting it in a test helper or using a ref-based approach.
Comment on lines
+86
to
+92
| {body != null ? ( | ||
| <ModalDescription className="p-5 pt-0 text-muted-foreground"> | ||
| {body} | ||
| </ModalDescription> | ||
| ) : ( | ||
| <ModalDescription className="sr-only"> </ModalDescription> | ||
| )} |
There was a problem hiding this comment.
Screen-reader-only description contains only whitespace.
When body is not provided, the fallback ModalDescription with className="sr-only" contains just a space. This means aria-describedby points to an element with no meaningful content, which can confuse screen readers. Consider either omitting the description entirely (and not setting aria-describedby) or providing a sensible default like the title text repeated.
🤖 Prompt for AI Agents
In `@src/components/ui/confirmation.tsx` around lines 86 - 92, The fallback
ModalDescription currently renders only whitespace when body is null, leaving
aria-describedby pointing at an empty element; update the component that renders
ModalDescription (look for the Confirmation dialog render where body and title
are used) to either omit the ModalDescription entirely when body is null (and
ensure aria-describedby is not set) or render a meaningful default (e.g., repeat
the title text) instead of a blank space; modify the logic around body and the
aria-describedby prop so it only references ModalDescription when it contains
real content.
| ))} | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| <FieldDescription>{ZONE_NAME_HELPER}</FieldDescription> |
There was a problem hiding this comment.
Copy-paste issue: wrong helper text for the Countries field.
ZONE_NAME_HELPER ("Give a meaningful name for your reference") is reused as the description for the Countries field, which doesn't make sense. This should have its own description or be omitted.
Proposed fix
- <FieldDescription>{ZONE_NAME_HELPER}</FieldDescription>
+ <FieldDescription>Select the countries for this shipping zone</FieldDescription>📝 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
| <FieldDescription>{ZONE_NAME_HELPER}</FieldDescription> | |
| <FieldDescription>Select the countries for this shipping zone</FieldDescription> |
🤖 Prompt for AI Agents
In `@src/components/ui/Modal.stories.tsx` at line 97, The Countries field is
displaying the wrong helper text: replace the usage of ZONE_NAME_HELPER inside
the FieldDescription for the Countries field with the appropriate helper (e.g.,
COUNTRIES_HELPER) or remove the FieldDescription entirely; locate the
FieldDescription component instance that currently renders ZONE_NAME_HELPER and
update it to reference the correct constant (or an inline string) so the
Countries field shows relevant copy instead of the zone-name helper.
Closed
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.
add
Confirmationcomponents examples for the storybookSummary by CodeRabbit
New Features
Style
Accessibility