Repository navigation
Fix File Preview hang when drag-selecting large files (#4576) - #4962
Conversation
File Preview hosts files up to 16 MB in a plain TextKit 2 NSTextView. TextKit 2's NSTextSelectionNavigation hit-tests are O(N) in line-fragment count, so drag-selecting deep into a large file pegs the main thread (#4576). A real hang report on cmux 0.64.9 traced to opening a large JSON file in File Preview. Extract the text-view configuration into SavingTextView.makeFilePreviewTextView() so production and the test share one path, then add a regression test that times selection hit-testing near the bottom of a 60k-line file. With the current TextKit 2 configuration this takes several seconds and fails the 1.0s ceiling; the next commit makes it pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ous layout Drop the File Preview text view to TextKit 1 and enable non-contiguous layout. TextKit 2's NSTextSelectionNavigation hit-tests are O(N) in line-fragment count, so drag-selecting deep into a large file pegged the main thread (#4576). TextKit 1 hit-testing is ~O(log N), and non-contiguous layout keeps display lazy so large files still open instantly. Validated with headless benchmarks (characterIndexForInsertion at the bottom of a file): 60k lines 7936ms -> 6ms, 200k lines 43327ms -> 18ms. An earlier TextKit 1 attempt regressed because it omitted allowsNonContiguousLayout, forcing eager full layout; enabling it is what keeps both selection and display fast. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (2)
📝 WalkthroughWalkthroughFilePreviewTextEditor now obtains a preconfigured SavingTextView from SavingTextView.makeFilePreviewTextView(), which enables non-contiguous layout on the layout manager before textContainer access; a regression test verifies hit-testing stays responsive on ~60k-line documents. ChangesLarge Document Selection Performance Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryFixes the drag-selection hang in File Preview by switching
Confidence Score: 5/5The change is safe to merge: the factory pattern guarantees call order, the production path is straightforward, and the regression test provides a clean before/after signal well-separated from both TextKit regimes. The fix is targeted — accessing layoutManager before any TextKit 2 property is the documented, correct way to force TextKit 1 compatibility mode on macOS 12+, and the factory method makes that ordering invariant hard to violate accidentally. No new actor isolation, blocking primitives, or complexity regressions are introduced. The moved configuration is identical to what was inline before, so display behavior is unchanged. No files require special attention; both changed files are self-contained and straightforward. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["makeFilePreviewTextView()"] --> B["SavingTextView()"]
B --> C["enableLargeDocumentSelectionPerformance()\naccess layoutManager → forces TextKit 1"]
C --> D{layoutManager\nnon-nil?}
D -- Yes --> E["allowsNonContiguousLayout = true\nLazy glyph layout enabled"]
D -- No\n(TK2 already locked) --> F["Silent no-op\n(misuse: TK2 already accessed)"]
E --> G["Configure textContainer,\nresizing, font, insets"]
G --> H["Return SavingTextView\n(TextKit 1, non-contiguous)"]
H --> I["makeNSView: set drawsBackground,\nstring, delegate, attach to scroll view"]
Reviews (2): Last reviewed commit: "Harden TextKit 1 opt-in and selection re..." | Re-trigger Greptile |
| func enableLargeDocumentSelectionPerformance() { | ||
| layoutManager?.allowsNonContiguousLayout = true | ||
| } |
There was a problem hiding this comment.
Silent no-op leaves the bug unfixed
layoutManager?.allowsNonContiguousLayout = true uses optional chaining. The method's own docstring says that if any TextKit 2 property (textLayoutManager, textContainer, etc.) is accessed before this call, layoutManager returns nil — and the optional silently short-circuits without setting allowsNonContiguousLayout. That means the view stays in TextKit 2, the entire performance fix silently doesn't apply, and the hang from #4576 persists with no indication. An assert(layoutManager != nil, "layoutManager is nil; enableLargeDocumentSelectionPerformance must be called before any TextKit 2 access") (or even a non-optional dereference) would catch this ordering violation at the call site instead of silently regressing.
There was a problem hiding this comment.
Fixed in the latest commit: replaced the optional chaining with guard let + assertionFailure, so a call-order violation now fails loudly in DEBUG while release behavior stays a no-op.
— Claude Code
| textView.sizeToFit() | ||
|
|
||
| let bottomY = max(textView.bounds.height - 5, 1) | ||
| let start = ProcessInfo.processInfo.systemUptime | ||
| for offset in 0..<20 { | ||
| _ = textView.characterIndexForInsertion(at: CGPoint(x: 200, y: bottomY - CGFloat(offset))) | ||
| } |
There was a problem hiding this comment.
Headless
sizeToFit() may yield zero bounds, making the test a false negative
The text view has no window and no initial frame. With allowsNonContiguousLayout = true, NSLayoutManager performs lazy on-demand layout and may not realize the full document during sizeToFit(). If textView.bounds.height remains 0 (or near-zero), bottomY clamps to 1, and all 20 characterIndexForInsertion calls land near y ≤ 1 — the very top of the document, where TextKit 2 is also O(1). The test would then always pass regardless of whether the TextKit 1 downgrade succeeded, turning the regression guard into a no-op. A safer approach would call layoutManager?.ensureLayout(forCharacterRange: NSRange(location: 0, length: textView.textStorage?.length ?? 0)) before checking bounds.height, or assert bounds.height > 1000 as a precondition so the test fails fast when the geometry precondition isn't met.
There was a problem hiding this comment.
Fixed: added a geometry precondition (XCTAssertGreaterThan(bounds.height, 100_000)) so the test fails loudly if headless layout collapses instead of silently passing. Deliberately not realizing layout via layoutManager/ensureLayout, since accessing it would force a TextKit mode in the test view and stop the test from ever catching a regression back to TextKit 2. sizeToFit is mode-neutral and empirically realizes full layout (~820k pt for 60k lines).
— Claude Code
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/Panels/FilePreviewTextEditor.swift`:
- Around line 158-160: enableLargeDocumentSelectionPerformance currently
silently no-ops when layoutManager is nil; change its implementation to guard
that layoutManager is non-nil, call allowsNonContiguousLayout = true on the
unwrapped layoutManager, and call assertionFailure (or similar) if nil so DEBUG
builds fail loudly while keeping release behavior unchanged; update the function
enableLargeDocumentSelectionPerformance and reference layoutManager and its
allowsNonContiguousLayout property when making this change.
🪄 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: 007ff713-6b35-4d06-ba4d-6b761509d54f
📒 Files selected for processing (2)
Sources/Panels/FilePreviewTextEditor.swiftcmuxTests/FilePreviewReviewFeedbackTests.swift
Address CodeRabbit and Greptile findings on #4962: - enableLargeDocumentSelectionPerformance() previously used optional chaining (layoutManager?...), which would silently no-op and reintroduce the hang if it ever ran after a TextKit 2 access. Replace with guard let + assertionFailure so the call-order violation fails loudly in DEBUG; release behavior is unchanged. - The regression test could be a false negative if headless sizeToFit left bounds.height near zero, putting all hit-tests at the cheap top of the doc. Add a geometry precondition (bounds.height > 100_000) so it fails loudly instead of passing trivially. Layout is realized via mode-neutral sizeToFit, not layoutManager access, which would force a TextKit mode and defeat the regression signal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes #4576.
A crash/hang report on cmux 0.64.9 traced to opening a large JSON file in File Preview and drag-selecting. The main thread pegged inside TextKit 2's
NSTextSelectionNavigation.textSelectionsInteractingAtPoint, which is O(N) in line-fragment count per hit-test, so each mouse-drag event re-walks fragments from the document start.File Preview (
SavingTextView) hosts files up to 16 MB in a plain TextKit 2NSTextView. The fix drops that view to TextKit 1 and enablesallowsNonContiguousLayout: TextKit 1 mouse hit-testing is ~O(log N) and non-contiguous layout keeps display lazy, so large files still open instantly.The #4576 comment noted an earlier TextKit 1 attempt regressed. That attempt omitted
allowsNonContiguousLayout, which forces eager full layout. Enabling it is what keeps both selection and display fast.Validated with a headless benchmark timing
characterIndexForInsertion(at:)near the bottom of a file (the realistic "scroll down, select a few lines" case):Open-file cost stays bounded (one-time, not size-dependent) and first-viewport layout stays instant, so there is no display regression.
Two-commit structure per repo policy: commit 1 adds the failing regression test (config refactored into a shared
SavingTextView.makeFilePreviewTextView()so production and test share one path), commit 2 adds the fix. The test times bottom-of-file selection hit-tests against a 1.0s ceiling that sits far from both the TextKit 2 (seconds) and TextKit 1 (milliseconds) costs, so it is a clean, non-flaky signal. It lives in the already-wiredFilePreviewReviewFeedbackTests, so thecmux-unitCI suite runs it.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a hang when drag-selecting deep into large files in File Preview. Switches to TextKit 1 with non-contiguous layout so selection stays responsive, and hardens the config and tests to prevent regressions (fixes #4576).
allowsNonContiguousLayout = trueto avoid TextKit 2’s O(N) selection hit-tests.SavingTextView.makeFilePreviewTextView()and requireenableLargeDocumentSelectionPerformance()before any TextKit 2 access (DEBUG assertion if violated).Written for commit 6364c21. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Release Notes
Performance
Tests