Repository navigation
Fix QuickLook preview crash on deactivated QLPreviewView - #6402
azooz2003-bit merged 5 commits into
Conversation
QLPreviewView aborts the process when a non-nil preview item is assigned
after AppKit has deactivated the view (it leaves the window hierarchy):
[QL] -[QLPreviewView setPreviewItem:blockingUntilLoading:timeoutDate:transition:]:
item == nil || _reserved->internalState != QLPreviewDeactivatedInternalState
SwiftUI keeps the representable's NSView mounted across tab switches,
visibility toggles, and panel reuse, then re-runs configure() ->
previewView.previewItem = ... on a view AppKit already deactivated. This
is the still-recurring crash from manaflow-ai#4453: dropping close() and adding
dismantleNSView (its fix directions manaflow-ai#2/manaflow-ai#3) prevented cmux-initiated reuse
but not system-initiated deactivation, so the abort still fires on
macOS 26 (Tahoe).
Implements fix direction manaflow-ai#1: host the QLPreviewView inside a stable
container view (matching the PDF/image session pattern) and swap in a
fresh preview view once the previous instance has detached from its
window, so a non-nil item is never assigned to a deactivated view.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Fu21KvnjR3dAe6sjKaVUw
|
@thiveeiyan is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughIntroduces QuickLook Container and Session Wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (22 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 SummaryFixes the recurring QuickLook abort (#4453) by wrapping the fragile
Confidence Score: 5/5The crash fix is sound — a non-nil preview item is never assigned to a deactivated QLPreviewView on any code path through the new container. The core lifecycle invariant is correctly maintained: livePreviewView() always returns either an existing non-deactivated TrackedQLPreviewView or a freshly created one, and the stale view's item is cleared (nil assignment passes QL's own assertion guard) before it's removed. The monotonic didDetachFromWindow flag correctly models QL's irreversible deactivation. The only concern is that the container itself inherits from QLPreviewView when NSView would be the cleaner and safer base class, but this does not affect correctness of the fix as written. FilePreviewQuickLookContainerView.swift — the container's QLPreviewView base class is unnecessary and adds QL lifecycle overhead to the stable host view. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant SwiftUI
participant Container as FilePreviewQuickLookContainerView
participant Tracked as TrackedQLPreviewView
participant QL as QuickLook (QLPreviewView internals)
SwiftUI->>Container: makeNSView → make()
Container->>Tracked: livePreviewView() creates fresh inner view
Tracked-->>Container: addSubview(fresh)
Container-->>SwiftUI: return container
Note over Container,Tracked: Tab switch / window detach
QL->>Tracked: "viewDidMoveToWindow() [window == nil]"
Tracked->>Tracked: "didDetachFromWindow = true (monotonic)"
SwiftUI->>Container: updateNSView → configure()
Container->>Container: livePreviewView()
Container->>Tracked: "stale.previewItem = nil (safe: nil branch)"
Container->>Tracked: stale.removeFromSuperview()
Container->>Tracked: create fresh TrackedQLPreviewView
Tracked-->>Container: addSubview(fresh)
Container->>Tracked: "fresh.previewItem = previewItem (safe: fresh, active)"
%%{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 SwiftUI
participant Container as FilePreviewQuickLookContainerView
participant Tracked as TrackedQLPreviewView
participant QL as QuickLook (QLPreviewView internals)
SwiftUI->>Container: makeNSView → make()
Container->>Tracked: livePreviewView() creates fresh inner view
Tracked-->>Container: addSubview(fresh)
Container-->>SwiftUI: return container
Note over Container,Tracked: Tab switch / window detach
QL->>Tracked: "viewDidMoveToWindow() [window == nil]"
Tracked->>Tracked: "didDetachFromWindow = true (monotonic)"
SwiftUI->>Container: updateNSView → configure()
Container->>Container: livePreviewView()
Container->>Tracked: "stale.previewItem = nil (safe: nil branch)"
Container->>Tracked: stale.removeFromSuperview()
Container->>Tracked: create fresh TrackedQLPreviewView
Tracked-->>Container: addSubview(fresh)
Container->>Tracked: "fresh.previewItem = previewItem (safe: fresh, active)"
Reviews (5): 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}
| func livePreviewView() -> QLPreviewView? { | ||
| if let previewView, !previewView.didDetachFromWindow { | ||
| return previewView | ||
| } | ||
|
|
||
| // Retire a deactivated instance before mounting a fresh one. Assigning | ||
| // `nil` is always safe (the assertion's `item == nil` branch holds). | ||
| if let stale = previewView { | ||
| stale.previewItem = nil | ||
| stale.removeFromSuperview() | ||
| } | ||
| previewView = nil | ||
|
|
||
| guard let fresh = TrackedQLPreviewView(frame: bounds, style: .normal) else { | ||
| return nil | ||
| } | ||
| fresh.autostarts = true | ||
| fresh.autoresizingMask = [.width, .height] | ||
| addSubview(fresh) | ||
| previewView = fresh | ||
| return fresh | ||
| } |
There was a problem hiding this comment.
clearPreviewItem() leaves a deactivated inner view as a subview of the container
clearPreviewItem() reads previewView?.previewItem = nil directly. If a deactivated view is the current previewView at the time releaseView() is called (i.e., the detach happened but configure() hasn't run yet to replace it), the container's previewView property still points to the stale TrackedQLPreviewView. clearPreviewItem() safely sets previewItem = nil on it (nil assignment passes the QL assertion), but the stale view remains as a subview of the container until ARC deallocates the container.
This is harmless for the current lifecycle (container is removed from its superview immediately after by view.removeFromSuperview()), but a removeFromSuperview() call on the stale inner view inside clearPreviewItem() would make the cleanup explicit and match the retirement logic in livePreviewView().
| private final class TrackedQLPreviewView: QLPreviewView { | ||
| private(set) var didDetachFromWindow = false | ||
|
|
||
| override func viewDidMoveToWindow() { | ||
| super.viewDidMoveToWindow() | ||
| // `viewDidMoveToWindow` fires both on attach (window != nil) and detach | ||
| // (window == nil). Only the detach transition deactivates the view. | ||
| if window == nil { | ||
| didDetachFromWindow = true | ||
| } | ||
| } |
There was a problem hiding this comment.
didDetachFromWindow is monotonic — re-attaching to a window does not reset it
This is intentional and correctly documented: once QL deactivates a view, the deactivated internal state is permanent, so the flag should never flip back. Worth making this invariant explicit in the doc comment (e.g., "once set, this flag is never cleared; a deactivated view cannot be reactivated") so future maintainers don't add a didDetachFromWindow = false reset in the attach branch of viewDidMoveToWindow, which would silently re-introduce the crash.
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!
|
Maintainer verification pass:
Conclusion: the fix matches the Sentry crash path and prevents the concrete AppKit assertion repro. Remaining PR blocker is external to this crash fix: fork Vercel deploy authorization is required for |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ated-preview-crash
…ated-preview-crash
This branch was successfully deployed
2 participants
Summary
Fixes a fatal QuickLook abort when previewing files.
QLPreviewViewcallsabort()if a non-nilpreviewItemis assigned after AppKit has deactivated the view (i.e. after it has left the window hierarchy):This is the still-recurring crash tracked in #4453.
Root cause
FilePreviewQuickLookSessionvends a bareQLPreviewViewstraight to the SwiftUI representable. SwiftUI keeps thatNSViewmounted across tab switches, visibility toggles, and panel reuse, and hands the same instance back toupdateNSView. AppKit deactivates aQLPreviewViewwhenever it leaves its window — routine in a tabbed terminal — and the nextconfigure()runspreviewView.previewItem = …on the now-deactivated view, tripping the assertion and aborting the whole app.#4453's fix directions #2 (drop
close()) and #3 (adddismantleNSView) shipped, and they stop cmux-initiated reuse of a closed view. But they don't cover system-initiated deactivation (the window detach AppKit does on its own), so the abort still fires. This reproduces on macOS 26 (Tahoe) and the QuickLook code is identical across 0.64.14 → 0.64.16 →main.Evidence
Decoded the minidumps from a user's 35
.ghosttycrashreports — every one is this crash, in two forms of the same QuickLook abort:QuickLookPreviewView.make/updateNSView→QLPreviewView setPreviewItem:blockingUntilLoading:…→_QLRaiseAssert→_QLCrash→abort()_QLDumpMachPortRights(QuickLook's crash handler dumping mach-port rights before the sameabort())Dates run from May through today, on the latest builds.
Fix
Implements #4453's remaining fix direction #1: host the fragile
QLPreviewViewinside a stable container view (the same pattern already used by the PDF/image sessions) and swap in a fresh preview view once the previous instance has detached from its window. A non-nil preview item is therefore never assigned to a deactivated view, while SwiftUI never has to re-mount the representable. A smallQLPreviewViewsubclass records the window-detach transition since the deactivated state has no public accessor.Testing
swiftc -parsepasses and the new AppKit/Quartz helper classes type-check standalone. I was not able to run a full app build (developed against the crash reports on a machine with only Command Line Tools, which can't parse the macOS 26.5 SDK) — please run it through CI / a local Xcode build before merging. Manual repro to verify: open a QuickLook-previewed file, switch tabs/windows away and back (or close the pane) so the view detaches and re-attaches, which previously aborted.Refs #4453.
🤖 Generated with Claude Code
https://claude.ai/code/session_011Fu21KvnjR3dAe6sjKaVUw
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a Quick Look crash by hosting
QLPreviewViewin a stable container and swapping in a fresh view after window detaches. Stops the abort when switching tabs or panes, resolving #4453.FilePreviewQuickLookContainerViewthat vends a safe, liveQLPreviewViewand recreates it after deactivation.TrackedQLPreviewViewto detect window detaches and avoid assigning a non-nilpreviewItemto a deactivated view.close()).QLPreviewViews and validate the container-based setup.Written for commit 0818a42. Summary will update on new commits.
Summary by CodeRabbit