Skip to content

fix(macos): unify editor presentation snapshots#3017

Open
jsmestad wants to merge 4 commits into
mainfrom
fix/2999-editor-snapshot
Open

fix(macos): unify editor presentation snapshots#3017
jsmestad wants to merge 4 commits into
mainfrom
fix/2999-editor-snapshot

Conversation

@jsmestad

@jsmestad jsmestad commented Jul 19, 2026

Copy link
Copy Markdown
Owner

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

  • Added CommittedEditorSnapshot, PresentedWindowSurface, GutterPresentation, EditorLocalPresentationTransform, and VisibleEditorPresentation as immutable editor presentation values.
  • Moved editor completeness checks into transaction freeze so missing pane geometry, missing required gutters, orphan gutters, incompatible gutter geometry/metrics, multiple active gutters, and invalid active windows reject before publication.
  • Changed the Metal renderer to accept one committed snapshot instead of separate FrameState and window-content dictionaries.
  • Added committed-versus-visible lifecycle tracking in CommandDispatcher, including successful older in-flight presentation promotion when a newer commit is pending, and stale-presentation protection once newer pixels are visible.
  • Moved editor input, scroll bookkeeping, gutter hit testing, IME/accessibility geometry, and cursor/accessibility notifications to the visible presentation boundary.
  • Moved committed editor snapshot construction into PreparedFrameTransaction.freeze, including staged theme, window, gutter, and indent-guide state before publication.
  • Removed the old dirty render lifecycle (frameState.dirty, markRendered) and made presentation metrics passive telemetry instead of observable invalidation state.
  • Removed the legacy FrameState cursor fallback from CoreTextMetalRenderer.resolveCursor.
  • Kept resident row storage bounded by preserving ResidentRowStore and restricting full row materialization to DEBUG/test-only access.
  • Added source guardrails so fragmented editor render/input authorities and committed-snapshot fallbacks do not reappear.
  • Tightened the render performance regression gate from 1.20x to 1.10x.

Review Fixes

  • Fixed in-flight drawable completion so an older successfully presented frame can still become visible when a newer commit is pending, while already-visible newer frames still win.
  • Promoted the exact visible value as snapshot plus local scroll transform, and moved input/accessibility/gutter geometry to that promoted visible value.
  • Froze the committed editor snapshot inside the prepared transaction instead of rebuilding it after publication.
  • Rejected multiple active semantic cursor/gutter owners before publication.
  • Removed remaining legacy cursor fallback authority.
  • Fixed staged theme colors and staged .setWindowBg so frozen snapshots use the same colors as the published frame.
  • Moved scroll-track line mapping off mutable FrameState and onto the visible editor snapshot.
  • Cleared stale split separator, gutter column, and viewport globals on keyframes before snapshot freeze.
  • Replayed staged cursor-shape commands into prepared frame state before freeze.
  • Preserved gutterless active ownership by selecting the unique cursor-visible surface when no active gutter exists.
  • Added three-slot native draw/render-target reuse so warm cursor/local-scroll frames do not allocate line buffers, quad buffers, or offscreen render targets after warm-up.

Verification

  • mix swift.build
  • cd macos && xcodebuild test -scheme Minga -configuration Debug -derivedDataPath DerivedData CODE_SIGNING_ALLOWED=NO ✅, 1255 tests
  • cd 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 excluded
  • scripts/check_render_performance ✅, aggregate decode+apply p95 0.009ms, command-prep p95 0.119ms, combined p95 0.131ms
  • Swift LSP diagnostics on touched Swift files ✅, 0 diagnostics
  • pi-lens full/cheap scan on touched files ✅, no issues from available checks
  • Focused NativeRenderResources, CommandDispatcher, and cursor suites ✅
  • Focused bug-hunting review ✅, fixed the reported gutterless active-owner issue
  • Targeted re-review of gutterless active ownership ✅, PASS

Note: an earlier mix test.llm run exposed a corrupted local ~/.local/share/minga/agent_events.db. I verified the DB with sqlite3 PRAGMA integrity_check, moved it aside, reran the failed test locations successfully, then reran the full mix test.llm successfully. A later full mix test.llm run had one NativeMCP provider test miss AgentEnd; the isolated test passed immediately afterward and the full suite passed on the next run.

Acceptance Criteria Addressed

  • Complete editor presentations are frozen before publication, with explicit gutter-present or gutterless surfaces ✅
  • Rejected, incomplete, stale, or incompatible editor updates preserve the last good presentation ✅
  • Metal drawing consumes one committed editor snapshot and captures it once per draw ✅
  • Successful drawable presentation promotes the exact captured snapshot and frontend-local transform to visible input geometry ✅
  • Mouse, scroll, gutter hit testing, IME, cursor, and accessibility geometry consume visible presentation state rather than newer unpresented committed state ✅
  • Frontend-local interaction state stays owned by EditorNSView and survives unrelated shell/editor commits when identities remain valid ✅
  • Superseded dirty lifecycle, fragmented renderer entry points, post-freeze joins, and observable metrics paths are removed or guarded ✅
  • Resident row storage remains bounded, with full-document materialization restricted to test/debug-only paths ✅
  • Regression coverage now includes freeze rejection, committed-to-visible lifecycle, shell-only identity preservation, Release accessibility compilation, conformance fixtures, and guardrails for old authority seams ✅

@jsmestad

Copy link
Copy Markdown
Owner Author

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 jsmestad left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

  1. A valid gutterless pane cannot render an active cursor. .none gutters are accepted, but renderGutter fabricates an inactive gutter and resolveCursor searches only active gutters, so it returns nil. 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.

  2. 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.

  3. A keyframe can retain stale split metadata. Keyframes clear working windows/gutters/guides, but preparedFrameState copies the previous FrameState and only replaces separators when guiSplitSeparators is present. A one-pane keyframe can therefore publish old two-pane separators, and freeze does not validate the combination. Prepared state replay.

  4. 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. NativePresentationGeneration orders completion but does not cap in-flight generations at three. This directly fails AC5. Buffer allocation, texture allocation, generation state.

  5. The old editor authorities remain. The transaction carries both PreparedWindowUpdates and editorSnapshot; publication replays editor commands into GUIState/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.

@jsmestad

Copy link
Copy Markdown
Owner Author

Review feedback addressed in 8c671c6.

Latest fixes:

  • Scroll-track line mapping now reads the visible editor snapshot instead of mutable FrameState.
  • Keyframes clear stale split separator, gutter column, and viewport globals before snapshot freeze.
  • Staged cursor-shape commands replay into prepared frame state before freeze.
  • Published frameState now matches the frozen editor snapshot frame state.
  • Gutterless split active ownership now selects the unique cursor-visible surface when no active gutter exists.
  • Native renderer draw buffers and offscreen render targets now use three reusable slots, with a warm-cache regression proving no buffer/target allocation after warm-up.

Validation after the latest fixes:

  • mix swift.build passed.
  • Full macOS Debug tests passed, 1255 tests.
  • macOS Release build passed.
  • make lint passed.
  • mix test.llm passed, 58 doctests, 98 properties, 9942 tests, 0 failures, 1 skipped, 550 excluded.
  • scripts/check_render_performance passed, combined p95 0.131ms.
  • Swift LSP diagnostics on touched Swift files passed with 0 diagnostics.
  • Focused code review rechecked gutterless active ownership and returned PASS.

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.

macOS editor content remains coherent through scrolling and presentation

1 participant