Repository navigation
Fix stale Metal drawable after terminal layer realization - #6057
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 bounded retry state and scheduling in GhosttyNSView for deferred surface sizing when the backing layer is not a CAMetalLayer, plus a unit test (and project wiring) that verifies drawable-size reconciliation after a non‑Metal refresh followed by Metal layer realization. ChangesDrawable-size retry validation
Sequence Diagram(s)sequenceDiagram
participant Test as GhosttyDrawableSizeRetryTests
participant NSWindow
participant TerminalSurface
participant GhosttyNSView
participant GhosttyMetalLayer
participant RunLoop
Test->>NSWindow: create and show
Test->>TerminalSurface: attach view to window
TerminalSurface->>GhosttyNSView: reconcile geometry, compute expected backing drawable size
Test->>GhosttyNSView: swap to non-Metal CALayer and force refresh
GhosttyNSView->>RunLoop: schedule deferred retry
Test->>GhosttyNSView: install GhosttyMetalLayer with stale drawableSize
RunLoop->>GhosttyMetalLayer: deferred tick -> layer realizes
GhosttyMetalLayer->>GhosttyNSView: drawableSize updated -> test asserts convergence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ 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 |
8d46b04 to
d06295c
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: 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 `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 6606-6608: Replace the one-shot RunLoop.current.run(until:
Date().addingTimeInterval(0.05)) with a bounded poll that repeatedly advances
the run loop in short increments (e.g., 0.01s) until realizedLayer.drawableSize
== expectedDrawableSize or a hard timeout elapses (e.g., 1.0s); keep the final
assertion (`#expect`(realizedLayer.drawableSize == expectedDrawableSize)) after
the loop and, if you prefer, fail the test explicitly on timeout (XCTFail or
equivalent). Use the existing realizedLayer.drawableSize and
expectedDrawableSize checks inside the loop and RunLoop.current.run(until:) for
the short sleeps so the test is deterministic on slower CI.
🪄 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: 7c7257e9-c9a9-4feb-b114-1f16b3d4ebb7
📒 Files selected for processing (1)
cmuxTests/GhosttyConfigTests.swift
Greptile SummaryThis PR fixes a race where a stale
Confidence Score: 3/5The fix correctly bounds and coalesces retries, but the rescue path in The production path now has two entry points to Sources/GhosttyTerminalView.swift — specifically Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit
participant GhosttyNSView
participant updateSurfaceSize
participant ScheduledTask
Note over GhosttyNSView: Resize arrives while layer is non-Metal
GhosttyNSView->>updateSurfaceSize: updateSurfaceSize(size:)
updateSurfaceSize->>updateSurfaceSize: "layer is not CAMetalLayer, count < 8"
updateSurfaceSize->>ScheduledTask: scheduleDeferredSurfaceSizeRetryIfNeeded() → true
updateSurfaceSize->>GhosttyNSView: "needsSurfaceSizeRetryAfterMetalLayerRealizes = true, count += 1"
Note over AppKit: AppKit calls makeBackingLayer()
AppKit->>GhosttyNSView: makeBackingLayer()
GhosttyNSView->>ScheduledTask: Task MainActor → reconcileSurfaceSizeAfterMetalLayerAttachIfNeeded()
AppKit->>GhosttyNSView: "self.layer = newMetalLayer (synchronous, before run loop yields)"
Note over ScheduledTask: Next main-actor turn
ScheduledTask->>GhosttyNSView: reconcileSurfaceSizeAfterMetalLayerAttachIfNeeded()
GhosttyNSView->>GhosttyNSView: guard needsSurfaceSizeRetryAfterMetalLayerRealizes passes
GhosttyNSView->>GhosttyNSView: "deferredSurfaceSizeNonMetalRetryCount = 0"
GhosttyNSView->>updateSurfaceSize: updateSurfaceSize()
updateSurfaceSize->>updateSurfaceSize: "layer is CAMetalLayer → update drawableSize, flag = false"
Reviews (10): Last reviewed commit: "merge: resolve conflicts with main" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| RunLoop.current.run(until: Date().addingTimeInterval(0.05)) | ||
|
|
||
| #expect(realizedLayer.drawableSize == expectedDrawableSize) |
There was a problem hiding this comment.
RunLoop.current.run(until: Date().addingTimeInterval(0.05)) ties the pass/fail boundary to 50 ms of real time. Any production fix that lands a DispatchQueue.main.async callback will reliably beat that budget under normal conditions, but a second hop (e.g. an async dispatch that itself enqueues another dispatch, a deferred retry with a timer, or a KVO cascade that re-schedules) can miss the window on a loaded runner and produce a spurious failure. Since the test is @MainActor and already running on the main thread, a more robust approach is to directly observe realizedLayer.drawableSize via KVO or a notification until it matches, with a generous but bounded timeout — eliminating any false-negative risk from CI load while still protecting against infinite spin.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Already addressed in the current branch: GhosttyDrawableSizeRetryTests uses condition-based bounded polling for drawable reconciliation, so the fixed 50 ms delay called out here is no longer present. This thread is outdated.
— Claude Code
There was a problem hiding this comment.
Already addressed in the current test: the fixed 50ms drain was replaced with a condition-based loop that advances the run loop until the drawable matches or a bounded deadline is reached.
— Claude Code
There was a problem hiding this comment.
Current HEAD no longer uses the fixed 50ms drain. The regression test waits on realizedLayer.drawableSize until it matches the expected backing size with a bounded run-loop pump, and CI is green on that path.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/GhosttyDrawableSizeRetryTests.swift`:
- Around line 65-74: Add an explicit assertion to verify the intermediate stale
drawable size before the retry runs: after installing the Metal layer (after
surfaceView.layer = realizedLayer) assert that realizedLayer.drawableSize ==
staleDrawableSize so the test verifies the before state; keep the existing final
assertion that realizedLayer.drawableSize == expectedDrawableSize to confirm the
retry corrected it. Use the existing GhosttyMetalLayer instance (realizedLayer)
and the staleDrawableSize/expectedDrawableSize variables.
- Line 72: Replace the hardcoded RunLoop.current.run(until:
Date().addingTimeInterval(0.05)) drain in GhosttyDrawableSizeRetryTests with a
condition-based wait: poll realizedLayer.drawableSize against
expectedDrawableSize until a deadline (e.g., Date().addingTimeInterval(0.5)),
calling short RunLoop runs (or sleeping briefly) inside the loop to let the
main-queue reconciliation run, and then assert equality after the loop (fail if
deadline reached). Refer to realizedLayer.drawableSize and expectedDrawableSize
when implementing the loop condition.
🪄 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: 9cb32ebf-3a1c-4d12-8cf5-3292241206fd
📒 Files selected for processing (2)
cmux.xcodeproj/project.pbxprojcmuxTests/GhosttyDrawableSizeRetryTests.swift
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/GhosttyTerminalView.swift`:
- Around line 8422-8435: The non‑Metal branch increments
deferredSurfaceSizeNonMetalRetryCount before calling
scheduleDeferredSurfaceSizeRetryIfNeeded(), which can waste retries when the
scheduler coalesces via deferredSurfaceSizeRetryQueued; change the logic so the
counter is only incremented when a retry is actually enqueued: either move the
increment into the protected section inside
scheduleDeferredSurfaceSizeRetryIfNeeded() immediately after
deferredSurfaceSizeRetryQueued is set, or modify
scheduleDeferredSurfaceSizeRetryIfNeeded() to return Bool (true if it actually
enqueued) and only increment deferredSurfaceSizeNonMetalRetryCount when that
Bool is true; update usages accordingly to reference
deferredSurfaceSizeNonMetalRetryCount, deferredSurfaceSizeRetryQueued and
scheduleDeferredSurfaceSizeRetryIfNeeded().
🪄 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: 6c49bb4c-1498-402d-ba81-955dc7a543da
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/GhosttyDrawableSizeRetryTests.swift
|
Addressed Greptile's dual-path drawable reconciliation concern in 574040b by removing the extra GhosttyMetalLayer.setSurfaceView Task. The fix now has one owner for this path: the bounded updateSurfaceSize retry, with the retry counter only incrementing when a retry is actually enqueued. |
|
Addressed Greptile's retry-exhaustion concern in 4e3e61e. updateSurfaceSize now records a pending Metal-layer reconciliation when it encounters a non-Metal backing layer, and setSurfaceView only invokes the guarded reconcile when that pending flag exists, so late Metal realization has an event-driven recovery without unbounded polling. |
|
Addressed Greptile's fallback edge case in 2e51813. The layer-attach fallback now resets the non-Metal retry counter before calling the guarded reconcile, so if AppKit still reports a non-Metal layer at that moment, the retry path can re-arm instead of dead-ending after an exhausted budget. |
This branch was successfully deployed
1 participant
Closes #6041
Summary
GhosttyNSView.layeris a plainCALayer, then installs a staleGhosttyMetalLayerand waits fordrawableSizeto reconcile.GhosttyNSView.updateSurfaceSize(size:)so a positive-size, in-window resize against a non-Metal backing layer schedules the existing deferred resize retry. The retry is bounded, coalesced, and resets once a Metal layer is observed.GhosttyMetalLayer.setSurfaceView(_:)triggers the same guarded reconcile once the layer attaches.Reproduction
/Users/austinwang/manaflow/term/cmux2-repro-6041on unfixed code.cmux-unitscheme with isolated DerivedData andCMUX_SKIP_ZIG_BUILD=1; it did not launch the app or touch the default socket.GhosttyDrawableSizeRetryTests/reconcilesDrawableAfterFullSizeUpdateRunsBeforeMetalLayerRealizes()failed because no deferred retry was queued and the realized Metal layer remained stale:debugDeferredSurfaceSizeRetryQueuedForTesting()wasfalserealizedLayer.drawableSizestayed(1600.0, 1200.0)instead of reconciling to the full backing size(2592.0, 1786.0)Testing
git diff --checkpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv./scripts/lint-pbxproj-test-wiring.sh./scripts/check-pbxproj.shDemo Video
Localization
Checklist
manaflow-ai/cmuxorigin