Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .axioma/spark.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,7 @@
## 2025-05-26 - [Shadowing standard props for ISO transforms]
**Learning:** For components that wrap native HTML inputs, adding `iso*` variants of standard props (like `isoMin` for `min`) allows the component to handle standard data formats while gracefully falling back to native behavior when the specialized prop is absent.
**Pattern:** Implement specialized props with an `iso` prefix and use a ternary in the render function to prioritize the transformed ISO value: `min={isoMin ? iso2LocalDateTime(isoMin) : min}`.

## 2026-05-26 - [Syncing React State with Native Events]
**Learning:** For components wrapping native elements with their own internal state (like `<dialog>`), synchronizing React state via native event listeners (like `onClose`) is critical. This ensures the component remains in sync when the state changes via browser shortcuts (ESC key) or other non-React means.
**Pattern:** Use native event handlers on the underlying element to trigger React state updates, rather than relying solely on imperative method calls (like `dialog.close()`) to drive the `onClose` logic.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ Accessible dialog/modal component built on top of the native `<dialog>` element.
| `onClose` | `() => void` | Triggered on close |
| `opener` | `ReactNode` | Element to trigger opening |
| `children` | `ReactNode` | Content inside the dialog |
| `closeOnBackdropClick` | `boolean` (default: `false`) | Whether to close when clicking the backdrop |
| `...dialogProps` | All native `<dialog>` props | Inherits all HTML dialog element attributes |

***
Expand Down
55 changes: 53 additions & 2 deletions lib/components/Dialog/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ describe('Dialog', () => {
this.setAttribute('open', '')
})
vi.spyOn(HTMLDialogElement.prototype, 'close').mockImplementation(function (this: HTMLDialogElement) {
this.removeAttribute('open')
if (this.hasAttribute('open')) {
this.removeAttribute('open')
this.dispatchEvent(new Event('close'))
}
})
})

Expand Down Expand Up @@ -70,7 +73,7 @@ describe('Dialog', () => {
expect(onOpen).toHaveBeenCalled()
})

it('should call onClose callback when closed', () => {
it('should call onClose callback when closed via prop change', () => {
const onClose = vi.fn()
const { rerender } = render(
<Dialog isOpen={true} onClose={onClose}>
Expand All @@ -84,9 +87,57 @@ describe('Dialog', () => {
</Dialog>
)

// The component calls dialog.close() which triggers the onClose event handler
expect(onClose).toHaveBeenCalled()
})

it('should call onClose and update internal state when closed via native event', () => {
const onClose = vi.fn()
const { container } = render(
<Dialog isOpen={true} onClose={onClose}>
<p>Content</p>
</Dialog>
)

const dialog = container.querySelector('dialog')
// Dispatching native close event
fireEvent(dialog!, new Event('close'))

expect(onClose).toHaveBeenCalled()
})

it('should close on backdrop click when closeOnBackdropClick is true', () => {
// Mock getBoundingClientRect for the dialog
vi.spyOn(HTMLDialogElement.prototype, 'getBoundingClientRect').mockReturnValue({
top: 100,
bottom: 400,
left: 100,
right: 400,
width: 300,
height: 300,
x: 100,
y: 100,
toJSON: () => {}
})

const onClose = vi.fn()
const { container } = render(
<Dialog isOpen={true} closeOnBackdropClick={true} onClose={onClose}>
<p>Content</p>
</Dialog>
)

const dialog = container.querySelector('dialog')

// Click inside the dialog (should not close)
fireEvent.click(dialog!, { clientX: 200, clientY: 200 })
expect(HTMLDialogElement.prototype.close).not.toHaveBeenCalled()

// Click outside the dialog (backdrop)
fireEvent.click(dialog!, { clientX: 50, clientY: 50 })
expect(HTMLDialogElement.prototype.close).toHaveBeenCalled()
})

it('should toggle dialog when opener is clicked', () => {
render(
<Dialog
Expand Down
38 changes: 35 additions & 3 deletions lib/components/Dialog/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const Dialog = (props: DialogProps): JSX.Element => {
const {
isOpen = false,
behavior = 'modal',
closeOnBackdropClick = false,
opener,
onOpen,
onClose,
Expand All @@ -62,17 +63,41 @@ export const Dialog = (props: DialogProps): JSX.Element => {
} else {
if (dialogElement.open) {
dialogElement.close()
onClose?.()
}
}
}, [open, behavior, onOpen, onClose])
}, [open, behavior, onOpen])

const handleNativeClose = () => {
setOpen(false)
onClose?.()
}
Comment on lines +70 to +73

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | βš–οΈ Poor tradeoff

Stale closure risk: handleNativeClose may capture an outdated onClose callback.

The handleNativeClose function is defined inline and will capture the onClose prop from the render where it was created. If the parent component passes a new onClose function on subsequent renders (e.g., due to inline arrow functions or changing dependencies), the event handler will continue invoking the stale callback until the component re-renders and re-attaches the handler.

This is a common pitfall in React event handlers that capture props or state.

πŸ”„ Proposed fix: wrap in useCallback or use a ref

Option 1: Wrap in useCallback (simpler)

+const handleNativeClose = useCallback(() => {
+    setOpen(false)
+    onClose?.()
+}, [onClose])
-const handleNativeClose = () => {
-    setOpen(false)
-    onClose?.()
-}

Option 2: Use a ref to always call the latest callback (no re-render on prop change)

+const onCloseRef = useRef(onClose)
+useEffect(() => {
+    onCloseRef.current = onClose
+}, [onClose])
+
+const handleNativeClose = useCallback(() => {
+    setOpen(false)
+    onCloseRef.current?.()
+}, [])
-const handleNativeClose = () => {
-    setOpen(false)
-    onClose?.()
-}

Option 2 is preferred if you want to avoid re-attaching the event listener when onClose changes, though for dialog close events this overhead is negligible.

πŸ“ 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
const handleNativeClose = () => {
setOpen(false)
onClose?.()
}
const handleNativeClose = useCallback(() => {
setOpen(false)
onClose?.()
}, [onClose])
πŸ€– 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 `@lib/components/Dialog/index.tsx` around lines 70 - 73, handleNativeClose
currently closes the dialog and calls onClose but is declared inline, risking a
stale closure over the onClose prop; update handleNativeClose to either (a) wrap
it in useCallback with onClose and setOpen in its dependency array so it always
calls the latest onClose, or (b) store the latest onClose in a ref (e.g.,
onCloseRef.current) and have handleNativeClose call that ref so the handler need
not be re-created β€” change the implementation referencing the handleNativeClose
function, the setOpen call, and the onClose prop accordingly.


const handleClick = (event: React.MouseEvent<HTMLDialogElement>) => {
if (!closeOnBackdropClick || behavior !== 'modal') return

const rect = event.currentTarget.getBoundingClientRect()
const isInDialog =
rect.top <= event.clientY &&
event.clientY <= rect.bottom &&
rect.left <= event.clientX &&
event.clientX <= rect.right

if (!isInDialog) {
dialog.current?.close()
}
}

const handleToggle = () => setOpen((prev) => !prev)

return (
<>
{opener && cloneElement(opener, { onClick: handleToggle })}
<dialog ref={dialog} {...restProps} />
<dialog
ref={dialog}
{...restProps}
onClose={handleNativeClose}
onClick={handleClick}
/>
</>
)
}
Expand Down Expand Up @@ -104,4 +129,11 @@ export interface DialogProps extends React.HTMLAttributes<HTMLDialogElement> {
* The element that triggers the dialog to open or close.
*/
opener?: ReactElement
/**
* Whether the dialog should close when clicking on the backdrop.
* Only applies when behavior is 'modal'.
*
* @default false
*/
closeOnBackdropClick?: boolean
}
Loading