fix(modal-v2): compose scroll locks + respect nested Radix layers on Escape - #1675
Open
michaelassraf wants to merge 2 commits into
Open
fix(modal-v2): compose scroll locks + respect nested Radix layers on Escape#1675michaelassraf wants to merge 2 commits into
michaelassraf wants to merge 2 commits into
Conversation
…Escape [preview:none] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds reusable frontend UI components, expands ChangesFrontend UI updates
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant DropdownButton
participant ActionsMenu
participant Button
User->>DropdownButton: select trigger
DropdownButton->>Button: render standard trigger when configured
DropdownButton->>ActionsMenu: pass grouped or flat menu data
ActionsMenu-->>User: display menu actions
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@openframe-frontend-core/src/components/ui/modal-v2.tsx`:
- Around line 151-157: Update the Escape handling in the ModalV2 document-level
useEffect so onClose is invoked only when the current modal is the topmost entry
in modalStack. Preserve the existing event.key === 'Escape' and
!event.defaultPrevented checks for nested Radix layers, and add the stack check
before calling onClose.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18c76279-f47e-4736-8259-0a1503d80e30
📒 Files selected for processing (2)
openframe-frontend-core/package.jsonopenframe-frontend-core/src/components/ui/modal-v2.tsx
Comment on lines
151
to
+157
| // Escape key (document-level: top-of-stack semantics for modals) | ||
| useEffect(() => { | ||
| const handleKeyDown = (event: KeyboardEvent) => { | ||
| if (event.key === 'Escape') { | ||
| // A nested Radix layer (Select, DropdownMenu) preventDefaults the | ||
| // Escape it consumes — without this check, closing a select inside | ||
| // the modal closed the WHOLE modal. | ||
| if (event.key === 'Escape' && !event.defaultPrevented) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict Escape handling to the topmost modal.
Each open ModalV2 instance registers this document listener. The listener does not check modalStack. If two modals are open, one Escape can call onClose for both instances. event.defaultPrevented only filters nested Radix layers.
Add a top-of-stack check before calling onClose.
Proposed fix
- if (event.key === 'Escape' && !event.defaultPrevented) {
+ if (
+ event.key === 'Escape' &&
+ !event.defaultPrevented &&
+ modalStack[modalStack.length - 1] === stackIdRef.current
+ ) {
onClose()
}📝 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
| // Escape key (document-level: top-of-stack semantics for modals) | |
| useEffect(() => { | |
| const handleKeyDown = (event: KeyboardEvent) => { | |
| if (event.key === 'Escape') { | |
| // A nested Radix layer (Select, DropdownMenu) preventDefaults the | |
| // Escape it consumes — without this check, closing a select inside | |
| // the modal closed the WHOLE modal. | |
| if (event.key === 'Escape' && !event.defaultPrevented) { | |
| // Escape key (document-level: top-of-stack semantics for modals) | |
| useEffect(() => { | |
| const handleKeyDown = (event: KeyboardEvent) => { | |
| // A nested Radix layer (Select, DropdownMenu) preventDefaults the | |
| // Escape it consumes — without this check, closing a select inside | |
| // the modal closed the WHOLE modal. | |
| if ( | |
| event.key === 'Escape' && | |
| !event.defaultPrevented && | |
| modalStack[modalStack.length - 1] === stackIdRef.current | |
| ) { | |
| 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 `@openframe-frontend-core/src/components/ui/modal-v2.tsx` around lines 151 -
157, Update the Escape handling in the ModalV2 document-level useEffect so
onClose is invoked only when the current modal is the topmost entry in
modalStack. Preserve the existing event.key === 'Escape' and
!event.defaultPrevented checks for nested Radix layers, and add the stack check
before calling onClose.
…oHint, JsonDetailList, DropdownButton groups/variant/loading, Tabs admin-rail variant, useConfirm/useGuardedClose/UnsavedChangesChip + stories [preview:none] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@openframe-frontend-core/src/components/ui/dropdown-button.tsx`:
- Around line 81-89: Update handleOpenChange so disabled or loading only
prevents opening the menu, not closing it: guard the setOpen call when next is
true and either state is active, while allowing next === false to close an
already-open menu.
In `@openframe-frontend-core/src/components/ui/field.tsx`:
- Line 24: Update the Field component’s children contract to require a render
function accepting FieldRenderProps, removing support for plain React nodes. In
the Field render logic, invoke this function directly with renderProps so the
child receives controlId and the Label target remains valid.
In `@openframe-frontend-core/src/components/ui/json-detail-list.tsx`:
- Around line 72-92: Update JsonDetailList to track whether a string parse
succeeded, and use that state in the non-object/array branch. Render parsed
arrays and scalar values from obj through Value, while retaining the original
data string only when JSON.parse fails; preserve existing handling for
non-string inputs.
In `@openframe-frontend-core/src/components/ui/modal-guarded-close.tsx`:
- Around line 36-63: Update useConfirm to store the active pending confirmation
request in a ref alongside its state, and add an unmount cleanup effect that
resolves any active request with false and clears the asking state. Keep decide
synchronized with the ref so normal decisions resolve the same request without
later cleanup resolving it again.
In `@openframe-frontend-core/src/stories/DropdownButton.stories.tsx`:
- Line 30: Replace the raw pixel sizing utilities with the appropriate ODS
sizing tokens at all affected sites:
openframe-frontend-core/src/stories/DropdownButton.stories.tsx lines 30-30,
openframe-frontend-core/src/components/ui/info-hint.tsx lines 39-39, and
openframe-frontend-core/src/stories/InfoHint.stories.tsx lines 34-34 and 52-52.
Preserve the existing layout intent while removing min-h-[320px], max-w-[280px],
and both min-h-[160px] utilities.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 160cc0a8-07cf-455b-ae6c-6097b24cc651
📒 Files selected for processing (13)
openframe-frontend-core/src/components/ui/dropdown-button.tsxopenframe-frontend-core/src/components/ui/field.tsxopenframe-frontend-core/src/components/ui/index.tsopenframe-frontend-core/src/components/ui/info-hint.tsxopenframe-frontend-core/src/components/ui/json-detail-list.tsxopenframe-frontend-core/src/components/ui/modal-guarded-close.tsxopenframe-frontend-core/src/components/ui/tabs.tsxopenframe-frontend-core/src/stories/DropdownButton.stories.tsxopenframe-frontend-core/src/stories/Field.stories.tsxopenframe-frontend-core/src/stories/InfoHint.stories.tsxopenframe-frontend-core/src/stories/JsonDetailList.stories.tsxopenframe-frontend-core/src/stories/ModalGuardedClose.stories.tsxopenframe-frontend-core/src/stories/Tabs.stories.tsx
Comment on lines
81
to
+89
| const [open, setOpen] = React.useState(false) | ||
|
|
||
| const handleOpenChange = React.useCallback( | ||
| (next: boolean) => { | ||
| if (disabled) return | ||
| if (disabled || loading) return | ||
| setOpen(next) | ||
| }, | ||
| [disabled] | ||
| [disabled, loading] | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow the menu to close after the trigger becomes disabled or loading.
Line 85 rejects both open and close transitions. If the menu is open when loading or disabled changes to true, Escape and outside-click dismissal cannot close it.
Close the menu when either state becomes true. Only block next === true.
Proposed fix
const [open, setOpen] = React.useState(false)
+ React.useEffect(() => {
+ if (disabled || loading) setOpen(false)
+ }, [disabled, loading])
+
const handleOpenChange = React.useCallback(
(next: boolean) => {
- if (disabled || loading) return
+ if (next && (disabled || loading)) return
setOpen(next)
},📝 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 [open, setOpen] = React.useState(false) | |
| const handleOpenChange = React.useCallback( | |
| (next: boolean) => { | |
| if (disabled) return | |
| if (disabled || loading) return | |
| setOpen(next) | |
| }, | |
| [disabled] | |
| [disabled, loading] | |
| ) | |
| const [open, setOpen] = React.useState(false) | |
| React.useEffect(() => { | |
| if (disabled || loading) setOpen(false) | |
| }, [disabled, loading]) | |
| const handleOpenChange = React.useCallback( | |
| (next: boolean) => { | |
| if (next && (disabled || loading)) return | |
| setOpen(next) | |
| }, | |
| [disabled, loading] | |
| ) |
🤖 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 `@openframe-frontend-core/src/components/ui/dropdown-button.tsx` around lines
81 - 89, Update handleOpenChange so disabled or loading only prevents opening
the menu, not closing it: guard the setOpen call when next is true and either
state is active, while allowing next === false to close an already-open menu.
| /** Definition shown in a hover `InfoHint` next to the label. */ | ||
| hint?: React.ReactNode | ||
| required?: boolean | ||
| children: React.ReactNode | ((props: FieldRenderProps) => React.ReactNode) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require the render-prop child contract.
Line 24 permits a plain React node. That node cannot receive controlId. The Label at Line 56 then points to an element that does not exist.
Make children a required render function. Render it directly with renderProps.
Proposed fix
export interface FieldProps {
label: string
hint?: React.ReactNode
required?: boolean
- children: React.ReactNode | ((props: FieldRenderProps) => React.ReactNode)
+ children: (props: FieldRenderProps) => React.ReactNode
error?: string | null
}
- {typeof children === 'function' ? children(renderProps) : children}
+ {children(renderProps)}Also applies to: 63-63
🤖 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 `@openframe-frontend-core/src/components/ui/field.tsx` at line 24, Update the
Field component’s children contract to require a render function accepting
FieldRenderProps, removing support for plain React nodes. In the Field render
logic, invoke this function directly with renderProps so the child receives
controlId and the Label target remains valid.
Comment on lines
+72
to
+92
| export function JsonDetailList({ data, depth = 0 }: JsonDetailListProps) { | ||
| const obj = | ||
| typeof data === 'string' | ||
| ? (() => { | ||
| try { | ||
| return JSON.parse(data) as unknown | ||
| } catch { | ||
| return null | ||
| } | ||
| })() | ||
| : data | ||
|
|
||
| if (!obj || typeof obj !== 'object' || Array.isArray(obj)) { | ||
| // Not an object — render whatever it is faithfully. | ||
| return typeof data === 'string' ? ( | ||
| <p className="whitespace-pre-wrap text-h6 text-ods-text-primary">{data}</p> | ||
| ) : ( | ||
| <pre className="whitespace-pre-wrap text-code text-ods-text-secondary"> | ||
| {JSON.stringify(data, null, 2)} | ||
| </pre> | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render parsed JSON arrays and scalar values from obj.
A valid JSON string such as ["a","b"] reaches Line 84 because obj is an array. Line 86 then renders the original source string instead of the parsed array. The same occurs for JSON scalar strings.
Track whether parsing succeeded. For parsed non-object values, render obj through Value. Keep the raw-string fallback only for parse failures.
🤖 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 `@openframe-frontend-core/src/components/ui/json-detail-list.tsx` around lines
72 - 92, Update JsonDetailList to track whether a string parse succeeded, and
use that state in the non-object/array branch. Render parsed arrays and scalar
values from obj through Value, while retaining the original data string only
when JSON.parse fails; preserve existing handling for non-string inputs.
Comment on lines
+36
to
+63
| export function useConfirm() { | ||
| const [open, setOpen] = React.useState(false) | ||
| const [pending, setPending] = React.useState<{ | ||
| title: string | ||
| body: string | ||
| resolve: (ok: boolean) => void | ||
| } | null>(null) | ||
| const askingRef = React.useRef(false) | ||
| const ask = React.useCallback((title: string, body: string) => { | ||
| return new Promise<boolean>((resolve) => { | ||
| if (askingRef.current) { | ||
| resolve(false) | ||
| return | ||
| } | ||
| askingRef.current = true | ||
| setPending({ title, body, resolve }) | ||
| setOpen(true) | ||
| }) | ||
| }, []) | ||
| const decide = React.useCallback( | ||
| (ok: boolean) => { | ||
| askingRef.current = false | ||
| setOpen(false) | ||
| pending?.resolve(ok) | ||
| setPending(null) | ||
| }, | ||
| [pending] | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file:"
git ls-files | rg '^openframe-frontend-core/src/components/ui/modal-guarded-close\.tsx$' || true
echo
echo "File outline and relevant contents:"
if [ -f openframe-frontend-core/src/components/ui/modal-guarded-close.tsx ]; then
wc -l openframe-frontend-core/src/components/ui/modal-guarded-close.tsx
sed -n '1,150p' openframe-frontend-core/src/components/ui/modal-guarded-close.tsx | cat -n
fi
echo
echo "Search usages of useConfirm/useGuardedClose:"
rg -n "useConfirm|useGuardedClose|guard.*close|UnsavedChangesChip" openframe-frontend-core/src -g '!components/ui/modal-guarded-close.tsx' || true
echo
echo "Static check of hook call order and cleanup presence in file:"
python3 - <<'PY'
from pathlib import Path
p=Path('openframe-frontend-core/src/components/ui/modal-guarded-close.tsx')
lines=p.read_text().splitlines()
in_use_confirm=False
brace=0
for i,line in enumerate(lines,1):
if 'export function useConfirm' in line:
in_use_confirm=True; start=i
if in_use_confirm:
print(f"{i}: {line}")
PYRepository: flamingo-stack/openframe-oss-lib
Length of output: 12002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file:"
git ls-files | rg '^openframe-frontend-core/src/components/ui/modal-guarded-close\.tsx$' || true
echo
echo "File outline and relevant contents:"
if [ -f openframe-frontend-core/src/components/ui/modal-guarded-close.tsx ]; then
wc -l openframe-frontend-core/src/components/ui/modal-guarded-close.tsx
sed -n '1,150p' openframe-frontend-core/src/components/ui/modal-guarded-close.tsx | cat -n
fi
echo
echo "Search usages of useConfirm/useGuardedClose:"
rg -n "useConfirm|useGuardedClose|guard.*close|UnsavedChangesChip" openframe-frontend-core/src -g '!components/ui/modal-guarded-close.tsx' || true
echo
echo "Static check of hook call order and cleanup presence in file:"
python3 - <<'PY'
from pathlib import Path
p=Path('openframe-frontend-core/src/components/ui/modal-guarded-close.tsx')
lines=p.read_text().splitlines()
in_use_confirm=False
for i,line in enumerate(lines,1):
if 'export function useConfirm' in line:
in_use_confirm=True; start=i
if in_use_confirm:
print(f"{i}: { line }")
PYRepository: flamingo-stack/openframe-oss-lib
Length of output: 12002
Resolve an active confirmation during unmount.
If the host unmounts while the confirmation is open, ask() never resolves. This leaves callers waiting on the promise after the only decision UI has been removed.
Store the active request in a ref and resolve it with false from a cleanup effect when the component unmounts.
🤖 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 `@openframe-frontend-core/src/components/ui/modal-guarded-close.tsx` around
lines 36 - 63, Update useConfirm to store the active pending confirmation
request in a ref alongside its state, and add an unmount cleanup effect that
resolves any active request with false and clears the asking state. Keep decide
synchronized with the ref so normal decisions resolve the same request without
later cleanup resolving it again.
| }, | ||
| decorators: [ | ||
| (Story) => ( | ||
| <div className="flex min-h-[320px] items-start justify-center p-8"> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace raw pixel dimensions with ODS sizing tokens.
These additions use raw pixel dimensions instead of ODS sizing tokens.
openframe-frontend-core/src/stories/DropdownButton.stories.tsx#L30-L30: replacemin-h-[320px].openframe-frontend-core/src/components/ui/info-hint.tsx#L39-L39: replacemax-w-[280px].openframe-frontend-core/src/stories/InfoHint.stories.tsx#L34-L34: replacemin-h-[160px].openframe-frontend-core/src/stories/InfoHint.stories.tsx#L52-L52: replacemin-h-[160px].
As per coding guidelines, “Do not use hardcoded styles; translate Figma/Tailwind output and all hand-written styling to ODS tokens, raw hex colors, pixel sizes, and font shorthands are not allowed.”
📍 Affects 3 files
openframe-frontend-core/src/stories/DropdownButton.stories.tsx#L30-L30(this comment)openframe-frontend-core/src/components/ui/info-hint.tsx#L39-L39openframe-frontend-core/src/stories/InfoHint.stories.tsx#L34-L34openframe-frontend-core/src/stories/InfoHint.stories.tsx#L52-L52
🤖 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 `@openframe-frontend-core/src/stories/DropdownButton.stories.tsx` at line 30,
Replace the raw pixel sizing utilities with the appropriate ODS sizing tokens at
all affected sites:
openframe-frontend-core/src/stories/DropdownButton.stories.tsx lines 30-30,
openframe-frontend-core/src/components/ui/info-hint.tsx lines 39-39, and
openframe-frontend-core/src/stories/InfoHint.stories.tsx lines 34-34 and 52-52.
Preserve the existing layout intent while removing min-h-[320px], max-w-[280px],
and both min-h-[160px] utilities.
Source: Coding guidelines
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.
Two ModalV2 defects surfaced by real-input testing on the hub (Radix Select open inside a form modal):
Scroll-lock fight. ModalV2 locked scroll with react-aria's
usePreventScroll; a Radix Select opened inside the modal adds its ownreact-remove-scrolllock on top. The two techniques don't compose — body jumped and the page behind rendered black while the select was open. ModalV2 now locks withreact-remove-scrollitself (declared dep), which is ref-counted and composes with the locks Radix primitives add.Escape closed the whole modal. ModalV2's document-level Escape handler ignored
event.defaultPrevented, so the Escape a nested Select/DropdownMenu consumed ALSO closed the modal. Now checked.🤖 Generated with Claude Code
Summary by CodeRabbit