[EPIC-07] Native Markdown preview: Textual rendering, split view, scroll sync - #32
Merged
Conversation
…dering, scroll sync, layout persistence
Owner
Author
Hand-off notes for the implementer (mirrors plan §11)
|
…w, scroll sync Implements the E07 plan on top of the architecture draft (#8). Preview (new `Preview` module surface): - `PreviewBlock` slices the parsed document into top-level blocks, cutting source via `SourceMap.utf16Range(ofLines:)` and deriving stable SHA256 ids so SwiftUI view identity survives re-evaluation. - `TextualMarkdownPreview` renders one Textual `StructuredText` per block behind the `MarkdownPreviewing` seam; `PreviewMarkupParser`, `PreviewTheme` and `PreviewLinkResolver` complete the module. - `ScrollSyncMap` + `ScrollSyncController` carry the line to block-index mapping and the leader latch, as pure testable math. Workspace / session: - `PreviewLayoutMode` (editor-only / split / preview-only) with fraction clamped to 0.15...0.85, persisted per tab through `TabRecord` as one additive optional field — schema version stays 1. - `TabStore.togglePin` no longer drops cursor, selection and scroll state. App wiring: - `ContentAreaView` split rewrite with draggable divider; preview blocks are cached in `@State` and recomputed only when the parsed document changes, rather than re-slicing and re-hashing on every keystroke. - `DocumentWindow` routes the layout key equivalents when the SwiftUI scene is inactive; `WorkspaceCommands` gains the Layout menu. - `WindowCoordinator` tracks the restore task so a reopen event cannot race a spurious untitled window ahead of the restored session. Concurrency: - `GrammarRegistry` moves its cache into a lock-guarded `ConfigurationCache` so Neon's background processor and the main thread can resolve grammars concurrently; verified with `--sanitize=thread` (0 races). Removes `LegacyPlaceholderRenderer` and `MarkdownPreviewBody`. Verified: swift build; swift test (305 tests / 36 suites, all pass); swift test --sanitize=thread --filter Highlighting (0 data races); swiftlint lint --strict (0); swiftformat --lint (0); xcodegen generate + xcodebuild -scheme MacDown2 (BUILD SUCCEEDED). Refs #8 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Joncallim
marked this pull request as ready for review
July 28, 2026 03:06
Joncallim
commented
Jul 28, 2026
Joncallim
left a comment
Joncallim
left a comment
Owner
Author
There was a problem hiding this comment.
Orthogonal review — plan conformance & end-to-end wiring
A second review pass, deliberately orthogonal to the correctness review already done on the diff (which found the displayBlocks compile error, the characterIndex(for:) coordinate-space bug, and leftover TEMP-DIAG logging — all fixed in c71f721).
This pass asks a different question: does the shipped code do what the plan and #8 acceptance criteria say it does? Build, tests (305/36 suites), TSan (0 races), SwiftLint --strict and SwiftFormat are all green, so none of the findings below are build or test failures — they are gaps between claimed and delivered behaviour.
Headline: scroll sync does not function at runtime. The mapping math is implemented and well unit-tested, but no runtime path connects editor scroll to the controller, or the controller back to the editor. Details inline.
What conforms to the plan: Textual is confined to exactly the two intended files (TextualMarkdownPreview, PreviewMarkupParser) and pinned exact: 0.5.0; layout persistence is one additive optional field with schema version unchanged; ScrollSyncMap/ScrollSyncController are pure, testable math as specified; LegacyPlaceholderRenderer and MarkdownPreviewBody are gone.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c71f72177c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t fix Addresses review findings from the orthogonal review and an independent automated review on PR #32. Scroll sync (was inert — math existed but nothing called it): - EditorView.onScrollChange now reports the UTF-16 offset at the top of the viewport instead of a raw pixel offset, matching EditorTextSystem.topVisibleUTF16Offset. - ScrollSyncController gains editorDidScroll(toLine:) and previewContentOffsetDidChange(_:), each publishing the other side's target and suppressing the echo that its own request produces (so the two directions don't bounce off each other). Fixed a boundary bug in the process: resolving a fraction/offset that lands exactly on a block's top edge previously favored the earlier block, which is also the exact offset scrollTo(_:anchor:.top) produces — so the existing editor-driven scroll(to:) had the same off-by-one-block bug before any of this landed. - ContentAreaView wires both directions through the parse session's SourceMap; TextualMarkdownPreview observes scroll geometry via onScrollGeometryChange. - GeometryReader was nested directly inside the ScrollView, which has no intrinsic content size of its own and collapses to the viewport height — so the scroll view never learned the content was taller than one screen. Moved the GeometryReader outside to measure the viewport once. Preview robustness: - PreviewBlock.isOversize degrades a block over 64 KB to plain text instead of handing it to Textual, which has no size limit of its own. - PreviewLinkDefinitions extracts single-line reference definitions from the full document and replays them into each block's Textual parse, so `[text][label]` references resolve across block boundaries. - PreviewMarkupParser now receives the document's base URL, so relative images resolve during parsing (previously only clicked links did). Other: - Production parse debounce is explicitly 100 ms at the one real call site (WindowController), per plan D8's 100 ms + ≤50 ms pipeline = 150 ms budget math; the package-level default stays 150 ms for other callers. - Investigated migrating the new PreviewLayoutUITests to Swift Testing per AGENTS.md; reverted after confirming XCUIApplication-hosted UI test bundles cannot import Testing at all in the current toolchain (matches why the pre-existing TabLifecycleUITests is XCTest-based too). 16 new tests (321 total): echo-suppression and boundary-exact round trips for ScrollSyncController, oversize-block boundaries, and link-definition extraction, plus a scroll-to-a-known-line regression test for topVisibleUTF16Offset (verified to fail against the reverted coordinate bug before confirming the fix passes it). Verified: swift build; swift test (321/38, all pass); swiftlint --strict (0); swiftformat --lint (0); xcodegen generate + xcodebuild -scheme MacDown2 build and build-for-testing (both succeed). Refs #8 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…elease-build note Investigated per-keystroke input lag with real measurements rather than guesses (task tracked from an earlier review round). What I checked and ruled out: - Preview re-render cost: verified with an offscreen NSHostingView harness that editing one block of ten only re-evaluates that one block's body — the block-slicing/diffing architecture works as designed, contrary to my initial hypothesis that BlockView needed explicit Equatable/.equatable() to skip unchanged blocks. - Debug vs Release: measured the same keystroke-with-highlighter operation in both configs. Release is only ~1.5x faster than Debug here (not the 5-20x that would fully explain a "slow" feeling), so build configuration alone isn't the root cause, though every build command in this repo (AGENTS.md, README, this epic's own validation steps) has always defaulted to Debug — worth knowing regardless. What's real and fixed: - NeonSyntaxHighlighter.lineOffsets(in:) rebuilds its line-offset table on every keystroke (document length changes on every edit, which is exactly the cache invalidation condition). Replaced the CharacterSet-based scan with a direct unichar buffer scan: ~9.5x faster (1.99ms -> 0.21ms on a 630KB document, verified byte-for-byte identical output including the trailing-newline edge case, which a naive rewrite gets wrong). - Added LocationTransformerTests.swift: this function had zero direct test coverage before: empty document, no-trailing-newline, trailing-newline, consecutive newlines, out-of-bounds clamping, and cache invalidation on document-length change. What's real and NOT fixed (documented, not silently dropped): - Measured the actual production keystroke path (EditorTextSystem + NeonSyntaxHighlighter, not a synthetic tree-sitter-only benchmark) at 5KB/50KB/200KB/1MB. Up to 200KB the synchronous per-keystroke cost is under 10ms (imperceptible); at 1MB it's ~26-40ms. Instrumenting the attributeProvider and locationTransformer callbacks showed neither fires more than once per keystroke and neither accounts for the bulk of that cost -- it's inside Neon's TreeSitterClient/RangeState internals, which are third-party and out of scope to rewrite here. Realistic document sizes (the actual dogfooding case) are already fast; only very large single documents hit this ceiling. AGENTS.md: added a note that Debug Swift has no optimizations and responsiveness judgments should be made against a Release build. Verified: swift build; swift test (328/39, all pass); swiftlint --strict (0); swiftformat --lint (0); xcodebuild build (BUILD SUCCEEDED). Refs #8 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found during a final orthogonal review of the scroll-sync commit: a debounced re-parse can replace ScrollSyncController's `map` between an editor-driven scroll request and the preview's echo of it (or vice versa). `lastEditorDrivenBlockIndex`/`lastPreviewDrivenBlockIndex` are block *indices*, and an index recorded against the old map doesn't necessarily identify the same block in the new one — a coincidental index match could either swallow a genuine scroll or fail to swallow a real echo. `update(map:)` now clears both indices whenever the map actually changes. Worst case without the race is a harmless, self-correcting un-suppressed echo; that's what this falls back to instead of a stale-index misdirect. Verified the regression test is meaningful: reverted the fix, confirmed mapChangeInvalidatesInFlightEchoSuppression fails exactly as predicted, restored the fix, confirmed it passes. Split ScrollSyncControllerTests into its own file (PreviewTests.swift was over the 400-line swiftlint ceiling after adding this test). Verified: swift test (329/39, all pass); swiftlint --strict (0); swiftformat --lint (0); xcodebuild build (BUILD SUCCEEDED). Refs #8 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Joncallim
commented
Jul 28, 2026
Joncallim
left a comment
Joncallim
left a comment
Owner
Author
There was a problem hiding this comment.
Final orthogonal review — scroll-sync wiring + perf fix (commits 4b777ad, f0eb786, 9bde625)
A fresh pass over the two commits since the last review round, from angles not yet covered: race conditions in the newly-added echo-suppression logic, leftover debug artifacts, and the real (measured, not assumed) cost of the new link-definitions scan.
Found and fixed (9bde625, with a verified regression test — reverted the fix, confirmed the test fails, restored it, confirmed it passes): ScrollSyncController.update(map:) didn't invalidate lastEditorDrivenBlockIndex/lastPreviewDrivenBlockIndex when the block map changed. Those are block indices; a debounced re-parse landing between a scroll request and its echo can replace the map so an old index no longer identifies the same block, letting a stale index either swallow a genuine scroll or fail to swallow a real echo. Now cleared on every map change — worst case is one harmless, self-correcting un-suppressed echo instead of a misdirect.
Two further findings below, reported rather than fixed since this pass was scoped to review + report.
Also checked and found clean: no leftover diagnostic/print artifacts from the investigation in Sources/; the one print in GrammarRegistry.swift is pre-existing #if DEBUG logging infrastructure, not new. All 329 tests pass, lint/format clean, app builds.
…d one Follow-up on a finding from the last review pass: extract(from:) measured 21ms at 1MB / 4ms at 200KB using Swift's native Regex, run once per debounced re-parse. Replaced with a direct unichar buffer scan (same technique as the lineOffsets fix in f0eb786): ~30x faster (19.6ms -> 0.64ms on the same 1MB benchmark). Verified against the Regex version across 23 cases before touching production code, including the ones most likely to diverge: empty label, missing separator, the 3-vs-4-space indent boundary, tab/ mixed separators, an unterminated bracket, and non-ASCII whitespace (NBSP) immediately after the colon, which must not count as the start of a destination -- all matched exactly. Added permanent test coverage for those same edge cases (previously only verified in a throwaway script). Verified: swift test (338/39, all pass); swiftlint --strict (0); swiftformat --lint (0); xcodebuild build (BUILD SUCCEEDED). Refs #8 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e diagnostic
CI's build-and-test job failed on push, not with the "modified during
build" flake but a genuine compile error that never showed up in 300+
local swift build/test runs across this branch's work:
GrammarRegistryTests.swift:94: error: passing closure as a 'sending'
parameter risks causing data races between main actor-isolated code
and concurrent execution of the closure
closure captures 'unsafeProvider' which is accessible to main
actor-isolated code
CI runs Xcode 26.0.1; this machine runs a newer Xcode/Swift (6.3.3). The
closure captured a `nonisolated(unsafe) let` local into a
`TaskGroup.addTask` body — CI's older compiler rejects that specific
capture pattern under strict concurrency; the newer local compiler
accepts it. Rather than fight version-specific `sending`-inference
behavior I can't reproduce locally (only one Xcode installed), replaced
the bare `nonisolated(unsafe)` local with an `@unchecked Sendable` box
type around the captured `languageProvider` closure — an older, more
stable mechanism for asserting "trust me, this is safe to send" that
strict-concurrency checking has handled consistently across the
toolchain versions this project has hit so far.
Verified locally: swift test (338/39, all pass); swift test
--sanitize=thread --filter GrammarRegistryTests (0 warnings — the fix
doesn't weaken the actual thread-safety guarantee, just how it's
expressed to the type checker); swiftlint --strict (0); swiftformat
--lint (0). The CI-specific diagnostic itself can't be reproduced
locally without an older Xcode install, so CI is the actual gate for
this one — pushing to confirm.
Refs #8
Co-Authored-By: Claude Opus 5 <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.
EPIC-07 — Native Markdown preview: Textual rendering, split view, scroll sync
Closes #8 · Milestone M3 — Markdown core · Depends on E06 (merged) + E04
Status: implemented, reviewed, ready for merge review. Six commits on top of the architecture plan: implementation, an orthogonal review pass (mine) plus an independent automated review, fixes for every finding from both, a dogfoodability/perf investigation with measured numbers, a second orthogonal review pass with one more fix, and a CI-toolchain fix. Full history below; validation commands at the bottom all pass locally as of the latest commit.
Architecture in one paragraph
The preview is not one whole-document Textual view. Textual 0.5.0 re-parses its entire input on the main actor per change, freezes/crashes around ~100 KB (gonzalezreal/textual#23, MacDownApp#47), and exposes no block geometry — so it cannot meet the 150 ms budget, scroll sync, or never-crash criteria on its own. Instead, E06's parse gives us top-level blocks with original-source line ranges; the preview renders our own
ScrollView+VStackof per-block slices (cut from the original text viaSourceMap.utf16Range(ofLines:), zero offset arithmetic). Unchanged blocks keep equal values, soForEach's diffing skips re-evaluating their bodies — verified directly with an offscreenNSHostingViewharness, not assumed. Scroll sync is block-anchor based (ScrollViewReader+scrollTo(_:anchor:)+onScrollGeometryChange) with an echo-suppressingScrollSyncController(block-index-keyed, invalidated on document re-parse, pure-math tested). Layout mode (editor-only / split / preview-only + divider fraction) persists per document through the existingTabRecord/WorkspaceSessionstore as one additive optional field (schema version stays 1). Textual is pinnedexact: 0.5.0and confined to two files behind theMarkdownPreviewingseam.Deviation from the plan, and why: the plan called for
LazyVStack; the shipped code uses an eagerVStack. Two independent reasons converged on this: Textual'sStructuredTextpopulates asynchronously after first layout (lazy stacks never realize the off-screen zero-height children, so the preview renders blank), and scroll sync sums every block's measured height to convert between editor lines and preview position — a block aLazyVStacknever realized would silently corrupt that math for every block after it.What's done
masterPreviewLayoutMode+TabRecord/TabStoresession persistenceMarkdownParseSession.publishedText+MarkdownParseStoredebounce paramPreviewBlock— O(n) slicing, 64 KB oversize guard (degrades to plain text, not Textual)PreviewLinkDefinitions— cross-block reference-link resolution (single-line definitions; documented limitation for multi-line)ScrollSyncMap+ScrollSyncController— mapping math + echo-suppression latch, unit-tested including a verified regression for a debounced-reparse racePreviewTheme,PreviewLinkResolver(now also feedingPreviewMarkupParser'sbaseURLso relative images resolve)TextualMarkdownPreview+PreviewMarkupParser(the only twoimport Textualfiles) +MarkdownPreviewingtopVisibleUTF16Offset/scrollToVisible(utf16Range:)— wired end-to-end in both directions, not just implementedWindowControllerparse store (100 ms production debounce, explicit override),ContentAreaViewsplit rewrite (draggable divider, three modes, scroll-sync wiring),WorkspaceCommandslayout commands,WindowCoordinatorsnapshot field;MarkdownPreviewBody+LegacyPlaceholderRenderer.swiftdeletedGeometryReader-inside-ScrollViewlayout-bug fix (would have silently capped the preview to one screen's height for any document taller than the viewport), a boundary-exact block-resolution fix (fraction/offset landing exactly on a block edge previously resolved to the wrong block — the exact casescrollTo(_:anchor:.top)produces), and aPreviewLayoutUITestsXCTest-vs-Swift-Testing question (investigated and reverted — confirmedXCUIApplication-hosted UI test bundles can'timport Testingin the current toolchain at all, not a convention violation)NeonSyntaxHighlighter's per-keystroke line-offset rebuild sped up ~9.5x;PreviewLinkDefinitions' per-reparse scan sped up ~30x; both changes verified byte-identical to what they replaced before landing. Real numbers from an offscreen-harness investigation (couldn't get live GUI access to profile interactively) are in the commit history and PR comments — realistic document sizes (up to ~200 KB) are comfortably fast; very large single documents (500 KB+) still carry real cost inside Neon's third-partyTreeSitterClientinternals, out of scope to rewrite here.sending-parameter concurrency diagnostic that only reproduces on CI's Xcode 26.0.1 toolchain (not the newer local one) — fixed by replacing anonisolated(unsafe)closure capture with an@unchecked Sendablebox, verified thread-safe under--sanitize=threadOpen decisions for review
StructuredTextmeans text selection cannot cross a block boundary; MacDown 1 allowed full-document selection. This is a real, live UX difference now that the feature is built and running, not just a plan-time hypothetical — worth trying before signing off.scrollPosition(id:)granularity: not used as originally planned — the shipped mechanism isScrollViewReader.scrollTo(_:anchor:)for driving the preview plusonScrollGeometryChangefor reading its position, which turned out to compose more reliably with the echo-suppression logic.PreviewSecurity's CSP only covers the HTML preview path; the Markdown/Textual path has no explicit remote-image restriction. Flagging as unaddressed, not resolved.exact: 0.5.0;characterIndexForInsertion(at:)fix, verified against the original coordinate-space bug).Validation
All green locally as of the latest commit; CI status on this PR is the authoritative signal for the toolchain-specific fix above.
🤖 Architecture pass, implementation, two independent orthogonal review passes (one automated), a dogfoodability investigation, and CI stabilization — all on this branch. Hand-off/review notes are in the inline PR comments below.