fix(desktop): restore immediate Enter action in Close Workspace dialog#1800
Merged
Kitenite merged 6 commits intosuperset-sh:mainfrom Feb 25, 2026
Merged
fix(desktop): restore immediate Enter action in Close Workspace dialog#1800Kitenite merged 6 commits intosuperset-sh:mainfrom
Kitenite merged 6 commits intosuperset-sh:mainfrom
Conversation
Fixes superset-sh#1790 by addressing the full keyboard-focus race when opening the close/delete dialog from workspace context menus. What changed: - Defer dialog open to next macrotask to avoid synchronous open during ContextMenu select. - Prevent ContextMenu close auto-focus from restoring focus to trigger when 'Close Worktree' is selected. - Force dialog open auto-focus to a guaranteed actionable button (Close/Hide) so pressing Enter immediately works. - Keep Hide action available during initial status loading. - Extract pure helpers for scheduler/coordinator/autofocus behavior. Tests: - Add and expand regression tests for: - deferred open ordering and synchrony guard - context-menu close auto-focus prevention and one-shot semantics - dialog open autofocus behavior (including null target) - timer replacement and cleanup semantics for deferred scheduling Result: - Keyboard behavior now works in both collapsed and expanded sidebar modes, including immediate Enter on dialog open.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new timer-based scheduling mechanism with focus coordination has been introduced to defer delete dialog opening until after context menu autofocus completes, enabling proper keyboard focus capture for the dialog's primary action button when the dialog appears. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContextMenu
participant Coordinator as Dialog Coordinator
participant DeleteDialog as Delete Dialog
participant Timer
User->>ContextMenu: Right-click workspace + select Delete
ContextMenu->>Coordinator: requestOpenDeleteDialog()
Coordinator->>Coordinator: Set shouldOpenDeleteDialog flag
ContextMenu->>ContextMenu: Begin close autofocus restoration
ContextMenu->>Coordinator: handleCloseAutoFocus(event)
Coordinator->>Coordinator: preventDefault autofocus
Coordinator->>Timer: scheduleDeleteDialogOpen (defer via setTimeout)
Timer->>DeleteDialog: Call deferred setter after autofocus completes
DeleteDialog->>DeleteDialog: focusPrimaryDialogAction on open
DeleteDialog->>User: Primary action button receives focus
User->>User: Press Enter
User->>DeleteDialog: Primary action activated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/desktop/src/renderer/react-query/workspaces/useWorkspaceDeleteHandler.ts (2)
35-40: Wrapper callback onsetShowDeleteDialogis unnecessary.The wrapper
(show) => { setShowDeleteDialog(show); }is functionally identical to passingsetShowDeleteDialogdirectly. However, this may be intentional if you plan to add side effects later — in which case, consider adding a brief comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/useWorkspaceDeleteHandler.ts` around lines 35 - 40, The wrapper arrow function passed to scheduleDeleteDialogOpen is redundant: replace the object property setShowDeleteDialog: (show) => { setShowDeleteDialog(show); } with the function reference setShowDeleteDialog directly. Update the call to scheduleDeleteDialogOpen({ pendingTimerRef: pendingOpenTimerRef.current, setShowDeleteDialog: setShowDeleteDialog }) (or keep the wrapper but add a comment if you intend future side effects) so the code uses the existing setShowDeleteDialog symbol without needless indirection.
23-25: Double-nested ref adds unnecessary indirection.
useRef<PendingDeleteDialogTimerRef>({ current: null })creates a ref-within-a-ref:pendingOpenTimerRef.current.currentholds the actual timer. SinceReact.MutableRefObjectalready has the same{ current: T }shape asPendingDeleteDialogTimerRef, you can pass the React ref directly:Suggested simplification
-const pendingOpenTimerRef = useRef<PendingDeleteDialogTimerRef>({ - current: null, -}); +const pendingOpenTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);Then pass
pendingOpenTimerRef(not.current) to the scheduler and cleanup functions, since it already satisfiesPendingDeleteDialogTimerRef:useEffect(() => { return () => { - clearPendingDeleteDialogOpen(pendingOpenTimerRef.current); + clearPendingDeleteDialogOpen(pendingOpenTimerRef); }; }, []); scheduleDeleteDialogOpen({ - pendingTimerRef: pendingOpenTimerRef.current, + pendingTimerRef: pendingOpenTimerRef, setShowDeleteDialog: (show) => { setShowDeleteDialog(show); }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/useWorkspaceDeleteHandler.ts` around lines 23 - 25, The ref is double-wrapped so code uses pendingOpenTimerRef.current.current; change the ref declaration to const pendingOpenTimerRef = useRef<number | null>(null) (so the ref holds the timer ID directly) and update all usages to pass pendingOpenTimerRef (not .current) into the scheduler/cleanup functions and to read/write pendingOpenTimerRef.current (remove the extra .current). This touches the pendingOpenTimerRef declaration and any code in the scheduler, cleanup, or handler functions that currently do pendingOpenTimerRef.current.current.apps/desktop/src/renderer/react-query/workspaces/deleteDialogOpenScheduler.ts (1)
29-33: Wrapper callback can be simplified without losing generality.The inner
(show) => { setShowDeleteDialog(show); pendingTimerRef.current = null; }always receivestruefrom the defaultdeferDeleteDialogOpen. While the generic pass-through is harmless and supports injecteddeferOpenoverrides in tests, a cleaner alternative is to wrapsetShowDeleteDialogonce before passing it in, keeping the ref-null logic out of the lambda body:♻️ Optional refactor
- clearPendingDeleteDialogOpen(pendingTimerRef, clearTimer); - pendingTimerRef.current = deferOpen((show) => { - setShowDeleteDialog(show); - pendingTimerRef.current = null; - }); + clearPendingDeleteDialogOpen(pendingTimerRef, clearTimer); + const wrappedSet: typeof setShowDeleteDialog = (show) => { + pendingTimerRef.current = null; + setShowDeleteDialog(show); + }; + pendingTimerRef.current = deferOpen(wrappedSet);This also makes the ref-null happen before the state setter, which matches the natural "timer is done, clear the ref, then dispatch" ordering. Completely optional — current code is correct as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/react-query/workspaces/deleteDialogOpenScheduler.ts` around lines 29 - 33, The lambda passed into deferOpen can be simplified: instead of (show) => { setShowDeleteDialog(show); pendingTimerRef.current = null; } wrap setShowDeleteDialog once and clear the ref before calling it so the ref-nulling happens prior to state update; update the call site that uses pendingTimerRef.current = deferOpen(...) (and related clearPendingDeleteDialog) to pass a function that calls pendingTimerRef.current = null; setShowDeleteDialog(true) (using the known true from deferOpen) — adjust references to clearPendingDeleteDialog, pendingTimerRef, deferOpen, and setShowDeleteDialog accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.test.ts`:
- Line 2: Replace the fragile 7-level relative import of deferDeleteDialogOpen
in DeleteWorkspaceDialog.test.ts with the project path alias defined in tsconfig
(e.g., use the alias that maps to
apps/desktop/src/renderer/react-query/workspaces or similar); locate the import
statement referencing deferDeleteDialogOpen and update it to the corresponding
tsconfig path alias so the test imports deferDeleteDialogOpen via the alias
rather than "../../../../../../../react-query/workspaces/deferDeleteDialogOpen".
---
Nitpick comments:
In
`@apps/desktop/src/renderer/react-query/workspaces/deleteDialogOpenScheduler.ts`:
- Around line 29-33: The lambda passed into deferOpen can be simplified: instead
of (show) => { setShowDeleteDialog(show); pendingTimerRef.current = null; } wrap
setShowDeleteDialog once and clear the ref before calling it so the ref-nulling
happens prior to state update; update the call site that uses
pendingTimerRef.current = deferOpen(...) (and related clearPendingDeleteDialog)
to pass a function that calls pendingTimerRef.current = null;
setShowDeleteDialog(true) (using the known true from deferOpen) — adjust
references to clearPendingDeleteDialog, pendingTimerRef, deferOpen, and
setShowDeleteDialog accordingly.
In
`@apps/desktop/src/renderer/react-query/workspaces/useWorkspaceDeleteHandler.ts`:
- Around line 35-40: The wrapper arrow function passed to
scheduleDeleteDialogOpen is redundant: replace the object property
setShowDeleteDialog: (show) => { setShowDeleteDialog(show); } with the function
reference setShowDeleteDialog directly. Update the call to
scheduleDeleteDialogOpen({ pendingTimerRef: pendingOpenTimerRef.current,
setShowDeleteDialog: setShowDeleteDialog }) (or keep the wrapper but add a
comment if you intend future side effects) so the code uses the existing
setShowDeleteDialog symbol without needless indirection.
- Around line 23-25: The ref is double-wrapped so code uses
pendingOpenTimerRef.current.current; change the ref declaration to const
pendingOpenTimerRef = useRef<number | null>(null) (so the ref holds the timer ID
directly) and update all usages to pass pendingOpenTimerRef (not .current) into
the scheduler/cleanup functions and to read/write pendingOpenTimerRef.current
(remove the extra .current). This touches the pendingOpenTimerRef declaration
and any code in the scheduler, cleanup, or handler functions that currently do
pendingOpenTimerRef.current.current.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src/renderer/react-query/workspaces/deferDeleteDialogOpen.tsapps/desktop/src/renderer/react-query/workspaces/deleteDialogOpenScheduler.test.tsapps/desktop/src/renderer/react-query/workspaces/deleteDialogOpenScheduler.tsapps/desktop/src/renderer/react-query/workspaces/useWorkspaceDeleteHandler.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/focus-primary-dialog-action.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/context-menu-delete-dialog-coordinator.ts
...paceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.test.ts
Outdated
Show resolved
Hide resolved
Collaborator
|
Pushed follow-up fixes to PR head branch.
Local validation:
Waiting on CI checks to run for latest commit a28f5ec. |
# Conflicts: # apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx (1)
58-61:useMemois not semantically guaranteed to preserve values — preferuseReffor mutable state.The coordinator object holds mutable state (
shouldOpenDeleteDialogflag) that is written inonSelectand read shortly after inonCloseAutoFocus. React's docs explicitly stateuseMemois a performance hint, not a semantic guarantee — the cache may be discarded. AuseRefwould be the correct primitive for retaining a mutable object across the render cycle.Suggested refactor
-import { type RefObject, useMemo, useState } from "react"; +import { type RefObject, useRef, useState } from "react";- const deleteDialogCoordinator = useMemo( - () => createContextMenuDeleteDialogCoordinator(onDeleteClick), - [onDeleteClick], - ); + const coordinatorRef = useRef(createContextMenuDeleteDialogCoordinator(onDeleteClick)); + coordinatorRef.current = createContextMenuDeleteDialogCoordinator(onDeleteClick); + const deleteDialogCoordinator = coordinatorRef.current;Or, alternatively, keep the coordinator stable and update only the callback:
- const deleteDialogCoordinator = useMemo( - () => createContextMenuDeleteDialogCoordinator(onDeleteClick), - [onDeleteClick], - ); + const onDeleteClickRef = useRef(onDeleteClick); + onDeleteClickRef.current = onDeleteClick; + const deleteDialogCoordinator = useMemo( + () => createContextMenuDeleteDialogCoordinator(() => onDeleteClickRef.current()), + [], + );The second approach is preferable: it keeps a single coordinator instance alive for the component's lifetime, so the mutable
shouldOpenDeleteDialogflag is never accidentally reset by a re-creation, while always calling the latestonDeleteClick.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx` around lines 58 - 61, Replace the useMemo-based coordinator with a stable ref: create a latestOnDeleteClickRef = useRef(onDeleteClick) and update it in a useEffect whenever onDeleteClick changes, then create deleteDialogCoordinatorRef = useRef(createContextMenuDeleteDialogCoordinator((...args) => latestOnDeleteClickRef.current(...args))) only once (assign if ref.current is undefined) and use deleteDialogCoordinatorRef.current in onSelect and onCloseAutoFocus; this ensures the coordinator instance (createContextMenuDeleteDialogCoordinator) is preserved for the component lifetime while always invoking the latest onDeleteClick callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx`:
- Around line 58-61: Replace the useMemo-based coordinator with a stable ref:
create a latestOnDeleteClickRef = useRef(onDeleteClick) and update it in a
useEffect whenever onDeleteClick changes, then create deleteDialogCoordinatorRef
= useRef(createContextMenuDeleteDialogCoordinator((...args) =>
latestOnDeleteClickRef.current(...args))) only once (assign if ref.current is
undefined) and use deleteDialogCoordinatorRef.current in onSelect and
onCloseAutoFocus; this ensures the coordinator instance
(createContextMenuDeleteDialogCoordinator) is preserved for the component
lifetime while always invoking the latest onDeleteClick callback.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx
Contributor
Author
Thank you! |
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.
Description
Fixes #1790 by addressing the full keyboard-focus race when opening the close/delete dialog from workspace context menus.
What changed:
Result:
Related Issues
Type of Change
Testing
Added and expanded regression tests for:
Commands run:
src/renderer/react-query/workspaces/deleteDialogOpenScheduler.test.ts
apps/desktop/src/renderer/react-query/workspaces/deleteDialogOpenScheduler.ts apps/desktop/src/renderer/react-query/workspaces/deleteDialogOpenScheduler.test.ts apps/
desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx apps/desktop/src/renderer/screens/main/components/
WorkspaceSidebar/WorkspaceListItem/context-menu-delete-dialog-coordinator.ts apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/
components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsx apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/
DeleteWorkspaceDialog/focus-primary-dialog-action.ts apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/
DeleteWorkspaceDialog.test.ts
Manual verification:
Screenshots (if applicable)
N/A (behavioral keyboard/focus fix).
Additional Notes
The original triage test was extended to cover all new helper paths and edge cases introduced by this fix.
Summary by CodeRabbit
Bug Fixes
Improvements