Keep Markdown footnote links inside the preview - #3586
Open
danielbachhuber wants to merge 3 commits into
Open
danielbachhuber wants to merge 3 commits into
danielbachhuber wants to merge 3 commits into
Conversation
Fragment links no longer open a new window; clicking one scrolls to the matching id within the same preview. MarkdownListItem now forwards the footnote id so the destination exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- markdown-preview.test.tsx: the only marked conflict, and it was adjacency in the import block. This branch added `onTestFinished` to the vitest import; main added `highlightMarkdownCode` on the next line. Both imports stay. - markdown-preview.tsx merged cleanly and needed no follow-up. Main's changes there came from the incremental streaming render (get-bb#3591) and the timeline lightbox work (get-bb#3489, get-bb#3596); none of them touch `MarkdownAnchor` or `MarkdownListItem`, so the fragment-link handling and the forwarded `<li id>` survive as written. - The incremental renderer does not undercut this fix. It treats `footnoteDefinition` and `footnoteReference` as global constructs (markdown-incremental-pieces.ts), so a message with footnotes is not split across pieces, and every piece renders inside the same `[data-markdown-preview]` root the destination lookup scopes to.
- markdown-preview.test.tsx: this branch's footnote-scroll test and main's "keeps a rewritten link mounted across unrelated preview rerenders" test (get-bb#4096) were both appended after the same test, which is what conflicted. Both are kept. - markdown-preview.tsx merged cleanly. Main's only change there is a Prettier reformat of MarkdownImageProps; the fragment handling already routes through rewrittenHref, so localhost rewriting does not affect footnotes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
What was wrong
Markdown footnote references use
#user-content-fn-…links, butMarkdownAnchorappliedtarget="_blank"to every link. Fragment links therefore opened the app URL with a hash in a new browser window instead of staying in the preview.MarkdownListItemalso dropped the footnoteid, leaving the reference with no rendered destination.What changed
apps/app/src/components/ui/markdown-preview.tsxnow handles#links as in-preview navigation: it prevents default navigation, finds the matching id inside the closest[data-markdown-preview], and scrolls it into view atblock: "start". It does not addtargetorrelto fragment links. Broken fragments also remain in the preview rather than opening a browser window.MarkdownListItemnow forwardsidto its<li>, preserving the footnote destination without changing the existing handling of other list-item props. Scoping the lookup to the preview keeps footnote ids from colliding when the same Markdown renders in more than one preview at once. #3395 remains out of scope: it concerns external links in the file preview and is not fixed here.Implementation details
The destination search iterates
[id]elements instead of building a CSS id selector, so ids need no CSS escaping. Back-reference links use the same path, though the new test covers only the forward footnote reference. A sticky header in a scroll container can still cover a destination because scrolling usesblock: "start".How you verified
apps/app/src/components/ui/markdown-preview.test.tsxadds a two-preview footnote test. It confirms the first preview's reference has notarget, prevents its default action, does not calllinkRouting.onOpenLink, and callsscrollIntoViewonce on#user-content-fn-odein that preview only. The test failed before the change because_blankwas present, then still failed after the anchor change alone because the dropped list-item id leftscrollIntoViewuncalled. It passes after both changes.pnpm exec turbo run typecheck --filter=@bb/apppasses.pnpm exec turbo run test --filter=@bb/appruns 4,448 passing tests, butsrc/components/thread/terminal/ThreadTerminalView.test.tsfails while importing@xterm/xtermPlatform.tswithCannot read properties of undefined (reading 'indexOf'). The same failure occurs with this diff reverted, so it is pre-existing and unrelated.The behavior has not been checked in the running desktop app or iOS Safari; verification is limited to jsdom.
🤖 Generated with Claude Code