fix(desktop): restore focus to new workspace modal after alt-tab#3276
fix(desktop): restore focus to new workspace modal after alt-tab#3276kkjcheng wants to merge 2 commits intosuperset-sh:mainfrom
Conversation
The Dialog defaulted to non-modal mode, so Radix did not trap focus. When alt-tabbing away and back, focus landed on the window body instead of returning to the modal. Enabling `modal` on both workspace dialog variants activates Radix's built-in focus trap.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEnabled the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Greptile SummaryThis PR enables Key changes:
Confidence Score: 3/5Not safe to merge as-is — DashboardNewWorkspaceModal will close on alt-tab instead of preserving focus, making the fix worse than the original bug for that variant. The fix is correct and complete for NewWorkspaceModal, but DashboardNewWorkspaceModal is missing the onFocusOutside prevention that is essential to the stated goal. Without it, modal=true causes Radix to dismiss the dialog on alt-tab, which is a regression. One targeted one-liner fix is needed before merge. apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceModal.tsx — needs onFocusOutside={(e) => e.preventDefault()} on DialogContent
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx | Adds modal prop to Dialog to activate Radix focus trap; already has onFocusOutside prevention to keep the dialog open on alt-tab — this file is correct. |
| apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceModal.tsx | Adds modal prop to Dialog but is missing onFocusOutside={(e) => e.preventDefault()}, so alt-tabbing away will close the modal entirely instead of preserving and restoring focus. |
Sequence Diagram
sequenceDiagram
participant User
participant OS
participant RadixDialog as Radix Dialog (modal=true)
participant DialogContent
User->>RadixDialog: Opens modal
RadixDialog->>DialogContent: Traps focus inside FocusScope
User->>OS: Alt-tab away
OS-->>RadixDialog: Window loses focus → onFocusOutside fires
alt NewWorkspaceModal (has onFocusOutside preventDefault)
DialogContent-->>RadixDialog: e.preventDefault() — suppress close
RadixDialog-->>DialogContent: Modal stays open
User->>OS: Alt-tab back
OS-->>RadixDialog: Window regains focus
RadixDialog-->>DialogContent: Focus restored to modal ✅
else DashboardNewWorkspaceModal (missing preventDefault)
DialogContent-->>RadixDialog: Default: onOpenChange(false)
RadixDialog-->>DialogContent: Modal closes ❌
User->>OS: Alt-tab back
OS-->>User: Modal is gone — fix incomplete
end
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceModal.tsx, line 30-33 (link)Missing
onFocusOutsideprevention — modal will close on alt-tabDashboardNewWorkspaceModalsetsmodal(good, fixes focus trap on return), but it is missing theonFocusOutside={(e) => e.preventDefault()}handler thatNewWorkspaceModalalready carries.With
modal={true}, Radix UI's default behavior foronFocusOutsideis to callonOpenChange(false)— i.e., close the dialog — when focus leaves the window scope. This means that when the user alt-tabs away,DashboardNewWorkspaceModalwill close entirely rather than keeping the modal open and restoring focus on return. This is arguably worse than the original bug.NewWorkspaceModalavoids this by suppressing the default:onFocusOutside={(e) => e.preventDefault()}
The same handler should be added to
DashboardNewWorkspaceModal'sDialogContent:
Reviews (1): Last reviewed commit: "fix(desktop): restore focus to new works..." | Re-trigger Greptile
Add onFocusOutside prevention to match NewWorkspaceModal — without it, modal mode causes Radix to dismiss the dialog when focus moves outside on alt-tab, which is a regression.
Applies the same fix as #3276 to both v1 (NewWorkspaceModal) and v2 (DashboardNewWorkspaceModal) Dialogs so alt-tabbing back restores focus to the modal.
…uperset-sh#3392) * fix(desktop): enable modal focus trap on v1 + v2 workspace dialogs Applies the same fix as superset-sh#3276 to both v1 (NewWorkspaceModal) and v2 (DashboardNewWorkspaceModal) Dialogs so alt-tabbing back restores focus to the modal. * Lint
Summary
modal={false}default), so Radix UI did not trap focusmodalon bothNewWorkspaceModalandDashboardNewWorkspaceModalDialog variants to activate Radix's built-in focus trapTest plan
Summary by cubic
Restores focus and keeps the New Workspace dialogs open after alt-tab by enabling
modalon theDialogand preventing focus-outside dismiss inDashboardNewWorkspaceModal.Written for commit bdf35f2. Summary will update on new commits.
Summary by CodeRabbit