ContentView drain: command palette (stack E)#6029
Merged
Merged
Conversation
…e (stack E) Wave 2 of the ContentView god-file drain. Lifts the pure command-palette search/orchestration/value layer out of ContentView.swift and Sources/CommandPalette/ into a new CmuxCommandPalette domain package (no AppKit, fully testable). - New package CmuxCommandPalette (exemplar-shaped Package.swift: tools 6.0, macOS 14, v6 language mode, ExistentialAny/InternalImportsByDefault, test target). - ~22 nested CommandPalette* value/model types removed from ContentView.swift (CommandPaletteMode, PendingActivation, ResolvedActivation, RenameTarget, WorkspaceDescriptionTarget, etc.) and re-homed one-type-per-file under Values/Context/Handling/Orchestration/Policy/Search/. - CommandPaletteSearch/NucleoSearch/SearchOrchestrator moved verbatim from Sources/CommandPalette/ into the package; bodies byte-identical (machine-diffed vs base), only public/DocC added and grab-bag types split out. - Dropped the mergedSwiftFallbackMatchesForTests production shim; the real method is now internal and exercised directly by package tests (no test-only methods in source). - Nucleo dylib #filePath path walk adjusted for the deeper package location; nucleo FFI tests pass against it. - ContentView.swift 19265 -> ~18940 lines; ContentView+* palette extensions and app-side call sites import CmuxCommandPalette. 51 package tests pass (swift test). App compile + budget/lint gates run on rebase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts command-palette domain into CmuxCommandPalette and CmuxAppKitSupportUI packages; exposes fuzzy-matcher and search engine APIs; adds Nucleo FFI search wrapper, tests, fixtures, and CI build/export for the Rust dylib; and updates Xcode and app imports to consume the packages. ChangesCmuxCommandPalette package extraction
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Closes the only gap in the lifted Wave 2 command-palette tranche: the app-target test files still referenced symbols and shims that moved into CmuxCommandPalette. - Import CmuxCommandPalette (hoisted above the canImport(cmux_DEV) block) in the two app-target palette test files so the moved public search/orchestrator symbols resolve. - Remove the two app-target tests that called the deleted production test-only shims (testSwiftFallbackMergeKeepsCombinedResultsSortedByScore, testCommandPreviewSearchUsesFullCommandCorpus); both are covered byte-identically by the package suite via @testable access to the now-internal real methods. - Remaining app-target palette tests (ContentView/ForkableAgent app-glue and the app-bundle dylib presence test) keep running unchanged. - Refresh the Swift file length budget for the moved package files and the shrunk app test file. Gates green: budget exit 0, lint-ios-package-conventions exit 0, CmuxCommandPalette swift build + 51 swift tests pass, full app compiles (xcodebuild -derivedDataPath /tmp/cmux-cview build -> BUILD SUCCEEDED), pbxproj normalize/check/ test-wiring clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (stack E) The Wave 2 lift left CommandPaletteOverlayPromotionPolicy declared in BOTH ContentView.swift (app-internal) and CmuxCommandPalette (public). The app compiled only because the app-internal copy shadowed the package type at the one call site, leaving the lifted package version dead and a name collision latent (the app-side-duplicate-plus-unconditional-import ambiguity trap). - Delete the app-side enum from ContentView.swift; the single call site (overlay re-promotion on hidden->visible) now binds to the package's public CommandPaletteOverlayPromotionPolicy. - Remove the now-redundant app-target CommandPaletteOverlayPromotionPolicyTests; the package suite covers all four transition cases identically. - Refresh the Swift file length budget for the shrunk ContentView/test files. Swept all 40 CmuxCommandPalette public types against app Sources/: this was the only remaining lifted-but-not-removed duplicate. ContentView.swift 18,942 -> 18,936. App compiles (BUILD SUCCEEDED), 51 package tests pass, budget + lint exit 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Greptile SummaryStack E (tranches 1–4) of the ContentView god-object drain: four independent slices are lifted out of
Confidence Score: 5/5All four tranches are machine-verified faithful lifts with green package tests and a green app compile; no behavior is changed. Every moved type has its body byte-identical to the pre-tranche tree. Actor isolation is correct throughout (value types are nonisolated, UI coordinators are @mainactor, @unchecked Sendable is documented). The nucleo #filePath depth is correctly updated. The only observation is a missing in-code comment on the DispatchQueue.main.async deferral in ScrollBackgroundClearer, which the PR description already explains. No files require special attention; the DispatchQueue.main.async in ScrollBackgroundClearer.swift is intentional and documented in the PR description. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
APP["App Target\n(ContentView.swift -588 lines)"]
subgraph PKG1["CmuxCommandPalette (new)"]
CP_VAL["Values/ (~22 types)"]
CP_CTX["Context/ (Snapshot, Keys)"]
CP_SRC["Search/ (Engine + FuzzyMatcher)"]
CP_NUC["Search/Nucleo/ (NucleoIndex + Library)"]
CP_ORC["Orchestration/ (SearchOrchestrator)"]
CP_HDL["Handling/ (Registry, ActionHandling)"]
CP_POL["Policy/ (OverlayPromotionPolicy)"]
end
subgraph PKG2["CmuxAppKitSupportUI (new)"]
UI_POP["Popover/ (ArrowlessPopoverAnchor)"]
UI_MOUSE["Mouse/ (MiddleClickCapture)"]
UI_SCROLL["Scroll/ (ClearScrollBackground, SidebarScrollViewConfigurator, SidebarScrollViewResolverView)"]
end
subgraph PKG3["CmuxFoundation (extended)"]
FND_HEX["NSColor+HexString"]
FND_NIL["String+NilIfEmpty"]
end
APP -- "import CmuxCommandPalette" --> PKG1
APP -- "import CmuxAppKitSupportUI" --> PKG2
APP -- "import CmuxFoundation (11 files)" --> PKG3
CP_NUC -- "dlopen FFI" --> RUST["Rust nucleo dylib"]
Reviews (4): Last reviewed commit: "ContentView drain: retarget SidebarScrol..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+31
to
+52
| switch kind { | ||
| case .workspace: | ||
| return String(localized: "commandPalette.rename.workspaceTitle", defaultValue: "Rename Workspace", bundle: .main) | ||
| case .tab: | ||
| return String(localized: "commandPalette.rename.tabTitle", defaultValue: "Rename Tab", bundle: .main) | ||
| } | ||
| } | ||
|
|
||
| /// Localized editor description. | ||
| public var description: String { | ||
| switch kind { | ||
| case .workspace: | ||
| return String(localized: "commandPalette.rename.workspaceDescription", defaultValue: "Choose a custom workspace name.", bundle: .main) | ||
| case .tab: | ||
| return String(localized: "commandPalette.rename.tabDescription", defaultValue: "Choose a custom tab name.", bundle: .main) | ||
| } | ||
| } | ||
|
|
||
| /// Localized input placeholder. | ||
| public var placeholder: String { | ||
| switch kind { | ||
| case .workspace: |
Contributor
There was a problem hiding this comment.
bundle: .main couples domain package to the app bundle
CommandPaletteRenameTarget (and CommandPaletteWorkspaceDescriptionTarget) use bundle: .main to look up localized strings. At runtime in the app this works, but it makes the package silently non-portable: in any context where Bundle.main is not the cmux .app bundle — including swift test runs for this very package — every localized property (title, description, placeholder) resolves to its defaultValue English string. If a catalog key is ever renamed or removed, package tests will never catch the regression because they never load the app bundle. The PR's stated goal is a "domain-pure, no AppKit, fully testable" package; bundle: .main is an implicit app-bundle dependency that sits outside that contract. Consider either keeping these localized-text computed properties app-side (where Bundle.main is appropriate) or vending a resource bundle via Bundle.module backed by a thin LocalizationStrings resource target inside the package.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Around line 4290-4294: Add the missing package product dependency entry for
CmuxCommandPalette to the cmux target's packageProductDependencies: locate the
PBXNativeTarget "cmux" block and append the existing dependency object
C9A1C00000000000000000A2 /* CmuxCommandPalette */ into its
packageProductDependencies array (the same format as other entries), ensuring
the isa XCSwiftPackageProductDependency, package reference
C9A1C00000000000000000A1 /* XCLocalSwiftPackageReference "CmuxCommandPalette"
*/, and productName = CmuxCommandPalette are present so the project wiring is
consistent with the Frameworks linkage.
In
`@Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteActionHandling.swift`:
- Around line 10-13: Change the handler closure contract on
CommandPaletteActionHandling to require main-actor isolation: update the
register(commandId:handler:) parameter type to accept a `@MainActor-isolated`
closure and change handler(for:) to return an optional `@MainActor-isolated`
closure (i.e., use the `@MainActor` closure type for both the stored handler and
the returned value). Update all conforming implementations of
CommandPaletteActionHandling and any call sites (e.g., where ContentView invokes
command.action()) to accept and invoke the `@MainActor` closures (or explicitly
hop to MainActor) so the stored handlers and their invocation are main-actor
isolated.
In
`@Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/CommandPaletteSearchCorpusEntry.swift`:
- Around line 24-55: The initializer public init(payload: Payload, rank: Int,
title: String, searchableTexts: [String]) currently only processes
searchableTexts and can omit the title from preparedSearchableTexts and related
structures; update the init in CommandPaletteSearchCorpusEntry to ensure the
title is always included as a searchable candidate by inserting the trimmed
title into nucleoSearchTexts and the title's normalized form (normalizedTitle)
into normalizedTexts/seen before the loop (or immediately after computing
normalizedTitle/preparedTitle), then let preparedSearchableTexts,
searchableTextSet, searchablePrefixScoreByToken, and nucleoSearchText be derived
as before so the title participates in matching and prefix scoring (ensure you
dedupe via seen to avoid duplicates).
In
`@Packages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteNucleoFFILibrarySupport.swift`:
- Around line 60-88: The initializer opens a native handle with dlopen but if
any of the Self.symbol(...) calls (e.g., "cmux_nucleo_index_create",
"cmux_nucleo_index_destroy", "cmux_nucleo_index_search",
"cmux_nucleo_ffi_version") throw, the handle is never dlclose'd and leaks; fix
by adding a defer immediately after dlopen(path, ...) that calls dlclose(handle)
on failure and cancels the close when initialization completes successfully (for
example, set a Bool success flag or nil-out a temporary handle) so that on any
thrown error the defer will close the handle but on success the real self.handle
remains open.
In
`@Packages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteNucleoFFITests.swift`:
- Around line 10-13: The test currently uses "guard let library = try
NucleoLibrary.loadIfAvailable() else { return }" in
nucleoFFIPrefersOpenFolderForOpenFolderQuery which silently marks the test
passed when the FFI dylib is missing; replace the silent return with a test skip
by throwing XCTSkip("nucleo FFI dylib not available") (or an explicit XCTFail if
you prefer failing rather than skipping) so the absence is reported, and apply
the same change to all other guards that use
CommandPaletteNucleoSearchIndex(...) or NucleoLibrary.loadIfAvailable() that
currently "return" so they explicitly skip/fail instead of passing.
🪄 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: 733e772f-8a2b-4cc1-a6b7-a1a50247aea3
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (54)
.github/workflows/ci.ymlPackages/CmuxCommandPalette/Package.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Context/CommandPaletteCommandsContext.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Context/CommandPaletteContextKeys.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Context/CommandPaletteContextSnapshot.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteActionHandling.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteCommandContribution.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteHandlerRegistry.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Orchestration/CommandPaletteListScope.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Orchestration/CommandPaletteResolvedSearchMatch.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Orchestration/CommandPaletteSearchOrchestrator.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Orchestration/CommandPaletteUsageEntry.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Policy/CommandPaletteOverlayPromotionPolicy.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/CommandPaletteFuzzyMatcher.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/CommandPaletteSearchCorpusEntry.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/CommandPaletteSearchCorpusResult.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/CommandPaletteSearchEngine.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/CommandPaletteSwitcherSearchIndexer.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/CommandPaletteSwitcherSearchMetadata.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/Nucleo/CommandPaletteNucleoSearchIndex.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/Nucleo/CommandPaletteNucleoSearchLibrary.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/Nucleo/CommandPaletteNucleoSearchResult.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteCommand.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteInputFocusPolicy.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteInputFocusTarget.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteMode.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPalettePendingActivation.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPalettePendingActivationResolutionResult.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteRenameTarget.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteResolvedActivation.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteSearchResult.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteSwitcherFingerprintContext.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteSwitcherFingerprintSurface.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteSwitcherFingerprintWorkspace.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteTextSelectionBehavior.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteWorkspaceDescriptionTarget.swiftPackages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteNucleoFFILibrarySupport.swiftPackages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteNucleoFFITests.swiftPackages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteNucleoFixtures.swiftPackages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteOverlayPromotionPolicyTests.swiftPackages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteSearchEngineTests.swiftSources/App/TerminalDirectoryOpenSupport.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/ContentView+AuthCommandPalette.swiftSources/ContentView+MoveTabToNewWorkspace.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/ContentView+ViewCommandPalette.swiftSources/ContentView.swiftSources/ContentViewIdentifierCopyCommands.swiftSources/TextBoxMentionCandidateIndex.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CommandPaletteNucleoFFITests.swiftcmuxTests/CommandPaletteSearchEngineTests.swiftcmuxTests/WorkspaceManualUnreadTests.swift
Comment on lines
+10
to
+13
| func register(commandId: String, handler: @escaping () -> Void) | ||
|
|
||
| /// The action registered for `commandId`, when any. | ||
| func handler(for commandId: String) -> (() -> Void)? |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all command-palette handler signatures/call-sites align with main-actor closure semantics.
rg -n -C2 --type=swift 'register\(commandId:.*handler:.*\(\)\s*->\s*Void|handler\(for:.*\)\s*->\s*\(\(\)\s*->\s*Void\)\?' Packages Sources cmuxTestsRepository: manaflow-ai/cmux
Length of output: 1526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all usages of handler(for:) and direct retrieval/invocation patterns
rg -n --type=swift 'handler\(for:\s*\w+' Packages Sources cmuxTests
# 2) Find all calls where a returned handler closure is invoked, e.g. "handler()"
# (heuristic: look for ".handler(for:" then a subsequent "()" call nearby)
rg -n -C3 --type=swift '\.handler\(for:.*\)\s*\(\s*\)' Packages Sources cmuxTests
# 3) Find conformances/implementations of CommandPaletteActionHandling
rg -n --type=swift 'CommandPaletteActionHandling' Packages Sources cmuxTests
# 4) Show the relevant code for the protocol + registry
rg -n --type=swift 'public protocol CommandPaletteActionHandling|struct .*CommandPaletteHandlerRegistry|class .*CommandPaletteHandlerRegistry|func register\(commandId: String, handler: `@escaping` \(\) -> Void\)|func handler\(for commandId: String\) ->' \
Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteActionHandling.swift \
Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteHandlerRegistry.swiftRepository: manaflow-ai/cmux
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the call-site in ContentView.swift around the handler retrieval
sed -n '6225,6285p' Sources/ContentView.swift
# Show the protocol + registry implementation files fully (small enough) or key sections
sed -n '1,200p' Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteActionHandling.swift
sed -n '1,200p' Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteHandlerRegistry.swift
# Find every registration site (register(commandId:..., handler:...))
rg -n --type=swift 'register\(commandId:\s*[^,]+,\s*handler:\s*\{|' Packages Sources cmuxTests || true
rg -n --type=swift 'register\(commandId:\s*[^,]+,\s*handler:' Packages Sources cmuxTests || true
# Find every handlerRegistry.handler(for:) invocation (to catch any other call-sites)
rg -n --type=swift 'handlerRegistry\.handler\(for:' Sources Packages cmuxTests || true
# If action is invoked as a closure, look for "action()" within nearby region
rg -n --type=swift 'action\(\)' Sources/ContentView.swift || trueRepository: manaflow-ai/cmux
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the definition of CommandPaletteCommand and where its `action` is stored
rg -n --type=swift 'struct CommandPaletteCommand|typealias CommandPaletteCommand|enum CommandPaletteCommand' Packages/CmuxCommandPalette/Sources
rg -n --type=swift 'action:\s*action\b|let action\b|var action\b|CommandPaletteCommand\(' Sources Packages/CmuxCommandPalette/Sources
# Find where the command's action closure gets invoked
rg -n --type=swift '\baction\s*\(\s*\)' Packages/CmuxCommandPalette/Sources Sources
# Specifically look for execution pathways: "run", "perform", "execute", "trigger", "dismissOnRun"
rg -n --type=swift '\bperform\b|\bexecute\b|\btrigger\b|\brun\b|\bactivate\b|\bdidSelect\b|\bhandler\b|dismissOnRun' Packages/CmuxCommandPalette/SourcesRepository: manaflow-ai/cmux
Length of output: 18489
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect command invocation in ContentView.swift
sed -n '8580,8720p' Sources/ContentView.swift
# Inspect CommandPaletteCommand type
sed -n '1,120p' Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteCommand.swift
# Inspect pending activation + orchestration if it affects execution context
sed -n '1,120p' Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteCommandContribution.swift
sed -n '1,120p' Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPalettePendingActivationResolutionResult.swiftRepository: manaflow-ai/cmux
Length of output: 10289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check if ContentView.swift or the containing type is `@MainActor`
rg -n --type=swift '`@MainActor`' Sources/ContentView.swift
# 2) Show the type definition that contains runCommandPaletteCommand (around its enclosing scope)
# We'll locate the nearest surrounding "struct/class" near line ~8680.
python3 - <<'PY'
import re, pathlib
p = pathlib.Path("Sources/ContentView.swift")
text = p.read_text(encoding="utf-8", errors="ignore").splitlines()
target = 8680 # approx from earlier results
# find line index with "func runCommandPaletteCommand"
idx = None
for i,line in enumerate(text):
if "func runCommandPaletteCommand(" in line:
idx=i; break
print("runCommandPaletteCommand line:", idx+1 if idx is not None else None)
# print upwards until hit 'struct ' or 'class ' or 'actor ' start
if idx is not None:
start = idx
for j in range(idx, -1, -1):
if re.match(r'^\s*(struct|class|actor)\s+\w+', text[j]):
start=j
break
for j in range(start, min(start+60, len(text))):
print(f"{j+1}:{text[j]}")
PY
# 3) Confirm all command.action() call-sites that are specifically commandPalette commands.
rg -n --type=swift 'commandPalette.*action\(\)|runCommandPaletteCommand|CommandPaletteCommand\(' Sources/ContentView.swift
# 4) Confirm that "CommandPaletteCommand" action is only invoked in ContentView.swift.
rg -n --type=swift 'command\.action\(\)' Sources | head -n 50Repository: manaflow-ai/cmux
Length of output: 5539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate ContentView declaration and see whether it's annotated with `@MainActor`
rg -n --type=swift 'struct ContentView|`@MainActor`.*struct ContentView|actor ContentView|`@MainActor`.*actor' Sources/ContentView.swift
# Show a small window around the first ContentView struct definition
python3 - <<'PY'
import re, pathlib
p=pathlib.Path("Sources/ContentView.swift")
lines=p.read_text(encoding="utf-8", errors="ignore").splitlines()
for i,l in enumerate(lines):
if re.search(r'\bstruct ContentView\b', l):
start=max(0,i-20); end=min(len(lines), i+40)
for j in range(start,end):
print(f"{j+1}:{lines[j]}")
break
PY
# Inspect registerCommandPaletteHandlers definition and a small section where it's called
rg -n --type=swift 'registerCommandPaletteHandlers\(' Sources/ContentView.swift
python3 - <<'PY'
import re, pathlib
p=pathlib.Path("Sources/ContentView.swift")
lines=p.read_text(encoding="utf-8", errors="ignore").splitlines()
# print around the first occurrence of "func registerCommandPaletteHandlers"
for i,l in enumerate(lines):
if re.search(r'func\s+registerCommandPaletteHandlers\s*\(', l):
start=max(0,i-20); end=min(len(lines), i+120)
for j in range(start,end):
print(f"{j+1}:{lines[j]}")
break
PY
# Inspect runCommandPaletteCommand around its signature and immediate surrounding lines
sed -n '8660,8710p' Sources/ContentView.swiftRepository: manaflow-ai/cmux
Length of output: 14152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show lines around runCommandPaletteCommand signature (look for `@MainActor` annotation above)
nl -ba Sources/ContentView.swift | sed -n '8665,8710p'
# Show all call-sites to runCommandPaletteCommand near the earlier line numbers
rg -n --type=swift 'runCommandPaletteCommand\(' Sources/ContentView.swift
# For each call-site, show a small neighborhood to see if it occurs in an `@MainActor` context or from Task { `@MainActor` ... }
# We'll grab around each match line number.
python3 - <<'PY'
import re, pathlib
p=pathlib.Path("Sources/ContentView.swift")
lines=p.read_text(encoding="utf-8", errors="ignore").splitlines()
text=p.read_text(encoding="utf-8", errors="ignore")
# find line numbers for runCommandPaletteCommand( occurrences
for m in re.finditer(r'runCommandPaletteCommand\(', text):
# compute line number
# avoid expensive; approximate by counting newlines up to match
prefix=text[:m.start()]
ln=prefix.count('\n')+1
if ln<8500 or ln>9000:
continue
# print neighborhood
start=max(1, ln-10)
end=min(len(lines), ln+15)
print("\n--- context around line", ln, "---")
for i in range(start, end+1):
print(f"{i}:{lines[i-1]}")
PYRepository: manaflow-ai/cmux
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib, re
p=pathlib.Path("Sources/ContentView.swift")
lines=p.read_text(encoding="utf-8", errors="ignore").splitlines()
def print_block(center, before=12, after=18):
start=max(1, center-before)
end=min(len(lines), center+after)
for i in range(start, end+1):
print(f"{i}:{lines[i-1]}")
# find runCommandPaletteCommand signature
for i,l in enumerate(lines, start=1):
if "private func runCommandPaletteCommand(" in l:
print("SIGNATURE at", i)
print_block(i)
break
# find call-sites (runCommandPaletteCommand(...)
for i,l in enumerate(lines, start=1):
if "runCommandPaletteCommand(" in l and i!=0:
if "func runCommandPaletteCommand" in l:
continue
if 8500 <= i <= 9000:
print("\nCALLSITE at", i)
print_block(i, before=10, after=12)
PYRepository: manaflow-ai/cmux
Length of output: 3788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find call-sites for command palette submission / activation.
rg -n --type=swift 'handleCommandPaletteSubmitRequest\(|runCommandPaletteResult\(|runCommandPaletteResolvedActivation\(' Sources/ContentView.swift
# Show surrounding context for each handleCommandPaletteSubmitRequest call-site.
python3 - <<'PY'
import pathlib, re
p=pathlib.Path("Sources/ContentView.swift")
lines=p.read_text(encoding="utf-8", errors="ignore").splitlines()
# collect line numbers where handleCommandPaletteSubmitRequest( is referenced
targets=[]
for i,l in enumerate(lines, start=1):
if "handleCommandPaletteSubmitRequest()" in l:
targets.append(i)
print("call-sites:", targets)
def print_block(center, before=12, after=18):
start=max(1, center-before)
end=min(len(lines), center+after)
for j in range(start, end+1):
print(f"{j}:{lines[j-1]}")
for t in targets:
print("\n--- context around call-site", t, "---")
print_block(t)
PYRepository: manaflow-ai/cmux
Length of output: 4072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=swift 'commandPaletteSubmitRequested' Sources Packages cmuxTests
rg -n --type=swift 'NotificationCenter\.default\.post\(' Sources Packages cmuxTests | rg -n 'commandPaletteSubmitRequested|commandPaletteDismissRequested|commandPaletteRenameTabRequested' || true
rg -n --type=swift 'extension Notification\.Name|struct .*Notification|enum .*Notification' Sources Packages cmuxTests | rg -n 'commandPalette' || true
rg -n --type=swift '\.commandPaletteSubmitRequested\b' Sources Packages cmuxTests | head -n 50Repository: manaflow-ai/cmux
Length of output: 1369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant parts of AppDelegate around the post call(s)
sed -n '12820,12920p' Sources/AppDelegate.swift
# Show where the dismiss is posted (neighboring block)
sed -n '12740,12820p' Sources/AppDelegate.swift
# Show NotificationCenter publisher usage already found
sed -n '2860,2930p' Sources/ContentView.swiftRepository: manaflow-ai/cmux
Length of output: 12999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show exact blocks around the submit/dismiss notification posts
sed -n '12840,12910p' Sources/AppDelegate.swift
sed -n '12760,12820p' Sources/AppDelegate.swift
sed -n '12810,12895p' Sources/AppDelegate.swift
# Also locate any surrounding function/type that might be `@MainActor`
rg -n --type=swift '`@MainActor`|actor|class AppDelegate' Sources/AppDelegate.swift | head -n 50Repository: manaflow-ai/cmux
Length of output: 13673
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=swift 'CommandPaletteActionHandling' Packages/CmuxCommandPalette/Sources Sources cmuxTests
# Ensure command.action() is only invoked in the command-palette execution path
rg -n --type=swift '\bcommand\.action\(\)' Sources Packages cmuxTestsRepository: manaflow-ai/cmux
Length of output: 501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the palette execution functions are explicitly annotated `@MainActor`
rg -n --type=swift '`@MainActor`\s+private func runCommandPaletteCommand|`@MainActor`\s+private func runCommandPaletteResolvedActivation|`@MainActor`\s+private func runCommandPaletteResult|`@MainActor`\s+private func handleCommandPaletteSubmitRequest' Sources/ContentView.swift || true
# Show a few lines above each function definition to see if `@MainActor` is just above
python3 - <<'PY'
import pathlib
p=pathlib.Path("Sources/ContentView.swift")
lines=p.read_text(encoding="utf-8", errors="ignore").splitlines()
targets=[
"private func handleCommandPaletteSubmitRequest()",
"private func runCommandPaletteResult(commandID: String)",
"private func runCommandPaletteResolvedActivation(_ activation: CommandPaletteResolvedActivation)",
"private func runCommandPaletteCommand(_ command: CommandPaletteCommand)",
]
for t in targets:
for i,l in enumerate(lines):
if t in l:
start=max(0,i-6); end=min(len(lines),i+4)
print("\n===",t,"at line",i+1,"===")
for j in range(start,end):
print(f"{j+1}:{lines[j]}")
break
PY
# Confirm the posting site is on the main thread by finding the method/type containing those key shortcut routes (heuristic)
# We just want any `@MainActor` around the function that posts notifications in the excerpt.
rg -n --type=swift 'post\(name: \.commandPaletteSubmitRequested|post\(name: \.commandPaletteDismissRequested' Sources/AppDelegate.swift -n -C2Repository: manaflow-ai/cmux
Length of output: 3200
Make command-palette handler/action closures explicitly @MainActor
CommandPaletteActionHandling is @MainActor, but it registers/returns () -> Void closures, and ContentView invokes command.action() without any main-actor isolation on the call path—so the closure contract allows off-main execution of UI-bound handlers.
Proposed contract change
// Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteActionHandling.swift
`@MainActor`
public protocol CommandPaletteActionHandling {
- func register(commandId: String, handler: `@escaping` () -> Void)
+ func register(commandId: String, handler: `@escaping` `@MainActor` () -> Void)
/// The action registered for `commandId`, when any.
- func handler(for commandId: String) -> (() -> Void)?
+ func handler(for commandId: String) -> (`@MainActor` () -> Void)?
} // Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteHandlerRegistry.swift
public struct CommandPaletteHandlerRegistry {
- private var handlers: [String: () -> Void] = [:]
+ private var handlers: [String: `@MainActor` () -> Void] = [:]
public init() {}
- public mutating func register(commandId: String, handler: `@escaping` () -> Void) {
+ public mutating func register(commandId: String, handler: `@escaping` `@MainActor` () -> Void) {
handlers[commandId] = handler
}
- public func handler(for commandId: String) -> (() -> Void)? {
+ public func handler(for commandId: String) -> (`@MainActor` () -> Void)? {
handlers[commandId]
}
} // Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/Values/CommandPaletteCommand.swift
public struct CommandPaletteCommand: Identifiable {
...
- public let action: () -> Void
+ public let action: `@MainActor` () -> Void
...
public init(
...
dismissOnRun: Bool,
- action: `@escaping` () -> Void
+ action: `@escaping` `@MainActor` () -> Void
) {
...
self.action = action
}
} // Sources/ContentView.swift
- private func runCommandPaletteCommand(_ command: CommandPaletteCommand) {
+ `@MainActor` private func runCommandPaletteCommand(_ command: CommandPaletteCommand) {
...
command.action()
...
}🤖 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/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteActionHandling.swift`
around lines 10 - 13, Change the handler closure contract on
CommandPaletteActionHandling to require main-actor isolation: update the
register(commandId:handler:) parameter type to accept a `@MainActor-isolated`
closure and change handler(for:) to return an optional `@MainActor-isolated`
closure (i.e., use the `@MainActor` closure type for both the stored handler and
the returned value). Update all conforming implementations of
CommandPaletteActionHandling and any call sites (e.g., where ContentView invokes
command.action()) to accept and invoke the `@MainActor` closures (or explicitly
hop to MainActor) so the stored handlers and their invocation are main-actor
isolated.
Comment on lines
+24
to
+55
| public init(payload: Payload, rank: Int, title: String, searchableTexts: [String]) { | ||
| self.payload = payload | ||
| self.rank = rank | ||
| self.title = title | ||
| let normalizedTitle = CommandPaletteFuzzyMatcher.normalizeForSearch(title) | ||
| self.preparedTitle = CommandPaletteFuzzyMatcher.prepareNormalizedCandidateText(normalizedTitle) | ||
|
|
||
| var nucleoSearchTexts: [String] = [] | ||
| var normalizedTexts: [String] = [] | ||
| var seen: Set<String> = [] | ||
| normalizedTexts.reserveCapacity(searchableTexts.count) | ||
| nucleoSearchTexts.reserveCapacity(searchableTexts.count) | ||
| for text in searchableTexts { | ||
| let trimmedText = text.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if !trimmedText.isEmpty { | ||
| nucleoSearchTexts.append(trimmedText) | ||
| } | ||
| let normalizedText = CommandPaletteFuzzyMatcher.normalizeForSearch(text) | ||
| guard !normalizedText.isEmpty else { continue } | ||
| guard seen.insert(normalizedText).inserted else { continue } | ||
| normalizedTexts.append(normalizedText) | ||
| } | ||
|
|
||
| let preparedSearchableTexts = normalizedTexts.compactMap( | ||
| CommandPaletteFuzzyMatcher.prepareNormalizedCandidateText | ||
| ) | ||
| self.preparedSearchableTexts = preparedSearchableTexts | ||
| self.searchableTextSet = Set(normalizedTexts) | ||
| self.searchablePrefixScoreByToken = CommandPaletteFuzzyMatcher.wholeCandidatePrefixScoreByToken( | ||
| preparedCandidates: preparedSearchableTexts | ||
| ) | ||
| self.nucleoSearchText = nucleoSearchTexts.joined(separator: "\n") |
There was a problem hiding this comment.
Include title in normalized searchable candidates by default.
At Line 36, only searchableTexts are ingested. If a caller passes searchableTexts without the title, preparedSearchableTexts can miss title text, and CommandPaletteSearchEngine.weightedScore will return nil before title bonus logic runs, dropping valid title-only matches.
Suggested fix
public init(payload: Payload, rank: Int, title: String, searchableTexts: [String]) {
self.payload = payload
self.rank = rank
self.title = title
let normalizedTitle = CommandPaletteFuzzyMatcher.normalizeForSearch(title)
self.preparedTitle = CommandPaletteFuzzyMatcher.prepareNormalizedCandidateText(normalizedTitle)
+ let allSearchableTexts = [title] + searchableTexts
var nucleoSearchTexts: [String] = []
var normalizedTexts: [String] = []
var seen: Set<String> = []
- normalizedTexts.reserveCapacity(searchableTexts.count)
- nucleoSearchTexts.reserveCapacity(searchableTexts.count)
- for text in searchableTexts {
+ normalizedTexts.reserveCapacity(allSearchableTexts.count)
+ nucleoSearchTexts.reserveCapacity(allSearchableTexts.count)
+ for text in allSearchableTexts {
let trimmedText = text.trimmingCharacters(in: .whitespacesAndNewlines)
if !trimmedText.isEmpty {
nucleoSearchTexts.append(trimmedText)
}
let normalizedText = CommandPaletteFuzzyMatcher.normalizeForSearch(text)📝 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
| public init(payload: Payload, rank: Int, title: String, searchableTexts: [String]) { | |
| self.payload = payload | |
| self.rank = rank | |
| self.title = title | |
| let normalizedTitle = CommandPaletteFuzzyMatcher.normalizeForSearch(title) | |
| self.preparedTitle = CommandPaletteFuzzyMatcher.prepareNormalizedCandidateText(normalizedTitle) | |
| var nucleoSearchTexts: [String] = [] | |
| var normalizedTexts: [String] = [] | |
| var seen: Set<String> = [] | |
| normalizedTexts.reserveCapacity(searchableTexts.count) | |
| nucleoSearchTexts.reserveCapacity(searchableTexts.count) | |
| for text in searchableTexts { | |
| let trimmedText = text.trimmingCharacters(in: .whitespacesAndNewlines) | |
| if !trimmedText.isEmpty { | |
| nucleoSearchTexts.append(trimmedText) | |
| } | |
| let normalizedText = CommandPaletteFuzzyMatcher.normalizeForSearch(text) | |
| guard !normalizedText.isEmpty else { continue } | |
| guard seen.insert(normalizedText).inserted else { continue } | |
| normalizedTexts.append(normalizedText) | |
| } | |
| let preparedSearchableTexts = normalizedTexts.compactMap( | |
| CommandPaletteFuzzyMatcher.prepareNormalizedCandidateText | |
| ) | |
| self.preparedSearchableTexts = preparedSearchableTexts | |
| self.searchableTextSet = Set(normalizedTexts) | |
| self.searchablePrefixScoreByToken = CommandPaletteFuzzyMatcher.wholeCandidatePrefixScoreByToken( | |
| preparedCandidates: preparedSearchableTexts | |
| ) | |
| self.nucleoSearchText = nucleoSearchTexts.joined(separator: "\n") | |
| public init(payload: Payload, rank: Int, title: String, searchableTexts: [String]) { | |
| self.payload = payload | |
| self.rank = rank | |
| self.title = title | |
| let normalizedTitle = CommandPaletteFuzzyMatcher.normalizeForSearch(title) | |
| self.preparedTitle = CommandPaletteFuzzyMatcher.prepareNormalizedCandidateText(normalizedTitle) | |
| let allSearchableTexts = [title] + searchableTexts | |
| var nucleoSearchTexts: [String] = [] | |
| var normalizedTexts: [String] = [] | |
| var seen: Set<String> = [] | |
| normalizedTexts.reserveCapacity(allSearchableTexts.count) | |
| nucleoSearchTexts.reserveCapacity(allSearchableTexts.count) | |
| for text in allSearchableTexts { | |
| let trimmedText = text.trimmingCharacters(in: .whitespacesAndNewlines) | |
| if !trimmedText.isEmpty { | |
| nucleoSearchTexts.append(trimmedText) | |
| } | |
| let normalizedText = CommandPaletteFuzzyMatcher.normalizeForSearch(text) | |
| guard !normalizedText.isEmpty else { continue } | |
| guard seen.insert(normalizedText).inserted else { continue } | |
| normalizedTexts.append(normalizedText) | |
| } | |
| let preparedSearchableTexts = normalizedTexts.compactMap( | |
| CommandPaletteFuzzyMatcher.prepareNormalizedCandidateText | |
| ) | |
| self.preparedSearchableTexts = preparedSearchableTexts | |
| self.searchableTextSet = Set(normalizedTexts) | |
| self.searchablePrefixScoreByToken = CommandPaletteFuzzyMatcher.wholeCandidatePrefixScoreByToken( | |
| preparedCandidates: preparedSearchableTexts | |
| ) | |
| self.nucleoSearchText = nucleoSearchTexts.joined(separator: "\n") |
🤖 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/CmuxCommandPalette/Sources/CmuxCommandPalette/Search/CommandPaletteSearchCorpusEntry.swift`
around lines 24 - 55, The initializer public init(payload: Payload, rank: Int,
title: String, searchableTexts: [String]) currently only processes
searchableTexts and can omit the title from preparedSearchableTexts and related
structures; update the init in CommandPaletteSearchCorpusEntry to ensure the
title is always included as a searchable candidate by inserting the trimmed
title into nucleoSearchTexts and the title's normalized form (normalizedTitle)
into normalizedTexts/seen before the loop (or immediately after computing
normalizedTitle/preparedTitle), then let preparedSearchableTexts,
searchableTextSet, searchablePrefixScoreByToken, and nucleoSearchText be derived
as before so the title participates in matching and prefix scoring (ensure you
dedupe via seen to avoid duplicates).
Comment on lines
+60
to
+88
| private init(path: String) throws { | ||
| guard let handle = dlopen(path, RTLD_NOW | RTLD_LOCAL) else { | ||
| throw NSError( | ||
| domain: "CommandPaletteNucleoFFITests", | ||
| code: 1, | ||
| userInfo: [NSLocalizedDescriptionKey: "dlopen failed: \(Self.dlerrorText())"] | ||
| ) | ||
| } | ||
| self.handle = handle | ||
| self.createIndex = try Self.symbol( | ||
| "cmux_nucleo_index_create", | ||
| from: handle, | ||
| as: CreateIndex.self | ||
| ) | ||
| self.destroyIndex = try Self.symbol( | ||
| "cmux_nucleo_index_destroy", | ||
| from: handle, | ||
| as: DestroyIndex.self | ||
| ) | ||
| self.searchIndex = try Self.symbol( | ||
| "cmux_nucleo_index_search", | ||
| from: handle, | ||
| as: SearchIndex.self | ||
| ) | ||
| self.version = try Self.symbol( | ||
| "cmux_nucleo_ffi_version", | ||
| from: handle, | ||
| as: Version.self | ||
| ) |
There was a problem hiding this comment.
Close the dlopen handle on symbol-resolution throw paths.
At Lines 69-88, if Self.symbol(...) throws, initialization exits before deinit, so dlclose(handle) is skipped and the native handle leaks.
Proposed fix
private init(path: String) throws {
guard let handle = dlopen(path, RTLD_NOW | RTLD_LOCAL) else {
throw NSError(
domain: "CommandPaletteNucleoFFITests",
code: 1,
userInfo: [NSLocalizedDescriptionKey: "dlopen failed: \(Self.dlerrorText())"]
)
}
self.handle = handle
- self.createIndex = try Self.symbol(
- "cmux_nucleo_index_create",
- from: handle,
- as: CreateIndex.self
- )
- self.destroyIndex = try Self.symbol(
- "cmux_nucleo_index_destroy",
- from: handle,
- as: DestroyIndex.self
- )
- self.searchIndex = try Self.symbol(
- "cmux_nucleo_index_search",
- from: handle,
- as: SearchIndex.self
- )
- self.version = try Self.symbol(
- "cmux_nucleo_ffi_version",
- from: handle,
- as: Version.self
- )
+ do {
+ self.createIndex = try Self.symbol(
+ "cmux_nucleo_index_create",
+ from: handle,
+ as: CreateIndex.self
+ )
+ self.destroyIndex = try Self.symbol(
+ "cmux_nucleo_index_destroy",
+ from: handle,
+ as: DestroyIndex.self
+ )
+ self.searchIndex = try Self.symbol(
+ "cmux_nucleo_index_search",
+ from: handle,
+ as: SearchIndex.self
+ )
+ self.version = try Self.symbol(
+ "cmux_nucleo_ffi_version",
+ from: handle,
+ as: Version.self
+ )
+ } catch {
+ dlclose(handle)
+ throw error
+ }
let resolvedVersion = self.version()
guard resolvedVersion == Self.supportedVersion else {
dlclose(handle)
throw NSError(
domain: "CommandPaletteNucleoFFITests",🤖 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/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteNucleoFFILibrarySupport.swift`
around lines 60 - 88, The initializer opens a native handle with dlopen but if
any of the Self.symbol(...) calls (e.g., "cmux_nucleo_index_create",
"cmux_nucleo_index_destroy", "cmux_nucleo_index_search",
"cmux_nucleo_ffi_version") throw, the handle is never dlclose'd and leaks; fix
by adding a defer immediately after dlopen(path, ...) that calls dlclose(handle)
on failure and cancels the close when initialization completes successfully (for
example, set a Bool success flag or nil-out a temporary handle) so that on any
thrown error the defer will close the handle but on success the real self.handle
remains open.
Comment on lines
+10
to
+13
| @Test func nucleoFFIPrefersOpenFolderForOpenFolderQuery() throws { | ||
| guard let library = try NucleoLibrary.loadIfAvailable() else { return } | ||
| // Skipped: nucleo FFI dylib not built in this environment. | ||
| #expect(library.version() == 2) |
There was a problem hiding this comment.
Do not silently pass tests when the FFI dylib is unavailable.
At Line 11, else { return } marks the test as passed instead of skipped/failed, which can hide broken FFI build/wiring in CI. The same pattern appears repeatedly in this suite.
Proposed fix pattern
-guard let library = try NucleoLibrary.loadIfAvailable() else { return }
-// Skipped: nucleo FFI dylib not built in this environment.
+let library = try `#require`(NucleoLibrary.loadIfAvailable())Apply the same pattern to optional CommandPaletteNucleoSearchIndex(...) guards that currently 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
`@Packages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteNucleoFFITests.swift`
around lines 10 - 13, The test currently uses "guard let library = try
NucleoLibrary.loadIfAvailable() else { return }" in
nucleoFFIPrefersOpenFolderForOpenFolderQuery which silently marks the test
passed when the FFI dylib is missing; replace the silent return with a test skip
by throwing XCTSkip("nucleo FFI dylib not available") (or an explicit XCTFail if
you prefer failing rather than skipping) so the absence is reported, and apply
the same change to all other guards that use
CommandPaletteNucleoSearchIndex(...) or NucleoLibrary.loadIfAvailable() that
currently "return" so they explicitly skip/fail instead of passing.
…ortUI (stack E) Faithful lift of the shared AppKit-bridge UI primitives out of ContentView's preamble/tail into a new CmuxAppKitSupportUI package (Wave 3 of the ContentView plan): ArrowlessPopoverAnchor, MiddleClickCapture (+MiddleClickCaptureView), and ClearScrollBackground (+ScrollBackgroundClearer). Bodies are byte-identical to pre-tranche HEAD; the only deltas are package-seam mechanics: public visibility, synthesized public inits (cross-module memberwise init is inaccessible), public import SwiftUI on public-API files, and @mainactor on the popover Coordinator (ContentView was implicitly main-actor). App-side call sites in ContentView and SessionIndexView gain 'import CmuxAppKitSupportUI'; no shadow copies remain. ContentView.swift: 18936 -> 18699 lines. Package builds + 1 test pass; app compile BUILD SUCCEEDED; pbxproj 6-entry wiring mirrors CmuxCommandPalette, normalized + checked; file-length budget ratcheted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…E, Wave 1) Faithful Wave-1 foundation lift: the NSColor.hexString(includeAlpha:) helper moves out of ContentView's tail into Packages/CmuxFoundation as NSColor+HexString.swift, alongside the existing String+JavaScriptStringLiteral foundation extension precedent. Body is byte-identical to pre-tranche HEAD (machine-diffed); the only delta is public visibility + DocC + a file-scoped public import AppKit. Adopted at every call site in the same change (TASTE: extraction isn't done until adopted everywhere): all 11 app files that call .hexString (GhosttyTerminalView, Workspace, cmuxApp, GhosttyConfig, GhosttyTerminalAppearance, WorkspaceAppearanceResolution, WorkspaceContentView, WindowAppearanceSnapshot, WindowBackdropController, FeedButtonStyleDebugWindowController, ContentView) gain 'import CmuxFoundation'. Sole hexString declaration now lives in the package; no shadow copies remain. ContentView.swift: 18,699 -> 18,682 lines. CmuxFoundation builds + 4 new behavior tests pass (RGB/alpha/byte-rounding round-trips); app compile BUILD SUCCEEDED (0 errors); lint exit 0; budget ratcheted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…E, Wave 1) Faithful Wave-1 foundation lift: the String.nilIfEmpty helper moves out of ContentView's private extension into Packages/CmuxFoundation as String+NilIfEmpty.swift, next to the existing String+JavaScriptStringLiteral extension. Body is byte-identical to pre-tranche HEAD (machine-diffed); only delta is public visibility + DocC. ContentView already imports CmuxFoundation (prior tranche), so its 4 call sites resolve unchanged; the helper was ContentView-private with no other callers, so no further adoption is needed. Sole nilIfEmpty declaration now lives in the package. ContentView.swift: 18,682 -> 18,677 lines. CmuxFoundation builds + 3 new behavior tests pass; app compile BUILD SUCCEEDED (0 errors); lint exit 0; budget ratcheted down. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmux.xcodeproj/project.pbxproj (1)
2525-2559:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
CmuxAppKitSupportUIto thecmuxtargetpackageProductDependencies.
CmuxAppKitSupportUIis declared as anXCSwiftPackageProductDependency(Line 4302) and linked inFrameworks(Line 1598), but it is missing fromPBXNativeTarget "cmux"packageProductDependencies(Line 2525). This leaves target wiring inconsistent.As per coding guidelines,
cmux.xcodeproj/**changes must preserve correct project wiring for expected build/runtime behavior.🔧 Proposed fix
A5001050 /* cmux */ = { isa = PBXNativeTarget; @@ packageProductDependencies = ( @@ A8BD195031FC4B82B4354297 /* StackAuth */, EFB18E3B3099DFE2ECA3C263 /* CMUXMobileCore */, + C9A2B00000000000000000B2 /* CmuxAppKitSupportUI */, );Also applies to: 4302-4306, 1598-1602
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 2525 - 2559, The cmux target's packageProductDependencies array is missing the CmuxAppKitSupportUI Swift package product which is declared elsewhere as an XCSwiftPackageProductDependency and linked in Frameworks; open the PBXNativeTarget "cmux" block (the packageProductDependencies list) and add the CmuxAppKitSupportUI entry to that array (use the same identifier/name used in the XCSwiftPackageProductDependency declaration), ensuring it is listed alongside the other packageProductDependencies and not duplicated, then save so the target wiring matches the Frameworks linking.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
`@Packages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift`:
- Line 125: The code uses KVC on NSPopover (popover.setValue(true, forKeyPath:
"shouldHideAnchor")) which targets a private API; remove that KVC usage in
ArrowlessPopoverAnchor (or whatever type/method contains that line) and
implement a supported arrow-hiding strategy instead — for example, present the
NSPopover relative to a temporary transparent positioning view (create a
transient NSView, position it off-screen or outside the visible window bounds,
and call popover.show(relativeTo:of:preferredEdge:) using that view) or replace
the popover usage with a custom popover/NSWindow-based implementation if
deterministic arrow control is required; update any methods that reference
popover.setValue(...keyPath: "shouldHideAnchor") to use the new positioning-view
or custom popover approach.
---
Outside diff comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Around line 2525-2559: The cmux target's packageProductDependencies array is
missing the CmuxAppKitSupportUI Swift package product which is declared
elsewhere as an XCSwiftPackageProductDependency and linked in Frameworks; open
the PBXNativeTarget "cmux" block (the packageProductDependencies list) and add
the CmuxAppKitSupportUI entry to that array (use the same identifier/name used
in the XCSwiftPackageProductDependency declaration), ensuring it is listed
alongside the other packageProductDependencies and not duplicated, then save so
the target wiring matches the Frameworks linking.
🪄 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: da93c794-8b8b-4534-96ca-beeedc59a182
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (10)
Packages/CmuxAppKitSupportUI/Package.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Mouse/MiddleClickCapture.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Mouse/MiddleClickCaptureView.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Scroll/ClearScrollBackground.swiftPackages/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Scroll/ScrollBackgroundClearer.swiftPackages/CmuxAppKitSupportUI/Tests/CmuxAppKitSupportUITests/MiddleClickCaptureViewTests.swiftSources/ContentView.swiftSources/SessionIndexView.swiftcmux.xcodeproj/project.pbxproj
| let popover = NSPopover() | ||
| popover.behavior = .semitransient | ||
| popover.animates = true | ||
| popover.setValue(true, forKeyPath: "shouldHideAnchor") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is shouldHideAnchora documented public API/property forNSPopover in AppKit (macOS 14 through latest)? If not, what stable/public alternative exists to hide a popover arrow?
💡 Result:
The property shouldHideAnchor is not a documented public API for NSPopover in AppKit [1][2]. While it appears in some runtime headers [3][4] and has been discussed in developer forums as a potential private or internal flag that can be accessed via Key-Value Coding (e.g., popover.setValue(true, forKeyPath: "shouldHideAnchor")) [5], using private APIs is discouraged and may lead to app rejection or unexpected behavior in future macOS versions. There is no native, stable, or public public API to remove or hide the popover arrow in AppKit [6][7]. The most reliable, non-private "stable" workaround used by developers is to exploit the system's built-in behavior: the popover automatically hides its arrow when the positioning view is moved outside the visible rect of its window [8][9]. You can achieve this by: 1. Creating a secondary, transparent positioning view or using a placeholder view. 2. Presenting the popover relative to this view [8]. 3. Immediately moving the positioning view outside the visible bounds of the screen or window (e.g., by adjusting its frame to a negative coordinate), which triggers the system to hide the anchor arrow [8][9]. Alternatively, for more robust or custom control over the popover appearance and behavior, many developers choose to implement a custom popover using an NSWindow or utilize established, well-maintained third-party libraries [6].
Citations:
- 1: https://developer.apple.com/documentation/appkit/nspopover
- 2: https://apple-docs.everest.mt/docs/appkit/nspopover/
- 3: https://github.com/mstg/OSX-Runtime-Headers/blob/master/AppKit/NSPopover.h
- 4: https://github.com/w0lfschild/macOS_headers/blob/master/macOS/Frameworks/AppKit/1537.22/NSPopover.h
- 5: https://stackoverflow.com/questions/68744895/swift-ui-macos-menubar-nspopover-no-arrow
- 6: https://stackoverflow.com/questions/54670456/can-we-remove-nspopovers-arrow-in-macos-using-objective-c
- 7: https://developer.apple.com/forums/thread/676190
- 8: https://nyrra33.com/2018/08/08/a-small-trick-to-hide-nspopovers-arrow/
- 9: https://github.com/yigitkonur/skills-by-yigitkonur/blob/main/skills/apply-macos-hig/skills/apply-macos-hig/references/components/05-popovers-tooltips-and-disclosure.md
Avoid KVC against NSPopover private key (shouldHideAnchor).
ArrowlessPopoverAnchor.swift line 125 uses popover.setValue(true, forKeyPath: "shouldHideAnchor"), but shouldHideAnchor is not a documented public NSPopover API; relying on this private KVC contract risks runtime failures or broken popover presentation if AppKit changes.
- Use a non-private arrow-hiding approach instead (e.g., present relative to a temporary/transparent positioning view and move that view outside the visible window/screen bounds so AppKit hides the arrow), or switch to a custom popover implementation if you need deterministic arrow control.
🤖 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/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift`
at line 125, The code uses KVC on NSPopover (popover.setValue(true, forKeyPath:
"shouldHideAnchor")) which targets a private API; remove that KVC usage in
ArrowlessPopoverAnchor (or whatever type/method contains that line) and
implement a supported arrow-hiding strategy instead — for example, present the
NSPopover relative to a temporary transparent positioning view (create a
transient NSView, position it off-screen or outside the visible window bounds,
and call popover.show(relativeTo:of:preferredEdge:) using that view) or replace
the popover usage with a custom popover/NSWindow-based implementation if
deterministic arrow control is required; update any methods that reference
popover.setValue(...keyPath: "shouldHideAnchor") to use the new positioning-view
or custom popover approach.
…lor.hexString (stack E) test-depot (run 27454829534) failed: moving NSColor.hexString into CmuxFoundation left 7 cmuxTests files referencing it without the package import, so cmuxTests failed to compile (the app-host unit-test gate that package swift build and the no-test app build do not exercise — the documented Stack E tranche-1 trap). Adds 'import CmuxFoundation' (hoisted above the #if canImport(cmux_DEV) block, right after import AppKit) to: GhosttyConfigTests, GhosttyNotificationDispatcherTests, TerminalAndGhosttyTests, WindowAndDragTests, WindowAppearanceSnapshotTests, WorkspaceAppearanceConfigResolutionTests, WorkspaceUnitTests. The test target already links CmuxFoundation (SentryEventScrubberTests imports it on main), so no pbxproj change is needed. nilIfEmpty and the CmuxAppKitSupportUI types have zero cmuxTests references, so no further test imports are required. Budget ratcheted for the +1 import line on the three tracked large test files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 `@Packages/CmuxFoundation/Sources/CmuxFoundation/NSColor`+HexString.swift:
- Around line 8-13: The hexString(includeAlpha:) implementation must not
silently fall back to self when usingColorSpace(.sRGB) returns nil; instead,
check the result of usingColorSpace(.sRGB) and if it is nil return a sentinel
hex (e.g., "`#000000`" or "`#000000FF`" when includeAlpha is true). Locate
NSColor.hexString(includeAlpha:) and replace the current "let color =
usingColorSpace(.sRGB) ?? self" path with a guarded branch that calls
usingColorSpace(.sRGB), and if nil immediately returns the chosen sentinel
string; only then call color.getRed(_:green:blue:alpha:) on the guaranteed sRGB
color.
In
`@Packages/CmuxFoundation/Tests/CmuxFoundationTests/NSColor`+HexStringTests.swift:
- Around line 1-29: Add a unit test that exercises the fallback path of
NSColor.hexString when usingColorSpace(.sRGB) returns nil (e.g., pattern,
deviceGray, or other non-sRGB colors). Create a test (e.g.,
patternColorFallbackBehavior) that constructs a non-sRGB NSColor (like
NSColor(patternImage:) or a deviceGray color), calls hexString() and asserts the
documented/expected result for that case; reference
NSColor.hexString(includeAlpha:) and the usingColorSpace(.sRGB) conversion path
in the test so the behavior is explicit (either assert the actual current output
like "`#000000`" or assert and document the chosen fallback behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21d4bb52-91f0-49b7-8226-2d22597911d2
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (15)
Packages/CmuxFoundation/Sources/CmuxFoundation/NSColor+HexString.swiftPackages/CmuxFoundation/Sources/CmuxFoundation/String+NilIfEmpty.swiftPackages/CmuxFoundation/Tests/CmuxFoundationTests/NSColor+HexStringTests.swiftPackages/CmuxFoundation/Tests/CmuxFoundationTests/String+NilIfEmptyTests.swiftSources/ContentView.swiftSources/Feed/FeedButtonStyleDebugWindowController.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalAppearance.swiftSources/GhosttyTerminalView.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowBackdropController.swiftSources/Workspace.swiftSources/WorkspaceAppearanceResolution.swiftSources/WorkspaceContentView.swiftSources/cmuxApp.swift
Comment on lines
+1
to
+29
| import AppKit | ||
| import Testing | ||
|
|
||
| @testable import CmuxFoundation | ||
|
|
||
| /// Behavior tests for ``AppKit/NSColor/hexString(includeAlpha:)``: round-trips known | ||
| /// sRGB component values to their `#RRGGBB` / `#RRGGBBAA` encoding. | ||
| @Suite struct NSColorHexStringTests { | ||
| @Test func opaquePrimaryEncodesAsUppercaseRGB() { | ||
| let red = NSColor(srgbRed: 1, green: 0, blue: 0, alpha: 1) | ||
| #expect(red.hexString() == "#FF0000") | ||
| } | ||
|
|
||
| @Test func midGrayRoundsComponentsToBytes() { | ||
| let gray = NSColor(srgbRed: 0.5, green: 0.5, blue: 0.5, alpha: 1) | ||
| // 0.5 * 255 = 127.5 -> Int truncates to 127 -> 0x7F | ||
| #expect(gray.hexString() == "#7F7F7F") | ||
| } | ||
|
|
||
| @Test func includeAlphaAppendsAlphaByte() { | ||
| let translucent = NSColor(srgbRed: 0, green: 0, blue: 1, alpha: 0.5) | ||
| #expect(translucent.hexString(includeAlpha: true) == "#0000FF7F") | ||
| } | ||
|
|
||
| @Test func alphaIsOmittedByDefault() { | ||
| let translucent = NSColor(srgbRed: 0, green: 1, blue: 0, alpha: 0.25) | ||
| #expect(translucent.hexString() == "#00FF00") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding edge-case test for non-sRGB colors.
The test suite covers basic sRGB behavior but does not exercise the fallback path when usingColorSpace(.sRGB) returns nil (e.g., pattern or device-grayscale colors). If the API is expected to handle such colors, add a test case to document the behavior.
Example test case
`@Test` func patternColorFallbackBehavior() {
let pattern = NSColor(patternImage: NSImage(size: NSSize(width: 1, height: 1)))
// Document expected behavior: fallback to self and getRed may return zeros
let hex = pattern.hexString()
// Either assert expected output or document limitation
`#expect`(hex == "`#000000`") // or whatever the actual behavior is
}🤖 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/CmuxFoundation/Tests/CmuxFoundationTests/NSColor`+HexStringTests.swift
around lines 1 - 29, Add a unit test that exercises the fallback path of
NSColor.hexString when usingColorSpace(.sRGB) returns nil (e.g., pattern,
deviceGray, or other non-sRGB colors). Create a test (e.g.,
patternColorFallbackBehavior) that constructs a non-sRGB NSColor (like
NSColor(patternImage:) or a deviceGray color), calls hexString() and asserts the
documented/expected result for that case; reference
NSColor.hexString(includeAlpha:) and the usingColorSpace(.sRGB) conversion path
in the test so the behavior is explicit (either assert the actual current output
like "`#000000`" or assert and document the chosen fallback behavior).
…xAppKitSupportUI (stack E) Faithfully lift SidebarScrollViewResolver (NSViewRepresentable, was in ContentView.swift), SidebarScrollViewResolverView, and SidebarScrollViewConfigurator into CmuxAppKitSupportUI/Scroll/. These are AppKit-only sidebar scroll-resolution primitives with no domain reach-through, completing the package's scroll cluster per ContentView.plan.md. Bodies are byte-identical; the only deltas are public access modifiers and public import (cross-module), an explicit public init on the resolver (memberwise inits aren't public across modules), and nonisolated(unsafe) on the observer token to satisfy Swift 6 strict-concurrency in the package (accessed only on the main thread; removed in the nonisolated deinit after main-thread access has ceased; justified inline). Removed the two app-side files from pbxproj (8 entries), normalized, ratcheted the file-length budget for ContentView (18677 -> 18662). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…xAppKitSupportUI (stack E) The app-host XCTest SidebarScrollViewConfiguratorTests (regression coverage for the #3241 stuck-scrollbar bug) referenced SidebarScrollViewConfigurator and SidebarScrollViewResolverView, which moved into CmuxAppKitSupportUI in the prior commit. test-depot (the only gate that compiles cmuxTests) went red because the symbols no longer live in the app module. Add 'import CmuxAppKitSupportUI' hoisted above the canImport(cmux_DEV) block (matching the SentryEventScrubberTests/CmuxFoundation precedent and avoiding the dead-conditional-import trap). All test logic and assertions are byte-identical; this is a pure faithful retarget of the call sites, no coverage weakened. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tack E, Wave 2) Lifts the feedback-composer domain vertical out of ContentView into a new CmuxFeedback package: FeedbackComposerSettings, FeedbackComposerAttachment, PreparedFeedbackComposerAttachment, FeedbackComposerAppMetadata, FeedbackComposerSubmissionError, FeedbackComposerClient, FeedbackComposerBridgeError, FeedbackComposerBridge. Bodies are byte-identical; the only deltas are the expected cross-module ones (public/public import, explicit public visibility, @mainactor on openComposer which reads NSApp, and any Error for ExistentialAny). The .feedbackComposerRequested Notification.Name is redeclared in the package with the identical raw value 'cmux.feedbackComposerRequested' so posts from the package interoperate with the app's existing observers (the app-side constant in TabManager.swift is unchanged). Defaults key 'sidebarHelpFeedbackEmail' and the endpoint env override are preserved byte-identical. Consumers re-import CmuxFeedback: ContentView (composer sheet UI half, still app-side), CmuxHelpCommands, TerminalController, and TerminalController+ControlSystemContext. AppDelegate only posts the notification via its own constant and needs no import. The feedback UI half (SidebarFeedbackComposerSheet + message-editor NSViews) stays in ContentView as a later CmuxFeedbackUI Wave-3 lift. Package: swift build + 5 behavior tests green. App: full bounded xcodebuild BUILD SUCCEEDED. pbxproj wired with 6 mirrored entries, normalized + checked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…muxFeedbackUI (stack E, Wave 3) Lifts the four self-contained feedback message-editor view primitives out of ContentView into a new CmuxFeedbackUI package: FeedbackComposerMessageEditor (NSViewRepresentable), FeedbackComposerMessageEditorView (self-sizing NSView), FeedbackComposerMessageScrollView, and FeedbackComposerPassthroughLabel (package-internal). View bodies are byte-identical; the only deltas are the expected cross-module ones: an explicit public init on the representable (the implicit memberwise init is no longer visible across the boundary) and per-file AppKit/SwiftUI imports. These views have no ContentView coupling (they take @binding text + plain strings). SidebarFeedbackComposerSheet stays app-side and now constructs FeedbackComposerMessageEditor through the package; its call site is unchanged because the explicit init's first label (text:) matches the former memberwise init. The FeedbackComposerMessageEditorView layout-behavior test moves faithfully from cmuxTests into CmuxFeedbackUITests (XCTest, assertions byte-identical) and is removed from the app host test file, so the package owns its own coverage. Package: swift build + 2 behavior tests green. App: full bounded xcodebuild BUILD SUCCEEDED. pbxproj wired with 6 mirrored entries, normalized + checked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ack E) The prior commit added 'import CmuxAppKitSupportUI' to the retargeted SidebarScrollViewConfiguratorTests, but the cmuxTests target was not linked against the package, so test-depot (which compiles cmuxTests) failed to resolve the module. App-host XCTest files must link every package they import directly; only test-depot exercises this — swift build and 'xcodebuild build' compile the app, not the test target. Wire the 4 standard pbxproj entries for the test target, mirroring the existing CmuxFoundation linkage that SentryEventScrubberTests already relies on: PBXBuildFile, the cmuxTests Frameworks-phase listing, the packageProductDependencies listing, and an XCSwiftPackageProductDependency def pointing at the existing CmuxAppKitSupportUI local package reference. Normalized; check-pbxproj and lint-pbxproj-test-wiring both exit 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tte (stack E CI fix)
A prior leg lifted CommandPaletteContextSnapshot, CommandPaletteContextKeys, and
the CommandPaletteSwitcherFingerprint{Context,Workspace,Surface} value types from
ContentView into the CmuxCommandPalette package, but three app-host test files
still referenced them as nested ContentView.CommandPalette* members. The local
app build never compiles cmuxTests, so this only surfaced on the cmux-unit gate
('type ContentView has no member CommandPaletteContextSnapshot').
De-qualify the lifted type references to their package names and add the hoisted
import CmuxCommandPalette where missing. The app-side methods the tests exercise
(commandPaletteRightSidebarModeCommandContributions, commandPaletteShortcutAction,
etc.) stay on ContentView and are unchanged. All assertions are untouched.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ew() nonisolated (stack E CI fix) test-depot failed compiling CmuxAppKitSupportUI: 'call to main actor-isolated instance method resolveScrollView() in a synchronous nonisolated context' at the NotificationCenter observer closure. NSView is @mainactor under the package's Swift 6 strict concurrency, so the class (and the method) inherited main-actor isolation; the addObserver(queue:.main) closure is @sendable and cannot call a @mainactor method synchronously. The app target compiled it before only because it isn't in strict-concurrency mode (the Stack E app-test-compile gap). resolveScrollView()'s body only schedules a @mainactor Task and performs no isolated work itself, so marking it nonisolated is the faithful fix: it stays callable from the observer closure and from the @mainactor lifecycle overrides, and the actual resolution still runs on the main actor inside the Task. No MainActor.assumeIsolated (banned). Package swift build + test green; app xcodebuild BUILD SUCCEEDED. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(stack E CI fix) The browser-stack drop-planner lift retargeted SidebarWorkspaceDropPlannerTests to `import CmuxSidebarProviderKit`, but the cmuxTests target did not link that package, so the test target failed to compile on CI (the local app-scheme build does not compile the test target, so it was not caught locally). Mirrors the existing CmuxFoundation test-target wiring: adds a test-scoped XCSwiftPackageProductDependency for CmuxSidebarProviderKit, its Frameworks build-file, the Frameworks-phase entry, and the packageProductDependencies entry. pbxproj normalized; new IDs collision-checked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv # cmux.xcodeproj/project.pbxproj
…umers (stack E CI fix) Two test files referenced types that earlier tranches lifted into CmuxFoundation but were not in the retarget set, so the cmuxTests target failed to compile on test-depot (the local app-scheme build does not compile the test target): - SidebarOrderingTests.swift uses SidebarDropPlanner / SidebarDropIndicator (lifted in the drop-planner tranche) but only imported CmuxAppKitSupportUI. - SessionPersistenceTests.swift uses SidebarDragFailsafePolicy (lifted in the drag-lifecycle-policies tranche) with no CmuxFoundation import. Adds the CmuxFoundation import to both. A full sweep of every lifted public type against all cmuxTests and Sources consumers now reports zero missing owning-package imports. CmuxFoundation is already linked into the cmuxTests target, so no further pbxproj wiring is needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move the 382-line feedback composer modal out of ContentView into CmuxFeedbackUI as a public View. Body ported verbatim; the only changes are the expected module-boundary ones: public struct + public init(), `error: any Error` (package enables ExistentialAny), and `bundle: .module` on every String(localized:) so the 27 sidebar.help.feedback.* keys resolve from the package catalog. Localization preserved faithfully: the 27 keys (en/ja/ko/uk) are copied into a new package Localizable.xcstrings; Package.swift gains defaultLocalization "en" + resources processing + a CmuxFeedback dep. FeedbackComposerAttachment gains Sendable (immutable value type with all-Sendable stored properties) so the attachments array can cross into the nonisolated FeedbackComposerClient.submit boundary that the package's strict isolation surfaces. ContentView 17544 -> 17162. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…to CmuxFoundation Faithful lift of seven independent pure value/policy leaf types out of the ContentView preamble/sidebar cluster into CmuxFoundation (the established home for ContentView's sidebar pure-policy leaves): - ShortcutHintModifierPolicy, ShortcutHintDebugSettings, ShortcutHintModifierActivation, SidebarWorkspaceShortcutHintMetrics, SidebarTrailingAccessoryWidthPolicy (ShortcutHint/ subfolder) - DevBuildBannerDebugSettings - SidebarMarkdownRenderer Bodies are byte-identical to the app-target originals (machine-diff verified); only additions are public/DocC and two strict-Swift-6 faithful fixes: SidebarWorkspaceShortcutHintMetrics' static NSFont constant and lock-guarded memo get nonisolated(unsafe) with inline justification (NSFont non-Sendable in package mode; immutable, read only under the lock). App-target consumers (ContentView, ShortcutHintPill, RightSidebarPanelView, UpdateTitlebarAccessory, BrowserPanelView, cmuxApp) and the three cmuxTests files retarget by adding `import CmuxFoundation`; no assertion changes. ContentView.swift 17,162 -> 16,957 (-205). Budget regenerated (test +1 import line). Package swift build + swift test green; app xcodebuild Debug green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…undation Faithful lift of the pure sidebar multi-selection reconciliation / shift-click anchor policy out of ContentView into CmuxFoundation/Sidebar. All six members are static functions over Set<UUID>/[UUID]/Int with no AppKit and no cross-slice god types, so it is a clean foundation leaf. Body is byte-identical to the app-target original (machine-diff verified). The app-target enum carried a redundant @mainactor; dropped on the lift since every member is a pure static over Sendable values (isolation is inert here, callable from any context including the existing @mainactor call sites and the @mainactor Swift Testing suite). The five ContentView call sites resolve through the existing import CmuxFoundation; SidebarWorkspaceSelectionAnchorPolicyTests retargets by adding the import (assertions unchanged). ContentView.swift 16,957 -> 16,867 (-90). Budget regenerated (prior-tranche import lines on three tracked files). Package swift build + swift test green; app xcodebuild Debug green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation Faithful lift of the sidebar tab-drag lifecycle notification vocabulary (Notification.Name constants + post/parse helpers over UUID/String) out of ContentView into CmuxFoundation/Sidebar. No cross-slice god types, no mutable state; the only consumer is ContentView (already imports CmuxFoundation), no test references. Body byte-identical to the app-target original (machine-diff verified). The two Notification.Name raw values are the in-process wire contract and are preserved exactly; the NotificationCenter posting is kept as-is (the plan's NotificationCenter->AsyncStream modernization is a separate later phase, not part of a faithful lift). ContentView.swift 16,867 -> 16,833 (-34). Budget respected (no tracked-file growth). Package swift build green; app xcodebuild Debug green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv
…erges) Resolve conflicts taking main's version for shared/sibling content now in main (GhosttyConfig typealias from stack D; CmuxCommandPalette/UI package wiring; NSColor.hexString already in CmuxFoundation/Color), keeping only 6029's ContentView-drain slice (CmuxCommandPalette UI + CmuxAppKitSupportUI/ CmuxFeedback/CmuxFeedbackUI/CmuxFoundation/CmuxSidebarProviderKit lifts). Drop the duplicate NSColor+HexString.swift (main's NSColor+Hex.swift already defines hexString). Drop stale CmuxApplicationSupportDirectories app-target wiring (lifted to CmuxFoundation in main). pbxproj resolved by union with de-duped CmuxCommandPalette refs (use main's UUIDs, add cmuxTests linkage); budget regenerated; ghostty pinned to origin/main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The package-conventions namespace-type lint now runs against this PR's package diff (it did not on the pre-resync runs). The 25 flagged types are pure stateless policy/value namespaces lifted verbatim out of ContentView with no natural receiver, matching the sanctioned pattern already grandfathered in main (e.g. CmuxApplicationSupportDirectories). Add the convention's lint:allow namespace-type marker + justification above each; refresh the file-length budget for the +1 comment line. Comments only, build unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#6067 (iOS terminal scrollback) merged after the first re-sync. Only the regenerable file-length budget conflicted; TerminalController.swift and pbxproj auto-merged (new TerminalController+MobileScrollPrefetch.swift is main's, wired by main). Budget regenerated; ghostty pinned to origin/main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv # Sources/ContentView.swift # cmux.xcodeproj/project.pbxproj
# Conflicts: # .github/swift-file-length-budget.tsv
# Conflicts: # .github/swift-file-length-budget.tsv
austinywang
added a commit
that referenced
this pull request
Jun 14, 2026
…lve) Resolve .github/swift-file-length-budget.tsv conflict from main's ContentView command-palette drain (#6029, 19204->16826) plus line-count drift on TerminalController, BrowserPanel, GhosttyTerminalView, BrowserPanelView, TerminalAndGhosttyTests, and BrowserConfigTests. Budget entries set to actual merged on-disk line counts; verified with scripts/swift_file_length_budget.py ("budget respected"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ContentView is the largest god object in cmux (19,265 lines on main). This PR is stack E of the modular refactor: draining ContentView.swift one domain tranche at a time into domain + UI SPM packages, per
docs/cmux-refactor-audit/blueprint/gods/ContentView.plan.md. Base ismain; tranches are committed and pushed separately and this body is updated per tranche.Tranche 1 — Command palette domain (Wave 2)
Lifts the pure command-palette search / orchestration / value layer out of
ContentView.swiftandSources/CommandPalette/into a newCmuxCommandPalettedomain package (no AppKit, fully testable). This is the leaf-first, domain-pure slice; the palette UI (overlay, focus controller, responder resolver) stays app-side for a later UI-pair tranche per the plan.What moved:
Packages/CmuxCommandPaletteshaped exactly like theCmuxSettingsexemplar:// swift-tools-version: 6.0,platforms: [.macOS(.v14)], one.library, v6 language mode +ExistentialAny/InternalImportsByDefault, a matching test target.CommandPalette*value/model types removed fromContentView.swift(CommandPaletteMode,CommandPalettePendingActivation,CommandPaletteResolvedActivation,CommandPalettePendingActivationResolutionResult,CommandPaletteRenameTarget,CommandPaletteWorkspaceDescriptionTarget, etc.) and re-homed one-type-per-file underValues/,Context/,Handling/,Orchestration/,Policy/,Search/,Search/Nucleo/.CommandPaletteSearch.swift,CommandPaletteNucleoSearch.swift,CommandPaletteSearchOrchestrator.swiftmoved verbatim fromSources/CommandPalette/into the package; bodies machine-diffed byte-identical against the pre-tranche tree (onlypublic+ DocC added and grab-bag types split into their own files).#filePathpath walk was adjusted for the deeper package location (7deletingLastPathComponent()levels); nucleo FFI tests pass against it.Dependency hygiene:
CmuxCommandPalette(hoisted above the#if canImport(cmux_DEV)block).CommandPaletteContextKeys.terminalOpenTargetAvailable(_:)keeps the terminal-domain dependency out of the package (dependency inversion; package stays terminal-free).mergedSwiftFallbackMatchesForTests,commandPreviewMatchCommandIDsForTests); the real methods are nowinternaland exercised directly by package tests via@testable(no test-only methods in source).Tests (faithful retarget, no assertion weakened):
swift test), including the nucleo FFI suite and the search benchmarks (cmd+shift+p 1943ms→339ms, cmd+p 2988ms→789ms).ContentView/ForkableAgentapp glue) keep running and nowimport CmuxCommandPalettefor the moved public symbols.testAppBundleContainsNucleoSearchLibrarystays app-side (it inspects the built app bundle and cannot move to the package).ContentView.swift: 19,265 → 18,942 lines (-323).
Gates
swift_file_length_budget.pyexit 0 (budget refreshed for the new package files, which are faithful lifts of pre-existing large files).lint-ios-package-conventions.shexit 0.swift build+swift testgreen forCmuxCommandPalette.check-pbxproj.sh/lint-pbxproj-test-wiring.sh/ normalize: clean.xcodebuild ... -derivedDataPath /tmp/cmux-cview build.Salvage note
This branch began from a predecessor session that drafted (but never committed) the Wave 2 extraction against an older base. That work was verified faithful (machine-diffed, tests pass), committed, rebased onto current
main, and completed (the two broken test-shim call sites were the only gap, now closed). The round-1 drafts against the 3c-era base are archived onfeat-command-palettefor reference.Cross-stack: none. TabManager forwarders preserve the API surface; this tranche has no dependency on another stack's branch.
Update: tranche 1 finalized + completeness sweep
Three commits on this branch:
CmuxCommandPalette(the salvaged Wave 2 extraction).CommandPaletteOverlayPromotionPolicyhad been lifted into the package but its original was never deleted fromContentView.swift, so the lifted version was dead and a name-collision was latent. Deleted the app-side copy (call site now binds to the package's public type) and the redundant app-target test. Swept all 40CmuxCommandPalettepublic types against appSources/; this was the only such leftover.ContentView.swift: 19,265 → 18,936 lines. App compiles (
BUILD SUCCEEDED), 51 package tests pass, budget + lint + pbxproj checks all exit 0.Adversarial verdict (machine-diff vs pre-tranche tree)
Of every
func/type declared in the three moved source files (CommandPaletteSearch,CommandPaletteNucleoSearch,CommandPaletteSearchOrchestrator), exactly two are absent from the package — and they are precisely the two*ForTestsproduction shims intentionally removed (now exercised by package-internal tests). Zero behavior dropped.Next ContentView tranche (not in this PR — cross-stack coordination needed)
The next clean Wave 2 lift is
WorkspaceMountPolicy+MountedWorkspacePresentation(+Policy)(≈80 pure-logic lines inContentView.swift, used only within ContentView). Per the plan these belong in aCmuxWorkspacedomain package. That package name is owned by the Workspace god-file drain (a different stack); creating it from the ContentView stack would collide. Deferred to avoid a cross-stack package conflict; should be coordinated with whoever landsCmuxWorkspace.Tranche 2 — AppKit-support UI primitives (Wave 3)
Lifts the shared AppKit-bridge UI primitives out of
ContentView.swiftinto a newCmuxAppKitSupportUIpackage (the plan'sCmuxAppKitSupportUI"small reusable bridge layer"). These are self-containedNSViewRepresentable/ViewModifierwrappers with no cmux app-type coupling (Foundation/AppKit/SwiftUI only), so they are the lowest-risk drainable surface remaining after the palette tranche.What moved (one major type per file, foldered by role):
ArrowlessPopoverAnchor(+ itsCoordinator) ->Popover/ArrowlessPopoverAnchor.swift.MiddleClickCapture->Mouse/MiddleClickCapture.swift;MiddleClickCaptureView->Mouse/MiddleClickCaptureView.swift.ClearScrollBackground->Scroll/ClearScrollBackground.swift;ScrollBackgroundClearer->Scroll/ScrollBackgroundClearer.swift.Faithful lift: method bodies and the popover positioning geometry are byte-identical to pre-tranche HEAD (machine-diffed). The only deltas are package-seam mechanics —
publicvisibility, synthesizedpublic inits (the cross-module memberwise init is otherwise inaccessible),public import SwiftUIon public-API files, and@MainActoron the popoverCoordinator(ContentView was implicitly main-actor).ScrollBackgroundClearerstaysinternal(onlyClearScrollBackgrounduses it). The bannedDispatchQueue.main.async/MainActor.assumeIsolatedpatterns are carried verbatim per faithful-lift-first; modernization is a separate later phase.Call sites:
ContentView.swiftandSessionIndexView.swiftgainimport CmuxAppKitSupportUI; the trailing-closure call sites (ArrowlessPopoverAnchor(...) { ... },MiddleClickCapture { ... },.modifier(ClearScrollBackground())) compile unchanged against the public inits. Shadow-copy sweep: zero app-side declarations of any of the five lifted types remain.ContentView.swift: 18,936 -> 18,699 lines (-237).
Gates
swift_file_length_budget.pyexit 0 (--write-budgetratchet on the cutover).lint-ios-package-conventions.shexit 0 (no free functions, no namespace-enums, no unjustified statics in the new package).swift build+swift testgreen forCmuxAppKitSupportUI(1 behavior test: non-middleotherMouseDowndoes not fire the middle-click handler).check-pbxproj.sh/ normalize: clean; 6-entry wiring mirrorsCmuxCommandPalette, distinct ID block, no collisions.xcodebuild ... -derivedDataPath /tmp/cmux-cview2 build: BUILD SUCCEEDED, 0 errors.Adversarial verdict (machine-diff vs pre-tranche HEAD)
Normalized diff of each moved type's body against
HEAD:Sources/ContentView.swiftshows only the expected seam tokens (public, synthesized init,public import,@MainActor); every statement ofmakeNSView/updateNSView/present/dismiss/makePopover/positioningRect/hitTest/otherMouseDown/body/findScrollViewand the scroll-clearing block is byte-identical. Zero behavior changed.Tranche 3 — NSColor.hexString foundation lift (Wave 1)
Lifts the
NSColor.hexString(includeAlpha:)helper out ofContentView.swift's tail intoPackages/CmuxFoundationasNSColor+HexString.swift— the plan's Wave-1 foundation lift, sitting next to the existingString+JavaScriptStringLiteralextension precedent in the same leaf package.Faithful lift: the body is byte-identical to pre-tranche HEAD (machine-diffed); the only deltas are
publicvisibility, a DocC comment, and a file-scopedpublic import AppKit(CmuxFoundation was AppKit-free; this one file adds the dependency, scoped to the NSColor extension).Adopted at every call site in the same change (no half-migration): all 11 app-target files that call
.hexStringgainimport CmuxFoundation—GhosttyTerminalView,Workspace,cmuxApp,GhosttyConfig,GhosttyTerminalAppearance,WorkspaceAppearanceResolution,WorkspaceContentView,WindowAppearanceSnapshot,WindowBackdropController,FeedButtonStyleDebugWindowController, andContentView. The solefunc hexStringdeclaration now lives in the package; shadow-copy sweep confirms zero app-side copies.ContentView.swift: 18,699 -> 18,682 lines (-17).
Gates
swift build+swift testgreen forCmuxFoundation(4 new behavior tests:#FF0000, mid-gray byte-rounding#7F7F7F, alpha#0000FF7F, alpha-omitted-by-default).lint-ios-package-conventions.shexit 0;swift_file_length_budget.pyexit 0 (--write-budgetratchet — the +1 import lines on already-tracked Workspace/cmuxApp/WorkspaceContentView are absorbed).xcodebuild ... -derivedDataPath /tmp/cmux-cview2 build: BUILD SUCCEEDED, 0 errors.Adversarial verdict (machine-diff vs pre-tranche HEAD)
The moved
hexStringbody diffs againstHEAD:Sources/ContentView.swiftwith a single delta token (public); every statement (sRGB conversion, component extraction, byte clamping,#%02Xformatting, alpha branch) is identical. Zero behavior changed.Tranche 4 — String.nilIfEmpty foundation lift (Wave 1)
Lifts the
String.nilIfEmptyhelper out of ContentView'sprivate extension StringintoPackages/CmuxFoundationasString+NilIfEmpty.swift, completing the small string-helper foundation drain. Body byte-identical to pre-tranche HEAD (onlypublic+ DocC added). The helper was ContentView-private with no external callers, and ContentView already importsCmuxFoundation, so its 4 call sites resolve unchanged. SolenilIfEmptydeclaration now lives in the package.ContentView.swift: 18,682 -> 18,677 lines (-5).
Gates
CmuxFoundationswift test: 3 new behavior tests (empty→nil, non-empty pass-through, whitespace-is-non-empty).lint-ios-package-conventions.shexit 0;swift_file_length_budget.pyexit 0 (ratcheted down).xcodebuild ... build: BUILD SUCCEEDED, 0 errors.Adversarial verdict
nilIfEmptybody diffs againstHEAD:Sources/ContentView.swiftwith a singlepublictoken; theisEmpty ? nil : selfbody is identical.Cumulative status (tranches 1–4)
ContentView.swift: 19,265 (main) → 18,677. Every tranche is a faithful, machine-diffed lift with package tests and a green app compile; no behavior changed. Remaining ContentView domains (sidebar cluster, feedback composer, window-overlay/chrome installers, the workspace-mount/selected-directory policies) are larger and/or cross-stack with the Workspace god-file drain (
CmuxWorkspaceCore), and are deferred to dedicated tranches/coordination per the plan.CI follow-up: the first
test-depotrun failed at thecmuxTestsapp-host compile because movingNSColor.hexStringintoCmuxFoundationleft 7 unit-test files referencing it without the package import (the documented Stack E trap: packageswift buildand a no-test appbuilddon't compilecmuxTests). Fixed by addingimport CmuxFoundation(hoisted above the#if canImport(cmux_DEV)block) to those 7 files; the test target already linksCmuxFoundation.nilIfEmptyand theCmuxAppKitSupportUItypes have zerocmuxTestsreferences, so no further test imports were needed.test-depot.yml(unit) re-dispatched on the fixed head.Summary by CodeRabbit
New Features
Tests
Chores