Repository navigation
Coalesce sidebar git metadata probes - #5402
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
📝 WalkthroughWalkthroughThis PR adds reusable hex-encoding helpers for git index parsing and implements a protocol-backed, concurrency-limited, coalescing workspace git metadata snapshot pipeline in TabManager with tests validating probe coalescing. ChangesGit Index Hex Encoding Refactor
Concurrency-Limited Batched Git Metadata Snapshot Probing
Sequence DiagramsequenceDiagram
participant TabManager
participant Limiter as WorkspaceGitMetadataProbeLimiter
participant Reader as WorkspaceGitMetadataReader
participant Storage as SnapshotQueue
rect rgba(100, 149, 237, 0.5)
Note over TabManager,Storage: First probe for directory
TabManager->>Storage: enqueue probe request
TabManager->>Limiter: request permit
Limiter-->>TabManager: permit granted
TabManager->>Reader: fetch workspace metadata(reader.workspaceMetadata(for:))
Reader-->>TabManager: return snapshot
end
rect rgba(144, 238, 144, 0.5)
Note over TabManager,Storage: Concurrent probes for same directory
TabManager->>Storage: enqueue additional probe request(s)
Note over Storage: requests coalesced into single fetch
end
rect rgba(255, 182, 193, 0.5)
Note over TabManager,Storage: Apply snapshot to queued probes
TabManager->>TabManager: apply snapshot to all queued probe keys
TabManager->>Limiter: release permit
Limiter-->>TabManager: permit available
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (13 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 |
|
To use Codex here, create a Codex account and connect to github. |
0aac5a4 to
6fc0594
Compare
![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)
Packages/CmuxGit/Sources/CmuxGit/Parsing/GitMetadataService+Index.swift (1)
270-275: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider applying the same hex encoding refactor to
gitIndexFileSignaturefor consistency.This function still uses the old
map { String(format: "%02x", $0) }.joined()pattern, which could be replaced withgitIndexHexString(data.suffix(20))for consistency and performance.♻️ Optional consistency refactor
- return data.suffix(20).map { String(format: "%02x", $0) }.joined() + return gitIndexHexString(data.suffix(20))🤖 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/CmuxGit/Sources/CmuxGit/Parsing/GitMetadataService`+Index.swift around lines 270 - 275, Replace the manual hex encoding in nonisolated static func gitIndexFileSignature with the shared helper: when reading the last 20 bytes (currently data.suffix(20)), pass that slice to gitIndexHexString(...) and return its result instead of using map { String(format: "%02x", $0) }. Keep the same guard behavior and return types; reference gitIndexFileSignature and gitIndexHexString to locate the change.
🤖 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/TabManagerUnitTests.swift`:
- Around line 98-103: waitForCallCount(_:) currently appends a checked
continuation to callCountWaiters with no cancellation path; make it cancellable
by using a throwing continuation (withCheckedThrowingContinuation) or a
withTaskCancellationHandler so that when the awaiting Task is cancelled you
remove the corresponding (expected, continuation) entry from callCountWaiters
and resume the continuation with a CancellationError. Update the function to
append the waiter atomically, and ensure both normal completion (when callCount
reaches expected) and cancellation resume the continuation and remove the waiter
to avoid retaining reader/expectations.
- Around line 822-829: After the existing waitForCondition that asserts all
panels' branches are "main", add a final assertion to ensure no additional
workspaceMetadata reads were started after release: assert observedCallCount ==
1 (or an equivalent idle check) to lock the coalescing contract; locate the test
block that calls reader.releaseAll(), the waitForCondition that checks panelIds
and workspace.panelGitBranches, and add the observedCallCount assertion
immediately after that branch assertion to prevent delayed duplicate probe
startups.
In `@Packages/CmuxGit/Sources/CmuxGit/Parsing/GitMetadataService`+Index.swift:
- Around line 204-212: The helper function gitIndexHexString is currently
package-visible but appears only used within this file; make its declaration
private to restrict scope by changing its signature to private nonisolated
static func gitIndexHexString<S: Sequence>(_ bytes: S) -> String where S.Element
== UInt8 so it documents internal usage and prevents accidental external
access—ensure there are no external callers before making it private.
In `@Sources/TabManager.swift`:
- Around line 2814-2844: The batching key is using expectedDirectory so panels
inside the same repo launch separate snapshot tasks; update the logic to resolve
the repository root and use that as the batching key instead of
expectedDirectory (e.g. call the existing workspaceMetadata(for:) or add a
lightweight repo-root resolver on WorkspaceGitMetadataReading to get repoRoot)
and then replace uses of
workspaceGitSnapshotRequestsByDirectory[expectedDirectory] and
workspaceGitSnapshotTasksByDirectory[expectedDirectory] with the repoRoot key so
all dirs in one repo share the same Task and the same
WorkspaceGitMetadataProbeLimiter permit; ensure
initialWorkspaceGitMetadataSnapshot(for: expectedDirectory, reader:) and any
reader calls use the resolved repoRoot or continue to pass the original dir as
needed but only the queue/task/limiter are keyed by repoRoot.
---
Outside diff comments:
In `@Packages/CmuxGit/Sources/CmuxGit/Parsing/GitMetadataService`+Index.swift:
- Around line 270-275: Replace the manual hex encoding in nonisolated static
func gitIndexFileSignature with the shared helper: when reading the last 20
bytes (currently data.suffix(20)), pass that slice to gitIndexHexString(...) and
return its result instead of using map { String(format: "%02x", $0) }. Keep the
same guard behavior and return types; reference gitIndexFileSignature and
gitIndexHexString to locate the change.
🪄 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: b2e51c72-c2a1-4040-baf6-89915edbce0b
📒 Files selected for processing (4)
Packages/CmuxGit/Sources/CmuxGit/Parsing/GitMetadataService+Index.swiftPackages/CmuxGit/Tests/CmuxGitTests/GitMetadataServiceTests.swiftSources/TabManager.swiftcmuxTests/TabManagerUnitTests.swift
| func waitForCallCount(_ expected: Int) async { | ||
| guard callCount < expected else { return } | ||
| await withCheckedContinuation { continuation in | ||
| callCountWaiters.append((expected, continuation)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Make waitForCallCount(_:) cancellable.
Line 813 starts an unstructured task that waits for call count 2 under an inverted expectation. When coalescing works, that count is never reached, so this helper leaves the task suspended forever because the stored continuation has no cancellation/removal path. That retains reader and the expectation past test completion.
Also applies to: 121-130
🤖 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/TabManagerUnitTests.swift` around lines 98 - 103,
waitForCallCount(_:) currently appends a checked continuation to
callCountWaiters with no cancellation path; make it cancellable by using a
throwing continuation (withCheckedThrowingContinuation) or a
withTaskCancellationHandler so that when the awaiting Task is cancelled you
remove the corresponding (expected, continuation) entry from callCountWaiters
and resume the continuation with a CancellationError. Update the function to
append the waiter atomically, and ensure both normal completion (when callCount
reaches expected) and cancellation resume the continuation and remove the waiter
to avoid retaining reader/expectations.
| workspaceGitSnapshotRequestsByDirectory[expectedDirectory, default: []].append( | ||
| WorkspaceGitSnapshotProbeRequest( | ||
| probeKey: probeKey, | ||
| isLastAttempt: isLastAttempt | ||
| ) | ||
| ) | ||
| guard workspaceGitSnapshotTasksByDirectory[expectedDirectory] == nil else { | ||
| #if DEBUG | ||
| cmuxDebugLog( | ||
| "workspace.gitProbe.joinSnapshot dir=\(expectedDirectory) " + | ||
| "queued=\(workspaceGitSnapshotRequestsByDirectory[expectedDirectory]?.count ?? 0)" | ||
| ) | ||
| #endif | ||
| return | ||
| } | ||
|
|
||
| let reader = workspaceGitMetadataReader | ||
| workspaceGitSnapshotTasksByDirectory[expectedDirectory] = Task.detached(priority: .utility) { [weak self] in | ||
| let didAcquirePermit = await WorkspaceGitMetadataProbeLimiter.shared.acquire() | ||
| guard didAcquirePermit else { return } | ||
| defer { | ||
| Task { | ||
| await WorkspaceGitMetadataProbeLimiter.shared.release() | ||
| } | ||
| } | ||
|
|
||
| guard !Task.isCancelled else { return } | ||
| let snapshot = await Self.initialWorkspaceGitMetadataSnapshot( | ||
| for: expectedDirectory, | ||
| reader: reader | ||
| ) |
There was a problem hiding this comment.
Batch by repository root, not panel cwd.
This queue is keyed on expectedDirectory, but workspaceMetadata(for:) resolves the enclosing repository before it reads index/head state. Panels at /repo and /repo/subdir therefore still launch separate snapshot tasks and consume separate limiter permits, so the common multi-panel same-repo storm is not actually coalesced here. Key the batch/limiter on resolved repository identity (or add a lightweight repo-root resolver to WorkspaceGitMetadataReading) so all directories inside one repo share a single snapshot read.
Also applies to: 3166-3171
🤖 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 2814 - 2844, The batching key is using
expectedDirectory so panels inside the same repo launch separate snapshot tasks;
update the logic to resolve the repository root and use that as the batching key
instead of expectedDirectory (e.g. call the existing workspaceMetadata(for:) or
add a lightweight repo-root resolver on WorkspaceGitMetadataReading to get
repoRoot) and then replace uses of
workspaceGitSnapshotRequestsByDirectory[expectedDirectory] and
workspaceGitSnapshotTasksByDirectory[expectedDirectory] with the repoRoot key so
all dirs in one repo share the same Task and the same
WorkspaceGitMetadataProbeLimiter permit; ensure
initialWorkspaceGitMetadataSnapshot(for: expectedDirectory, reader:) and any
reader calls use the resolved repoRoot or continue to pass the original dir as
needed but only the queue/task/limiter are keyed by repoRoot.
Greptile SummaryThis PR coalesces git metadata initial snapshot reads in the sidebar so multiple panels pointing at the same directory share a single in-flight index scan, bounded by a new two-permit async
Confidence Score: 4/5Safe to merge with awareness of two open concurrency issues in The coalescing logic itself is solid: snapshot tasks are correctly launched once per directory, fanned out to all queued panels via Sources/TabManager.swift — specifically Important Files Changed
Sequence DiagramsequenceDiagram
participant TM as TabManager (MainActor)
participant LIM as WorkspaceGitMetadataProbeLimiter (actor)
participant TASK as Task.detached (utility)
participant RDR as WorkspaceGitMetadataReader
Note over TM: Panel A, B, C all in /repo
TM->>TM: enqueueSnapshotRequest(panelA, /repo)
TM->>TASK: create snapshot task for /repo
TM->>TM: enqueueSnapshotRequest(panelB, /repo)
Note over TM: task exists → join queue
TM->>TM: enqueueSnapshotRequest(panelC, /repo)
Note over TM: task exists → join queue
TASK->>LIM: acquire() [blocks if 2 already active]
LIM-->>TASK: true (permit granted)
TASK->>RDR: workspaceMetadata(for: /repo)
RDR-->>TASK: GitWorkspaceMetadata
TASK->>TM: "MainActor.run { applyBatch(snapshot, /repo) }"
TM->>TM: removeTask(/repo), dequeue [A,B,C]
TM->>TM: applySnapshot → panelA
TM->>TM: applySnapshot → panelB
TM->>TM: applySnapshot → panelC
TASK->>LIM: release() [via defer Task]
Note over LIM: activeCount-- or hand to next waiter
Reviews (2): Last reviewed commit: "fix: coalesce sidebar git metadata probe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func cancelWaiter(id: UUID) { | ||
| if let index = waiters.firstIndex(where: { $0.id == id }) { | ||
| let waiter = waiters.remove(at: index) | ||
| waiter.continuation.resume(returning: false) | ||
| } else { | ||
| cancelledWaiterIds.insert(id) | ||
| } | ||
| } |
There was a problem hiding this comment.
cancelledWaiterIds
There is a race window where release() can dequeue a waiter from waiters and resume its continuation with true before the onCancel-spawned cancelWaiter task runs on the actor. When cancelWaiter eventually executes, it finds the ID absent from waiters and inserts it into cancelledWaiterIds. That ID is now permanently stale: release() only cleans cancelledWaiterIds entries for IDs it encounters inside the waiters loop, so an ID for a waiter that was already dequeued is never evicted. Under a probe storm followed by rapid panel closes, cancelledWaiterIds can accumulate unboundedly.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| actor WorkspaceGitMetadataProbeLimiter { | ||
| static let shared = WorkspaceGitMetadataProbeLimiter(limit: 2) |
There was a problem hiding this comment.
WorkspaceGitMetadataReading was correctly made injectable (and the test stubs it out), but WorkspaceGitMetadataProbeLimiter.shared is a module-level singleton referenced directly inside the Task.detached body. This means the limiter's 2-permit semantics cannot be varied or isolated in tests — all tests share its live state. If two test cases that exercise the probe path run concurrently, they compete for the same 2 permits. Passing the limiter through TabManager's initializer (with the shared instance as the default) would mirror the pattern already applied to workspaceGitMetadataReader and eliminate the shared mutable singleton.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/TabManager.swift`:
- Around line 2871-2884: The current removeWorkspaceGitSnapshotRequest(for:) is
inefficient because it scans all keys; add and use a reverse index (e.g.,
workspaceGitSnapshotDirectoryByProbeKey: [WorkspaceGitProbeKey: String]) that
maps each WorkspaceGitProbeKey to its directory when requests are enqueued,
update that mapping wherever requests are added and cleared, and change
removeWorkspaceGitSnapshotRequest(for:) to look up the directory from
workspaceGitSnapshotDirectoryByProbeKey, fetch and mutate
workspaceGitSnapshotRequestsByDirectory[directory] to remove the matching probe,
remove the directory entries from workspaceGitSnapshotRequestsByDirectory and
workspaceGitSnapshotTasksByDirectory and cancel the task if empty, and finally
remove the probeKey from the reverse index; ensure all enqueue/clear points
maintain the reverse map to keep consistency.
- Around line 2831-2860: The detached Task can still run its MainActor closure
after cancellation and clobber a newer batch; add a cancellation guard inside
the MainActor.run closure (before calling
applyWorkspaceGitMetadataSnapshotBatch) so the closure returns early when
Task.isCancelled, ensuring the stale task does not remove/overwrite entries in
workspaceGitSnapshotTasksByDirectory or workspaceGitSnapshotRequestsByDirectory;
update the closure that currently calls
applyWorkspaceGitMetadataSnapshotBatch(...) (the Task.detached { ... await
MainActor.run { [weak self] in ... } }) to check Task.isCancelled at the start
of that MainActor block and skip calling applyWorkspaceGitMetadataSnapshotBatch
when cancelled.
🪄 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: 0ee96701-6473-44d8-accb-fff0e9b2ccdd
📒 Files selected for processing (4)
Packages/CmuxGit/Sources/CmuxGit/Parsing/GitMetadataService+Index.swiftPackages/CmuxGit/Tests/CmuxGitTests/GitMetadataServiceTests.swiftSources/TabManager.swiftcmuxTests/TabManagerUnitTests.swift
6fc0594 to
a23bf2c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Stale CodeRabbit change request addressed on latest head a23bf2c; CodeRabbit re-review check completed.
This branch was successfully deployed
1 participant
Summary
Issue
Verification
Dogfood
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches sidebar git dirty/branch state and concurrent disk reads; behavior is covered by new coalescing and hex tests but affects many panels sharing one repo.
Overview
Sidebar git metadata initial snapshot work is restructured so multiple panels in the same directory share one in-flight read instead of each firing its own detached probe.
TabManagerqueues probe keys per normalized directory, runs a single utility task per directory, and fans the resulting snapshot out to every queued panel. A sharedWorkspaceGitMetadataProbeLimiter(two concurrent permits, cancellation-aware) caps how many snapshot reads run at once.WorkspaceGitMetadataReadingis injectable (defaults toGitMetadataService); snapshot tasks and queued requests are torn down when git watching is disabled or a probe is cleared.In
CmuxGit, git index parsing replaces per-byteString(format:)hex withgitIndexHexString/ fixed-width helpers; a test locks object ID and checksum text.TabManagerUnitTestsadds a coalescing test with a blocking reader.Reviewed by Cursor Bugbot for commit 6fc0594. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Coalesced sidebar git metadata probes by directory and added a 2-permit async limiter to stop probe storms. Also replaced slow hex encoding in git index parsing with fast lowercase byte encoders to reduce CPU use.
Bug Fixes
Refactors
String(format:)hex in git index parsing with byte encoders and fixed-width helpers inCmuxGit(object IDs and checksums).WorkspaceGitMetadataReadingfor injection; added unit tests for coalescing and hex stability.Written for commit a23bf2c. Summary will update on new commits.
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests