Stop propagation of the Escape event when looker is a Modal [FOEPD-862] #6387
Conversation
WalkthroughGrid now prompts for confirmation before clearing selections when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant L as Looker (keydown handler / config:isModal)
participant G as Grid useEscape
participant D as Confirm Dialog
rect rgba(230,240,255,0.6)
note over U,L: Escape handling updated with modal-aware propagation
end
U->>L: Press Escape
alt Looker configured as modal (isModal === true)
L-->>L: stopPropagation() (prevent bubble)
note right of L: Escape handled/consumed by Looker
else Looker not modal (isModal !== true)
L->>G: keydown event (Escape) bubbles
alt modal == null and selectedSamples exist
G->>D: Show confirm "Are you sure you want to clear your current selection?"
alt User confirms
D-->>G: Confirmed
G-->>G: reset(fos.selectedSamples)
else User cancels
D-->>G: Canceled (no reset)
end
else modal active or no selections
G-->>G: No reset
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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.
| // 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.
bf3d2c3 to
c1f3ddc
Compare
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
use it to reliably stopPropagation of the Escape key event
jleven
force-pushed
the
josh/explore-modal-close-selection-clear
branch
from
f862e4a to
3e9d53d
Compare
| // TODO: modal is always `null` here right after a modal closes, so this isn't the condition we want | ||
| if (modal === null && selectedSampleIds.size > 0) { |
There was a problem hiding this comment.
I'm not sure I understand. This seems to work. I assume only if propagation is stopped in looker?
There was a problem hiding this comment.
I think modal === null was put here to try to accomplish what this PR is actually accomplishing. Unfortunately, modal is always null here, so we do need this PR.
TODO
isModalis the right way to do it.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:
When nothing is selected:
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
Chores