Modernize and ship Feed entry points - #8174
Closed
lawrencecchen wants to merge 54 commits into
Closed
lawrencecchen wants to merge 54 commits into
lawrencecchen wants to merge 54 commits into
Conversation
📝 WalkthroughWalkthroughThe PR promotes Feed from a beta setting to a remotely controlled feature, adds native Feed workspace and pane entry points, introduces Feed presentation and interaction components, refactors blocking coordination and notifications, changes workstream persistence to bounded snapshots, and updates localization, project wiring, and tests. ChangesNative Feed rollout
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (9 errors, 2 warnings)
✅ Passed checks (14 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 ships the Feed UI behind a default-on
Confidence Score: 3/5Safe to merge after addressing the two concurrency issues; the core async-registry design is solid but leaves a cooperative-thread disk-read and a legacy Thread.isMainThread guard that can race with the main actor. Two issues remain on the same hot path: Sources/Feed/FeedCoordinator+Attention.swift (resolveAttentionTarget sync disk read) and Sources/Feed/FeedCoordinator.swift (concludeAttentionOnMain Thread.isMainThread guard) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant SW as Socket Worker
participant Bridge as FeedBlockingCallBridge
participant IC as ingestBlocking
participant Registry as FeedBlockingWaiterRegistry
participant Main as MainActor
SW->>Bridge: wait(timeout)
Bridge->>IC: Task await ingestBlocking
IC->>Registry: await register(requestID)
Registry-->>IC: AsyncStream
IC->>IC: resolveAttentionTarget sync disk IO
IC->>Main: await MainActor.run ingest and surfaceAttention
Main-->>IC: itemID and attentionTarget
IC->>Registry: await recordIngest
IC->>IC: postNotificationIfStillAwaiting
IC->>IC: withTaskGroup decisionStream or timeout
IC->>Registry: await remove requestID
IC->>Main: concludeAttention or markResolved
IC-->>Bridge: IngestBlockingResult
Bridge-->>SW: result unblocks worker
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant SW as Socket Worker
participant Bridge as FeedBlockingCallBridge
participant IC as ingestBlocking
participant Registry as FeedBlockingWaiterRegistry
participant Main as MainActor
SW->>Bridge: wait(timeout)
Bridge->>IC: Task await ingestBlocking
IC->>Registry: await register(requestID)
Registry-->>IC: AsyncStream
IC->>IC: resolveAttentionTarget sync disk IO
IC->>Main: await MainActor.run ingest and surfaceAttention
Main-->>IC: itemID and attentionTarget
IC->>Registry: await recordIngest
IC->>IC: postNotificationIfStillAwaiting
IC->>IC: withTaskGroup decisionStream or timeout
IC->>Registry: await remove requestID
IC->>Main: concludeAttention or markResolved
IC-->>Bridge: IngestBlockingResult
Bridge-->>SW: result unblocks worker
Reviews (8): Last reviewed commit: "Share Feed projections and cancel timed-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🤖 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/FeatureFlags.swift`:
- Around line 174-177: Replace the positional lookup in the isFeedUIEnabled
accessor and the other feature-flag accessors using Self.allFlags indices with
stable key-based lookups or named flag definitions. Ensure each accessor always
resolves its intended flag regardless of declaration order or future insertions.
In `@Sources/Feed/FeedBlockingWaiterRegistry.swift`:
- Around line 53-56: Update isAwaitingDecision(requestID:) to return true only
when a waiter exists and its decision is nil; preserve false for missing,
removed, resolved, or timed-out request IDs.
- Around line 22-28: Model each waiter as a single-terminal state machine: in
Sources/Feed/FeedBlockingWaiterRegistry.swift lines 22-28, make register reject
duplicate request IDs without replacing active waiters; in lines 36-45, make
delivery accept only the first decision and return an explicit accepted/rejected
result. In Sources/Feed/FeedCoordinator.swift lines 161-177, honor a decision
that won before removal instead of expiring solely from DispatchTimeoutResult,
and in lines 199-213, resolve persisted state only when registry delivery is
accepted.
In `@Sources/Feed/FeedCoordinator.swift`:
- Around line 118-120: Update the socket event handling around
FeedCoordinator.shared.store.ingest to remove DispatchQueue.main.sync and keep
ingestion asynchronous. Store the resulting item ID and attention target in the
waiter state without blocking on the main actor, preserving waitTimeout behavior
and avoiding synchronous main-thread calls.
In `@Sources/Feed/FeedCoordinator`+Attention.swift:
- Around line 26-37: Update lifecycleStatusKey(forSource:) and its callers to
resolve the source through the registered agent descriptor and use only its
authoritative panel ID. Remove the fallback that returns the raw source or
infers identity from the focused panel; when no registered descriptor or panel
ID exists, skip the panel lifecycle mutation entirely.
In `@Sources/Feed/FeedCoordinator`+Notifications.swift:
- Around line 20-36: Remove the DEBUG-only FeedCoordinatorTestHooks usage from
the notification flow, including the isAppActive override and
notificationPostObserver short-circuit. Use the production NSApp.isActive
behavior and normal notification delivery directly; move test control or
observation to coordinator dependency injection or the test target without
adding test-named members under Sources/.
In `@Sources/Feed/FeedCoordinator`+Socket.swift:
- Around line 43-46: Move FeedJumpResolver target resolution in
Sources/Feed/FeedCoordinator+Socket.swift lines 43-46 off MainActor for both
focus and send actions, using an off-main actor or repository, and return to
MainActor only for UI event dispatch. In
Sources/Feed/FeedNotificationPolicyContext.swift lines 56-60, snapshot the exact
window/workspace context on MainActor, then perform hook loading and parsing
off-main before applying the policy result.
- Around line 9-20: Update snapshot(pendingOnly:) to use the receiver’s
actor-isolated state rather than FeedCoordinator.shared, and remove the
synchronous DispatchQueue.main.sync path. Change off-main/socket callers to
cross the actor asynchronously while preserving the pendingOnly selection and
snapshot results.
In `@Sources/Feed/FeedCoordinatorState.swift`:
- Around line 15-27: Remove the entire FeedCoordinatorTestHooks registry,
including all static callback properties and the surrounding `#if` DEBUG block,
from the production FeedCoordinatorState source. Update any production
references to these hooks to use injected dependencies or test-target
observation instead, without introducing replacement test-only globals under
Sources.
In `@Sources/Feed/FeedExitPlanView.swift`:
- Around line 368-370: Update the heading-detection condition in
FeedExitPlanView so colon-terminated headings do not require a space; allow
single-word headings such as “Summary:” while preserving the existing length and
word-count limits.
In `@Sources/Feed/FeedItemRow.swift`:
- Around line 195-200: Update relativeTimeChip(_:) to return localized catalog
entries instead of hardcoded "<1m" and interpolated minute/hour/day
abbreviations. Add matching localization keys for the under-minute case and
explicit .one and .other plural forms for minutes, hours, and days in every
supported locale, preserving the current interval thresholds and displayed
counts.
In `@Sources/Feed/FeedItemSnapshot.swift`:
- Around line 37-98: Carry the payload’s authoritative request ID in
FeedItemSnapshot and include it in the approvePermission, replyQuestion, and
approveExitPlan action inputs; update FeedRowActions.bound to pass that ID
directly to FeedCoordinator.shared.deliverReply. Remove requestId(for:) and the
itemId.uuidString fallback, returning without submitting when the ID is absent.
Replace the unowned fire-and-forget reply tasks with explicitly actor- and
lifecycle-owned execution.
In `@Sources/Feed/FeedJumpResolver.swift`:
- Around line 6-80: Replace the static-only FeedJumpResolver and its
feedRequestFocus/feedRequestSendText notifications with an injectable routing
service/protocol that owns parsing, session lookup, focus, and send-text
operations. Make the service constructable and inject it into the feed
coordinator, then invoke the existing typed action owner for coordinator actions
instead of posting global notifications. Preserve the current session-file
lookup and target parsing behavior while removing the ambient notification side
channel.
In `@Sources/Feed/FeedKeyboardFocus.swift`:
- Around line 184-190: Remove the charactersIgnoringModifiers read and its chars
interpolation from FeedKeyboardFocus.keyDown(with:). Keep DEBUG diagnostics
limited to the key code, modifier metadata, and existing responder summary,
without logging any typed character content.
In `@Sources/Feed/FeedListView.swift`:
- Around line 19-23: Move Feed filtering and grouping out of FeedListView.body
into immutable FeedItemSnapshot collections prepared by FeedPanelViewModel, so
rendering reuses precomputed data. In Sources/Feed/FeedListView.swift lines
19-23, consume the view-model snapshots instead of rescanning raw items; in
Sources/Feed/FeedExitPlanView.swift line 327, parse plan blocks and Markdown
once when creating each snapshot; in Sources/Feed/FeedPermissionView.swift lines
150-153, parse permission JSON once before row construction; and in
Sources/Feed/FeedTelemetryView.swift lines 151-156, group todos by state in one
pass and reuse that grouped result.
- Around line 406-408: Scope Feed focus to the owning host window: update
FeedListView.activeFeedWindow() to obtain the window through the bridge rather
than NSApp.keyWindow/mainWindow, and revise FeedQuestionView.activeEditor
handling so blur targets the current host window/coordinator instead of a
process-wide editor singleton. Apply the corresponding changes in
Sources/Feed/FeedListView.swift lines 406-408 and
Sources/Feed/FeedQuestionView.swift lines 514-539.
In `@Sources/Feed/FeedNotificationPolicyContext.swift`:
- Around line 19-23: The FeedNotificationPolicyContext workspace lookup must
fail closed instead of falling back to an arbitrary window’s cmuxConfigStore. In
the context-building flow around workspaceID and context, resolve policy only
from the exact event.workspaceId context; when it is missing or unresolved, skip
workspace-scoped hooks or use an explicitly app-global policy source. Also
update the session/workspace identity handling around the line-41 path to use
the authoritative structured workspace source rather than treating a session ID
as a workspace ID.
In `@Sources/Feed/FeedPanelViewModel.swift`:
- Around line 24-34: Move the initial recheck into the storeInstallTask closure:
after confirming coordinator is available and before entering the
notificationCenter.notifications loop, call self.arm() while safely unwrapping
the weak self. Keep the existing notification-triggered arm() behavior
unchanged.
In `@Sources/Feed/FeedPermissionView.swift`:
- Around line 224-241: Remove the DEBUG-only styling laboratory from
FeedPermissionView.swift and keep FeedButton’s production implementation limited
to plainFeedButton behavior. Move debugStyleGeneration, systemGlassButton,
usesSystemGlassButtonStyle, and all related alternate visual implementations
into a dedicated debug-only file or wrapper outside the production FeedButton
type, preserving the existing debug behavior and availability handling there.
- Around line 244-317: Update plainFeedButton to use SwiftUI’s real
disabled-button semantics whenever dimmed is true, while preserving
performAction’s guard. Extend the hover/cursor handling around handleHover so
changes to dimmed while the pointer remains over the button immediately
reconcile the cursor stack, popping any stale not-allowed cursor when re-enabled
or no longer hovered.
In `@Sources/Feed/FeedQuestionView.swift`:
- Around line 446-470: Remove the text-based fallback from isPlanAskUserQuestion
and delete questionTextLooksLikePlanInterview. Gate the “Skip + plan
immediately” behavior solely on context?.permissionMode being case-insensitively
equal to “plan”; return false when the context or permission mode is absent.
- Around line 794-810: Update FeedQuestionView.blurField() to call
focusRightSidebarInActiveMainWindow(...) synchronously on the current focus
turn, removing the deferred Task { `@MainActor` in ... } wrapper. Preserve the
existing arguments and immediately fall back to window.makeFirstResponder(nil)
when the coordinator does not return true.
In `@Sources/Feed/FeedSocketEncoding.swift`:
- Around line 50-61: Update assignLimitedText to use the existing
primaryTextLimit constant as its default limit instead of the duplicated 8,000
literal, keeping the truncation and dictionary assignment behavior unchanged.
- Around line 85-86: Update FeedSocketEncoding.itemDict to reuse a cached or
shared ISO8601DateFormatter instead of instantiating one on each call. Keep the
existing date-encoding behavior unchanged while ensuring formatter creation
occurs outside the per-item mapping path.
- Around line 111-121: Update the feed encoding logic around the
permissionRequest case and its corresponding tool-result encoding to redact
sensitive payloads before assigning tool_input or tool_result. Replace raw JSON
truncation via assignLimitedText with the existing redaction mechanism, while
preserving the narrow tool_input_capabilities projection and existing field
names.
In `@Sources/Feed/FeedTelemetryView.swift`:
- Around line 177-180: Update the count-bearing localized strings in
FeedTelemetryView, including the feed.todos.moreCompleted text and the related
strings around the referenced section, to select explicit `.one` and `.other`
localization keys based on the count. Ensure singular values such as 1
completed, 1 done, 1 in progress, and 1 open use `.one`, while all other counts
use `.other`, preserving the existing displayed counts and fallback text.
🪄 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: 650da199-1292-4c68-99ec-d223b81aeacb
📒 Files selected for processing (50)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/BetaFeaturesCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BetaFeaturesSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Values/NewWorkspaceInitialSurface.swiftResources/Localizable.xcstringsSources/App/WorkspaceRuntimeSettings.swiftSources/AppDelegate+NewWorkspaceContextMenu.swiftSources/AppDelegate+NewWorkspaceMenuRendering.swiftSources/AppDelegate.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/ContentView.swiftSources/FeatureFlags.swiftSources/Feed/FeedBlockingWaiterRegistry.swiftSources/Feed/FeedCoordinator+Attention.swiftSources/Feed/FeedCoordinator+Notifications.swiftSources/Feed/FeedCoordinator+Socket.swiftSources/Feed/FeedCoordinator.swiftSources/Feed/FeedCoordinatorState.swiftSources/Feed/FeedExitPlanView.swiftSources/Feed/FeedItemRow.swiftSources/Feed/FeedItemSnapshot.swiftSources/Feed/FeedJumpResolver.swiftSources/Feed/FeedKeyboardFocus.swiftSources/Feed/FeedListView.swiftSources/Feed/FeedNotificationPolicyContext.swiftSources/Feed/FeedOpeningCoordinator.swiftSources/Feed/FeedPanelView.swiftSources/Feed/FeedPanelViewModel.swiftSources/Feed/FeedPermissionView.swiftSources/Feed/FeedQuestionView.swiftSources/Feed/FeedSocketEncoding.swiftSources/Feed/FeedTelemetryView.swiftSources/FileExplorerState.swiftSources/NewWorkspaceMenuModel.swiftSources/RightSidebarMode+Availability.swiftSources/RightSidebarModeShortcutMatcher.swiftSources/RightSidebarPanelView.swiftSources/RightSidebarToolPanel.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/TabManager.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/FeedCoordinatorTests.swiftcmuxTests/FileExplorerStateModePersistenceTests.swiftcmuxTests/NewWorkspaceMenuModelTests.swiftcmuxTests/PostHogAnalyticsPropertiesTests.swiftcmuxTests/RightSidebarCommandPaletteTests.swift
💤 Files with no reviewable changes (8)
- Sources/SettingsSearchAliases.swift
- Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swift
- Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/BetaFeaturesCatalogSection.swift
- Sources/SettingsNavigation.swift
- Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BetaFeaturesSection.swift
- Packages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swift
- Sources/App/WorkspaceRuntimeSettings.swift
- Sources/CommandPalette/CommandPaletteSettingsToggle.swift
# Conflicts: # Sources/Workspace.swift
# Conflicts: # Sources/FeatureFlags.swift # Sources/Workspace.swift
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 (2)
cmuxTests/FeedCoordinatorTests.swift (1)
509-520: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAwait task results and an ingest completion signal.
These tests block on semaphores and use a fixed 500-iteration
Task.yield()poll, while sharing the result through@unchecked Sendable. Keep the ingestionTaskhandle, await its value, and signal store ingestion deterministically; use the injected virtual clock for timeout behavior.As per coding guidelines, tests must await real completion signals or virtual clocks rather than fixed waits and scheduler-dependent polling.
Also applies to: 554-583, 652-654
🤖 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/FeedCoordinatorTests.swift` around lines 509 - 520, Update the ingestion tests around the Task-based ingestBlocking calls to retain and await the Task handle’s result instead of using DispatchSemaphore, IngestResultBox, or fixed Task.yield polling. Add and await the store-ingestion completion signal, and drive timeout behavior through the injected virtual clock so completion and timing remain deterministic; apply the same pattern to the referenced test sections.Source: Coding guidelines
Sources/Feed/FeedItemRow.swift (1)
267-276: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the payload’s authoritative request ID.
Each payload case already provides a non-optional request ID. Reading the duplicated optional
snapshot.requestIDmakes inconsistent state representable and silently disables—or misroutes—the action. Bind the associated ID directly in each switch case.Proposed fix
- case .permissionRequest(_, let toolName, let toolInputJSON, _): + case .permissionRequest(let requestID, let toolName, let toolInputJSON, _): ... onApprove: { mode in - guard let requestID = snapshot.requestID else { return } actions.approvePermission(requestID, mode) } - case .exitPlan(_, let plan, _): + case .exitPlan(let requestID, let plan, _): ... onApprove: { mode, feedback in - guard let requestID = snapshot.requestID else { return } actions.approveExitPlan(requestID, mode, feedback) } - case .question(_, let questions): + case .question(let requestID, let questions): ... onReply: { selections in - guard let requestID = snapshot.requestID else { return } actions.replyQuestion(requestID, selections) }As per path instructions, correctness-critical routing must use one reliable structured source and fail closed rather than relying on conflicting derived values.
Also applies to: 279-290, 293-307
🤖 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/Feed/FeedItemRow.swift` around lines 267 - 276, Use the authoritative non-optional request ID associated with each permission payload case in the FeedItemRow switch. Bind the ID directly in the .permissionRequest case and the other affected cases at the referenced action closures, then pass that bound ID to the approve, deny, or corresponding action methods instead of reading snapshot.requestID; preserve the existing fail-closed behavior only where the payload itself cannot provide an ID.Sources: Coding guidelines, Path instructions
🤖 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 `@Resources/Localizable.xcstrings`:
- Around line 103499-103505: Update the Japanese values for the “Plan ready” and
“Permission needed” localization entries in Localizable.xcstrings to preserve
their English meanings: use wording equivalent to “プランの準備完了” for the former and
“権限が必要です” for the latter.
In `@Sources/Feed/FeedActivitySnapshotGroups.swift`:
- Around line 2-6: Store the combined ordered snapshots during
FeedActivitySnapshotGroups initialization instead of computing them through the
ordered property on every access. In
Sources/Feed/FeedActivitySnapshotGroups.swift lines 2-6, add stored
ordered-array state; in Sources/Feed/FeedListView.swift lines 25-29, consume it
once and reuse snapshots for focus navigation; in
Sources/Feed/FeedListView.swift lines 125-162, iterate identifiable snapshots
directly without Array(enumerated()) materialization.
In `@Sources/Feed/FeedPanelViewModel.swift`:
- Around line 25-34: Update the storeInstallTask notification loop in
FeedPanelViewModel so it does not strongly capture self for the lifetime of the
asynchronous sequence. Avoid the outer guard let self; capture the view model
weakly and unwrap it only for each arm() call, allowing deinit to cancel and
release the task when the panel is dismissed.
---
Outside diff comments:
In `@cmuxTests/FeedCoordinatorTests.swift`:
- Around line 509-520: Update the ingestion tests around the Task-based
ingestBlocking calls to retain and await the Task handle’s result instead of
using DispatchSemaphore, IngestResultBox, or fixed Task.yield polling. Add and
await the store-ingestion completion signal, and drive timeout behavior through
the injected virtual clock so completion and timing remain deterministic; apply
the same pattern to the referenced test sections.
In `@Sources/Feed/FeedItemRow.swift`:
- Around line 267-276: Use the authoritative non-optional request ID associated
with each permission payload case in the FeedItemRow switch. Bind the ID
directly in the .permissionRequest case and the other affected cases at the
referenced action closures, then pass that bound ID to the approve, deny, or
corresponding action methods instead of reading snapshot.requestID; preserve the
existing fail-closed behavior only where the payload itself cannot provide an
ID.
🪄 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: 0eaf68c5-69ca-426e-b525-afbeb311f684
📒 Files selected for processing (46)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamPersistence.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/BetaFeaturesCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BetaFeaturesSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftResources/Localizable.xcstringsSources/App/WorkspaceRuntimeSettings.swiftSources/AppDelegate+NotificationDeliverySeams.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/FeatureFlags.swiftSources/Feed/FeedActivitySnapshotGroups.swiftSources/Feed/FeedBlockingCallBridge.swiftSources/Feed/FeedBlockingWaiterRegistry.swiftSources/Feed/FeedCoordinator+Attention.swiftSources/Feed/FeedCoordinator+Notifications.swiftSources/Feed/FeedCoordinator+Socket.swiftSources/Feed/FeedCoordinator.swiftSources/Feed/FeedCoordinatorState.swiftSources/Feed/FeedExitPlanView.swiftSources/Feed/FeedItemRow.swiftSources/Feed/FeedItemSnapshot.swiftSources/Feed/FeedJumpResolver.swiftSources/Feed/FeedKeyboardFocus.swiftSources/Feed/FeedListView.swiftSources/Feed/FeedNotificationPolicyContext.swiftSources/Feed/FeedNotificationPolicySnapshot.swiftSources/Feed/FeedPanelView.swiftSources/Feed/FeedPanelViewModel.swiftSources/Feed/FeedPermissionView.swiftSources/Feed/FeedPresentationSnapshot.swiftSources/Feed/FeedQuestionView.swiftSources/Feed/FeedSocketEncoding.swiftSources/Feed/FeedTelemetryView.swiftSources/RightSidebarToolPanel.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/TabManager.swiftSources/TerminalController+ControlFeedContext.swiftSources/TerminalController.swiftSources/TerminalNotificationPolicy.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/FeedCoordinatorTests.swiftcmuxTests/PostHogAnalyticsPropertiesTests.swift
💤 Files with no reviewable changes (2)
- Sources/Feed/FeedCoordinatorState.swift
- Sources/App/WorkspaceRuntimeSettings.swift
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 (3)
Sources/Feed/FeedCoordinator+Notifications.swift (2)
191-253: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
await center.notificationSettings()instead of the callback pyramid.Using the
asyncversion ofnotificationSettingsflattens the closure pyramid and avoids the nestedTask {@mainactorin }context, making the asynchronous control flow much cleaner.As per path instructions, flag new or materially expanded use of legacy asynchronous patterns (like callback pyramids) when
asyncfunctions are appropriate.♻️ Proposed refactor
let request = UNNotificationRequest( identifier: "feed.\(requestId)", content: content, trigger: nil ) let center = UNUserNotificationCenter.current() - center.getNotificationSettings { settings in - Task { `@MainActor` [weak self] in - guard let self, await self.isAwaitingDecision(requestId: requestId) else { return } - switch settings.authorizationStatus { - case .authorized, .provisional: - await self.addNotificationIfStillAwaiting( - center: center, - request: request, - requestId: requestId, - effects: effects - ) - case .notDetermined: - var granted = false - var requestFailed = false - do { - granted = try await center.requestAuthorization(options: [.alert, .sound]) - } catch { - requestFailed = true - } - guard await self.isAwaitingDecision(requestId: requestId) else { return } - if granted { - await self.addNotificationIfStillAwaiting( - center: center, - request: request, - requestId: requestId, - effects: effects - ) - } else { - // A non-grant without an error is the user declining - // the prompt just now: honor the fresh denial on this - // very notification. A request error is not a user - // decision, so the fallback stays audible (fail-open). - await self.runFallbackEffectsIfStillAwaiting( - requestId: requestId, - title: title, - subtitle: subtitle, - body: body, - effects: TerminalNotificationStore.fallbackEffects( - effects, - authorizationState: requestFailed ? .unknown : .denied - ), - runCommand: false - ) - } - default: - await self.runFallbackEffectsIfStillAwaiting( - requestId: requestId, - title: title, - subtitle: subtitle, - body: body, - effects: TerminalNotificationStore.fallbackEffects( - effects, - authorizationState: TerminalNotificationStore.authorizationState( - from: settings.authorizationStatus - ) - ), - runCommand: false - ) - } - } - } + let settings = await center.notificationSettings() + guard await isAwaitingDecision(requestId: requestId) else { return } + + switch settings.authorizationStatus { + case .authorized, .provisional: + await addNotificationIfStillAwaiting( + center: center, + request: request, + requestId: requestId, + effects: effects + ) + case .notDetermined: + var granted = false + var requestFailed = false + do { + granted = try await center.requestAuthorization(options: [.alert, .sound]) + } catch { + requestFailed = true + } + guard await isAwaitingDecision(requestId: requestId) else { return } + if granted { + await addNotificationIfStillAwaiting( + center: center, + request: request, + requestId: requestId, + effects: effects + ) + } else { + await runFallbackEffectsIfStillAwaiting( + requestId: requestId, + title: title, + subtitle: subtitle, + body: body, + effects: TerminalNotificationStore.fallbackEffects( + effects, + authorizationState: requestFailed ? .unknown : .denied + ), + runCommand: false + ) + } + default: + await runFallbackEffectsIfStillAwaiting( + requestId: requestId, + title: title, + subtitle: subtitle, + body: body, + effects: TerminalNotificationStore.fallbackEffects( + effects, + authorizationState: TerminalNotificationStore.authorizationState( + from: settings.authorizationStatus + ) + ), + runCommand: false + ) + } }🤖 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/Feed/FeedCoordinator`+Notifications.swift around lines 191 - 253, Refactor the notification-settings flow around UNUserNotificationCenter so the enclosing async method awaits center.notificationSettings() directly instead of using getNotificationSettings with a nested Task. Preserve the existing MainActor isolation, awaiting-decision checks, authorization handling, and fallback behavior while flattening the callback structure.Source: Path instructions
266-293: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
try await center.add(request)instead of the completion callback.Using the
asyncversion ofadd(_:)flattens the closure pyramid and avoids creating a detached@MainActorTask.As per path instructions, prefer modern Swift concurrency over legacy callback pyramids.
♻️ Proposed refactor
- center.add(request) { error in - let didFail = error != nil - Task { `@MainActor` [weak self] in - guard let self else { return } - if !(await self.isAwaitingDecision(requestId: requestId)) { - self.cancelNotification(requestId: requestId) - return - } - if didFail { - await self.runFallbackEffectsIfStillAwaiting( - requestId: requestId, - title: title, - subtitle: subtitle, - body: body, - effects: effects, - runCommand: false - ) - return - } - if effects.command { - NotificationSoundSettings.runCustomCommand( - title: title, - subtitle: subtitle, - body: body - ) - } - } - } + do { + try await center.add(request) + guard await isAwaitingDecision(requestId: requestId) else { + cancelNotification(requestId: requestId) + return + } + if effects.command { + NotificationSoundSettings.runCustomCommand( + title: title, + subtitle: subtitle, + body: body + ) + } + } catch { + guard await isAwaitingDecision(requestId: requestId) else { + cancelNotification(requestId: requestId) + return + } + await runFallbackEffectsIfStillAwaiting( + requestId: requestId, + title: title, + subtitle: subtitle, + body: body, + effects: effects, + runCommand: false + ) + }🤖 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/Feed/FeedCoordinator`+Notifications.swift around lines 266 - 293, Refactor the notification scheduling flow around center.add(request) to use its async throwing form with try await instead of a completion callback and nested `@MainActor` Task. Preserve the existing awaiting-decision check, failure fallback via runFallbackEffectsIfStillAwaiting, and successful effects.command handling, while propagating or handling the add error through the existing didFail behavior.Source: Path instructions
Sources/Feed/FeedCoordinator.swift (1)
202-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
Thread.isMainThreadconditional dispatch.Using
Thread.isMainThreadto conditionally dispatch work is an anti-pattern in modern Swift concurrency and fragments MainActor lifecycle ownership. Since both callers (ingestBlockinganddeliverReply) areasync, you can either use an unconditionalTask {@mainactorin }for fire-and-forget, or better, fold the conclusion directly into the existingawait MainActor.runblocks in the callers.As per path instructions, do not patch symptoms with delayed dispatch or split UI lifecycle ownership, and use one explicit MainActor owner.
♻️ Proposed refactor (folding into existing `MainActor.run` blocks)
- /// Concludes an attention overlay (if any) on the main actor, hopping if - /// called from the socket worker thread. - private func concludeAttentionOnMain(_ target: AttentionTarget?) { - guard let target else { return } - let conclude: `@Sendable` () -> Void = { [target] in - MainActor.assumeIsolated { - FeedCoordinator.shared.concludeBlockingDecisionAttention(target) - } - } - if Thread.isMainThread { - conclude() - } else { - Task { `@MainActor` in conclude() } - } - } - `@MainActor` private func concludeBlockingDecisionAttentionIfPresent(_ target: AttentionTarget?) {Then update the call sites. In
ingestBlocking(around line 197):cancelNotification(requestId: requestId) - concludeAttentionOnMain(waiter?.attentionTarget) - expireTimedOutItem(waiter?.itemID) + Task { `@MainActor` [weak self] in + self?.concludeBlockingDecisionAttentionIfPresent(waiter?.attentionTarget) + if let itemId = waiter?.itemID { + self?.store?.markExpired(itemId) + } + }In
deliverReply(around line 239):- concludeAttentionOnMain(delivery.attentionTarget) - await MainActor.run { + concludeBlockingDecisionAttentionIfPresent(delivery.attentionTarget) guard let store, let itemID = delivery.itemID else { 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/Feed/FeedCoordinator.swift` around lines 202 - 216, Remove the Thread.isMainThread-based dispatch from concludeAttentionOnMain and use a single explicit MainActor owner. Update ingestBlocking and deliverReply to conclude the attention within their existing MainActor execution, reusing the appropriate concludeBlockingDecisionAttentionIfPresent helper and preserving item expiration in the ingestBlocking path.Source: Path instructions
🤖 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/Feed/FeedCoordinator.swift`:
- Around line 202-216: Remove the Thread.isMainThread-based dispatch from
concludeAttentionOnMain and use a single explicit MainActor owner. Update
ingestBlocking and deliverReply to conclude the attention within their existing
MainActor execution, reusing the appropriate
concludeBlockingDecisionAttentionIfPresent helper and preserving item expiration
in the ingestBlocking path.
In `@Sources/Feed/FeedCoordinator`+Notifications.swift:
- Around line 191-253: Refactor the notification-settings flow around
UNUserNotificationCenter so the enclosing async method awaits
center.notificationSettings() directly instead of using getNotificationSettings
with a nested Task. Preserve the existing MainActor isolation, awaiting-decision
checks, authorization handling, and fallback behavior while flattening the
callback structure.
- Around line 266-293: Refactor the notification scheduling flow around
center.add(request) to use its async throwing form with try await instead of a
completion callback and nested `@MainActor` Task. Preserve the existing
awaiting-decision check, failure fallback via runFallbackEffectsIfStillAwaiting,
and successful effects.command handling, while propagating or handling the add
error through the existing didFail behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abc7005e-98c4-4eae-961e-c7b53adac33b
📒 Files selected for processing (6)
Sources/Feed/FeedCoordinator+Notifications.swiftSources/Feed/FeedCoordinator.swiftSources/Feed/FeedNotificationPolicyContext.swiftSources/Feed/FeedNotificationPolicySnapshot.swiftSources/Feed/FeedPanelViewModel.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swift
# Conflicts: # cmux.xcodeproj/project.pbxproj
Contributor
|
Too many files changed for review. ( Bypass the limit by tagging |
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 (4)
Sources/Feed/FeedPlanBodyView.swift (4)
1-6:⚠️ Potential issue | 🔴 CriticalLGTM!
Source: Coding guidelines
5628-5644: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftKeep Feed coordination asynchronous instead of blocking the socket worker.
Both paths use
FeedBlockingCallBridge.waitto synchronously block the socket worker thread for up to 10–130 seconds while waiting for async actor work. Since socket workers handle requests sequentially on a limited pool of worker threads, this stalls unrelated incoming socket commands for minutes at a time. This was raised in a previous review and remains unaddressed.As per coding guidelines, do not introduce thread-blocking waits for async work.
Sources/TerminalController.swift#L5628-L5644: Updatev2FeedPush(viasocketWorkerV2Response) to use the existingv2AsyncResultCallpattern so the socket worker can return immediately and process other commands while waiting foringestBlocking.Sources/TerminalController.swift#L5764-L5779: Updatev2DeliverFeedReply(and its callers likev2FeedPermissionReply) to use thev2AsyncResultCallpattern rather than blocking for up to 10 seconds.🤖 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/TerminalController.swift` around lines 5628 - 5644, The Feed response paths must remain asynchronous instead of blocking socket workers. In Sources/TerminalController.swift:5628-5644, update v2FeedPush via socketWorkerV2Response to use the existing v2AsyncResultCall pattern around ingestBlocking, preserving completion publication and response encoding. In Sources/TerminalController.swift:5764-5779, update v2DeliverFeedReply and callers such as v2FeedPermissionReply to use v2AsyncResultCall and remove FeedBlockingCallBridge.wait-based blocking.Source: Coding guidelines
🤖 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/Feed/FeedRowActions.swift`:
- Around line 14-31: Update TaskStore’s task collection to use explicit NSLock
synchronization instead of relying on `@MainActor` isolation, including locking
all reads, insertions, removals, and cancellation during deinit. Ensure deinit
safely snapshots or clears the protected tasks dictionary while holding the
lock, then cancels the tasks without accessing isolated state.
---
Outside diff comments:
In `@Sources/Feed/FeedPlanBodyView.swift`:
- Around line 1-6: Keep Feed coordination asynchronous in the socket request
routing. Update the feed.push path around socketWorkerV2Response and v2FeedPush
to use the existing v2AsyncResultCall pattern, removing FeedBlockingCallBridge,
ingestTask, and bridge.wait. Apply the same asynchronous response flow to feed
permission, question, and exit-plan replies by replacing the synchronous
v2DeliverFeedReply wait while preserving their validation and response payloads.
- Around line 5628-5658: Update the feed.* route handlers in
socketWorkerV2Response, including the additional handlers near the
FeedBlockingCallBridge usage, to use the existing v2AsyncResultCall pattern like
mobile.attach_ticket.create. Remove synchronous FeedBlockingCallBridge.wait
calls while preserving the current FeedCoordinator task results and error
responses.
- Around line 5628-5644: Update the feed.* route handlers in FeedPlanBodyView to
use the existing v2AsyncResultCall pattern, matching
mobile.attach_ticket.create, instead of FeedBlockingCallBridge.wait. Return the
asynchronous result immediately while FeedCoordinator tasks complete, and remove
the synchronous wait path from the affected handlers, including the additional
handler range noted in the review.
- Around line 5628-5644: Update the feed.* route handlers in
socketWorkerV2Response, including v2FeedPush and v2FeedPermissionReply, to use
the existing v2AsyncResultCall pattern instead of FeedBlockingCallBridge.wait.
Return from the socket worker immediately while FeedCoordinator tasks complete
asynchronously, matching the mobile.attach_ticket.create flow and preserving
each handler’s existing success and error responses.
---
Duplicate comments:
In `@Sources/TerminalController.swift`:
- Around line 5628-5658: Replace the blocking Feed handlers with async variants:
in Sources/TerminalController.swift lines 5628-5658, update v2FeedPush and its
socketWorkerV2Response route to use v2AsyncResultCall, await
FeedCoordinator.shared.ingestBlocking, and remove FeedBlockingCallBridge and
synchronous waiting; in lines 5764-5779, similarly update v2DeliverFeedReply and
its corresponding reply routes to use v2AsyncResultCall and return the awaited
result without blocking the socket worker.
- Around line 5628-5644: The Feed response paths must remain asynchronous
instead of blocking socket workers. In
Sources/TerminalController.swift:5628-5644, update v2FeedPush via
socketWorkerV2Response to use the existing v2AsyncResultCall pattern around
ingestBlocking, preserving completion publication and response encoding. In
Sources/TerminalController.swift:5764-5779, update v2DeliverFeedReply and
callers such as v2FeedPermissionReply to use v2AsyncResultCall and remove
FeedBlockingCallBridge.wait-based blocking.
🪄 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: e229ff99-78ca-4926-918d-48caefbc3f21
📒 Files selected for processing (18)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftSources/AppDelegate.swiftSources/Feed/FeedBlockingWaiterRegistry.swiftSources/Feed/FeedCoordinator+Notifications.swiftSources/Feed/FeedCoordinator.swiftSources/Feed/FeedInlineNativeTextView.swiftSources/Feed/FeedJumpResolver.swiftSources/Feed/FeedListView.swiftSources/Feed/FeedNotificationPolicyContext.swiftSources/Feed/FeedPanelView.swiftSources/Feed/FeedPanelViewModel.swiftSources/Feed/FeedPlanBodyView.swiftSources/Feed/FeedPresentationStore.swiftSources/Feed/FeedRowActions.swiftSources/TerminalController.swiftcmuxTests/FeedCoordinatorTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Feed/FeedRowActions.swift (1)
15-32: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix strict concurrency violation in
TaskStore.deinit.Because
deinitis non-isolated and runs on whatever thread drops the final reference, it cannot safely read or modify the implicit@MainActor-isolatedtasksdictionary. In Swift 6, this is a compiler error and can trap at runtime.As per coding guidelines, "Avoid Swift 6 actor-isolation mistakes such as... shared mutable Sendable references without isolation". Protect the task collection manually with a lock so it can be safely cleared upon deallocation.
🔒️ Proposed fix using NSLock
- `@MainActor` - final class TaskStore { + final class TaskStore: `@unchecked` Sendable { private var tasks: [UUID: Task<Void, Never>] = [:] + private let lock = NSLock() deinit { - for task in tasks.values { + lock.lock() + let currentTasks = tasks.values + lock.unlock() + for task in currentTasks { task.cancel() } } func run(_ operation: `@escaping` `@MainActor` () async -> Void) { let id = UUID() - tasks[id] = Task { [weak self] in + let task = Task { [weak self] in await operation() - self?.tasks.removeValue(forKey: id) + self?.remove(id) } + lock.lock() + tasks[id] = task + lock.unlock() + } + + private func remove(_ id: UUID) { + lock.lock() + tasks.removeValue(forKey: id) + lock.unlock() } }🤖 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/Feed/FeedRowActions.swift` around lines 15 - 32, Update TaskStore’s task-collection management to use an NSLock, protecting all reads, insertions, removals, and deinit cancellation/clearing of tasks from nonisolated access. Ensure deinit acquires the lock before extracting and clearing the collection, then cancels the extracted tasks outside the lock, while preserving run’s task lifecycle cleanup.Source: Coding guidelines
🤖 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/AppDelegate.swift`:
- Around line 9331-9338: Update the selection flow surrounding
invokeFeedSocketCommand so the workspaceId/surfaceId pair is validated through
the authoritative typed surface mapping before issuing any mutating command.
Fail closed when the mapping is missing or mismatched, then perform
workspace.select and surface.focus atomically or through a shared validated
operation so a rejected surface cannot leave the workspace changed.
---
Duplicate comments:
In `@Sources/Feed/FeedRowActions.swift`:
- Around line 15-32: Update TaskStore’s task-collection management to use an
NSLock, protecting all reads, insertions, removals, and deinit
cancellation/clearing of tasks from nonisolated access. Ensure deinit acquires
the lock before extracting and clearing the collection, then cancels the
extracted tasks outside the lock, while preserving run’s task lifecycle cleanup.
🪄 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: 9b5dfde3-9ec5-4d74-b292-a63fa70f2a5f
📒 Files selected for processing (11)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/Feed/FeedBlockingWaiterCompletion.swiftSources/Feed/FeedBlockingWaiterRegistry.swiftSources/Feed/FeedItemRow.swiftSources/Feed/FeedPreviewWindowController.swiftSources/Feed/FeedRowActions.swiftSources/Feed/FeedStopDraft.swiftSources/Feed/FeedTelemetryView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/FeedFocusRoutingTests.swift
# Conflicts: # Sources/ContentView.swift # Sources/TerminalWindowPortal.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swift`:
- Line 89: Remove the feedPrefix(512) truncation from WorkstreamItem identity
fields and preserve the original protocol identifiers consistently across
session, request, question, option, todo, and workstream references. Ensure
context-cache insertion, lookup, and removal use the same raw authoritative
identifiers, and reject over-limit events at the transport boundary rather than
rewriting IDs.
- Around line 204-208: Update String.feedPrefix(_:) to enforce the limit using
UTF-8 byte capacity rather than String.count, avoiding a full-input scan and
safely handling grapheme boundaries. Construct only the bounded UTF-8 prefix,
ensuring the returned string never exceeds maximumCharacters bytes while
preserving the original string when already within the limit.
- Around line 123-128: Redact permission JSON before truncating it, so
over-limit payloads remain valid for JSON-aware secret redaction; update the
permissionRequest handling in WorkstreamItem.swift at lines 123-128 accordingly.
Apply the same ordering to tool-use and tool-result JSON at WorkstreamItem.swift
lines 155-164. In WorkstreamPersistence.swift lines 95-98, redact the original
JSON first and then bound the safe result, and add a regression test covering an
over-limit payload containing a secret.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamPersistence.swift`:
- Around line 147-152: The loadLegacyRemovedItemIDs migration must fail closed
when the legacy tombstone file exceeds maximumLegacyReadBytes: do not return an
empty Set, which permits deleted rows to be restored. Propagate an
unavailable/failed migration outcome through the caller so restoration is
skipped while preserving deletion semantics; keep normal bounded-file loading
unchanged.
- Around line 111-125: Update the snapshot persistence flow around the
selectedLines handling so the new snapshot write, or empty-snapshot file
deletion, completes successfully before removing removedItemsFileURL. Do not
suppress errors from deleting the old snapshot in the empty branch, and preserve
tombstones whenever directory creation, writing, or deletion fails.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swift`:
- Around line 69-76: Update start() so persistence.replacePendingItems is called
only after loadPendingItems succeeds; when restoration fails, preserve the
existing persisted snapshot by skipping compaction, or propagate the load error
instead. Keep the restored-item mapping and rebuildContextIndex behavior
unchanged for successful loads.
- Around line 181-191: Update schedulePendingSnapshot so the asynchronous
replacePendingItems operation is owned by a stored, cancellable writer task or
awaited through a caller-owned persistence path. Coalesce successive snapshots
while preserving the latest persistenceGeneration and pendingItems, and handle
replacement failures rather than discarding them, ensuring interrupted or failed
writes cannot leave resolved or expired cards persisted as pending.
- Around line 120-127: The removeItem(id:) implementation should not call
rebuildContextIndex() after deleting a card, because that discards
telemetry-only context absent from items. Preserve the existing context cache or
update only the removed item’s affected workstream, while leaving persistence
generation and replacement behavior unchanged.
In
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamStoreTests.swift`:
- Around line 96-98: Update the restart setup in WorkstreamStoreTests by
constructing a new WorkstreamPersistence with the same temporary file URL used
by the original persistence, then pass that fresh actor to WorkstreamStore. Keep
the existing ringCapacity, start call, and restored item assertion 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e284b80b-b150-4390-ab58-c0a6bf65539d
📒 Files selected for processing (6)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamKind.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamPersistence.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamPersistenceTests.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamStoreTests.swift
| func retainedForFeed() -> WorkstreamItem { | ||
| WorkstreamItem( | ||
| id: id, | ||
| workstreamId: workstreamId.feedPrefix(512), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not truncate protocol identifiers.
This rewrites session, request, question, option, and todo IDs, creating collisions and breaking exact reply routing. It also mismatches the context cache: lookup/removal uses raw event.sessionId, while insertion uses the truncated workstreamId.
Preserve identifiers exactly, or reject over-limit events at the transport boundary.
As per path instructions, correctness-critical identity must use one authoritative structured source and fail closed rather than be rewritten.
Also applies to: 125-125, 132-132, 138-148, 175-175
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swift`
at line 89, Remove the feedPrefix(512) truncation from WorkstreamItem identity
fields and preserve the original protocol identifiers consistently across
session, request, question, option, todo, and workstream references. Ensure
context-cache insertion, lookup, and removal use the same raw authoritative
identifiers, and reject over-limit events at the transport boundary rather than
rewriting IDs.
Source: Path instructions
Comment on lines
+123
to
+128
| case .permissionRequest(let requestId, let toolName, let toolInputJSON, let pattern): | ||
| return .permissionRequest( | ||
| requestId: requestId.feedPrefix(512), | ||
| toolName: toolName.feedPrefix(512), | ||
| toolInputJSON: toolInputJSON.feedPrefix(32_768), | ||
| pattern: pattern?.feedPrefix(4_096) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Redact JSON before any destructive truncation.
An over-limit JSON payload becomes invalid before persistence redaction. The redactor then falls back to environment-assignment matching, so JSON secrets such as "api_key":"sk-..." near the beginning can be persisted unredacted.
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swift#L123-L128: avoid raw prefix truncation of permission JSON before redaction.Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swift#L155-L164: apply the same fix to tool-use and tool-result JSON.Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamPersistence.swift#L95-L98: redact the original JSON first, then bound the safe projection; add an over-limit secret regression test.
📍 Affects 2 files
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swift#L123-L128(this comment)Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swift#L155-L164Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamPersistence.swift#L95-L98
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swift`
around lines 123 - 128, Redact permission JSON before truncating it, so
over-limit payloads remain valid for JSON-aware secret redaction; update the
permissionRequest handling in WorkstreamItem.swift at lines 123-128 accordingly.
Apply the same ordering to tool-use and tool-result JSON at WorkstreamItem.swift
lines 155-164. In WorkstreamPersistence.swift lines 95-98, redact the original
JSON first and then bound the safe result, and add a regression test covering an
over-limit payload containing a secret.
Comment on lines
+204
to
+208
| private extension String { | ||
| func feedPrefix(_ maximumCharacters: Int) -> String { | ||
| guard count > maximumCharacters else { return self } | ||
| return String(prefix(maximumCharacters)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound UTF-8 bytes without scanning the full string.
String.count traverses the entire input, while one extended grapheme can contain unbounded bytes. Agent-controlled fields can therefore bypass the intended memory ceiling and stall MainActor ingestion. Implement a byte-bounded prefix instead.
As per coding guidelines, production hot paths must use bounded construction rather than full-input scans.
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamItem.swift`
around lines 204 - 208, Update String.feedPrefix(_:) to enforce the limit using
UTF-8 byte capacity rather than String.count, avoiding a full-input scan and
safely handling grapheme boundaries. Construct only the bounded UTF-8 prefix,
ensuring the returned string never exceeds maximumCharacters bytes while
preserving the original string when already within the limit.
Source: Coding guidelines
Comment on lines
+111
to
+125
| let fileManager = FileManager.default | ||
| try? fileManager.removeItem(at: removedItemsFileURL) | ||
| guard !selectedLines.isEmpty else { | ||
| try? fileManager.removeItem(at: fileURL) | ||
| return | ||
| } | ||
| try fileManager.createDirectory( | ||
| at: fileURL.deletingLastPathComponent(), | ||
| withIntermediateDirectories: true | ||
| ) | ||
| if !fm.fileExists(atPath: fileURL.path) { | ||
| fm.createFile(atPath: fileURL.path, contents: nil) | ||
| var snapshot = Data(capacity: selectedByteCount) | ||
| for line in selectedLines.reversed() { | ||
| snapshot.append(line) | ||
| } | ||
| let fh = try FileHandle(forWritingTo: fileURL) | ||
| handle = fh | ||
| return fh | ||
| try snapshot.write(to: fileURL, options: .atomic) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Commit the snapshot before deleting legacy tombstones.
If directory creation or the atomic write fails after Line 112, the legacy log remains but its removal records are gone, allowing dismissed cards to reappear. The empty-snapshot branch has the same problem because file deletion errors are suppressed.
Delete tombstones only after the new snapshot—or deletion of the old snapshot—succeeds.
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamPersistence.swift`
around lines 111 - 125, Update the snapshot persistence flow around the
selectedLines handling so the new snapshot write, or empty-snapshot file
deletion, completes successfully before removing removedItemsFileURL. Do not
suppress errors from deleting the old snapshot in the empty branch, and preserve
tombstones whenever directory creation, writing, or deletion fails.
Comment on lines
+147
to
+152
| private func loadLegacyRemovedItemIDs() throws -> Set<UUID> { | ||
| guard FileManager.default.fileExists(atPath: removedItemsFileURL.path) else { return [] } | ||
| let attributes = try FileManager.default.attributesOfItem(atPath: removedItemsFileURL.path) | ||
| let fileSize = (attributes[.size] as? NSNumber)?.intValue ?? 0 | ||
| guard fileSize <= maximumLegacyReadBytes else { return [] } | ||
| let data = try Data(contentsOf: removedItemsFileURL) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Fail closed when the legacy tombstone file exceeds the read cap.
Returning an empty set treats every prior removal as absent, so deleted pending rows in the legacy tail can be restored. Report migration as unavailable and skip restoration, or implement a bounded migration that preserves deletion semantics.
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamPersistence.swift`
around lines 147 - 152, The loadLegacyRemovedItemIDs migration must fail closed
when the legacy tombstone file exceeds maximumLegacyReadBytes: do not return an
empty Set, which permits deleted rows to be restored. Propagate an
unavailable/failed migration outcome through the caller so restoration is
skipped while preserving deletion semantics; keep normal bounded-file loading
unchanged.
Comment on lines
69
to
+76
| public func start() async { | ||
| if let persistence { | ||
| if let page = try? await persistence.loadPage(limit: min(initialLoadLimit, ringCapacity)) { | ||
| items = page.items | ||
| hasMorePersistedItems = page.hasMoreBefore | ||
| oldestLoadedPersistenceOffset = page.startOffset | ||
| if let restored = try? await persistence.loadPendingItems(limit: ringCapacity) { | ||
| items = restored.suffix(ringCapacity).map { $0.retainedForFeed() } | ||
| rebuildContextIndex() | ||
| } | ||
| persistenceGeneration &+= 1 | ||
| try? await persistence.replacePendingItems(items, generation: persistenceGeneration) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not compact after a failed restore.
When loadPendingItems throws, items remains empty and Line 76 immediately replaces or deletes the persisted snapshot as though the empty state were authoritative. Compact only after a successful load; otherwise preserve the file or propagate the failure.
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swift`
around lines 69 - 76, Update start() so persistence.replacePendingItems is
called only after loadPendingItems succeeds; when restoration fails, preserve
the existing persisted snapshot by skipping compaction, or propagate the load
error instead. Keep the restored-item mapping and rebuildContextIndex behavior
unchanged for successful loads.
Comment on lines
+120
to
+127
| public func removeItem(id: UUID) async throws -> Bool { | ||
| guard items.contains(where: { $0.id == id }) else { return false } | ||
| items.removeAll { $0.id == id } | ||
| rebuildContextIndex() | ||
| if let persistence { | ||
| persistenceGeneration &+= 1 | ||
| try await persistence.replacePendingItems(items, generation: persistenceGeneration) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not rebuild the context cache from retained cards after removal.
The cache also contains telemetry-only context that is absent from items. Removing one unrelated card therefore erases prompt/preamble context for sessions that have not produced an actionable card yet. Preserve the cache or update only the affected workstream.
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swift`
around lines 120 - 127, The removeItem(id:) implementation should not call
rebuildContextIndex() after deleting a card, because that discards
telemetry-only context absent from items. Preserve the existing context cache or
update only the removed item’s affected workstream, while leaving persistence
generation and replacement behavior unchanged.
Comment on lines
+181
to
+191
| private func schedulePendingSnapshot() { | ||
| guard let persistence else { return } | ||
| persistenceGeneration &+= 1 | ||
| let generation = persistenceGeneration | ||
| let pendingItems = items.filter { $0.status.isPending } | ||
| Task { [persistence, pendingItems] in | ||
| try? await persistence.replacePendingItems( | ||
| pendingItems, | ||
| generation: generation | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Own snapshot writes and handle failures.
This fire-and-forget task discards disk errors and has no flushable lifecycle. If it fails or is interrupted, resolved or expired cards remain pending on disk and reappear after restart. Use an owned/coalesced writer task or an awaited persistence path.
As per coding guidelines, meaningful asynchronous work must be stored, cancellable, or tied to a caller-owned operation.
🤖 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/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamStore.swift`
around lines 181 - 191, Update schedulePendingSnapshot so the asynchronous
replacePendingItems operation is owned by a stored, cancellable writer task or
awaited through a caller-owned persistence path. Coalesce successive snapshots
while preserving the latest persistenceGeneration and pendingItems, and handle
replacement failures rather than discarding them, ensuring interrupted or failed
writes cannot leave resolved or expired cards persisted as pending.
Source: Coding guidelines
Comment on lines
+96
to
+98
| let restored = WorkstreamStore(persistence: persistence, ringCapacity: 10) | ||
| await restored.start() | ||
| #expect(restored.items.map(\.id) == [kept.id]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a new persistence actor to model restart.
Reusing persistence carries its in-memory latestGeneration, so this can hide initialization or generation defects. Construct a new WorkstreamPersistence(fileURL: tmp) for restored.
Proposed fix
- let restored = WorkstreamStore(persistence: persistence, ringCapacity: 10)
+ let restartedPersistence = WorkstreamPersistence(fileURL: tmp)
+ let restored = WorkstreamStore(
+ persistence: restartedPersistence,
+ ringCapacity: 10
+ )📝 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
| let restored = WorkstreamStore(persistence: persistence, ringCapacity: 10) | |
| await restored.start() | |
| #expect(restored.items.map(\.id) == [kept.id]) | |
| let restartedPersistence = WorkstreamPersistence(fileURL: tmp) | |
| let restored = WorkstreamStore( | |
| persistence: restartedPersistence, | |
| ringCapacity: 10 | |
| ) | |
| await restored.start() | |
| `#expect`(restored.items.map(\.id) == [kept.id]) |
🤖 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/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/Workstream/WorkstreamStoreTests.swift`
around lines 96 - 98, Update the restart setup in WorkstreamStoreTests by
constructing a new WorkstreamPersistence with the same temporary file URL used
by the original persistence, then pass that fresh actor to WorkstreamStore. Keep
the existing ringCapacity, start call, and restored item assertion unchanged.
lawrencecchen
added
the
stale-revisit
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
2 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
feed-ui-enabled-release: Debug is enabled for dogfood, Release fails closed, and the PostHog rollout is active at 0%Verification
898c25133bwith tagfd8174feed.jumppreflight returnedmatched: true, selected surface69232B6A-3767-478F-9E29-D818008FA2DC, and recorded flash count 1Answered: Answering a questionfeed.listexcluded injected telemetry and both legacy history files remained absent after resolutionNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Ships Feed by default behind the
feed-ui-enabled-releaseflag with clear entry points: right sidebar, “Open Feed as Pane”, and a pinned.feedworkspace. Addsfeed.jumpwith main‑actor terminal focus, attention and notifications, bounded waits, and tighter retention/restore for pending decisions.New Features
feed-ui-enabled-release; show in right sidebar, command palette (“Open Feed as Pane”), and titlebar new‑workspace menu; create a native.feedpinned workspace viaFeedOpeningCoordinator.feed.jumpon the control socket to run the same UI focus used by Feed rows; resolve sessions via a file‑backed lookup, prefer live surfaces, and follow stable surface identities; share a process‑wide projection viaFeedPresentationStoreand post inline‑action notifications using asyncUNUserNotificationCenter.Bug Fixes
feed.jumptargets and execute on the UI lane; add tests for jump dispatch, live surface ownership, and identity resolution.FeedBlockingWaiterRegistry/FeedBlockingCallBridge; split waiter completion to close reply/timeout races; cancel timed‑out ingests and restore reply/notification fallbacks.Written for commit 898c251. Summary will update on new commits.
Summary by CodeRabbit
New Features
Updates
Bug Fixes