Repository navigation
Conversation
|
@rursache is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds PaneFirstClickGate (installed at launch) that records app activation time and, when the pane-first-click setting is disabled, swallows the first click for a 0.2s grace window; UI handlers consult the gate before performing actions. ChangesPane First-Click Gate
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant NSApp as NSApplication
participant Gate as PaneFirstClickGate
participant Settings as PaneFirstClickFocusSettings
participant UI as UIHandler
AppDelegate->>Gate: install()
Note over Gate: register didBecomeActiveNotification observer
NSApp->>Gate: didBecomeActiveNotification (record ProcessInfo.processInfo.systemUptime)
UI->>Gate: shouldSwallowFirstClick(now)
Gate->>Settings: isEnabled()
alt setting enabled
Gate-->>UI: return false (allow click)
else setting disabled
Gate->>Gate: check elapsed time since lastBecameActiveAt
alt within 0.2s
Gate-->>UI: return true (swallow)
else after 0.2s
Gate-->>UI: return false (allow)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 gates minimal-mode chrome and sidebar actions on a new
Confidence Score: 3/5The core gate logic is correct for the common case, but the 200 ms timing constant introduces silent edge-case failures and the actor-isolation contract in The core gate logic is correct for the common case, but the 200 ms timing constant introduces silent edge-case failures and the actor-isolation contract in Sources/App/WorkspaceRuntimeSettings.swift (timing-gate design) and Sources/WindowDecorationsController.swift (unverified actor isolation). Important Files Changed
Sequence DiagramsequenceDiagram
participant OS as macOS
participant NSApp as NSApplication
participant Gate as PaneFirstClickGate
participant Chrome as TitlebarControlButton / WindowDecorationsController
participant Action as Action Handler
OS->>NSApp: App becomes active (user clicks window)
NSApp->>Gate: didBecomeActiveNotification
Gate->>Gate: "lastBecameActiveAt = systemUptime"
Note over OS,Action: First click (within 200ms grace window)
OS->>Chrome: leftMouseDown / SwiftUI tap
Chrome->>Gate: shouldSwallowFirstClick(now:)
Gate-->>Chrome: "true (elapsed < 0.2s)"
Chrome-->>OS: swallow / return nil
Note over OS,Action: Second click (after grace window)
OS->>Chrome: leftMouseDown / SwiftUI tap
Chrome->>Gate: shouldSwallowFirstClick(now:)
Gate-->>Chrome: "false (elapsed >= 0.2s)"
Chrome->>Action: perform action
Reviews (3): Last reviewed commit: "Gate minimal-mode chrome and sidebar row..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
rursache
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/App/WorkspaceRuntimeSettings.swift`:
- Around line 96-123: The static state in PaneFirstClickGate
(lastBecameActiveAt, installed) is accessed from multiple functions (install(),
shouldSwallowFirstClick(now:), markActivatedForTesting(at:)) without guaranteed
main-thread isolation; annotate the enum with `@MainActor` (i.e., make the entire
PaneFirstClickGate `@MainActor`) so all reads/writes are main-thread isolated and
Swift concurrency checks pass, ensuring the notification handler,
shouldSwallowFirstClick(now:), and markActivatedForTesting(at:) are all
MainActor-bound.
🪄 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: aba31b09-8958-47b5-9f1e-8c79af0442f6
📒 Files selected for processing (6)
Sources/App/WorkspaceRuntimeSettings.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/WindowDecorationsController.swiftcmuxTests/InactivePaneFirstClickFocusTests.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| enum PaneFirstClickGate { | ||
| private static let graceInterval: TimeInterval = 0.2 | ||
| private static var lastBecameActiveAt: TimeInterval = 0 | ||
| private static var installed = false | ||
|
|
||
| @MainActor | ||
| static func install() { | ||
| guard !installed else { return } | ||
| installed = true | ||
| NotificationCenter.default.addObserver( | ||
| forName: NSApplication.didBecomeActiveNotification, | ||
| object: nil, | ||
| queue: .main | ||
| ) { _ in | ||
| lastBecameActiveAt = ProcessInfo.processInfo.systemUptime | ||
| } | ||
| } | ||
|
|
||
| static func shouldSwallowFirstClick(now: TimeInterval = ProcessInfo.processInfo.systemUptime) -> Bool { | ||
| if PaneFirstClickFocusSettings.isEnabled() { return false } | ||
| let elapsed = now - lastBecameActiveAt | ||
| return elapsed >= 0 && elapsed < graceInterval | ||
| } | ||
|
|
||
| static func markActivatedForTesting(at time: TimeInterval = ProcessInfo.processInfo.systemUptime) { | ||
| lastBecameActiveAt = time | ||
| } | ||
| } |
There was a problem hiding this comment.
PaneFirstClickGate static vars
lastBecameActiveAt and installed are plain static var properties with no isolation annotation. The notification callback that writes lastBecameActiveAt runs on queue: .main (i.e., @MainActor), but shouldSwallowFirstClick() and markActivatedForTesting() are nonisolated, so Swift 6 strict concurrency cannot verify the reads are safe — they are a static data race from the compiler's perspective. All callers (onTapGesture, SwiftUI Button body, AppKit event handlers) are on the main actor in practice, but without the annotation the compiler cannot check this. Adding @MainActor to the two static vars (or to shouldSwallowFirstClick and markActivatedForTesting) makes the intent explicit and lets Swift 6 enforce it.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/App/WorkspaceRuntimeSettings.swift (1)
96-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIsolate the full gate on MainActor to avoid static-state races
install()is main-actor isolated, butshouldSwallowFirstClickandmarkActivatedForTestingcan still touch shared static state off-main. On Line 114 and Line 120, this keeps a concurrency hole open under strict Swift concurrency checks.Suggested fix
+@MainActor enum PaneFirstClickGate { private static let graceInterval: TimeInterval = 0.2 private static var lastBecameActiveAt: TimeInterval = 0 private static var installed = false - `@MainActor` static func install() { guard !installed else { return } installed = true NotificationCenter.default.addObserver(You can verify call-site isolation and current annotations with:
#!/bin/bash set -euo pipefail echo "Definitions and actor annotations:" rg -n -C2 'enum PaneFirstClickGate|@MainActor|static func (install|shouldSwallowFirstClick|markActivatedForTesting)' Sources/App/WorkspaceRuntimeSettings.swift echo echo "Call sites of shouldSwallowFirstClick:" rg -n -C2 'PaneFirstClickGate\.shouldSwallowFirstClick\s*\(' Sources cmuxTests echo echo "Call sites of markActivatedForTesting:" rg -n -C2 'PaneFirstClickGate\.markActivatedForTesting\s*\(' Sources cmuxTests🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/App/WorkspaceRuntimeSettings.swift` around lines 96 - 123, The static shared state in PaneFirstClickGate is not fully MainActor-isolated (install is but shouldSwallowFirstClick and markActivatedForTesting are not), so annotate the whole enum with `@MainActor` (add `@MainActor` before enum PaneFirstClickGate) to ensure lastBecameActiveAt and all static methods are actor-isolated; alternatively annotate both static methods and the stored properties with `@MainActor` if you prefer finer-grained changes, and update any tests/call sites to call these APIs from the main actor if needed.
🤖 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 `@cmuxTests/InactivePaneFirstClickFocusTests.swift`:
- Line 78: The test currently asserts
PaneFirstClickGate.shouldSwallowFirstClick(now: now + 0.05); update it to assert
behavior closer to the 200ms grace boundary to catch edge cases—replace or add
an assertion using PaneFirstClickGate.shouldSwallowFirstClick(now: now + 0.15)
or now + 0.19 (e.g., now + 0.19) so the test validates the gate behavior near
the 200ms threshold; keep the original assertion or add an additional test case
if you want both near-start and near-boundary coverage.
---
Duplicate comments:
In `@Sources/App/WorkspaceRuntimeSettings.swift`:
- Around line 96-123: The static shared state in PaneFirstClickGate is not fully
MainActor-isolated (install is but shouldSwallowFirstClick and
markActivatedForTesting are not), so annotate the whole enum with `@MainActor`
(add `@MainActor` before enum PaneFirstClickGate) to ensure lastBecameActiveAt and
all static methods are actor-isolated; alternatively annotate both static
methods and the stored properties with `@MainActor` if you prefer finer-grained
changes, and update any tests/call sites to call these APIs from the main actor
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: db4a390d-35f6-4401-ac4e-b69744f587d0
📒 Files selected for processing (6)
Sources/App/WorkspaceRuntimeSettings.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/WindowDecorationsController.swiftcmuxTests/InactivePaneFirstClickFocusTests.swift
|
Addressed CodeRabbit's actor-isolation note in the latest force-push: annotated |
When `focusPaneOnFirstClick` is disabled, the setting was honoured for pane content (terminal, web view, markdown observer) but five chrome paths still fired their actions on the first click of an inactive window: - `TitlebarControlButton` SwiftUI `Button`s in the titlebar (bell, sidebar toggle, `+`), whose action fires regardless of activation. - The `NSEvent.addLocalMonitor` in `WindowDecorationsController` that resolves minimal-mode chrome clicks via the hit-region registry and calls `performMinimalModeSidebarControlAction` directly, bypassing every view-level check. - The per-window send-event hook in the same controller. - The sidebar workspace row's `.onTapGesture`, which switched workspaces on first click (the most surprising symptom). Adds `PaneFirstClickGate` next to `PaneFirstClickFocusSettings`. It observes `NSApplication.didBecomeActiveNotification` and exposes `shouldSwallowFirstClick(now:)`, true within a 200 ms grace after activation when the setting is disabled. By the time these SwiftUI actions and event-monitor callbacks run, `NSApp.isActive` is already true, so an activation-timestamp gate is the only reliable per-click signal. The gate is installed once from `applicationDidFinishLaunching` and queried at each of the four chrome/sidebar action sites. Extends `InactivePaneFirstClickFocusTests` with grace-window, after-grace, and setting-enabled coverage for the gate.
|
Added a near-boundary assertion ( |
|
closing because it's fixed in #3881 |
1 participant
Closes #3856.
Summary
With
app.focusPaneOnFirstClick: false, clicks on cmux chrome while the window was inactive still fired actions on first click. The setting was already honoured for pane content (terminal, web view, markdown observer) but bypassed for chrome and sidebar via five separate code paths that I traced with a temporary instrumented build:TitlebarControlButtonSwiftUIButtons in the titlebar (bell, sidebar toggle,+)NSEvent.addLocalMonitorinWindowDecorationsControllerthat resolves minimal-mode chrome clicks via the hit-region registry and callsperformMinimalModeSidebarControlActiondirectly (this one bypasses every view-level check, including anyacceptsFirstMouseoverride).onTapGesture, which switched workspaces on first click — the most surprising symptomApproach
Adds
PaneFirstClickGatenext toPaneFirstClickFocusSettingsinWorkspaceRuntimeSettings.swift. It observesNSApplication.didBecomeActiveNotificationand exposesshouldSwallowFirstClick(now:), which returnstruefor 200 ms after activation when the setting is disabled. The four action sites consult the gate before firing.The grace-window approach is necessary because by the time these SwiftUI actions and event-monitor callbacks run,
NSApp.isActiveis alreadytrue— there's no other reliable per-click signal. 200 ms is short enough that a deliberate second click is well past the window and long enough that the activation click reliably falls inside it.Test plan
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -only-testing:cmuxTests/InactivePaneFirstClickFocusTests test— all 9 tests pass (6 existing + 3 new gate-behaviour cases)focusPaneOnFirstClick: falseandminimalMode: true, cmux unfocused:+button: first click only activates cmux, second click fires the actionfocusPaneOnFirstClick: true: all four still fire on first click as beforeSummary by CodeRabbit