Skip to content

Fix current Swift warning regressions - #8898

Merged
1 commit merged into
mainfrom
fix/swift-warning-budget-current-main
Jul 26, 2026
Merged

1 commit merged into
mainfrom
fix/swift-warning-budget-current-main

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Current main fails the speculative merge gate because three newly introduced warnings exceed the checked Swift warning budget.

This isolates the remote-mirror snapshot helper to MainActor, resolves the feature-flag singleton inside ContentView.init, and restores the group-header reuse check to inspect stored presentation state. The existing suspension regression test covers the sidebar behavior.

Verified by tagged cloud build swbud on macOS 26. The build log contains none of the three failing warning signatures. No user-facing strings changed.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Restores main to within the Swift warning budget by fixing three new warnings. No functional or UI changes.

  • Bug Fixes
    • Annotated SessionWindowSnapshot.omitsRemoteMirrorOnlyWindow with @MainActor to silence cross-actor access warnings.
    • Updated ContentView init to accept CmuxFeatureFlags? and default to .shared inside, removing the static default-arg warning.
    • In SidebarGroupHeaderTableCellView, compute requiresFullApply from self.actions so reuse checks inspect stored presentation state.

Written for commit 8ad567f. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Improved sidebar group header updates so available actions and layouts refresh correctly when first displayed.
    • Improved window handling for remote mirror-only sessions to ensure they are filtered consistently.
  • Refactor
    • Improved feature configuration initialization for more reliable app startup behavior.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b499f17b-f013-4171-a87a-a7c9ba461941

📥 Commits

Reviewing files that changed from the base of the PR and between 46cc01f and 8ad567f.

📒 Files selected for processing (3)
  • Sources/AppDelegate+WindowDock.swift
  • Sources/ContentView.swift
  • Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift

📝 Walkthrough

Walkthrough

Three targeted Swift changes update main-actor isolation for window snapshot filtering, optional feature flag initialization with a shared fallback, and sidebar header full-apply detection based on stored action state.

Changes

Window dock concurrency

Layer / File(s) Summary
Window snapshot actor contract
Sources/AppDelegate+WindowDock.swift
SessionWindowSnapshot.omitsRemoteMirrorOnlyWindow(liveWorkspaces:) is annotated with @MainActor without changing its implementation.

Content view configuration

Layer / File(s) Summary
Feature flag initializer fallback
Sources/ContentView.swift
The featureFlags initializer parameter is optional and defaults to nil; stored configuration falls back to .shared.

Sidebar group header state

Layer / File(s) Summary
Sidebar action initialization state
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift
The full-apply condition checks the cell’s existing self.actions state instead of the incoming actions parameter.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • manaflow-ai/cmux#8690: Introduced the window snapshot filtering method in the Dock session restore flow.
  • manaflow-ai/cmux#8892: Contains the same targeted edits to actor isolation, feature flag initialization, and sidebar action handling.

Suggested reviewers: austinywang, azooz2003-bit

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is informative but does not follow the required template headings or include the review trigger and checklist sections. Add the required Summary, Testing, Demo Video, Review Trigger, and Checklist sections in the repository template format.
✅ Passed checks (24 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing Swift warning regressions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed The diff tightens isolation: the snapshot helper and sidebar cell are on MainActor, and ContentView still resolves the existing @MainActor feature-flag singleton in UI code.
Cmux Swift Blocking Runtime ✅ Passed The PR only adds MainActor, changes a defaulted optional, and tweaks a nil-check; no new waits, sleeps, syncs, or locks were introduced.
Cmux Browser Automation Off-Main ✅ Passed Diff only changes three UI/helper files; no browser router, worker policy, or policy tests were touched.
Cmux Expensive Synchronous Load ✅ Passed Diff only adds @MainActor to a snapshot predicate and tweaks a feature-flag default; no synchronous agent-history load, JSON/JSONL parse, or scan was added.
Cmux Cache Substitution Correctness ✅ Passed No authoritative-read→cache swap in a persisted/snapshot path; the edits are actor isolation, a default-parameter refactor, and a reuse-state guard fix.
Cmux No Hacky Sleeps ✅ Passed PR only touches Swift actor/initializer/reuse logic; no covered TS/JS/shell/runtime-script changes or new sleeps/polls/delays are introduced.
Cmux Algorithmic Complexity ✅ Passed Changes are limited to actor isolation, a nil-coalescing initializer default, and a guard predicate fix; no new scalable scans, sorts, or batch rescans were introduced.
Cmux Swift Concurrency ✅ Passed The diff only adds MainActor isolation, makes a constructor arg optional, and fixes a reuse guard; it introduces no new Dispatch/Combine/Task anti-patterns.
Cmux Swift @Concurrent ✅ Passed The diff adds only @MainActor to a synchronous helper and adjusts two synchronous UI-bound call paths; no @concurrent misuse or missing nonisolated async boundary appears.
Cmux Swift Package Boundaries ✅ Passed PASS: The edits are app-target UI/lifecycle glue (ContentView init, AppDelegate snapshot helper, AppKit sidebar cell) and don’t keep reusable domain logic in the app target.
Cmux Swiftpm Lockfiles ✅ Passed PR diff only touches Swift source files; no .gitignore, Package.resolved, Xcode project, or SwiftPM dependency files changed, so the rule isn’t triggered.
Cmux Swift Logging ✅ Passed The patch only changes actor isolation, a defaulted parameter, and a reuse guard; it adds no print/debugPrint/dump/NSLog or Logger changes.
Cmux User-Facing Error Privacy ✅ Passed The diff only adds @MainActor and changes internal fallback/state checks; it doesn’t add or alter any user-facing error, alert, or recovery text.
Cmux Full Internationalization ✅ Passed Diff only changes actor isolation and internal logic; no user-facing text or localization resources were added or modified.
Cmux Swiftui State Layout ✅ Passed The diff only changes a non-view helper, a value-parameter fallback in ContentView.init, and an AppKit cell; no new SwiftUI state/layout patterns were introduced.
Cmux Architecture Rethink ✅ Passed Only local correctness fixes: MainActor isolation, nil-coalesced feature-flag fallback, and a reuse check that reads stored cell state; no timing/observer/split-ownership patterns.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: the diff only changes MainActor, a featureFlags default, and a reuse guard; no standalone window/controller or cmuxAuxiliaryWindowIdentifiers changes.
Cmux Source Artifacts ✅ Passed All changed paths are handwritten Swift source files; the diff only tweaks code, with no logs, caches, build output, or scratch artifacts added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The touched code only adds @MainActor, changes a feature-flag default, and adjusts a reuse guard; no new test/debug seam or DEBUG-only accessor was added.
Cmux No Ambient Global State ✅ Passed No new ambient global surface was introduced: only an actor annotation, an injectable init fallback to existing .shared, and an instance-state reuse check changed.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/swift-warning-budget-current-main

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Three targeted fixes for Swift warning regressions that were blocking the speculative merge gate: a missing @MainActor annotation, a singleton accessed as a default argument, and a comparison against the wrong actions variable. No user-facing behavior changes; the sidebar full-apply fix also corrects a latent reuse bug where reconfigured cells after suspension never received a complete model apply.

  • AppDelegate+WindowDock.swift: @MainActor added to omitsRemoteMirrorOnlyWindow because the function accesses @MainActor-bound Workspace key paths via allSatisfy; all callers are already on the main actor so there is no behavioral change.
  • ContentView.swift: featureFlags default moved from a @MainActor-isolated expression in argument position (= .shared) to a nil-coalescing assignment inside the init body (?? .shared), where ContentView's inferred @MainActor isolation makes the access safe; the injectable seam for tests is preserved.
  • SidebarGroupHeaderRowView.swift: requiresFullApply now checks self.actions (the stored optional property, nil after suspendPresentation()) instead of the non-optional actions parameter (which was always non-nil), restoring correct first-configure semantics on recycled cells.

Confidence Score: 5/5

All three changes are minimal, mechanically correct fixes with no user-facing side effects and no new API surface introduced.

Each change addresses the stated root cause — actor isolation annotation, default-argument isolation, and a wrong nil-comparison variable — without adding flags, side channels, or new state. The sidebar cell fix also closes a real reuse gap where cells after suspension never received a full model apply, but the scope is narrow and the existing suspension regression test covers the observable behavior.

Files Needing Attention: No files require special attention; all three touch sites are small and self-contained.

Important Files Changed

Filename Overview
Sources/AppDelegate+WindowDock.swift Adds @mainactor to omitsRemoteMirrorOnlyWindow to resolve a Swift 6 isolation warning; fix is correct because the function calls allSatisfy(.isRemoteTmuxMirror) on Workspace values that are @MainActor-bound, and all call sites are already on the main actor.
Sources/ContentView.swift Changes featureFlags default parameter from CmuxFeatureFlags = .shared to CmuxFeatureFlags? = nil, deferring singleton access to the init body; resolves a Swift 6 warning about @mainactor expressions in default argument position while preserving the injectable seam for tests.
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift Fixes requiresFullApply to check self.actions (the stored optional property) instead of the actions parameter (always non-nil), restoring correct full-apply semantics on first configure after cell suspension; also eliminates the Swift compiler warning about comparing a non-optional to nil.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["omitsRemoteMirrorOnlyWindow\n(SessionWindowSnapshot extension)"]
    B["@MainActor annotation added\n-> resolves Workspace.isRemoteTmuxMirror\nisolation warning"]
    A --> B

    C["ContentView.init\n(featureFlags: CmuxFeatureFlags = .shared)"]
    D["Swift 6 warning:\n@MainActor expression\nas default argument"]
    E["Fix: featureFlags: CmuxFeatureFlags? = nil\nself.featureFlags = featureFlags ?? .shared\n(evaluated inside @MainActor init body)"]
    C --> D --> E

    F["SidebarGroupHeaderTableCellView.configure\nrequiresFullApply = actions == nil"]
    G["Bug: 'actions' parameter is non-optional\n-> comparison always false\n-> full-apply never triggered on reuse"]
    H["Fix: self.actions == nil\n-> checks stored optional property\n-> full-apply on first configure after suspend"]
    F --> G --> H
Loading

Reviews (1): Last reviewed commit: "Fix current Swift warning regressions" | Re-trigger Greptile

@lawrencecchen lawrencecchen closed this pull request by merging all changes into main in 7c82622 Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant