fix(macos): unify editor presentation snapshots#3017
Conversation
|
Addressed the request-changes follow-up in commit 40fd333: visible promotion now carries the exact presented snapshot plus local transform, transaction freeze owns committed snapshot construction, invalid active gutter ownership rejects before publication, legacy FrameState cursor fallback is removed, and the render perf gate is tightened to 1.10x. Final validation: mix swift.build, full Debug xcodebuild tests (1249), Release build, make lint, mix test.llm, scripts/check_render_performance (combined p95 0.136ms), Swift LSP, pi-lens, focused bug review, and final reviewer PASS. |
jsmestad
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed at 40fd33362e4ca15d6679590b26d0f6c4949d8712. The snapshot direction is right, and this revision fixes the in-flight promotion race, captures the local transform with the presented snapshot, and constructs the snapshot before publication. It still does not satisfy the binding acceptance criteria in #2999.
Blocking correctness
-
A valid gutterless pane cannot render an active cursor.
.nonegutters are accepted, butrenderGutterfabricates an inactive gutter andresolveCursorsearches only active gutters, so it returnsnil. A mixed guttered/gutterless split also applies editor-global gutter padding to the gutterless pane. Pixels, IME/accessibility geometry, and the accepted pane geometry can diverge. Gutter projection, cursor resolution. -
Scroll-track input still uses the newest mutable
FrameState, not the visible presentation. If A is visible and B commits with different scroll metrics before B presents, clicking or dragging A's visible scrollbar is mapped with B's rows, top line, and total line count. This violates the committed-to-visible input contract. Mutable scroll-track reads. -
A keyframe can retain stale split metadata. Keyframes clear working windows/gutters/guides, but
preparedFrameStatecopies the previousFrameStateand only replaces separators whenguiSplitSeparatorsis present. A one-pane keyframe can therefore publish old two-pane separators, and freeze does not validate the combination. Prepared state replay. -
The required allocation and generation bounds are not implemented. Every draw still allocates a line
MTLBuffer, three quad buffers, and a viewport-sized render-target texture.NativePresentationGenerationorders completion but does not cap in-flight generations at three. This directly fails AC5. Buffer allocation, texture allocation, generation state. -
The old editor authorities remain. The transaction carries both
PreparedWindowUpdatesandeditorSnapshot; publication replays editor commands intoGUIState/FrameState; the renderer immediately decomposes paired surfaces back into dictionaries and rejoins them by window ID. That is the split authority AC6 requires removing, and it is the source of the gutterless regression. Dual transaction output, renderer decomposition.
Required evidence
The PR still lacks the ticket's temporal offscreen Metal captures, production interaction/host assertions, complete manual macOS matrix, warm allocation counters, GPU/drop measurements, three-generation retention evidence, and equivalent-baseline Release percentiles. The reported preparation aggregate does not establish those contracts.
Current CI is also red: Test (Elixir) failed in MingaAgent.SessionManagerTest. Swift and lint checks pass, but the branch is not merge-safe under the repository gate.
The smallest corrective shape is already present in the new types: freeze one editor snapshot, publish that value as the sole editor authority, keep PresentedWindowSurface intact through rendering and input, derive active ownership independently of gutter presence, and delete legacy replay, semantic FrameState mirrors, synthetic gutters, and renderer dictionary projections.
|
Review feedback addressed in Latest fixes:
Validation after the latest fixes:
|
Closes #2999
Context
Issue #2999 identified a structural macOS editor bug: independently owned frame pieces could be combined at draw or input time, which allowed blank editor frames, line-number flicker, mismatched gutter/content pairs, and pointer or accessibility geometry that did not match the pixels onscreen.
The fix follows the locked architecture in the ticket: freeze one complete committed editor snapshot, render from that exact value, then promote that captured value to the visible input snapshot only after presentation succeeds.
Changes
CommittedEditorSnapshot,PresentedWindowSurface,GutterPresentation,EditorLocalPresentationTransform, andVisibleEditorPresentationas immutable editor presentation values.FrameStateand window-content dictionaries.CommandDispatcher, including successful older in-flight presentation promotion when a newer commit is pending, and stale-presentation protection once newer pixels are visible.PreparedFrameTransaction.freeze, including staged theme, window, gutter, and indent-guide state before publication.frameState.dirty,markRendered) and made presentation metrics passive telemetry instead of observable invalidation state.FrameStatecursor fallback fromCoreTextMetalRenderer.resolveCursor.ResidentRowStoreand restricting full row materialization to DEBUG/test-only access.Review Fixes
.setWindowBgso frozen snapshots use the same colors as the published frame.FrameStateand onto the visible editor snapshot.Verification
mix swift.build✅cd macos && xcodebuild test -scheme Minga -configuration Debug -derivedDataPath DerivedData CODE_SIGNING_ALLOWED=NO✅, 1255 testscd macos && xcodebuild build -scheme Minga -configuration Release -derivedDataPath DerivedData CODE_SIGNING_ALLOWED=NO✅make lint✅mix test.llm✅, 58 doctests, 98 properties, 9942 tests, 0 failures, 1 skipped, 550 excludedscripts/check_render_performance✅, aggregate decode+apply p95 0.009ms, command-prep p95 0.119ms, combined p95 0.131msNote: an earlier
mix test.llmrun exposed a corrupted local~/.local/share/minga/agent_events.db. I verified the DB withsqlite3 PRAGMA integrity_check, moved it aside, reran the failed test locations successfully, then reran the fullmix test.llmsuccessfully. A later fullmix test.llmrun had one NativeMCP provider test missAgentEnd; the isolated test passed immediately afterward and the full suite passed on the next run.Acceptance Criteria Addressed
EditorNSViewand survives unrelated shell/editor commits when identities remain valid ✅