Stop propagation of the Escape event when looker is a Modal [FOEPD-862] #6387
Conversation
WalkthroughUpdates Escape key handling across Grid and Looker: Grid now prompts for confirmation before clearing selections when no modal is active; Looker stops Escape propagation when controls/modal are shown to prevent Grid from receiving the event. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant L as Looker (keydown handler)
participant G as Grid useEscape
participant D as Dialog (confirm)
rect rgba(230,240,255,0.6)
note over U,L: Updated Escape handling
end
U->>L: Press Escape
alt Controls/modal visible (state.options.showControls === true)
L-->>L: stopPropagation()
note right of L: Prevents Escape from reaching Grid
else No controls/modal
L-->>G: keydown event (Escape)
alt modal == null
G->>D: Show confirm "You are about to clear all selections..."
alt User confirms
D-->>G: Confirmed
G-->>G: reset(fos.selectedSamples)
else User cancels
D-->>G: Canceled
end
else modal active
G-->>G: No reset triggered
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
jleven
changed the title
jleven
changed the title
![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
🧹 Nitpick comments (1)
app/packages/core/src/components/Grid/useEscape.ts (1)
24-26: Replace nativeconfirm()with a React modal component.Using the browser's native
confirm()dialog breaks the application's visual consistency and is blocking, which prevents React from updating during the confirmation.Consider using the application's modal system instead:
// Import the modal action import { showConfirmationModal } from "@fiftyone/components"; // Replace confirm() with: const confirmed = await showConfirmationModal({ title: "Clear Selections", message: "You are about to clear all selections. This cannot be undone. Are you sure?", confirmText: "Clear", cancelText: "Cancel" }); if (confirmed) { reset(fos.selectedSamples); }Note: Verify the exact modal component API in your codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/core/src/components/Grid/useEscape.ts(1 hunks)app/packages/looker/src/elements/common/looker.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker/src/elements/common/looker.tsapp/packages/core/src/components/Grid/useEscape.ts
🧬 Code graph analysis (1)
app/packages/core/src/components/Grid/useEscape.ts (1)
app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts (1)
reset(237-243)
🔇 Additional comments (1)
app/packages/looker/src/elements/common/looker.ts (1)
44-45: Escape key propagation is already stopped synchronously. Theupdatecallback (includingmatchedControl.action) and the subsequente.stopPropagation()run in the same call stack before the event bubbles, so Escape cannot escape before being stopped.
| // TODO: modal is always `null` here right after a modal closes, so this isn't the condition we want | ||
| if (modal === null) { | ||
| if ( | ||
| confirm( | ||
| "You are about to clear all selections. This cannot be undone. Are you sure?" | ||
| ) | ||
| ) { | ||
| reset(fos.selectedSamples); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Acknowledged race condition makes this logic unreliable.
The TODO on line 21 explicitly states the condition is incorrect: modal is always null immediately after a modal closes due to a race condition. This means:
- The check
modal === nulldoesn't reliably distinguish "no modal was open" from "modal just closed" - Users closing a modal with Escape might immediately see a confirmation dialog, which is confusing UX
- The intended behavior (confirm before clearing when NO modal is open) may not work correctly
This requires a proper fix rather than shipping with a known bug. Consider:
- Use event propagation state: Since
looker.tsnow callsstopPropagation()for modal Escapes, checkevent.defaultPreventedor use a flag to determine if Escape came from a modal - Use escapeKeyHandlerIdsAtom: This atom (checked on line 13-17) already tracks active escape handlers. Register the modal as an escape handler rather than relying on modal state
- Add a frame delay: Use
requestAnimationFrameto defer the check, allowing modal state to update
Run the following to understand the modal lifecycle:
🏁 Script executed:
#!/bin/bash
# Description: Examine modal state management and escape handling
# Find modal selector definition and usage
rg -n --type=ts -C5 'modalSelector'
# Find escapeKeyHandlerIdsAtom usage
rg -n --type=ts -C5 'escapeKeyHandlerIdsAtom'
# Check if modals register as escape handlers
rg -n --type=ts -C3 'escapeKeyHandlerId.*modal|modal.*escapeKeyHandlerId'Length of output: 34416
Grid/useEscape.ts: fix Escape handler race condition and replace native confirm
- Checking
modal === nullimmediately after a modal closes misfires, sincemodalSelectoris reset on close; useevent.defaultPreventedor track active handlers viaescapeKeyHandlerIdsAtominstead ofmodal === null. - Replace the blocking
window.confirm()with a non-blocking React modal/dialog for consistent UX. - Optionally defer the clear check (e.g. via
requestAnimationFrame) to allowmodalSelectorstate to settle.
| // If this is a Modal, we don't want Escape to bubble up to the Grid | ||
| // Hack: `lookerOptions` only sets showControl=true when a Modal | ||
| const isModal = state.options.showControls === true; | ||
| if (e.key === "Escape" && isModal) { | ||
| e.stopPropagation(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the modal detection approach and document the coupling.
The comment explicitly labels this as a "Hack" and relies on the assumption that lookerOptions only sets showControls=true for modals. This creates fragile coupling that could break if the options usage changes elsewhere.
Run the following script to verify that showControls is only used for modal contexts:
Additionally, consider:
- Extracting an explicit
isModalproperty in the state/options to make this relationship explicit rather than relying on a side effect - Documenting this coupling in both the looker options definition and this usage site
🏁 Script executed:
#!/bin/bash
# Description: Check all usages of showControls to verify it's only used for modals
# Search for all assignments to showControls
rg -n --type=ts -C3 'showControls\s*[:=]'
# Search for all usages of options.showControls
rg -n --type=ts -C3 'options\.showControls'Length of output: 10101
Introduce an explicit isModal flag and use it for Escape handling instead of showControls. state.options.showControls is also toggled for hover and initial display, so it isn’t a reliable modal indicator. Add an isModal property to your looker options/state and update this handler to check state.options.isModal rather than showControls.
🤖 Prompt for AI Agents
In app/packages/looker/src/elements/common/looker.ts around lines 41 to 46,
replace the current modal-detection that relies on state.options.showControls
with an explicit state.options.isModal flag: add isModal to the looker options
interface and default initialization (preserve existing showControls behavior),
update any code that constructs/merges looker options to accept and propagate
isModal, and change the Escape key handler to check state.options.isModal (and
call e.stopPropagation() only when true). Ensure type definitions and any
consumers are updated for the new flag and maintain backward compatibility by
defaulting isModal to false when not provided.
1. Stops propagation when looker is a Modal 2. Using the Escape key to clear selections in the grid now first opens a confirmation modal Hack: used `showControls === true` as an isModal check Ideally, looker would have an explicit isModel flag to check
jleven
force-pushed
the
josh/explore-modal-close-selection-clear
branch
from
bf3d2c3 to
c1f3ddc
Compare
| if (modal === null) { | ||
| if ( | ||
| confirm( | ||
| "You are about to clear all selections. This cannot be undone. Are you sure?" |
There was a problem hiding this comment.
nit: can we use this copy?
Are you sure you want to clear your current selection?
2 participants
What changes are proposed in this pull request?
Two changes:
How is this patch tested? If it is not, please explain why.
Clicked around in the app:
Selecting and Clearing:
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Fixed bug where grid selections would be cleared when closing a modal using the escape key. Also added a confirmation dialog when clearing grid selections using the escape key.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes