fix(settings): scope Escape to the open modal and polish shared UI - #2163
Closed
cpvalente wants to merge 2 commits into
Closed
fix(settings): scope Escape to the open modal and polish shared UI#2163cpvalente wants to merge 2 commits into
cpvalente wants to merge 2 commits into
Conversation
Modals inside the settings panel replaced inline forms that used preventEscape to stop Escape reaching the panel. Base UI dismisses its own popup and calls stopPropagation, but the panel handler listens on the same document node, so stopPropagation cannot reach it and both fire in registration order. Track open overlays instead and stand the panel handler down while one is open. Also: - give Tag a transparent border so the active variant does not resize it - move the modal width onto explicit size classes, dropping the dead min-width resets and the missing default class - normalise the settings search query once at the entry points - render the trigger delete error once rather than after every row - let Panel.Field omit its description Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LNZdnMwj99B7xxGB8xqhyY
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
The Escape-key count only needs to be read at the moment a key is pressed, not rendered — a plain module counter does the job without a store, subscriptions, or the isDisabled indirection through useKeyDown. AppSettings now wraps `close` itself and checks the counter synchronously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LNZdnMwj99B7xxGB8xqhyY
Owner
Author
|
we have no evidence of the issue |
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
2 participants
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.
Summary
Follow-up fixes on top of #2154, found during review:
preventEscapeguard the inline forms used to stop Escape from also closing the whole settings panel. Base UI's dialog dismiss and the settings panel's ownEscapelistener both sit ondocument, sostopPropagationfrom one doesn't reach the other — both would fire. Added a smalloverlayStorethatModal/Dialogregister into while open, and the settings panel now disables its own Escape handler while any overlay is open.Tag: reserved a transparent border on the base class so theactivevariant's border doesn't resize the tag.Modal: moved width off the base.modalclass onto explicit.default/.compact/.small/.wideclasses, removing deadmin-width: 0resets and a.defaultgap that leftstyle[size]unresolved on the default path.useAppSettingsMenu: search query is now normalised once (normaliseSettingsQuery) at the call sites instead of being re-trimmed/re-lowercased insidematchesSettingsOptionQueryand again in the submit handler.TriggersList: the delete error banner now renders once per list instead of once per row.Panel.Field:descriptionis optional, soAppVersion's error state no longer passes an empty string for it.Test plan
pnpm typecheck— cleanpnpm lint— 0 errors, 119 warnings (same as base)pnpm build— verified.default/.compact/.small/.wideall emit in the built Modal CSS, andTag's.activeborder-color renders correctlypnpm test— 223/223 passingGenerated by Claude Code