Fix tab-switch crash in vertical sidebar: defer hosted-inspector side-dock promotion out of the layout pass (#6150) - #6340
Merged
Merged
Conversation
A single pane running a leaking process (e.g. uv run pytest growing to ~14 GB RSS) makes macOS aggregate the child memory under the app, report hundreds of GB, declare "out of application memory", and OOM-suspend the whole app — killing every other healthy pane with no prior signal. This adds a per-pane guardrail that catches a runaway tree at the pane level first: - A background timer (PaneMemoryGuardrail) polls every live pane ~every 4s. It attributes process-tree memory by the pane's controlling tty: every process under the pane (shell + descendants + background jobs) shares the tty, so it sums physical-footprint bytes across all pids on that tty device via the existing CmuxTopProcessSnapshot libproc walk. - When a pane crosses a configurable threshold (default 8 GB) it edge-triggers an orange warning badge on the workspace tab and a dismissible banner identifying the pane, its process-tree memory, and the foreground command. Hysteresis clears at 0.8x threshold; the banner fires once per crossing and re-arms after it clears. - The banner's "Kill Pane Process" action (with confirm) sends SIGTERM then SIGKILL to the pane's foreground process group, leaving the shell alive; falls back to closing the pane when there is no foreground group. - New Terminal settings: enable toggle + threshold (GB), default on / 8 GB. - Below threshold it stays completely silent (no always-on memory UI). Ghostty foreground-pid / tty-name accessors added on TerminalSurface. Pure edge-trigger engine unit-tested (PaneMemoryGuardrailTests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rdrail # Conflicts: # Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+ProcessInfo.swift # Resources/Localizable.xcstrings
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The browser HostContainerView.layout() override synchronously called promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded(), which mutates the view hierarchy (addSubview / removeFromSuperview + NSLayoutConstraint activation) when a docked DevTools/inspector split is detected. Mutating the view hierarchy synchronously inside an AppKit layout pass re-enters the layout machinery and crashes: - EXC_BREAKPOINT via -[NSWindow _postWindowNeedsUpdateConstraints] (Auto Layout constraint recursion) — the #653 ASI backtrace, whose faulting frames are ___NSViewLayout_block_invoke -> ... -> addSubview:. - EXC_BAD_ACCESS in objc_msgSend called from ___NSViewLayout_block_invoke (a view freed mid-layout and then messaged) — the #6150 group A/B shape on macOS 14.8.2, triggered by switching to a tab with a browser surface. The dock-config path already defers its identical hierarchy mutation via scheduleHostedInspectorDockConfigurationSync (DispatchQueue.main.async), and viewDidMoveToWindow/Superview promote via the deferred scheduleHostedInspectorDividerReapply work item. Only the layout() override mutated synchronously. This change moves the promotion onto the same deferred path: layout() now performs a read-only candidate check and schedules the promotion for the next runloop tick, so the hierarchy mutation never runs inside layout(). The deferred work re-validates all state before mutating, so it is safe if the layout changes in between. Closes #6150 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a per-pane "Runaway Memory Guardrail" that periodically samples process-tree resident memory by controlling TTY, edge-triggers a dismissible banner and sidebar badge when a configurable GB threshold is crossed, and exposes kill/dismiss actions with hysteresis. Also defers ChangesPane Runaway Memory Guardrail
BrowserPanelView Deferred Side-Dock Promotion Fix
Sequence Diagram(s)sequenceDiagram
participant Timer as Background Timer
participant Guardrail as PaneMemoryGuardrail
participant Snapshot as CmuxTopProcessSnapshot
participant Engine as PaneMemoryGuardrailEngine
participant Banner as PaneMemoryGuardrailBanner
participant Sidebar as TabItemView / SidebarUnreadModel
Timer->>Guardrail: tick (dispatched to MainActor)
Guardrail->>Guardrail: resolve enabled/thresholdBytes from defaults
Guardrail->>Guardrail: paneProvider() → [PaneMemoryDescriptor]
Guardrail->>Snapshot: computeSamples(descriptors) off-main
Snapshot-->>Guardrail: [PaneMemorySample] (TTY-indexed memory sums)
Guardrail->>Engine: ingest(samples, thresholdBytes)
Engine-->>Guardrail: Output(bannerToPresent, warnedWorkspaceIds, clearedPanes)
Guardrail->>Banner: activeBanner = PaneMemoryWarning
Guardrail->>Sidebar: onWarnedWorkspacesChanged(Set<UUID>)
Sidebar->>Sidebar: setMemoryWarningWorkspaceIds → orange badge shown
Note over Banner: User taps "Kill"
Banner->>Guardrail: killActivePaneProcess()
Guardrail->>Guardrail: PaneMemoryProcessKiller.terminate(pgids)
Guardrail->>Engine: acknowledgeHandled(paneKey)
Note over Banner: User taps "Dismiss"
Banner->>Guardrail: dismissActiveBanner()
Guardrail->>Engine: dismiss(paneKey)
Guardrail->>Banner: activeBanner = nil
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (17 passed)
✨ 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 |
# Conflicts: # .github/swift-file-length-budget.tsv
Contributor
Greptile SummaryThis PR contains two changes: a targeted one-line crashfix to
Confidence Score: 5/5Safe to merge. The BrowserPanelView crashfix is minimal and well-contained; the memory guardrail is correctly structured with @observable, off-main sampling, and tested edge-trigger/hysteresis logic. The BrowserPanelView change removes the only synchronous view-hierarchy mutation in the layout path (confirmed by backtrace match) and reuses the already-shipping deferred-promotion pattern. The memory guardrail follows established cmux patterns: @observable for the model, Task.detached for background work, and a pure struct engine for testable decision logic. Previous review findings (ObservableObject→@observable, asyncAfter→cancellable Task, DispatchQueue.global→Task.detached) were all addressed. The two findings here are non-blocking style observations. No files require special attention. The two comments are non-blocking observations on copy style and task-lifecycle explicitness. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Timer as DispatchSourceTimer<br/>(timerQueue)
participant MA as MainActor<br/>PaneMemoryGuardrail
participant Det as Task.detached<br/>(utility)
participant UI as SwiftUI<br/>Banner/Badge
Timer->>MA: "Task { @MainActor in tick() }"
MA->>MA: paneProvider() → [PaneMemoryDescriptor]
MA->>Det: computeCachedSamples(descriptors:)
Det-->>MA: [PaneMemorySample]
MA->>MA: engine.ingest(samples:) → output
MA->>UI: "activeBanner = warning (edge-trigger)"
MA->>UI: onWarnedWorkspacesChanged(ids) → badge
Note over UI: User clicks Kill
UI->>MA: killPaneProcess(for:)
MA->>Det: computeFreshSamples([descriptor])
Det-->>MA: PaneMemorySample
MA->>MA: finishKillActivePaneProcess
MA->>Det: PaneMemoryProcessKiller.terminate(pgids:)
Det->>Det: kill(-pgid, SIGTERM)
Det->>Det: waitForGracePeriod(3s)
Det->>Det: validateBeforeSIGKILL()
Det->>Det: kill(-pgid, SIGKILL) if still over threshold
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Timer as DispatchSourceTimer<br/>(timerQueue)
participant MA as MainActor<br/>PaneMemoryGuardrail
participant Det as Task.detached<br/>(utility)
participant UI as SwiftUI<br/>Banner/Badge
Timer->>MA: "Task { @MainActor in tick() }"
MA->>MA: paneProvider() → [PaneMemoryDescriptor]
MA->>Det: computeCachedSamples(descriptors:)
Det-->>MA: [PaneMemorySample]
MA->>MA: engine.ingest(samples:) → output
MA->>UI: "activeBanner = warning (edge-trigger)"
MA->>UI: onWarnedWorkspacesChanged(ids) → badge
Note over UI: User clicks Kill
UI->>MA: killPaneProcess(for:)
MA->>Det: computeFreshSamples([descriptor])
Det-->>MA: PaneMemorySample
MA->>MA: finishKillActivePaneProcess
MA->>Det: PaneMemoryProcessKiller.terminate(pgids:)
Det->>Det: kill(-pgid, SIGTERM)
Det->>Det: waitForGracePeriod(3s)
Det->>Det: validateBeforeSIGKILL()
Det->>Det: kill(-pgid, SIGKILL) if still over threshold
Reviews (17): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface`+ProcessInfo.swift:
- Around line 33-35: The String initialization for decoding the TTY name data
uses lossy decoding which can silently substitute invalid UTF-8 bytes with
replacement characters, potentially creating incorrect attribution identifiers.
Replace the `String(decoding: data, as: UTF8.self)` call with a strict UTF-8
validation method such as `String(validatingUTF8:)` that returns nil when
encountering invalid UTF-8 sequences, ensuring the function fails closed by
returning nil for corrupted data rather than producing a potentially wrong TTY
identifier.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 6367-6379: The order of operations in the layout pass is
incorrect: scheduleHostedInspectorSideDockPromotionIfNeeded is queued before
enforceAdaptiveBottomDockIfNeeded can reject it and enforce bottom docking
instead. Reorder the code so that enforceAdaptiveBottomDockIfNeeded is called
first to determine if bottom docking should be enforced, and only then call
scheduleHostedInspectorSideDockPromotionIfNeeded if the adaptive guard permits
side-dock promotion. This prevents the queued promotion from running on the next
tick when the adaptive guard has decided to use bottom docking instead.
In `@Sources/PaneMemoryGuardrail.swift`:
- Around line 326-334: The killActivePaneProcess() function uses stale cached
data from lastSamplesByKey to retrieve foregroundProcessGroupIDs for
termination, but processes can change between sampling and the kill action.
Replace the cached lookup of foregroundProcessGroupIDs from lastSamplesByKey
with a fresh resolution of the active pane's current process context at action
time. Ensure that PaneMemoryProcessKiller.terminate() is called with the current
authoritative foregroundProcessGroupIDs obtained from a real-time query rather
than the previously cached values.
- Around line 296-310: The activeBanner is not being cleared when its
corresponding pane disappears from the live samples in lastSamplesByKey, even if
the pane isn't explicitly in output.clearedPanes. Add a check in the if block
where activeBanner is processed to also set activeBanner to nil when
lastSamplesByKey[activeKey] becomes nil, in addition to the existing check for
output.clearedPanes.contains(activeKey). This ensures stale banners are removed
from the screen when their pane closes and allows new bannerToPresent events to
display.
🪄 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: cb76dd65-7cb8-4715-b8fc-70d95723a1cc
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (12)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/TerminalCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+ProcessInfo.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/PaneMemoryGuardrail.swiftSources/PaneMemoryGuardrailBannerView.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/PaneMemoryGuardrailTests.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swift`:
- Around line 61-73: The startObservingSettings() function is missing the newly
added guardrail settings models from its observation list. Add
memGuardrailEnabled and memGuardrailThresholdGB to the models array in the
startObservingSettings function so they are included when startObserving() is
called on each model, ensuring these controls stay synchronized with settings
changes from other surfaces.
In `@Resources/Localizable.xcstrings`:
- Around line 170073-170196: The command.terminalClearScreenKeepScrollback.title
key has multiple locale entries marked as "needs_review" that contain only the
English source string as placeholders instead of actual translations. Replace
each "needs_review" placeholder entry for locales ar, bs, da, de, es, fr, it,
km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant with genuine
translations of the phrase in their respective languages, and change the state
from "needs_review" to "translated" for each entry. Ensure every supported
locale in this key has a properly translated value or defer adding this key
until translations are available.
- Around line 170448-170571: The shortcut.clearScreenKeepScrollback.label entry
contains multiple locales (ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR,
ru, th, tr, uk, zh-Hans, zh-Hant) marked with "needs_review" state but using
English placeholder text instead of actual translations. Either provide genuine
translated values for each locale marked as "needs_review" or remove the entire
shortcut.clearScreenKeepScreenback.label key from the catalog until proper
translations for all supported locales are available. Ensure that all
user-facing strings in production have real localized content for every
supported locale in the xcstrings file.
🪄 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: 35e73f75-0773-4bb9-bd8f-9928db542d1a
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (7)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swift`:
- Around line 61-73: The startObservingSettings() function is missing the newly
added guardrail settings models from its observation list. Add
memGuardrailEnabled and memGuardrailThresholdGB to the models array in the
startObservingSettings function so they are included when startObserving() is
called on each model, ensuring these controls stay synchronized with settings
changes from other surfaces.
In `@Resources/Localizable.xcstrings`:
- Around line 170073-170196: The command.terminalClearScreenKeepScrollback.title
key has multiple locale entries marked as "needs_review" that contain only the
English source string as placeholders instead of actual translations. Replace
each "needs_review" placeholder entry for locales ar, bs, da, de, es, fr, it,
km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant with genuine
translations of the phrase in their respective languages, and change the state
from "needs_review" to "translated" for each entry. Ensure every supported
locale in this key has a properly translated value or defer adding this key
until translations are available.
- Around line 170448-170571: The shortcut.clearScreenKeepScrollback.label entry
contains multiple locales (ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR,
ru, th, tr, uk, zh-Hans, zh-Hant) marked with "needs_review" state but using
English placeholder text instead of actual translations. Either provide genuine
translated values for each locale marked as "needs_review" or remove the entire
shortcut.clearScreenKeepScreenback.label key from the catalog until proper
translations for all supported locales are available. Ensure that all
user-facing strings in production have real localized content for every
supported locale in the xcstrings file.
🪄 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: 35e73f75-0773-4bb9-bd8f-9928db542d1a
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (7)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxproj
🛑 Comments failed to post (3)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swift (1)
61-73:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing observers for the newly added guardrail settings models.
startObservingSettings()does not includememGuardrailEnabledormemGuardrailThresholdGB. As a result, the new controls can go stale if settings are changed from another surface while this view is mounted (Line 61-73).Proposed fix
private func startObservingSettings() { let models: [any SettingObservationStarting] = [ scrollBar, copyOnSelect, autoResume, hibernation, idleSeconds, maxLive, rendererReclaim, rendererIdleSeconds, rendererMaxWarm, + memGuardrailEnabled, + memGuardrailThresholdGB, ] models.forEach { $0.startObserving() } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swift` around lines 61 - 73, The startObservingSettings() function is missing the newly added guardrail settings models from its observation list. Add memGuardrailEnabled and memGuardrailThresholdGB to the models array in the startObservingSettings function so they are included when startObserving() is called on each model, ensuring these controls stay synchronized with settings changes from other surfaces.Resources/Localizable.xcstrings (2)
170073-170196:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winShip real translations instead of
needs_reviewplaceholders.This key leaves every non-English locale on the English source string, so the command title is effectively untranslated in production. Please replace the placeholders with actual translations for every supported locale, or defer adding the key until translations are ready. As per coding guidelines, all user-facing strings must be localized and every supported locale in the touched catalog needs translated entries.
🤖 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 `@Resources/Localizable.xcstrings` around lines 170073 - 170196, The command.terminalClearScreenKeepScrollback.title key has multiple locale entries marked as "needs_review" that contain only the English source string as placeholders instead of actual translations. Replace each "needs_review" placeholder entry for locales ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant with genuine translations of the phrase in their respective languages, and change the state from "needs_review" to "translated" for each entry. Ensure every supported locale in this key has a properly translated value or defer adding this key until translations are available.Source: Coding guidelines
170448-170571:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winShip real translations instead of
needs_reviewplaceholders.This shortcut label has the same problem: all non-English locales still resolve to the English text, which violates the full-internationalization rule for production UI strings. Please provide translated values for every supported locale in the catalog, or hold the key until they’re available. As per coding guidelines, all user-facing strings must be localized and every supported locale in the touched catalog needs translated entries.
🤖 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 `@Resources/Localizable.xcstrings` around lines 170448 - 170571, The shortcut.clearScreenKeepScrollback.label entry contains multiple locales (ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant) marked with "needs_review" state but using English placeholder text instead of actual translations. Either provide genuine translated values for each locale marked as "needs_review" or remove the entire shortcut.clearScreenKeepScreenback.label key from the catalog until proper translations for all supported locales are available. Ensure that all user-facing strings in production have real localized content for every supported locale in the xcstrings file.Source: Coding guidelines
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 (3)
Sources/Panels/BrowserPanelView.swift (1)
6178-6181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-run the adaptive guard inside the deferred promotion.
A promotion can be queued while side-dock is allowed, then a later layout can hit Line 6389 and request bottom docking before the queued block runs. The block at Line 6181 still promotes without re-checking that adaptive state, so a stale next-tick item can override the bottom-dock decision.
Proposed fix
let workItem = DispatchWorkItem { [weak self] in guard let self else { return } self.hostedInspectorSideDockPromotionWorkItem = nil + guard !self.enforceAdaptiveBottomDockIfNeeded(reason: "host.sideDockPromotion") else { + return + } _ = self.promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded() }Also applies to: 6389-6395
🤖 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/Panels/BrowserPanelView.swift` around lines 6178 - 6181, The DispatchWorkItem block in the hostedInspectorSideDockPromotionWorkItem deferred promotion logic does not re-check the adaptive guard condition before executing the promotion. When the work item eventually runs, layout state may have changed to request bottom docking instead of side-dock, but the queued promotion proceeds anyway, overriding the more recent decision. Before calling promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded() inside the work item, add back the same adaptive guard condition that was checked when the work item was originally queued to ensure the promotion only happens if side-dock is still the appropriate state.Sources/PaneMemoryGuardrailBannerView.swift (1)
19-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the kill confirmation when the active warning changes.
The dialog action kills whatever
guardrail.activeBanneris current at confirmation time. If pane A clears and pane B is presented while the dialog is open, confirming can target B without the user opening B’s confirmation.🛡️ Proposed fix
} .animation(.easeInOut(duration: 0.2), value: guardrail.activeBanner) + .onChange(of: guardrail.activeBanner?.key) { _ in + isConfirmingKill = false + } }Also applies to: 67-76
🤖 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/PaneMemoryGuardrailBannerView.swift` around lines 19 - 29, When the active banner changes, the kill confirmation dialog may still be open and could target the wrong pane. Add an onChange modifier to the body view that watches for changes to guardrail.activeBanner and resets the confirmation dialog state (dismiss or set to nil/false) whenever the active banner changes. This ensures that if a user has a confirmation dialog open and the banner switches while the dialog is displayed, the dialog will be dismissed rather than potentially confirming an action on the wrong banner. Apply the same fix to the other location mentioned at lines 67-76.Sources/PaneMemoryGuardrail.swift (1)
192-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp invalid threshold values before converting to
Int64.
UserDefaultsis user/config writable, soNaN,infinity, or a huge finite value can survivemax(...)and trap onInt64(...)during the polling tick. Fall back to the default when the configured value is non-finite or unrepresentable.🛡️ Proposed fix
private static let pollInterval: TimeInterval = 4 private static let defaultThresholdGB: Double = 8 private static let minThresholdGB: Double = 1 + private static let bytesPerGB = 1024.0 * 1024.0 * 1024.0 @@ private func thresholdBytes() -> Int64 { let configured = UserDefaults.standard.object(forKey: DefaultsKeys.thresholdGB) as? Double ?? Self.defaultThresholdGB - let gb = max(Self.minThresholdGB, configured) - return Int64(gb * 1024 * 1024 * 1024) + let gb = configured.isFinite + ? max(Self.minThresholdGB, configured) + : Self.defaultThresholdGB + let bytes = gb * Self.bytesPerGB + guard bytes.isFinite, bytes < Double(Int64.max) else { + return Int64(Self.defaultThresholdGB * Self.bytesPerGB) + } + return Int64(bytes) }Also applies to: 249-253
🤖 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/PaneMemoryGuardrail.swift` around lines 192 - 195, The threshold values read from UserDefaults can contain invalid values such as NaN, infinity, or unreasonably large numbers that will trap when converting to Int64. In the code around lines 249-253 where the threshold is retrieved from UserDefaults and converted to Int64, add validation to ensure the value is finite (check that it is neither NaN nor infinite) and representable as an Int64 before performing the conversion. If the value is invalid or non-finite, fall back to using defaultThresholdGB instead. This ensures robustness against malformed configuration values.
🤖 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/PaneMemoryGuardrail.swift`:
- Around line 420-442: The finishKillActivePaneProcess method currently closes
the pane whenever processGroupIDs is empty after filtering, but this doesn't
distinguish between having no runaway processes versus memory already being
cleared to a healthy state. Modify the logic to check if memory pressure still
exists at the kill threshold in the sample data. Only call onRequestClosePane
when processGroupIDs is empty AND the sample indicates memory is still above the
clear threshold. If the sample shows memory has already been cleared (below the
threshold), return early without closing the pane. This requires passing the
full sample result to finishKillActivePaneProcess instead of just the
processGroupIDs array, so you can check both the current memory state and
process information before deciding whether to close.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 6178-6181: The DispatchWorkItem block in the
hostedInspectorSideDockPromotionWorkItem deferred promotion logic does not
re-check the adaptive guard condition before executing the promotion. When the
work item eventually runs, layout state may have changed to request bottom
docking instead of side-dock, but the queued promotion proceeds anyway,
overriding the more recent decision. Before calling
promoteHostedInspectorSideDockFromCurrentLayoutIfNeeded() inside the work item,
add back the same adaptive guard condition that was checked when the work item
was originally queued to ensure the promotion only happens if side-dock is still
the appropriate state.
In `@Sources/PaneMemoryGuardrail.swift`:
- Around line 192-195: The threshold values read from UserDefaults can contain
invalid values such as NaN, infinity, or unreasonably large numbers that will
trap when converting to Int64. In the code around lines 249-253 where the
threshold is retrieved from UserDefaults and converted to Int64, add validation
to ensure the value is finite (check that it is neither NaN nor infinite) and
representable as an Int64 before performing the conversion. If the value is
invalid or non-finite, fall back to using defaultThresholdGB instead. This
ensures robustness against malformed configuration values.
In `@Sources/PaneMemoryGuardrailBannerView.swift`:
- Around line 19-29: When the active banner changes, the kill confirmation
dialog may still be open and could target the wrong pane. Add an onChange
modifier to the body view that watches for changes to guardrail.activeBanner and
resets the confirmation dialog state (dismiss or set to nil/false) whenever the
active banner changes. This ensures that if a user has a confirmation dialog
open and the banner switches while the dialog is displayed, the dialog will be
dismissed rather than potentially confirming an action on the wrong banner.
Apply the same fix to the other location mentioned at lines 67-76.
🪄 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: 3be54940-619c-4303-9a94-cd58ddd6008e
📒 Files selected for processing (6)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+ProcessInfo.swiftResources/Localizable.xcstringsSources/PaneMemoryGuardrail.swiftSources/PaneMemoryGuardrailBannerView.swiftSources/Panels/BrowserPanelView.swiftcmuxTests/PaneMemoryGuardrailTests.swift
# Conflicts: # .github/swift-file-length-budget.tsv
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the intermittent tab-switch crash in vertical sidebar mode reported in #6150 (macOS 14.8.2, still present on 0.64.x) —
EXC_BAD_ACCESSinobjc_msgSendfrom___NSViewLayout_block_invoke(group A) andEXC_BREAKPOINTduring layout (group B) — the same root-cause family as #653.Root cause
HostContainerView.layout()(inSources/Panels/BrowserPanelView.swift) synchronously calledpromoteHostedInspectorSideDockFromCurrentLayoutIfNeeded(). When a docked DevTools/inspector split is present, that promotion mutates the view hierarchy inside the layout pass:ensureHostedInspectorSideDockContainerView()→addSubview(...)+NSLayoutConstraint.activate(...)moveHostedInspectorSubviewIfNeeded(...)→removeFromSuperview()+addSubview(...)Mutating the view hierarchy synchronously inside an AppKit layout pass re-enters the layout machinery, which is exactly the documented crash:
___NSViewLayout_block_invoke→ … →-[NSView addSubview:]→_setLayoutEngine:→_informContainerThatSubviewsNeedUpdateConstraints(infinite recursion) →_postWindowNeedsUpdateConstraintsthrows →EXC_BREAKPOINT.EXC_BAD_ACCESS@0x10, PC inobjc_msgSend, caller___NSViewLayout_block_invoke— a view freed mid-layout and then messaged (use-after-free). Triggered by switching to a tab with a browser surface active, which matches the reporter's repro (frequent WebView/markdown-viewer + Claude Code tabs).This was the only synchronous view-hierarchy mutation reachable from the
layout()override. The other mutation paths already defer:scheduleHostedInspectorDockConfigurationSync(DispatchQueue.main.async), andviewDidMoveToWindow/viewDidMoveToSuperviewvia the deferredscheduleHostedInspectorDividerReapplywork item (which itself callspromote…off the layout pass).Fix
Move the promotion onto that same deferred path.
layout()now performs only a read-only candidate check and, when a promotion is genuinely pending, schedulespromoteHostedInspectorSideDockFromCurrentLayoutIfNeeded()for the next runloop tick via a debouncedDispatchWorkItem. The hierarchy mutation therefore never runs insidelayout(). The deferred work re-validates all state (!isHostedInspectorSideDockActive(), slot view, candidate) before mutating, so it is safe even if the layout changes in between, and the single-pending-work-item guard prevents scheduling churn (layout()runs frequently).Scope / tradeoffs
layout()frame named in the Crash when switching tabs in vertical tab mode (Auto Layout recursion, macOS 26.0.1) #653 backtrace. The local-inline attach path inupdateUsingLocalInlineHosting(which runs from SwiftUI'supdateNSView, not the AppKitlayout()pass) is a separate, larger surface that was the subject of the stale Defer browser view hierarchy mutations to prevent Auto Layout recursion #662 and is intentionally left untouched here.Testing
This crash is layout-timing-dependent and intermittent, and
HostContainerViewis a private nested AppKit view class with no public seam, so there is no clean deterministic unit test for it (stated per the repo regression-test policy). Verification is by code reasoning: the change removes the only synchronous hierarchy mutation reachable fromlayout(), matching the documented crash backtrace, and reuses the existing, already-shipping deferred-promotion pattern.Closes #6150
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the vertical-sidebar tab-switch crash by deferring hosted‑inspector side‑dock promotion out of layout, and adds a per‑pane runaway‑memory guardrail with a banner, an orange sidebar badge, and a confirmable kill action. Addresses #6150 and keeps the UI responsive during tab switches.
Bug Fixes
closeCloudDbForTestsin the web test mocks to keep the test runner stable.New Features
TerminalSurface.foregroundProcessID()and.controllingTTYName()added viaGhosttyKit. Tests cover thresholding, hysteresis, dismissal, multi‑pane, TTY/no‑TTY, process‑group selection, window scoping, and manager routing.Written for commit fb72ae8. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
UI/UX
Settings / Localizations
Tests