Split AppDelegate support code into focused files - #3104
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracted many AppDelegate-scoped helpers into five new Swift source files, added those files to the Xcode project build, added a new test, and relaxed Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Caller (UI)
participant Controller as VSCodeServeWebController
participant Process as serve-web Process
participant Collector as ServeWebOutputCollector
participant FS as FileSystem (tmp)
UI->>Controller: ensureServeWebURL(vscodeApplicationURL, completion)
Controller->>Process: spawn serve-web (executableURL, env, args)
Process-->>Collector: stdout/stderr lines (stream)
Collector->>Controller: signal when Web UI URL parsed
Controller->>FS: write connection token file (tmp)
Controller-->>UI: completion(.success(webUIURL))
Note over Controller,Process: on stop/restart -> terminate Process and remove token file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR mechanically splits code from Confidence Score: 5/5Safe to merge — pure mechanical extraction with no logic changes and correct Xcode project registration. All five new files are verbatim moves of existing code. The Xcode project is updated correctly in all three sections (PBXBuildFile, PBXFileReference, Sources build phase). No new logic, no new coupling, and no behaviour changes are introduced. All remaining findings are P2 or lower. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
AD[Sources/AppDelegate.swift]
AD -->|extracted| CLI[App/CmuxCLIPathInstaller.swift\nCLI symlink install/uninstall]
AD -->|extracted| MB[App/MenuBarExtraController.swift\nMenu bar controller + formatters]
AD -->|extracted| SI[App/ScreenIdentity.swift\nNSScreen.cmuxDisplayID]
AD -->|extracted| SR[App/ShortcutRoutingSupport.swift\nKeyboard + browser routing helpers]
AD -->|extracted| TD[App/TerminalDirectoryOpenSupport.swift\nOpen-in-app targets + VSCode serve-web]
MB -->|still calls| AD
SR -->|still calls| AD
Reviews (1): Last reviewed commit: "Split AppDelegate support code into focu..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/App/TerminalDirectoryOpenSupport.swift (1)
84-90: Centralize the deprecated API call in a wrapper to avoid re-spreading the deprecation warning.
NSWorkspace.shared.fullPath(forApplication:)is deprecated on macOS. Rather than call it directly in theDetectionEnvironment.liveclosure, wrap it in a private helper (similar to themakeAppcastItempattern used inSources/Update/UpdateTestSupport.swift) so the deprecation warning is localized to a single definition site and accepted there, rather than propagating to each call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/TerminalDirectoryOpenSupport.swift` around lines 84 - 90, The use of the deprecated NSWorkspace.shared.fullPath(forApplication:) should be localized: create a private helper function (e.g., private func applicationPathForNameDeprecated(_ name: String) -> String?) that calls NSWorkspace.shared.fullPath(forApplication:) and annotate/guard the deprecation in that single function, then replace the inline closure in DetectionEnvironment.live's applicationPathForName with a reference to that helper; this centralizes the deprecation warning to one definition site (similar to makeAppcastItem) and prevents the warning from being emitted at every call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/App/CmuxCLIPathInstaller.swift`:
- Around line 253-284: The runPrivilegedShellCommand function can deadlock
because process.waitUntilExit() is called before draining the stdout/stderr
pipes; update runPrivilegedShellCommand to install readabilityHandler closures
on stdout.fileHandleForReading and stderr.fileHandleForReading that append
incoming Data into local Data buffers (or use readInBackgroundAndNotify) before
calling try process.run() and process.waitUntilExit(), then remove/clear the
handlers and convert the accumulated Data to Strings to produce
stderrText/stdoutText and the InstallerError message; keep the existing logic
for terminationStatus and throwing InstallerError.
In `@Sources/App/ShortcutRoutingSupport.swift`:
- Around line 536-548: Remove the unsafe dereference of NSTextView.delegate in
the non-field-editor branch: keep the existing field-editor path that uses
cmuxFieldEditorOwnerView(_ ) -> cmuxOwningGhosttyView(for:), but delete the `if
!textView.isFieldEditor` branch that casts/reads textView.delegate; instead
allow the code to fall through to the existing nextResponder/superview traversal
(and any hostedView.responderMatchesPreferredKeyboardFocus(...) logic) to
resolve non-field-editor ownership safely. Ensure no other code in
ShortcutRoutingSupport.swift reads NSTextView.delegate on this routing path.
---
Nitpick comments:
In `@Sources/App/TerminalDirectoryOpenSupport.swift`:
- Around line 84-90: The use of the deprecated
NSWorkspace.shared.fullPath(forApplication:) should be localized: create a
private helper function (e.g., private func applicationPathForNameDeprecated(_
name: String) -> String?) that calls
NSWorkspace.shared.fullPath(forApplication:) and annotate/guard the deprecation
in that single function, then replace the inline closure in
DetectionEnvironment.live's applicationPathForName with a reference to that
helper; this centralizes the deprecation warning to one definition site (similar
to makeAppcastItem) and prevents the warning from being emitted at every call
site.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 625763cd-5d76-4f00-bf8f-d0bed78bbbc4
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojSources/App/CmuxCLIPathInstaller.swiftSources/App/MenuBarExtraController.swiftSources/App/ScreenIdentity.swiftSources/App/ShortcutRoutingSupport.swiftSources/App/TerminalDirectoryOpenSupport.swiftSources/AppDelegate.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/App/ShortcutRoutingSupport.swift">
<violation number="1" location="Sources/App/ShortcutRoutingSupport.swift:132">
P2: Ignore Caps Lock in command-palette navigation flag normalization; otherwise arrow and Ctrl+N/Ctrl+P navigation can fail when Caps Lock is enabled.</violation>
</file>
<file name="Sources/App/TerminalDirectoryOpenSupport.swift">
<violation number="1" location="Sources/App/TerminalDirectoryOpenSupport.swift:572">
P1: Clear `readabilityHandler` on EOF to prevent an infinite CPU spin.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/App/CmuxCLIPathInstaller.swift`:
- Around line 268-277: The race occurs because startDraining installs
readabilityHandler callbacks for stdout/stderr that stay active until the defer,
while drainRemainingOutput synchronously calls readDataToEndOfFile on the same
file handles; to fix, stop the handlers before doing the final synchronous
drain: remove or nil-out stdout.fileHandleForReading.readabilityHandler and
stderr.fileHandleForReading.readabilityHandler immediately after
process.waitUntilExit (and before calling drainRemainingOutput), then perform
drainRemainingOutput on stdout and stderr, keeping the existing defer only as a
safety no-op; update the code around startDraining, process.waitUntilExit, and
drainRemainingOutput to reflect this ordering so handlers cannot overlap with
readDataToEndOfFile.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50b43f9d-f946-4bdc-bdab-1c087763f9d1
📒 Files selected for processing (1)
Sources/App/CmuxCLIPathInstaller.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/App/CmuxCLIPathInstaller.swift (1)
268-277:⚠️ Potential issue | 🟡 MinorResidual race between
readabilityHandlerand finalreadDataToEndOfFile.The prior draining-race concern was marked as addressed, but the current ordering still leaves the
readabilityHandlercallbacks installed whiledrainRemainingOutputsynchronously callsreadDataToEndOfFileon the same file handles (thedeferthat nils them runs only at scope exit). The handlers self-clear when they observe an emptyavailableData, but on process exit there's still a window where a background readability callback and the synchronous end-of-file read can both touch the pipe fd, potentially splitting bytes or — on some releases — triggeringNSFileHandleOperationException.Explicitly nil both
readabilityHandlers immediately afterprocess.waitUntilExit()and before thedrainRemainingOutput(...)calls (option b from the earlier suggestion), or commit fully to option a (rely on the EOF callback and drop the trailing synchronous drain).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/CmuxCLIPathInstaller.swift` around lines 268 - 277, After process.waitUntilExit() nil out the readability handlers on the stdout and stderr file handles before calling drainRemainingOutput to avoid races: explicitly set stdout.fileHandleForReading.readabilityHandler = nil and stderr.fileHandleForReading.readabilityHandler = nil immediately after process.waitUntilExit() (and keep the existing defer that clears them as a safety net), so the installed startDraining callbacks are removed prior to calling drainRemainingOutput on the same handles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/App/TerminalDirectoryOpenSupport.swift`:
- Around line 45-53: The current deduplication uses the unresolved path string
(created from directoryURL.path(percentEncoded: false) and
canonicalDirectoryPath) while isSameOrDescendant and resolvedDirectoryURL(from:)
operate on resolved (symlink-resolved) URLs, so the same physical directory can
be added twice via symlink and real paths; fix by deriving a single
canonical/resolved identifier before checking and inserting into seen: call
resolvedDirectoryURL(from:) then compute its canonicalDirectoryPath (or another
symlink-resolved path string) and use that resolvedPath for the seen set
membership test and for appending to directories instead of the unresolved path;
keep isSameOrDescendant checks as-is but ensure seen tracks the resolved
canonical path.
---
Duplicate comments:
In `@Sources/App/CmuxCLIPathInstaller.swift`:
- Around line 268-277: After process.waitUntilExit() nil out the readability
handlers on the stdout and stderr file handles before calling
drainRemainingOutput to avoid races: explicitly set
stdout.fileHandleForReading.readabilityHandler = nil and
stderr.fileHandleForReading.readabilityHandler = nil immediately after
process.waitUntilExit() (and keep the existing defer that clears them as a
safety net), so the installed startDraining callbacks are removed prior to
calling drainRemainingOutput on the same handles.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2935de7-89d5-449f-a80f-64b63a544431
📒 Files selected for processing (2)
Sources/App/CmuxCLIPathInstaller.swiftSources/App/TerminalDirectoryOpenSupport.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/App/TerminalDirectoryOpenSupport.swift (1)
87-92: Wrap the deprecated API call behind a private helper to centralize the warning.Line 91 directly calls
NSWorkspace.shared.fullPath(forApplication:), which spreads the deprecation warning across the codebase. Instead, route this through a private_legacyFullPath(forApplication:)wrapper so the warning stays localized to a single site. Per repository practice, the helper should remain markedavailable(macOS, deprecated: 11.0)to accept the warning at the point where it has no non-deprecated replacement.Suggested refactor
static let live = DetectionEnvironment( homeDirectoryPath: FileManager.default.homeDirectoryForCurrentUser.path, fileExistsAtPath: { FileManager.default.fileExists(atPath: $0) }, isExecutableFileAtPath: { FileManager.default.isExecutableFile(atPath: $0) }, - applicationPathForName: { NSWorkspace.shared.fullPath(forApplication: $0) } + applicationPathForName: { Self._legacyFullPath(forApplication: $0) } ) } + + `@available`(macOS, deprecated: 11.0) + private static func _legacyFullPath(forApplication applicationName: String) -> String? { + NSWorkspace.shared.fullPath(forApplication: applicationName) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/TerminalDirectoryOpenSupport.swift` around lines 87 - 92, The live DetectionEnvironment currently calls the deprecated NSWorkspace.shared.fullPath(forApplication:) directly; add a private helper named _legacyFullPath(forApplication:) annotated with available(macOS, deprecated: 11.0) that calls NSWorkspace.shared.fullPath(forApplication:), then change the applicationPathForName closure in DetectionEnvironment.live to call this helper instead so the deprecation warning is centralized to one location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/App/TerminalDirectoryOpenSupport.swift`:
- Around line 87-92: The live DetectionEnvironment currently calls the
deprecated NSWorkspace.shared.fullPath(forApplication:) directly; add a private
helper named _legacyFullPath(forApplication:) annotated with available(macOS,
deprecated: 11.0) that calls NSWorkspace.shared.fullPath(forApplication:), then
change the applicationPathForName closure in DetectionEnvironment.live to call
this helper instead so the deprecation warning is centralized to one location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe6e04df-3801-419c-bdc0-d7361e399c44
📒 Files selected for processing (2)
Sources/App/TerminalDirectoryOpenSupport.swiftcmuxTests/OmnibarAndToolsTests.swift
* Split AppDelegate support code into focused files * Drain privileged installer output before waiting * Clear pipe readers at EOF * Avoid overlapping privileged output reads * Deduplicate resolved Finder directories * Harden shortcut routing edge cases --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
1 participant
Summary
Split large top-level support domains out of
Sources/AppDelegate.swiftinto focused files underSources/App/:No behavior changes intended. This sets up clearer future package boundaries for app integration helpers while keeping the PR as a mechanical move.
Verification
./scripts/reload.sh --tag appsplitSummary by CodeRabbit
New Features
Refactor
Tests
Summary by cubic
Split support code out of
AppDelegateinto focused files underSources/App. Improved installer I/O to prevent hangs, deduplicated Finder-selected directories that resolve to the same path, and hardened shortcut routing edge cases.Refactors
TerminalDirectoryOpenSupport.swift,ShortcutRoutingSupport.swift,MenuBarExtraController.swift,CmuxCLIPathInstaller.swift,ScreenIdentity.swift.AppDelegate.swiftby moving support code.Bug Fixes
NSTextView.delegatelookups in focus checks (tests added).Written for commit f7aef0f. Summary will update on new commits.