[EPIC-08] Content browser: heading outline of the active document - #33
Conversation
Implementation plan for the sidebar heading outline (issue #9), written to the same contract as the E06/E07 plans: binding decisions, exact API surface, exact file layout, acceptance-mapped test plan, and hand-off pitfalls. Two things the read-through of `master` turned up, both folded into the plan as their own early commits because E08's acceptance cannot be met without them: - The app has injected `NoOpStateStore` since E03's native-tabs rewrite, so E02's sidebar persistence has been dead code — nothing about the sidebar survives relaunch today. Acceptance box 4 ("section order persists across relaunch") is unreachable until that is a real store. - `EditorTextSystem.scrollToVisible(utf16Range:)` forwards an unclamped range built from the parsed document into a text view holding the live text. The two disagree for up to one debounce interval, and an out-of-bounds NSRange raises an uncatchable NSRangeException. E07 can hit this; E08 puts a clickable trigger for it one click away at all times. The plan also records that E06 already did most of this epic's engine work: `MarkdownDocument.headings` is flattened, plain-text titled, code-block-free, and in original-source line numbers. E08 adds no parse, no debounce, no store and no actor — and no Package.swift, project.yml or ci.yml change, since the OutlineUI target and its test target already exist. Refs #9 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Joncallim
added
markdown
…state - Add sidebarSectionOrder to WorkspaceStateStoring and WorkspaceStateStore. - Make SidebarSection Identifiable with defaultOrder and reconcile(_:). - Hydrate sectionOrder in WorkspaceModel and cache sectionExpanded. - Add WorkspaceModel.moveSections(fromOffsets:toOffset:) with write-through. - Replace NoOpStateStore with real WorkspaceStateStore in WindowCoordinator. - Add SidebarSectionOrderTests and extend Workspace state/model tests.
…parameterise tests
![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: a22447c7d7
ℹ️ 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".
…mantics CI (Xcode 26.0.1) rejected `Array(remaining[..<i] + moved + remaining[i...])` in `moveSections` — `ArraySlice + Array` failed overload resolution there even though the newer local toolchain accepted it. Replaced with an explicit `insert(contentsOf:at:)`. While rewriting the line, fixed the offset convention it encoded. SwiftUI's `ForEach.onMove` passes `toOffset` as an insertion point in the *pre-move* ordering, but the old code applied it to the post-removal array. The two disagree whenever an item moves down past another, so `[a,b,c]` moving index 0 to offset 2 produced `[b,c,a]` instead of `[b,a,c]`. Invisible today because `SidebarSection` has two cases, but D10 explicitly plans for a third. - Extract `Workspace.reorder(_:fromOffsets:toOffset:)` — a pure, generic helper that follows the onMove convention, ignores out-of-range sources and clamps the destination. `moveSections` is now a call plus the write-through. - Add `ReorderTests`: 10 cases over 3- and 4-element collections, the only place the convention is observable while the enum has two cases. Also resolves both Codex P2 findings on the plan: - Ordinal-keyed UI state (MacDownApp#279): intersecting collapsed/selected ids with the new tree's `allIDs` is not reconciliation — after an insert above, the stale ordinal still exists and the collapse slides onto the next section down. D4 now specifies `OutlineIdentityMap.remap` ((level, title) nearest-ordinal, each new ordinal claimed once, unmatched ids dropped) ahead of the intersection, with its own file, tests and step in the implementation order. - Stale source map for the current section (MacDownApp#403): the caret offset is live but the `SourceMap` is up to one debounce old, so translating at the event site froze a line computed against the previous document and the tint stuck until the next editor event. D5 now stores a UTF-16 *offset* and re-translates it inside `update(...)` with the map that just arrived; `referenceLineDidChange(_:)` becomes `referenceOffsetDidChange(_:)` and the app forwards `range.location` raw. Validation: swift build + 351 tests pass; swiftformat --lint and swiftlint --strict clean; app, CLI and build-for-testing all succeed locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things, both from the final review pass.
1. Land the clamp §2.3 promised. `scrollToVisible(utf16Range:)` forwards a
range built from the parsed document's `SourceMap` into a text view holding
the live text; those disagree for up to one debounce interval. It now
clamps via `clampedToLiveText`, measured with `NSString.length` (UTF-16 code
units) rather than `String.count` (Characters), which under-clamps on emoji
and CJK.
2. Correct why it lands. The plan asserts the unclamped range "raises
NSRangeException, which is not catchable in Swift", and schedules the fix as
implementation step 1 on that basis. **It does not reproduce.** Probed on
macOS 26.6 with the text view hosted in a real NSWindow and layout ensured,
`scrollRangeToVisible(_:)`, `setSelectedRange(_:)` and
`showFindIndicator(for:)` were each given a range starting past the end of
the text; all three clamped internally and returned normally
(`setSelectedRange` reported {length, 0}). The first version of the test
here was written as a crash regression and passed with the clamp removed —
which is how the claim got caught.
The clamp still earns its place: it makes the seam's behavior defined by
this code rather than by an undocumented AppKit detail, and it fixes the
unit bug a hand-rolled clamp would otherwise have. But it is hardening, not
a defect fix, and §2.3 / §4.4 / §7 / §8 now say so — including a dated
correction block, so no one re-derives the crash claim from the plan.
Tests assert the clamp arithmetic per case (end-collapse, length-truncation,
empty document, and an emoji/CJK document where a Characters-based clamp would
land mid-document), not the absence of a raise.
Validation: 353 tests pass; swiftformat --lint and swiftlint --strict clean;
app, CLI and build-for-testing all succeed locally. Prior commit's CI run is
green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Final review — CI fixed, Codex findings resolved, not ready to mergeCI was red on The blocker is scope, not quality: this branch does not contain EPIC-08. Details in §4. 1. The CI failure
order = Array(remaining[..<targetIndex] + moved + remaining[targetIndex...])
2. What the fix uncovered — wrong
|
[a, b, c], move {0} → offset 2 |
result |
|---|---|
| SwiftUI convention | [b, a, c] |
| as written | [b, c, a] |
Invisible today — SidebarSection has two cases and every existing test case coincides under both readings — but D10 of the plan explicitly designs for "a future epic adding a third section", and moveSections is public API whose whole purpose is to be handed to .onMove. It would have shipped as a latent reorder bug that appears the day a third section lands.
Extracted Workspace.reorder(_:fromOffsets:toOffset:) — pure, generic, follows the convention, ignores out-of-range sources, clamps the destination. moveSections is now that call plus the write-through. Added ReorderTests: 10 cases over 3- and 4-element collections, which is the only place the convention is observable while the enum has two cases. All six pre-existing MoveSectionsCase expectations were already correct under the new semantics and are unchanged.
3. Codex findings — both valid, both fixed in the plan
#279 — ordinal state on reparse. Correct. update(...) said collapsed/selected ids are "reconciled against OutlineTree.allIDs", which only handles the tree getting shorter. Insert a heading above a collapsed node and every old ordinal still exists — the intersection drops nothing and the collapse slides onto the next section down. D4 now specifies OutlineIdentityMap.remap ((level, title) nearest-ordinal, each new ordinal claimed once, unmatched dropped) ahead of the intersection, with its own file, tests, and step in the implementation order. Open decision #4 now separates cross-relaunch persistence (still deliberately not done) from in-session remap (now required) — the old wording could be read as excusing both.
#403 — stale SourceMap for the current section. Correct, and its suggested fix is the one taken. D5 becomes "one reference offset": referenceLineDidChange(_:) → referenceOffsetDidChange(_:), the controller stores the raw UTF-16 offset and translates it through the newly published document.sourceMap inside update(...). The app forwards range.location raw and never calls sourceMap.line(atUTF16Offset:) — strictly less work at the call site than the original wiring.
4. ⚠️ The branch is missing most of what the PR describes
| Promised | State |
|---|---|
| Architecture plan | ✅ c7b8c15 |
| Defect #1 — sidebar persistence | ✅ c6c7dcb |
Defect #2 — clamp in EditorTextSystem |
✅ now, in df2610b (was absent) |
OutlineUI: OutlineItem, OutlineTree, OutlineSelection, OutlineIdentityMap, OutlineAvailability, OutlineController |
❌ not started |
SidebarView rewrite, ContentAreaView wiring, WindowController, ⌃⌘O |
❌ not started |
OutlineNavigationUITests |
❌ not started |
SidebarView still hardcodes two Section blocks and renders Text("Outline will appear here"). sectionOrder and moveSections have no call site — acceptance box 4 ("section order persists across relaunch") is not reachable from the UI, because nothing can reorder anything yet. Steps 3–11 of the plan's own implementation order are untouched.
I have not implemented them: that is the epic, and it is the implementer's half of the workflow, not a review fix. Merging now would land E08 as plan + one defect fix, with master's README claiming the content browser is "in progress" and nothing in the sidebar. Say the word if you'd rather merge it as a checkpoint anyway and carry the outline on a follow-up branch — that is a reasonable call, just not one I'll make silently.
5. Correction to the plan: defect #2's crash does not exist
§2.3 asserted the stale range "reaches NSTextContentStorage and raises NSRangeException, which is not catchable in Swift", and scheduled the fix as implementation step 1 on that basis.
It does not reproduce on the deployment target. Probed on macOS 26.6 with the text view hosted in a real NSWindow and layout ensured — scrollRangeToVisible(_:), setSelectedRange(_:) and showFindIndicator(for:) were each given a range starting well past the end of the text. All three clamped internally and returned normally; setSelectedRange reported {length, 0} afterwards.
I caught this because the first version of my regression test passed with the clamp removed. Worth noting that had this landed as written, the branch would have carried a test whose comment claimed to prove a crash it never exercised.
The clamp landed anyway, with the justification restated: it makes the seam's behavior defined by our code rather than by an undocumented AppKit detail, and it fixes the unit bug a hand-rolled clamp would otherwise have had (String.count is Characters; NSRange is UTF-16 code units — a count-based clamp cuts a 22-unit emoji/CJK document to 17 and lands mid-document). §2.3 now carries a dated correction block, and §4.4 / §7 / §8 are reworded so nobody re-derives the crash claim. The tests assert clamp arithmetic per case and say in their own comment that they are not crash regressions.
6. Accepted as-is (noted, not changed)
- Sidebar state is now app-wide. Deleting
NoOpStateStoremakes visibility / expansion / order shared across windows via theUserDefaultssuite. This is open decision [EPIC-00] Project foundations: Xcode 26 project, SPM modules, CI #1 and the PR flags it as wanting a call — it landed without one, which is fine given it replaces state that never persisted at all, but it is a real behavior change and worth an explicit yes. - Cross-window caches go stale. Each window's
WorkspaceModelhydratessectionOrder/sectionExpandedat init and never re-reads. Window A reorders → window B keeps the old order, and if B then reorders, it writes its stale list back over A's. Not a regression (nothing persisted before) and documented as "last writer wins", but the fix is cross-window change notification, which is the deferred decision, not a one-liner. Flagging so it isn't discovered as a surprise later. reconciletotality, the expansion cache, thesidebarSectionOrderJSON round-trip, and their tests all read correctly — no findings.
🤖 Review pass by Claude Opus 5. Commits: 2ca62cb (CI fix + onMove semantics), df2610b (clamp + plan corrections).
|
CI green on Holding rather than merging, per §4 above: the branch stays open and green, and the outline implementation lands here as the PR body describes. Branch is ready for the implementation hand-off — the two prerequisite defect commits are both in place now, so steps 3–11 of the plan can start from a clean base. Note the plan changed under §§2.3, 4.3, 4.4, 4.7, 5, 7, 8, 10 in this pass; re-read D4, D5 and the §2.3 correction before starting. |
…cument Fills the OutlineUI module and wires it end to end, per the plan already on this branch. No parsing of its own (D2): the outline is a pure readout of the same MarkdownParseSession the preview already reads. New in OutlineUI (all pure, headless-testable, MarkdownEngine-only per D1): - OutlineItem — a tree node; id is the heading's flattened ordinal (D4). - OutlineTree — stack-fold build() (D3: skipped levels attach to the nearest shallower ancestor, level stays display-only), visibleItems() (collapse- aware depth-first flatten), allIDs(), item(withID:). - OutlineSelection — binary-search currentItemID(forLine:in:) (D5). - OutlineIdentityMap — remaps ordinal-keyed UI state (collapse, selection) across a re-parse by nearest (level, title) match, so an insert/delete above a collapsed heading doesn't slide the collapse onto a neighbor. Intersecting with allIDs alone (as an earlier draft of the plan had it) is not reconciliation — it only catches ids that vanished outright. - OutlineAvailability — four-case gate (notParsed/unsupportedFormat/ noHeadings/ready), computed at the app layer from a format verdict so a non-Markdown file's `# comment` lines never populate the tree, only maybe the label (D7/§2.4). - OutlineController — per-window, @mainactor @observable. update(document: isMarkdown:formatName:) no-ops on an unchanged revision+availability; referenceOffsetDidChange(_:) stores a UTF-16 offset and re-translates it through whichever SourceMap is current on every rebuild, so the tint never freezes on a stale parse (D5). activate(_:) targets the heading line only, not the full (possibly Setext) range. App wiring: - EditorTextSystem.revealSelection(utf16Range:flash:) — select, ensure layout, scroll, then showFindIndicator(for:) (D9). Reuses the clamp from the earlier defect-#2 hardening commit. - WindowController owns one OutlineController per window (D8), passed through WorkspaceShellView to both SidebarView and ContentAreaView. - DocumentEditorSplitView (split out of ContentAreaView.swift to stay under the file-length lint budget): refreshes the outline alongside the preview on every parse; wires EditorView's already-existing onSelectionChange and the existing scroll callback into referenceOffsetDidChange, forwarding raw UTF-16 offsets rather than translating with a possibly-stale SourceMap; consumes pendingJumpLineRange into revealSelection with flash: true. - SidebarView rewritten: ForEach(model.sectionOrder) + .onMove drives section reordering (D10, wired to a real call site for the first time); each section keeps its DisclosureGroup; outline rows are a flat, depth-annotated traversal (indent by tree position, D3) with a disclosure chevron per non-leaf row, List(selection:) bound to selectedItemID (D6 — a distinct channel from the current-section tint), ⌃⌘O focus via focusRequestID. - WorkspaceCommands: "Focus Outline" ⌃⌘O next to Toggle Sidebar. - WindowCoordinator.focusOutline() (split into WindowCoordinator+SessionRestore.swift alongside the pre-existing restore pipeline, to stay under the type-body-length lint budget): reveals the sidebar and the outline section before requesting focus, since focusing a hidden list is a dead shortcut. Tests: 45 new OutlineUI-package tests (tree building incl. a 5k-heading corpus, traversal/collapse, selection binary search, identity remap across inserts/deletes/retitles, controller gating/reconciliation/no-op/jump/ reference-offset re-translation) plus OutlineNavigationUITests (⌃⌘O reveals rows, arrow+Return jumps the editor). Verified interactively via screenshot: typing headings populates the sidebar outline live, and the section under the caret renders bold — this is what caught an accessibility-identifier placement bug the first automated UI test run surfaced (the identifier needs to sit at the ForEach call site, not buried inside the row's own body). Validation: 397 package tests pass (45 new); swiftformat --lint and swiftlint --strict clean; app, CLI, and build-for-testing all succeed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
EPIC-08 implementation landed — CI green, ready for review
What shipped
App wiring:
Tests: 45 new ( A bug the process actually caughtFirst automated UI-test run failed both row-visibility assertions. Direct interactive verification (screenshot, typing headings into a live build) showed the feature working correctly — rows populate, the current section renders bold as the caret moves — which narrowed it to an accessibility-identifier placement bug: RecommendationReady to merge once you're satisfied — all three CI gates pass, both Codex findings are resolved in both the plan and the implementation, and the feature works end-to-end interactively. The one open item is the local UI-test re-run noted above. 🤖 Implementation pass by Claude Opus 5. |
`referenceOffsetDidChange` (fired from EditorView's onSelectionChange, i.e. on every keystroke as the caret advances) unconditionally assigned `currentItemID`. @observable sends a change notification on every *assignment*, not every *value change*, so this re-rendered SidebarView — including its collapse-aware tree traversal — on every keystroke in the document, even for the overwhelming majority where the caret stays under the same heading and nothing in the outline actually changes. Guard both write sites (referenceOffsetDidChange and update(...)) behind an equality check before assigning. 397 tests still pass; behavior is unchanged, only the redundant-notification elimination is new. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding on my own implementation commit. `OutlineTree.visibleItems` was public API with four tests and *zero* production callers, while `SidebarView.visibleRows` reimplemented the same collapse-aware walk inline — and its doc comment claimed it delegated to the module it was duplicating. So the tested copy was dead and the rendered copy was untested. - `visibleItems` → `visibleRows`, returning `[OutlineRow]` (item + tree depth). Depth belongs in the module: it is a property of the tree, and moving it there means the traversal the sidebar renders is the one the tests cover. - `SidebarView` calls it instead of walking the tree itself; the local `visibleRows` computed property and its nested `visit` are gone. - Added the depth assertions that were missing at the row level, including the skipped-level case: H1 → H3 must indent ONE step (depth [0, 1]) while `level` stays [1, 3]. That is D3's core rule and nothing pinned it before — indenting by `level` would have passed every prior test. 399 package tests pass; swiftformat --lint and swiftlint --strict clean. App-target build deferred to CI (local xcodebuild stalls in destination enumeration in this environment; the only app-target change is mechanical — same property names as the tuple it replaces). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Final review — recommend merge, with one manual check firstCI green on Findings from this pass (both fixed)1. The tested outline traversal wasn't the one being rendered. Fixed in That also surfaced a genuine coverage hole: nothing pinned depth at the row level, so indenting by 2. Redundant Verified clean
|
| Issue | Why not here |
|---|---|
| #34 — sidebar layout caches go stale across windows | Unresolved tail of open decision #1. The naive fix (re-read the store in moveSections) is wrong — drag offsets index the window's own rendered list. Real fix needs cross-window propagation, and the app-wide-vs-per-window decision has to land first. Not a regression: nothing persisted at all before E08. |
| #35 — non-Markdown files still fully Markdown-parsed every keystroke | E08 gates the outline correctly (test pins it); gating the parse changes what E07's preview receives. An E11 concern, flagged in the plan as §10.6. |
| #36 — verify outline interaction paths end-to-end | Needs interactive/UI-test execution this environment can't provide. |
Two smaller notes, not worth issues: OutlineIdentityMap.remap is O(collapsed × candidates) worst case with many duplicate-titled headings (bounded by user collapse count in practice), and Debug-vs-Release is being handled separately.
Recommendation: merge
Everything is additive — a new module plus a sidebar rewrite. CI is green three pushes running, the two defect fixes carried on this branch are independently valuable, and both Codex findings are resolved in the plan and the implementation. Holding the branch longer doesn't buy the one thing still missing, because only manual interaction can provide it.
Before you merge, spend two minutes on: drag a section header to reorder, click an outline row, press ⌃⌘O. If drag-reorder turns out dead, that's a follow-up under #36 — it's contained to SidebarView and independent of everything else on the branch.
🤖 Review pass by Claude Opus 5. Commits 89b3992 (keystroke thrash), 65e208b (traversal dedup + D3 depth coverage).
…ke ⌃⌘O visible Both fixed after interactive confirmation on device (Debug build) surfaced what CI and the package test suite could not: drag-reorder showed the insertion-line affordance but the drop never applied, and ⌃⌘O had no noticeable effect. **Drag-reorder.** `.onMove` on a `ForEach` of plain rows gives macOS native drag reordering; applied to a `ForEach` producing `Section`s — which this sidebar needs, since each section is independently collapsible — it is an unsupported construction, and the confirmed behavior (affordance shown, drop silently no-ops) matches that. Rather than keep fighting undefined List behavior, reordering is now two explicit chevron buttons per section header, calling the same `model.moveSections(fromOffsets:toOffset:)` the removed `.onMove` did — the `Workspace.reorder` semantics and its 16 tests are unchanged, only the trigger changed from an unreliable gesture to a deterministic tap. Buttons disable at the ends of the list. **⌃⌘O.** `requestFocus()` only bumped a counter that moves keyboard focus onto the List via `@FocusState`. A focused-but-unselected List has no visible highlight on macOS, so from the user's side the shortcut did nothing observable. `requestFocus()` now also selects a row when nothing is already selected — the current section if one is tracked, else the first row — so the shortcut visibly does something and the next arrow-key press has somewhere sensible to move from. Three new tests cover: selects current section, falls back to first row, never steals an existing selection. 402 package tests pass (up from 399); swiftformat --lint and swiftlint --strict clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed both issues from the manual test pass, in Drag-reorder — confirmed dead as suspected in #36: ⌃⌘O — had no visible effect because it only moved keyboard focus onto an unselected 402 tests (+3 for the new Issue #36 updated with what's still unconfirmed: click-to-jump, arrow+Return keyboard jump, and the find-indicator flash. Recommend the same quick manual pass on those before merging, now that focus/selection actually lands somewhere. |
The preview inherited SwiftUI's default `.body` font (~13pt, sized for UI chrome, not sustained reading) and Textual's default paragraph line-spacing (0.23x font size) — a noticeable outlier next to the library's own other block styles (block quote 0.471x, code block 0.39x). Since paragraphs are most of a typical document, that mismatch is what reads as "the whole preview is cramped" rather than one odd block. - Base font: 15.5pt, set once at the top of the block stack. Textual scales headings/code/spacing relative to whatever font is ambient, so this one number scales the whole hierarchy proportionally rather than just body text. - Paragraph line spacing: 0.23 → 0.42, in line with the rest of Textual's own scale (not an arbitrary new value). Headings, code blocks, block quotes, lists, and tables keep Textual's defaults untouched — only the two values that were inconsistent with the rest of the system changed. 402 tests pass; swiftformat --lint and swiftlint --strict clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…spacing Root cause of "scrolling completely doesn't work on raw editor" (both manual scroll wheel and keyboard-driven caret navigation): the text view's frame height was never being grown past roughly one viewport's worth of content. `EditorView.makeNSView`'s one-time `NSString.boundingRect` estimate ran before AppKit had laid out the scroll view (so at an unreliable width), and nothing afterward ever corrected it — `isVerticallyResizable = true` alone does not keep a *manually-constructed* TextKit 2 stack (custom `NSTextContentStorage`/`NSTextLayoutManager`/`NSTextContainer`, not the `NSTextView(usingTextLayoutManager:)` convenience initializer) in sync with content outside the viewport-lazy layout pass. `scrollRangeToVisible` and keyboard caret-follow both had nowhere further to go once the frame stopped growing, which read as "scrolling doesn't work" even though the selection was genuinely moving (confirmed via the outline's current-section tracking). Root-caused with a temporary os_log instrumentation pass, run interactively against the live app via Console.app and Xcode's debug console — this is not guessed at. Fix, in `EditorTextSystem`: - `syncFrameHeightToContent()`: forces one full-document layout pass via `NSTextLayoutManager.enumerateTextLayoutFragments(from:options:[.ensuresLayout])` and applies the real content height. `usageBoundsForTextContainer` was tried first and measured to not reflect a forced `ensureLayout(for:)` pass for this manually-built stack — it stayed pinned near viewport height regardless. Skipped for documents ≥ 100 KB (matches the existing `makeNSView` threshold) and no-ops via a (text length, width) signature once the frame has already caught up. - Applied as a **watermark**, never shrinking: TextKit 2's viewport layout controller reclaims fragments outside the visible area as its own housekeeping, so a later enumeration of the same unchanged document can legitimately report a smaller `maxY` than an earlier one — not because the document got shorter. Applying that directly intermittently shrank the frame back to viewport size and reintroduced the bug; this is what several earlier attempts this session actually hit. - `ensureSelectionVisible()`: hooks `NSTextViewDelegate`'s selection-change callback (fires on every caret move, including keyboard navigation, which — unlike SwiftUI-driven jumps — never flows through `updateNSView`). Deferred one run-loop turn: the delegate notification fires *during* `NSTextView`'s own keyboard handling, which does its own scroll-to-caret immediately afterward using its own (still-short) idea of the frame; correcting synchronously just loses that race. Landing the fix on the next turn, after AppKit's own handling has finished, is what makes it stick. - `scrollToVisible`/`revealSelection` (the outline's jump) now call `syncFrameHeightToContent()` before scrolling, for the same reason. Preview: heading-to-body spacing. `DefaultHeadingStyle`'s bottom block spacing (0.8× font size) read as too tight against a paragraph's own top spacing (1.0×, from the prior typography pass) — a heading needs to visually separate from what follows it more than a paragraph needs to separate from the next paragraph. Added `PreviewTypography.HeadingStyle`, identical to Textual's default (same per-level font scale and line spacing, copied since the library doesn't expose those constants for reuse) except bottom spacing raised to 1.2×. 402 tests pass; swiftformat --lint and swiftlint --strict clean project-wide. Verified interactively: arrow-key scroll now reaches document end, outline jump works in both directions, typing responsiveness unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All four issues fixed in 1. Scroll didn't work at all on the raw editorRoot cause: the text view's frame height was never growing past roughly one viewport's worth of content. Root-caused with a temporary
2. Preview heading-to-body spacingTextual's default heading bottom-spacing (0.8× font size) was a real outlier against the paragraph top-spacing from the earlier typography pass (1.0×) — a heading needs to separate from what follows it more than two paragraphs need to separate from each other. Raised to 1.2×, same per-level font scale/line-spacing as Textual's default otherwise. 3. Drag-reorderAlready fixed on this branch ( 4. Click/Return jump-to-headingWas blocked by #1, not a separate outline bug — both the outline's jump and normal keyboard auto-scroll call the same Verified interactively end to end: arrow-key scroll reaches document end, outline jump works both directions, typing responsiveness unaffected by the fix, reorder buttons work. 402 tests pass, lint clean. |
…ole errors Six issues from manual testing. The scroll ones (1/3/6) were a regression I introduced in eb2b94f. **Scroll bounced back to a point mid-document (and jump-to-heading was erratic).** `ensureSelectionVisible()` called `scrollRangeToVisible(selection)` on *every* selection change, one run-loop turn later. Scrolling away from the caret and releasing therefore snapped straight back to wherever the caret happened to be — the "strange location". That scroll was never needed: `NSTextView` already follows the caret on its own; all the earlier fix actually needed was a frame tall enough to have somewhere to scroll to. Renamed to `scheduleFrameHeightSync()` and it now only corrects the frame. Also hooked `textDidChange` so typing keeps the frame in step, which is what the removed scroll was accidentally covering. **Editor↔preview sync oscillated.** `ScrollSyncController`'s echo latch was one-shot: it recorded the block the other pane was asked to move to and cleared the record on the first matching report. But one user gesture produces *many* reports — SwiftUI's `onScrollGeometryChange` fires throughout a scroll and AppKit posts a bounds-change notification per frame of a smooth scroll — so it absorbed the first echo and let every subsequent one through as a genuine scroll command, and the two panes fought. The latch is now sticky: held until a genuinely different block is reported. Regression test `repeatedEchoesAtTheSameBlockAreAllSuppressed` fails against the old latch (4 escaped echoes per direction), passes now. **Console errors.** Two were ours, both firing continuously: - `No symbol named 'richtext' found in system symbol set` — `documentIcon(for:)` returned `"richtext"` for Markdown, which is not an SF Symbol. Verified against the live symbol set: `richtext` misses, `doc.richtext` resolves (as do all our other symbol names). Fixed. - `Bound preference BlockFramePreferenceKey tried to update multiple times per frame.` — every preview block wrote its own entry into one dictionary-valued `PreferenceKey`, all reduced together, and the resulting write triggered the re-layout that wrote it again. Replaced with per-block `onGeometryChange`, outside the preference system entirely; only the height was ever used. `BlockFramePreferenceKey`, `updateBlockHeights` and the now-unused `previewContent` coordinate space are deleted. Verified with `log stream --predicate 'process == "MacDown2"'` while exercising both paths (heading + paragraph + list + block quote): 0 hits for each across 231 captured lines. Everything still logged is system-framework noise (linkd, ViewBridge, CFPrefsD, MobileGestalt), not ours. Xcode's Issue Navigator is empty — there were never build errors, only these runtime ones. **Preview headings were oversized and still tight.** Textual's default scales top out at 2.353×, which against the 15.5pt base gave a ~36pt H1 — sized for a wide article column, not a side-by-side pane. Retuned to top out at 1.55× (~24pt H1, ~20pt H2) and gave headings more room below them. Not changed: outline rows are not drag-reorderable (they mirror document order); only the Folder/Outline *sections* reorder. 403 tests pass; swiftformat --lint and swiftlint --strict clean project-wide; app builds. Verified interactively: scroll to bottom and to top both hold position with no bounce, headings render proportionally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both from manual testing, and both were bugs I introduced in the previous two commits rather than pre-existing ones. **Editor snapped to the bottom and would not stay scrolled elsewhere.** `ScrollSyncController.blockIndex(forContentOffset:)` accumulates measured block heights until the running total passes the reported offset. Block heights are measured asynchronously by the preview, so they are all zero before its first layout pass — and with every height zero the running total never passes any offset, the loop falls through, and the final `return` names the LAST block. The editor faithfully obeyed that and scrolled to the end of the document, on every preview scroll report. I made this the normal case rather than a startup transient: the `onGeometryChange` refactor in 29f1d33 cleared the whole height dictionary on every `displayBlocks` change (i.e. every debounced re-parse), and `onGeometryChange` only re-fires when a block's height actually *changes*, so the heights stayed zero indefinitely. - `blockIndex(forContentOffset:)` now returns nil when total height is zero. There is genuinely no mapping available yet; saying so is correct, and guessing "the last block" is the worst possible guess. - The preview prunes only the stale *tail* of the height dictionary on a re-parse instead of wiping it, and re-publishes the surviving heights. Regression test `unmeasuredBlockHeightsDoNotResolveEveryScrollToTheLastBlock` fails against the unguarded resolver (returns line 5, the last block) and passes now. **Preview was "extremely cramped" — wrong root cause the first two times.** I had been tuning Textual's `blockSpacing`, which cannot work here: `TextualMarkdownPreview` slices the document into one `StructuredText` *per block* (so scroll sync can measure each block independently) and stacks them in a `VStack`. Textual's block spacing separates blocks *within a single* `StructuredText`; between separate sliced views it does nothing. The real gap was the `VStack`'s `spacing: 0`, so every `blockSpacing` value I set in the last two commits was a no-op. Inter-block spacing now comes from `PreviewTypography.gapAbove(_:)`, applied as top padding on each block and varied by block kind (headings get ~1.6em, body blocks ~0.95em). Applied *above* the `onGeometryChange` measurement so the gap is inside the frame the scroll map sees and the sync math stays exact. The dead `blockSpacing` calls are removed from both styles rather than left in place looking meaningful. 404 tests pass; swiftformat --lint and swiftlint --strict clean; app builds. Verified by running the built app directly: scrolling up now holds position instead of snapping to the end, and block spacing is visibly separated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eel scroll Continuous mouse-wheel scrolling fires editorDidScroll/previewContentOffsetDidChange many times per second. Each call overwrote the echo-suppression latch with the newest requested block before an older request's async echo (a SwiftUI scrollTo/AppKit bounds-change round trip) came back. Once that echo named a block that no longer matched the latch, the exact-match-only check let it through as a "genuine" scroll and re-targeted the other side to the stale, already-passed block — visibly fighting the user's still-active wheel gesture. Fix: remember every block requested (not just the latest) for a short window, so a stale echo of a superseded-but-still-in-flight request is still recognized regardless of which specific block the current latch points to. Suppression must still key on the specific requested block, not "any report in the window," so a genuine scroll to a block never requested is not swallowed just because it lands inside the same window. Split the growing echo-suppression regression tests into their own file to stay under the repo's file/type-length lint limits.
…l-sync bounce Outline sidebar heading jumps now animate smoothly on both editor and preview sides, driven from the same source line rather than one side deriving its target by reading back where the other landed — that round trip made a large jump fragile against its own animation's intermediate scroll reports. The outline's "current heading" highlight no longer goes stale after a jump: it was being overwritten by the animation's own in-flight scroll frames. The preview can now reveal content below a block's top edge instead of always re-snapping to it, without reintroducing the height-corruption bug an earlier SwiftUI ScrollPosition-based attempt caused. The proportional scroll anchor this needed is only applied when a block actually overflows the viewport — otherwise scrollTo's own anchor formula spills the resulting offset into the previous block, defeating echo suppression and bouncing the editor backward on ordinary scroll-sync ticks. Fixes the remaining scroll-sync bounce, root-caused to a lossy fraction round trip (divide by totalHeight, then multiply back) used to recover which block the preview should scroll to; ScrollSyncController now publishes the exact block and within-block progress alongside the fraction so the view never needs to re-derive it. Also fixes the outline sidebar's row hit target only covering its content (chevron + text) rather than the full row width. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
EPIC-08 — Content browser: heading outline of the active document
Refs #9 · Milestone M3 — Markdown core · Depends on E06 + E02/E03 (all merged)
Status: architecture pass only — no implementation yet. This PR carries the plan; the implementation, review, and fixes all land on this same branch. Same shape as #31 and #32.
The plan in one paragraph
E06 already did most of this epic's engine work and said so:
HeadingItemships with the comment "flattened for E08", andMarkdownDocument.headingsis already complete, plain-text titled, code-block-free (E06 has the passing test), and expressed in original-source line numbers with front matter already accounted for. So E08 adds no parse, no debounce, no store, no actor, and no source-range index — the outline is a pure function over the sameMarkdownParseSessionthe preview already reads. That is what makes acceptance box 2 ("updates within the same 150 ms debounce as the preview") true by construction rather than by luck, and the test plan asserts the absence of a second pipeline rather than timing one. Likewise box 3 ("switching tabs swaps outline instantly, cached per tab"): under the #28 native-tabs amendment a tab switch is a window switch, and each window already owns its own model, parse store, and view hierarchy — there is nothing to recompute. The newOutlineUIcode is four small value types plus one@MainActorcontroller: a stack fold that turns the flat heading list into a tree (skipped levels attach to the nearest shallower ancestor; nesting is tree position,levelstays display-only), a binary search that answers "which section is line N in", a four-case availability enum so every empty state has a name, and a per-window controller that owns collapse state, the navigation cursor, and a consume-and-clear jump request. No new dependencies, and noPackage.swift,project.ymlorci.ymldiff — theOutlineUItarget, its product entry, its test target, and its app-target dependency all already exist from E00 scaffolding.Two defects on
masterthat block this epic's acceptanceBoth are in scope because E08 cannot meet its criteria without them, and both land as their own early commits so they are reviewable in isolation.
1. Sidebar state has not persisted since E03.
WorkspaceStateStore(E02) persists sidebar visibility and section expansion toUserDefaultsand is fully tested — butWindowCoordinator.makeWindowModel()injectsNoOpStateStore(), added by E03's native-tabs rewrite to stop windows fighting over one store. E02's persistence has been dead code ever since; nothing about the sidebar survives relaunch today. E08's acceptance box 4 is "section order persists across relaunch", which is unreachable while the app writes to a no-op. The fix treats sidebar layout as an app-wide user preference (each window's live toggle stays independent; the persisted copy is shared, which is what theUserDefaultssuite already gives us) and deletesNoOpStateStore.NoOpSessionStorestays — it looks alike and is load-bearing.2.
scrollToVisible(utf16Range:)can raise on a stale range. It forwards an unclampedNSRange— built from the parsed document'sSourceMap— into a text view holding the live text. Those disagree for up to one debounce interval, so deleting a trailing section and then triggering a sync inside that window produces an out-of-bounds range, andNSRangeExceptionis not catchable in Swift. E07 can already hit this via preview→editor scroll sync, but only if you happen to be scrolling the preview as you delete. E08 makes it easy: an outline row is one click away at all times, and clicking one right after a big edit is the natural thing to do. Fixed inside the seam so both paths are covered, with a regression test that deliberately drives a too-long range through the live method.Also recorded
ContentAreaViewcallsparseNowfor every open document regardless of format, so a Python or shell file has a populatedheadingsin which every# commentis an H1. E08 therefore gates the outline on format before building the tree, not just before picking a label — which is the same mechanism the issue's "graceful empty state for non-Markdown formats" asks for. Whether those files should be parsed at all is an E11 question; flagged, not fixed here.List(selection:)is the user's arrow-key cursor; the current section follows the caret and changes on every keystroke. Binding them together would yank the user's navigation position out from under them mid-typing.NSTextView.showFindIndicator(for:)— AppKit's own Find callout. One line, respects Reduce Motion for free, no custom overlay.Open decisions for review
session.json, notUserDefaults— bigger, and out of scope here. Wants a call before implementation starts.> # quoted headingin a review snippet becoming a top-level outline entry is debatable for a navigation aid. Plan includes them; if review disagrees the filter belongs inOutlineTree.build, not in E06.showFindIndicatorunder TextKit 2 needs laid-out geometry; the plan specifies ensure-layout → scroll → flash. To be confirmed on a large document when the implementation lands.Validation
Nothing to build yet — this commit is the plan plus a README status line.
masterwas verified clean before branching (swiftformat --lintandswiftlint lint --strict: 0 violations at1be58c9), so unlike E07 there is no pre-flight lint commit.Full validation gate for the implementation commits is in §9 of the plan.
🤖 Architecture pass by Claude Opus 5. Implementation and review notes will follow as inline comments on this PR.