Skip to content

fix(macos): coalesce ComposerTextEditor.measureHeight and skip redundant ensureLayout passes (LUM-827)#26054

Merged
ashleeradka merged 2 commits into
mainfrom
devin/lum-827-1776357598
Apr 16, 2026
Merged

fix(macos): coalesce ComposerTextEditor.measureHeight and skip redundant ensureLayout passes (LUM-827)#26054
ashleeradka merged 2 commits into
mainfrom
devin/lum-827-1776357598

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot commented Apr 16, 2026

Routes every measureHeight call site in the chat composer through a DispatchWorkItem-based scheduler and adds a (textStorage.hash, textContainer.containerSize.width) skip-guard inside measureHeight, so upstream SwiftUI state cascades, scroll/resize notifications, and per-keystroke textDidChange no longer stack O(n) synchronous NSLayoutManager.ensureLayout passes that were blocking the main thread for 2s+ on large composer content (LUM-827, Sentry VELLUM-ASSISTANT-MACOS-EH). All layout work stays on the main thread (NSLayoutManager is not thread-safe), the TextKit 1 stack and IntrinsicScrollView.contentHeight one-way flow are untouched, ensureLayout is still invoked whenever inputs change so usedRect never returns stale values, and dismantleNSView cancels any pending work.

What changed

  • Coordinator.scheduleMeasureHeight(_:) cancels any pending DispatchWorkItem and enqueues a new one via DispatchQueue.main.async — rapid bursts collapse into one ensureLayout pass on the next main-queue turn.
  • measureHeight early-returns when (textStorage.hash, containerSize.width) match the last measurement. NSAttributedString.hash is content-and-attribute based (Foundation), so it invalidates on any edit; containerSize.width (under widthTracksTextView = true) captures window/split-view resize.
  • invalidateMeasureHeightCache() is called in the fontChanged || colorChanged branch of updateNSView as a belt-and-braces fallback for typography mutations that may not produce a deterministic hash delta.
  • All five call sites (frame observer, bounds observer, makeNSView initial measurement, textDidChange, and the textWasExternallyReplaced || fontChanged-guarded re-measure in updateNSView) go through scheduleMeasureHeight so they coalesce across a single event.
  • dismantleNSView calls cancelPendingMeasureHeight() — no deferred layout pass fires against a torn-down view.

Builds on top of #25597 / LUM-832, which already converted the focus/cursor bindings to callbacks and guarded updateNSView behind change checks. That PR reduced the frequency of measureHeight calls; this one makes each call cheap when nothing layout-relevant changed.

Reviewer focus (highest risk first)

  • Skip-guard correctness. Relies on NSAttributedString.hash changing whenever content or attributes do. The typography-change fallback is defensive; worth sanity-checking that font/theme changes actually re-flow the composer.
  • One-runloop-tick defer. Every measurement (including the guarded updateNSView re-measure after external text replacement) now lands one DispatchQueue.main.async hop later. IntrinsicScrollView.contentHeight already smooths < 0.5pt deltas, so this should land within the same render frame — confirm no visible pop on mount, send (text → ""), or first paste of large content.
  • Lifecycle. DispatchWorkItem captures [weak self, weak textView]; dismantleNSView cancels pending work. No retain cycle and no layout against a torn-down view.
  • CI does not build macOS. Requires a local Xcode build to confirm it compiles. Behaviour to exercise manually: type, large paste, window resize, split-view resize, theme/font change, sending (text → "").

Alternatives considered and rejected

Documented so future work doesn't revisit dead ends:

  • Moving ensureLayout off-main — NSLayoutManager is not thread-safe.
  • Removing ensureLayout entirely — stale usedRect causes composer jump/clip (regresses LUM-622).
  • Bounded ensureLayout(forGlyphRange:in:) — doesn't yield total content height.
  • Dropping measureHeight for isVerticallyResizable + frameDidChangeNotification only — NSTextView resizes after layout, but SwiftUI needs the height at the start of the render cycle.
  • TextKit 2 migration — out of scope; the explicit TK1 stack at L98–112 avoids a known TK2→TK1 downgrade bug.
  • Storage-hash-only cache — misses window/split-view resize.
  • Length-only invalidation — misses same-length glyph-width edits (e.g. font substitution).
  • Timed debounce — introduces perceptible latency; next-runloop-tick is sufficient.

Related context

  • #24557 / LUM-746 added Equatable on ComposerView/ComposerSection to reduce the frequency of updateNSView; complementary to this PR.
  • #23672 established the IntrinsicScrollView.contentHeight one-way flow — preserved here.
  • #24401 → reverted in #24417 bridge refactor is deliberately not reintroduced.

Similar ensureLayout patterns in HighlightedTextView.swift, VCodeView.swift, and SelectableTextView.swift are out of scope here and candidates for a follow-up with the same coalescing shape.

Test plan

  • Pre-push hook (lint / type-check / design tokens): passing.
  • CI: Socket Security checks passing; macOS Build / macOS Tests skipped by CI (macOS toolchain not available in CI).
  • Local Xcode build + manual exercise of composer height tracking on type, large paste, window resize, split-view resize, theme/font change, and send (text → "") — pending by reviewer on macOS.

Link to Devin session: https://app.devin.ai/sessions/823de84dc8854081aea1f926200f39e5
Requested by: @ashleeradka


Open with Devin

…ant ensureLayout passes

`Coordinator.measureHeight(_:)` called `NSLayoutManager.ensureLayout(for:)`
synchronously on five hot paths (frame/bounds notifications, textDidChange,
updateNSView, initial dispatch after makeNSView). For large text the first
layout pass is O(n) in glyph count and can block the main thread for 2 s+;
upstream SwiftUI state cascades repeatedly stacked these calls.

Add a `DispatchWorkItem`-based coalescer so rapid successive requests
collapse into one layout pass on the next main-queue turn, and add a
storage-hash / container-width skip-guard inside `measureHeight` so
re-renders without layout-relevant changes are free. Typography updates
(font, line spacing, text color) in `updateNSView` explicitly invalidate
the cache. `dismantleNSView` cancels any pending work.

All layout work stays on the main thread (`NSLayoutManager` is not
thread-safe), the TextKit 1 stack and `IntrinsicScrollView.contentHeight`
one-way flow are untouched, and `ensureLayout` is still invoked when
inputs change so `usedRect` never returns stale values.

Fixes LUM-827.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

Copy link
Copy Markdown
Contributor

@vex-assistant-bot vex-assistant-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — Clean coalescing fix for composer main-thread hang

LUM-827 — 2s+ hang from uncoalesced NSLayoutManager.ensureLayout in ComposerTextEditor.measureHeight.

Approach validation

  • No circular fixes — git history shows no add/revert cycles on measurement coalescing
  • No competing in-flight work — PR #25597 (cursor flicker) is complementary, not conflicting
  • PR #24401 (bridge refactor) was reverted in #24417 — PR description correctly notes it's not reintroduced
  • Sentry evidence (VELLUM-ASSISTANT-MACOS-EH) confirms the hang

What this does

1. DispatchWorkItem coalescing — All 5 measureHeight call sites now go through scheduleMeasureHeight, which cancels any pending work item before scheduling a new one via DispatchQueue.main.async. Rapid bursts (upstream SwiftUI cascades, frame+bounds notifications, per-keystroke textDidChange) collapse to a single ensureLayout pass on the next runloop turn.

2. Skip-guardmeasureHeight now early-returns when (textStorage.hash, containerSize.width) haven't changed. This makes the unconditional scheduleMeasureHeight at the end of updateNSView safe — previously guarded by if textWasExternallyReplaced || fontChanged, the skip-guard is a more comprehensive check that catches ALL no-op cases.

3. invalidateMeasureHeightCache() — Called in the fontChanged || colorChanged branch of updateNSView as a belt-and-braces fallback for typography mutations that may not alter NSAttributedString.hash deterministically.

4. Lifecycle cleanupdismantleNSView calls cancelPendingMeasureHeight() so deferred work never fires against a torn-down view.

Review notes

  • Unconditional scheduleMeasureHeight in updateNSView replaces the old if textWasExternallyReplaced || fontChanged guard. This is correct — the skip-guard inside measureHeight handles the no-op case more comprehensively (hash+width vs. just text/font change detection). The deferred call is cheap when skipped (one hash comparison).
  • DispatchWorkItem cancellation means during high-frequency streaming re-evals, only the last scheduled measurement fires. No accumulation of queued work items.
  • NSAttributedString.hash is content-and-attribute based per Foundation — reliable for detecting text changes. Width check catches resize without text change.
  • TextKit 1 stays on main threadensureLayout is deferred but never moved off-main. Correct per Apple docs for NSLayoutManager.
  • IntrinsicScrollView.contentHeight 0.5pt delta smoothing is preserved — deferred measurement should land within the same render frame.

QA focus

  • Composer height tracking on: type, paste, resize, split-view, window resize, font/theme change
  • No visible height pop on mount or send (text → "")
  • First paste of large content should not hang

Follow-up candidates (noted in PR)

  • HighlightedTextView.swift:320 — same uncoalesced ensureLayout pattern
  • VCodeView.swift:336 — same pattern
  • SelectableTextView.swift:107,186 — same pattern

Adopt the callback-based focus / cursor-position wiring and per-property\nchange guards introduced by #25597, and keep the measureHeight coalescer\n+ skip-guard from this branch. The guarded `if textWasExternallyReplaced\n|| fontChanged` re-measure in updateNSView now routes through\n`scheduleMeasureHeight` so it coalesces with frame/bounds notifications\nthat the same event will fire.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
@ashleeradka ashleeradka merged commit bb0e620 into main Apr 16, 2026
6 checks passed
@ashleeradka ashleeradka deleted the devin/lum-827-1776357598 branch April 16, 2026 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant