Repository navigation
Fix split-time terminal config inheritance crash - #1927
jorgitin02 wants to merge 5 commits into
Conversation
|
@jorgitin02 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughTabManager now materializes CMUX-inherited Ghostty configs into a mutable config and applies a "last remembered" font-size fallback when available; Workspace removes runtime-derived font-size overrides and simplifies inheritance resolution. New tests validate remembered-zoom behavior and stale-zoom handling for new terminal splits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant TabMgr as TabManager
participant Snapshot as Snapshot
participant Workspace as Workspace
participant Panel as TerminalPanel
User->>TabMgr: request new split
TabMgr->>Snapshot: read selectedWorkspace & rememberedFontPoints
Snapshot-->>TabMgr: rememberedFontPoints (maybe nil)
TabMgr->>Workspace: request cmux-inherited config
Workspace-->>TabMgr: inheritedConfig
TabMgr->>TabMgr: copy inheritedConfig -> config (mutable)
alt rememberedFontPoints > 0
TabMgr->>TabMgr: set config.font_size = rememberedFontPoints
end
TabMgr->>Panel: create new terminal with config
Panel-->>User: new split ready
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 |
![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: ed5caa9301
ℹ️ 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".
Greptile SummaryThis PR fixes a split-time terminal config inheritance crash (issue #1870) by removing the live Ghostty runtime font-size probe (
Process note (CLAUDE.md regression test commit policy): The CLAUDE.md style guide requires a two-commit structure for bug-fix regression tests (commit 1: failing test only; commit 2: fix), so that CI demonstrates the test catches the bug. The author explicitly notes this structure was not followed here because the tests did not reproduce the crash locally. This PR should be merged with awareness that the tests serve as behavioral coverage on the current host, but the red/green commit sequence that CLAUDE.md requires for confidence in regression tests is absent. Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Split / New-workspace created"] --> B{"Live source surface\navailable?"}
B -->|Yes| C["cmuxInheritedSurfaceConfig\n(ghostty_surface_inherited_config only;\nno runtime font probe)"]
C --> D{"rememberedFontPoints\n> 0?"}
D -->|Yes| E["Override config.font_size\nwith remembered value"]
D -->|No| F["Keep ghostty-inherited font_size"]
E --> G["Return config"]
F --> G
B -->|No – split path| H["resolvedTerminalInheritanceFontPoints"]
H --> I{"terminalInheritanceFontPointsByPanelId\n(rooted) > 0?"}
I -->|Yes| J["Use rooted (cached) points"]
I -->|No| K{"inheritedConfig.font_size\n> 0?"}
K -->|Yes| L["Use inheritedConfig.font_size"]
K -->|No| M["Return nil"]
J --> N["config.font_size = rootedFontPoints\nupdate panel cache"]
L --> N
M --> O{"lastTerminalConfigInheritanceFontPoints\nset?"}
O -->|Yes| P["Bare config with\nlast-known font size"]
O -->|No| Q["Return nil\n(no inheritance)"]
G --> R["New terminal created\nwith inherited config"]
N --> R
P --> R
Last reviewed commit: "fix: avoid split-tim..." |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 1103-1125: The readiness check can miss a notification because it
happens before the observer is registered; to fix, register the
NotificationCenter observer for .terminalSurfaceDidBecomeReady on panel.surface
first (using the observer variable and testCase.expectation(description:
"terminal surface ready \(panel.id)")), then immediately check
panel.surface.surface and, if already non-nil, fulfill the expectation
(ready.fulfill()), otherwise wait(for: [ready], timeout: 5.0) and finally call
drainMainQueue(); ensure the defer block still removes the observer if set.
In `@Sources/Workspace.swift`:
- Around line 6568-6578: The rooted font cache
(terminalInheritanceFontPointsByPanelId) can be stale after a zoom; update the
logic so splits see the live runtime font: either invoke
rememberTerminalConfigInheritanceSource() immediately after successful
zoom/zoom-binding code paths (so panelWasSelected isn't the only place that sets
a rooted value), or change
resolvedTerminalInheritanceFontPoints(for:inheritedConfig:) to compare the
cached rooted value against the current live runtime font (queried via
inheritedTerminalConfig() or the panel's live config) and return the
newer/non-zero value; also ensure when selecting the winner you update
lastTerminalConfigInheritanceFontPoints consistently so future splits aren't
poisoned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffbdca45-34ce-42f7-8b6d-b776140fa619
📒 Files selected for processing (3)
Sources/TabManager.swiftSources/Workspace.swiftcmuxTests/WorkspaceUnitTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:6693">
P2: Split-created terminals may inherit stale zoom values when live terminal zoom changed after lineage was last recorded. The removed runtime font sampling in `cmuxInheritedSurfaceConfig()` and divergence detection in `resolvedTerminalInheritanceFontPoints()` means splits no longer get current visible zoom if the user zooms without a focus transition.</violation>
</file>
<file name="cmuxTests/WorkspaceUnitTests.swift">
<violation number="1" location="cmuxTests/WorkspaceUnitTests.swift:1103">
P2: Race condition in test helper: check-then-subscribe pattern can miss notification if surface becomes ready during the gap between surface nil-check and observer registration</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Follow-up pushed in Changes:
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86224014c5
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
6568-6582:⚠️ Potential issue | 🟠 MajorPrefer the cached lineage font when both values exist.
Lines 6573-6575 let
inheritedConfig.font_sizeoverrideterminalInheritanceFontPointsByPanelIdwhenever they disagree. That flips the intended “prefer remembered inheritance font points” rule and can still regresszoom → splitif Ghostty’s inherited config is carrying the pre-zoom size.inheritedConfig.font_sizeshould only fill the gap when the panel has no remembered value.🔧 Suggested fix
private func resolvedTerminalInheritanceFontPoints( for terminalPanel: TerminalPanel, inheritedConfig: ghostty_surface_config_s ) -> Float? { if let rooted = terminalInheritanceFontPointsByPanelId[terminalPanel.id], rooted > 0 { - if inheritedConfig.font_size > 0, - abs(inheritedConfig.font_size - rooted) > 0.05 { - return inheritedConfig.font_size - } return rooted } if inheritedConfig.font_size > 0 { return inheritedConfig.font_size } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 6568 - 6582, The function resolvedTerminalInheritanceFontPoints currently lets inheritedConfig.font_size override the cached value in terminalInheritanceFontPointsByPanelId when they differ; change the logic in resolvedTerminalInheritanceFontPoints(for:inheritedConfig:) so that if a cached value exists for terminalPanel.id (rooted) and rooted > 0 it is always returned (regardless of inheritedConfig.font_size), and only when there is no cached value should you return inheritedConfig.font_size if > 0; keep returning nil otherwise.
🧹 Nitpick comments (1)
cmuxTests/WorkspaceUnitTests.swift (1)
1128-1128: Consider consolidatingdrainMainQueue()helpers.
WorkspacePanelGitBranchTests(line 1449) defines a privatedrainMainQueue(), while this class relies on a presumed global definition. If a shared test-utility version exists, consider using it consistently to avoid duplication and divergent implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` at line 1128, The test calls and defines duplicate drainMainQueue() helpers across tests (this file and WorkspacePanelGitBranchTests); remove the local private drainMainQueue() from WorkspacePanelGitBranchTests (or this class if it defines it locally) and replace all calls with the shared test-utility implementation, or if the shared helper is internal/private to another test target, move/restore it into a common test helpers file and adjust access control so both WorkspacePanelGitBranchTests and this test class call the same consolidated drainMainQueue() function; ensure you remove the duplicate helper definition and update imports/visibility so all tests reference the single shared function.
🤖 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/Workspace.swift`:
- Around line 6697-6700: The code updates lastTerminalConfigInheritancePanelId
regardless of config.font_size but only writes
lastTerminalConfigInheritanceFontPoints when font_size > 0, causing inconsistent
snapshots; update the code so that whenever you set
lastTerminalConfigInheritancePanelId you also set or clear
lastTerminalConfigInheritanceFontPoints based on config.font_size (i.e., assign
the positive point value when config.font_size > 0, otherwise explicitly
clear/unset it), keeping lastTerminalConfigInheritancePanelId and
lastTerminalConfigInheritanceFontPoints consistent.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 6568-6582: The function resolvedTerminalInheritanceFontPoints
currently lets inheritedConfig.font_size override the cached value in
terminalInheritanceFontPointsByPanelId when they differ; change the logic in
resolvedTerminalInheritanceFontPoints(for:inheritedConfig:) so that if a cached
value exists for terminalPanel.id (rooted) and rooted > 0 it is always returned
(regardless of inheritedConfig.font_size), and only when there is no cached
value should you return inheritedConfig.font_size if > 0; keep returning nil
otherwise.
---
Nitpick comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Line 1128: The test calls and defines duplicate drainMainQueue() helpers
across tests (this file and WorkspacePanelGitBranchTests); remove the local
private drainMainQueue() from WorkspacePanelGitBranchTests (or this class if it
defines it locally) and replace all calls with the shared test-utility
implementation, or if the shared helper is internal/private to another test
target, move/restore it into a common test helpers file and adjust access
control so both WorkspacePanelGitBranchTests and this test class call the same
consolidated drainMainQueue() function; ensure you remove the duplicate helper
definition and update imports/visibility so all tests reference the single
shared function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64b8f601-4a6a-41a7-89c7-32c57b7719cd
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/WorkspaceUnitTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 676c573012
ℹ️ 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".
676c573 to
f3b1fad
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
7265-7268:⚠️ Potential issue | 🟠 MajorKeep the remembered source/font snapshot in sync.
Line 7265 always advances
lastTerminalConfigInheritancePanelId, but Line 7266 only updateslastTerminalConfigInheritanceFontPointswhenconfig.font_size > 0. If this source resolves to an unset font size, later fallback paths can still reuse stale points from an older panel instead of leavingfont_sizeunset.🔧 Suggested fix
lastTerminalConfigInheritancePanelId = terminalPanel.id - if config.font_size > 0 { - lastTerminalConfigInheritanceFontPoints = config.font_size - } + lastTerminalConfigInheritanceFontPoints = + config.font_size > 0 ? config.font_size : nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 7265 - 7268, When advancing the remembered terminal source snapshot, keep the panel id and font snapshot in sync: currently lastTerminalConfigInheritancePanelId is updated unconditionally while lastTerminalConfigInheritanceFontPoints is only set when config.font_size > 0, which can leave stale points behind; modify the update so that whenever lastTerminalConfigInheritancePanelId is assigned you also explicitly clear or set lastTerminalConfigInheritanceFontPoints based on config.font_size (e.g. set to the new points when >0, otherwise clear it) so the remembered snapshot reflects an unset font size correctly (references: lastTerminalConfigInheritancePanelId, lastTerminalConfigInheritanceFontPoints, config.font_size).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 7265-7268: When advancing the remembered terminal source snapshot,
keep the panel id and font snapshot in sync: currently
lastTerminalConfigInheritancePanelId is updated unconditionally while
lastTerminalConfigInheritanceFontPoints is only set when config.font_size > 0,
which can leave stale points behind; modify the update so that whenever
lastTerminalConfigInheritancePanelId is assigned you also explicitly clear or
set lastTerminalConfigInheritanceFontPoints based on config.font_size (e.g. set
to the new points when >0, otherwise clear it) so the remembered snapshot
reflects an unset font size correctly (references:
lastTerminalConfigInheritancePanelId, lastTerminalConfigInheritanceFontPoints,
config.font_size).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b77aa68-2149-4117-b15f-1c449de02619
📒 Files selected for processing (4)
Sources/TabManager.swiftSources/Workspace.swiftcmuxTests/TabManagerUnitTests.swiftcmuxTests/WorkspaceUnitTests.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/TabManager.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/WorkspaceUnitTests.swift
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:7154">
P1: Split/new terminal inheritance now re-enters unsafe runtime font probing (`cmuxCurrentSurfaceFontSizePoints`), which can dereference stale unretained CTFont pointers and reintroduce crash risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return rooted | ||
| if terminalPanel.surface.hasLiveSurface, | ||
| let sourceSurface = terminalPanel.surface.surface, | ||
| let runtimePoints = cmuxCurrentSurfaceFontSizePoints(sourceSurface) { |
There was a problem hiding this comment.
P1: Split/new terminal inheritance now re-enters unsafe runtime font probing (cmuxCurrentSurfaceFontSizePoints), which can dereference stale unretained CTFont pointers and reintroduce crash risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 7154:
<comment>Split/new terminal inheritance now re-enters unsafe runtime font probing (`cmuxCurrentSurfaceFontSizePoints`), which can dereference stale unretained CTFont pointers and reintroduce crash risk.</comment>
<file context>
@@ -7135,16 +7149,17 @@ final class Workspace: Identifiable, ObservableObject {
- return rooted
+ if terminalPanel.surface.hasLiveSurface,
+ let sourceSurface = terminalPanel.surface.surface,
+ let runtimePoints = cmuxCurrentSurfaceFontSizePoints(sourceSurface) {
+ return runtimePoints
}
</file context>
|
Pushed follow-up in Changes:
Verification:
|
|
Branch updated again after rebasing onto the latest remote tip; the fix is now at This keeps the split inheritance path off the unsafe runtime font probe and keeps the remembered inheritance panel/font snapshot synchronized. Verification:
|
1 participant
Summary
Fixes #1870.
Test Plan
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-1870-green-final -only-testing:cmuxTests/WorkspaceTerminalZoomInheritanceTests -only-testing:cmuxTests/WorkspaceTerminalConfigInheritanceSelectionTests test./scripts/reload.sh --tag issue-1870-repro3, thenView > Split Right,Split Right,Split Down; debug log showedsplit.shortcut,split.didSplit,split.created, andsurface.attach.create.doneNotes
Summary by cubic
Fixes the split-time crash (#1870) and stale zoom inheritance for splits and new workspaces. Inheritance now uses the source’s inherited config, falling back to the live zoom when
font_sizeis unset, then to remembered zoom.cmuxInheritedSurfaceConfigno longer probes runtime font size; added a debug-only override for tests.font_sizeis missing; otherwise use remembered zoom. Added regression tests for crash coverage, no-refocus, stale-remembered, and inherited-font-size-unset scenarios.Written for commit 7c61480. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests