Repository navigation
Fix git index.lock polling in sidebar metadata watcher - #2797
Merged
Merged
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:
📝 WalkthroughWalkthroughAdds a per-workspace and global toggle to disable the Git Metadata Watcher, wires UI (context menu + Settings), adds localization, persists the flag in session snapshots, clears cached sidebar git metadata when disabled or on remote transitions, extends settings parsing, pins two GitHub Action steps, and adds extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (context menu / settings)"
participant UI as "ContentView / TabItemView"
participant TM as "TabManager"
participant WS as "Workspace"
participant SP as "SessionPersistence"
User->>UI: Toggle watcher via context menu or Settings
UI->>TM: setWorkspaceGitMetadataWatcherDisabled(workspaceIds, disabled)
TM->>WS: update gitMetadataWatcherDisabled for each workspace
WS->>WS: if local && disabled -> clearCachedSidebarGitMetadata()
WS->>SP: include gitMetadataWatcherDisabled in snapshot
SP-->>SP: persist snapshot (omit git fields when disabled)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
295-301:⚠️ Potential issue | 🟠 MajorDon't persist git metadata when the watcher is disabled.
Line 301 saves the opt-out flag, but the surrounding snapshot code still serializes
gitBranch, andsessionPanelSnapshot(...)still serializes each panel's branch. That means the branch data still lands in the session JSON even while the watcher is disabled;restoreSessionSnapshotonly hides it after reload.Suggested fix
- let gitBranchSnapshot = gitBranch.map { branch in + let gitBranchSnapshot = gitMetadataWatcherDisabled ? nil : gitBranch.map { branch in SessionGitBranchSnapshot(branch: branch.branch, isDirty: branch.isDirty) }Also gate the per-panel branch snapshot in
sessionPanelSnapshot(panelId:includeScrollback:):- let branchSnapshot = panelGitBranches[panelId].map { + let branchSnapshot = gitMetadataWatcherDisabled ? nil : panelGitBranches[panelId].map { SessionGitBranchSnapshot(branch: $0.branch, isDirty: $0.isDirty) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 295 - 301, The snapshot currently persists git branch data even when gitMetadataWatcherDisabled is true; update the snapshot creation to omit git metadata when the watcher is disabled by: in SessionWorkspaceSnapshot construction (SessionWorkspaceSnapshot(...)) set workspace-level git fields (e.g., gitBranch) to nil or skip them when gitMetadataWatcherDisabled is true, and in sessionPanelSnapshot(panelId:includeScrollback:) gate per-panel branch serialization so each panel's branch is only included when gitMetadataWatcherDisabled is false (or the watcher is enabled); keep restoreSessionSnapshot behavior unchanged. Ensure you reference SessionWorkspaceSnapshot and sessionPanelSnapshot(panelId:includeScrollback:) when making these conditional changes.
🧹 Nitpick comments (2)
cmuxTests/SessionPersistenceTests.swift (1)
875-883: Add one explicit non-nilround-trip test forgitMetadataWatcherDisabled.The updated fixture only validates the
nilpath. A serialization regression fortrue/falsewould still pass this suite.💡 Suggested test addition
+ func testSaveAndLoadRoundTripPreservesGitMetadataWatcherDisabled() { + let tempDir = FileManager.default.temporaryDirectory + .appendingPathComponent("cmux-session-tests-\(UUID().uuidString)", isDirectory: true) + try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: tempDir) } + + let snapshotURL = tempDir.appendingPathComponent("session.json", isDirectory: false) + var snapshot = makeSnapshot(version: SessionSnapshotSchema.currentVersion) + snapshot.windows[0].tabManager.workspaces[0].gitMetadataWatcherDisabled = true + + XCTAssertTrue(SessionPersistenceStore.save(snapshot, fileURL: snapshotURL)) + let loaded = SessionPersistenceStore.load(fileURL: snapshotURL) + XCTAssertEqual( + loaded?.windows.first?.tabManager.workspaces.first?.gitMetadataWatcherDisabled, + true + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/SessionPersistenceTests.swift` around lines 875 - 883, Add explicit non-nil round-trip tests for gitMetadataWatcherDisabled by extending the SessionPersistenceTests to create AppSessionSnapshot instances via makeSnapshot with SessionWorkspaceSnapshot.gitMetadataWatcherDisabled set to true and false (in addition to the existing nil case), serialize and deserialize them, and assert the deserialized snapshots preserve the true/false values; locate the helper makeSnapshot and the AppSessionSnapshot/SessionWorkspaceSnapshot usage to add two small test cases that verify round-trip equality of gitMetadataWatcherDisabled for both true and false.cmuxTests/WorkspaceGitMetadataWatcherContextMenuTests.swift (1)
36-51: Consider extracting a shared remote-workspace fixture helper.The remote configuration setup is duplicated; pulling it into a small helper would reduce noise and make future test updates safer.
♻️ Optional refactor sketch
final class WorkspaceGitMetadataWatcherContextMenuTests: XCTestCase { + private func makeRemoteWorkspace(in manager: TabManager) -> Workspace { + let workspace = manager.addWorkspace(select: false) + workspace.configureRemoteConnection( + WorkspaceRemoteConfiguration( + destination: "cmux-macmini", + port: nil, + identityFile: nil, + sshOptions: [], + localProxyPort: nil, + relayPort: 64017, + relayID: String(repeating: "a", count: 16), + relayToken: String(repeating: "b", count: 64), + localSocketPath: "/tmp/cmux-debug-test.sock", + terminalStartupCommand: "ssh cmux-macmini" + ), + autoConnect: false + ) + return workspace + } + func testContextMenuModeHidesToggleWhenAnyTargetWorkspaceIsRemote() { let manager = TabManager() guard let localWorkspace = manager.selectedWorkspace else { XCTFail("Expected local workspace") return } - - let remoteWorkspace = manager.addWorkspace(select: false) - remoteWorkspace.configureRemoteConnection( - WorkspaceRemoteConfiguration( - destination: "cmux-macmini", - port: nil, - identityFile: nil, - sshOptions: [], - localProxyPort: nil, - relayPort: 64017, - relayID: String(repeating: "a", count: 16), - relayToken: String(repeating: "b", count: 64), - localSocketPath: "/tmp/cmux-debug-test.sock", - terminalStartupCommand: "ssh cmux-macmini" - ), - autoConnect: false - ) + let remoteWorkspace = makeRemoteWorkspace(in: manager) @@ func testSetWorkspaceGitMetadataWatcherDisabledSkipsRemoteWorkspace() { let manager = TabManager() - let remoteWorkspace = manager.addWorkspace(select: false) - remoteWorkspace.configureRemoteConnection(...) + let remoteWorkspace = makeRemoteWorkspace(in: manager)Also applies to: 64-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceGitMetadataWatcherContextMenuTests.swift` around lines 36 - 51, Extract the duplicated remote workspace setup into a small test helper (e.g., a function like makeRemoteWorkspace or addRemoteWorkspaceFixture) that calls manager.addWorkspace(select:), builds the WorkspaceRemoteConfiguration (with destination "cmux-macmini", relayPort 64017, relayID/relayToken, localSocketPath, terminalStartupCommand, etc.), and calls workspace.configureRemoteConnection(autoConnect: false); replace the duplicated blocks in WorkspaceGitMetadataWatcherContextMenuTests (the current block using manager.addWorkspace and the similar block at lines 64–79) with calls to that helper to centralize configuration and reduce duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 1599-1635: The test shows git watcher state is not reattached when
a workspace is moved via detachWorkspace/attachWorkspace: update
TabManager.attachWorkspace to reinitialize git watchers for the incoming
workspace by iterating its surfaces/panels and starting the watcher for any
panel with a directory (i.e. call whatever internal start/ensure watcher helper
you have for panels) so
attachedWorkspaceGitWatcherPanelIdsForTesting(workspaceId:) contains the
original panelId after attach; also review detachWorkspace to ensure it clears
watcher state for the correct workspace id so watchers aren’t left stranded
under the wrong key.
In `@Sources/ContentView.swift`:
- Around line 2342-2355: The context-menu enable/disable logic in
workspaceGitMetadataWatcherContextMenuMode incorrectly treats globalDisabled as
a per-workspace flag so the menu shows "Enable" even when the global watcher is
off; fix by either (A) hiding/disabling the Enable action when
GitMetadataWatcherSettings.isEnabled() is false (so
workspaceGitMetadataWatcherContextMenuMode returns .hidden when the global
setting is off), or (B) add a setter on GitMetadataWatcherSettings (e.g.,
setEnabled(_:)) and update setWorkspaceGitMetadataWatcherDisabled to clear the
global flag when the user chooses "Enable" so both
GitMetadataWatcherSettings.isEnabled() and !workspace.gitMetadataWatcherDisabled
become true. Ensure references to workspaceGitMetadataWatcherContextMenuMode,
setWorkspaceGitMetadataWatcherDisabled, and GitMetadataWatcherSettings are
updated accordingly.
In `@Sources/Workspace.swift`:
- Line 6498: When gitMetadataWatcherDisabled changes at runtime we must clear
the same cached git state that restoreSessionSnapshot(_:) clears to avoid stale
metadata; add logic to observe changes to the `@Published` var
gitMetadataWatcherDisabled and when it flips (true or false as appropriate)
clear gitBranch, panelGitBranches, pullRequest, and panelPullRequests (reuse or
call the same clearing helper used by restoreSessionSnapshot(_:)), so that
disabling the watcher immediately purges those fields and related UI helpers
don’t surface stale data.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 295-301: The snapshot currently persists git branch data even when
gitMetadataWatcherDisabled is true; update the snapshot creation to omit git
metadata when the watcher is disabled by: in SessionWorkspaceSnapshot
construction (SessionWorkspaceSnapshot(...)) set workspace-level git fields
(e.g., gitBranch) to nil or skip them when gitMetadataWatcherDisabled is true,
and in sessionPanelSnapshot(panelId:includeScrollback:) gate per-panel branch
serialization so each panel's branch is only included when
gitMetadataWatcherDisabled is false (or the watcher is enabled); keep
restoreSessionSnapshot behavior unchanged. Ensure you reference
SessionWorkspaceSnapshot and sessionPanelSnapshot(panelId:includeScrollback:)
when making these conditional changes.
---
Nitpick comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 875-883: Add explicit non-nil round-trip tests for
gitMetadataWatcherDisabled by extending the SessionPersistenceTests to create
AppSessionSnapshot instances via makeSnapshot with
SessionWorkspaceSnapshot.gitMetadataWatcherDisabled set to true and false (in
addition to the existing nil case), serialize and deserialize them, and assert
the deserialized snapshots preserve the true/false values; locate the helper
makeSnapshot and the AppSessionSnapshot/SessionWorkspaceSnapshot usage to add
two small test cases that verify round-trip equality of
gitMetadataWatcherDisabled for both true and false.
In `@cmuxTests/WorkspaceGitMetadataWatcherContextMenuTests.swift`:
- Around line 36-51: Extract the duplicated remote workspace setup into a small
test helper (e.g., a function like makeRemoteWorkspace or
addRemoteWorkspaceFixture) that calls manager.addWorkspace(select:), builds the
WorkspaceRemoteConfiguration (with destination "cmux-macmini", relayPort 64017,
relayID/relayToken, localSocketPath, terminalStartupCommand, etc.), and calls
workspace.configureRemoteConnection(autoConnect: false); replace the duplicated
blocks in WorkspaceGitMetadataWatcherContextMenuTests (the current block using
manager.addWorkspace and the similar block at lines 64–79) with calls to that
helper to centralize configuration and reduce duplication.
🪄 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: 8742623b-8a32-4f67-97a1-efa6b17d9c01
📒 Files selected for processing (12)
.github/workflows/claude.ymlResources/Localizable.xcstringsSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/TabManagerSessionSnapshotTests.swiftcmuxTests/TabManagerUnitTests.swiftcmuxTests/WorkspaceGitMetadataWatcherContextMenuTests.swift
Contributor
Greptile SummaryThis PR replaces sidebar git metadata polling (
Confidence Score: 5/5Safe to merge; all previous blocking issues are addressed in the current head. All previously flagged defects in the watcher, index parser, and shell integration have been resolved. The new code correctly handles gitlinks via commit comparison, sparse-checkout via extended-flags filtering, v4 VLI decoding matching git's varint.c bijection, and the stash scenario via dual index-signature tracking. The two open notes are a file-organisation suggestion and a correctness note for a non-default git configuration. Sources/TabManager.swift warrants a follow-up extraction into a focused SwiftPM package given its 9,250-line size post-merge. Important Files Changed
Reviews (26): Last reviewed commit: "fix: honor assume unchanged git index en..." | Re-trigger Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:13199">
P2: Avoid reading `tabManager` inside `TabItemView`’s body. This view is marked as typing-latency sensitive and should only use precomputed parameters; pulling `tabManager.tabs` during body evaluation can invalidate the `.equatable()` optimization and cause extra re-renders while typing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmuxTests/SessionPersistenceTests.swift (1)
120-156: Add the missing-key compatibility case for the new optional field.These tests cover explicit
trueandfalse, but the backward-compatible path is an older snapshot omittinggitMetadataWatcherDisabledentirely. A companion assertion thatnilomits the key and decodes back asnilwould protect the legacy restore behavior this optional schema change depends on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/SessionPersistenceTests.swift` around lines 120 - 156, Add a third test to cover the backward-compatibility case where gitMetadataWatcherDisabled is omitted: create a snapshot via makeSnapshot, explicitly set snapshot.windows[0].tabManager.workspaces[0].gitMetadataWatcherDisabled = nil, save with SessionPersistenceStore.save(snapshot, fileURL:), then load with SessionPersistenceStore.load(fileURL:) and assert that loaded?.windows.first?.tabManager.workspaces.first?.gitMetadataWatcherDisabled is nil; optionally also read the saved JSON from snapshotURL and assert the "gitMetadataWatcherDisabled" key is absent to ensure the encoder omits the optional key.cmuxTests/TabManagerUnitTests.swift (2)
511-539: The terminal-sweep branch still isn't exercised.This covers overdue polls and suppression cases, but it never hits the
nextPollAt > now+ non-.open+ stalelastTerminalStateRefreshAtpath. Add one positive sweep assertion so the background refresh branch stays pinned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 511 - 539, Add a test assertion that exercises the terminal-sweep branch of TabManager.shouldRefreshWorkspacePullRequestForTesting by calling it with now: now, nextPollAt set in the future (nextPollAt > now), currentPullRequestStatus set to a non-.open value (e.g., .closed or .merged), and lastTerminalStateRefreshAt set to a stale timestamp (e.g., now.addingTimeInterval(-3600)); assert that the result is true so the terminal-sweep/background refresh path is covered. Reference TabManager.shouldRefreshWorkspacePullRequestForTesting, nextPollAt, lastTerminalStateRefreshAt, and currentPullRequestStatus when adding this new positive assertion.
471-509: Add the unsupported-repository switch variant here.This only locks down GitHub → GitHub repo switches. The brittle path is switching to a non-GitHub/unsupported repo, where the old PR badge and poll state must be cleared; without that case, the repository-switch regression can still slip through.
Based on learnings: "In applyWorkspacePullRequestRefreshResults(...), resolution .unsupportedRepository must (1) clear any existing workspace.panelPullRequests[panelId] entry and (2) reset lastTerminalState timestamp."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 471 - 509, The test only covers GitHub→GitHub switches; add a variant that switches the panel directory to a non-GitHub/unsupported repo and assert that applyWorkspacePullRequestRefreshResults handles resolution .unsupportedRepository by clearing any existing workspace.panelPullRequests[panelId] entry and resetting the workspace's lastTerminalState timestamp (or the manager field that tracks it). Concretely, in testWorkspacePullRequestOnDemandRefreshUsesCurrentPanelDirectoryAfterRepoSwitch add steps to (1) prime a pull-request/PR-badge state for panelId, (2) switch the panel directory to a non-GitHub repo URL, (3) invoke the same refresh/resolution path, and (4) assert workspace.panelPullRequests[panelId] is nil/removed and lastTerminalState is updated/reset; reference applyWorkspacePullRequestRefreshResults, resolution .unsupportedRepository, workspace.panelPullRequests[panelId], and lastTerminalState to locate the logic to validate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 1496-1541: The test mutates the global PATH via setenv while
background git watchers may run, causing flakiness; update the test and/or code
under test so the git shim is used only for the process that computes the
workspace metadata instead of changing process-wide environment: refactor
TabManager.workspaceGitMetadataSummaryForTesting (or add a new helper) to accept
an environment dictionary or PATH string and pass that env into the subprocess
launcher used for git calls (or run the shimbed git in a serialized/external
helper process), and remove the global setenv/unsetenv usage in the test so
other watcher threads cannot observe the shimmed PATH.
In `@Sources/Workspace.swift`:
- Around line 6510-6515: The gitMetadataWatcherDisabled flag is being treated as
global across local/remote transitions; update the Workspace property logic so
it is ignored or reset for remote workspaces: in the gitMetadataWatcherDisabled
didSet (and the analogous setter around the other occurrence), only perform the
clearCachedSidebarGitMetadata() when gitMetadataWatcherDisabled changed AND
remoteConfiguration == nil (i.e., guard gitMetadataWatcherDisabled,
gitMetadataWatcherDisabled != oldValue, remoteConfiguration == nil) or
alternatively set gitMetadataWatcherDisabled = false whenever
remoteConfiguration becomes non-nil (observe/handle changes to
remoteConfiguration in Workspace and reset the flag), so remote workspaces keep
their gitBranch/pullRequest state.
---
Nitpick comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 120-156: Add a third test to cover the backward-compatibility case
where gitMetadataWatcherDisabled is omitted: create a snapshot via makeSnapshot,
explicitly set
snapshot.windows[0].tabManager.workspaces[0].gitMetadataWatcherDisabled = nil,
save with SessionPersistenceStore.save(snapshot, fileURL:), then load with
SessionPersistenceStore.load(fileURL:) and assert that
loaded?.windows.first?.tabManager.workspaces.first?.gitMetadataWatcherDisabled
is nil; optionally also read the saved JSON from snapshotURL and assert the
"gitMetadataWatcherDisabled" key is absent to ensure the encoder omits the
optional key.
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 511-539: Add a test assertion that exercises the terminal-sweep
branch of TabManager.shouldRefreshWorkspacePullRequestForTesting by calling it
with now: now, nextPollAt set in the future (nextPollAt > now),
currentPullRequestStatus set to a non-.open value (e.g., .closed or .merged),
and lastTerminalStateRefreshAt set to a stale timestamp (e.g.,
now.addingTimeInterval(-3600)); assert that the result is true so the
terminal-sweep/background refresh path is covered. Reference
TabManager.shouldRefreshWorkspacePullRequestForTesting, nextPollAt,
lastTerminalStateRefreshAt, and currentPullRequestStatus when adding this new
positive assertion.
- Around line 471-509: The test only covers GitHub→GitHub switches; add a
variant that switches the panel directory to a non-GitHub/unsupported repo and
assert that applyWorkspacePullRequestRefreshResults handles resolution
.unsupportedRepository by clearing any existing
workspace.panelPullRequests[panelId] entry and resetting the workspace's
lastTerminalState timestamp (or the manager field that tracks it). Concretely,
in
testWorkspacePullRequestOnDemandRefreshUsesCurrentPanelDirectoryAfterRepoSwitch
add steps to (1) prime a pull-request/PR-badge state for panelId, (2) switch the
panel directory to a non-GitHub repo URL, (3) invoke the same refresh/resolution
path, and (4) assert workspace.panelPullRequests[panelId] is nil/removed and
lastTerminalState is updated/reset; reference
applyWorkspacePullRequestRefreshResults, resolution .unsupportedRepository,
workspace.panelPullRequests[panelId], and lastTerminalState to locate the logic
to validate.
🪄 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: e4be677c-a6e8-400f-b116-a815a056fcdd
📒 Files selected for processing (7)
Sources/ContentView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/TabManagerSessionSnapshotTests.swiftcmuxTests/TabManagerUnitTests.swiftcmuxTests/WorkspaceGitMetadataWatcherContextMenuTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- cmuxTests/WorkspaceGitMetadataWatcherContextMenuTests.swift
- cmuxTests/TabManagerSessionSnapshotTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmuxTests/TabManagerUnitTests.swift (1)
656-695: Consider using the newmakeTempGitRepoWithInitialCommithelper.This test manually sets up the git repository with the same pattern as the new helper function at lines 125-153. Refactoring to use the helper would reduce duplication and improve maintainability.
♻️ Proposed refactor
func testInheritedBackgroundWorkspaceFetchesGitBranchWithoutSelection() throws { let fileManager = FileManager.default - let repoURL = fileManager.temporaryDirectory.appendingPathComponent( - "cmux-git-inherited-background-\(UUID().uuidString)", - isDirectory: true - ) - try fileManager.createDirectory(at: repoURL, withIntermediateDirectories: true) + let repoURL = try makeTempGitRepoWithInitialCommit(prefix: "cmux-git-inherited-background") defer { try? fileManager.removeItem(at: repoURL) } - try runGit(["init", "-b", "main"], in: repoURL) - try runGit(["config", "user.name", "cmux tests"], in: repoURL) - try runGit(["config", "user.email", "cmux@example.invalid"], in: repoURL) - try "seed\n".write( - to: repoURL.appendingPathComponent("README.md"), - atomically: true, - encoding: .utf8 - ) - try runGit(["add", "README.md"], in: repoURL) - try runGit(["commit", "-m", "Initial commit"], in: repoURL) - let manager = TabManager()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 656 - 695, The test testInheritedBackgroundWorkspaceFetchesGitBranchWithoutSelection duplicates repo setup; replace the manual Git init/commit block with the new helper makeTempGitRepoWithInitialCommit to create the temporary repo and return its URL, then assign workspace.currentDirectory = repoURL.path as before; ensure you preserve the defer cleanup semantics the helper provides and keep the subsequent assertions and background workspace creation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 8148-8156: configureRemoteConnection assigns remoteConfiguration
before clearing gitMetadataWatcherDisabled, triggering its didSet to bail and
leaving stale git state (gitBranch, panelGitBranches, pullRequest,
panelPullRequests) that will be serialized by sessionSnapshot for remote
workspaces; fix by resetting/clearing the git-related state (clear gitBranch,
panelGitBranches, pullRequest, panelPullRequests) immediately after assigning
remoteConfiguration and/or flip gitMetadataWatcherDisabled before setting
remoteConfiguration so the didSet can run, ensuring unsupportedRepository
handling in TabManager is mirrored to clear PR/state when remote target changes.
---
Nitpick comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 656-695: The test
testInheritedBackgroundWorkspaceFetchesGitBranchWithoutSelection duplicates repo
setup; replace the manual Git init/commit block with the new helper
makeTempGitRepoWithInitialCommit to create the temporary repo and return its
URL, then assign workspace.currentDirectory = repoURL.path as before; ensure you
preserve the defer cleanup semantics the helper provides and keep the subsequent
assertions and background workspace creation unchanged.
🪄 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: 938f2dd9-6927-4017-85be-ad6a72129f6a
📒 Files selected for processing (3)
Sources/TabManager.swiftSources/Workspace.swiftcmuxTests/TabManagerUnitTests.swift
austinywang
added a commit
that referenced
this pull request
Apr 14, 2026
Addresses coderabbitai feedback on #2797: the prior fix reset gitMetadataWatcherDisabled to false in configureRemoteConnection, but the didSet only fires a cache clear when transitioning to true. That meant local-origin gitBranch / panelGitBranches / pullRequest / panelPullRequests survived the local→remote promotion, and because sessionSnapshot() now serializes git metadata for remote workspaces, stale local badges could get persisted and restored as remote state — especially bad when the new remote target is unsupported or offline. Capture the previous remoteConfiguration, and when transitioning from local→remote or switching to a different remote target, explicitly clear the cached sidebar git metadata before assigning the new config. Reconfigure with the same configuration is still a no-op. Also reorder testRemoteWorkspaceIgnoresGitMetadataWatcherDisabledFlag to seed git state *after* the remote promotion (since the transition now clears it) and assert the clear-on-promote behavior explicitly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmuxTests/TabManagerUnitTests.swift (2)
1036-1042: Prefer waiting for metadata clear instead of same-tick assertions.This test currently assumes
workspace.gitMetadataWatcherDisabled = trueclears metadata synchronously. UsingwaitForCondition { ... }(as done in nearby tests) would make it less brittle if clear behavior is ever scheduled asynchronously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 1036 - 1042, The test sets workspace.gitMetadataWatcherDisabled = true and then asserts metadata cleared synchronously; instead, change the assertions to waitForCondition { ... } to poll until workspace.panelGitBranches[panelId], workspace.panelPullRequests[panelId], workspace.gitBranch, and workspace.pullRequest are nil, so the test tolerates asynchronous clearing; locate the assertions referencing panelGitBranches, panelPullRequests, gitBranch, and pullRequest and wrap them in a waitForCondition closure (as used in nearby tests) that returns true when all four are nil.
1098-1106: Consider a small helper for global watcher defaults save/restore.The
UserDefaultssnapshot/restore pattern is repeated many times. A tiny test helper (e.g.,withGitWatcherDefault(_:)) would reduce repetition and lower the chance of inconsistent cleanup in future edits.Also applies to: 1137-1145, 1194-1202, 1239-1250, 1304-1315, 1679-1690, 1730-1738, 1777-1785, 1856-1864
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 1098 - 1106, The tests repeatedly snapshot and restore UserDefaults for GitMetadataWatcherSettings.disabledKey, so add a small test helper (e.g., withGitWatcherDefault(_ value: Any?, execute: () -> Void) or withGitWatcherDefault<T>(_ value: T?, block: () -> Void)) that captures UserDefaults.standard.object(forKey: GitMetadataWatcherSettings.disabledKey), sets or removes the key, runs the provided closure, and always restores the original setting in a defer; then replace the repeated blocks around UserDefaults.standard / GitMetadataWatcherSettings.disabledKey in TabManagerUnitTests.swift (and the other listed ranges) with calls to this helper to centralize snapshot/restore logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 2422-2436: The helper workspaceGitMetadataWatcherContextMenuMode
currently hides the menu when any selected workspace is remote; change it to
first filter targetWorkspaces to only local ones (e.g., let localTargets =
targetWorkspaces.filter { !$0.isRemoteWorkspace }) and base the empty/isDisabled
checks and the allEffectivelyDisabled computation on localTargets so mixed
selections still show an action when locals exist; additionally thread a
gitMetadataWatcherWorkspaceIds (only local workspace IDs) through TabItemView
and include that property in TabItemView == so remote IDs are not forwarded and
equality reflects the new ID set.
In `@Sources/Workspace.swift`:
- Around line 8167-8182: When disconnectRemoteConnection(clearConfiguration:
true) demotes remote→local it must mirror the remote→remote change: call
clearCachedSidebarGitMetadata() and explicitly reset remote-related state (clear
gitBranch, panelGitBranches, pullRequest, panelPullRequests and any stored
remote git metadata) so the last remote badge doesn't survive into local
sessions; locate disconnectRemoteConnection(clearConfiguration:) and add the
same cache clear/reset logic used around remoteConfiguration handling (i.e. the
clearCachedSidebarGitMetadata() call and clearing of watcher flags/remote git
fields).
---
Nitpick comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 1036-1042: The test sets workspace.gitMetadataWatcherDisabled =
true and then asserts metadata cleared synchronously; instead, change the
assertions to waitForCondition { ... } to poll until
workspace.panelGitBranches[panelId], workspace.panelPullRequests[panelId],
workspace.gitBranch, and workspace.pullRequest are nil, so the test tolerates
asynchronous clearing; locate the assertions referencing panelGitBranches,
panelPullRequests, gitBranch, and pullRequest and wrap them in a
waitForCondition closure (as used in nearby tests) that returns true when all
four are nil.
- Around line 1098-1106: The tests repeatedly snapshot and restore UserDefaults
for GitMetadataWatcherSettings.disabledKey, so add a small test helper (e.g.,
withGitWatcherDefault(_ value: Any?, execute: () -> Void) or
withGitWatcherDefault<T>(_ value: T?, block: () -> Void)) that captures
UserDefaults.standard.object(forKey: GitMetadataWatcherSettings.disabledKey),
sets or removes the key, runs the provided closure, and always restores the
original setting in a defer; then replace the repeated blocks around
UserDefaults.standard / GitMetadataWatcherSettings.disabledKey in
TabManagerUnitTests.swift (and the other listed ranges) with calls to this
helper to centralize snapshot/restore logic.
🪄 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: bf1efd3f-3493-4702-aadd-bc1ca9971cdb
📒 Files selected for processing (8)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/TabManagerUnitTests.swift
✅ Files skipped from review due to trivial changes (2)
- Sources/KeyboardShortcutSettingsFileStore.swift
- Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/SessionPersistence.swift
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/TabManagerUnitTests.swift">
<violation number="1" location="cmuxTests/TabManagerUnitTests.swift:1074">
P2: This emptiness check is effectively vacuous: metadata is already cleared before promotion, so the test won’t catch regressions where remote promotion fails to clear existing local metadata.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
austinywang
added a commit
that referenced
this pull request
Apr 14, 2026
Four follow-up fixes from coderabbitai, cubic, and cursor on #2797: - WorkspaceGitEventWatcher.flushPendingPaths now captures the debounce timer before nilling it and calls setEventHandler({}) + cancel() on the fired timer, instead of dropping the reference. GCD dispatch sources should be cancelled before their last strong reference is released (cursor-bot, Sources/TabManager.swift#L428). - Workspace.disconnectRemoteConnection(clearConfiguration: true) now mirrors the clear-on-promotion behavior by calling clearCachedSidebarGitMetadata() before nilling remoteConfiguration, so remote-origin sidebar badges don't survive into local sessions and can't be serialized as local metadata (coderabbitai major). - ContentView.workspaceGitMetadataWatcherContextMenuMode now filters mixed local+remote selections down to the local subset instead of hiding the toggle outright, and TabItemView threads a new gitMetadataWatcherWorkspaceIds (local-only) through to the button so remote IDs are never forwarded to setWorkspaceGitMetadataWatcherDisabled (coderabbitai minor). Updated the existing context-menu test to assert the new derive-from-local-subset behavior and added an all-remote-hides coverage case. - testRemoteWorkspaceIgnoresGitMetadataWatcherDisabledFlag now seeds local-origin panelGitBranches / panelPullRequests before the local→remote promotion so the post-promotion emptiness assertions are no longer vacuous and actually verify configureRemoteConnection's clear path (cubic P2). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
10195-10211:⚠️ Potential issue | 🟠 MajorAdd a parent-level refresh hook for workspace git-watcher-disabled changes, consistent with terminalScrollBarHidden.
The parent
VerticalTabsSidebarprecomputesgitMetadataWatcherMenuModeat lines 10208–10211, then passes it down toTabItemView. When the user toggles the watcher via the context menu button (lines 13512–13514),setWorkspaceGitMetadataWatcherDisabled(…)mutatesworkspace.gitMetadataWatcherDisabled. UnliketerminalScrollBarHidden, which triggers an explicit notification observed at lines 10383–10394 to bumpterminalScrollBarVisibilityGenerationand invalidate the parent's precomputed state,gitMetadataWatcherDisabledhas no parent-level generation hook. The precomputed mode remains stale until an unrelated parent state change forces re-evaluation, so reopening the menu shows the old label and resends the olddisabled:value.Add a
gitMetadataWatcherDisabledNotificationtoWorkspaceand post it from thedidSetblock, then observe it inVerticalTabsSidebarto bump a generation variable (following theterminalScrollBarHiddenDidChangeNotificationpattern).Snapshot of terminalScrollBarHidden pattern for reference
Workspace.swift:6508-6510: static let terminalScrollBarHiddenDidChangeNotification = Notification.Name( "cmux.workspaceTerminalScrollBarHiddenDidChange" ) ContentView.swift:10383-10394: .onReceive( NotificationCenter.default.publisher(for: Workspace.terminalScrollBarHiddenDidChangeNotification) .receive(on: RunLoop.main) ) { notification in guard let workspace = notification.object as? Workspace, tabManager.tabs.contains(where: { $0 === workspace }) else { return } // Workspace scrollbar visibility changes do not publish on TabManager.tabs, // so bump a local generation to refresh the precomputed context-menu state. terminalScrollBarVisibilityGeneration &+= 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10195 - 10211, Add a Workspace-level notification and a parent-side observer so toggling Workspace.gitMetadataWatcherDisabled invalidates the precomputed gitMetadataWatcherMenuMode in the VerticalTabsSidebar/ContentView. Specifically, add a static Notification.Name like Workspace.gitMetadataWatcherDisabledDidChangeNotification and post it from the didSet of Workspace.gitMetadataWatcherDisabled (same place setWorkspaceGitMetadataWatcherDisabled mutates the property). Then in VerticalTabsSidebar (where gitMetadataWatcherMenuMode is precomputed) add an .onReceive(NotificationCenter.default.publisher(for: Workspace.gitMetadataWatcherDisabledDidChangeNotification).receive(on: RunLoop.main)) handler that checks the notification.object is the workspace in tabManager.tabs and increments a local generation counter (e.g. gitMetadataWatcherGeneration) analogous to terminalScrollBarVisibilityGeneration so the precomputed gitMetadataWatcherMenuMode is recomputed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Line 312: The code currently overwrites the persisted per-workspace flag by
setting gitMetadataWatcherDisabled to isRemoteWorkspace ? false :
gitMetadataWatcherDisabled, which loses a user's saved local opt-out; instead,
stop mutating the stored preference and keep gitMetadataWatcherDisabled equal to
the saved value, and derive runtime behavior from isRemoteWorkspace when
deciding whether to actually run the watcher (e.g., compute
effectiveWatcherDisabled = isRemoteWorkspace || gitMetadataWatcherDisabled where
the watcher decision is made). Update the initializer/snapping code that
contains gitMetadataWatcherDisabled and any other spots mentioned (lines
~8177-8182) to preserve the persisted flag and only combine it with
isRemoteWorkspace at runtime so returning to a local session restores the user’s
saved preference.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 10195-10211: Add a Workspace-level notification and a parent-side
observer so toggling Workspace.gitMetadataWatcherDisabled invalidates the
precomputed gitMetadataWatcherMenuMode in the VerticalTabsSidebar/ContentView.
Specifically, add a static Notification.Name like
Workspace.gitMetadataWatcherDisabledDidChangeNotification and post it from the
didSet of Workspace.gitMetadataWatcherDisabled (same place
setWorkspaceGitMetadataWatcherDisabled mutates the property). Then in
VerticalTabsSidebar (where gitMetadataWatcherMenuMode is precomputed) add an
.onReceive(NotificationCenter.default.publisher(for:
Workspace.gitMetadataWatcherDisabledDidChangeNotification).receive(on:
RunLoop.main)) handler that checks the notification.object is the workspace in
tabManager.tabs and increments a local generation counter (e.g.
gitMetadataWatcherGeneration) analogous to terminalScrollBarVisibilityGeneration
so the precomputed gitMetadataWatcherMenuMode is recomputed.
🪄 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: ac717944-0213-45f7-84bd-70e6c5830920
📒 Files selected for processing (5)
Sources/ContentView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/TabManagerUnitTests.swiftcmuxTests/WorkspaceGitMetadataWatcherContextMenuTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/WorkspaceGitMetadataWatcherContextMenuTests.swift
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/TabManagerUnitTests.swift">
<violation number="1" location="cmuxTests/TabManagerUnitTests.swift:1088">
P2: This test no longer actually flips `gitMetadataWatcherDisabled` before asserting remote metadata retention, so it may miss regressions in the flag-change path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/claude.yml">
<violation number="1">
P1: Use a commit-pinned ref for third-party actions to reduce CI supply-chain risk.</violation>
<violation number="2">
P1: Pin GitHub Actions to a full commit SHA instead of a mutable tag to prevent unreviewed upstream changes from altering CI behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Worth to mention #2722 here. Any chance this can get merged soon? |
austinywang
force-pushed
the
issue-2722-git-index-lock-poll
branch
from
May 12, 2026 03:30
0810ac7 to
d23d126
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b23f367. Configure here.
Merged
This was referenced May 22, 2026
This was referenced May 23, 2026
This was referenced Jun 1, 2026
This was referenced Jun 10, 2026
Open
This was referenced Jun 26, 2026
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 23, 2026
testPullRequestRefreshRepositoryDiscoveryDoesNotBlockMainRunLoop counted calls to a stubbed `git remote -v` subprocess as its proxy for "repository discovery ran". The refresh stopped spawning that process in manaflow-ai#2797, which replaced it with in-process config parsing, so the counter sat at zero and the assertion failed. The two assertions after it were worse than failing: with no discovery observed, "did not run on the main thread" and "the run loop kept ticking" both hold when nothing happens at all, so the test could not have caught the regression it names. Repository discovery is now the only blocking filesystem work the refresh does before it reaches the network, so that is what the test should watch. This adds GitRepositoryDiscovering for the two calls PullRequestProbeService makes while resolving candidate seeds, and lets a host inject it. GitMetadataService conforms and stays the only implementation the app installs, so behavior is unchanged; PullRequestPollService and the probe service just accept the protocol instead of the concrete type, which every existing call site already satisfies. The test injects a discovery that counts, records its thread, and sleeps, so all three assertions describe something that happened. It resolves no slugs, which also keeps the refresh off the GitHub transport and away from `gh auth token`.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 23, 2026
testPullRequestRefreshRepositoryDiscoveryDoesNotBlockMainRunLoop counted calls to a stubbed `git remote -v` subprocess as its proxy for "repository discovery ran". The refresh stopped spawning that process in manaflow-ai#2797, which replaced it with in-process config parsing, so the counter sat at zero and the assertion failed. The checks after it were worse than failing: with no discovery observed, they held whether or not anything happened at all. Repository discovery is the blocking filesystem work the refresh does before it reaches the network, so that is what the test should watch. This adds GitRepositoryDiscovering for the two calls PullRequestProbeService makes while resolving candidate seeds, and lets a host inject it. GitMetadataService conforms and stays the only implementation the app installs, so behavior is unchanged; PullRequestPollService and the probe service accept the protocol instead of the concrete type, which every existing call site already satisfies. The test injects a discovery that counts, records its thread, and sleeps. It resolves no slugs, which keeps the refresh off the GitHub transport and away from `gh auth token`. The invocation count is the assertion that can now fail for a product reason. The thread observation is recorded but not asserted as a product property: because `repositorySlugs` is a nonisolated async requirement, SE-0338 runs it off the caller's actor no matter how the refresh schedules it, so asserting it would pass even if discovery were awaited inline. The run-loop tick gap stays as a coarse guard against a seconds-long stall.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 23, 2026
testPullRequestRefreshRepositoryDiscoveryDoesNotBlockMainRunLoop counted calls to a stubbed `git remote -v` subprocess as its proxy for "repository discovery ran". The refresh stopped spawning that process in manaflow-ai#2797, which replaced it with in-process config parsing, so the counter sat at zero and the assertion failed. The checks after it were worse than failing: with no discovery observed, they held whether or not anything happened at all. Repository discovery is the blocking filesystem work the refresh does before it reaches the network, so that is what the test should watch. This adds GitRepositoryDiscovering for the two calls PullRequestProbeService makes while resolving candidate seeds, and lets a host inject it. GitMetadataService conforms and stays the only implementation the app installs, so behavior is unchanged; PullRequestPollService and the probe service accept the protocol instead of the concrete type, which every existing call site already satisfies. The test injects a discovery that counts and sleeps. It resolves no slugs, which keeps the refresh off the GitHub transport and away from `gh auth token`. The test now makes two claims rather than three. The invocation count is the one that can fail for a product reason, and it is the one that was broken. The run-loop tick gap stays as a coarse guard against a seconds-long stall. The old "discovery did not run on the main thread" check is gone, along with the observation box that fed it. `repositorySlugs` is a nonisolated async requirement, so SE-0338 runs it off the caller's actor however the refresh schedules it: the check passed no matter what the product did, including if discovery were rewritten to be awaited inline. A test that cannot fail is not evidence, and keeping it would have implied coverage the test does not have.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 23, 2026
testPullRequestRefreshRepositoryDiscoveryDoesNotBlockMainRunLoop counted calls to a stubbed `git remote -v` subprocess as its proxy for "repository discovery ran". The refresh stopped spawning that process in manaflow-ai#2797, which replaced it with in-process config parsing, so the counter sat at zero and the assertion failed. The checks after it were worse than failing: with no discovery observed, they held whether or not anything happened at all. Repository discovery is the blocking filesystem work the refresh does before it reaches the network, so that is what the test should watch. This adds GitRepositoryDiscovering for the two calls PullRequestProbeService makes while resolving candidate seeds, and lets a host inject it. GitMetadataService conforms and stays the only implementation the app installs, so behavior is unchanged; PullRequestPollService and the probe service accept the protocol instead of the concrete type, which every existing call site already satisfies. The test injects a discovery that counts and sleeps. It resolves no slugs, which keeps the refresh off the GitHub transport and away from `gh auth token`. The test now makes two claims rather than three. The invocation count is the one that can fail for a product reason, and it is the one that was broken. The run-loop tick gap stays as a coarse guard against a seconds-long stall. The old "discovery did not run on the main thread" check is gone, along with the observation box that fed it. `repositorySlugs` is a nonisolated async requirement, so SE-0338 runs it off the caller's actor however the refresh schedules it: the check passed no matter what the product did, including if discovery were rewritten to be awaited inline. A test that cannot fail is not evidence, and keeping it would have implied coverage the test does not have. The run-loop tick gap stays as a coarse guard, and its comment now says why it is loose: 45 seeds times 30ms of injected blocking is 1.35s against a 2.0s ceiling, so this test's own work cannot trip it. It fires only if the product adds a multi-second main-thread stall on top. The counter is renamed to RepositoryDiscoveryInvocationCounter, since it counts discovery calls rather than command-runner calls, and the new TabManager parameter carries a note that it overrides discovery for the pull-request refresh only.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 25, 2026
…inst Four session-restore tests in WorkspaceManualUnreadTests still describe the pre-manaflow-ai#2797-era restore model, where an unread notification present at snapshot time was dropped and came back as a purely visual "restored unread indicator" with a count of zero. e485692 changed that on purpose. Snapshots now carry the notifications themselves, restore re-inserts them still unread, and the restored-unread indicator is set only when a snapshot claims unread with no unread notification to back it. Both gates are live: the workspace level checks `snapshot.notifications?.contains { !$0.isRead }` before setting the indicator, and the panel level does the same. Setting both would count one notification twice. So these tests asserted an indicator that the product deliberately no longer sets, and a count of zero for a notification the product deliberately keeps unread. They now assert the restored notification directly and leave the indicator false, which is the behavior the product implements. The independence the last two tests are named for still holds: manual unread and a restored notification each contribute, so the count is two until the manual half is cleared. The assertions after markPanelRead and markRead are untouched, because marking read clears the notification and the old expectations there were already correct. The test names are unchanged; the CI shard timings key on them.
This branch was successfully deployed
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
3 participants
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
git status --porcelain -uno/git branch --show-currentpath that users confirmed still touched.git/index.lockin 0.64.4 and 0.64.7..git/HEAD,.git/config, and.git/indexreads so sidebar branch/dirty/PR metadata refreshes without taking Git's index lock.sidebar.watchGitStatusopt-out in Settings, command palette, shell integration env, andcmux.json; disabling it clears cached sidebar git/PR badge state, and re-enabling restarts probes from current terminal directories.Regression Coverage
testNoIndexLockTouchDuringSidebarGitMetadataRefreshWindowwatches.git/index.lockevery 100ms for 90.5s while driving the sidebar git-refresh path repeatedly..git/index.lockis observed zero times.Validation
git status --porcelain -unocan create.git/index.lockbriefly in/tmp/cmux-clonewhile a watcher observes the path.Resources/Localizable.xcstringsandweb/data/cmux.schema.json.git diff --checkand targeted search for remaining sidebar/shell git subprocess commands.CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-2722-git-index-lock-poll --launch.GitLockProbewithcwd=/tmp/cmux-cloneandgit_branch=main clean; watched/tmp/cmux-clone/.git/index.lockevery 100ms for 95s:LOCK_COUNT=0,SAMPLES=768,FIRST_LOCK=none,LAST_LOCK=none.top -pid 63222:0.0CPU,497Mmemory. Full suite validation is running in CI.Note
Medium Risk
Moderate risk due to a large refactor of sidebar git/PR metadata collection (new file-watcher + git index parsing) and new settings-driven enable/disable behavior that affects live workspace UI state.
Overview
Replaces sidebar git/PR metadata refresh from periodic
gitsubprocess polling with file-based detection:TabManagernow resolves repositories by walking.git/worktree metadata, watches relevant paths via FSEvents, and derives branch/dirty/index state by directly readingHEAD, refs, and parsing the git index (including gitlinks/submodules). A fallback timer remains to refresh metadata when watchers miss events.Adds a new
sidebar.watchGitStatussetting (UI, command palette, settings search aliases, JSON paths/template) that can disable all git watching; when off, shell integration and socket handlers stop emitting/accepting git/PR updates, existing sidebar git/PR badges are cleared, and re-enabling restarts watching from current panel directories.Updates bash/zsh shell integrations to avoid invoking
gitfor branch andoriginURL discovery by reading.git/HEADand parsing git config files (includinginclude/includeIf), and introducesCMUX_NO_GIT_WATCHto control this behavior. Also tightens agent-hook session status handling to avoid idle updates clobbering newer sessions, refines Grok stop-notification fallback behavior, and improves shortcut window resolution plus related tests.CI/perf workflows are adjusted to run an additional shell config parsing test and to cache/resolve Swift packages for faster perf builds.
Reviewed by Cursor Bugbot for commit ac1ff65. Bugbot is set up for automated code reviews on this repo. Configure here.