[8220] Accessibility: Consider keyboard shortcut disabling #4882
[8220] Accessibility: Consider keyboard shortcut disabling #4882jvega190 wants to merge 10 commits into
Conversation
…nt for enabling shortcuts
|
Warning Review limit reached
More reviews will be available in 58 minutes and 50 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThe PR adds a per-user keyboard shortcuts enable/disable preference stored in localStorage and Redux preview state, with a new switch in PreviewSettingsPanel and shortcut blocking in PreviewConcierge. It also moves widget dialog opening and updating to the Redux dialog stack and enables widget dialogs to minimize. ChangesKeyboard Shortcuts Toggle
Widget Dialog Redux Stack Refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant PreviewSettingsPanel
participant KeyboardShortcutsSwitch
participant localStorage
participant previewReducer
participant PreviewConcierge
User->>PreviewSettingsPanel: toggles keyboard shortcuts
PreviewSettingsPanel->>KeyboardShortcutsSwitch: onChange(checked)
KeyboardShortcutsSwitch->>localStorage: setStoredEnabledKeyboardShortcutsState(checked, username)
KeyboardShortcutsSwitch->>previewReducer: setKeyboardShortcutsEnabled({ enabled: checked })
previewReducer-->>previewReducer: state.keyboardShortcutsEnabled = checked
User->>PreviewConcierge: keypress event
PreviewConcierge->>PreviewConcierge: check dialogs and keyboardShortcutsEnabled
alt shortcuts disabled or dialog open
PreviewConcierge-->>User: no shortcut action
else shortcuts enabled
PreviewConcierge-->>User: handle shortcut
end
sequenceDiagram
participant User
participant ToolsPanelEmbeddedAppViewButton
participant WidgetDialogIconButton
participant dialogStack
participant ReduxStore
User->>ToolsPanelEmbeddedAppViewButton: open embedded app
ToolsPanelEmbeddedAppViewButton->>ReduxStore: read dialogStack.byId[widgetDialogId]
alt dialog exists
ToolsPanelEmbeddedAppViewButton->>dialogStack: updateDialogState(...)
else no dialog exists
ToolsPanelEmbeddedAppViewButton->>dialogStack: pushDialog(...)
end
User->>WidgetDialogIconButton: open widget dialog
WidgetDialogIconButton->>ReduxStore: read dialogStack.byId[widget.id]
alt dialog exists
WidgetDialogIconButton->>dialogStack: updateDialogState(...)
else no dialog exists
WidgetDialogIconButton->>dialogStack: pushDialog(...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PR Details ==="
gh pr view 4882 --repo craftercms/studio-ui --json title,body,state,additions,deletions,changedFiles,baseRefName,headRefName,labels,reviewRequests
echo ""
echo "=== Changed Files ==="
gh pr diff 4882 --repo craftercms/studio-ui --name-only
echo ""
echo "=== PR Diff ==="
gh pr diff 4882 --repo craftercms/studio-uiLength of output: 13825
1. Import inconsistency in
|
… improve accessibility with aria-label
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PR Details ==="
gh pr view 4882 --repo craftercms/studio-ui --json title,body,state,additions,deletions,changedFiles,baseRefName,headRefName
echo ""
echo "=== Changed Files ==="
gh pr diff 4882 --repo craftercms/studio-ui --name-only
echo ""
echo "=== Full PR Diff ==="
gh pr diff 4882 --repo craftercms/studio-uiLength of output: 13280
A couple of additional things I noticed while scanning the updated diff that look correct:
The implementation is solid and ready for human review. 🐇 |
rart
left a comment
rart
left a comment
There was a problem hiding this comment.
Apart from this, I think there's a few instances (inputs, inside dialogs) where the key event is reaching the shortcut instead of stopping at the current interaction target. Please double check.
…lEmbeddedAppViewButton and WidgetDialogIconButton to use new dialogs system
|
@coderabbitai I don't see a review of 792b9b9. Please review this PR and report findings |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="craftercms/studio-ui"
pr="4882"
commit="792b9b98ab0d9d1a9554acb36d11db19a4bcc5af"
echo "=== PR metadata ==="
gh pr view "$pr" --repo "$repo" --json title,body,files,commits \
--jq '{title, files: [.files[].path], commits: [.commits[] | {oid, messageHeadline}] }'
echo
echo "=== Commit metadata ==="
gh api "repos/$repo/commits/$commit" --jq '{sha: .sha, files: [.files[] | {filename, status, additions, deletions, changes}]}'
echo
echo "=== Commit patch ==="
gh api "repos/$repo/commits/$commit" --jq '.files[] | "FILE: \(.filename)\n\(.patch // "[no patch]")\n---"'
echo
echo "=== PR diff names ==="
gh pr diff "$pr" --repo "$repo" --name-only
echo
echo "=== Outline: ui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsx ==="
ast-grep outline ui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsx --view expanded || true
echo
echo "=== Outline: ui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsx ==="
ast-grep outline ui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsx --view expanded || true
echo
echo "=== Outline: ui/app/src/state/epics/mappedDialogs.ts ==="
ast-grep outline ui/app/src/state/epics/mappedDialogs.ts --view expanded || true
echo
echo "=== Outline: ui/app/src/state/actions/dialogs.ts ==="
ast-grep outline ui/app/src/state/actions/dialogs.ts --view expanded || true
echo
echo "=== Snippet: ToolsPanelEmbeddedAppViewButton.tsx ==="
sed -n '1,260p' ui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsx
echo
echo "=== Snippet: WidgetDialogIconButton.tsx ==="
sed -n '1,260p' ui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsx
echo
echo "=== Snippet: mappedDialogs.ts ==="
sed -n '1,260p' ui/app/src/state/epics/mappedDialogs.ts
echo
echo "=== Search: generateDialogId, showWidgetDialog, updateWidgetDialog ==="
rg -n -C3 'generateDialogId|showWidgetDialog|updateWidgetDialog' ui/app/src
echo
echo "=== Search: allowMinimizeDialogs ==="
rg -n -C3 'allowMinimizeDialogs' ui/app/srcLength of output: 58880 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Locate WidgetDialog-related files ==="
fd -i 'WidgetDialog*' ui/app/src/components ui/app/src/models ui/app/src/state || true
echo
echo "=== Outline: ui/app/src/components/WidgetDialog/WidgetDialog.tsx ==="
ast-grep outline ui/app/src/components/WidgetDialog/WidgetDialog.tsx --view expanded || true
echo
echo "=== Snippet: ui/app/src/components/WidgetDialog/WidgetDialog.tsx ==="
sed -n '1,260p' ui/app/src/components/WidgetDialog/WidgetDialog.tsx || true
echo
echo "=== Search: WidgetDialogStateProps ==="
rg -n -C3 'WidgetDialogStateProps' ui/app/src
echo
echo "=== Search: craftercms.components.WidgetDialog ==="
rg -n -C3 'craftercms\.components\.WidgetDialog|pushDialog\(|dialogStack' ui/app/src/components ui/app/src/state
echo
echo "=== Search: useWithPendingChangesCloseRequest / useEnhancedDialogState around widget dialog usages ==="
rg -n -C4 'useWithPendingChangesCloseRequest|useEnhancedDialogState|WidgetDialog' ui/app/src/components
echo
echo "=== Search: interface/type EnhancedDialogProps ==="
rg -n -C3 'interface EnhancedDialogProps|type EnhancedDialogProps' ui/app/src/components ui/app/src/models ui/app/src/state
echo
echo "=== Search: state.dialogs shape / open checks ==="
rg -n -C3 'state\.dialogs|dialogs\).+open|dialog\.open' ui/app/srcLength of output: 196238
Finding
Why this matters
Suggested direction
Other than that, the ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ui/app/src/components/KeyboardShortcutsSwitch/KeyboardShortcutsSwitch.tsx (1)
49-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: consumer-supplied
onChangeis silently overridden.Since
{...props}is spread beforeonChange={onChange}, anyonChangepassed by a parent is dropped. The only current caller passes justedge="end", so this isn't a live bug, but composing the handlers would make the component safer against future reuse.♻️ Compose handlers
- const onChange = (e: ChangeEvent<HTMLInputElement>) => { + const onChange = (e: ChangeEvent<HTMLInputElement>) => { setStoredEnabledKeyboardShortcutsState(e.target.checked, username); dispatch(enableKeyboardShortcutsAction({ enabled: e.target.checked })); + props.onChange?.(e, e.target.checked); };🤖 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 `@ui/app/src/components/KeyboardShortcutsSwitch/KeyboardShortcutsSwitch.tsx` around lines 49 - 58, The KeyboardShortcutsSwitch component is overriding any consumer-provided onChange by setting its own handler after spreading props. Update KeyboardShortcutsSwitch so the Switch composes the internal onChange with any onChange from props instead of replacing it, preserving parent behavior while still toggling shortcuts. Use the existing KeyboardShortcutsSwitch and onChange wiring around the Switch element to locate the fix.
🤖 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
`@ui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsx`:
- Around line 36-50: The embedded app launcher is reusing a single dialog id
from generateDialogId(showWidgetDialog.type), so openEmbeddedApp can only ever
target one shared dialog entry. Update the dialog id to include a
launcher/widget-specific key such as widget.uiKey or widget.id, and pass that id
through both showWidgetDialog and updateWidgetDialog. Also make sure the dialog
action types/handlers that consume these actions honor the supplied id so each
widget opens and restores independently.
In `@ui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsx`:
- Around line 40-54: The WidgetDialogIconButton is still using the shared
type-level WIDGET_DIALOG id, which can target the wrong dialog in the Redux
stack. Update openEmbeddedApp to generate a per-widget dialog id from the
widget/launcher instance, then pass that same id through both showWidgetDialog
and updateWidgetDialog so the lookup in dialogStack.byId and the dispatches stay
scoped to the correct widget dialog.
In `@ui/app/src/state/actions/preview.ts`:
- Around line 451-452: The action name in preview.ts is semantically misleading
because enableKeyboardShortcuts implies only turning shortcuts on, while the
payload on createAction<{ enabled: boolean }> supports both states. Rename
enableKeyboardShortcuts to setKeyboardShortcutsEnabled and update any references
to match, keeping the KEYBOARD_SHORTCUTS_TOGGLE action creator behavior
unchanged.
---
Nitpick comments:
In `@ui/app/src/components/KeyboardShortcutsSwitch/KeyboardShortcutsSwitch.tsx`:
- Around line 49-58: The KeyboardShortcutsSwitch component is overriding any
consumer-provided onChange by setting its own handler after spreading props.
Update KeyboardShortcutsSwitch so the Switch composes the internal onChange with
any onChange from props instead of replacing it, preserving parent behavior
while still toggling shortcuts. Use the existing KeyboardShortcutsSwitch and
onChange wiring around the Switch element to locate the fix.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51647a64-0921-4c3f-b807-29480bbd4206
📒 Files selected for processing (12)
ui/app/src/components/AccountManagement/utils.tsxui/app/src/components/KeyboardShortcutsSwitch/KeyboardShortcutsSwitch.tsxui/app/src/components/KeyboardShortcutsSwitch/index.tsui/app/src/components/PreviewConcierge/PreviewConcierge.tsxui/app/src/components/PreviewSettingsPanel/PreviewSettingsPanel.tsxui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsxui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsxui/app/src/models/GlobalState.tsui/app/src/state/actions/preview.tsui/app/src/state/epics/mappedDialogs.tsui/app/src/state/reducers/preview.tsui/app/src/utils/state.ts
… related actions and selectors
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsx (1)
36-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNamespace the widget dialog id before using it as a global stack key.
dialogStack.byIdis shared across dialog types, and the reducer uses the suppliediddirectly. A rawwidget.idcan collide with another explicit dialog id and make Line 39/43 update the wrong dialog entry. Keep the per-widget behavior, but prefix it with theWidgetDialogcomponent id.Suggested change
const title = usePossibleTranslation(props.title); const dispatch = useDispatch(); const store = useStore<GlobalState>(); - const widgetDialogId = widget.id; + const widgetDialogComponentId = createComponentId('WidgetDialog'); + const widgetDialogId = `${widgetDialogComponentId}:${widget.id}`; const openEmbeddedApp = () => { const existing = store.getState().dialogStack.byId[widgetDialogId]; @@ pushDialog({ id: widgetDialogId, - component: createComponentId('WidgetDialog'), + component: widgetDialogComponentId, allowMinimize: true, props: { title, widget } })🤖 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 `@ui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsx` around lines 36 - 59, Namespace the dialog key used by ToolsPanelEmbeddedAppViewButton so it cannot collide with other entries in dialogStack.byId. Update openEmbeddedApp to use a WidgetDialog-scoped id instead of the raw widget.id, and apply that same namespaced value consistently in both the updateDialogState and pushDialog paths so the existing lookup and per-widget behavior still work without affecting other dialog types.ui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsx (1)
40-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNamespace the widget dialog id before storing it in the shared dialog stack.
This uses
widget.iddirectly as a globaldialogStack.byIdkey. Prefix it with theWidgetDialogcomponent id so this launcher cannot accidentally update a different dialog type with the same explicit id.Suggested change
const title = usePossibleTranslation(props.title); const dispatch = useDispatch(); const store = useStore<GlobalState>(); - const widgetDialogId = widget.id; + const widgetDialogComponentId = createComponentId('WidgetDialog'); + const widgetDialogId = `${widgetDialogComponentId}:${widget.id}`; const openEmbeddedApp = () => { const existing = store.getState().dialogStack.byId[widgetDialogId]; @@ pushDialog({ id: widgetDialogId, - component: createComponentId('WidgetDialog'), + component: widgetDialogComponentId, allowMinimize: true, props: { title, widget } })🤖 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 `@ui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsx` around lines 40 - 63, The dialog key is using widget.id directly in the shared dialog stack, which can collide with other dialog types. Update openEmbeddedApp in WidgetDialogIconButton to namespace widgetDialogId with the WidgetDialog component id via createComponentId('WidgetDialog') before reading or writing dialogStack.byId. Keep using that namespaced id consistently in both updateDialogState and pushDialog so this launcher only targets its own dialog instance.
🤖 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.
Nitpick comments:
In
`@ui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsx`:
- Around line 36-59: Namespace the dialog key used by
ToolsPanelEmbeddedAppViewButton so it cannot collide with other entries in
dialogStack.byId. Update openEmbeddedApp to use a WidgetDialog-scoped id instead
of the raw widget.id, and apply that same namespaced value consistently in both
the updateDialogState and pushDialog paths so the existing lookup and per-widget
behavior still work without affecting other dialog types.
In `@ui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsx`:
- Around line 40-63: The dialog key is using widget.id directly in the shared
dialog stack, which can collide with other dialog types. Update openEmbeddedApp
in WidgetDialogIconButton to namespace widgetDialogId with the WidgetDialog
component id via createComponentId('WidgetDialog') before reading or writing
dialogStack.byId. Keep using that namespaced id consistently in both
updateDialogState and pushDialog so this launcher only targets its own dialog
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2e5c814-c17c-427f-8853-67bb71fbe733
📒 Files selected for processing (7)
ui/app/src/components/KeyboardShortcutsSwitch/KeyboardShortcutsSwitch.tsxui/app/src/components/PreviewConcierge/PreviewConcierge.tsxui/app/src/components/ToolsPanelEmbeddedAppViewButton/ToolsPanelEmbeddedAppViewButton.tsxui/app/src/components/WidgetDialogIconButton/WidgetDialogIconButton.tsxui/app/src/models/GlobalState.tsui/app/src/state/actions/preview.tsui/app/src/state/reducers/preview.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/app/src/models/GlobalState.ts
- ui/app/src/components/KeyboardShortcutsSwitch/KeyboardShortcutsSwitch.tsx
- ui/app/src/components/PreviewConcierge/PreviewConcierge.tsx
…o enhancement/8220 # Conflicts: # ui/app/src/utils/state.ts
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Overall, the keyboard shortcuts accessibility feature looks solid:
That said, I would not call this fully ready yet because there’s still one substantive regression risk from commit
the widget dialog handling appears to have shifted from per-launcher/per-instance dialogs to a single shared dialog id. That means opening widget B can update/replace widget A’s dialog instead of allowing them to coexist independently. If that behavior change is intentional, it should be explicitly documented; otherwise I’d recommend fixing it before human review. Bottom line: feature work looks good, but I’d address or explicitly justify the widget-dialog singleton behavior before handoff. ✅ Action performedFull review finished. |
|
@rart what do you think of only one widget dialog of a single component can be open at a time? |
2 participants
craftercms/craftercms#8220
Summary by CodeRabbit