Repository navigation
Extract CmuxFileWatch and consolidate file watchers (modular refactor, wave 2) - #5244
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 CmuxFileWatch (FSEvents wrapper, FileWatchClock, RecursivePathWatcher actor and tests), integrates it into TabManager with per-probe RecursivePathWatcher Tasks to schedule git-metadata refreshes, wires the package into Xcode, and removes a timing-based test. ChangesFile System Watcher Package and Integration
Sequence DiagramsequenceDiagram
participant FSEvents
participant FileSystemEventStream
participant RecursivePathWatcher
participant TabManager
participant FileWatchClock
FSEvents->>FileSystemEventStream: deliver low-level events
FileSystemEventStream->>RecursivePathWatcher: invoke onEvent callback
RecursivePathWatcher->>RecursivePathWatcher: handleFileSystemEvent() (leading-edge)
alt no pending throttle
RecursivePathWatcher->>FileWatchClock: sleep(for: throttleWindow)
end
FileWatchClock-->>RecursivePathWatcher: sleep completes
RecursivePathWatcher->>TabManager: yield one event via events AsyncStream
TabManager->>TabManager: scheduleWorkspaceGitMetadataRefreshIfPossible()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR extracts a new
Confidence Score: 5/5Safe to merge — the FSEvent stream lifecycle is correctly bounded with synchronous teardown, and the leading-edge coalescing behavior is deterministically verified with an injected gate clock. The extraction correctly preserves the shared-queue thread-bounding strategy in FileSystemEventStream, fixes the old JSONConfigFileWatcher deferred-start race window, and resolves the fire-and-forget teardown concern from the previous review round. No behavioral regressions were found in the lifecycle paths. FileWatcher.swift — per-instance DispatchQueue (vs. FileSystemEventStream's shared static queue) is worth tracking as more consumers migrate onto it. Important Files Changed
Sequence DiagramsequenceDiagram
participant TM as TabManager (MainActor)
participant RPW as RecursivePathWatcher (actor)
participant FSES as FileSystemEventStream
participant FQ as FSEvents shared queue
participant CT as Consumer Task (@MainActor)
TM->>RPW: init?(paths:) — synchronous
RPW->>FSES: init?(paths:latency:onEvent:) — synchronous
FSES->>FQ: FSEventStreamCreate + Start
RPW->>RPW: "Task { [weak self] pump rawEvents }"
TM->>CT: "Task { @MainActor for await _ in watcher.events }"
FQ-->>FSES: FSEvent callback (onEvent)
FSES->>RPW: rawContinuation.yield(())
RPW->>RPW: handleRawEvent() → arm throttleTask
Note over RPW: leading-edge: first event arms one delay
RPW->>CT: continuation.yield(()) after clock.sleep
CT->>TM: scheduleWorkspaceGitMetadataRefreshIfPossible
TM->>CT: task.cancel()
TM->>RPW: removeValue (last strong ref) → deinit
RPW->>FSES: stop() — synchronous queue.sync
FSES->>FQ: FSEventStreamInvalidate (synchronous)
RPW->>RPW: rawContinuation.finish() → pump exits
RPW->>RPW: continuation.finish() → events stream done
Reviews (5): Last reviewed commit: "Add FileWatcher tests for directory targ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1712
to
+1716
| if let watcher = workspaceGitMetadataWatchersByKey.removeValue(forKey: key) { | ||
| // stop() finishes the events stream, which ends the consumer loop; | ||
| // the watcher's own deinit also tears down the FSEventStream. | ||
| Task { await watcher.stop() } | ||
| } |
Contributor
There was a problem hiding this comment.
watcher.stop() used to run synchronously via DispatchQueue.sync inside the old class, guaranteeing the FSEventStream was fully invalidated before stopWorkspaceGitMetadataWatcher returned. The new fire-and-forget Task { await watcher.stop() } defers that invalidation until the actor hop fires: in the common stop-then-start path (line 1690 in the same file), the new watcher's FSEventStream is created and running before the old one's FSEventStreamInvalidate executes, so both streams briefly co-monitor the same paths. Under the cmux concurrency policy, fire-and-forget tasks with meaningful lifecycle should be avoided; consider making stopWorkspaceGitMetadataWatcher and its callers async so the await can be expressed directly, or keeping a cancellation token that forces the actor's isStopped flag synchronously while deferring only the C-level stream teardown.
| self.events = stream | ||
| self.continuation = continuation | ||
| self.eventStream = nil | ||
| Task { await self.startEventStream() } |
Contributor
There was a problem hiding this comment.
init
Task { await self.startEventStream() } is a fire-and-forget created inside init, making the FSEventStream attachment invisible to callers and the Swift structured-concurrency tree. The isStopped guard in startEventStream correctly handles the early-stop() race, but the approach also means the actor holds a strong self reference in an escaping task during the window between init returning and startEventStream running. The pattern is called out in the doc comment; consider an async static factory (WorkspaceGitMetadataWatcher.start(descriptor:)) as the idiomatic actor alternative, which would also remove the need to document the race window.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/CmuxWorkspaceGit/README.md (1)
12-31: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd an explicit test-pattern section for injected dependencies.
Please add a small “Testing” section that shows concrete instantiation with a test clock (and note/update corresponding DocC test pattern), not just a prose mention. This package has a non-trivial public surface and the README should include the test-friendly setup pattern directly.
As per coding guidelines: “
Packages/*/README.md: Document the test pattern for any non-trivial public surface in the package README and DocC catalog, showing how to instantiate the type with test-friendly dependencies.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxWorkspaceGit/README.md` around lines 12 - 31, Add a "Testing" section to the README that shows the concrete instantiation pattern for injecting a test clock into the watcher: demonstrate creating a controllable test implementation of WorkspaceGitWatchClock (used in place of SystemWorkspaceGitWatchClock), passing it into WorkspaceGitMetadataWatcher, starting the Task that iterates watcher.events, releasing the test clock to simulate event coalescing, and tearing down via task.cancel() and await watcher.stop(); also update the DocC test pattern to match this concrete example and mention CmuxSidebarGit only as the consumer of resolved paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/WorkspacePullRequestSidebarTests.swift`:
- Around line 1635-1640: Add a single app-level smoke test in
WorkspacePullRequestSidebarTests.swift that verifies the TabManager bridge still
consumes repeated WorkspaceGitMetadataWatcher events and triggers the sidebar
refresh path: instantiate or inject a fake WorkspaceGitMetadataWatcher that can
emit a burst of events, start the TabManager consumer task (reference TabManager
and its consumer/bridge entrypoint), and assert that the sidebar refresh
scheduler or refresh side-effect (e.g., Sidebar.refresh invocation,
scheduleSidebarRefresh call, or the observable state the UI reads) was invoked
at least once; keep the test narrow (no timing-based waits) by using
deterministically emitted events and awaiting the expected refresh signal from
the TabManager.
In
`@Packages/CmuxWorkspaceGit/Sources/CmuxWorkspaceGit/WorkspaceGitMetadataWatcher.swift`:
- Around line 92-104: The init? currently defers starting the
FileSystemEventStream via Task { await self.startEventStream() }, which can miss
early filesystem events and hides start failures; instead call
startEventStream() synchronously inside the initializer, set self.eventStream
and self.continuation only after successful creation, and return nil if
startEventStream() (or FileSystemEventStream creation) fails; update the
initializer that takes descriptor: Descriptor and clock: any
WorkspaceGitWatchClock (SystemWorkspaceGitWatchClock default) to perform
synchronous startup and failure handling so events, continuation and eventStream
are valid when init returns.
In
`@Packages/CmuxWorkspaceGit/Tests/CmuxWorkspaceGitTests/WorkspaceGitMetadataWatcherTests.swift`:
- Around line 12-18: GateClock.sleep(for:) currently uses a non-throwing
continuation so it never resumes with cancellation, preventing the watcher’s
canceled throttle task from observing cancellation; change GateClock.sleep(for:)
to use withCheckedThrowingContinuation (CheckedContinuation<Void,
Error>/throwing continuation) and store that throwing continuation in sleepers,
and ensure the continuation is resumed(throwing: CancellationError()) when the
awaiting Task is cancelled (or when stop() triggers cancellation) so
WorkspaceGitWatchClock.sleep(for:) mirrors Task.sleep behavior; then add a test
that calls stop() while clock.waitForSleeper() has a pending sleeper and assert
the throttle flush path runs (i.e., stop-while-throttled is exercised). Include
references to GateClock.sleep(for:), WorkspaceGitWatchClock.sleep(for:), and the
test that calls stop().
---
Outside diff comments:
In `@Packages/CmuxWorkspaceGit/README.md`:
- Around line 12-31: Add a "Testing" section to the README that shows the
concrete instantiation pattern for injecting a test clock into the watcher:
demonstrate creating a controllable test implementation of
WorkspaceGitWatchClock (used in place of SystemWorkspaceGitWatchClock), passing
it into WorkspaceGitMetadataWatcher, starting the Task that iterates
watcher.events, releasing the test clock to simulate event coalescing, and
tearing down via task.cancel() and await watcher.stop(); also update the DocC
test pattern to match this concrete example and mention CmuxSidebarGit only as
the consumer of resolved paths.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e866ef07-dd74-438a-8cd5-bf6956162c6d
📒 Files selected for processing (9)
Packages/CmuxWorkspaceGit/Package.swiftPackages/CmuxWorkspaceGit/README.mdPackages/CmuxWorkspaceGit/Sources/CmuxWorkspaceGit/FileSystemEventStream.swiftPackages/CmuxWorkspaceGit/Sources/CmuxWorkspaceGit/WorkspaceGitMetadataWatcher.swiftPackages/CmuxWorkspaceGit/Sources/CmuxWorkspaceGit/WorkspaceGitWatchClock.swiftPackages/CmuxWorkspaceGit/Tests/CmuxWorkspaceGitTests/WorkspaceGitMetadataWatcherTests.swiftSources/TabManager.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspacePullRequestSidebarTests.swift
Comment on lines
+12
to
+18
| func sleep(for duration: Duration) async throws { | ||
| await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in | ||
| sleepers.append(continuation) | ||
| let waiters = arrivalWaiters | ||
| arrivalWaiters.removeAll() | ||
| for waiter in waiters { waiter.resume() } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
clock_file=$(fd 'WorkspaceGitWatchClock.swift$' Packages | head -n1)
watcher_file=$(fd 'WorkspaceGitMetadataWatcher.swift$' Packages | head -n1)
test_file=$(fd 'WorkspaceGitMetadataWatcherTests.swift$' Packages | head -n1)
echo "== WorkspaceGitWatchClock / production clock =="
sed -n '1,200p' "$clock_file"
echo
echo "== Watcher stop/cancel path =="
rg -n -C3 'stop\(|cancel\(|sleep\(for:' "$watcher_file"
echo
echo "== GateClock and current tests =="
sed -n '1,140p' "$test_file"
rg -n -C2 'waitForSleeper|releaseOne|stop\(' "$test_file"Repository: manaflow-ai/cmux
Length of output: 9375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
watcher_file=$(fd 'WorkspaceGitMetadataWatcher.swift$' Packages | head -n1)
rg -n "throttleTask|flushThrottle|testThrottleClock|WorkspaceGitWatchClock" "$watcher_file" -S
echo "---- flushThrottle context ----"
rg -n -C3 "func flushThrottle|flushThrottle\(" "$watcher_file" -S
echo "---- throttleTask context ----"
rg -n -C5 "throttleTask = Task|try\?\s+await\s+clock\.sleep" "$watcher_file" -SRepository: manaflow-ai/cmux
Length of output: 1846
Make GateClock.sleep(for:) cancellation-aware and cover stop-while-throttled.
WorkspaceGitMetadataWatcher cancels the throttle Task and relies on the clock contract (WorkspaceGitWatchClock.sleep(for:) async throws; production uses Task.sleep, which throws on cancellation) so the task can reach flushThrottle() (guarded by isStopped) instead of being left parked. GateClock.sleep(for:) never resumes or throws on cancellation (non-throwing continuation), so cancellation/stop behavior isn’t exercised and a flushThrottle()-after-stop() regression could slip through.
Update GateClock.sleep(for:) to resume on cancellation by throwing CancellationError, and add a test that calls stop() while clock.waitForSleeper() has a pending sleeper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/CmuxWorkspaceGit/Tests/CmuxWorkspaceGitTests/WorkspaceGitMetadataWatcherTests.swift`
around lines 12 - 18, GateClock.sleep(for:) currently uses a non-throwing
continuation so it never resumes with cancellation, preventing the watcher’s
canceled throttle task from observing cancellation; change GateClock.sleep(for:)
to use withCheckedThrowingContinuation (CheckedContinuation<Void,
Error>/throwing continuation) and store that throwing continuation in sleepers,
and ensure the continuation is resumed(throwing: CancellationError()) when the
awaiting Task is cancelled (or when stop() triggers cancellation) so
WorkspaceGitWatchClock.sleep(for:) mirrors Task.sleep behavior; then add a test
that calls stop() while clock.waitForSleeper() has a pending sleeper and assert
the throttle flush path runs (i.e., stop-while-throttled is exercised). Include
references to GateClock.sleep(for:), WorkspaceGitWatchClock.sleep(for:), and the
test that calls stop().
![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 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
azooz2003-bit
force-pushed
the
feat-cmux-workspace-git
branch
from
June 2, 2026 21:42
d03f5f7 to
986a861
Compare
azooz2003-bit
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Packages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swift (1)
12-19: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMake
GateClock.sleep(for:)cancellation-aware and cover stop-while-throttled.
RecursivePathWatcher.stop()cancels the throttle task, but this fake clock never throws or resumes on cancellation, so the tests do not modelSystemFileWatchClock/Task.sleepbehavior. That leaves the “stop while a throttle delay is parked” path unexercised and can hide regressions in the post-cancel flush logic.#!/bin/bash set -euo pipefail test_file='Packages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swift' watcher_file='Packages/CmuxFileWatch/Sources/CmuxFileWatch/RecursivePathWatcher.swift' clock_file='Packages/CmuxFileWatch/Sources/CmuxFileWatch/FileWatchClock.swift' echo "== GateClock implementation ==" sed -n '1,40p' "$test_file" echo echo "== Watcher cancellation path ==" sed -n '110,145p' "$watcher_file" echo echo "== Production clock contract ==" sed -n '18,36p' "$clock_file"Also applies to: 94-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swift` around lines 12 - 19, The fake GateClock's sleep(for:) must model Task.sleep cancellation by making GateClock.sleep(for:) async throws and resume or throw on Task cancellation; update the test implementation of sleep(for:) (the function named sleep(for duration: Duration) async throws in the GateClock test helper) to check for Task.isCancelled and resume the waiting CheckedContinuation by throwing CancellationError (or directly throw when awaiting), so cancellations resume/throw like SystemFileWatchClock/Task.sleep; then add or adjust the RecursivePathWatcherTests to include a stop-while-throttled scenario that triggers RecursivePathWatcher.stop() while a throttle delay is parked to ensure the post-cancel flush logic is exercised.Packages/CmuxFileWatch/Sources/CmuxFileWatch/RecursivePathWatcher.swift (1)
63-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStart the
FSEventStreambefore returning the watcher.Deferring
startEventStream()throughTask { ... }reintroduces a real blind spot: any write that lands afterinitreturns but before that task constructs the stream is outside the observed range becauseFileSystemEventStreamsubscribes withkFSEventStreamEventIdSinceNow. It also turns create/start failure into a silent no-op by still returning a watcher whoseeventsstream never yields. This changes behavior for callers that assume a non-nilwatcher is live.#!/bin/bash set -euo pipefail watcher_file='Packages/CmuxFileWatch/Sources/CmuxFileWatch/RecursivePathWatcher.swift' stream_file='Packages/CmuxFileWatch/Sources/CmuxFileWatch/FileSystemEventStream.swift' echo "== Deferred startup in RecursivePathWatcher ==" sed -n '55,95p' "$watcher_file" echo echo "== FSEvents subscription point in FileSystemEventStream ==" sed -n '67,90p' "$stream_file"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxFileWatch/Sources/CmuxFileWatch/RecursivePathWatcher.swift` around lines 63 - 80, The initializer currently defers starting the FSEventStream via Task { await startEventStream() } which creates a blind spot and hides start failures; change init? to start the stream synchronously (call startEventStream on the current executor before returning), make startEventStream surface errors (throw or return Bool) and if stream creation/start fails return nil from init?, and ensure self.eventStream and the AsyncStream continuation are only exposed after successful start so a non-nil watcher always has an active FileSystemEventStream (refer to init?, startEventStream(), eventStream, events, continuation, and FileSystemEventStream).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Line 1933: The project file is missing wiring for the new CmuxFileWatch
package in the unit-test target: add an XCLocalSwiftPackageReference and
corresponding XCSwiftPackageProductDependency for CmuxFileWatch, insert a
PBXBuildFile entry for CmuxFileWatch into the Frameworks build phase (the
F1000006 /* Frameworks */ list) of the cmuxTests target (F1000004 /* cmuxTests
*/), and ensure both the main cmux and the cmuxTests targets mirror the same
package/product entries (so cmux, cmux-cli and the unit-test target all include
CmuxFileWatch). Ensure identifiers match the existing CmuxFileWatch entries used
elsewhere in the pbxproj so the package is consistently referenced across
targets.
In `@Packages/CmuxFileWatch/README.md`:
- Around line 17-25: Add a short testing example to the README alongside the
runtime snippet that demonstrates injecting a fake FileWatchClock into
RecursivePathWatcher for deterministic tests: describe constructing a test
FakeFileWatchClock (or using an existing test helper), initializing
RecursivePathWatcher with that fake clock and test paths, producing/scheduling
synthetic events via the fake clock, awaiting the watcher's events in a Task to
assert reload/handling, and then tearing down by cancelling the Task and calling
await watcher.stop(); reference the RecursivePathWatcher type and the
FileWatchClock (or FakeFileWatchClock) so readers know which symbols to use.
In `@Sources/TabManager.swift`:
- Around line 1691-1703: The Task consumers created and stored in
workspaceGitMetadataWatcherRefreshTasksByKey (consuming
RecursivePathWatcher.events and calling
scheduleWorkspaceGitMetadataRefreshIfPossible) are not cancelled on teardown;
update TabManager to cancel those Tasks and stop/close the associated
RecursivePathWatcher entries (workspaceGitMetadataWatchersByKey) during deinit
(or the existing stop helper called from deinit) so the unstructured for-await
loops are terminated; ensure the same cleanup is applied for the other
watcher/task pairs referenced around the 1711-1716 and 1727-1734 ranges and that
you remove entries from both dictionaries after cancelling/stopping.
---
Duplicate comments:
In `@Packages/CmuxFileWatch/Sources/CmuxFileWatch/RecursivePathWatcher.swift`:
- Around line 63-80: The initializer currently defers starting the FSEventStream
via Task { await startEventStream() } which creates a blind spot and hides start
failures; change init? to start the stream synchronously (call startEventStream
on the current executor before returning), make startEventStream surface errors
(throw or return Bool) and if stream creation/start fails return nil from init?,
and ensure self.eventStream and the AsyncStream continuation are only exposed
after successful start so a non-nil watcher always has an active
FileSystemEventStream (refer to init?, startEventStream(), eventStream, events,
continuation, and FileSystemEventStream).
In
`@Packages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swift`:
- Around line 12-19: The fake GateClock's sleep(for:) must model Task.sleep
cancellation by making GateClock.sleep(for:) async throws and resume or throw on
Task cancellation; update the test implementation of sleep(for:) (the function
named sleep(for duration: Duration) async throws in the GateClock test helper)
to check for Task.isCancelled and resume the waiting CheckedContinuation by
throwing CancellationError (or directly throw when awaiting), so cancellations
resume/throw like SystemFileWatchClock/Task.sleep; then add or adjust the
RecursivePathWatcherTests to include a stop-while-throttled scenario that
triggers RecursivePathWatcher.stop() while a throttle delay is parked to ensure
the post-cancel flush logic is exercised.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90cc516c-a59a-4a5f-9d55-5fe8c458a3e1
📒 Files selected for processing (9)
Packages/CmuxFileWatch/Package.swiftPackages/CmuxFileWatch/README.mdPackages/CmuxFileWatch/Sources/CmuxFileWatch/FileSystemEventStream.swiftPackages/CmuxFileWatch/Sources/CmuxFileWatch/FileWatchClock.swiftPackages/CmuxFileWatch/Sources/CmuxFileWatch/RecursivePathWatcher.swiftPackages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swiftSources/TabManager.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspacePullRequestSidebarTests.swift
azooz2003-bit
force-pushed
the
feat-cmux-workspace-git
branch
from
June 3, 2026 00:57
986a861 to
61136b4
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TabManager.swift (1)
1654-1665: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winOwn the descriptor-resolution task per probe.
Line 1654 starts a fire-and-forget
TaskforworkspaceGitMetadataWatchedPaths(for:), but nothing cancels it when the directory changes or the probe is removed. The stale-request guard only drops the result after the git/index walk has already run. MirrorworkspaceGitMetadataWatcherRefreshTasksByKeyhere so each key has a cancellable descriptor task, and cancel it fromstopWorkspaceGitMetadataWatcher(for:)/stopAllWorkspaceGitMetadataWatchers().Based on learnings: Applies to **/*.swift : Flag fire-and-forget
Task { ... }work with meaningful lifecycle that is not stored, cancelled, or tied to a caller-owned operation in cmux-owned Swift code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TabManager.swift` around lines 1654 - 1665, The code fires an uncancelled Task around workspaceGitMetadataWatchedPaths(for:) and then applies the result with applyWorkspaceGitMetadataWatcherDescriptor(...); instead, create and store a cancellable Task per key in workspaceGitMetadataWatcherRefreshTasksByKey (similar to existing watcher maps), use that Task to perform the detached work and await its value, and ensure stopWorkspaceGitMetadataWatcher(for:) and stopAllWorkspaceGitMetadataWatchers() cancel and remove the stored Task so directory changes or probe removal cancel in-flight descriptor-resolution work. Ensure weak-self/stale-request checks remain but move the Task creation into the per-key storage so each probe owns its lifecycle.
♻️ Duplicate comments (2)
Sources/TabManager.swift (1)
1726-1734:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCall this from
deinit.This helper finally centralizes watcher/task shutdown, but Line 1434 still never invokes it. That leaves the stored
for awaitconsumers and theirRecursivePathWatcherstreams alive afterTabManagerteardown.Suggested cleanup
deinit { + stopAllWorkspaceGitMetadataWatchers() workspaceCycleCooldownTask?.cancel() agentPIDSweepTimer?.cancel() workspacePullRequestPollTimer?.cancel() workspaceGitMetadataFallbackTimer?.cancel() workspacePullRequestRefreshTask?.cancel() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TabManager.swift` around lines 1726 - 1734, The TabManager deinitialization never calls stopAllWorkspaceGitMetadataWatchers, leaving for-await consumers and RecursivePathWatcher streams running; update TabManager.deinit to call stopAllWorkspaceGitMetadataWatchers() (ensuring any async stop calls are awaited or invoked via Task if deinit can't be async) so that workspaceGitMetadataWatcherRefreshTasksByKey and workspaceGitMetadataWatchersByKey are cancelled/stopped and then cleared by the existing helper.cmuxTests/WorkspacePullRequestSidebarTests.swift (1)
1635-1640:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep one app-level smoke test for the TabManager watcher bridge.
Line 1635 removes the only sustained-event integration assertion and replaces it with package-level coverage notes. That leaves the
TabManagerconsumer path (draining watcher events and scheduling sidebar refresh) unverified at the app layer. Please restore one deterministic, non-timing smoke test for that bridge contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/WorkspacePullRequestSidebarTests.swift` around lines 1635 - 1640, Restore a single deterministic app-level smoke test in WorkspacePullRequestSidebarTests that verifies the TabManager watcher bridge: add a test that injects a fake watcher or test double (mirroring the approach in WorkspaceGitMetadataWatcherTests.burstCoalescesAndThrottleRearms) and a controllable clock into TabManager, simulate a sustained burst of file-change events, advance the injected clock instead of sleeping, and assert that TabManager drains the watcher events and schedules exactly one sidebar refresh (verify via the Sidebar refresh hook/mock). Use the existing CmuxWorkspaceGit watcher-logic test strategy (injected clock & no real waiting) to keep this test deterministic and non-timing-based.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 1654-1665: The code fires an uncancelled Task around
workspaceGitMetadataWatchedPaths(for:) and then applies the result with
applyWorkspaceGitMetadataWatcherDescriptor(...); instead, create and store a
cancellable Task per key in workspaceGitMetadataWatcherRefreshTasksByKey
(similar to existing watcher maps), use that Task to perform the detached work
and await its value, and ensure stopWorkspaceGitMetadataWatcher(for:) and
stopAllWorkspaceGitMetadataWatchers() cancel and remove the stored Task so
directory changes or probe removal cancel in-flight descriptor-resolution work.
Ensure weak-self/stale-request checks remain but move the Task creation into the
per-key storage so each probe owns its lifecycle.
---
Duplicate comments:
In `@cmuxTests/WorkspacePullRequestSidebarTests.swift`:
- Around line 1635-1640: Restore a single deterministic app-level smoke test in
WorkspacePullRequestSidebarTests that verifies the TabManager watcher bridge:
add a test that injects a fake watcher or test double (mirroring the approach in
WorkspaceGitMetadataWatcherTests.burstCoalescesAndThrottleRearms) and a
controllable clock into TabManager, simulate a sustained burst of file-change
events, advance the injected clock instead of sleeping, and assert that
TabManager drains the watcher events and schedules exactly one sidebar refresh
(verify via the Sidebar refresh hook/mock). Use the existing CmuxWorkspaceGit
watcher-logic test strategy (injected clock & no real waiting) to keep this test
deterministic and non-timing-based.
In `@Sources/TabManager.swift`:
- Around line 1726-1734: The TabManager deinitialization never calls
stopAllWorkspaceGitMetadataWatchers, leaving for-await consumers and
RecursivePathWatcher streams running; update TabManager.deinit to call
stopAllWorkspaceGitMetadataWatchers() (ensuring any async stop calls are awaited
or invoked via Task if deinit can't be async) so that
workspaceGitMetadataWatcherRefreshTasksByKey and
workspaceGitMetadataWatchersByKey are cancelled/stopped and then cleared by the
existing helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52b2ff55-92b8-4a5b-a4a6-d58e8c92697c
📒 Files selected for processing (9)
Packages/CmuxFileWatch/Package.swiftPackages/CmuxFileWatch/README.mdPackages/CmuxFileWatch/Sources/CmuxFileWatch/FileSystemEventStream.swiftPackages/CmuxFileWatch/Sources/CmuxFileWatch/FileWatchClock.swiftPackages/CmuxFileWatch/Sources/CmuxFileWatch/RecursivePathWatcher.swiftPackages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swiftSources/TabManager.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspacePullRequestSidebarTests.swift
Wave-2 infrastructure leaf of the modular refactor. Pulls the FSEvents-backed path watcher out of TabManager into a standalone, git-agnostic CmuxFileWatch package, redesigned onto Swift 6 concurrency. No behavior change intended. The extracted code has no git knowledge: it just watches a set of paths and yields coalesced events. The git-specific path resolution (which paths matter, dirty-state computation) deliberately stays in TabManager, bound for the Wave-3 CmuxSidebarGit domain package that will depend on this leaf. This mirrors the CmuxProcess infra-leaf precedent (one outside-world capability per package). - `actor RecursivePathWatcher` exposes coalesced changes as `AsyncStream<Void>` instead of the old `init?(descriptor:onChange:)` completion-handler callback. Takes `paths: [String]` directly and exposes `watchedPaths` for caller dedup; the git-flavored `Descriptor` is gone. - The leading-edge throttle moves off the shared-queue `DispatchWorkItem` onto a stored, cancel-on-stop `Task` driven by an injected `FileWatchClock` (the bounded/cancellable delay carve-out, justified inline). Behavior preserved: first event in a window arms one delay; events during the window coalesce; it reacts during a storm rather than waiting for changes to stop. - `FileSystemEventStream` keeps the hardened `FSEventStream` internals: one shared serial queue to bound threads, synchronous queue-confined teardown so the stream can't leak before deinit. The FSEvents context is passed unretained (lifetime guaranteed by invalidate-before-deinit), which lets the callback box and both retain/release C trampolines be deleted, leaving one event callback. - TabManager imports the package, resolves watched paths, stores a per-key consumer Task that drains `watcher.events` on the MainActor, and tears it down alongside the watcher. Net -262 lines from TabManager. - The old DEBUG semaphore + Thread.sleep storm helper (and its XCTest) is replaced by a deterministic Swift Testing test that proves leading-edge coalescing with a gate clock and no real waiting. Gates green from real logs: swift build + 4 swift tests; cmux and cmux-unit both BUILD SUCCEEDED (GlobalISel workaround). pbxproj wired into both targets; check-pbxproj passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
azooz2003-bit
force-pushed
the
feat-cmux-workspace-git
branch
from
June 3, 2026 01:41
61136b4 to
adef166
Compare
Collaborator
Author
|
Thanks — addressed the two Major findings in
On the app-level smoke test for the TabManager bridge (nitpick): respectfully skipping. The consumer bridge lives inside the |
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/TabManager.swift (1)
1635-1645:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecompute watcher paths even when the directory is unchanged.
This fast-path makes the source directory the only invalidation key, but
workspaceGitMetadataWatchedPaths(for:)also depends on the repo’s current git metadata layout. After a checkout that adds/removes submodules or changes included config paths, the watcher keeps the old path set and never subscribes to the new metadata locations, so later filesystem events can stop refreshing the sidebar state.Suggested fix
- if workspaceGitMetadataWatcherSourceDirectoryByKey[key] == directory, - workspaceGitMetadataWatchersByKey[key] != nil { - if workspaceGitMetadataWatcherDescriptorRequestsByKey[key]?.directory != directory { - workspaceGitMetadataWatcherDescriptorRequestsByKey.removeValue(forKey: key) - } - return - } - if workspaceGitMetadataWatcherDescriptorRequestsByKey[key]?.directory == directory { return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TabManager.swift` around lines 1635 - 1645, The current early-return uses only workspaceGitMetadataWatcherSourceDirectoryByKey[key] to skip work, but workspaceGitMetadataWatchedPaths(for:) can change without the directory changing (e.g., submodule/config layout updates); modify the logic in the block that references workspaceGitMetadataWatcherSourceDirectoryByKey, workspaceGitMetadataWatchersByKey and workspaceGitMetadataWatcherDescriptorRequestsByKey so that you still recompute the watched paths and update/replace the descriptor request and watcher when workspaceGitMetadataWatchedPaths(for: key) yields a different set (or when the descriptor’s paths differ), instead of returning solely because the source directory is equal; in short, remove or bypass the fast-path return and compare/update based on the computed watched paths (using workspaceGitMetadataWatchedPaths(for:)) to refresh subscriptions when the repo metadata layout changes.
♻️ Duplicate comments (1)
Packages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swift (1)
12-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
GateClock.sleep(for:)cancellation-aware and cover stop-while-throttled.Line 12 parks
sleep(for:)on a non-throwing continuation, so cancellingRecursivePathWatcher’sthrottleTasknever wakes the fake clock the waySystemFileWatchClock.sleep(for:)does. That leaves the test seam out of sync with production behavior and misses the importantstop()-while-throttled path.Please switch
GateClockto a throwing, cancellation-aware continuation and add a test that stops the watcher afterwaitForSleeper()but beforereleaseOne(). That closes the gap and catches regressions where a cancelled throttle still flushes or leaves a parked task behind.Also applies to: 62-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swift` around lines 12 - 18, GateClock.sleep(for:) currently uses a non-throwing continuation and must be changed to a throwing, cancellation-aware continuation like SystemFileWatchClock.sleep(for:) so cancelling RecursivePathWatcher’s throttleTask actually wakes the fake clock; update GateClock.sleep(for:) to use withCheckedThrowingContinuation (CheckedContinuation<Void, Error>), store/resume continuations with resume(throwing: CancellationError()) on task cancellation and resume normally on releaseOne(), and ensure arrivalWaiters behavior is preserved. Then add a test that starts a RecursivePathWatcher, calls waitForSleeper(), calls stop() before releaseOne(), and asserts the throttleTask was cancelled and no flush/parked task remains (no further callbacks after stop()), to cover the stop-while-throttled path and mirror production behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/WorkspacePullRequestSidebarTests.swift`:
- Around line 1635-1640: The comment incorrectly names the package/tests; update
the comment text that currently references CmuxWorkspaceGit and
WorkspaceGitMetadataWatcherTests to instead reference CmuxFileWatch and
RecursivePathWatcherTests.burstCoalescesAndThrottleRearms so the note points to
the actual deterministic replacement test suite; locate the comment block
containing the symbols CmuxWorkspaceGit and WorkspaceGitMetadataWatcherTests and
replace them with CmuxFileWatch and
RecursivePathWatcherTests.burstCoalescesAndThrottleRearms respectively.
---
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 1635-1645: The current early-return uses only
workspaceGitMetadataWatcherSourceDirectoryByKey[key] to skip work, but
workspaceGitMetadataWatchedPaths(for:) can change without the directory changing
(e.g., submodule/config layout updates); modify the logic in the block that
references workspaceGitMetadataWatcherSourceDirectoryByKey,
workspaceGitMetadataWatchersByKey and
workspaceGitMetadataWatcherDescriptorRequestsByKey so that you still recompute
the watched paths and update/replace the descriptor request and watcher when
workspaceGitMetadataWatchedPaths(for: key) yields a different set (or when the
descriptor’s paths differ), instead of returning solely because the source
directory is equal; in short, remove or bypass the fast-path return and
compare/update based on the computed watched paths (using
workspaceGitMetadataWatchedPaths(for:)) to refresh subscriptions when the repo
metadata layout changes.
---
Duplicate comments:
In
`@Packages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swift`:
- Around line 12-18: GateClock.sleep(for:) currently uses a non-throwing
continuation and must be changed to a throwing, cancellation-aware continuation
like SystemFileWatchClock.sleep(for:) so cancelling RecursivePathWatcher’s
throttleTask actually wakes the fake clock; update GateClock.sleep(for:) to use
withCheckedThrowingContinuation (CheckedContinuation<Void, Error>), store/resume
continuations with resume(throwing: CancellationError()) on task cancellation
and resume normally on releaseOne(), and ensure arrivalWaiters behavior is
preserved. Then add a test that starts a RecursivePathWatcher, calls
waitForSleeper(), calls stop() before releaseOne(), and asserts the throttleTask
was cancelled and no flush/parked task remains (no further callbacks after
stop()), to cover the stop-while-throttled path and mirror production behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 936f3b33-e337-4096-96f4-bf5d135c1875
📒 Files selected for processing (9)
Packages/CmuxFileWatch/Package.swiftPackages/CmuxFileWatch/README.mdPackages/CmuxFileWatch/Sources/CmuxFileWatch/FileSystemEventStream.swiftPackages/CmuxFileWatch/Sources/CmuxFileWatch/FileWatchClock.swiftPackages/CmuxFileWatch/Sources/CmuxFileWatch/RecursivePathWatcher.swiftPackages/CmuxFileWatch/Tests/CmuxFileWatchTests/RecursivePathWatcherTests.swiftSources/TabManager.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspacePullRequestSidebarTests.swift
Comment on lines
+1635
to
+1640
| // The watcher's leading-edge coalescing (a sustained FSEvents burst yields | ||
| // one refresh per window rather than waiting for the repo to go quiet) is now | ||
| // verified deterministically in CmuxWorkspaceGit's package tests | ||
| // (WorkspaceGitMetadataWatcherTests.burstCoalescesAndThrottleRearms), with an | ||
| // injected clock and no real waiting, instead of the previous timing-based | ||
| // semaphore storm helper. |
There was a problem hiding this comment.
Correct stale package/test names in the replacement-note comment.
Line 1637–1639 references CmuxWorkspaceGit / WorkspaceGitMetadataWatcherTests, but the deterministic replacement in this PR is under CmuxFileWatch (RecursivePathWatcherTests.burstCoalescesAndThrottleRearms). Please align the comment so future maintenance points to the right suite.
Suggested edit
- // verified deterministically in CmuxWorkspaceGit's package tests
- // (WorkspaceGitMetadataWatcherTests.burstCoalescesAndThrottleRearms), with an
+ // verified deterministically in CmuxFileWatch package tests
+ // (RecursivePathWatcherTests.burstCoalescesAndThrottleRearms), with an📝 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.
Suggested change
| // The watcher's leading-edge coalescing (a sustained FSEvents burst yields | |
| // one refresh per window rather than waiting for the repo to go quiet) is now | |
| // verified deterministically in CmuxWorkspaceGit's package tests | |
| // (WorkspaceGitMetadataWatcherTests.burstCoalescesAndThrottleRearms), with an | |
| // injected clock and no real waiting, instead of the previous timing-based | |
| // semaphore storm helper. | |
| // The watcher's leading-edge coalescing (a sustained FSEvents burst yields | |
| // one refresh per window rather than waiting for the repo to go quiet) is now | |
| // verified deterministically in CmuxFileWatch package tests | |
| // (RecursivePathWatcherTests.burstCoalescesAndThrottleRearms), with an | |
| // injected clock and no real waiting, instead of the previous timing-based | |
| // semaphore storm helper. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/WorkspacePullRequestSidebarTests.swift` around lines 1635 - 1640,
The comment incorrectly names the package/tests; update the comment text that
currently references CmuxWorkspaceGit and WorkspaceGitMetadataWatcherTests to
instead reference CmuxFileWatch and
RecursivePathWatcherTests.burstCoalescesAndThrottleRearms so the note points to
the actual deterministic replacement test suite; locate the comment block
containing the symbols CmuxWorkspaceGit and WorkspaceGitMetadataWatcherTests and
replace them with CmuxFileWatch and
RecursivePathWatcherTests.burstCoalescesAndThrottleRearms respectively.
Adds the DispatchSource-backed single-path counterpart to RecursivePathWatcher: `FileWatcher(path:throttle:clock:)` exposing coalesced changes as `AsyncStream<Void>`, with nearest-existing-ancestor recovery (watches the closest existing ancestor and migrates the source closer as directories appear, reattaching the path source to the current inode) and an optional leading-edge throttle on the shared `FileWatchClock` seam. Sources attach synchronously in init (no missed-events window); a Sendable raw-event continuation keeps the DispatchSource handlers off the actor so creation stays in-init. Generalizes the bespoke single-file watchers scattered across the app so they can converge here. 4 new tests (real-FS write, create-after-start, ancestor recovery, stop) join the existing watcher suite; all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces five hand-rolled DispatchSource file watchers with CmuxFileWatch. Each consumer now constructs a FileWatcher and drains its events stream on the MainActor; the bespoke watcher classes and their fd/source/debounce bookkeeping are deleted. - FileExplorerStore: FileExplorerDirectoryWatcher -> FileWatcher (0.3s debounce becomes a leading-edge throttle). - MarkdownPanel: file+directory DispatchSource watchers (incl. the nearest-ancestor walk) -> FileWatcher, whose recovery subsumes that logic. - KeyboardShortcutSettingsFileStore: ShortcutSettingsFileWatcher (primary + fallbacks) -> array of FileWatchers + consumer tasks. - CmuxConfigStore: global-config and per-hook-file watchers -> FileWatcher (reattach/fallback/createDirectory machinery removed). The local-config watcher stays bespoke: it does search-directory *path re-resolution*, not reload-on-change. - CmuxSettings JSONConfigStore: its JSONConfigFileWatcher (already an actor+AsyncStream watcher, the model FileWatcher was based on) is replaced by FileWatcher and deleted; CmuxSettings now takes a path dependency on CmuxFileWatch. Deliberately NOT migrated (not file-reload watchers): TerminalController's socket-path liveness monitor (generation/identity state machine on the socket auth path; belongs to the CmuxControlSocket extraction) and FeedCoordinator / CMUXWorkstream process-exit sources. Gates: package swift test (8); cmux + cmux-unit BUILD SUCCEEDED (GlobalISel workaround); CmuxSettings standalone build + tests green (the pre-existing SettingCatalog duplicate-key test failure also fails on main, unrelated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Solidifies the behaviors the migrated consumers depend on: watching a directory target (FileExplorer), reattaching across an atomic inode replacement (MarkdownPanel / JSONConfigStore saves), and delivering events under a throttle. 11 tests total, all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
azooz2003-bit
changed the title
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)
Packages/CmuxSettings/Sources/CmuxSettings/Stores/JSONConfigStore.swift (1)
112-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister the subscriber before reading the initial value.
JSONConfigStore.values(for:)reads/yieldsinitialbeforeawait self.addSubscriber(...); a file change handled in that gap can be broadcast to existing subscribers while this one isn’t registered yet, so the stream may miss the first external edit after iteration starts (breaking “current value, then every later change”).Suggested fix
- let initial = await self.value(for: key) - continuation.yield(initial) - let id = UUID() // bufferingNewest(1): the signal carries no payload, so under // burst file changes we only care that *something* changed. // Dropping intermediate signals is correct because the typed // value is re-read on every consumed signal and deduped below. // Bounded buffering prevents unbounded growth under load. let (signal, signalContinuation) = AsyncStream<Void>.makeStream( bufferingPolicy: .bufferingNewest(1) ) await self.addSubscriber(id: id, continuation: signalContinuation) + + let initial = await self.value(for: key) + continuation.yield(initial)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/CmuxSettings/Sources/CmuxSettings/Stores/JSONConfigStore.swift` around lines 112 - 124, In values(for:) register the subscriber before reading/yielding the initial value so you don't miss an external change that arrives between the read and registration: call await self.addSubscriber(id: id, continuation: signalContinuation) (using the same UUID id and AsyncStream/continuation created) before invoking let initial = await self.value(for: key) and continuation.yield(initial), ensuring the new subscriber is registered prior to delivering the current value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/CmuxSettings/README.md`:
- Around line 172-174: The README header still claims "Zero non-Foundation
dependencies" but Package.swift now lists a dependency on CmuxFileWatch; update
the README.md header/landing copy to reflect that CmuxSettings depends on
CmuxFileWatch (or otherwise remove the zero-dependencies claim). Locate the
top-of-file header text in README.md and replace the incorrect claim with an
accurate short description (e.g., mention the CmuxFileWatch dependency or simply
remove the zero-dependencies sentence) so it matches the Package.swift
dependency list.
---
Outside diff comments:
In `@Packages/CmuxSettings/Sources/CmuxSettings/Stores/JSONConfigStore.swift`:
- Around line 112-124: In values(for:) register the subscriber before
reading/yielding the initial value so you don't miss an external change that
arrives between the read and registration: call await self.addSubscriber(id: id,
continuation: signalContinuation) (using the same UUID id and
AsyncStream/continuation created) before invoking let initial = await
self.value(for: key) and continuation.yield(initial), ensuring the new
subscriber is registered prior to delivering the current value.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75df848e-dcc3-41a7-b8e6-6f380d2d97d2
📒 Files selected for processing (11)
Packages/CmuxFileWatch/Sources/CmuxFileWatch/FileWatcher.swiftPackages/CmuxFileWatch/Tests/CmuxFileWatchTests/FileWatcherTests.swiftPackages/CmuxSettings/Package.swiftPackages/CmuxSettings/README.mdPackages/CmuxSettings/Sources/CmuxSettings/CmuxSettings.docc/CmuxSettings.mdPackages/CmuxSettings/Sources/CmuxSettings/Stores/JSONConfigFileWatcher.swiftPackages/CmuxSettings/Sources/CmuxSettings/Stores/JSONConfigStore.swiftSources/CmuxConfig.swiftSources/FileExplorerStore.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/MarkdownPanel.swift
💤 Files with no reviewable changes (2)
- Packages/CmuxSettings/Sources/CmuxSettings/CmuxSettings.docc/CmuxSettings.md
- Packages/CmuxSettings/Sources/CmuxSettings/Stores/JSONConfigFileWatcher.swift
Comment on lines
+172
to
+174
| `CmuxFileWatch.FileWatcher` and fans out file-change events to per-subscriber | ||
| bounded signal streams (no `N × parse` work under burst changes). File | ||
| watching itself lives in the `CmuxFileWatch` package. |
There was a problem hiding this comment.
Update the README’s dependency claim.
This section correctly says file watching now lives in CmuxFileWatch, but the README header still says CmuxSettings has “Zero non-Foundation dependencies.” That is no longer true after the new package dependency in Packages/CmuxSettings/Package.swift, so the landing description now misleads package consumers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Packages/CmuxSettings/README.md` around lines 172 - 174, The README header
still claims "Zero non-Foundation dependencies" but Package.swift now lists a
dependency on CmuxFileWatch; update the README.md header/landing copy to reflect
that CmuxSettings depends on CmuxFileWatch (or otherwise remove the
zero-dependencies claim). Locate the top-of-file header text in README.md and
replace the incorrect claim with an accurate short description (e.g., mention
the CmuxFileWatch dependency or simply remove the zero-dependencies sentence) so
it matches the Package.swift dependency list.
azooz2003-bit
dismissed
coderabbitai[bot]’s stale review
June 3, 2026 04:12
Dismissing per repo owner instruction to merge: gating CI is green and the remaining CodeRabbit findings are minor doc nits (stale comment names; CmuxSettings README dependency line), not behavior issues. Bot reviewers are advisory on this PR.
azooz2003-bit
added a commit
that referenced
this pull request
Jun 3, 2026
Carryover doc nit from PR #5244: CmuxSettings has depended on CmuxFileWatch since the file-watcher consolidation. Doc-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
azooz2003-bit
added a commit
that referenced
this pull request
Jun 4, 2026
…vice + PullRequestProbeService) (#5277) * CmuxGit stage 1: extract local-git metadata service from TabManager Lift the pure on-disk git-metadata domain out of TabManager into a new Layer-2 package, CmuxGit, behind a stateless GitMetadataService actor: - ResolvedGitRepository / GitWorkspaceMetadata value types. - workspaceMetadata(for:) (branch, dirty, signatures), watchedPaths(for:) (incl. submodule gitlinks), repositorySlugs(forDirectory:). - ~30 nonisolated static parsing helpers: repository resolution (.git file + commondir), HEAD/ref reads, git config include/includeIf + glob, index v2/v3/v4 parse (prefix compression, assume-unchanged/skip-worktree), stat-based dirty detection, GitHub slug parsing. TabManager now injects a GitMetadataService and calls it at the three entry points (watched paths, repo slugs, initial metadata snapshot); the GitMetadataService replaces the manual Task.detached offloading. ~1,120 lines of helpers + value types removed from TabManager (11,506 -> 10,385). 37 package tests (repo resolution, root-escape stop, branch/detached HEAD, dirty detection, index v2/v4, content-signature stability, watched paths + submodule gitlinks, slug parsing, full include/includeIf rules) migrated from the app test target where they tested the now-lifted statics. Remaining WorkspacePullRequestSidebarTests integration tests exercise the cutover end-to-end through TabManager. Fixes the carryover stale-comment doc nit (CmuxWorkspaceGit/WorkspaceGitMetadataWatcherTests -> CmuxGit/CmuxFileWatch). cmux + cmux-unit build green; swift build + swift test green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxGit: organize sources into Model/ and Parsing/ folders Pure file moves (git mv), no code changes. SwiftPM scans the target recursively, so subfolders are organizational only (one module, no namespacing/access-control effect): GitMetadataService.swift stays at the target root as the public entry point, value types move to Model/, the nonisolated-static parsing extensions move to Parsing/, and test fixtures move to Tests/CmuxGitTests/Fixtures/. swift build + 37 swift tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxGit: make GitMetadataService a stateless struct, not an actor The service holds no mutable shared state (every read is a pure function of the directory argument), so an actor's serial executor protects nothing and only hurts: it would funnel independent per-workspace reads through one executor and, since the methods have no internal await, run them strictly sequentially. The original TabManager code ran these in parallel via Task.detached/@Concurrent. Make it a `Sendable` struct with `async` methods instead. A nonisolated async method runs on the global concurrent executor (SE-0338), so callers still get off-main offloading (replacing the hand-rolled Task.detached) AND concurrent reads for independent repositories run in parallel again. All call sites already await, so they're unchanged. Promote to an actor only if an in-memory cache is added later (real shared state would then justify the serialization). swift build + 37 swift tests, cmux, and cmux-unit all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxGit: mark GitMetadataService reads @Concurrent (guarded for <6.2) The reads do blocking filesystem work and must stay off the caller's thread. A bare nonisolated async method runs off-actor today (SE-0338), but under Swift 6.2's NonisolatedNonsendingByDefault it would instead run on the caller's actor (the main thread, when called from @mainactor TabManager). @Concurrent makes the off-main / parallel execution an explicit property of the API, robust to that default. Guarded with #if compiler(>=6.2) so the package still compiles on older toolchains (where the bare async already runs off-main); mirrors the existing githubRepositorySlugs(directory:) pattern in TabManager. swift build + 37 swift tests, cmux, and cmux-unit all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxGit: bump swift-tools-version to 6.2, drop the @Concurrent compiler guard tools-version 6.2 requires a >=6.2 toolchain to build, so the #if compiler(>=6.2) guard around @Concurrent is always true and is removed (the attribute is now unconditional). CI builds the cmux project on Xcode 26 (the tests job already compiled @Concurrent), so it resolves a 6.2 manifest fine. swift build + 37 tests, cmux, and cmux-unit green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxGit: drop @Concurrent + the 6.2 bump; use plain nonisolated async CI's tests/tests-build-and-lag/ui-regressions jobs run on warp-macos-15 (Xcode 16.4 / Swift 6.1), which cannot resolve a swift-tools-version 6.2 manifest ("package 'cmuxgit' is using Swift tools version 6.2.0 but the installed version is 6.1.0"). Only release-build (warp-macos-26 / Xcode 26) has 6.2. @Concurrent (which needs the 6.2 compiler, hence the tools bump + #if guard) is unnecessary here: a struct's plain `async` methods are nonisolated, and a nonisolated async function already runs on the global concurrent executor off the caller's actor (SE-0338) — identical to @Concurrent under the semantics we compile with (we don't enable NonisolatedNonsendingByDefault anywhere). So drop @Concurrent and revert the manifest to 6.0. Same off-main / parallel behavior, resolves on Swift 6.1. A doc note flags that adopting NonisolatedNonsendingByDefault later would require re-adding @Concurrent. swift build + 37 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxGit: spell nonisolated explicitly and pin the off-main execution contract Make the three reads `public nonisolated func ... async` (the keyword was implicit on a struct, but the whole design hinges on it — say it). Add a small internal probe + a @mainactor pinning test proving the SE-0338 contract the service relies on: a nonisolated async method awaited from the main actor runs on the global concurrent executor, not the main thread. There is no `nonisolated(sending)` spelling — the off-actor behavior IS the current default for bare nonisolated async; `nonisolated(nonsending)` (caller's actor) only becomes the default under NonisolatedNonsendingByDefault, and if that mode is ever adopted this test fails and points at `@concurrent` as the fix (pthread_main_np because Thread.isMainThread is noasync). swift build + 38 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxSettings: fix stale "zero non-Foundation dependencies" README claim Carryover doc nit from PR #5244: CmuxSettings has depended on CmuxFileWatch since the file-watcher consolidation. Doc-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxGit stage 2: extract the GitHub PR probe pipeline from TabManager Lift sub-domain B into PullRequestProbeService, a stateless Sendable struct (same shape as GitMetadataService: nonisolated async, off the caller's actor, parallel): candidate-seed slug resolution (via GitMetadataService), concurrent REST repo fetch + per-branch lookups with the caller-owned repo cache, GH_TOKEN/`gh auth token` auth via injected CommandRunning, and the pure selection/policy logic (preferred PR, stale-merged filtering, cache/refresh policy). Value types (WorkspacePullRequest* family, GitHubPullRequestProbeItem) move to the package; new PullRequestStatus enum bridges the app's SidebarPullRequestStatus by raw value, so the apply path is unchanged. TabManager keeps the orchestration (probe state machines, per-key dictionaries, poll timers, the apply path, and ownership of the repo cache) and calls the service; commandRunner stays an init parameter and now feeds the service; a debugLog closure seam carries cmuxDebugLog in DEBUG builds. Another -840 lines (11,506 -> 9,557 across both stages). The DispatchSourceTimer poll loops are deliberately NOT redesigned in this PR (existing app-target orchestration; timing changes need their own dogfood). Tests: the 7 pure TabManagerPullRequestProbeTests migrate to the package (48 package tests total), plus new REST-decode (merged_at -> MERGED synthesis), branch-endpoint encoding, and result-resolution coverage. The old "not a url" malformed-URL fixture only failed URL(string:) on pre-macOS-14 SDK parsing; replaced with an empty string, malformed on every SDK. ci.yml's package-test list gains CmuxFileWatch, CmuxGit, and CmuxProcess - their suites were compiled but never executed in CI (each verified passing standalone). swift build + 48 swift tests, cmux, and cmux-unit all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * CmuxGit: strengthen two package tests (CodeRabbit) returnsNilOutsideAnyRepository asserts nil strictly (the temp dir is never inside a repo), and the includeIf gitdir: assertion matches against the git directory path, which is what git evaluates the condition against. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
7 tasks done
2 of 6 tasks
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wave-2 infrastructure leaf of the modular refactor. Pulls the FSEvents-backed path watcher out of
TabManagerinto a standalone, git-agnosticCmuxFileWatchpackage, redesigned onto Swift 6 concurrency. No behavior change intended.Scope: generic infra, not a git slice
The extracted code has no git knowledge — it just watches a set of paths and yields coalesced events. The git-specific path resolution (which paths matter, dirty-state computation) deliberately stays in
TabManager, bound for the Wave-3CmuxSidebarGitdomain package that will depend on this leaf. This mirrors theCmuxProcessinfra-leaf precedent (one outside-world capability per package), andCmuxSettings'JSONConfigFileWatcheris a candidate second consumer to migrate onto it later.Redesign at the seam
actor RecursivePathWatcherexposes coalesced changes asAsyncStream<Void>instead of the oldinit?(descriptor:onChange:)completion-handler callback (forbidden in new package code). Takespaths: [String]directly and exposeswatchedPathsfor caller dedup; the git-flavoredDescriptoris gone.DispatchWorkItemonto a stored, cancel-on-stopTaskdriven by an injectedFileWatchClock(the bounded/cancellable delay carve-out, justified inline). Behavior preserved: first event in a window arms one delay; events during the window coalesce; it reacts during a storm rather than waiting for changes to stop.FileSystemEventStreamkeeps the hardenedFSEventStreaminternals: one shared serial queue to bound threads, synchronous queue-confined teardown so the stream can't leak beforedeinit. The FSEvents context is now passed unretained (lifetime guaranteed by invalidate-before-deinit), which let me delete the callback box and both retain/release C trampolines, leaving only the one irreducible event callback.TabManagerimports the package, resolves watched paths, stores a per-key consumerTaskthat drainswatcher.eventson the MainActor, and tears it down alongside the watcher. Net -262 lines fromTabManager.Tests
The old DEBUG semaphore +
Thread.sleepstorm helper (and itstestGitMetadataWatcherRefreshesDuringSustainedEventStormXCTest) is replaced by a deterministic Swift Testing test that proves leading-edge coalescing with a gate clock and no real waiting (burstCoalescesAndThrottleRearms). The submodule-paths test stays (it exercises the git path-resolution builder, which stayed inTabManager).Gates (all green from real logs)
swift build+ 4swift test(package)cmuxscheme: BUILD SUCCEEDEDcmux-unitscheme: BUILD SUCCEEDED (GlobalISel workaround)check-pbxproj.shpasses; package linked into bothcmuxandcmux-unitAuthor: @azooz2003-bit (via Aziz Albahar)
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features
Documentation
Tests
Review feedback addressed (CodeRabbit / Greptile)
FSEventStreamin aTaskinsideinit, which (a) could miss events in the gap before the task ran (kFSEventStreamEventIdSinceNow) and (b) swallowed creation failures (TabManager cached a silent watcher). Reworked so the stream is created synchronously ininit?and returnsnilon failure, matching the original watcher. The FSEvents sink forwards into a private raw-eventAsyncStream(capturing a Sendable continuation, not the actor), which is what lets creation happen in-init; a single actor-isolated pump drains that raw stream and applies the leading-edge clock throttle.Task { await stop() }).TabManagernow drops the watcher reference, which runs the watcher'sdeinitsynchronously and invalidates theFSEventStreamon its shared queue before returning — no window where the old and new streams co-watch. The consumer task captures the events stream (not the watcher), so removal is the last reference.swift test(4),cmux+cmux-unitBUILD SUCCEEDED; behavior screenshot-verified end-to-end (sidebar git dirty indicator updates on file change) on the tagged build.Update: consolidated all file-reload watchers onto CmuxFileWatch (per request)
Beyond the leaf extraction, this PR now also adds a single-file/dir
FileWatcherto
CmuxFileWatchand migrates every hand-rolled file-reload watcher in the apponto it, deleting ~350 net lines of bespoke
DispatchSourcecode:FileExplorerStore(FileExplorerDirectoryWatcher)MarkdownPanel(file + nearest-ancestor directory watchers)KeyboardShortcutSettingsFileStore(ShortcutSettingsFileWatcherprimary + fallbacks)CmuxConfigStoreglobal-config + per-hook-file watchers (local-config watcher kept — it does search-directory path re-resolution, not reload-on-change)CmuxSettingsJSONConfigStore(JSONConfigFileWatcherreplaced + deleted;CmuxSettingsnow depends onCmuxFileWatch)Not migrated, by design (not file-reload watchers):
TerminalController's socket-path liveness monitor (auth-path state machine, slated forCmuxControlSocket) and process-exit kqueue sources (FeedCoordinator,CMUXWorkstream).FileWatcheradds nearest-existing-ancestor recovery (subsumes MarkdownPanel's ancestor walk) and an optional leading-edge throttle on the sharedFileWatchClock.Tests + verification
CmuxFileWatch: 11 tests (write→event, create-after-start, ancestor recovery, directory target, atomic inode replace, throttled yield, stop) — all green.cmux+cmux-unitBUILD SUCCEEDED with the cross-packageCmuxSettings → CmuxFileWatchdependency.RecursivePathWatcher); MarkdownPanel auto-reloads on external edit (FileWatcher).