Repository navigation
Remove terminal scrollbar workspace menu - #5072
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes per-workspace terminal scrollbar overrides: localization and Settings UI text updated, Workspace no longer stores/persists per-workspace scrollbar-hidden state, Ghostty now consults only global settings, ContentView/context-menu plumbing and observers were removed, and TabManager no longer fingerprints per-workspace scrollbar state. ChangesTerminal Scroll Bar Setting Removal
Sequence DiagramsequenceDiagram
participant ContentView
participant Workspace
participant GhosttySurfaceScrollView
participant SessionManager
ContentView->>Workspace: stops reading/writing terminalScrollBarHidden (context-menu removed)
ContentView->>GhosttySurfaceScrollView: render without per-workspace scrollbar flag
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: terminalScrollBarAllowedBySettings() checks GhosttyApp.shared.scrollbarVisibility() and TerminalScrollBarSettings.isVisible()
SessionManager->>Workspace: build session snapshot without terminalScrollBarHidden
SessionManager->>GhosttySurfaceScrollView: restore UI without per-workspace scrollbar state
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (15 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 84090a7192
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| Menu(String(localized: "contextMenu.workspaceColor", defaultValue: "Workspace Color")) { |
There was a problem hiding this comment.
When the Workspace Settings menu is removed, users who previously hid the scrollbar for a workspace can no longer unhide it: Workspace still persists/restores terminalScrollBarHidden, and GhosttyTerminalView.terminalScrollBarAllowedBySettings() still returns false when that flag is true, regardless of the global Settings toggle. In that state, turning “Show Terminal Scroll Bar” back on in Settings does not restore the scrollbar for affected/restored workspaces, so this removal needs either a migration/reset path or the per-workspace flag should stop being applied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commits: the sidebar context menu no longer exposes the per-workspace control, terminal rendering no longer reads the per-workspace flag, and session snapshots no longer write or restore that flag. Existing persisted hidden flags are ignored so the global Settings toggle controls the scrollbar again.
— Claude Code
There was a problem hiding this comment.
Verified again on the rebased head a79b543: the removed context-menu path is gone, GhosttyTerminalView no longer reads Workspace.terminalScrollBarHidden, Workspace session snapshots no longer write or restore terminalScrollBarHidden, and the autosave fingerprint no longer includes it. Existing persisted hidden flags are ignored, so the global Settings toggle controls scrollbar visibility.
— Claude Code
Greptile SummaryThis PR removes the per-workspace terminal scroll bar override — the "Workspace Settings" context menu, its toggle action, the observer/notification chain that propagated the setting to
Confidence Score: 5/5Safe to merge — this is a straightforward feature removal with no new logic introduced, and every changed code path is simpler than what it replaces. All changes are deletions or simplifications: the observer chain, context menu, snapshot fields, and hasher entry are cleanly removed. Localization is updated in both supported locales. The retained terminalScrollBarHidden property is correctly documented as a no-op kept for test compatibility. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User opens terminal] --> B[terminalScrollBarAllowedBySettings]
B --> C{scrollbarVisibility == .never?}
C -- yes --> D[Hide scroll bar]
C -- no --> E{TerminalScrollBarSettings.isVisible?}
E -- no --> D
E -- yes --> F[Show scroll bar]
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This branch was successfully deployed
1 participant
Removes the Workspace Settings submenu from the workspace context menu so the terminal scroll bar is controlled from Settings instead. Also updates the Settings subtitle to avoid advertising a per-workspace menu action that no longer exists.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
UI and copy cleanup around a deprecated per-workspace preference; global terminal settings still govern scroll bars.
Overview
Removes per-workspace terminal scroll bar control from the sidebar: the Workspace Settings context submenu and its toggle are gone, along with the sidebar state, notifications, and
GhosttyTerminalViewlogic that reacted to workspace overrides.Scroll bar visibility is now driven only by global Settings (
terminal.showScrollBar/TerminalScrollBarSettings); the on-state subtitle no longer mentions disabling it per workspace.Workspace.terminalScrollBarHiddenstays in memory for legacy helpers/tests but is no longer persisted, observed in the sidebar, or applied when rendering.Reviewed by Cursor Bugbot for commit a79b543. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Removed the Workspace Settings submenu and the per-workspace terminal scroll bar override. Scroll bar visibility now follows only Settings; legacy per-workspace state remains in-memory for compatibility but is ignored.
ContentViewandGhosttyTerminalView.terminalScrollBarHidden; kept the property as a documented no-op and removed sidebar observation.TerminalSection.swiftandcmuxApp.swift.Written for commit a79b543. Summary will update on new commits.
Summary by CodeRabbit