Add sidebar sections with persistence and auto-apply - #2642
rodchristiansen wants to merge 12 commits into
Conversation
Users can now organize workspace tabs into named, collapsible sections in the sidebar. Sections are opt-in — the sidebar behaves identically when no sections exist. - New SidebarSection model with name, collapse state, and ordered workspace membership - Section CRUD in TabManager (create, rename, delete, reorder) - Context menu: "Move to Section" submenu on workspace tabs with "New Section..." option - SidebarSectionHeaderView with disclosure chevron, inline rename, context menu, accessibility labels, and drop target - Drag workspaces onto section headers to assign them - Section state persists across sessions via SessionPersistence - Pinned workspaces always render at top regardless of section - Backward-compatible: old session snapshots restore cleanly
The new file needs to be registered in the .pbxproj so Xcode can find the SidebarSection and SidebarLayout types.
- Forward section objectWillChange to TabManager via sectionRevision counter so sidebar re-renders immediately on any section mutation - Use @focusstate on rename TextField with delayed auto-focus so cursor lands in the text field instead of the terminal - Guard onTapGesture during rename editing to prevent conflicts - Add toggleCollapsed()/setCollapsed() helpers that bump revision - Pipe Combine observers from each section to TabManager on didSet
|
@rodchristiansen is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
📝 WalkthroughWalkthroughThis pull request introduces sidebar sections functionality and auto-apply workspace targeting for the GhosttyTabs application. It adds collapsible section grouping for workspaces in the sidebar, workspace ID persistence for session restoration, and conditional command auto-execution based on workspace target configuration. The changes span tab management, session persistence, cmux configuration, and UI rendering. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TabMgr as TabManager
participant CmuxStore as CmuxConfigStore
participant CmuxExec as CmuxConfigExecutor
participant Workspace
User->>TabMgr: Switch Workspace (selectedTabId)
TabMgr->>CmuxStore: Observe workspace change (0.15s debounce)
activate CmuxStore
CmuxStore->>CmuxStore: checkAutoApply()
alt Command autoApply = true && target = .current && not previously applied
CmuxStore->>CmuxStore: Mark workspace as auto-applied
CmuxStore->>CmuxExec: execute(command, workspace, .current)
activate CmuxExec
CmuxExec->>Workspace: Update workspace in-place<br/>(title, color, layout)
CmuxExec-->>CmuxStore: Done
deactivate CmuxExec
else Skip (already applied or condition not met)
CmuxStore->>CmuxStore: No-op
end
deactivate CmuxStore
TabMgr->>User: Display updated workspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds persistent sidebar sections that survive app restart, Confidence Score: 3/5Not safe to merge — a live Apple app-specific password is committed in scripts/build-local-release.sh. The P0 credential exposure in build-local-release.sh is a hard blocker; all other findings are P2 style/quality issues that do not affect runtime correctness. scripts/build-local-release.sh — credentials must be rotated and moved to environment variables before this script can be shipped. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Tab Switch / Config Load] --> B{checkAutoApply}
B --> C{workspace.id in<br/>autoAppliedWorkspaceIds?}
C -- yes --> D[Skip]
C -- no --> E{first command with<br/>autoApply=true AND<br/>target=current?}
E -- none --> D
E -- found --> F[Insert workspace.id<br/>into autoAppliedSet]
F --> G[CmuxConfigExecutor.execute]
G --> H[Close all panels<br/>except focused]
H --> I[applyCustomLayout]
J[App Restore] --> K[Decode sections JSON]
K --> L[Filter workspaceIds<br/>to restored IDs]
L --> M[TabManager.sections =<br/>restoredSections]
Reviews (1): Last reviewed commit: "AutoApply: track per-session, fire once ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| SIGN_IDENTITY="Developer ID Application: Emily Carr University of Art and Design (7TF6CSP83S)" | ||
| SIGN_HASH="C0277EBA633F1AA2BC2855E45B3B38A1840053BA" | ||
| TEAM_ID="7TF6CSP83S" | ||
| APPLE_ID="applenotarization@ecuad.ca" | ||
| APPLE_PASSWORD="zdtm-jyob-rhbb-cbfq" |
There was a problem hiding this comment.
Lines 24–28 embed a live Apple app-specific password (zdtm-jyob-rhbb-cbfq), signing identity, team ID, and Apple ID directly in the script. Anyone with repo read access can use this password to submit notarization jobs under the developer's Apple account — app-specific passwords cannot be scoped read-only. These credentials must be rotated immediately and the script rewritten to read them from environment variables.
| SIGN_IDENTITY="Developer ID Application: Emily Carr University of Art and Design (7TF6CSP83S)" | |
| SIGN_HASH="C0277EBA633F1AA2BC2855E45B3B38A1840053BA" | |
| TEAM_ID="7TF6CSP83S" | |
| APPLE_ID="applenotarization@ecuad.ca" | |
| APPLE_PASSWORD="zdtm-jyob-rhbb-cbfq" | |
| SIGN_IDENTITY="${CMUX_SIGN_IDENTITY:-}" | |
| SIGN_HASH="${CMUX_SIGN_HASH:-}" | |
| TEAM_ID="${CMUX_TEAM_ID:-}" | |
| APPLE_ID="${CMUX_APPLE_ID:-}" | |
| APPLE_PASSWORD="${CMUX_APPLE_PASSWORD:-}" |
| } | ||
| .disabled(targetIds.isEmpty) | ||
|
|
||
| if !tabManager.sections.isEmpty || true { |
There was a problem hiding this comment.
|| true makes condition unconditional
The expression !tabManager.sections.isEmpty || true always evaluates to true, making the first clause dead code. If the intent is to always show the "Move to Section" menu so users can create sections from the context menu even before any sections exist, remove the if wrapper entirely rather than leaving a forever-true condition that reads like a debugging artifact.
| guard let command = loadedCommands.first(where: { | ||
| $0.autoApply == true && $0.workspace?.target == .current | ||
| }) else { return } |
There was a problem hiding this comment.
autoApply command fires; others are silently dropped
checkAutoApply uses .first(where:), so if a config defines multiple commands with autoApply: true and target: "current", only the first one executes. If "at most one" is intentional, a comment here would prevent future confusion; if all matching commands should run, this should iterate over loadedCommands.filter { ... } instead.
There was a problem hiding this comment.
Pull request overview
This PR adds user-defined sidebar sections that persist across restarts, extends session persistence to restore stable workspace/section identities, and introduces autoApply / target: "current" behavior for cmux workspace commands.
Changes:
- Persist and restore sidebar sections (create/rename/reorder/collapse + workspace membership) via session snapshots.
- Preserve workspace UUIDs across app restarts by persisting/restoring workspace IDs.
- Add cmux command support for
autoApplyandworkspace.target(including applying layouts to the currently selected workspace).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/Workspace.swift | Persist workspace IDs into session snapshots and allow restoring a workspace with a previous UUID. |
| Sources/TabManager.swift | Adds section state, sidebar layout computation, autosave hashing of sections, and section restore/cleanup hooks. |
| Sources/SidebarSection.swift | New model for collapsible sidebar sections + layout container types. |
| Sources/SessionPersistence.swift | Extends session schema to include workspace IDs and sidebar section snapshots. |
| Sources/ContentView.swift | Renders pinned/ungrouped/sectioned workspaces, adds section header UI, and adds “Move to Section” context menu. |
| Sources/CmuxConfigExecutor.swift | Implements workspace.target: current behavior when executing workspace commands. |
| Sources/CmuxConfig.swift | Adds autoApply and CmuxWorkspaceTarget, plus per-session tracking to auto-apply commands on workspace switch. |
| scripts/build-local-release.sh | Adds a local build/sign/notarize/install helper script. |
| GhosttyTabs.xcodeproj/project.pbxproj | Registers SidebarSection.swift in the Xcode project build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| APPLE_ID="applenotarization@ecuad.ca" | ||
| APPLE_PASSWORD="zdtm-jyob-rhbb-cbfq" |
There was a problem hiding this comment.
This script hard-codes Apple notarization credentials (Apple ID and app-specific password). Please remove these secrets from the repository immediately, rotate the leaked password, and load credentials via environment variables / Keychain / xcrun notarytool store-credentials instead.
| APPLE_ID="applenotarization@ecuad.ca" | |
| APPLE_PASSWORD="zdtm-jyob-rhbb-cbfq" | |
| # Load notarization credentials from the environment instead of hard-coding them. | |
| # Prefer a Keychain-backed profile created with: | |
| # xcrun notarytool store-credentials <profile-name> --apple-id "$APPLE_ID" --team-id "$TEAM_ID" --password "$APPLE_PASSWORD" | |
| # and then pass/use NOTARYTOOL_PROFILE in the notarization step if supported there. | |
| APPLE_ID="${APPLE_ID:?Set APPLE_ID in the environment or use a notarytool Keychain profile.}" | |
| APPLE_PASSWORD="${APPLE_PASSWORD:?Set APPLE_PASSWORD in the environment or use a notarytool Keychain profile.}" |
| let keep = current.focusedPanelId | ||
| for panelId in current.panels.keys where panelId != keep { | ||
| current.closePanel(panelId, force: true) |
There was a problem hiding this comment.
focusedPanelId is optional; if it is nil here, the loop will close all panels because panelId != keep is always true, potentially leaving the workspace empty before applying the layout. Please guard against nil (e.g., fall back to a known panel to keep, or early-return when there is no focused panel).
| let keep = current.focusedPanelId | |
| for panelId in current.panels.keys where panelId != keep { | |
| current.closePanel(panelId, force: true) | |
| if let keep = current.focusedPanelId ?? current.panels.keys.first { | |
| for panelId in current.panels.keys where panelId != keep { | |
| current.closePanel(panelId, force: true) | |
| } |
| if !tabManager.sections.isEmpty || true { | ||
| let sections = tabManager.sections | ||
| let currentSection = tabManager.sectionForWorkspace(tab.id) | ||
| Menu(String(localized: "contextMenu.moveToSection", defaultValue: "Move to Section")) { | ||
| Button(String(localized: "contextMenu.noSection", defaultValue: "No Section")) { | ||
| for id in targetIds { | ||
| tabManager.removeWorkspaceFromSection(tabId: id) | ||
| } | ||
| } | ||
| .disabled(currentSection == nil) | ||
|
|
||
| if !sections.isEmpty { | ||
| Divider() | ||
| } | ||
|
|
||
| ForEach(sections, id: \.id) { section in | ||
| Button(section.name) { | ||
| for id in targetIds { | ||
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | ||
| } | ||
| } | ||
| .disabled(currentSection?.id == section.id) | ||
| } | ||
|
|
||
| Divider() | ||
|
|
||
| Button(String(localized: "contextMenu.newSection", defaultValue: "New Section…")) { | ||
| let section = tabManager.createSection( | ||
| name: String(localized: "sidebar.newSectionDefaultName", defaultValue: "New Section") | ||
| ) | ||
| for id in targetIds { | ||
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This condition is always true because of || true, so the “Move to Section” menu will always render regardless of whether sections exist. If this was a debug shim, please remove || true; if it’s intended to always show, drop the surrounding if entirely to avoid dead logic.
| if !tabManager.sections.isEmpty || true { | |
| let sections = tabManager.sections | |
| let currentSection = tabManager.sectionForWorkspace(tab.id) | |
| Menu(String(localized: "contextMenu.moveToSection", defaultValue: "Move to Section")) { | |
| Button(String(localized: "contextMenu.noSection", defaultValue: "No Section")) { | |
| for id in targetIds { | |
| tabManager.removeWorkspaceFromSection(tabId: id) | |
| } | |
| } | |
| .disabled(currentSection == nil) | |
| if !sections.isEmpty { | |
| Divider() | |
| } | |
| ForEach(sections, id: \.id) { section in | |
| Button(section.name) { | |
| for id in targetIds { | |
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | |
| } | |
| } | |
| .disabled(currentSection?.id == section.id) | |
| } | |
| Divider() | |
| Button(String(localized: "contextMenu.newSection", defaultValue: "New Section…")) { | |
| let section = tabManager.createSection( | |
| name: String(localized: "sidebar.newSectionDefaultName", defaultValue: "New Section") | |
| ) | |
| for id in targetIds { | |
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | |
| } | |
| } | |
| } | |
| let sections = tabManager.sections | |
| let currentSection = tabManager.sectionForWorkspace(tab.id) | |
| Menu(String(localized: "contextMenu.moveToSection", defaultValue: "Move to Section")) { | |
| Button(String(localized: "contextMenu.noSection", defaultValue: "No Section")) { | |
| for id in targetIds { | |
| tabManager.removeWorkspaceFromSection(tabId: id) | |
| } | |
| } | |
| .disabled(currentSection == nil) | |
| if !sections.isEmpty { | |
| Divider() | |
| } | |
| ForEach(sections, id: \.id) { section in | |
| Button(section.name) { | |
| for id in targetIds { | |
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | |
| } | |
| } | |
| .disabled(currentSection?.id == section.id) | |
| } | |
| Divider() | |
| Button(String(localized: "contextMenu.newSection", defaultValue: "New Section…")) { | |
| let section = tabManager.createSection( | |
| name: String(localized: "sidebar.newSectionDefaultName", defaultValue: "New Section") | |
| ) | |
| for id in targetIds { | |
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | |
| } | |
| } |
| /// Monotonically increasing revision counter. Bumped on every mutation so | ||
| /// parent views that read this value (via the TabManager computed layout) | ||
| /// re-evaluate even when the `sections` array identity hasn't changed. | ||
| @Published var revision: UInt64 = 0 | ||
|
|
||
| init(id: UUID = UUID(), name: String, isCollapsed: Bool = false, workspaceIds: [UUID] = []) { | ||
| self.id = id | ||
| self.name = name | ||
| self.isCollapsed = isCollapsed | ||
| self.workspaceIds = workspaceIds | ||
| } | ||
|
|
||
| private func bumpRevision() { | ||
| revision &+= 1 | ||
| } |
There was a problem hiding this comment.
SidebarSection.revision (and bumpRevision()) appears unused in the codebase, and each mutation already changes @Published properties (workspaceIds, isCollapsed, etc.) which will emit objectWillChange. Consider removing this extra revision counter (or wiring it into layout invalidation) to reduce redundant state and notifications.
| private func rebindSectionObservers() { | ||
| sectionObserverCancellables.removeAll() | ||
| for section in sections { | ||
| section.objectWillChange | ||
| .receive(on: RunLoop.main) | ||
| .sink { [weak self] _ in | ||
| self?.sectionRevision &+= 1 | ||
| } | ||
| .store(in: §ionObserverCancellables) | ||
| } | ||
| } | ||
|
|
||
| private func notifySectionChange() { | ||
| sectionRevision &+= 1 | ||
| } | ||
|
|
||
| @discardableResult | ||
| func createSection(name: String) -> SidebarSection { | ||
| let section = SidebarSection(name: name) | ||
| sections.append(section) | ||
| // Delay so SwiftUI renders the new SidebarSectionHeaderView | ||
| // (and subscribes to $pendingRenameSectionId) before we emit. | ||
| DispatchQueue.main.async { [weak self] in | ||
| self?.pendingRenameSectionId = section.id | ||
| } | ||
| return section | ||
| } | ||
|
|
||
| func renameSection(sectionId: UUID, name: String) { | ||
| guard let section = sections.first(where: { $0.id == sectionId }) else { return } | ||
| section.name = name | ||
| notifySectionChange() | ||
| } |
There was a problem hiding this comment.
Section change invalidation is currently duplicated: rebindSectionObservers() bumps sectionRevision on every section.objectWillChange, but methods like renameSection also call notifySectionChange() after mutating a @Published section property. This can cause multiple revision bumps / extra sidebar recomputations per user action. Consider relying on one mechanism (observer-driven or explicit notify), not both.
| /// If the selected workspace hasn't been auto-applied this session and a | ||
| /// loaded command has `autoApply: true` with `target: "current"`, execute | ||
| /// it automatically. Tracks applied workspaces so it only fires once per | ||
| /// workspace per app session. | ||
| private func checkAutoApply() { | ||
| guard let tabManager = trackedTabManager, | ||
| let workspace = tabManager.selectedWorkspace, | ||
| !autoAppliedWorkspaceIds.contains(workspace.id), | ||
| let baseCwd = localConfigPath.map({ ($0 as NSString).deletingLastPathComponent }) | ||
| else { return } | ||
|
|
||
| guard let command = loadedCommands.first(where: { | ||
| $0.autoApply == true && $0.workspace?.target == .current | ||
| }) else { return } | ||
|
|
||
| autoAppliedWorkspaceIds.insert(workspace.id) | ||
| CmuxConfigExecutor.execute( | ||
| command: command, | ||
| tabManager: tabManager, | ||
| baseCwd: baseCwd, | ||
| configSourcePath: commandSourcePaths[command.id], | ||
| globalConfigPath: globalConfigPath | ||
| ) | ||
| } |
There was a problem hiding this comment.
checkAutoApply() can execute workspace commands automatically on tab switch based on a local cmux.json, but it does not apply any trust/confirmation gating (unlike confirm: true shell commands). Since this now runs without explicit user action, consider requiring the config source path to be trusted (or limiting autoApply to global config / trusted directories) to avoid unexpected behavior from untrusted repos.
| let tabIndexById = Dictionary(uniqueKeysWithValues: allOrdered.enumerated().map { | ||
| ($0.element.id, $0.offset) | ||
| }) | ||
| let orderedSelectedTabs = tabs.filter { selectedTabIds.contains($0.id) } |
There was a problem hiding this comment.
tabIndexById is now derived from sidebarLayout.allWorkspacesInOrder, but TabItemView logic (e.g. Move Up/Down via moveBy, Shift-range selection, and other index-based operations) still indexes into tabManager.tabs. When sections are present, sidebar order can diverge from tabs, so these actions will operate on the wrong indices/workspaces. Please make sure the index passed to TabItemView matches the ordering used by those mutations (or update the mutation/selection logic to use the sidebar layout order consistently).
| let orderedSelectedTabs = tabs.filter { selectedTabIds.contains($0.id) } | |
| let orderedSelectedTabs = allOrdered.filter { selectedTabIds.contains($0.id) } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
scripts/build-local-release.sh (1)
47-51: Use the canonical GhosttyKit preparation script to avoid stale framework state.The current ad-hoc checks only validate directory presence and a SHA stamp. This can drift from the repo’s canonical GhosttyKit validation/caching behavior and lead to inconsistent local release artifacts.
Proposed refactor
- # Re-download GhosttyKit if submodule changed - GHOSTTY_SHA="$(git -C ghostty rev-parse HEAD)" - if [ ! -d "GhosttyKit.xcframework" ] || ! grep -q "$GHOSTTY_SHA" GhosttyKit.xcframework/.ghostty_sha 2>/dev/null; then - echo "==> Downloading updated GhosttyKit..." - bash scripts/download-prebuilt-ghosttykit.sh - fi + # Ensure GhosttyKit via canonical helper + bash scripts/ensure-ghosttykit.sh @@ -# --- Ensure GhosttyKit --- -if [ ! -d "GhosttyKit.xcframework" ]; then - echo "==> Downloading GhosttyKit..." - bash scripts/download-prebuilt-ghosttykit.sh -fi +# --- Ensure GhosttyKit --- +bash scripts/ensure-ghosttykit.shAlso applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-local-release.sh` around lines 47 - 51, Replace the ad-hoc existence/SHA check (GHOSTTY_SHA, GhosttyKit.xcframework and the call to scripts/download-prebuilt-ghosttykit.sh) with the repository’s canonical GhosttyKit preparation script (for example scripts/prepare-ghosttykit.sh); remove the manual if-block and instead invoke the canonical script so it performs validation/caching consistently (also update the identical block around lines 55-59). Ensure GHOSTTY_SHA is either provided to or obtained by that canonical script if needed, and drop the direct call to scripts/download-prebuilt-ghosttykit.sh in favor of the single prepare-ghosttykit entrypoint.Sources/CmuxConfig.swift (1)
331-343: Consider documenting the timing dependency between observers.The 0.15s delay assumes directory tracking (which triggers config reload) completes before
checkAutoApply()runs. This relies on the separate observer at lines 314-329 firing first andupdateLocalConfigPath→loadAll()completing within that window. The timing should work in practice, but a comment explaining this ordering would help maintainability.📝 Suggested comment
// Separate observer for autoApply: fires on every workspace switch // (after a short delay so the workspace is fully visible). + // Note: The directory-tracking observer above triggers updateLocalConfigPath → loadAll() + // synchronously on workspace switch. The 0.15s delay here ensures that path completes + // before checkAutoApply() re-evaluates the newly loaded commands. tabManager.$selectedTabId .dropFirst() // skip the initial value on subscribe🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 331 - 343, Add a short in-code comment near the tabManager.$selectedTabId observer explaining the timing dependency: note that the 0.15s DispatchQueue.main.asyncAfter delay is chosen to let the other observer (the one that calls updateLocalConfigPath → loadAll()) finish reloading config for the new workspace before checkAutoApply() runs; reference the tabManager.$selectedTabId subscription, checkAutoApply(), updateLocalConfigPath, and loadAll() so future maintainers understand the ordering assumption and can adjust the delay or convert to a more robust synchronization if needed.Sources/ContentView.swift (1)
13505-13505: Remove always-true guard in section menu block.
if !tabManager.sections.isEmpty || trueis dead conditional logic and obscures intent. Consider making this unconditional (or restoring the real predicate).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` at line 13505, The conditional in the section menu block uses an always-true expression ("if !tabManager.sections.isEmpty || true") which is dead logic; update the check in ContentView's section/menu rendering (the code referencing tabManager.sections) by removing the "|| true" so it becomes a real predicate (e.g., "if !tabManager.sections.isEmpty") or make the block unconditional by removing the entire if statement, depending on intended behavior; ensure you edit the conditional around the section menu code that references tabManager.sections to reflect the correct intent.Sources/SidebarSection.swift (1)
33-36: Avoid no-op revision bumps inremoveWorkspace(_:).
removeWorkspace(_:)incrementsrevisioneven when the workspace ID was not present, which causes unnecessary observer churn during bulk section scans.♻️ Proposed refinement
func removeWorkspace(_ workspaceId: UUID) { - workspaceIds.removeAll { $0 == workspaceId } - bumpRevision() + let previousCount = workspaceIds.count + workspaceIds.removeAll { $0 == workspaceId } + if workspaceIds.count != previousCount { + bumpRevision() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/SidebarSection.swift` around lines 33 - 36, removeWorkspace(_:) currently calls bumpRevision() unconditionally; change it to only bump when the ID was actually removed by checking before/after state or using firstIndex(of:) to remove and then call bumpRevision() only when a removal occurred (e.g., compute oldCount and compare after removeAll or use firstIndex(of:) + remove(at:)). Ensure the logic references removeWorkspace(_:) and bumpRevision() so observers only see a revision bump when workspaceIds changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build-local-release.sh`:
- Line 68: The rm -rf invocation uses unguarded expansion of BUILD_DIR; change
the removal to use the shell guarded expansion form so accidental
empty/undefined BUILD_DIR can't cause wide deletion—replace the call that
references BUILD_DIR (the rm -rf "$BUILD_DIR/") with a guarded, quoted expansion
using ${BUILD_DIR:?} (e.g., rm -rf "${BUILD_DIR:?}/") so the shell fails fast if
BUILD_DIR is unset or empty.
- Line 113: The current use of pkill -f "cmux" is too broad and may kill
unrelated processes; change it to target the exact installed binary or process
name by using either pkill -x "cmux" to match the exact process name or pkill -f
"$INSTALL_PATH/cmux" (or the variable that holds the app binary path) to match
the full path; replace the pkill -f "cmux" line with one of these scoped
variants (referencing the pkill invocation and the cmux string) and keep the
existing redirection/error-silencing (2>/dev/null || true).
- Around line 27-29: The script currently hardcodes the Apple notarization
credential (APPLE_PASSWORD) and should be changed to read credentials from
environment/secrets storage: remove the literal APPLE_PASSWORD value and instead
read APPLE_ID and APPLE_PASSWORD from environment variables (e.g., process env
names APPLE_ID and APPLE_PASSWORD) or a secure secrets provider, validate they
are present at runtime in the build-local-release.sh bootstrap (exit with a
clear error if missing), and ensure ENTITLEMENTS remains configurable; rotate
the compromised app-specific password immediately after committing this change.
In `@Sources/CmuxConfigExecutor.swift`:
- Around line 99-107: Guard against a nil focusedPanelId before closing panels:
check current.focusedPanelId and if it's nil, skip the "close all except
focused" loop (or bail out of the layout branch) so you don't close every panel;
specifically, in the block that checks wsDef.layout, ensure you only set let
keep = current.focusedPanelId and iterate current.panels.keys to call
current.closePanel(panelId, force: true) when keep is non-nil, and only call
current.applyCustomLayout(layout, baseCwd: resolvedCwd) after confirming a valid
focusedPanelId (or handling the nil case explicitly).
In `@Sources/ContentView.swift`:
- Around line 9997-10004: The workspace shortcut digit mapping is built from
layout.allWorkspacesInOrder (allOrdered) which can differ from the canonical
TabManager.tabs order, causing sidebar hint pills (workspaceShortcutDigit) to
drift from actual Cmd+1…9 behavior; change the construction of tabIndexById to
use the canonical tabs array (tabs / TabManager.tabs) instead of
layout.allWorkspacesInOrder so the index mapping is derived from the same source
used for shortcut handling (also update the same logic locations referenced
around workspaceNumberShortcut/variables at the other occurrence noted).
In `@Sources/TabManager.swift`:
- Around line 2737-2744: The method moveWorkspaceToSection currently removes the
workspace from all sections before checking that the destination exists; change
the flow in moveWorkspaceToSection so you first find and validate the target
section (use sections.first(where: { $0.id == sectionId }) and guard let target
= ... else return), then remove the workspace from other sections (call
section.removeWorkspace(tabId) for sections where section.id != target.id) and
finally call target.addWorkspace(tabId, at: atIndex) and notifySectionChange();
reference moveWorkspaceToSection, sections, removeWorkspace, addWorkspace, and
notifySectionChange when making the change.
- Around line 2686-2784: reorderWorkspace currently allows flat reorders that
can move workspaces out of their SidebarSection, so add a guard at the top of
reorderWorkspace(tabId:toIndex:) to early-return if the workspace is grouped:
use sectionForWorkspace(tabId) (or check sections.contains(where: {
$0.contains(tabId) })) and return without mutating tabs when it returns non-nil;
keep the existing flat-reorder logic only for ungrouped workspaces and ensure
you still call notifySectionChange() or other signals only when appropriate for
ungrouped moves.
---
Nitpick comments:
In `@scripts/build-local-release.sh`:
- Around line 47-51: Replace the ad-hoc existence/SHA check (GHOSTTY_SHA,
GhosttyKit.xcframework and the call to scripts/download-prebuilt-ghosttykit.sh)
with the repository’s canonical GhosttyKit preparation script (for example
scripts/prepare-ghosttykit.sh); remove the manual if-block and instead invoke
the canonical script so it performs validation/caching consistently (also update
the identical block around lines 55-59). Ensure GHOSTTY_SHA is either provided
to or obtained by that canonical script if needed, and drop the direct call to
scripts/download-prebuilt-ghosttykit.sh in favor of the single
prepare-ghosttykit entrypoint.
In `@Sources/CmuxConfig.swift`:
- Around line 331-343: Add a short in-code comment near the
tabManager.$selectedTabId observer explaining the timing dependency: note that
the 0.15s DispatchQueue.main.asyncAfter delay is chosen to let the other
observer (the one that calls updateLocalConfigPath → loadAll()) finish reloading
config for the new workspace before checkAutoApply() runs; reference the
tabManager.$selectedTabId subscription, checkAutoApply(), updateLocalConfigPath,
and loadAll() so future maintainers understand the ordering assumption and can
adjust the delay or convert to a more robust synchronization if needed.
In `@Sources/ContentView.swift`:
- Line 13505: The conditional in the section menu block uses an always-true
expression ("if !tabManager.sections.isEmpty || true") which is dead logic;
update the check in ContentView's section/menu rendering (the code referencing
tabManager.sections) by removing the "|| true" so it becomes a real predicate
(e.g., "if !tabManager.sections.isEmpty") or make the block unconditional by
removing the entire if statement, depending on intended behavior; ensure you
edit the conditional around the section menu code that references
tabManager.sections to reflect the correct intent.
In `@Sources/SidebarSection.swift`:
- Around line 33-36: removeWorkspace(_:) currently calls bumpRevision()
unconditionally; change it to only bump when the ID was actually removed by
checking before/after state or using firstIndex(of:) to remove and then call
bumpRevision() only when a removal occurred (e.g., compute oldCount and compare
after removeAll or use firstIndex(of:) + remove(at:)). Ensure the logic
references removeWorkspace(_:) and bumpRevision() so observers only see a
revision bump when workspaceIds changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 690b50a8-616b-4993-8a85-bfafcf6f9d7e
📒 Files selected for processing (9)
GhosttyTabs.xcodeproj/project.pbxprojSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/ContentView.swiftSources/SessionPersistence.swiftSources/SidebarSection.swiftSources/TabManager.swiftSources/Workspace.swiftscripts/build-local-release.sh
| APPLE_ID="applenotarization@ecuad.ca" | ||
| APPLE_PASSWORD="zdtm-jyob-rhbb-cbfq" | ||
| ENTITLEMENTS="cmux.entitlements" |
There was a problem hiding this comment.
Hardcoded notarization secret must be removed immediately.
Line [28] commits an app-specific password to source control. Treat this as compromised, rotate it, and load credentials from environment/secrets storage instead.
Proposed fix
-SIGN_IDENTITY="Developer ID Application: Emily Carr University of Art and Design (7TF6CSP83S)"
-SIGN_HASH="C0277EBA633F1AA2BC2855E45B3B38A1840053BA"
-TEAM_ID="7TF6CSP83S"
-APPLE_ID="applenotarization@ecuad.ca"
-APPLE_PASSWORD="zdtm-jyob-rhbb-cbfq"
+SIGN_IDENTITY="${SIGN_IDENTITY:-Developer ID Application: Emily Carr University of Art and Design (7TF6CSP83S)}"
+SIGN_HASH="${SIGN_HASH:?Missing SIGN_HASH}"
+TEAM_ID="${TEAM_ID:?Missing TEAM_ID}"
+APPLE_ID="${APPLE_ID:?Missing APPLE_ID}"
+APPLE_PASSWORD="${APPLE_PASSWORD:?Missing APPLE_PASSWORD}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build-local-release.sh` around lines 27 - 29, The script currently
hardcodes the Apple notarization credential (APPLE_PASSWORD) and should be
changed to read credentials from environment/secrets storage: remove the literal
APPLE_PASSWORD value and instead read APPLE_ID and APPLE_PASSWORD from
environment variables (e.g., process env names APPLE_ID and APPLE_PASSWORD) or a
secure secrets provider, validate they are present at runtime in the
build-local-release.sh bootstrap (exit with a clear error if missing), and
ensure ENTITLEMENTS remains configurable; rotate the compromised app-specific
password immediately after committing this change.
|
|
||
| # --- Build Release --- | ||
| echo "==> Building Release..." | ||
| rm -rf "$BUILD_DIR/" |
There was a problem hiding this comment.
Harden destructive delete with guarded expansion.
Line [68] should guard rm -rf with ${BUILD_DIR:?} to prevent accidental wide deletion if this variable changes unexpectedly later.
Proposed hardening
-rm -rf "$BUILD_DIR/"
+rm -rf "${BUILD_DIR:?}/"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -rf "$BUILD_DIR/" | |
| rm -rf "${BUILD_DIR:?}/" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 68-68: Use "${var:?}" to ensure this never expands to / .
(SC2115)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build-local-release.sh` at line 68, The rm -rf invocation uses
unguarded expansion of BUILD_DIR; change the removal to use the shell guarded
expansion form so accidental empty/undefined BUILD_DIR can't cause wide
deletion—replace the call that references BUILD_DIR (the rm -rf "$BUILD_DIR/")
with a guarded, quoted expansion using ${BUILD_DIR:?} (e.g., rm -rf
"${BUILD_DIR:?}/") so the shell fails fast if BUILD_DIR is unset or empty.
|
|
||
| # --- Install --- | ||
| echo "==> Installing to $INSTALL_PATH..." | ||
| pkill -f "cmux" 2>/dev/null || true |
There was a problem hiding this comment.
pkill -f "cmux" is over-broad and can kill unrelated processes.
Line [113] matches any command line containing cmux, which risks terminating non-target processes. Scope the kill to the installed app binary path or exact process name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build-local-release.sh` at line 113, The current use of pkill -f
"cmux" is too broad and may kill unrelated processes; change it to target the
exact installed binary or process name by using either pkill -x "cmux" to match
the exact process name or pkill -f "$INSTALL_PATH/cmux" (or the variable that
holds the app binary path) to match the full path; replace the pkill -f "cmux"
line with one of these scoped variants (referencing the pkill invocation and the
cmux string) and keep the existing redirection/error-silencing (2>/dev/null ||
true).
| if let layout = wsDef.layout { | ||
| // Close all panels except the focused one so applyCustomLayout | ||
| // starts from a single pane and doesn't stack on existing splits. | ||
| let keep = current.focusedPanelId | ||
| for panelId in current.panels.keys where panelId != keep { | ||
| current.closePanel(panelId, force: true) | ||
| } | ||
| current.applyCustomLayout(layout, baseCwd: resolvedCwd) | ||
| } |
There was a problem hiding this comment.
Guard against nil focusedPanelId to prevent closing all panels.
If focusedPanelId is nil (e.g., empty workspace or transient state), keep will be nil and panelId != keep evaluates to true for all panel IDs. This closes every panel, leaving zero panes. Then applyCustomLayout silently returns at its guard (per Sources/Workspace.swift:765), resulting in an empty workspace with no layout applied.
🛡️ Proposed fix to guard focusedPanelId
if let layout = wsDef.layout {
// Close all panels except the focused one so applyCustomLayout
// starts from a single pane and doesn't stack on existing splits.
- let keep = current.focusedPanelId
- for panelId in current.panels.keys where panelId != keep {
- current.closePanel(panelId, force: true)
+ if let keep = current.focusedPanelId {
+ for panelId in current.panels.keys where panelId != keep {
+ current.closePanel(panelId, force: true)
+ }
}
current.applyCustomLayout(layout, baseCwd: resolvedCwd)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let layout = wsDef.layout { | |
| // Close all panels except the focused one so applyCustomLayout | |
| // starts from a single pane and doesn't stack on existing splits. | |
| let keep = current.focusedPanelId | |
| for panelId in current.panels.keys where panelId != keep { | |
| current.closePanel(panelId, force: true) | |
| } | |
| current.applyCustomLayout(layout, baseCwd: resolvedCwd) | |
| } | |
| if let layout = wsDef.layout { | |
| // Close all panels except the focused one so applyCustomLayout | |
| // starts from a single pane and doesn't stack on existing splits. | |
| if let keep = current.focusedPanelId { | |
| for panelId in current.panels.keys where panelId != keep { | |
| current.closePanel(panelId, force: true) | |
| } | |
| } | |
| current.applyCustomLayout(layout, baseCwd: resolvedCwd) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/CmuxConfigExecutor.swift` around lines 99 - 107, Guard against a nil
focusedPanelId before closing panels: check current.focusedPanelId and if it's
nil, skip the "close all except focused" loop (or bail out of the layout branch)
so you don't close every panel; specifically, in the block that checks
wsDef.layout, ensure you only set let keep = current.focusedPanelId and iterate
current.panels.keys to call current.closePanel(panelId, force: true) when keep
is non-nil, and only call current.applyCustomLayout(layout, baseCwd:
resolvedCwd) after confirming a valid focusedPanelId (or handling the nil case
explicitly).
| let allOrdered = layout.allWorkspacesInOrder | ||
| let workspaceCount = tabs.count | ||
| let canCloseWorkspace = workspaceCount > 1 | ||
| let workspaceNumberShortcut = self.workspaceNumberShortcut | ||
| let tabItemSettings = tabItemSettingsStore.snapshot | ||
| let tabIndexById = Dictionary(uniqueKeysWithValues: tabs.enumerated().map { | ||
| let tabIndexById = Dictionary(uniqueKeysWithValues: allOrdered.enumerated().map { | ||
| ($0.element.id, $0.offset) | ||
| }) |
There was a problem hiding this comment.
Workspace shortcut hint indexing is now sourced from sidebar layout order, not canonical tab order.
workspaceShortcutDigit now depends on tabIndexById built from layout.allWorkspacesInOrder. If section/pinned rendering order differs from TabManager.tabs, the sidebar hint pills can drift from actual Cmd+1…9 behavior.
💡 Suggested fix
- let allOrdered = layout.allWorkspacesInOrder
let workspaceCount = tabs.count
@@
- let tabIndexById = Dictionary(uniqueKeysWithValues: allOrdered.enumerated().map {
+ let tabIndexById = Dictionary(uniqueKeysWithValues: tabs.enumerated().map {
($0.element.id, $0.offset)
})Based on learnings: "As of PR #2528, workspace digit mapping ... intentionally uses TabManager.tabs as the canonical ordering for both sidebar rendering and app shortcut handling."
Also applies to: 9958-9961
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 9997 - 10004, The workspace shortcut
digit mapping is built from layout.allWorkspacesInOrder (allOrdered) which can
differ from the canonical TabManager.tabs order, causing sidebar hint pills
(workspaceShortcutDigit) to drift from actual Cmd+1…9 behavior; change the
construction of tabIndexById to use the canonical tabs array (tabs /
TabManager.tabs) instead of layout.allWorkspacesInOrder so the index mapping is
derived from the same source used for shortcut handling (also update the same
logic locations referenced around workspaceNumberShortcut/variables at the other
occurrence noted).
| // MARK: - Sidebar Sections | ||
|
|
||
| /// Subscribe to every section's `objectWillChange` so that any property | ||
| /// mutation (collapse, membership, name) bumps `sectionRevision` and | ||
| /// triggers a SwiftUI re-render of the sidebar layout. | ||
| private func rebindSectionObservers() { | ||
| sectionObserverCancellables.removeAll() | ||
| for section in sections { | ||
| section.objectWillChange | ||
| .receive(on: RunLoop.main) | ||
| .sink { [weak self] _ in | ||
| self?.sectionRevision &+= 1 | ||
| } | ||
| .store(in: §ionObserverCancellables) | ||
| } | ||
| } | ||
|
|
||
| private func notifySectionChange() { | ||
| sectionRevision &+= 1 | ||
| } | ||
|
|
||
| @discardableResult | ||
| func createSection(name: String) -> SidebarSection { | ||
| let section = SidebarSection(name: name) | ||
| sections.append(section) | ||
| // Delay so SwiftUI renders the new SidebarSectionHeaderView | ||
| // (and subscribes to $pendingRenameSectionId) before we emit. | ||
| DispatchQueue.main.async { [weak self] in | ||
| self?.pendingRenameSectionId = section.id | ||
| } | ||
| return section | ||
| } | ||
|
|
||
| func renameSection(sectionId: UUID, name: String) { | ||
| guard let section = sections.first(where: { $0.id == sectionId }) else { return } | ||
| section.name = name | ||
| notifySectionChange() | ||
| } | ||
|
|
||
| func deleteSection(sectionId: UUID) { | ||
| sections.removeAll { $0.id == sectionId } | ||
| } | ||
|
|
||
| func reorderSection(sectionId: UUID, toIndex targetIndex: Int) { | ||
| guard let currentIndex = sections.firstIndex(where: { $0.id == sectionId }) else { return } | ||
| let clamped = max(0, min(targetIndex, sections.count - 1)) | ||
| guard currentIndex != clamped else { return } | ||
| let section = sections.remove(at: currentIndex) | ||
| sections.insert(section, at: clamped) | ||
| } | ||
|
|
||
| func moveWorkspaceToSection(tabId: UUID, sectionId: UUID, atIndex: Int? = nil) { | ||
| // Remove from any existing section first | ||
| for section in sections { | ||
| section.removeWorkspace(tabId) | ||
| } | ||
| guard let section = sections.first(where: { $0.id == sectionId }) else { return } | ||
| section.addWorkspace(tabId, at: atIndex) | ||
| notifySectionChange() | ||
| } | ||
|
|
||
| func removeWorkspaceFromSection(tabId: UUID) { | ||
| for section in sections { | ||
| section.removeWorkspace(tabId) | ||
| } | ||
| notifySectionChange() | ||
| } | ||
|
|
||
| func sectionForWorkspace(_ tabId: UUID) -> SidebarSection? { | ||
| sections.first { $0.contains(tabId) } | ||
| } | ||
|
|
||
| var sidebarLayout: SidebarLayout { | ||
| // Read sectionRevision to establish a SwiftUI dependency so the | ||
| // layout is recomputed whenever any section property changes. | ||
| let _ = sectionRevision | ||
| let tabById = Dictionary(uniqueKeysWithValues: tabs.map { ($0.id, $0) }) | ||
| let pinnedWorkspaces = tabs.filter { $0.isPinned } | ||
|
|
||
| // Workspace IDs that are in some section (and not pinned) | ||
| var sectionedIds = Set<UUID>() | ||
| let sectionGroups: [SidebarLayout.SectionGroup] = sections.map { section in | ||
| let workspaces = section.workspaceIds.compactMap { id -> Workspace? in | ||
| guard let ws = tabById[id], !ws.isPinned else { return nil } | ||
| return ws | ||
| } | ||
| for ws in workspaces { | ||
| sectionedIds.insert(ws.id) | ||
| } | ||
| return SidebarLayout.SectionGroup(section: section, workspaces: workspaces) | ||
| } | ||
|
|
||
| let ungroupedWorkspaces = tabs.filter { !$0.isPinned && !sectionedIds.contains($0.id) } | ||
| return SidebarLayout( | ||
| pinnedWorkspaces: pinnedWorkspaces, | ||
| ungroupedWorkspaces: ungroupedWorkspaces, | ||
| sectionGroups: sectionGroups | ||
| ) | ||
| } |
There was a problem hiding this comment.
Protect grouped workspaces from flat reorder APIs.
Now that section grouping is introduced, reorderWorkspace(...) should early-return for grouped workspaces so flat reorders cannot violate section membership/order invariants.
🛡️ Suggested guard (outside this hunk)
`@discardableResult`
func reorderWorkspace(tabId: UUID, toIndex targetIndex: Int) -> Bool {
+ guard sectionForWorkspace(tabId) == nil else { return false }
guard let currentIndex = tabs.firstIndex(where: { $0.id == tabId }) else { return false }
if tabs.count <= 1 { return true }
...
}
`@discardableResult`
func reorderWorkspace(tabId: UUID, before beforeId: UUID? = nil, after afterId: UUID? = nil) -> Bool {
+ guard sectionForWorkspace(tabId) == nil else { return false }
guard tabs.contains(where: { $0.id == tabId }) else { return false }
...
}Based on learnings: Repo: manaflow-ai/cmux — Sources/TabManager.swift — reorderWorkspace(...) must reject grouped tabs (early return). Flat-array reorders are not allowed for workspaces inside a group; use the group-aware reorder APIs instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 2686 - 2784, reorderWorkspace
currently allows flat reorders that can move workspaces out of their
SidebarSection, so add a guard at the top of reorderWorkspace(tabId:toIndex:) to
early-return if the workspace is grouped: use sectionForWorkspace(tabId) (or
check sections.contains(where: { $0.contains(tabId) })) and return without
mutating tabs when it returns non-nil; keep the existing flat-reorder logic only
for ungrouped workspaces and ensure you still call notifySectionChange() or
other signals only when appropriate for ungrouped moves.
| func moveWorkspaceToSection(tabId: UUID, sectionId: UUID, atIndex: Int? = nil) { | ||
| // Remove from any existing section first | ||
| for section in sections { | ||
| section.removeWorkspace(tabId) | ||
| } | ||
| guard let section = sections.first(where: { $0.id == sectionId }) else { return } | ||
| section.addWorkspace(tabId, at: atIndex) | ||
| notifySectionChange() |
There was a problem hiding this comment.
Validate target section before removing current membership.
moveWorkspaceToSection removes the workspace from all sections before confirming that sectionId exists. If a stale/missing ID is passed, the workspace is unintentionally ungrouped.
🐛 Proposed fix
func moveWorkspaceToSection(tabId: UUID, sectionId: UUID, atIndex: Int? = nil) {
- // Remove from any existing section first
- for section in sections {
- section.removeWorkspace(tabId)
- }
- guard let section = sections.first(where: { $0.id == sectionId }) else { return }
- section.addWorkspace(tabId, at: atIndex)
+ guard let targetSection = sections.first(where: { $0.id == sectionId }) else { return }
+ // Remove from other sections first
+ for section in sections where section.id != sectionId {
+ section.removeWorkspace(tabId)
+ }
+ targetSection.addWorkspace(tabId, at: atIndex)
notifySectionChange()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 2737 - 2744, The method
moveWorkspaceToSection currently removes the workspace from all sections before
checking that the destination exists; change the flow in moveWorkspaceToSection
so you first find and validate the target section (use sections.first(where: {
$0.id == sectionId }) and guard let target = ... else return), then remove the
workspace from other sections (call section.removeWorkspace(tabId) for sections
where section.id != target.id) and finally call target.addWorkspace(tabId, at:
atIndex) and notifySectionChange(); reference moveWorkspaceToSection, sections,
removeWorkspace, addWorkspace, and notifySectionChange when making the change.
Update bonsplit submodule: the animated split entry path hardcoded 0.5 instead of using the configured divider ratio. Include sidebar sections (id, name, collapsed state, workspace membership) in the autosave fingerprint so that section changes trigger session persistence and survive app restart.
Preserve workspace UUIDs in session snapshots so section membership survives app restart. Include sections in the autosave fingerprint so create/rename/reorder/collapse changes trigger persistence. Auto-enter rename mode when a new section is created so the user can immediately type the name.
- Add "target": "current" to cmux.json workspace commands so layouts apply to the selected workspace in-place instead of spawning a new one - Fix section rename auto-focus race: delay pendingRenameSectionId to next run loop so SwiftUI mounts the header view before the publisher emits
- Decode 'target' field in CmuxWorkspaceDefinition custom decoder so target:current actually takes effect from cmux.json - Fix within-section drag reorder: reorder section.workspaceIds instead of the flat tabs array so visual order updates correctly - Fix section rename auto-focus timing race (from prior commit)
ecd776b to
27ddeee
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/Workspace.swift (1)
6829-6837: Consider making restore identity explicit in the initializer contract.Lines 6829-6837 currently allow silent UUID regeneration when
restoredIdis omitted. A restore-specific required-ID initializer (or equivalent guard at restore entrypoints) would reduce accidental identity drift.Based on learnings: "SessionWorkspaceSnapshot.id is required (no default UUID generation). Callers must pass the live workspace id to preserve identity across restore/migration."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 6829 - 6837, The initializer currently accepts restoredId: UUID? = nil which allows accidental UUID regeneration; change the contract so restores require an explicit id: either add a dedicated restore initializer (e.g., init(restoredId: UUID, ... ) or remove the default and make restoredId non-optional in the existing init) and update call sites to pass SessionWorkspaceSnapshot.id when restoring; ensure the initializer uses the provided restoredId directly (no fallback to UUID()) and keep non-restore creation using a separate init or factory that generates a new UUID.Sources/CmuxConfig.swift (1)
422-427: AutoApply silently skipped when only global config exists.If
localConfigPathis nil (user has~/.config/cmux/cmux.jsonbut no localcmux.json),baseCwdbecomes nil via themapand the guard fails. This meansautoApply: truecommands in the global config never execute.If this is intentional (global configs lack meaningful cwd context), consider documenting this limitation. Otherwise, consider falling back to
workspace.currentDirectory:♻️ Proposed fix to support global config autoApply
private func checkAutoApply() { guard let tabManager = trackedTabManager, let workspace = tabManager.selectedWorkspace, - !autoAppliedWorkspaceIds.contains(workspace.id), - let baseCwd = localConfigPath.map({ ($0 as NSString).deletingLastPathComponent }) + !autoAppliedWorkspaceIds.contains(workspace.id) else { return } + + let baseCwd: String + if let localPath = localConfigPath { + baseCwd = (localPath as NSString).deletingLastPathComponent + } else if let dir = workspace.currentDirectory, !dir.isEmpty { + baseCwd = dir + } else { + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 422 - 427, The guard in checkAutoApply currently prevents autoApply logic when localConfigPath is nil because baseCwd is derived via localConfigPath.map, causing global configs to be skipped; change checkAutoApply to compute baseCwd by using localConfigPath's parent directory if present, otherwise fall back to workspace.currentDirectory (use trackedTabManager?.selectedWorkspace?.currentDirectory) and keep the other guards (trackedTabManager, selectedWorkspace, autoAppliedWorkspaceIds check) intact so autoApply commands in global config run with the workspace cwd when no local config exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/CmuxConfig.swift`:
- Around line 422-427: The guard in checkAutoApply currently prevents autoApply
logic when localConfigPath is nil because baseCwd is derived via
localConfigPath.map, causing global configs to be skipped; change checkAutoApply
to compute baseCwd by using localConfigPath's parent directory if present,
otherwise fall back to workspace.currentDirectory (use
trackedTabManager?.selectedWorkspace?.currentDirectory) and keep the other
guards (trackedTabManager, selectedWorkspace, autoAppliedWorkspaceIds check)
intact so autoApply commands in global config run with the workspace cwd when no
local config exists.
In `@Sources/Workspace.swift`:
- Around line 6829-6837: The initializer currently accepts restoredId: UUID? =
nil which allows accidental UUID regeneration; change the contract so restores
require an explicit id: either add a dedicated restore initializer (e.g.,
init(restoredId: UUID, ... ) or remove the default and make restoredId
non-optional in the existing init) and update call sites to pass
SessionWorkspaceSnapshot.id when restoring; ensure the initializer uses the
provided restoredId directly (no fallback to UUID()) and keep non-restore
creation using a separate init or factory that generates a new UUID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39386954-524b-4324-a07d-a4210126df0a
📒 Files selected for processing (6)
Sources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/ContentView.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/ContentView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/SessionPersistence.swift
- Sources/TabManager.swift
|
Closing and recreating — previous force-push history contained a hardcoded credential (now revoked). |
Summary
Test plan
🤖 Generated with Claude Code
Summary by cubic
Adds user-defined, collapsible sidebar sections with drag-and-drop, context menu moves, and full session persistence. Also adds auto-apply workspace commands with
target: "current"that run once per workspace on tab switch, plus fixes for split divider and rename focus.New Features
target: "current"andautoApply: true. On tab switch, commands run once per workspace per session and apply layouts in place (closing extra panes first).scripts/build-local-release.shto build, sign, notarize, and install locally.Bug Fixes
Written for commit 27ddeee. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements