Skip to content

Add .fitContent option for content growing to natural size#75

Merged
luca-chen198 merged 4 commits into
nodes-app:mainfrom
scosman:main
Jun 20, 2026
Merged

Add .fitContent option for content growing to natural size#75
luca-chen198 merged 4 commits into
nodes-app:mainfrom
scosman:main

Conversation

@scosman

@scosman scosman commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

I wanted to use this control on a screen that already had a scroll view. Currently the control embeds its own scroll view, so I was stuck with a scrolling screen, with a second nested scrollable markdown notes section. Not great.

This change adds .fitContent option. The control grows to fit its content, so I can just scroll the containing page to scroll my markdown note. Doesn't work with the header pinning, but that's expected.

Isolation: implemented as new and optional init option, off by default. Zero impact to existing API or existing users.

AI disclosure: this was all Claude Opus, I'm no swift UI expert. However I did read it, embed it in another app, and do manual UI tests. It's working great.

scosman and others added 4 commits June 19, 2026 14:23
Add MarkdownEditorConfiguration.heightBehavior (.scrolls default / .fitsContent). In .fitsContent the editor reports its content height to SwiftUI via sizeThatFits + ClampedScrollView.intrinsicContentSize, skips the viewport-fill inflation and bottom overscroll, and makes the inner scroll view inert (no scroller, wheel events forwarded) so an enclosing ScrollView/page scrolls instead of a nested inner scroller. The default .scrolls path is gated behind every new branch and is behavior-identical to before.

Phase 1 of 2: config knob + static fit-to-content sizing. 15 new tests; swift build/test green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add .fitContent option for content growing to natural size
Remove tests that only duplicate existing coverage — no real assertions lost (134 -> 128 tests, all green):

- fitsContentScrollableContentHeightEqualsBaseContent: tautological (scrollableContentHeight = base + 0 by definition)
- fitsContentApplyManagedFrameSizeUsesExactContentHeight: duplicate of fitsContentNoInflation (same invariant, different number)
- switchFromScrollsToFitsContentChangesDecision / switchFromFitsContentToScrollsChangesDecision: re-call the pure wantsVerticalScroller(for:) with inputs already covered by fitsContentAlwaysDisablesVerticalScroller + scrollsRespectsScrollersPolicy (a pure function has no 'switch' state)
- widthChangeUpdatesHeightInReadingColumnMode: its 'width change' re-applies identical values (no actual change); duplicates readingWidthPreservedNoInflation
- readingWidthEmptyDocStillHasPositiveHeight: near-duplicate of emptyDocHasPositiveHeight (readingWidth doesn't affect the assertion)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@luca-chen198

Copy link
Copy Markdown
Member

PR #75 (.fitsContent) — review & performance notes

Notes from reviewing PR #75 ("Add .fitContent option for content growing to
natural size") on this branch: regression review, performance investigation
(headless + in-app), and the test cleanup committed here.

Verdict

  • No regression for existing (.scrolls) users. heightBehavior is opt-in
    and defaults to .scrolls; every new code path is gated behind
    heightBehavior == .fitsContent / the fitsContent flag. If you don't set
    .fitsContent, behaviour is exactly as before. Full suite green.
  • Performance is a documented trade-off, not a bug. .fitsContent must lay
    out the whole document to know its height → no TextKit-2 viewport
    virtualization → O(n) layout cost on open/resize for large docs.

Regression review

  • New config field is additive: appended init param, default .scrolls.
    MarkdownEditorConfiguration is Sendable only (not Equatable), so adding a
    field changes no diffing.
  • .scrolls branches in applyManagedFrameSize, ClampedScrollView,
    recalcOverscroll, etc. are the original code unchanged.

Performance — headless benchmark (engine layout cost)

Real engine paths (recalcOverscroll / measuredBaseContentHeight /
ensureLayout), plain wrapped text, sizes 200–12000 lines, Debug build.

  • open / width-resize: .scrolls.fitsContent within <1% at every size.
    (Both modes already force a full-document layout at init —
    NativeTextViewWrapper.makeNSViewensureLayout(documentRange) — so
    .fitsContent adds no new full-layout cost.)
  • typing: ~0.15 ms/keystroke, flat across all sizes, both modes (partial
    end-of-document measure, not a full relayout).
  • forced full layout: ~linear, ~0.06 ms/line (12000 lines ≈ 700 ms in Debug).

.fitsContent adds no engine-layout overhead vs .scrolls.

Performance — in-app (Demo harness, Debug build)

  • Memory: +~5 MB at 2000 lines (~3%).
  • Frame time: median 8.3 ms (120 Hz) in both modes; most sampling
    windows had 0 dropped frames.
  • Big spikes were a measurement artifact: the multi-second worst frames
    lined up with bursts of the system containerToPush … (Handoff/Continuity)
    log flood + Xcode console rendering — present in both modes (the single
    worst frame at 2000 lines was actually in .scrolls, 979 ms).
  • At 20000 lines the worst-case freezes were larger in .fitsContent
    (seconds, on initial load / mode-switch / resize) — the full-document-layout
    cost. Debug (-Onone) inflates these heavily; Release is expected far
    smoother.

Where the cost actually is

Action .fitsContent Freeze?
Open / width-resize full layout, O(n) yes — the cost for huge docs
Typing partial end-measure, ~0.15 ms, size-independent no
Scrolling no re-layout (giant view may add draw cost at extreme sizes) shouldn't; verify in Release

Guidance

  • Use .fitsContent for small-to-medium inline content; keep .scrolls for
    large documents (virtualization). Off by default → zero impact when unused.
  • If .fitsContent is ever needed on very large docs and Release is still slow,
    the real fix is an engine change: report an estimated height
    (lineCount × avgLineHeight) and refine lazily, restoring virtualization.

Test cleanup (committed on this branch)

Removed 6 redundant tests from HeightBehaviorTests.swift (134 → 128 tests, all
green) — duplicates only, no real coverage lost:

  • fitsContentScrollableContentHeightEqualsBaseContent — tautological.
  • fitsContentApplyManagedFrameSizeUsesExactContentHeight — dup of
    fitsContentNoInflation.
  • switchFromScrollsToFitsContentChangesDecision /
    switchFromFitsContentToScrollsChangesDecision — re-test the pure
    wantsVerticalScroller(for:) with already-covered inputs.
  • widthChangeUpdatesHeightInReadingColumnMode — its "width change" re-applies
    identical values; dup of readingWidthPreservedNoInflation.
  • readingWidthEmptyDocStillHasPositiveHeight — near-dup of
    emptyDocHasPositiveHeight.

Method note

A headless perf benchmark (HeightBehaviorPerfTests.swift) and a Demo perf
harness (FPS via CADisplayLink + memory readout) were used during this
investigation and then removed — throwaway tooling, not part of this branch.

@luca-chen198

Copy link
Copy Markdown
Member

gj

@luca-chen198 luca-chen198 merged commit 6296e2a into nodes-app:main Jun 20, 2026
1 check passed
luca-chen198 added a commit that referenced this pull request Jun 20, 2026
fitsContent height behavior (#75), BlockquoteStyle.extraLineHeight (#76), and fixes (#69/#70/#71/#73).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

2 participants