Skip to content

feat(browser): hide panel on last tab close, Cmd+T new tab#65

Open
IPJT wants to merge 3 commits intodebuglebowski:mainfrom
IPJT:feat/browser-tab-shortcuts
Open

feat(browser): hide panel on last tab close, Cmd+T new tab#65
IPJT wants to merge 3 commits intodebuglebowski:mainfrom
IPJT:feat/browser-tab-shortcuts

Conversation

@IPJT
Copy link
Copy Markdown

@IPJT IPJT commented Apr 10, 2026

Summary

  • Closing the last browser tab now hides the browser panel. Previously, closing the last tab would silently create a new blank tab, making it impossible to dismiss the panel by closing tabs. Now the panel hides — both via the close button (mouse) and via Cmd+W. Reopening the panel later creates a fresh blank tab automatically.
  • Cmd+T creates a new browser tab when the browser panel is focused. Previously Cmd+T always toggled the terminal panel regardless of context. Now it behaves like a standard browser: if the browser panel has focus, Cmd+T opens a new tab and focuses the URL bar. When any other panel is focused, Cmd+T toggles the terminal as before.

Video

Browser_tabs.1.mp4

Test plan

  • Open browser panel, open multiple tabs, close them one by one — panel should hide when the last tab is closed
  • Repeat using Cmd+W instead of the mouse close button — same behavior
  • After the panel hides, reopen it (Cmd+B) — should show a fresh blank tab
  • Focus the browser panel, press Cmd+T — should create a new browser tab and focus the URL bar
  • Focus the terminal panel, press Cmd+T — should toggle the terminal panel as before
  • Verify Cmd+W on the last tab does not close the task tab (it hides the browser panel instead)

🤖 Generated with Claude Code

Greptile Summary

This PR implements two UX improvements: closing the last browser tab now hides the browser panel (instead of silently creating a new blank tab), and Cmd+T creates a browser tab when the browser panel is focused rather than always toggling the terminal. The four issues flagged in prior review rounds (stale handlePanelToggle closure, unconditional e.preventDefault(), unused newTabUrl dep, DB persistence bypass) are all addressed in this revision via the handlePanelToggleRef pattern and conditional preventDefault placement.

Confidence Score: 5/5

Safe to merge — all previously flagged P1 issues are resolved and remaining findings are minor style improvements.

The four P1 concerns from prior review rounds (stale closure, unconditional preventDefault, unused dep, DB persistence bypass) are all addressed. The three remaining comments are P2: a latent stale-read risk that is safe under current React 18 batching, an inconsistency between the two Cmd+T guards in an edge-case native-menu code path, and an unstable inline callback reference. None block correctness.

packages/domains/task/src/client/TaskDetailPage.tsx — minor browserTabsRef and callback stability suggestions; packages/domains/task-terminals/src/client/TerminalContainer.tsx — native-menu edge case for the activeElement guard.

Important Files Changed

Filename Overview
packages/domains/task/src/client/TaskDetailPage.tsx Adds handlePanelToggleRef to fix stale-closure issues in the Cmd+W handler, new-tab logic on browser reopen, and Cmd+T browser-tab creation; browserTabs is captured in handlePanelToggle without being in its dep array (safe due to batching, but fragile).
packages/domains/task-browser/src/client/BrowserPanel.tsx closeTab now calls onRequestHide when the last tab is closed and clears tabs state; dependency array correctly updated from newTabUrl to onRequestHide.
packages/domains/task-terminals/src/client/TerminalContainer.tsx Adds a document.activeElement guard to skip creating a terminal group when the browser panel is focused; uses a different mechanism than the sticky lastFocusedPanelRef used in TaskDetailPage.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/domains/task/src/client/TaskDetailPage.tsx
Line: 950-952

Comment:
**`browserTabs` not in `handlePanelToggle`'s dep array**

`browserTabs` is read at line 950 but is absent from `handlePanelToggle`'s dep array (`[task, panelVisibility, onTaskUpdated, resetPanelSize]`). In practice React 18 batching ensures the closure is refreshed (closing the last tab and hiding the panel are batched into a single render that also recreates this callback), but the omission leaves a latent stale-read risk if the call graph ever changes. A `setBrowserTabs` functional update or a `browserTabsRef` (already present on line 763) would remove the dependency entirely.

```suggestion
        if (browserTabsRef.current.tabs.length === 0) {
          const newTab = { id: `tab-${crypto.randomUUID().slice(0, 8)}`, url: 'about:blank', title: 'New Tab' }
          setBrowserTabs({ tabs: [newTab], activeTabId: newTab.id })
        }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/domains/task-terminals/src/client/TerminalContainer.tsx
Line: 151-152

Comment:
**`document.activeElement` guard vs. the sticky `lastFocusedPanelRef` in `TaskDetailPage`**

`TaskDetailPage` notes (line 766) that *"macOS native menu accelerators clear activeElement by callback time"* — that's why it uses a sticky `lastFocusedPanelRef`. This guard uses `document.activeElement` instead, so when Cmd+T arrives via the native menu bar (not a raw key press), `activeElement` may already be null and the guard will not fire. In that edge case `TerminalContainer` would proceed to call `createTab()` while `TaskDetailPage` simultaneously creates a browser tab, resulting in both actions running at once. For a keyboard-triggered shortcut this is fine; it's only a risk for the native-menu path. Consider passing `lastFocusedPanelRef` (or a derived `isBrowserFocused` prop/store value) down to `TerminalContainer` so both handlers use the same source of truth.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/domains/task/src/client/TaskDetailPage.tsx
Line: 2013

Comment:
**Inline arrow for `onRequestHide` recreates on every render**

`() => handlePanelToggle('browser', false)` allocates a new function reference on every parent render. `BrowserPanel.closeTab` lists `onRequestHide` in its `useCallback` deps, so it is also recreated on every render of `TaskDetailPage`. Wrapping with `useCallback` (or reusing `handlePanelToggle` directly since the panel ID is fixed) would keep the reference stable.

Or hoist it as a named callback near `handlePanelToggle`.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (3): Last reviewed commit: "fix: persist panel visibility to DB on C..." | Re-trigger Greptile

…r tab

Previously, closing the last browser tab would replace it with a new blank tab,
keeping the panel visible indefinitely. Now the browser panel hides when the last
tab is closed (via mouse or Cmd+W). Reopening the panel creates a fresh tab.

Cmd+T is now context-aware: when the browser panel is focused it opens a new
browser tab instead of toggling the terminal panel.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use setPanelVisibility directly in Cmd+W handler to avoid stale
  handlePanelToggle closure
- Move e.preventDefault() into each Cmd+T branch so the keystroke
  isn't swallowed when both features are disabled
- Remove unused newTabUrl from closeTab dependency array

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 10, 2026

Tip:

Greploop — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.

Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Use a handlePanelToggleRef so the onCloseCurrent effect can call
handlePanelToggle (which persists to DB) without a stale closure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@IPJT
Copy link
Copy Markdown
Author

IPJT commented Apr 10, 2026

@debuglebowski - Opiniated change but feels way more intuitive to me. 🙏🤗

@debuglebowski
Copy link
Copy Markdown
Owner

Great efforts, I like it! I'm doing some deep thinking into how this should work. I'm thinking that we should change the terminal's panel shortcut, but I'm not sure to what yet.

@IPJT
Copy link
Copy Markdown
Author

IPJT commented Apr 13, 2026

Great efforts, I like it! I'm doing some deep thinking into how this should work. I'm thinking that we should change the terminal's panel shortcut, but I'm not sure to what yet.

Does it need to change though? I think focus aware shortcuts is totally fine.
If, the browser is in focus cmd+T till open a new tab. If not the terminal will toggle.

Just like with how cmd+W works today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants