Skip to content

Add headings navigator - #423

Open
yusufm wants to merge 1 commit into
schuyler:mainfrom
yusufm:Codex/add-headings-navigator-019def52-b3da-7962-96cd-afa05d8d1592
Open

Add headings navigator#423
yusufm wants to merge 1 commit into
schuyler:mainfrom
yusufm:Codex/add-headings-navigator-019def52-b3da-7962-96cd-afa05d8d1592

Conversation

@yusufm

@yusufm yusufm commented May 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a toolbar and View menu headings navigator
  • parse ATX and setext headings while skipping fenced code blocks
  • jump the editor to a selected heading from the navigator

Tests

  • xcodebuild test -workspace "MacDown 3000.xcworkspace" -scheme MacDown -only-testing:MacDownTests/MPToolbarControllerTests -only-testing:MacDownTests/MPDocumentLifecycleTests -destination 'platform=macOS'\n\nRelated to Suggestion: Headings Navigator #406

@schuyler schuyler left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR, @yusufm!

Issues

  • CRLF line endings: headingsInMarkdown: splits on \n without normalizing \r\n, so files saved with Windows line endings accumulate offset drift and jumpToHeading: lands in the wrong place (titles also capture a trailing \r).
  • Thematic break read as a heading: a --- horizontal rule after a paragraph is recorded as a setext H2, producing spurious navigator entries and wrong scroll targets, where the existing updateHeaderLocations guards this with an HR check.
  • location is unverified: the offset jumpToHeading: depends on is never asserted in the new tests, so a regression in offset accounting would pass unnoticed.
  • No multi-byte coverage: no test uses non-ASCII headings, leaving UTF-16-vs-byte offset accuracy unguarded.
  • Second heading parser: this adds a regex heading parser alongside MacDown's existing Hoedown-based heading handling, creating two sources of truth that can diverge on edge cases.

Suggestions

  • Typed heading model: a small value object instead of the stringly-typed NSDictionary (@"level"/@"title"/@"location") would catch key typos at compile time.
  • Localization: the new NSLocalizedString keys aren't yet in en.lproj/Localizable.strings, the source translators work from.
  • Toolbar icon: the new button reuses NSImageNameListViewTemplate, already used by the "table" item, making the two adjacent buttons hard to tell apart.
  • Setext underline: advancing past the ===/--- underline after detecting a setext heading would avoid reprocessing it.

Generated by Claude Code

@schuyler schuyler left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks @yusufm — this is a clean, well-tested first cut, and the headingsInMarkdown: parser is nicely self-contained. A few notes before merge:

1. Frontmatter false positive (would like this fixed first). The setext branch treats any non-empty line followed by a line of =/- as a heading. That means a closing YAML frontmatter --- turns the last frontmatter line into a phantom entry in the navigator. Since we hide front matter in the preview (#307, #312), it would be inconsistent to list it here — worth skipping a leading --- … --- block before parsing.

2. UX vs. the request (judgment call). #406 asked for a persistent, GitHub-style TOC panel kept alongside the editor, with optional filtering. This implements a transient pop-up NSMenu instead. That's a reasonable MVP and delivers the core jump-to-heading value, but a flat menu gets unwieldy for the long documents this feature is meant to help with. I'm fine shipping it as v1 if we file the panel + filtering as a follow-up — just flagging that it doesn't fully match the issue as pictured.

3. Minor (no action needed). Character offsets assume LF, which is safe now that #398 normalizes CRLF on load. The menu-bar invocation path (where sender is an NSMenuItem and falls back to the editor view) is handled correctly.

I'd like #1 addressed before merge; #2 is a call on how faithful to #406 we want the first version to be.


Generated by Claude Code

@schuyler schuyler left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Following up after a closer look — two things I'd want sorted before this lands, on top of the front-matter note above:

1. Toolbar indices are computed against a stale base. The diff moves flexibleSpaceAfterIndices from {2,3,5,7,11} to {3,4,6,8,12}, but current main is {2,3,5,8,12} — the table toolbar item (#420) landed after this branch and already shifted the boundaries. Applied as-is, the flexible spaces land in the wrong spots and several MPToolbarControllerTests fail (the count and exact-order assertions). This needs a rebase on main and a recompute — I believe it works out to {2,3,4,6,9,13} (6 flexible spaces, not 5) — with the test expectations updated to match.

2. Reuse the existing heading parser instead of adding a second one. headingsInMarkdown: re-derives heading detection and trips on a few edge cases — most notably it emits a phantom heading from YAML front matter (a leading ------ block turns the last front-matter line into an H2), and it's looser than the app's behavior on fence length and HR-vs-setext. We already have a robust, unit-tested parser for exactly this: +[MPDocument editorReferenceKindsForMarkdown:outLineNumbers:] (used by updateHeaderLocations), which already handles front matter, fenced blocks, thematic breaks, and CRLF. Building the navigator on top of it — mapping line numbers → character offsets the way updateHeaderLocations already does — removes the duplicate parser and all of these edge cases at once.

Minor: an early if (!self.editor) return; in showHeadingsNavigator: would harden the toolbar-button path, which doesn't go through the validateUserInterfaceItem: guard the menu item relies on.

The parser is reusable and the jump logic is sound, so this is close — it mainly wants a rebase plus the parser swap. Thanks!


Generated by Claude Code

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.

2 participants