Repository navigation
🤖 fix: fade sideways-scrolling rows, full skill descriptions, keep pop-out terminals out of the sidebar - #5732
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fceed5de26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ThomasK33
force-pushed
the
fix/scroll-row-fade-skill-descriptions
branch
from
October 6, 2026 12:46
fceed5d to
5acf2bb
Compare
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 6, 2026
## Summary In the desktop app, when the shell in one pop-out terminal window exits, only that window closes now. Other pop-out terminal windows of the same workspace stay open. Fixes coder#5739 ## Background `terminal-window.tsx` called `terminal.closeWindow({ workspaceId })` when its shell exited, and `TerminalWindowManager.closeTerminalWindow(workspaceId)` closed every pop-out of that workspace. Since coder#5732, those windows keep their sessions (app-closed windows are not user closes), but they were still hidden, and their shells came back as sidebar tabs on the next load. ## Implementation 1. `terminal.closeWindow` takes an optional `sessionId` (`.nullish()`). The pop-out window sends its own session when its shell exits. 2. A new Electron-free `TerminalWindowRegistry` (`src/desktop/terminalWindowRegistry.ts`) records which session each pop-out window shows, and selects the windows to close: only the one showing `sessionId`, or all of them when the field is absent (unchanged for other callers). 3. `TerminalWindowManager` uses the registry instead of its own `Map<workspaceId, Set<BrowserWindow>>`. I kept the registry free of Electron imports because `terminalWindowManager.ts` cannot load under bun (`electron` has no `BrowserWindow` export there), so the close rule needed a testable home. 4. It also merges a duplicate doc comment on `TerminalService.getWorkspaceSessionIds` left by coder#5732 (flagged by that PR's final check). ## Validation `src/desktop/terminalWindowRegistry.test.ts` "when one of two pop-outs exits, only its window is closed and the other stays open": two pop-outs are open in one workspace and one exits. The test asserts that only that window is selected, and that the other stays registered. With the old "close every window of the workspace" rule restored inside the registry, the test fails: ``` + "window-b", - Expected - 0 + Received + 1 (fail) TerminalWindowRegistry (coder#5739) > when one of two pop-outs exits, only its window is closed and the other stays open ``` No screenshots: the behavior exists only in the Electron app (in browser mode `closeWindow` is a no-op on the server, and each browser pop-out is its own tab). I did not run Electron for this change. Its window wiring is the 1:1 replacement of the old Map/Set shown in the diff. ## Risks Low. Callers that omit `sessionId` keep the old close-all behavior. A pop-out opened without a session (none exist today, because `openTerminalPopout` always passes one) is closed only by a close-all. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$16.86`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high -->
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
1 participant
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.
Summary
Three small layout and terminal fixes from the bug bash:
Fixes #5696
Fixes #5697
Fixes #5673
Implementation
#5696 One shared CSS class,
scroll-fade-x, inscrollbar-none.css(shared with the VS Code webview, because both rows are webview-importable). It is scroll-driven CSS (animation-timeline: scroll(self inline)animating two registered@propertylengths into amask-image), so there is no JS and no per-render work. A row that fits has an inactive scroll timeline and shows no fade. Browsers without scroll timelines keep today's plain row.#5697 The description drops
line-clamp-1and wraps (break-words). The list already scrolls insidemax-h-[min(400px,60vh)]. I chose full text over click-to-expand because expanding would be a new operation that needs its own keyboard shortcut.#5673 (coordinator-approved design, no persisted state):
TerminalRouterProvider popout, so theirterminal.attachcalls sendpopout: true(new optional input field, no new subscription).TerminalServicecounts the session as shown in a pop-out, andterminal.listSessionsskips it. So the right sidebar's reload sync (and layout presets) do not adopt it.TerminalWindowManagerreports a closed pop-out window, andTerminalServicecloses that session. This matches closing a sidebar terminal tab, which also does not ask first. Reloading or crashing a pop-out does not close its window, so the session survives those.Validation
Test-first for #5673. Before the fix:
routerSubscriptions.test.tsadds the stream-level test (reload-adoption and crash path): a pop-out attach hides the session while it is live, and aborting it lists the session again. IfattachTerminalignores the flag, the test fails.vscode/src/webview/webviewCss.test.tspasses (it first caught the class living only in desktop CSS, so the class moved to the shared file).#5696 and #5697 are CSS-only. Asserting class names would be a tautological test, so the evidence is measured in the browser (agent-browser against
tests/bugbash/startApp.tsservers built from main and from this branch):scrollWidth430 >clientWidth375)Footer at 375px: main (top), branch at the start (middle), branch scrolled to the end (bottom):
Timeline chips (row narrowed to 240px, as in a narrow sidebar): main (top), branch (bottom):
Skills popover on main (desktop) and on this branch (375px):
Risks
Low. The fade only paints a mask on two rows and needs scroll-timeline support (Chromium, so Electron and the VS Code webview). Long skill descriptions make the popover list taller, and it scrolls.
listSessionsnow hides sessions that a pop-out shows, so the layout-preset terminal mapping also skips them, which matches the intent. Closing a desktop pop-out now kills its shell. Before, the shell kept running unseen until the sidebar adopted it.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$3.19