Repository navigation
fix(web): right panel shortcuts on non-Latin layouts and terminal close dialog UX #13428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1891337
64db048
11408b5
25bd22b
74856fd
7f92a10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -246,17 +246,33 @@ function tabAudioState(overlay: DesktopPreviewOverlay | null): TabAudioState { | |
| type SurfaceShortcutEvent = Pick< | ||
| KeyboardEvent, | ||
| "altKey" | "ctrlKey" | "defaultPrevented" | "isComposing" | "key" | "metaKey" | ||
| >; | ||
| > & { readonly code?: string }; | ||
|
|
||
| /** | ||
| * Resolves the surface shortcut action matching a keyboard event, checking both | ||
| * the active keyboard layout character and physical key code fallbacks for non-Latin layouts. | ||
| */ | ||
| export function surfaceShortcutActionForKey< | ||
| const Action extends { available: boolean; shortcut: string }, | ||
| >(actions: readonly Action[], event: SurfaceShortcutEvent): Action | null { | ||
| if (event.defaultPrevented || event.isComposing) return null; | ||
| if (event.metaKey || event.ctrlKey || event.altKey) return null; | ||
| const layoutKey = event.key.toLowerCase(); | ||
| const letterCode = event.code?.match(/^Key([A-Z])$/)?.[1]?.toLowerCase(); | ||
| return ( | ||
| actions.find( | ||
| (action) => action.available && action.shortcut.toLowerCase() === event.key.toLowerCase(), | ||
| ) ?? null | ||
| actions.find((action) => { | ||
| if (!action.available) return false; | ||
| const targetShortcut = action.shortcut.toLowerCase(); | ||
| if (targetShortcut === layoutKey) return true; | ||
| if ( | ||
| letterCode && | ||
| layoutKey.length === 1 && | ||
| !/^[a-z]$/.test(layoutKey) && | ||
| targetShortcut === letterCode | ||
| ) | ||
| return true; | ||
| return false; | ||
| }) ?? null | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -314,8 +330,11 @@ function SurfaceMenuItem(props: { | |
| * outside a typing context, and arrows plus Enter work while the launcher is | ||
| * focused. The highlight only appears on hover or arrow use. Unavailable | ||
| * surfaces stay visible with a one-line reason. | ||
| * | ||
| * Empty launcher state rendered inside the right panel when no surface is active. | ||
| */ | ||
| function RightPanelEmptyState(props: { | ||
| open?: boolean | undefined; | ||
| onAddBrowser: () => void; | ||
| onAddBrowserInProfile: (profileId: string) => void; | ||
| browserProfiles: ReadonlyArray<{ readonly id: string; readonly name: string }>; | ||
|
|
@@ -425,11 +444,21 @@ function RightPanelEmptyState(props: { | |
| // is focused; focus moves around too easily (stray clicks) to carry them. | ||
| // Capture phase so app-level key handlers cannot swallow the event first; | ||
| // typing contexts and already-handled events are left alone. | ||
| const isPanelOpen = props.open !== false; | ||
| const containerRef = useRef<HTMLDivElement | null>(null); | ||
| const shortcutActionsRef = useRef(availableActions); | ||
|
|
||
| useEffect(() => { | ||
| shortcutActionsRef.current = availableActions; | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (isPanelOpen) { | ||
| containerRef.current?.focus(); | ||
| } | ||
| }, [isPanelOpen]); | ||
| useEffect(() => { | ||
| if (!isPanelOpen) return; | ||
| const handler = (event: KeyboardEvent) => { | ||
| const action = surfaceShortcutActionForKey(shortcutActionsRef.current, event); | ||
| if (!action) return; | ||
|
|
@@ -442,9 +471,10 @@ function RightPanelEmptyState(props: { | |
| }; | ||
| window.addEventListener("keydown", handler, true); | ||
| return () => window.removeEventListener("keydown", handler, true); | ||
| }, []); | ||
| }, [isPanelOpen]); | ||
|
|
||
| const handleKeyDown = (event: ReactKeyboardEvent<HTMLDivElement>) => { | ||
| if (!isPanelOpen) return; | ||
| if (event.defaultPrevented || event.metaKey || event.ctrlKey || event.altKey) return; | ||
| if (availableActions.length === 0) return; | ||
| if (event.key === "ArrowDown" || event.key === "ArrowRight") { | ||
|
|
@@ -471,12 +501,6 @@ function RightPanelEmptyState(props: { | |
| } | ||
| }; | ||
|
|
||
| // Stable identity so React only runs this callback ref on mount/unmount; | ||
| // an inline arrow would re-attach and re-focus on every render. | ||
| const focusOnMount = useCallback((node: HTMLDivElement | null) => { | ||
| node?.focus(); | ||
| }, []); | ||
|
|
||
| const isHighlighted = (action: SurfaceAction) => | ||
| highlightIndex !== -1 && availableActions[highlightIndex] === action; | ||
|
|
||
|
|
@@ -499,11 +523,13 @@ function RightPanelEmptyState(props: { | |
|
|
||
| return ( | ||
| <div | ||
| ref={focusOnMount} | ||
| ref={containerRef} | ||
| tabIndex={0} | ||
| onKeyDown={handleKeyDown} | ||
| aria-label="Open a surface" | ||
| data-surface-launcher-keys={availableActions.map((action) => action.shortcut).join("")} | ||
| data-surface-launcher-keys={ | ||
| isPanelOpen ? availableActions.map((action) => action.shortcut).join("") : undefined | ||
| } | ||
| className={cn( | ||
| "flex min-h-0 flex-1 items-center justify-center overflow-y-auto px-6 pt-6 outline-none", | ||
| // The panel topbar sits above this container; matching bottom padding | ||
|
|
@@ -1405,6 +1431,7 @@ export function RightPanelTabs(props: RightPanelTabsProps) { | |
| <div className="flex min-h-0 flex-1 flex-col" data-right-panel-surface-content> | ||
| {props.activeSurfaceId === null ? ( | ||
| <RightPanelEmptyState | ||
| open={props.open} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium When the sheet is closed and has no surfaces, 🤖 Copy this AI Prompt to have your agent fix this: |
||
| onAddBrowser={props.onAddBrowser} | ||
| onAddBrowserInProfile={props.onAddBrowserInProfile} | ||
| browserProfiles={browserProfiles} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 High
components/RightPanelTabs.tsx:437Rendering
RightPanelEmptyStatethrows a temporal-dead-zoneReferenceErrorbefore the launcher renders, because the focus effect evaluates its dependency array whileshortcutActionsRefis still uninitialized. Move the ref declaration above that effect.🤖 Copy this AI Prompt to have your agent fix this: