Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 5.3k
fix(web): <mod>-w closes focused right panel tabs instead of entire window#6351
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
4371d95010669283df0ead8a780bfc49686ece0328File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -168,7 +168,7 @@ import { | ||||||||||||||||||||||||||||||||||||
| PaperclipIcon, | ||||||||||||||||||||||||||||||||||||
| WifiOffIcon, | ||||||||||||||||||||||||||||||||||||
| } from "lucide-react"; | ||||||||||||||||||||||||||||||||||||
| import { cn, randomHex } from "~/lib/utils"; | ||||||||||||||||||||||||||||||||||||
| import { cn, isMacPlatform, randomHex } from "~/lib/utils"; | ||||||||||||||||||||||||||||||||||||
| import { COLLAPSED_SIDEBAR_TITLEBAR_INSET_CLASS } from "~/workspaceTitlebar"; | ||||||||||||||||||||||||||||||||||||
| import { stackedThreadToast, toastManager } from "./ui/toast"; | ||||||||||||||||||||||||||||||||||||
| import { decodeProjectScriptKeybindingRule } from "~/lib/projectScriptKeybindings"; | ||||||||||||||||||||||||||||||||||||
| @@ -194,6 +194,7 @@ import { useNewThreadHandler } from "../hooks/useHandleNewThread"; | ||||||||||||||||||||||||||||||||||||
| import { resolveAppModelSelectionForInstance } from "../modelSelection"; | ||||||||||||||||||||||||||||||||||||
| import { getTerminalFocusOwner } from "../lib/terminalFocus"; | ||||||||||||||||||||||||||||||||||||
| import { preventRepeatedTerminalCloseShortcut } from "../lib/terminalCloseShortcut"; | ||||||||||||||||||||||||||||||||||||
| import { isPreviewFocused } from "../lib/previewFocus"; | ||||||||||||||||||||||||||||||||||||
| import { resolveNewDraftStartFromOrigin } from "../lib/chatThreadActions"; | ||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||
| derivePhysicalProjectKey, | ||||||||||||||||||||||||||||||||||||
| @@ -4699,6 +4700,29 @@ function ChatViewContent(props: ChatViewProps) { | ||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| const terminalFocusOwner = getTerminalFocusOwner(); | ||||||||||||||||||||||||||||||||||||
| const modKey = isMacPlatform(navigator.platform) | ||||||||||||||||||||||||||||||||||||
| ? event.metaKey && !event.ctrlKey | ||||||||||||||||||||||||||||||||||||
| : event.ctrlKey && !event.metaKey; | ||||||||||||||||||||||||||||||||||||
| const activeBrowserTabId = | ||||||||||||||||||||||||||||||||||||
| activeRightPanelSurface?.kind === "preview" ? activeRightPanelSurface.resourceId : null; | ||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||
| activeRightPanelSurface && | ||||||||||||||||||||||||||||||||||||
| event.key.toLowerCase() === "w" && | ||||||||||||||||||||||||||||||||||||
| modKey && | ||||||||||||||||||||||||||||||||||||
| !event.altKey && | ||||||||||||||||||||||||||||||||||||
| !event.shiftKey && | ||||||||||||||||||||||||||||||||||||
| isPreviewFocused(activeBrowserTabId) | ||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||
Comment on lines
+4708
to
+4715
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.
Suggest gating the block on the terminal not owning focus (
Suggested change
Posted via Macroscope — UI Consistency | ||||||||||||||||||||||||||||||||||||
| event.preventDefault(); | ||||||||||||||||||||||||||||||||||||
| event.stopPropagation(); | ||||||||||||||||||||||||||||||||||||
| if (!event.repeat) { | ||||||||||||||||||||||||||||||||||||
| document | ||||||||||||||||||||||||||||||||||||
| .querySelector<HTMLElement>("[data-right-panel-surface-content]") | ||||||||||||||||||||||||||||||||||||
| ?.focus({ preventScroll: true }); | ||||||||||||||||||||||||||||||||||||
| closeRightPanelSurface(activeRightPanelSurface); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| if (event.defaultPrevented && terminalFocusOwner === null) { | ||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| @@ -4833,6 +4857,7 @@ function ChatViewContent(props: ChatViewProps) { | ||||||||||||||||||||||||||||||||||||
| activeThreadId, | ||||||||||||||||||||||||||||||||||||
| closeTerminal, | ||||||||||||||||||||||||||||||||||||
| closePanelTerminal, | ||||||||||||||||||||||||||||||||||||
| closeRightPanelSurface, | ||||||||||||||||||||||||||||||||||||
| createNewTerminal, | ||||||||||||||||||||||||||||||||||||
| setTerminalOpen, | ||||||||||||||||||||||||||||||||||||
| runProjectScript, | ||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { afterEach, describe, expect, it } from "vite-plus/test"; | ||
| import { isPreviewFocused } from "./previewFocus"; | ||
| class MockHTMLElement { | ||
| isConnected = true; | ||
| tagName = "WEBVIEW"; | ||
| previewTabId = "mini-player-tab"; | ||
| closest(): null { | ||
| return null; | ||
| } | ||
| getAttribute(name: string): string | null { | ||
| return name === "data-preview-server-tab" ? this.previewTabId : null; | ||
| } | ||
| } | ||
| const originalDocument = globalThis.document; | ||
| const originalHTMLElement = globalThis.HTMLElement; | ||
| afterEach(() => { | ||
| if (originalDocument === undefined) { | ||
| delete (globalThis as { document?: Document }).document; | ||
| } else { | ||
| globalThis.document = originalDocument; | ||
| } | ||
| if (originalHTMLElement === undefined) { | ||
| delete (globalThis as { HTMLElement?: typeof HTMLElement }).HTMLElement; | ||
| } else { | ||
| globalThis.HTMLElement = originalHTMLElement; | ||
| } | ||
| }); | ||
| describe("isPreviewFocused", () => { | ||
| it("only accepts a webview that matches the requested browser tab", () => { | ||
| const webview = new MockHTMLElement(); | ||
| globalThis.HTMLElement = MockHTMLElement as unknown as typeof HTMLElement; | ||
| globalThis.document = { activeElement: webview } as unknown as Document; | ||
| expect(isPreviewFocused("right-panel-tab")).toBe(false); | ||
| expect(isPreviewFocused("mini-player-tab")).toBe(true); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.
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/ChatView.tsx:4677The
Mod+Whandler closes the active right-panel surface only whenisPreviewFocused(activeBrowserTabId)returns true, but that function only recognizes<webview>elements or DOM under[data-preview-panel-mode]. When a terminal, diff, files, pull-request, or agents surface is focused,isPreviewFocusedreturns false, soMod+Wfalls through to Electron's default handler and closes the entire application window instead of the panel tab. Consecutive closes also break: after closing a preview tab, focus is moved to[data-right-panel-surface-content], butisPreviewFocuseddoes not recognize that container either, so the nextMod+Wfalls through again. Consider gating the branch on a broader right-panel focus check rather thanisPreviewFocused.🤖 Copy this AI Prompt to have your agent fix this:
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.
Verified this is already covered.
RightPanelTabsrenders its tab bar, every non-browser surface, and[data-right-panel-surface-content]as children ofPreviewPanelShell(RightPanelTabs.tsx:587-736). That shell sets[data-preview-panel-mode]on its root (PreviewPanelShell.tsx:51-68), soactiveElement.closest("[data-preview-panel-mode]")returns the shell for terminal, diff, files, pull-request, agents, and the post-close focus container.The detached Electron
<webview>is the only special case, and that path is scoped to the active browser tab ID to exclude the mini-player. I also exercisedisPreviewFocused(null)with a focused non-webview descendant of the shell; it returnstrue. No code change is needed here.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.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.