Animate every inspector disclosure, and stop the scroll restore from fighting the reader - #148
Merged
Merged
Conversation
The scrolling header's band was owned by the equality constraint while expanded (clip.height == host.height), so only a collapsed↔expanded edge went through applyExpansion and animated. Content disclosed while already expanded — an embedder opening a second inspector section — resized the band in a single layout pass: the first disclosure revealed over 0.32s and every later one snapped. The band now stays on the animatable constant constraint for the whole expanded state, and a new observer on the hosted view's frame animates it to the content's new height on the crossing's own curve. Taking the change from the HOST rather than the clip is what makes it a reveal: the clip has not moved yet when we are called, so the animation starts where the reader last saw the band. Reacting to the clip instead was measurably too late — it read 782 while the band still showed 913, i.e. the mask stood on the end height for a frame before jumping back. Both directions animate. On a shrink the embedder's content is already gone and the mask closes over blank space, but that is exactly what a crossing close has always done; animating only growth reads as a fault. A live resize and an offscreen container track directly instead of animating — neither is a disclosure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A disclosure while the band was already animating was dropped, not applied: the observer bailed out whenever an animation was in flight. Toggling a section twice in quick succession therefore finished on the height of the OPEN state while the content was already closed, leaving the band standing in the opened layout. Nothing corrected it afterwards. The band is held by the constant constraint for the whole expanded state now, so the equality constraint that used to re-sync it on the next layout pass is no longer there — a dropped change is permanent. Every path that declines a height change is a lasting wrong state. It retargets instead, which `animate(to:)` already supports (the token invalidates the stale completion). Mid-flight the constant is a moving value, so the comparison is against the target it is heading for. The direct-set path (live resize, offscreen) went through the animator at zero duration for the same reason: a plain property set leaves an in-flight animation ticking toward its stale target. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
remove(from:) removed the clip's frame observer but not the host's, which was only released in deinit — so a controller that outlives its header kept observing a view it no longer owns. Worse than the leak was the order. removeFromSuperview() lays out, and the host observer is delivered synchronously (queue: nil). Reached while lastExpanded, constantConstraint and hostingView were still set, it passed every guard in hostHeightChanged and started an animation — advancing animationToken right after this method had equalised it with settledToken. The controller then believed an animation was in flight for the rest of its life, which silently changes how the next disclosure is retargeted. Observers first, then the state they guard on, then the views. Reachable from Nodes: expanding the inspector and opening the Flow overlay drops the header (showsInspector: overlayState != .flow) and calls remove(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Switching documents changes the hosted header's height exactly like a disclosure does — the controller sees "the content is a different height" and nothing else — so the band animated, and because every band change restacks the body, the whole document slid on every switch. Two notes whose inspectors differ by 6pt were enough to see it. The embedder now says which is which: reconcileHeader compares the coordinator's documentId before updateNSView advances it, and arms snapNextHeightChange(). Applying it turned out to need three separate things, each of which was measured, not assumed: 1. Do not animate. A deadline rather than a one-shot flag: if the new document happens to have the same header height no change ever arrives, and a flag would stay armed and swallow the next real disclosure. 2. Assign the constant DIRECTLY. Even at zero duration the animator defers the model value to a later transaction — measured 57ms, which reads as the text sitting still and then jumping. The token is advanced instead so a completion from an in-flight animation cannot settle on top. 3. Resolve the layout in the reconcile pass. Setting the constant is not enough: `headerHeight`, and therefore the body's origin, is read from the CLIP, which trailed the constant by 42ms. The band has to be right before the new document paints, which is what the equality constraint used to do for free by resizing the clip in the same pass as the host. Measured after: the band settles 3ms after the switch is detected and before NODE_SWITCH_LOADED, where it previously moved 42ms after the new text was already laid out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Returning to a note after the embedder routed away — Nodes switching to an image or a PDF and back — reopened it at the top even though the offset was saved and read back correctly. The restore ran in updateNSView, where the clip view can still be zero-height. With no range to clamp against the scroll is accepted verbatim, so the "landed" check was true for every value: it proved the offset had been SET, not that it had survived. The latch retired on that, the first real layout dropped the reader back to the top, and nothing was left to correct it. Measured: saved=201 actual=201 landed=true viewportH=0. Requiring a measured viewport there only moved the failure: every update pass had viewportH=0, the attempts drained against nothing and the latch stayed armed forever — which also made teardown refuse to save, since a pending restore means the current offset is the load position. So the offset is handed to the scroll view instead and applied from its own `layout()`, the first moment viewport and document heights exist. It retries while the document keeps growing (measured 427 → 889 → 1327 across passes on one remount) and stops as soon as it lands. A scroll-wheel event drops it: the reader taking over must never be yanked back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The restore latch was only cleared by `scrollWheel`, but find-match reveal, keyboard paging and drag-select autoscroll all scroll the clip view directly and never pass through it — so the next layout pass replayed the remembered offset and threw the reader back. Also give the latch a deadline: a pass budget only ticks when a pass happens, so an offset that never landed stayed armed indefinitely and any later relayout could replay it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prints which coordinator answers a `findQuery` broadcast, the requested vs. clamped match index, and whether the reveal actually scrolled. Diagnosing next/previous not working while matches exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit 03cd27d.
The find query is broadcast to every live coordinator (`object: nil`), and an editor the host has routed away from can outlive its view. It answers for a document nobody is looking at — with an empty buffer, so it reports zero matches, and the host resets its match index on a zero count. Measured in the app: 24 coordinators replying to one Cmd+F, 22 of them windowless, leaving next/previous bouncing between the first two matches forever. The two find tests hosted their text view in no window at all, so they went red on the guard — correct: a document being searched is on screen by definition. The fixture now puts the view in a real NSWindow, and a new test covers the case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The inspector's header band only animated when its disclosure crossed between "nothing open" and "something open". Opening a second section while one was already open jumped. This animates every disclosure, then fixes the three things that turned up while making that hold.
Header band
Scroll restore
layout(), not from a SwiftUI state tick. A zero-height clip view accepts any offset verbatim, so the old code "succeeded" against no geometry and the first real layout dropped it (measured:saved=201 actual=201 landed=true viewportH=0).scrollWheelcleared it, but find-match reveal, keyboard paging and drag-select autoscroll all scroll the clip view directly — so the next layout pass replayed the remembered offset and threw the reader back.Find
object: nil), and an editor the host has routed away from can outlive its view. It answers for a document nobody is looking at — with an empty buffer, so it reports zero matches and the host resets its match index. Measured in the app: 24 coordinators replying to one ⌘F, 22 of them windowless, leaving next/previous bouncing between the first two matches forever.Tests
323 pass. Two find tests hosted their text view in no window at all and went red on the window guard — correct, a document being searched is on screen by definition; the fixture now uses a real
NSWindowand a new test covers the windowless case. The suite also caught a>= 0boundary I got wrong on the retarget guard.Note the animated band itself is not covered: the tests have no
NSWindowdrivingNSAnimationContext, so the animated path can only be verified by hand.