Repository navigation
Make empty Dock a default terminal - #3456
lawrencecchen wants to merge 13 commits into
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:
📝 WalkthroughWalkthroughAdds a default right‑sidebar terminal when Dock has no controls, a one‑time welcome message with CLI commands to dismiss/show/check it, localization/docs updates, and broad main‑window focus capture/restore APIs with dock-surface focus wiring and associated UI/store/test changes. ChangesDock Welcome Message & Default Terminal
Main-Window Focus Capture & Restore
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Handler
participant FS as File System
participant UI as DockPanelView
participant Store as DockControlsStore
participant Runtime as DockDefaultTerminalRuntime
User->>CLI: cmux dock welcome dismiss
CLI->>FS: write marker file (dismissed)
FS-->>CLI: success
CLI-->>User: confirmation (clear terminal if interactive)
Note over UI,Store: On Dock open
UI->>Store: resolve controls
alt no controls
Store->>Runtime: create default terminal runtime
Runtime->>FS: check marker file
alt marker exists
Runtime-->>UI: no welcome script
else
Runtime-->>UI: inject/show welcome startup script
end
UI-->>User: render default terminal
else controls exist
UI-->>User: render configured controls
end
sequenceDiagram
participant Caller
participant App as AppDelegate
participant Controller as MainWindowFocusController
participant Dock as DockKeyboardFocusView
Caller->>App: captureMainWindowKeyboardFocusRestoreTarget(owning responder)
App->>Controller: captureFocusRestoreTarget(owning responder)
Controller-->>App: MainWindowFocusRestoreTarget
Caller->>App: restoreMainWindowKeyboardFocus(target)
App->>Controller: restoreFocus(target)
alt dock-surface target
Controller->>Dock: request focusSurface(surfaceId)
Dock-->>Controller: acknowledged (Bool)
else main-panel target
Controller->>Controller: restore main-panel intent/focus
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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 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 replaces the empty Dock SwiftUI placeholder with a live default right-sidebar terminal, adds a suppressible first-launch welcome message, and fixes a CPU hot-loop caused by command-palette focus-restore re-entering across multiple Ghostty focus notifications.
Confidence Score: 5/5Safe to merge — the CPU hot-loop fix removes all re-entrant notification callsites and the dock terminal ownership lift prevents unintended terminal restarts during Match Terminal Background rebuilds. No functional regressions were found. The hot-loop elimination correctly funnels all restore calls through a single non-re-entrant path. The preserveFirstResponderDuringTransientReattach flag lifecycle is tightly scoped to the prepare/replace portal cycle. All observations are style or documentation improvements. No files require special attention; the dock focus controller additions in Sources/MainWindowFocusController.swift are the most complex and are well-covered by the new DockFocusReactivationTests and ShortcutAndCommandPaletteTests additions. Important Files Changed
Reviews (8): Last reviewed commit: "Preserve Dock focus through host rebind" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| private static func makePanel(baseDirectory: String, workspaceId: UUID) -> TerminalPanel { | ||
| TerminalPanel( | ||
| workspaceId: workspaceId, |
There was a problem hiding this comment.
@MainActor during dock activation
DockDefaultTerminalRuntime is annotated @MainActor, so its init (which calls makePanel → DockWelcomeMessage.shellStartupScriptIfNeeded()) runs synchronously on the main thread. shellStartupScriptIfNeeded() does three blocking file-system operations: a fileExists stat, a write(to:atomically:), and setAttributes(_:ofItemAtPath:). These are unlikely to cause a perceptible stall for small files today, but /tmp can be on a slow sandboxed path and the work is avoidable — the script URL could be generated and the write deferred to a Task.detached that hands the path back, or the path constructed eagerly and the write done lazily inside TerminalPanel startup.
| cmux_dock_user="$(id -un 2>/dev/null || printf '%s' "${USER:-}")" | ||
| cmux_dock_ds_shell="$(dscl . -read "/Users/$cmux_dock_user" UserShell 2>/dev/null | awk '{print $2; exit}')" | ||
| if [ -n "$cmux_dock_ds_shell" ] && [ -x "$cmux_dock_ds_shell" ]; then printf '%s\\n' "$cmux_dock_ds_shell" | ||
| elif [ -n "${SHELL:-}" ] && [ -x "${SHELL:-}" ]; then printf '%s\\n' "$SHELL" | ||
| else printf '%s\\n' /bin/sh; fi | ||
| } | ||
| cmux_dock_bundle_bin="" | ||
| if [ -n "${CMUX_BUNDLED_CLI_PATH:-}" ]; then cmux_dock_bundle_bin="$(dirname "$CMUX_BUNDLED_CLI_PATH")"; fi | ||
| if [ -n "$cmux_dock_bundle_bin" ]; then case ":${PATH:-}:" in *":$cmux_dock_bundle_bin:"*) ;; *) PATH="$cmux_dock_bundle_bin${PATH:+:$PATH}"; export PATH ;; esac; fi | ||
| cmux_dock_first=1 | ||
| for cmux_dock_line in \(encodedLines); do | ||
| cmux_dock_text="$(cmux_dock_decode "$cmux_dock_line")" | ||
| if [ "$cmux_dock_first" = "1" ]; then | ||
| printf '\\033[1m%s\\033[0m\\n' "$cmux_dock_text" | ||
| cmux_dock_first=0 | ||
| else | ||
| printf '%s\\n' "$cmux_dock_text" | ||
| fi | ||
| done | ||
| printf '\\n' | ||
| rm -f -- "$0" 2>/dev/null || true | ||
| exec "$(cmux_dock_login_shell)" -l |
There was a problem hiding this comment.
shellStartupScriptIfNeeded() writes a new cmux-dock-welcome-<UUID>.sh to FileManager.default.temporaryDirectory every time a default dock terminal is created. The script cleans itself up with rm -f -- "$0" before exec'ing the login shell, but if the TerminalPanel creation fails or the process is killed before the script runs, the file is never deleted. With every Dock open/close cycle that hits the welcome path, leftover scripts will accumulate until the OS clears /tmp. Consider tracking the URL and deleting it in DockDefaultTerminalRuntime.close() as a safety net.
| return scriptURL.path | ||
| } catch { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| private static func dismissedMarkerURL() -> URL { | ||
| if let override = ProcessInfo.processInfo.environment["CMUX_DOCK_WELCOME_DISMISSED_PATH"]? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !override.isEmpty { |
There was a problem hiding this comment.
DockWelcomeMessage.dismissedMarkerURL() (app) and dockWelcomeDismissedMarkerURL() in CLI/cmux.swift implement the same CMUX_DOCK_WELCOME_DISMISSED_PATH override + ~/.cmuxterm/dock-welcome-dismissed fallback. Since they're in separate compilation targets they can't share code easily, but if the path changes in one place it must be updated in the other. Consider at least a comment referencing the mirrored implementation so the pairing is obvious during future edits.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdff81e095
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static func shellStartupScriptIfNeeded() -> String? { | ||
| guard !isDismissed() else { return nil } | ||
|
|
There was a problem hiding this comment.
The new default Dock terminal treats the welcome as "visible" until the user explicitly runs cmux dock welcome dismiss, but this path never marks the message as seen after the first render. As a result, users who never run the CLI command will get the same onboarding block every time a default Dock terminal is recreated (e.g., reopening Dock/workspace/app), which conflicts with the commit/docs expectation of a one-time initial message and creates repeated terminal noise.
Useful? React with 👍 / 👎.
bdff81e to
57497ee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/DockPanelView.swift (1)
783-810: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd DEBUG event logging for the new Dock interactions.
The new docs-button click and default-terminal focus/flash path introduce fresh UI/focus events, but they do not emit any
cmuxDebugLog(...)entries. That leaves this new Dock behavior out of the required/tmp/cmux-debug*.logtrail.As per coding guidelines, `**/*.swift`: `Implement debug events in a unified log ... Use cmuxDebugLog("message") ... Log key events in AppDelegate.swift, mouse/UI events inline in views, and focus/bonsplit events with specific tags ("focus.panel", "tab.select", etc.).`Suggested pattern
private func openDockDocs() { guard let url = URL(string: "https://cmux.com/docs/dock") else { return } +#if DEBUG + cmuxDebugLog("dock.docs.open") +#endif NSWorkspace.shared.open(url) } ... DockTerminalView( attachment: attachment, onKeyboardFocusIntent: { window in +#if DEBUG + cmuxDebugLog("focus.panel dock.default") +#endif store.noteDefaultTerminalKeyboardFocusIntent(window: window) }, onTriggerFlash: { +#if DEBUG + cmuxDebugLog("dock.default.flash") +#endif store.triggerDefaultTerminalFlash() } )Also applies to: 821-830
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/DockPanelView.swift` around lines 783 - 810, The new Dock UI actions (the docs button handled by openDockDocs(), the reload button calling store.reload(rootDirectory:workspaceId:), and the default-terminal focus/flash path referenced around the default-terminal focus code) need explicit debug entries; add cmuxDebugLog(...) calls at the start of openDockDocs() (e.g., cmuxDebugLog("dock.docs.open")), inside the reload Button action before calling store.reload(...) (e.g., cmuxDebugLog("dock.reload.trigger")), and in the default-terminal focus/flash handler (use a tag like "focus.panel" or "tab.select" per guidelines) so each user interaction/focus event writes to the unified debug log; ensure messages are concise and include relevant identifiers (rootDirectory, workspaceId, or panel/tab id) where appropriate.
🧹 Nitpick comments (2)
docs/dock.md (1)
76-81: ⚡ Quick winClarify trust-gate applicability when Dock has no controls (default terminal path).
The “Trust” section reads like “project Dock configs” always trigger a trust gate “before launching controls.” Given the new behavior for “config missing or has no controls → open default right-side terminal,” consider adding a one-liner clarifying the trust gate is only relevant when Dock controls will actually be launched (i.e., in the presence of
controls).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dock.md` around lines 76 - 81, The Trust section implies project Dock configs always trigger a trust gate before launching controls; update the text to clarify the trust gate is only shown when the Dock config includes controls (i.e., when controls will be launched), and that configs missing controls (or when config is absent) fall back to the default right-side terminal path without a project trust gate. Edit the paragraph referencing "project Dock configs", "trust gate", and "controls" to add this one-line clarification so readers know the trust gate is not applicable for the default terminal behavior.CLI/cmux.swift (1)
19130-19135: Use the same interactive-tty check convention as other CLI paths.
clearTerminalIfInteractive()currently gates on stdout only. In this repo, interactive checks consistently require both stdin and stdout to be TTYs—seeshouldUseInteractiveThemePicker()atCLI/CMUXCLI+Themes.swift:30,runOpenTUIFeedTUI()atCLI/cmux.swift:18180, andrunLegacyFeedTUI()atCLI/cmux.swift:18381.Suggested patch
private func clearTerminalIfInteractive() { - guard isatty(STDOUT_FILENO) == 1 else { return } + guard isatty(STDIN_FILENO) == 1, isatty(STDOUT_FILENO) == 1 else { return } let term = ProcessInfo.processInfo.environment["TERM"]? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" guard !term.isEmpty, term.lowercased() != "dumb" else { return } print("\u{001B}[2J\u{001B}[H", terminator: "") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 19130 - 19135, clearTerminalIfInteractive() only checks stdout TTY; change it to require both stdin and stdout be TTYs consistent with other CLI paths (e.g., shouldUseInteractiveThemePicker(), runOpenTUIFeedTUI(), runLegacyFeedTUI()). Update the guard that uses isatty(STDOUT_FILENO) to instead check isatty(STDIN_FILENO) == 1 && isatty(STDOUT_FILENO) == 1 so the terminal is only cleared when both descriptors are interactive, leaving the TERM environment checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 19064-19094: The new runDockNoSocketCommand flow is missing
DEBUG-only debug logging; add cmuxDebugLog(...) calls wrapped in `#if` DEBUG /
`#endif` around the key branches: after a successful dismissDockWelcomeMessage()
(e.g., log "dock welcome dismissed"), after a successful
showDockWelcomeMessage() (e.g., "dock welcome scheduled to show"), and when
checking dockWelcomeDismissedMarkerURL() for the "status" branch (log the
computed state). Place the logs adjacent to the print() calls in
runDockNoSocketCommand so they are emitted only in DEBUG builds and reference
the function names above to locate the insertion points.
In `@docs/dock.md`:
- Around line 66-73: Add documentation for the missing command `cmux dock
welcome status` and explicitly clarify that the `cmux dock welcome dismiss`,
`cmux dock welcome show`, and `cmux dock welcome status` commands apply only to
the default Dock welcome message that appears when no Dock config or controls
exist, not to any user-managed Dock controls; update the existing welcome text
to list `status` alongside `dismiss` and `show`, and add a short sentence
specifying the scope (default terminal only) so readers know these commands do
not modify user-created Dock controls.
---
Outside diff comments:
In `@Sources/DockPanelView.swift`:
- Around line 783-810: The new Dock UI actions (the docs button handled by
openDockDocs(), the reload button calling
store.reload(rootDirectory:workspaceId:), and the default-terminal focus/flash
path referenced around the default-terminal focus code) need explicit debug
entries; add cmuxDebugLog(...) calls at the start of openDockDocs() (e.g.,
cmuxDebugLog("dock.docs.open")), inside the reload Button action before calling
store.reload(...) (e.g., cmuxDebugLog("dock.reload.trigger")), and in the
default-terminal focus/flash handler (use a tag like "focus.panel" or
"tab.select" per guidelines) so each user interaction/focus event writes to the
unified debug log; ensure messages are concise and include relevant identifiers
(rootDirectory, workspaceId, or panel/tab id) where appropriate.
---
Nitpick comments:
In `@CLI/cmux.swift`:
- Around line 19130-19135: clearTerminalIfInteractive() only checks stdout TTY;
change it to require both stdin and stdout be TTYs consistent with other CLI
paths (e.g., shouldUseInteractiveThemePicker(), runOpenTUIFeedTUI(),
runLegacyFeedTUI()). Update the guard that uses isatty(STDOUT_FILENO) to instead
check isatty(STDIN_FILENO) == 1 && isatty(STDOUT_FILENO) == 1 so the terminal is
only cleared when both descriptors are interactive, leaving the TERM environment
checks unchanged.
In `@docs/dock.md`:
- Around line 76-81: The Trust section implies project Dock configs always
trigger a trust gate before launching controls; update the text to clarify the
trust gate is only shown when the Dock config includes controls (i.e., when
controls will be launched), and that configs missing controls (or when config is
absent) fall back to the default right-side terminal path without a project
trust gate. Edit the paragraph referencing "project Dock configs", "trust gate",
and "controls" to add this one-line clarification so readers know the trust gate
is not applicable for the default terminal 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16276557-7ad7-482f-be34-e4b57d82b2c3
📒 Files selected for processing (4)
CLI/cmux.swiftResources/Localizable.xcstringsSources/DockPanelView.swiftdocs/dock.md
| private func runDockNoSocketCommand(commandArgs: [String]) throws { | ||
| guard let subcommand = commandArgs.first?.lowercased(), | ||
| ["help", "--help", "-h"].contains(subcommand) == false else { | ||
| print(subcommandUsage("dock") ?? "Usage: cmux dock welcome <dismiss|show|status>") | ||
| return | ||
| } | ||
|
|
||
| guard subcommand == "welcome" else { | ||
| throw CLIError(message: "Unknown dock subcommand: \(subcommand)") | ||
| } | ||
|
|
||
| let action = commandArgs.dropFirst().first?.lowercased() ?? "help" | ||
| switch action { | ||
| case "dismiss", "hide", "clear", "off": | ||
| try dismissDockWelcomeMessage() | ||
| clearTerminalIfInteractive() | ||
| print("Dock welcome message dismissed.") | ||
| case "show", "reset", "on": | ||
| try showDockWelcomeMessage() | ||
| print("Dock welcome message will show in future default Dock terminals.") | ||
| case "status": | ||
| let state = FileManager.default.fileExists(atPath: dockWelcomeDismissedMarkerURL().path) | ||
| ? "dismissed" | ||
| : "visible" | ||
| print("Dock welcome message: \(state)") | ||
| case "help", "--help", "-h": | ||
| print(subcommandUsage("dock") ?? "Usage: cmux dock welcome <dismiss|show|status>") | ||
| default: | ||
| throw CLIError(message: "Unknown dock welcome action: \(action)") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add required DEBUG event logging in the new Dock command flow.
The new command path introduces key user actions (dismiss/show/status) but has no cmuxDebugLog(...) instrumentation wrapped in #if DEBUG.
Suggested patch
private func runDockNoSocketCommand(commandArgs: [String]) throws {
+ `#if` DEBUG
+ cmuxDebugLog("dock.command invoked args=\(commandArgs)")
+ `#endif`
guard let subcommand = commandArgs.first?.lowercased(),
["help", "--help", "-h"].contains(subcommand) == false else {
print(subcommandUsage("dock") ?? "Usage: cmux dock welcome <dismiss|show|status>")
return
}
@@
switch action {
case "dismiss", "hide", "clear", "off":
try dismissDockWelcomeMessage()
clearTerminalIfInteractive()
+ `#if` DEBUG
+ cmuxDebugLog("dock.welcome.dismiss")
+ `#endif`
print("Dock welcome message dismissed.")
case "show", "reset", "on":
try showDockWelcomeMessage()
+ `#if` DEBUG
+ cmuxDebugLog("dock.welcome.show")
+ `#endif`
print("Dock welcome message will show in future default Dock terminals.")
case "status":
let state = FileManager.default.fileExists(atPath: dockWelcomeDismissedMarkerURL().path)
? "dismissed"
: "visible"
+ `#if` DEBUG
+ cmuxDebugLog("dock.welcome.status=\(state)")
+ `#endif`
print("Dock welcome message: \(state)")As per coding guidelines: “**/*.swift: ... Use cmuxDebugLog("message") ... All call sites must be wrapped in #if DEBUG / #endif.”
Also applies to: 19096-19137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 19064 - 19094, The new runDockNoSocketCommand
flow is missing DEBUG-only debug logging; add cmuxDebugLog(...) calls wrapped in
`#if` DEBUG / `#endif` around the key branches: after a successful
dismissDockWelcomeMessage() (e.g., log "dock welcome dismissed"), after a
successful showDockWelcomeMessage() (e.g., "dock welcome scheduled to show"),
and when checking dockWelcomeDismissedMarkerURL() for the "status" branch (log
the computed state). Place the logs adjacent to the print() calls in
runDockNoSocketCommand so they are emitted only in DEBUG builds and reference
the function names above to locate the insertion points.
| If neither file exists, or a Dock config has no controls, Dock opens a normal right-side terminal. The first default Dock terminal prints a short welcome message with setup pointers. Hide it for future default Dock terminals with: | ||
|
|
||
| ```sh | ||
| cmux dock welcome dismiss | ||
| ``` | ||
|
|
||
| Run `cmux dock welcome show` to show the message again. cmux does not add Dock controls automatically. | ||
|
|
There was a problem hiding this comment.
Document cmux dock welcome status and clarify command scope.
You document cmux dock welcome dismiss and cmux dock welcome show, but not the cmux dock welcome status command mentioned in the PR objective/help text. Also, since the text says the welcome is printed only for the default terminal (when no config exists / no controls), it would help to explicitly state that dismiss/show/status affect only that default welcome message (not user-managed Dock controls).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/dock.md` around lines 66 - 73, Add documentation for the missing command
`cmux dock welcome status` and explicitly clarify that the `cmux dock welcome
dismiss`, `cmux dock welcome show`, and `cmux dock welcome status` commands
apply only to the default Dock welcome message that appears when no Dock config
or controls exist, not to any user-managed Dock controls; update the existing
welcome text to list `status` alongside `dismiss` and `show`, and add a short
sentence specifying the scope (default terminal only) so readers know these
commands do not modify user-created Dock controls.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
19064-19094: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMissing
#if DEBUGdebug logging (already flagged in previous review).The
runDockNoSocketCommanddispatch and its action branches (dismiss,show,status) still lack anycmuxDebugLog(...)calls wrapped in#if DEBUG/#endif, as required by the project coding guidelines.As per coding guidelines: "
**/*.swift: UsecmuxDebugLog("message")to log from code. All call sites must be wrapped in#if DEBUG/#endif."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 19064 - 19094, Add debug logging wrapped in conditional compilation to runDockNoSocketCommand: insert cmuxDebugLog(...) calls inside the dispatch and each action branch—log the resolved subcommand and action at the start of runDockNoSocketCommand, log before/after calling dismissDockWelcomeMessage() and showDockWelcomeMessage(), and log the computed state for status using dockWelcomeDismissedMarkerURL(). Wrap every cmuxDebugLog call with `#if` DEBUG / `#endif` so logs only compile in debug builds.
🧹 Nitpick comments (1)
Sources/DockPanelView.swift (1)
229-295: 💤 Low value
DockDefaultTerminalRuntimeLGTM; optional extraction of shared terminal-management logic
focus()andsetVisibleInUI(_:)are byte-for-byte copies of the same methods inDockControlRuntime. Extracting them into a shared protocol or base class would cut duplication should a third runtime type ever appear.♻️ Sketch of a shared protocol
+protocol DockTerminalRuntime: AnyObject { + var panel: TerminalPanel { get } + func focus() + func close() + func setVisibleInUI(_ visible: Bool) +} + +extension DockTerminalRuntime { + func focus() { + panel.hostedView.ensureFocus( + for: panel.surface.tabId, + surfaceId: panel.id, + respectForeignFirstResponder: false + ) + } + func setVisibleInUI(_ visible: Bool) { + if visible { + panel.hostedView.setVisibleInUI(true) + TerminalWindowPortalRegistry.updateEntryVisibility(for: panel.hostedView, visibleInUI: true) + } else { + panel.unfocus() + panel.hostedView.setVisibleInUI(false) + TerminalWindowPortalRegistry.hideHostedView(panel.hostedView) + } + } + func close() { panel.close() } +}Both
DockControlRuntimeandDockDefaultTerminalRuntimewould thenconform: DockTerminalRuntime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/DockPanelView.swift` around lines 229 - 295, Extract the duplicated focus() and setVisibleInUI(_:) logic into a shared abstraction: define a DockTerminalRuntime protocol declaring focus() and setVisibleInUI(_:), then provide the current implementation in a protocol extension (or a small base class) that uses the TerminalPanel properties (panel.hostedView, panel.surface.tabId, panel.id) so both DockDefaultTerminalRuntime and DockControlRuntime can simply conform to DockTerminalRuntime and remove their copy-pasted methods; update each class to reference the shared implementation and ensure any class-specific behavior (e.g., panel.unfocus or TerminalWindowPortalRegistry calls) is preserved or exposed via a small overridable helper if needed.
🤖 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/DockPanelView.swift`:
- Around line 297-381: The welcome script is being recreated on every workspace
switch because shellStartupScriptIfNeeded() only checks the on-disk marker; add
a process-level guard so the message shows at most once per app run. In
DockWelcomeMessage add a private static Bool (e.g. hasShownThisSession)
defaulting to false, check it at the start of shellStartupScriptIfNeeded() and
return nil if true, and when you successfully create/return the scriptURL.path
set that flag to true; keep existing isDismissed() logic intact so the disk
marker still prevents future runs across restarts.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 19064-19094: Add debug logging wrapped in conditional compilation
to runDockNoSocketCommand: insert cmuxDebugLog(...) calls inside the dispatch
and each action branch—log the resolved subcommand and action at the start of
runDockNoSocketCommand, log before/after calling dismissDockWelcomeMessage() and
showDockWelcomeMessage(), and log the computed state for status using
dockWelcomeDismissedMarkerURL(). Wrap every cmuxDebugLog call with `#if` DEBUG /
`#endif` so logs only compile in debug builds.
---
Nitpick comments:
In `@Sources/DockPanelView.swift`:
- Around line 229-295: Extract the duplicated focus() and setVisibleInUI(_:)
logic into a shared abstraction: define a DockTerminalRuntime protocol declaring
focus() and setVisibleInUI(_:), then provide the current implementation in a
protocol extension (or a small base class) that uses the TerminalPanel
properties (panel.hostedView, panel.surface.tabId, panel.id) so both
DockDefaultTerminalRuntime and DockControlRuntime can simply conform to
DockTerminalRuntime and remove their copy-pasted methods; update each class to
reference the shared implementation and ensure any class-specific behavior
(e.g., panel.unfocus or TerminalWindowPortalRegistry calls) is preserved or
exposed via a small overridable helper if needed.
🪄 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: cbf2ca16-2381-4373-b1cd-62506c6ad144
📒 Files selected for processing (6)
CLI/cmux.swiftResources/Localizable.xcstringsSources/ContentView.swiftSources/DockPanelView.swiftSources/RightSidebarPanelView.swiftdocs/dock.md
✅ Files skipped from review due to trivial changes (2)
- Resources/Localizable.xcstrings
- docs/dock.md
| private enum DockWelcomeMessage { | ||
| private static let markerFileName = "dock-welcome-dismissed" | ||
|
|
||
| static func isDismissed() -> Bool { | ||
| FileManager.default.fileExists(atPath: dismissedMarkerURL().path) | ||
| } | ||
|
|
||
| static func shellStartupScriptIfNeeded() -> String? { | ||
| guard !isDismissed() else { return nil } | ||
|
|
||
| let title = String( | ||
| localized: "dock.welcome.title", | ||
| defaultValue: "Welcome to Dock" | ||
| ) | ||
| let intro = String( | ||
| localized: "dock.welcome.body", | ||
| defaultValue: "This is a normal terminal pinned to the right side of cmux. Use it for notes, logs, long-running commands, or anything you want close by." | ||
| ) | ||
| let customize = String( | ||
| localized: "dock.welcome.customize", | ||
| defaultValue: "Customize it with .cmux/dock.json in this project, or ~/.config/cmux/dock.json globally." | ||
| ) | ||
| let docs = String( | ||
| localized: "dock.welcome.docs", | ||
| defaultValue: "Run `cmux docs dock` or visit https://cmux.com/docs/dock for the Dock config format." | ||
| ) | ||
| let dismiss = String( | ||
| localized: "dock.welcome.dismiss", | ||
| defaultValue: "Hide this message with: cmux dock welcome dismiss" | ||
| ) | ||
|
|
||
| let lines = [title, "", intro, customize, docs, "", dismiss] | ||
| let encodedLines = lines | ||
| .map { Data($0.utf8).base64EncodedString() } | ||
| .map { "'\($0)'" } | ||
| .joined(separator: " ") | ||
| let scriptURL = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("cmux-dock-welcome-\(UUID().uuidString.lowercased()).sh") | ||
| let body = """ | ||
| #!/bin/sh | ||
| cmux_dock_decode() { printf '%s' "$1" | base64 --decode 2>/dev/null || printf '%s' "$1" | base64 -D 2>/dev/null; } | ||
| cmux_dock_login_shell() { | ||
| cmux_dock_user="$(id -un 2>/dev/null || printf '%s' "${USER:-}")" | ||
| cmux_dock_ds_shell="$(dscl . -read "/Users/$cmux_dock_user" UserShell 2>/dev/null | awk '{print $2; exit}')" | ||
| if [ -n "$cmux_dock_ds_shell" ] && [ -x "$cmux_dock_ds_shell" ]; then printf '%s\\n' "$cmux_dock_ds_shell" | ||
| elif [ -n "${SHELL:-}" ] && [ -x "${SHELL:-}" ]; then printf '%s\\n' "$SHELL" | ||
| else printf '%s\\n' /bin/sh; fi | ||
| } | ||
| cmux_dock_bundle_bin="" | ||
| if [ -n "${CMUX_BUNDLED_CLI_PATH:-}" ]; then cmux_dock_bundle_bin="$(dirname "$CMUX_BUNDLED_CLI_PATH")"; fi | ||
| if [ -n "$cmux_dock_bundle_bin" ]; then case ":${PATH:-}:" in *":$cmux_dock_bundle_bin:"*) ;; *) PATH="$cmux_dock_bundle_bin${PATH:+:$PATH}"; export PATH ;; esac; fi | ||
| cmux_dock_first=1 | ||
| for cmux_dock_line in \(encodedLines); do | ||
| cmux_dock_text="$(cmux_dock_decode "$cmux_dock_line")" | ||
| if [ "$cmux_dock_first" = "1" ]; then | ||
| printf '\\033[1m%s\\033[0m\\n' "$cmux_dock_text" | ||
| cmux_dock_first=0 | ||
| else | ||
| printf '%s\\n' "$cmux_dock_text" | ||
| fi | ||
| done | ||
| printf '\\n' | ||
| rm -f -- "$0" 2>/dev/null || true | ||
| exec "$(cmux_dock_login_shell)" -l | ||
| """ | ||
| do { | ||
| try body.write(to: scriptURL, atomically: true, encoding: .utf8) | ||
| try FileManager.default.setAttributes([.posixPermissions: 0o700], ofItemAtPath: scriptURL.path) | ||
| return scriptURL.path | ||
| } catch { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| private static func dismissedMarkerURL() -> URL { | ||
| if let override = ProcessInfo.processInfo.environment["CMUX_DOCK_WELCOME_DISMISSED_PATH"]? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !override.isEmpty { | ||
| return URL(fileURLWithPath: (override as NSString).expandingTildeInPath) | ||
| } | ||
| return FileManager.default.homeDirectoryForCurrentUser | ||
| .appendingPathComponent(".cmuxterm", isDirectory: true) | ||
| .appendingPathComponent(markerFileName, isDirectory: false) | ||
| } | ||
| } |
There was a problem hiding this comment.
Welcome message appears on every workspace switch before dismissal, not truly "once"
shellStartupScriptIfNeeded() is invoked inside DockDefaultTerminalRuntime.makePanel, which is called from DockControlsStore.reload(). Because activate() falls through to reload() whenever workspaceId changes (line 418–424), switching workspaces before the marker file exists causes a new welcome-message terminal to be spawned each time. The PR description calls this a one-time message — the current implementation shows it once per workspace visit until dismissed.
A session-level guard in DockWelcomeMessage would enforce the single-show invariant:
🐛 Proposed fix
private enum DockWelcomeMessage {
private static let markerFileName = "dock-welcome-dismissed"
+ private static var hasShownThisSession = false
static func isDismissed() -> Bool {
FileManager.default.fileExists(atPath: dismissedMarkerURL().path)
}
static func shellStartupScriptIfNeeded() -> String? {
- guard !isDismissed() else { return nil }
+ guard !isDismissed(), !hasShownThisSession else { return nil }
+ hasShownThisSession = true
// ... rest of function unchanged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/DockPanelView.swift` around lines 297 - 381, The welcome script is
being recreated on every workspace switch because shellStartupScriptIfNeeded()
only checks the on-disk marker; add a process-level guard so the message shows
at most once per app run. In DockWelcomeMessage add a private static Bool (e.g.
hasShownThisSession) defaulting to false, check it at the start of
shellStartupScriptIfNeeded() and return nil if true, and when you successfully
create/return the scriptURL.path set that flag to true; keep existing
isDismissed() logic intact so the disk marker still prevents future runs across
restarts.
57497ee to
875527c
Compare
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)
Sources/ContentView.swift (1)
8921-9011:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve portal-backed targets before generic responder capture.
This helper now does generic
underlyingRespondercapture before consulting the browser/terminal portal registries, even though the rest of this file treats portal lookup as the authoritative path for pointer-based targeting. A backdrop click over a portal-backed browser or terminal can therefore capture a wrapper responder first and skip the precise surface target, which breaks focus restoration after dismiss.💡 Suggested fix
private func commandPaletteBackdropFocusTarget( atWindowPoint windowPoint: NSPoint, in window: NSWindow ) -> MainWindowFocusRestoreTarget? { + if let webView = BrowserWindowPortalRegistry.webViewAtWindowPoint(windowPoint, in: window), + let target = commandPaletteBrowserFocusTarget(for: webView, in: window) { + return target + } + + if let terminalView = TerminalWindowPortalRegistry.terminalViewAtWindowPoint(windowPoint, in: window), + let target = AppDelegate.shared?.captureMainWindowKeyboardFocusRestoreTarget( + owning: terminalView, + in: window + ) { + return target + } + let overlayController = commandPaletteWindowOverlayController(for: window) if let responder = overlayController.underlyingResponder(atWindowPoint: windowPoint), - let target = commandPaletteBackdropFocusTarget(for: responder) { + let target = commandPaletteBackdropFocusTarget(for: responder, in: window) { return target } - - if let webView = BrowserWindowPortalRegistry.webViewAtWindowPoint(windowPoint, in: window), - let target = commandPaletteBrowserFocusTarget(for: webView) { - return target - } - - if let terminalView = TerminalWindowPortalRegistry.terminalViewAtWindowPoint(windowPoint, in: window), - let target = AppDelegate.shared?.captureMainWindowKeyboardFocusRestoreTarget( - owning: terminalView, - in: window - ) { - return target - } return nil } - private func commandPaletteBackdropFocusTarget(for responder: NSResponder) -> MainWindowFocusRestoreTarget? { + private func commandPaletteBackdropFocusTarget( + for responder: NSResponder, + in window: NSWindow? + ) -> MainWindowFocusRestoreTarget? { if let target = AppDelegate.shared?.captureMainWindowKeyboardFocusRestoreTarget( owning: responder, - in: observedWindow + in: window ) { return target } if let webView = commandPaletteOwningWebView(for: responder), - let target = commandPaletteBrowserFocusTarget(for: webView) { + let target = commandPaletteBrowserFocusTarget(for: webView, in: window) { return target } return nil } - private func commandPaletteBrowserFocusTarget(for webView: WKWebView) -> MainWindowFocusRestoreTarget? { + private func commandPaletteBrowserFocusTarget( + for webView: WKWebView, + in window: NSWindow? + ) -> MainWindowFocusRestoreTarget? { if let selectedWorkspace = tabManager.selectedWorkspace, - let target = commandPaletteBrowserFocusTarget(in: selectedWorkspace, for: webView) { + let target = commandPaletteBrowserFocusTarget(in: selectedWorkspace, for: webView, in: window) { return target } let selectedWorkspaceId = tabManager.selectedTabId for workspace in tabManager.tabs where workspace.id != selectedWorkspaceId { - if let target = commandPaletteBrowserFocusTarget(in: workspace, for: webView) { + if let target = commandPaletteBrowserFocusTarget(in: workspace, for: webView, in: window) { return target } } return nil @@ private func commandPaletteBrowserFocusTarget( in workspace: Workspace, - for webView: WKWebView + for webView: WKWebView, + in window: NSWindow? ) -> MainWindowFocusRestoreTarget? { for (panelId, panel) in workspace.panels { guard let browserPanel = panel as? BrowserPanel, browserPanel.webView === webView else { continue @@ return commandPaletteRestoreFocusTarget( workspaceId: workspace.id, panelId: panelId, fallbackIntent: .browser(.webView), - in: observedWindow + in: window ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 8921 - 9011, The backdrop hit-testing currently calls overlayController.underlyingResponder(atWindowPoint:) before consulting portal registries, causing portal-backed web/terminal views to be masked by wrapper responders; swap the order in commandPaletteBackdropFocusTarget(atWindowPoint:in:) so you first check BrowserWindowPortalRegistry.webViewAtWindowPoint(...) and TerminalWindowPortalRegistry.terminalViewAtWindowPoint(...), call commandPaletteBrowserFocusTarget(for:) or captureMainWindowKeyboardFocusRestoreTarget(...) for those portal results, and only then fall back to overlayController.underlyingResponder(atWindowPoint:) and commandPaletteBackdropFocusTarget(for:) for non-portal responders so portal surfaces remain authoritative for pointer-based targeting.
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
19064-19094: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMissing
cmuxDebugLoginstrumentation in new Dock command flow.The three key user-visible code paths —
dismiss,show, andstatus— and their file-system mutations have nocmuxDebugLog(...)calls wrapped in#if DEBUG /#endif``, violating the project's debug-logging mandate.♻️ Proposed patch
private func runDockNoSocketCommand(commandArgs: [String]) throws { + `#if` DEBUG + cmuxDebugLog("dock.command args=\(commandArgs)") + `#endif` guard let subcommand = commandArgs.first?.lowercased(), ["help", "--help", "-h"].contains(subcommand) == false else { print(subcommandUsage("dock") ?? "Usage: cmux dock welcome <dismiss|show|status>") return } ... switch action { case "dismiss", "hide", "clear", "off": try dismissDockWelcomeMessage() clearTerminalIfInteractive() + `#if` DEBUG + cmuxDebugLog("dock.welcome.dismiss") + `#endif` print("Dock welcome message dismissed.") case "show", "reset", "on": try showDockWelcomeMessage() + `#if` DEBUG + cmuxDebugLog("dock.welcome.show") + `#endif` print("Dock welcome message will show in future default Dock terminals.") case "status": let state = FileManager.default.fileExists(atPath: dockWelcomeDismissedMarkerURL().path) ? "dismissed" : "visible" + `#if` DEBUG + cmuxDebugLog("dock.welcome.status=\(state)") + `#endif` print("Dock welcome message: \(state)")As per coding guidelines: "
**/*.swift: UsecmuxDebugLog("message")to log from code. All call sites must be wrapped in#if DEBUG/#endif."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 19064 - 19094, Add debug logging to the new dock command flow by inserting cmuxDebugLog(...) calls wrapped in `#if` DEBUG / `#endif` in runDockNoSocketCommand for the three main branches: when handling "dismiss" (after calling dismissDockWelcomeMessage(), log that the dismiss action ran and include the marker path from dockWelcomeDismissedMarkerURL()), when handling "show" (after showDockWelcomeMessage(), log that the show/reset/on action ran and the marker path), and when handling "status" (log the resolved state and the marker path). Place logs close to the existing prints so they reflect final state, referencing runDockNoSocketCommand, dismissDockWelcomeMessage, showDockWelcomeMessage, and dockWelcomeDismissedMarkerURL() to locate the code.
🤖 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/MainWindowFocusController.swift`:
- Around line 848-865: The restoreDockSurfaceFocus(surfaceId:) currently resets
rightSidebarFocusState to .focused(mode: .dock, target: .firstItem) which loses
the exact UUID of the dock surface; update the focus/request state path to carry
the concrete surfaceId (either add a rememberedDockSurfaceId property or extend
rightSidebarFocusState/request to include an optional UUID), set that UUID when
restoreDockSurfaceFocus(surfaceId:) succeeds (and when building
intent/right-sidebar requests), and update rightSidebarRestoreTargetFromState to
read and prefer that UUID when present so subsequent restores return to the
exact dock surface instead of always .firstItem (apply the same change for the
analogous code at lines ~966-978).
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 8921-9011: The backdrop hit-testing currently calls
overlayController.underlyingResponder(atWindowPoint:) before consulting portal
registries, causing portal-backed web/terminal views to be masked by wrapper
responders; swap the order in
commandPaletteBackdropFocusTarget(atWindowPoint:in:) so you first check
BrowserWindowPortalRegistry.webViewAtWindowPoint(...) and
TerminalWindowPortalRegistry.terminalViewAtWindowPoint(...), call
commandPaletteBrowserFocusTarget(for:) or
captureMainWindowKeyboardFocusRestoreTarget(...) for those portal results, and
only then fall back to overlayController.underlyingResponder(atWindowPoint:) and
commandPaletteBackdropFocusTarget(for:) for non-portal responders so portal
surfaces remain authoritative for pointer-based targeting.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 19064-19094: Add debug logging to the new dock command flow by
inserting cmuxDebugLog(...) calls wrapped in `#if` DEBUG / `#endif` in
runDockNoSocketCommand for the three main branches: when handling "dismiss"
(after calling dismissDockWelcomeMessage(), log that the dismiss action ran and
include the marker path from dockWelcomeDismissedMarkerURL()), when handling
"show" (after showDockWelcomeMessage(), log that the show/reset/on action ran
and the marker path), and when handling "status" (log the resolved state and the
marker path). Place logs close to the existing prints so they reflect final
state, referencing runDockNoSocketCommand, dismissDockWelcomeMessage,
showDockWelcomeMessage, and dockWelcomeDismissedMarkerURL() to locate the code.
🪄 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: 322cfa58-c4c9-4c90-99c4-5710c64dea88
📒 Files selected for processing (9)
CLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/DockPanelView.swiftSources/MainWindowFocusController.swiftSources/RightSidebarPanelView.swiftcmuxTests/ShortcutAndCommandPaletteTests.swiftdocs/dock.md
✅ Files skipped from review due to trivial changes (3)
- docs/dock.md
- Resources/Localizable.xcstrings
- Sources/RightSidebarPanelView.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 875527c1d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let panelId = workspace.focusedPanelId, | ||
| let panel = workspace.panels[panelId], | ||
| let focusIntent = panel.ownedFocusIntent(for: responder, in: window) { | ||
| return mainPanelFocusRestoreTarget( |
There was a problem hiding this comment.
This focused-panel fast path can return the wrong restore target when ownedFocusIntent is not strictly responder-scoped (notably browser address bar state), so dismissing the command palette after clicking another panel can jump focus back to the previously focused panel instead of the clicked surface. In practice, if the browser address bar was focused when the palette opened, a backdrop click on a terminal can still restore browser focus because the selected panel is checked first here. The previous logic in ContentView resolved terminal/webview targets from the clicked responder directly, so this is a behavioral regression in focus restoration.
Useful? React with 👍 / 👎.
875527c to
fa09a60
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
9014-9040:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRetry focus restoration after transient failures.
On Lines 9031-9035, a failed restore only puts
targetback into state; it never schedules another attempt. IfrestoreMainWindowKeyboardFocusreturnsfalsebecause the responder is not ready on the same runloop turn as dismissal, the restore now succeeds only if some unrelated notification happens before the 500 ms timeout.Suggested fix
guard AppDelegate.shared?.restoreMainWindowKeyboardFocus(target, in: observedWindow) == true else { if commandPaletteRestoreTimeoutWorkItem != nil, commandPalettePendingDismissFocusTarget == nil { commandPalettePendingDismissFocusTarget = target + DispatchQueue.main.asyncAfter(deadline: .now() + 0.02) { + attemptCommandPaletteFocusRestoreIfNeeded() + } } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 9014 - 9040, The current logic in attemptCommandPaletteFocusRestoreIfNeeded clears commandPalettePendingDismissFocusTarget then, on a failed AppDelegate.shared?.restoreMainWindowKeyboardFocus(...), reassigns the target but does not re-schedule another attempt; update the failure branch so that when restoreMainWindowKeyboardFocus returns false you re-arm a retry: set commandPalettePendingDismissFocusTarget = target and schedule a new DispatchQueue.main.asyncAfter (or reuse commandPaletteRestoreTimeoutWorkItem) to call attemptCommandPaletteFocusRestoreIfNeeded again after a short delay (e.g. 0.05–0.1s), ensuring you cancel/replace any existing commandPaletteRestoreTimeoutWorkItem to avoid duplicates; touch the functions/requestCommandPaletteFocusRestore, attemptCommandPaletteFocusRestoreIfNeeded, and the symbols commandPaletteRestoreTimeoutWorkItem and commandPalettePendingDismissFocusTarget when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 9014-9040: The current logic in
attemptCommandPaletteFocusRestoreIfNeeded clears
commandPalettePendingDismissFocusTarget then, on a failed
AppDelegate.shared?.restoreMainWindowKeyboardFocus(...), reassigns the target
but does not re-schedule another attempt; update the failure branch so that when
restoreMainWindowKeyboardFocus returns false you re-arm a retry: set
commandPalettePendingDismissFocusTarget = target and schedule a new
DispatchQueue.main.asyncAfter (or reuse commandPaletteRestoreTimeoutWorkItem) to
call attemptCommandPaletteFocusRestoreIfNeeded again after a short delay (e.g.
0.05–0.1s), ensuring you cancel/replace any existing
commandPaletteRestoreTimeoutWorkItem to avoid duplicates; touch the
functions/requestCommandPaletteFocusRestore,
attemptCommandPaletteFocusRestoreIfNeeded, and the symbols
commandPaletteRestoreTimeoutWorkItem and commandPalettePendingDismissFocusTarget
when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80ed7ba4-9dbe-421e-81a0-76f2c496d9b4
📒 Files selected for processing (9)
CLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/DockPanelView.swiftSources/MainWindowFocusController.swiftSources/RightSidebarPanelView.swiftcmuxTests/ShortcutAndCommandPaletteTests.swiftdocs/dock.md
✅ Files skipped from review due to trivial changes (4)
- Resources/Localizable.xcstrings
- docs/dock.md
- Sources/RightSidebarPanelView.swift
- cmuxTests/ShortcutAndCommandPaletteTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/AppDelegate.swift
- CLI/cmux.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9615c3e872
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isDockSurface { | ||
| guard surfaceView.desiredFocus || isSurfaceViewFirstResponder() else { return } | ||
| } else if !matchesCurrentTerminalFocusTarget(tabId: tabId, surfaceId: panelId) { |
There was a problem hiding this comment.
This branch bypasses matchesCurrentTerminalFocusTarget for dock surfaces, so a queued deferred focus apply can still call makeFirstResponder after the user has already moved focus back to another panel. The regression is reachable when Dock focus is requested while the view is hidden/tiny (which schedules a deferred apply) and then the user switches focus before the apply runs; because desiredFocus remains true, the dock surface can steal focus later and interrupt typing in the newly focused panel.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/DockPanelView.swift (1)
306-307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSession-level guard still missing in
shellStartupScriptIfNeeded()— welcome message reappears on every workspace switch
shellStartupScriptIfNeeded()only checks the on-disk marker (isDismissed()). Becauseactivate()callsreload()wheneverworkspaceIdchanges (a newDockDefaultTerminalRuntimeis constructed each time), the welcome message reappears on every workspace visit until the user dismisses it — contradicting the stated "one-time" behaviour.The proposed fix from the earlier review still applies: add a process-level latch alongside the disk check.
🐛 Proposed fix
private enum DockWelcomeMessage { private static let markerFileName = "dock-welcome-dismissed" + private static var hasShownThisSession = false static func isDismissed() -> Bool { FileManager.default.fileExists(atPath: dismissedMarkerURL().path) } static func shellStartupScriptIfNeeded() -> String? { - guard !isDismissed() else { return nil } + guard !isDismissed(), !hasShownThisSession else { return nil } + hasShownThisSession = true // … rest unchanged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/DockPanelView.swift` around lines 306 - 307, shellStartupScriptIfNeeded() currently only checks persistent disk state via isDismissed(), so the welcome script reappears for each new DockDefaultTerminalRuntime instance; add a process-level latch (e.g. a static Bool like sessionDismissed) and check it at the top of shellStartupScriptIfNeeded() alongside isDismissed(), and set that latch when the user dismisses the welcome (wherever the dismissal handler writes the on-disk marker) so the welcome is suppressed for the rest of the app process even if activate()/reload() constructs new runtime instances.
🤖 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/MainWindowFocusController.swift`:
- Around line 869-890: The restoreDockSurfaceFocus flow leaves a stale
.dockSurface request when dockHost?.focusSurfaceFromCoordinator(surfaceId)
returns false; update restoreDockSurfaceFocus to clear the queued dock request
and fall back to a host-firstItem focus: after a failed
dockHost?.focusSurfaceFromCoordinator(surfaceId) check, reset any pending
beginRightSidebarFocusRequest/rightSidebarFocusState.request for .dock (or
explicitly cancel it), set rightSidebarFocusState to a fallback (.focused or
.request) that targets .host or .firstItem as appropriate, and call
publishFeedFocusSnapshot() so syncAfterResponderChange will not keep
short-circuiting; use the existing symbols restoreDockSurfaceFocus,
beginRightSidebarFocusRequest, rightSidebarFocusState.request,
dockHost?.focusSurfaceFromCoordinator(surfaceId), and publishFeedFocusSnapshot
when implementing this change.
- Around line 226-285: Add cmuxDebugLog(...) calls wrapped in `#if` DEBUG inside
the focus capture/restore entry points to record success, fallback, and failure
paths: insert logs in captureFocusRestoreTarget(),
captureFocusRestoreTarget(owning:),
captureFocusRestoreTarget(workspaceId:panelId:fallbackIntent:), and
restoreFocus(_:) that emit unified tags like "focus.capture", "focus.fallback",
"focus.restore", and more specific tags such as "focus.panel" or
"focus.rightSidebar"; include identifying data (workspaceId, panelId, intent,
mode, dockSurfaceId, and whether fallback was used) and log when a nil/failed
capture occurs so both successful and fallback/failed branches are recorded to
/tmp/cmux-debug.log in DEBUG builds.
---
Duplicate comments:
In `@Sources/DockPanelView.swift`:
- Around line 306-307: shellStartupScriptIfNeeded() currently only checks
persistent disk state via isDismissed(), so the welcome script reappears for
each new DockDefaultTerminalRuntime instance; add a process-level latch (e.g. a
static Bool like sessionDismissed) and check it at the top of
shellStartupScriptIfNeeded() alongside isDismissed(), and set that latch when
the user dismisses the welcome (wherever the dismissal handler writes the
on-disk marker) so the welcome is suppressed for the rest of the app process
even if activate()/reload() constructs new runtime instances.
🪄 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: 8ede37d0-b33e-4fde-90e8-657305178305
📒 Files selected for processing (5)
Sources/AppDelegate.swiftSources/DockPanelView.swiftSources/GhosttyTerminalView.swiftSources/MainWindowFocusController.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
| func captureFocusRestoreTarget() -> MainWindowFocusRestoreTarget? { | ||
| if let responder = window?.firstResponder, | ||
| let target = captureFocusRestoreTarget(owning: responder) { | ||
| return target | ||
| } | ||
|
|
||
| if let target = captureFocusRestoreTargetFromIntent() { | ||
| return target | ||
| } | ||
|
|
||
| return captureSelectedMainPanelFocusRestoreTarget() | ||
| } | ||
|
|
||
| func captureFocusRestoreTarget(owning responder: NSResponder) -> MainWindowFocusRestoreTarget? { | ||
| if let dockSurfaceId = dockHost?.focusedSurfaceIdFromCoordinator(for: responder) { | ||
| return rightSidebarFocusRestoreTarget(mode: .dock, target: .dockSurface(dockSurfaceId)) | ||
| } | ||
|
|
||
| if let mode = rightSidebarModeOwning(responder) { | ||
| return rightSidebarFocusRestoreTarget( | ||
| mode: mode, | ||
| target: rightSidebarRestoreTarget(mode: mode, responder: responder) | ||
| ) | ||
| } | ||
|
|
||
| return captureMainPanelFocusRestoreTarget(owning: responder) | ||
| } | ||
|
|
||
| func captureFocusRestoreTarget( | ||
| workspaceId: UUID, | ||
| panelId: UUID, | ||
| fallbackIntent: PanelFocusIntent | ||
| ) -> MainWindowFocusRestoreTarget? { | ||
| guard let panel = tabManager?.tabs | ||
| .first(where: { $0.id == workspaceId })? | ||
| .panels[panelId] else { | ||
| return nil | ||
| } | ||
| let capturedIntent = panel.captureFocusIntent(in: window) | ||
| return mainPanelFocusRestoreTarget( | ||
| workspaceId: workspaceId, | ||
| panelId: panelId, | ||
| intent: capturedIntent == .panel ? fallbackIntent : capturedIntent | ||
| ) | ||
| } | ||
|
|
||
| @discardableResult | ||
| func restoreFocus(_ target: MainWindowFocusRestoreTarget) -> Bool { | ||
| switch target.destination { | ||
| case .mainPanel(let panel): | ||
| return restoreMainPanelFocus( | ||
| workspaceId: panel.workspaceId, | ||
| panelId: panel.panelId, | ||
| intent: panel.intent | ||
| ) | ||
| case .rightSidebar(let sidebar): | ||
| return restoreRightSidebarFocus(mode: sidebar.mode, target: sidebar.target) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add DEBUG logs around the new focus capture/restore paths.
These new controller entry points are the core of the dock/palette focus restoration flow, but they currently add no unified debug-log events. Please emit cmuxDebugLog(...) entries under #if DEBUG for capture/restore success/fallback/failure so regressions in this path are reconstructible from the shared focus log. As per coding guidelines, "Implement debug events in a unified log in DEBUG builds at /tmp/cmux-debug.log" and "Log key events in AppDelegate.swift, mouse/UI events inline in views, and focus/bonsplit events with specific tags (focus.panel, tab.select, etc.)."
Also applies to: 823-891
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/MainWindowFocusController.swift` around lines 226 - 285, Add
cmuxDebugLog(...) calls wrapped in `#if` DEBUG inside the focus capture/restore
entry points to record success, fallback, and failure paths: insert logs in
captureFocusRestoreTarget(), captureFocusRestoreTarget(owning:),
captureFocusRestoreTarget(workspaceId:panelId:fallbackIntent:), and
restoreFocus(_:) that emit unified tags like "focus.capture", "focus.fallback",
"focus.restore", and more specific tags such as "focus.panel" or
"focus.rightSidebar"; include identifying data (workspaceId, panelId, intent,
mode, dockSurfaceId, and whether fallback was used) and log when a nil/failed
capture occurs so both successful and fallback/failed branches are recorded to
/tmp/cmux-debug.log in DEBUG builds.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be21d99bf8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func focusHostFromCoordinator() -> Bool { | ||
| focusFirstControl?() == true || window?.makeFirstResponder(self) == true | ||
| window?.makeFirstResponder(self) == true | ||
| } |
There was a problem hiding this comment.
This host-focus path now only makes the hidden DockKeyboardFocusView first responder and no longer attempts to focus a dock terminal first. In restore flows that request dock mode with a host target (for example, when re-establishing right-sidebar intent after window reactivation), focus can land on the bridge view instead of a terminal surface, so typing no longer reaches Dock until the user clicks a terminal manually.
Useful? React with 👍 / 👎.
| pendingTarget = nil | ||
| isApplying = true | ||
| return target |
There was a problem hiding this comment.
The state machine clears pendingTarget before attempting restoration, which removes the previous retry behavior for transient focus-restore failures. If the first restore attempt cannot complete immediately (for example during panel/workspace activation timing), the target is already discarded and later responder/focus notifications have nothing left to retry, leaving focus unrestored after dismissing the command palette.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
378b669 to
34dc5a6
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
34dc5a6 to
b206c3f
Compare
b206c3f to
fe08d58
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
fe08d58 to
370ba25
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
370ba25 to
2170471
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2170471 to
d0caf9f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
cmux dock welcome dismissto hide and clear itTesting
swift -frontend -parse CLI/cmux.swift CLI/CMUXCLI+DocsSettings.swift CLI/CMUXCLI+MoveTabToNewWorkspace.swift CLI/CMUXCLI+ThemeSupport.swift CLI/CMUXCLI+Themes.swift CLI/CMUXCLI+TopRendering.swift CLI/SocketOperationTelemetry.swift CLI/cmux_open.swiftswift -frontend -parse Sources/ContentView.swift Sources/RightSidebarPanelView.swift Sources/DockPanelView.swiftswift -frontend -parse Sources/MainWindowFocusController.swift Sources/DockPanelView.swift Sources/AppDelegate.swift Sources/ContentView.swift cmuxTests/ShortcutAndCommandPaletteTests.swiftjq empty Resources/Localizable.xcstringsgit diff --check./scripts/setup.shCMUX_SOCKET=/tmp/cmux-debug-dockcpu.sock xcodebuild test -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -only-testing:cmuxTests/MainWindowFocusControllerRightSidebarHideTests/testFocusRestoreTargetRestoresFocusedDockSurface -derivedDataPath /tmp/cmux-dockcpu-test CODE_SIGNING_ALLOWED=NO./scripts/reload.sh --tag dockfocus./scripts/reload.sh --tag dockcpudockfocusPID:sample 66270 10 -file /tmp/cmux-dockfocus.sample.txtshowed a hot loop throughContentView.attemptCommandPaletteFocusRestoreIfNeeded()andMainWindowFocusController.restoreMainPanelFocus(...)dockcpuruntime check: launched tagged app, sent Cmd-Shift-P then Escape via CGEvent,psshowed0.0%CPU,topshowed0.0-0.1%, andsample 86874 5 -file /tmp/cmux-dockcpu.sample.txthad no command-palette restore hot stackCMUX_DOCK_WELCOME_DISMISSED_PATH: status -> dismiss -> status -> show -> statusIssues
Summary by CodeRabbit
New Features
Localization
Documentation
Tests