Repository navigation
fix(app): scope tab shortcuts to the focused panel and calm the side tab strip - #57935
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach looks sound: the new tab view state moves into the region, the per-key extensionClaims check means a panel's own binding wins without blocking the host, and the same-site title handling in chromium.ts is reasonable. Two component stories fail, though, so this needs another pass before it's ready.
How I checked: I couldn't exercise the real desktop behaviour (Electron, native page focus) here, so I ran the browser-pane-restore.spec.ts stories in headless Chromium. All 11 pass on the base. On this branch, 11 of 13 pass:
- The existing story "opens a browser tab with ⌘T inside the pane and a session everywhere else" now fails at line 345. The
+menu showsCtrl+Alt+Bwherenoneis expected, because of the new always-onbrowser.openkeybind. - The new "⌘T and ⌘W follow focus" story fails every time (3 of 3 runs) at line 423:
#review-panelnever getsdata-focusedaftereditor.click().
Typecheck passes in app and gui-extensions, and connection.test.ts passes.
Public API: unchanged. Two internal additions: a focused pane IPC event and a default ⌃⌘B / Ctrl+Alt+B binding for browser.open, which users can rebind.
See the inline notes on the failing stories and on the fixture copying the session commands.
| icon: "globe", | ||
| // mod+t opens a browser tab only inside the pane, so advertise it only while the pane is on screen. | ||
| keybind: value.pane()?.visible() ? "browser.newTab" : undefined, | ||
| keybind: "browser.open", |
There was a problem hiding this comment.
The + menu now always shows this binding, so the existing story "opens a browser tab with ⌘T inside the pane and a session everywhere else" in browser-pane-restore.spec.ts fails at line 345. It still expects add-shortcut to be none while a file tab is shown, and it now gets Ctrl+Alt+B. It passes on the base. Please update that story to match the new behaviour.
| }) | ||
|
|
||
| // As the session's commands: ⌘T and ⌘W inside the side region, unless the focused panel binds the key there. | ||
| const sideKey = (event: KeyboardEvent) => |
There was a problem hiding this comment.
This copies the sideKey / file.close / file.tab.new logic from use-session-commands.tsx into the fixture, so the stories test the copy, not the commands the app ships. If the real when or closeTab changes, these stories will still pass. Could the fixture reuse the real command definitions, or could the stories be cut down to the region and focus behaviour they actually exercise?
| // The page's focus, which reaches the app only as the pane's report, counts as the side panel's. | ||
| await outside.focus() | ||
| await expect(panel).not.toHaveAttribute("data-focused") | ||
| await editor.click() |
There was a problem hiding this comment.
This step fails for me every time in headless Chromium (3 of 3 runs): after editor.click(), #review-panel never gets data-focused. My guess is that the parent document's own focusin on the <iframe> (which sits outside #review-panel in the fixture) resets the tracker around the fixture's synthetic focused report. In Electron there's no such parent focusin, so this may only affect the fixture, but the story needs to pass reliably.
ce5ee10 to
20b9146
Compare
…spect reduced motion
1 participant
From Kit's Scrawl of the side panel ("something crazy is happening up here", "if I'm focused here and I hit ⌘T, it takes me to a new session").
The "crazy" tab strip
The page in his browser tab was stuck in a redirect loop: a misrouted dev server answered
/tasks/controls.htmlwith an index page that runslocation.replace("controls.html"), which loads the same address again. Every time a navigation committed, Chromium titled the untitled document after its URL until<title>parsed. So the tab label switched betweenknobsand127.0.0.1:5291/tasks/c…several times a second. Because the tab sizes to its label, the tab and the+jumped sideways each time.chromium.tsnow keeps the shown title through navigations on the same site until the new document names itself or stops loading. A new site still shows its URL until it has a title, the same way its icon resets.Headless repro (the real
createBrowserPagein Electron, a fictionalField notespage that replaces itself every 40 ms):Field notes↔127.0.0.1:…/field/notes.html?…New tab→ URL (first commit) →Field notes, then steadyLeaving for another page still shows that page's title (
Quiet page) in both runs.⌘T / ⌘W follow focus
#browser-panel, not the pane itself, so the browser's scoped ⌘T didn't match and the shell's new session ran.#browser-panelis now focusable (tabIndex=-1), so a click on the pane's surface keeps its shortcuts.extensionClaims), so a scope that binds other keys no longer blocks the host.focusis reported over the pane IPC (focused), and the pane turns it into afocusin. The app's single side-focus tracker, now owned by the region, sees it like any other focus.PaneTabsinto the region (openNewTab/closeNewTab/newTab), so the shortcut and the+button share one owner.Focus indicator
While the keyboard is in the main column, the side panel's selected tab label dims to the strip's muted colour; its fill still marks the selection. With focus in the side panel (including a page) it's full contrast. 150 ms colour fade, no rings.
New tab page shortcuts
Tests
browser-pane-restore.spec.ts: ⌘T/⌘W follow focus (main column → session; side panel surface → new tab view, then close; page focus marks the panel focused), and ⌘T after a click on the pane's loading surface opens a browser tab rather than a session.bun typecheckin app and gui-extensions, targeted unit tests,lint:changed origin/beta.