diff --git a/app/src/ai/ai_document_view.rs b/app/src/ai/ai_document_view.rs index 96c9e306c8a..70d5ebec2f0 100644 --- a/app/src/ai/ai_document_view.rs +++ b/app/src/ai/ai_document_view.rs @@ -174,7 +174,7 @@ impl AIDocumentView { ) -> Self { let window_id = ctx.window_id(); - let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::Active(window_id), ctx)); + let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::active(window_id), ctx)); // Subscribe to document model updates so we can react to agent changes ctx.subscribe_to_model( diff --git a/app/src/code/editor/comment_editor.rs b/app/src/code/editor/comment_editor.rs index 9e39529a1ac..eb26ea98cf3 100644 --- a/app/src/code/editor/comment_editor.rs +++ b/app/src/code/editor/comment_editor.rs @@ -526,7 +526,7 @@ where let parent_view_id = ctx.view_id(); let model = ctx.add_model(|ctx| NotebooksEditorModel::new(rich_text_styles, window_id, ctx)); - let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::Active(window_id), ctx)); + let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::active(window_id), ctx)); let parent_view_name = ctx.view_name(window_id, parent_view_id).unwrap_or_default(); let parent_position_id = format!("{}_{}", parent_view_name, parent_view_id); diff --git a/app/src/code/editor/comment_editor_tests.rs b/app/src/code/editor/comment_editor_tests.rs index 531bbc21fa8..ddcd1628e37 100644 --- a/app/src/code/editor/comment_editor_tests.rs +++ b/app/src/code/editor/comment_editor_tests.rs @@ -85,7 +85,7 @@ fn initialize_editor( let (window, test_view) = app.add_window(WindowStyle::NotStealFocus, |ctx| { let window_id = ctx.window_id(); - let _links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::Active(window_id), ctx)); + let _links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::active(window_id), ctx)); let editor = match mode { CommentEditorMode::Editable => create_editable_comment_markdown_editor(None, ctx), CommentEditorMode::Readonly => create_readonly_comment_markdown_editor( diff --git a/app/src/notebooks/editor/model.rs b/app/src/notebooks/editor/model.rs index f585fd40fba..6132c72c120 100644 --- a/app/src/notebooks/editor/model.rs +++ b/app/src/notebooks/editor/model.rs @@ -12,7 +12,7 @@ use markdown_parser::FormattedText; use mermaid_to_svg::MermaidTheme; use num_traits::SaturatingSub; use regex::Regex; -use string_offset::CharOffset; +use string_offset::{CharCounter, CharOffset}; use url::Url; use vec1::{Vec1, vec1}; use warp_core::r#async::debounce; @@ -86,6 +86,17 @@ lazy_static! { Regex::new(r"^\[ ?\] $").expect("Markdown shortcut regex should be valid"); static ref COMPLETE_TASKLIST_INLINE_REGEX: Regex = Regex::new(r"^\[[xX]\] $").expect("Markdown shortcut regex should be valid"); + + /// Matches a raw HTML ``/`` anchor-target tag (single or double + /// quotes), capturing the id/name value. This is a plain text scan over the rendered buffer, + /// not a parser token: the phase-1 inline parser only recognizes `` as a delimiter + /// pair (see `markdown_parser::parse_inline_token_html_anchor_start`) — a bare ``/`` tag with no `href` never matches that grammar and falls through to literal text, + /// so its raw markup (including the id) survives verbatim in the buffer. Scanning for it here + /// avoids adding any new zero-width content-model construct. + static ref HTML_ANCHOR_TARGET_TAG: Regex = Regex::new( + r#"(?i), default_mermaid_display_mode: MarkdownDisplayMode, + /// A `#fragment` anchor to scroll to once the document's layout is first built. Set when a + /// cross-document link (`other-file.md#section`) opens this notebook in a new tab: the target + /// offset does not exist until the buffer parses and lays out, and there is no on-load + /// callback to hang the scroll on, so we stash the fragment and drain it on the first + /// `LayoutUpdated`. Draining reuses the same `scroll_to_matching_header` slug resolver as the + /// same-document jump; a miss is a silent no-op, consistent with same-document miss semantics. + pending_anchor: Option, } #[derive(Clone)] @@ -149,6 +167,79 @@ fn render_mermaid_clipboard_html(source: &str) -> Option { Some(mermaid_image_html(&svg)) } +/// Normalize heading text (or a `#fragment` link target) into a GitHub-compatible anchor slug. +/// +/// This mirrors GitHub's `gfm-auto-identifiers` rule set, which is deliberately **not** +/// ASCII-only: it lowercases (Unicode-aware), drops punctuation and symbols, converts whitespace +/// runs to single hyphens, and **preserves every other Unicode letter/digit/mark** (accented +/// Latin, CJK, Cyrillic, …). So `"Café Société"` slugs to `café-société` and `"日本語"` is +/// unchanged — an ASCII-only `[a-z0-9 -]` filter would silently break every non-English anchor. +/// +/// Applying the same function to both the heading text and the incoming fragment makes a +/// hyphenated `#target-section` resolve against a spaced heading "Target Section". +fn heading_slug(text: &str) -> String { + let mut slug = String::with_capacity(text.len()); + let mut pending_hyphen = false; + for ch in text.chars() { + if ch.is_whitespace() { + // Collapse runs of whitespace; only emit the hyphen once a kept character follows. + pending_hyphen = !slug.is_empty(); + } else if ch == '-' || ch.is_alphanumeric() { + // Preserve existing hyphens and any Unicode alphanumeric character. + if pending_hyphen { + slug.push('-'); + pending_hyphen = false; + } + slug.extend(ch.to_lowercase()); + } + // Everything else (ASCII punctuation, symbols) is dropped. + } + slug +} + +/// Find the first ``/`` anchor tag in `text` whose value exactly matches +/// `target` (as authored — no slug normalization, no `user-content-` rewriting), returning the +/// tag's own `CharOffset` range. Anchor ids are compared to the fragment via [`urlencoding`] +/// alone (already applied by the caller before this runs), not [`heading_slug`]: unlike a +/// heading's derived slug, an explicit `` is an author-chosen literal string. +/// +/// This walks the buffer's plain text with a regex rather than any parsed anchor index, mirroring +/// how heading matching re-reads text from the buffer on every click (no cache, nothing to +/// invalidate). Byte offsets from the regex match are converted to `CharOffset` with +/// [`CharCounter`], the same bridge used for search-match highlighting +/// (`global_search::view`) — necessary because a `CharOffset` counts `char`s, not bytes, and a +/// naive byte offset would misplace the scroll target on any non-ASCII text preceding the tag. +fn find_matching_anchor_tag(text: &str, target: &str) -> Option> { + if target.is_empty() { + return None; + } + let total_chars = CharOffset::from(text.chars().count()); + let mut char_counter = CharCounter::new(text); + for capture in HTML_ANCHOR_TARGET_TAG.captures_iter(text) { + let whole_match = capture.get(0).expect("capture group 0 always matches"); + let value = capture + .get(1) + .or_else(|| capture.get(2)) + .expect("id/name value must be captured by one of the quote alternatives") + .as_str(); + if value != target { + continue; + } + let start = char_counter.char_offset(whole_match.start())?; + // `char_offset` returns `None` when the byte offset is exactly the length of the string + // (there is no character starting there) — the tag-is-the-last-thing-in-the-document + // case. Fall back to the total character count, mirroring the same edge case handled in + // `GlobalSearchView::highlight_indices_from_submatches`. + let end = if whole_match.end() == text.len() { + total_chars + } else { + char_counter.char_offset(whole_match.end())? + }; + return Some(start..end); + } + None +} + impl NotebooksEditorModel { fn editable_markdown_mermaid_enabled() -> bool { FeatureFlag::MarkdownMermaid.is_enabled() @@ -228,6 +319,7 @@ impl NotebooksEditorModel { resize_tx, file_link_resolution_context: None, default_mermaid_display_mode: MarkdownDisplayMode::Raw, + pending_anchor: None, } } @@ -351,6 +443,9 @@ impl NotebooksEditorModel { if self.sync_mermaid_render_offsets(ctx) { self.rebuild_layout(ctx); } + // Now that the document is laid out, heading offsets exist, so a queued + // cross-document anchor can resolve and scroll. Drained once. + self.drain_pending_anchor(ctx); } _ => (), } @@ -1332,6 +1427,29 @@ impl NotebooksEditorModel { self.content.as_ref(app).link_url_at_offset(offset) } + /// Queue a `#fragment` anchor to be scrolled to once this document's layout is first built. + /// + /// Used for cross-document navigation (`other-file.md#section`) when the notebook is opened in + /// a *new* tab: the target offset does not exist until the buffer parses and lays out. The + /// pending anchor is drained on the first `LayoutUpdated` (see `handle_render_model_event`). + /// If the tab is already open, callers should use `scroll_to_matching_header` directly instead + /// — the buffer is already laid out, so no deferral is needed. + pub fn set_pending_anchor(&mut self, anchor: String) { + self.pending_anchor = Some(anchor); + } + + /// Drain a queued cross-document anchor, scrolling to it if it matches a heading. Called once + /// the document's layout is ready. A miss (no matching heading) is a silent no-op, matching + /// the same-document miss semantics. Returns whether an anchor was drained *and* matched a + /// heading (i.e. a scroll was requested) — used in tests; callers on the layout path ignore it. + fn drain_pending_anchor(&mut self, ctx: &mut ModelContext) -> bool { + let Some(anchor) = self.pending_anchor.take() else { + return false; + }; + // `find_matching_header` expects a leading `#`; the stored anchor is the bare fragment. + self.scroll_to_matching_header(&format!("#{anchor}"), ctx) + } + pub fn scroll_to_matching_header( &mut self, fragment: &str, @@ -1348,35 +1466,62 @@ impl NotebooksEditorModel { true } + /// Resolve a `#fragment` against the document's addressable targets: explicit ``/`` anchor tags and implicit heading slugs. Both kinds share one namespace — when a + /// fragment matches more than one candidate (an anchor and a heading colliding, or two of the + /// same kind), **first occurrence in document order wins**, mirroring GitHub's single shared + /// id space. This is a live walk with no cached index: anchors and headings are both read + /// straight out of the current buffer on every call, exactly like the pre-existing + /// heading-only walk this extends. fn find_matching_header(&self, fragment: &str, ctx: &AppContext) -> Option> { - let target = fragment.strip_prefix('#')?; - if target.is_empty() { + let raw_target = fragment.strip_prefix('#')?; + if raw_target.is_empty() { return None; } - let target = urlencoding::decode(target).ok()?; - let target = target.trim().to_lowercase(); - if target.is_empty() { + let raw_target = urlencoding::decode(raw_target).ok()?; + if raw_target.trim().is_empty() { return None; } + // Anchor ids match verbatim (as authored, no rewriting); headings match through the + // slug normalizer. Precompute both comparison keys once. + let anchor_target = raw_target.trim(); + let heading_target = heading_slug(&raw_target); let content = self.content.as_ref(ctx); - for outline in content.outline_blocks() { + + let heading_match = content.outline_blocks().find_map(|outline| { if !matches!( &outline.block_type, BlockType::Text(BufferBlockStyle::Header { .. }) ) { - continue; + return None; + } + if heading_target.is_empty() { + return None; } - let heading = content .text_in_range(outline.start + 1..outline.end) .into_string(); - if heading.trim().to_lowercase() == target { - return Some(outline.start..outline.end); + (heading_slug(&heading) == heading_target).then_some(outline.start..outline.end) + }); + + let anchor_match = find_matching_anchor_tag(&content.text().into_string(), anchor_target); + + // Merge both candidate kinds and take whichever starts earlier in the document — the + // shared-namespace, first-occurrence-wins rule (invariant: explicit anchors and headings + // compete on position, not on kind). + match (anchor_match, heading_match) { + (Some(anchor_range), Some(heading_range)) => { + Some(if anchor_range.start <= heading_range.start { + anchor_range + } else { + heading_range + }) } + (Some(anchor_range), None) => Some(anchor_range), + (None, Some(heading_range)) => Some(heading_range), + (None, None) => None, } - - None } /// Whether or not there's an active command block selection. diff --git a/app/src/notebooks/editor/model_tests.rs b/app/src/notebooks/editor/model_tests.rs index 70121ff9408..47ea2c71cc6 100644 --- a/app/src/notebooks/editor/model_tests.rs +++ b/app/src/notebooks/editor/model_tests.rs @@ -103,7 +103,7 @@ fn model_from_markdown( let (window, _) = app.add_window(WindowStyle::NotStealFocus, |ctx| { let window_id = ctx.window_id(); - let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::Active(window_id), ctx)); + let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::active(window_id), ctx)); let editor_model = ctx.add_model(|ctx| { let styles = rich_text_styles(Appearance::as_ref(ctx), FontSettings::as_ref(ctx)); NotebooksEditorModel::new(styles, window_id, ctx) @@ -580,6 +580,403 @@ fn test_find_matching_header_missing_returns_none() { }) } +#[test] +fn test_pending_anchor_drains_and_scrolls_on_match() { + // The deferred cross-document scroll: a pending anchor set before layout is drained once, + // resolving through the same slug matcher as the same-document jump. A hyphenated fragment + // must land on its spaced heading, and the pending state must clear after draining. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("Intro\n\n## Target Section\nBody", &mut app, true); + + editor.update(&mut app, |editor, ctx| { + editor.set_pending_anchor("target-section".to_owned()); + assert!( + editor.drain_pending_anchor(ctx), + "Pending anchor matching a heading should request a scroll" + ); + // Draining is one-shot: a second drain finds nothing pending and reports no scroll. + assert!( + !editor.drain_pending_anchor(ctx), + "Pending anchor should be cleared after the first drain" + ); + }); + }) +} + +#[test] +fn test_pending_anchor_miss_is_silent_no_op() { + // A pending anchor with no matching heading drains without scrolling and without error, + // mirroring the same-document miss semantics (product invariant 7). + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("## Target Section\nBody", &mut app, true); + + editor.update(&mut app, |editor, ctx| { + editor.set_pending_anchor("no-such-heading".to_owned()); + assert!( + !editor.drain_pending_anchor(ctx), + "A pending anchor with no matching heading must not scroll" + ); + }); + }) +} + +#[test] +fn test_find_matching_header_slug_hyphenated_fragment() { + // The issue's headline case: a hyphenated fragment resolves against a spaced heading. + // Before the slug fix this returned None (exact-text comparison). + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("## Target Section\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + let range = editor + .find_matching_header("#target-section", ctx) + .expect("Hyphenated fragment should match spaced heading"); + let heading = editor + .content + .as_ref(ctx) + .text_in_range(range.start + 1..range.end) + .into_string(); + assert_eq!(heading, "Target Section"); + }); + }) +} + +#[test] +fn test_find_matching_header_strips_punctuation() { + // GitHub-style slugging drops punctuation from both the heading and the fragment. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("## What's New? (v2)\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + assert!(editor.find_matching_header("#whats-new-v2", ctx).is_some()); + }); + }) +} + +#[test] +fn test_find_matching_header_unicode_accented() { + // The normalizer preserves accented Latin (not an ASCII-only filter). + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("## Café Société\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + assert!(editor.find_matching_header("#café-société", ctx).is_some()); + }); + }) +} + +#[test] +fn test_find_matching_header_unicode_cjk() { + // CJK headings slug to themselves — no ASCII case to fold, no characters to strip. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("## 日本語\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + assert!(editor.find_matching_header("#日本語", ctx).is_some()); + }); + }) +} + +#[test] +fn test_find_matching_header_html_and_markdown_equivalent() { + // Invariant 3: an `` and a markdown `[text](#slug)` targeting the same + // heading resolve to the same range — both reach find_matching_header via the same path. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown( + "html\n\n[md](#target-section)\n\n## Target Section\nBody", + &mut app, + true, + ); + + editor.read(&app, |editor, ctx| { + let html_range = editor + .find_matching_header("#target-section", ctx) + .expect("Fragment should resolve"); + // Both syntaxes carry the identical fragment string, so both resolve identically. + assert_eq!( + editor.find_matching_header("#target-section", ctx), + Some(html_range) + ); + }); + }) +} + +#[test] +fn test_heading_slug_pure_function() { + // Pure `&str -> String` normalizer, table-driven per the spec's testing section. + let cases = [ + ("Target Section", "target-section"), + ("Simple", "simple"), + ("What's New? (v2)", "whats-new-v2"), + (" Leading and trailing ", "leading-and-trailing"), + ("Already-Hyphenated", "already-hyphenated"), + // Unicode-preserving, genuinely GitHub-compatible (not ASCII-only): + ("Café Société", "café-société"), + ("日本語", "日本語"), + ("Section 日本語 Café", "section-日本語-café"), + ]; + for (input, expected) in cases { + assert_eq!(super::heading_slug(input), expected, "slug of {input:?}"); + } +} + +// --- ``/`` anchor-target resolution --- +// +// Characterization first: the phase-1 inline parser only recognizes `` as a delimiter +// pair (`parse_inline_token_html_anchor_start` requires an `href` attribute to match at all). +// A bare ``/`` with no `href` never becomes that token, falls through the +// `alt` parser chain to literal text, and its raw markup — including the id/name value — survives +// verbatim in the buffer. This is confirmed at the parser level by +// `markdown_parser_tests::test_parse_html_anchor_unterminated_falls_back_to_text`'s trailing case +// (`` -> `FormattedTextFragment::plain_text("")`). The tests below +// exercise that same fact end-to-end through the editor buffer, which is what makes the +// zero-cache, live-text-scan resolution mechanism possible: there is no separate anchor token to +// carry from parse time to click time, because the tag's literal text already survives the trip. + +#[test] +fn test_anchor_tag_survives_as_literal_text_in_buffer() { + // Characterization: an ``/`` tag with no `href` is not parsed into any special + // token — it round-trips into the buffer as plain visible text, exactly as authored. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("\n\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + let text = editor.content.as_ref(ctx).text().into_string(); + assert!( + text.contains(""), + "anchor tag markup should survive verbatim in the buffer, got: {text:?}" + ); + }); + }) +} + +#[test] +fn test_find_matching_header_resolves_anchor_id() { + // Invariant: `#custom-anchor` resolves against a bare `` marker + // with no heading involved at all. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown( + "Intro\n\n\n\nBody", + &mut app, + true, + ); + + editor.read(&app, |editor, ctx| { + assert!(editor.find_matching_header("#custom-anchor", ctx).is_some()); + }); + }) +} + +#[test] +fn test_find_matching_header_resolves_anchor_name() { + // The `name` attribute is an equally valid anchor-target form (older HTML convention). + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown( + "Intro\n\n\n\nBody", + &mut app, + true, + ); + + editor.read(&app, |editor, ctx| { + assert!(editor.find_matching_header("#custom-anchor", ctx).is_some()); + }); + }) +} + +#[test] +fn test_find_matching_header_anchor_id_matches_verbatim_not_slugged() { + // No id rewriting: an explicit `` value is matched exactly as authored, not run through + // `heading_slug`. A mixed-case, underscore-bearing id would be mangled by the heading + // normalizer (which lowercases and treats `_` as a non-alphanumeric drop candidate is wrong + // here) — matching it verbatim is the point. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("\n\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + assert!( + editor + .find_matching_header("#Custom_Anchor-1", ctx) + .is_some(), + "exact-case id should resolve" + ); + assert!( + editor + .find_matching_header("#custom_anchor-1", ctx) + .is_none(), + "lowercased fragment must NOT match a differently-cased explicit id — anchor ids \ + are not slug-normalized" + ); + }); + }) +} + +#[test] +fn test_find_matching_header_anchor_and_heading_first_in_document_order_wins() { + // Frederic's ruling: anchors and headings share one namespace; when both match the same + // fragment, whichever occurs FIRST in document order wins — not "anchors always win." + // Case 1: the anchor appears before the colliding heading, so the anchor wins. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown( + "\n\nIntro\n\n## Target\nBody", + &mut app, + true, + ); + + editor.read(&app, |editor, ctx| { + let range = editor + .find_matching_header("#target", ctx) + .expect("should resolve to the earlier anchor"); + // The anchor tag sits before the heading; assert the resolved range starts before the + // heading block by checking it's the anchor tag's position, not the heading's. + let heading_range = { + let content = editor.content.as_ref(ctx); + content + .outline_blocks() + .find(|o| { + matches!( + &o.block_type, + BlockType::Text(BufferBlockStyle::Header { .. }) + ) + }) + .map(|o| o.start..o.end) + .expect("document has one heading") + }; + assert!( + range.start < heading_range.start, + "anchor occurs earlier in the document and must win" + ); + }); + }) +} + +#[test] +fn test_find_matching_header_heading_before_anchor_wins() { + // Case 2: the heading appears FIRST and the colliding anchor comes later — document order + // means the heading wins here, demonstrating this is not an "anchors always win" rule. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = + model_from_markdown("## Target\nBody\n\n", &mut app, true); + + editor.read(&app, |editor, ctx| { + let range = editor + .find_matching_header("#target", ctx) + .expect("should resolve to the earlier heading"); + let content = editor.content.as_ref(ctx); + let heading_range = content + .outline_blocks() + .find(|o| { + matches!( + &o.block_type, + BlockType::Text(BufferBlockStyle::Header { .. }) + ) + }) + .map(|o| o.start..o.end) + .expect("document has one heading"); + assert_eq!( + range, heading_range, + "heading occurs earlier in the document and must win over the later anchor" + ); + }); + }) +} + +#[test] +fn test_find_matching_header_anchor_miss_is_silent_none() { + // A fragment matching neither an anchor nor a heading is a silent miss, same as the + // heading-only miss path (product invariant 7). + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("\n\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + assert!(editor + .find_matching_header("#no-such-anchor", ctx) + .is_none()); + }); + }) +} + +#[test] +fn test_find_matching_header_urlencoded_fragment_matches_anchor_id() { + // A URL-encoded fragment (e.g. produced by a browser-style link) decodes before comparison, + // matching an anchor id containing characters that require encoding. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("\n\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + assert!( + editor + .find_matching_header("#custom%20anchor", ctx) + .is_some(), + "urlencoded fragment should decode and match the anchor id verbatim" + ); + }); + }) +} + +#[test] +fn test_find_matching_header_anchor_single_quotes() { + // The anchor-tag scan accepts single-quoted attribute values too, mirroring the `` + // parser's support for both quote styles. + App::test((), |mut app| async move { + initialize_deps(&mut app); + let editor = model_from_markdown("\n\nBody", &mut app, true); + + editor.read(&app, |editor, ctx| { + assert!(editor.find_matching_header("#custom-anchor", ctx).is_some()); + }); + }) +} + +#[test] +fn test_find_matching_anchor_tag_pure_function() { + // Pure function test for the underlying scanner, independent of the editor model. The + // matched range covers the opening tag through its id/name attribute value (the regex has no + // `>`/`` requirement) — enough to identify a scroll-to position; only `.start` is used by + // callers, so the exact end isn't load-bearing, but it's asserted here to pin the contract. + assert_eq!( + super::find_matching_anchor_tag(r#""#, "foo"), + Some(CharOffset::from(0)..CharOffset::from(11)) + ); + assert_eq!( + super::find_matching_anchor_tag(r#""#, "foo"), + Some(CharOffset::from(0)..CharOffset::from(13)) + ); + assert_eq!( + super::find_matching_anchor_tag(r#""#, "bar"), + None + ); + assert_eq!( + super::find_matching_anchor_tag(r#""#, ""), + None + ); + // First occurrence wins among multiple anchors sharing the same id. + let text = r#" middle "#; + let first = super::find_matching_anchor_tag(text, "dup").expect("should match"); + assert_eq!(first.start, CharOffset::from(0)); + // A non-ASCII prefix before the tag must not misplace the char offset — regression guard for + // the byte-vs-char distinction `CharCounter` bridges. + let text_with_unicode_prefix = "日本語 "; + let range = super::find_matching_anchor_tag(text_with_unicode_prefix, "foo") + .expect("should match after unicode prefix"); + // "日本語 " is 4 chars (3 CJK + 1 space); the tag starts at char offset 4. + assert_eq!(range.start, CharOffset::from(4)); +} + #[test] fn test_cursor_bias_editing() { App::test((), |mut app| async move { diff --git a/app/src/notebooks/editor/view.rs b/app/src/notebooks/editor/view.rs index b6797620f68..e898b35aa0b 100644 --- a/app/src/notebooks/editor/view.rs +++ b/app/src/notebooks/editor/view.rs @@ -1973,17 +1973,21 @@ impl RichTextEditorView { }; if let Some(url) = url { - if url.starts_with('#') - && (cmd || matches!(self.interaction_state(ctx), InteractionState::Selectable)) - { - let scrolled = self - .model - .update(ctx, |model, ctx| model.scroll_to_matching_header(&url, ctx)); - if scrolled { - self.open_link = None; - ctx.notify(); - return; + if url.starts_with('#') { + // An in-document `#fragment` is resolved entirely within the viewer and never + // falls through to the URL/file opener. On an activating interaction (cmd-click, + // or a plain click in a read-only Selectable chip) a hit scrolls to the anchor. + // A miss — and every non-activating interaction — is a deliberate no-op: the + // fragment must not be handed to the resolver, which would treat it as a bogus + // relative file path and surface a broken-link tooltip (violating invariant 7's + // "no error on miss"). + if cmd || matches!(self.interaction_state(ctx), InteractionState::Selectable) { + self.model + .update(ctx, |model, ctx| model.scroll_to_matching_header(&url, ctx)); } + self.open_link = None; + ctx.notify(); + return; } // In read-only comment chips (Selectable), open the link directly on // click instead of showing a tooltip. diff --git a/app/src/notebooks/editor/view_tests.rs b/app/src/notebooks/editor/view_tests.rs index 1c42289b1dc..a3941dd5517 100644 --- a/app/src/notebooks/editor/view_tests.rs +++ b/app/src/notebooks/editor/view_tests.rs @@ -106,7 +106,7 @@ fn initialize_editor( let (window, test_view) = app.add_window(WindowStyle::NotStealFocus, |ctx| { let window_id = ctx.window_id(); - let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::Active(window_id), ctx)); + let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::active(window_id), ctx)); let editor_model = ctx.add_model(|ctx| { let styles = rich_text_styles(Appearance::as_ref(ctx), FontSettings::as_ref(ctx)); NotebooksEditorModel::new(styles, window_id, ctx) @@ -661,7 +661,13 @@ fn test_link_editing() { } #[test] -fn test_editable_markdown_anchor_click_opens_link_tooltip() { +fn test_editable_markdown_anchor_click_is_silent_no_op() { + // An in-document `#fragment` is resolved entirely within the viewer (see `maybe_open_url`'s + // `#`-branch) and never surfaces a link tooltip. A plain (non-cmd) click in an editable + // editor is a non-activating interaction, so it does nothing observable: no tooltip, no + // scroll, no fall-through to the URL/file opener. (Superseded: the pre-anchor-links behavior + // showed an editable `#goal` link tooltip here; the fragment now belongs to the resolver, not + // the tooltip path.) App::test((), |mut app| async move { let (_, editor_view, _) = initialize_editor(&mut app); reset_editor_with_markdown(&mut app, &editor_view, "- [Goal](#goal)\n\n## Goal").await; @@ -679,12 +685,10 @@ fn test_editable_markdown_anchor_click_opens_link_tooltip() { }); editor_view.read(&app, |editor, _ctx| { - let open_link = editor - .open_link - .as_ref() - .expect("Editable anchor click should show the link tooltip"); - assert_eq!(open_link.url, "#goal"); - assert!(open_link.editable); + assert!( + editor.open_link.is_none(), + "An in-document `#fragment` click must not show a link tooltip" + ); }); }); } @@ -717,10 +721,19 @@ fn test_cmd_click_markdown_anchor_navigates_without_link_tooltip() { } #[test] -fn test_cmd_click_missing_markdown_anchor_falls_back_to_link_resolution() { +fn test_cmd_click_missing_markdown_anchor_is_silent_no_op() { + // Product invariant 7: a `#fragment` that matches no heading does nothing observable — no + // scroll, no error, and crucially "no attempt to open it as an external URL". The fragment + // must NOT fall through to the link resolver, which would treat `#missing.png` as a bogus + // relative file path and emit an open event / broken-link tooltip. (Superseded: the + // pre-anchor-links behavior fell back to link resolution here and emitted `OpenFileWithTarget` + // for a literal `#missing.png` file; that fall-through is exactly what the anchor work + // removed.) App::test((), |mut app| async move { let (window_id, editor_view, _) = initialize_editor(&mut app); let base = tempdir().expect("Expected temp dir"); + // A file literally named `#missing.png` would have been the fall-through target under the + // old behavior. It must now be ignored entirely — no event for it. let fallback_path = base.path().join("#missing.png"); std::fs::File::create(&fallback_path).expect("Expected fallback file"); let session = Arc::new(Session::test().with_shell_launch_data( @@ -770,15 +783,20 @@ fn test_cmd_click_missing_markdown_anchor_falls_back_to_link_resolution() { ); }); - assert_eventually!( - events.lock().iter().any(|event| { - matches!( - event, - LinkEvent::OpenFileWithTarget { path, .. } if path == &fallback_path - ) - }), - "Missing anchor click should fall back to link resolution: {:?}", - events.lock().clone() + // The `#`-branch handles the fragment synchronously and returns before spawning any + // resolution future, so there is nothing async to await: if the branch is correct, no + // event was emitted and no tooltip was set by the time `handle_action` returned. + editor_view.read(&app, |editor, _ctx| { + assert!( + editor.open_link.is_none(), + "A `#fragment` miss must not surface a link tooltip" + ); + }); + let emitted = events.lock(); + assert!( + emitted.is_empty(), + "A `#fragment` miss must not fall through to link resolution (invariant 7), \ + but emitted: {emitted:?}" ); }); } diff --git a/app/src/notebooks/file/mod.rs b/app/src/notebooks/file/mod.rs index b1d6ec168c9..5d8d7261570 100644 --- a/app/src/notebooks/file/mod.rs +++ b/app/src/notebooks/file/mod.rs @@ -245,7 +245,7 @@ impl FileNotebookView { pub fn new(ctx: &mut ViewContext) -> Self { let window_id = ctx.window_id(); // Use the active session for links until we have something more specific. - let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::Active(window_id), ctx)); + let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::active(window_id), ctx)); let view_position_id = format!("file_notebook_view_{}", ctx.view_id()); @@ -434,6 +434,18 @@ impl FileNotebookView { self.pane_configuration.update(ctx, |pane_config, ctx| { pane_config.set_title(local_path.display().to_string(), ctx); }); + // Even without a session, seed the link resolver with the document's own parent + // directory as a base-directory fallback, so a standalone viewer tab (e.g. + // `open -a Warp file.md`, which has no terminal session cwd) can still resolve this + // document's relative links against where the document actually lives. + let window_id = ctx.window_id(); + let document_dir = local_path.parent().map(Path::to_path_buf); + self.links.update(ctx, |links, ctx| { + links.set_session_source( + SessionSource::active_for_document(window_id, document_dir), + ctx, + ); + }); } self.file_state = FileState::Loading(SourceFile::FileBased { @@ -714,6 +726,30 @@ impl FileNotebookView { self.links.clone() } + /// Immediately scroll to a `#fragment` anchor's heading. Used when a cross-document link + /// targets a notebook whose tab is *already* open and laid out, so no deferral is needed. + /// A miss (no matching heading) is a silent no-op, matching same-document miss semantics. + pub fn scroll_to_anchor(&self, anchor: &str, ctx: &mut ViewContext) { + let fragment = format!("#{anchor}"); + self.editor.update(ctx, |editor, ctx| { + editor.model().update(ctx, |model, ctx| { + model.scroll_to_matching_header(&fragment, ctx); + }); + }); + } + + /// Queue a `#fragment` anchor to scroll to once this notebook's layout is first built. Used + /// when a cross-document link opens the target in a *new* tab: the heading offset does not + /// exist until the buffer parses and lays out. The pending anchor is drained on the first + /// `LayoutUpdated` inside the editor model. + pub fn queue_pending_anchor(&self, anchor: String, ctx: &mut ViewContext) { + self.editor.update(ctx, |editor, ctx| { + editor.model().update(ctx, |model, _| { + model.set_pending_anchor(anchor); + }); + }); + } + fn is_markdown_file(&self) -> bool { self.file_state .path() diff --git a/app/src/notebooks/link.rs b/app/src/notebooks/link.rs index d9920b1c599..642df7f2bb2 100644 --- a/app/src/notebooks/link.rs +++ b/app/src/notebooks/link.rs @@ -33,9 +33,16 @@ pub enum LinkTarget { LocalFile { path: PathBuf, line_and_column: Option, - /// The base session when the link was resolved. It's stored here in case it changes - /// between resolving and opening the link. - session: Arc, + /// A `#fragment` anchor to scroll to once the destination Markdown document loads. + /// Split off the link before file resolution (so `other-file.md#section` resolves the + /// file, not a literal `#section` path) and carried here so the destination notebook can + /// perform a deferred scroll — the cross-document analog of `line_and_column`. + anchor: Option, + /// The base session when the link was resolved, if there was one. Stored here in case it + /// changes between resolving and opening the link. `None` for a link resolved in a + /// standalone Markdown viewer tab (opened from Finder / `open -a Warp file.md`), which has + /// a base directory — the document's own dir — but no terminal session. + session: Option>, /// Whether or not this file is a Markdown file viewable in Warp. is_markdown: bool, }, @@ -73,19 +80,28 @@ impl PartialEq for LinkTarget { Self::LocalFile { path: my_path, line_and_column: my_location, + anchor: my_anchor, session: my_session, .. }, Self::LocalFile { path: other_path, line_and_column: other_location, + anchor: other_anchor, session: other_session, .. }, ) => { my_path == other_path && my_location == other_location - && Arc::ptr_eq(my_session, other_session) + && my_anchor == other_anchor + && match (my_session, other_session) { + (Some(my_session), Some(other_session)) => { + Arc::ptr_eq(my_session, other_session) + } + (None, None) => true, + _ => false, + } } (Self::LocalDirectory { path: my_path }, Self::LocalDirectory { path: other_path }) => { my_path == other_path @@ -105,6 +121,51 @@ impl fmt::Display for LinkTarget { } } +/// Split a trailing `#fragment` anchor off a scheme-less link target. +/// +/// Returns the path portion and the decoded anchor (if any). A `#L[:]` +/// line-number suffix is intentionally *not* treated as an anchor — it is left on the path so the +/// downstream `CleanPathResult` line/column parser continues to handle it. An empty fragment +/// (`file.md#`) yields no anchor. Only the final `#` is split, so an earlier `#` in the path +/// (e.g. `weird#name.md`) is preserved. +fn split_anchor_fragment(link: &str) -> (&str, Option) { + let Some((path, fragment)) = link.rsplit_once('#') else { + return (link, None); + }; + + // Preserve `#L100` / `#L100:200` line-number routing: these are stripped by + // `CleanPathResult`, so they must stay attached to the path, not be peeled as an anchor. + if is_line_number_fragment(fragment) { + return (link, None); + } + + if fragment.is_empty() { + return (path, None); + } + + let anchor = urlencoding::decode(fragment) + .map(|decoded| decoded.into_owned()) + .unwrap_or_else(|_| fragment.to_owned()); + (path, Some(anchor)) +} + +/// Whether a fragment is a `#L` or `#L:` line-number suffix (handled by +/// `CleanPathResult`), rather than a document anchor. +fn is_line_number_fragment(fragment: &str) -> bool { + let Some(rest) = fragment.strip_prefix('L') else { + return false; + }; + match rest.split_once(':') { + Some((line, col)) => { + !line.is_empty() + && line.bytes().all(|b| b.is_ascii_digit()) + && !col.is_empty() + && col.bytes().all(|b| b.is_ascii_digit()) + } + None => !rest.is_empty() && rest.bytes().all(|b| b.is_ascii_digit()), + } +} + /// Model for resolving and opening links in a notebook, taking into account their context (for /// example, resolving relative file paths). pub struct NotebookLinks { @@ -140,26 +201,67 @@ impl NotebookLinks { && let Ok(file) = url.to_file_path() { // TODO(ben): Support line and column in file:// URLs. - return Either::Left(Self::resolve_file(file, session, None)); + return Either::Left(Self::resolve_file(file, Some(session), None, None)); } } return Either::Right(future::ready(Ok(LinkTarget::Url(url)))); } - // If parsing failed, see if this is a web URL without a scheme. - // The heuristic we use is to take the substring up to the first slash (if present), and - // check for a valid public domain name or IP address. - let maybe_domain = link.split_once('/').map_or(link, |(start, _)| start); - if (addr::parse_domain_name(maybe_domain) - .is_ok_and(|domain| domain.has_known_suffix() && domain.root().is_some()) - || maybe_domain.parse::().is_ok()) - && let Ok(url) = Url::parse(&format!("http://{link}")) - { - return Either::Right(future::ready(Ok(LinkTarget::Url(url)))); + // Peel a trailing `#fragment` off before any file resolution, so a cross-document link + // like `other-file.md#section` resolves the file (not a literal `#section` path) and the + // fragment rides along on the resolved target for a deferred scroll. A `#L100` + // line-number suffix is deliberately left in place — it is handled downstream by + // `CleanPathResult` and must not be mistaken for an anchor. (A bare `#fragment` with no + // path never reaches here: it is intercepted earlier by the viewer's `maybe_open_url` + // `#`-branch.) + let (link, anchor) = split_anchor_fragment(link); + + // REPAIR: a bare `file.md` (no `./`, no `/`) is classified as a valid public domain by + // the heuristic below, because `.md` (Moldova), `.dev`, `.com`, … are known suffixes. + // That misroutes an existing local Markdown file to the browser. So before applying the + // domain heuristic, if the scheme-less target resolves to an existing file relative to + // the base directory, treat it as a file. A genuine bare domain with no matching local + // file (e.g. `warp.dev`) still falls through to the browser. + #[cfg(feature = "local_fs")] + let resolves_to_local_file = { + let session = self.session_source.session(ctx); + match self.session_source.base_directory(ctx) { + Some(base_directory) => { + let clean_path = CleanPathResult::with_line_and_column_number(link); + crate::util::file::absolute_path_if_valid( + &clean_path, + crate::util::file::ShellPathType::PlatformNative( + base_directory.to_path_buf(), + ), + session.as_ref().and_then(|s| s.launch_data()), + ) + .is_some() + } + None => false, + } + }; + #[cfg(not(feature = "local_fs"))] + let resolves_to_local_file = false; + + if !resolves_to_local_file { + // If parsing failed, see if this is a web URL without a scheme. + // The heuristic we use is to take the substring up to the first slash (if present), and + // check for a valid public domain name or IP address. + let maybe_domain = link.split_once('/').map_or(link, |(start, _)| start); + if (addr::parse_domain_name(maybe_domain) + .is_ok_and(|domain| domain.has_known_suffix() && domain.root().is_some()) + || maybe_domain.parse::().is_ok()) + && let Ok(url) = Url::parse(&format!("http://{link}")) + { + return Either::Right(future::ready(Ok(LinkTarget::Url(url)))); + } } - // At this point, we can only resolve file targets, which require a session. + // At this point, we can only resolve file targets. These are normally resolved against a + // session's context, but a standalone Markdown viewer tab has no session — only a base + // directory (the document's own dir). The second arm below handles that case so relative + // links still resolve there. match self.session_source.session(ctx) { Some(session) if session.launch_data().is_some() => { let launch_data = session @@ -200,11 +302,16 @@ impl NotebookLinks { Either::Left(Self::resolve_file( path, - session, + Some(session), clean_path.line_and_column_num, + anchor, )) } - Some(session) => { + session => { + // Either a session without launch data, or no session at all (a standalone + // Markdown viewer tab). Both resolve a relative path against the base directory, + // which must be present — the document's own directory supplies it in the + // no-session case. let clean_path_result = CleanPathResult::with_line_and_column_number(link); let clean_path = Path::new(&clean_path_result.path); let path = if clean_path.is_relative() { @@ -224,27 +331,31 @@ impl NotebookLinks { path, session, clean_path_result.line_and_column_num, + anchor, )) } - None => Either::Right(future::ready(Err(ResolveError::MissingContext))), } } - /// Resolve a file path into a [`LinkTarget`], checking if it exists. + /// Resolve a file path into a [`LinkTarget`], checking if it exists. `session` is `None` for a + /// standalone Markdown viewer tab, which resolves relative links against the document's own + /// directory without any terminal session. async fn resolve_file( path: PathBuf, - session: Arc, + session: Option>, line_and_column: Option, + anchor: Option, ) -> Result { let metadata = async_fs::metadata(&path).await?; Ok(if metadata.is_dir() { - // Discard line/column information, which doesn't make sense for a directory. + // Discard line/column and anchor information, which don't make sense for a directory. LinkTarget::LocalDirectory { path } } else { LinkTarget::LocalFile { is_markdown: is_markdown_file(&path), path, line_and_column, + anchor, session, } }) @@ -268,6 +379,7 @@ impl NotebookLinks { LinkTarget::LocalFile { path, line_and_column, + anchor, session, is_markdown: true, } => { @@ -278,15 +390,27 @@ impl NotebookLinks { { let settings = EditorSettings::as_ref(ctx); if *settings.prefer_markdown_viewer { - ctx.emit(LinkEvent::OpenFileNotebook { path, session }); + ctx.emit(LinkEvent::OpenFileNotebook { + path, + session, + anchor, + }); } else { + // The external editor / code viewer has no heading-slug concept, so the + // anchor is dropped here per the product non-goal: the file opens + // unscrolled. + let _ = anchor; open_file(path, line_and_column, ctx); } } #[cfg(not(feature = "local_fs"))] { - ctx.emit(LinkEvent::OpenFileNotebook { path, session }); + ctx.emit(LinkEvent::OpenFileNotebook { + path, + session, + anchor, + }); } } LinkTarget::LocalFile { @@ -344,7 +468,7 @@ impl NotebookLinks { ) { // Re-resolve links against the new session info, especially if the working directory // changed. - if matches!(self.session_source, SessionSource::Active(_)) { + if matches!(self.session_source, SessionSource::Active { .. }) { ctx.emit(LinkEvent::RefreshLinks); } } @@ -439,7 +563,12 @@ pub enum LinkEvent { /// Emitted when the view should open a Markdown file as a notebook. OpenFileNotebook { path: PathBuf, - session: Arc, + /// The session the link was resolved from, if any. `None` for a link opened in a + /// standalone Markdown viewer tab, which has no terminal session. + session: Option>, + /// A `#fragment` anchor to scroll to once the destination notebook loads, if the link + /// carried one (`other-file.md#section`). `None` for a plain file link. + anchor: Option, }, OpenWarpDriveLink { open_warp_drive_args: OpenWarpDriveObjectArgs, @@ -475,23 +604,55 @@ pub enum SessionSource { base_directory: PathBuf, }, /// Use the window's active session and working directory. - Active(WindowId), + Active { + window_id: WindowId, + /// The open document's own parent directory, used as the base-directory fallback when the + /// window's active session has no local working directory. Without this, a Markdown + /// document opened in a standalone viewer (e.g. `open -a Warp file.md`, which has no + /// terminal session cwd) can't resolve its own relative links — the document knows where + /// it lives even when the window doesn't. The active session's cwd still takes precedence + /// when present, preserving existing behavior for notebooks opened inside a session. + document_dir: Option, + }, } impl SessionSource { + /// Use the window's active session, with no document-directory fallback. For surfaces that + /// are not backed by a file on disk (comment editors, AI documents, in-memory notebooks). + pub fn active(window_id: WindowId) -> Self { + SessionSource::Active { + window_id, + document_dir: None, + } + } + + /// Use the window's active session, falling back to the given document directory when the + /// active session has no local working directory. For file-backed Markdown notebooks. + pub fn active_for_document(window_id: WindowId, document_dir: Option) -> Self { + SessionSource::Active { + window_id, + document_dir, + } + } + fn session(&self, ctx: &AppContext) -> Option> { match self { SessionSource::Target { session, .. } => Some(session.clone()), - SessionSource::Active(window_id) => ActiveSession::as_ref(ctx).session(*window_id), + SessionSource::Active { window_id, .. } => { + ActiveSession::as_ref(ctx).session(*window_id) + } } } fn base_directory<'a>(&'a self, ctx: &'a AppContext) -> Option<&'a Path> { match self { SessionSource::Target { base_directory, .. } => Some(base_directory.as_path()), - SessionSource::Active(window_id) => { - ActiveSession::as_ref(ctx).path_if_local(*window_id) - } + SessionSource::Active { + window_id, + document_dir, + } => ActiveSession::as_ref(ctx) + .path_if_local(*window_id) + .or(document_dir.as_deref()), } } } diff --git a/app/src/notebooks/link_tests.rs b/app/src/notebooks/link_tests.rs index 86a9c16428e..84a7e0796d9 100644 --- a/app/src/notebooks/link_tests.rs +++ b/app/src/notebooks/link_tests.rs @@ -10,7 +10,7 @@ use url::Url; use warp_util::path::LineAndColumnArg; use warpui::{App, ModelHandle, SingletonEntity, WindowId}; -use super::{LinkTarget, NotebookLinks, ResolveError, SessionSource}; +use super::{split_anchor_fragment, LinkTarget, NotebookLinks, ResolveError, SessionSource}; use crate::notebooks::file::is_markdown_file; use crate::notebooks::link::LinkEvent; use crate::terminal::model::session::Session; @@ -34,7 +34,32 @@ fn local_file(path: impl Into) -> LinkTarget { is_markdown: is_markdown_file(&path), path, line_and_column: None, - session: TEST_SESSION.clone(), + anchor: None, + session: Some(TEST_SESSION.clone()), + } +} + +/// Like [`local_file`], but for the standalone-viewer case where the link was resolved without a +/// terminal session (see [`init_link_model_no_session`]). +fn local_file_no_session(path: impl Into) -> LinkTarget { + let path = path.into(); + LinkTarget::LocalFile { + is_markdown: is_markdown_file(&path), + path, + line_and_column: None, + anchor: None, + session: None, + } +} + +fn local_file_anchor(path: impl Into, anchor: &str) -> LinkTarget { + let path = path.into(); + LinkTarget::LocalFile { + is_markdown: is_markdown_file(&path), + path, + line_and_column: None, + anchor: Some(anchor.to_owned()), + session: Some(TEST_SESSION.clone()), } } @@ -47,7 +72,8 @@ fn local_file_location(path: impl Into, line: usize, column: Option) -> ModelHandle< base_directory: dir.to_owned(), }, // File links can't be resolved without a session, even if there's no working directory. - None => SessionSource::Active(window_id), + None => SessionSource::active(window_id), }; app.add_singleton_model(|ctx| { let mut session = ActiveSession::default(); @@ -79,6 +105,23 @@ fn init_link_model(app: &mut App, base_directory: Option<&Path>) -> ModelHandle< app.add_model(|ctx| NotebookLinks::new(source, ctx)) } +/// Initialize the link resolver for a *standalone Markdown viewer* tab: a file-backed notebook +/// whose window has no active terminal session, so the only base-directory context is the +/// document's own directory (`SessionSource::active_for_document`). This mirrors the real GUI +/// condition of opening a `.md` from Finder / `open -a Warp file.md`, which +/// [`init_link_model`] cannot reproduce because it always installs a `TEST_SESSION`. +fn init_link_model_no_session(app: &mut App, document_dir: &Path) -> ModelHandle { + initialize_settings_for_tests(app); + + let window_id = WindowId::new(); + // Register the window with ActiveSession, but with *no* session and no working directory, so + // `session(window_id)` and `path_if_local(window_id)` both return `None` — the standalone + // viewer case. The document directory is the sole base-directory source. + app.add_singleton_model(|_ctx| ActiveSession::default()); + let source = SessionSource::active_for_document(window_id, Some(document_dir.to_owned())); + app.add_model(|ctx| NotebookLinks::new(source, ctx)) +} + async fn resolve(app: &App, links: &ModelHandle, link: &str) -> LinkTarget { match links.read(app, |links, ctx| links.resolve(link, ctx)).await { Ok(target) => target, @@ -171,6 +214,173 @@ fn test_resolve_bare_url() { }); } +#[test] +fn test_split_anchor_fragment_pure() { + // No fragment. + assert_eq!( + split_anchor_fragment("other-file.md"), + ("other-file.md", None) + ); + // Simple fragment. + assert_eq!( + split_anchor_fragment("other-file.md#section"), + ("other-file.md", Some("section".to_owned())) + ); + // Dot-slash prefix preserved on the path side. + assert_eq!( + split_anchor_fragment("./other-file.md#section"), + ("./other-file.md", Some("section".to_owned())) + ); + // Only the final `#` splits; an earlier `#` stays in the path. + assert_eq!( + split_anchor_fragment("weird#name.md#frag"), + ("weird#name.md", Some("frag".to_owned())) + ); + // Empty fragment yields no anchor. + assert_eq!( + split_anchor_fragment("other-file.md#"), + ("other-file.md", None) + ); + // URL-encoded fragments are decoded. + assert_eq!( + split_anchor_fragment("doc.md#caf%C3%A9"), + ("doc.md", Some("café".to_owned())) + ); + // `#L` line-number suffixes are NOT peeled as anchors — left for CleanPathResult. + assert_eq!( + split_anchor_fragment("main.rs#L100"), + ("main.rs#L100", None) + ); + assert_eq!( + split_anchor_fragment("main.rs#L100:50"), + ("main.rs#L100:50", None) + ); + // A fragment that merely starts with `L` but isn't a line number is a real anchor. + assert_eq!( + split_anchor_fragment("doc.md#License"), + ("doc.md", Some("License".to_owned())) + ); + assert_eq!( + split_anchor_fragment("doc.md#L10x"), + ("doc.md", Some("L10x".to_owned())) + ); +} + +#[test] +fn test_bare_markdown_file_prefers_local_file_over_cctld() { + // Regression test for the `.md`-is-Moldova ccTLD collision. A bare `README.md` (no `./` + // prefix, no slash) is classified as a valid public domain by the bare-domain heuristic + // (`.md` has a known suffix and `README` is a root), so before this fix it misrouted to the + // browser. When such a target resolves to an existing local file relative to the base + // directory, it must be opened as a file, not a URL. + App::test((), |mut app| async move { + let base = tempdir().unwrap(); + let base_path = base.path(); + touch(base_path.join("README.md")).await; + touch(base_path.join("notes.md")).await; + touch(base_path.join("docs/guide.md")).await; + let links = init_link_model(&mut app, Some(base_path)); + + // Single-segment `file.md` targets that exist on disk resolve as files, not URLs — the + // shape the ccTLD heuristic misroutes (the heuristic takes the substring up to the first + // `/`, so only a slash-free name can be mistaken for a domain). + assert_eq!( + resolve(&app, &links, "README.md").await, + local_file(base_path.join("README.md")) + ); + assert_eq!( + resolve(&app, &links, "notes.md").await, + local_file(base_path.join("notes.md")) + ); + + // Guard: multi-segment `.md` paths were already safe (any `/` makes the pre-`/` substring + // a non-suffix string, so the heuristic can't fire) — assert the repair's file-first + // ordering doesn't regress them. + assert_eq!( + resolve(&app, &links, "docs/guide.md").await, + local_file(base_path.join("docs/guide.md")) + ); + assert_eq!( + resolve(&app, &links, "./docs/guide.md").await, + local_file(base_path.join("docs/guide.md")) + ); + + // A genuine bare domain that does NOT resolve to a local file still opens the browser. + assert_eq!( + resolve(&app, &links, "warp.dev").await, + url("http://warp.dev") + ); + // A bare `.md` target with no matching local file also falls through to the browser, + // preserving the domain heuristic where there's nothing to shadow it. + assert_eq!( + resolve(&app, &links, "nonexistent.md").await, + url("http://nonexistent.md") + ); + }); +} + +#[test] +fn test_split_fragment_before_file_resolution() { + // A cross-document link `other-file.md#section` must have its `#section` fragment split off + // before file resolution, so the file part resolves on disk and the fragment rides along on + // the resolved target for a deferred scroll. Before this fix, the literal `#section` was + // included in the stat and the file was never found. + App::test((), |mut app| async move { + let base = tempdir().unwrap(); + let base_path = base.path(); + touch(base_path.join("other-file.md")).await; + touch(base_path.join("multi#hash.md")).await; + let links = init_link_model(&mut app, Some(base_path)); + + // Bare `file.md#section` resolves to the file with the anchor attached. + assert_eq!( + resolve(&app, &links, "other-file.md#section").await, + local_file_anchor(base_path.join("other-file.md"), "section") + ); + // `./`-prefixed form (which dodges the ccTLD heuristic) also splits the fragment. + assert_eq!( + resolve(&app, &links, "./other-file.md#section").await, + local_file_anchor(base_path.join("other-file.md"), "section") + ); + // No fragment → no anchor. + assert_eq!( + resolve(&app, &links, "./other-file.md").await, + local_file(base_path.join("other-file.md")) + ); + // Only the final `#…` is treated as the fragment; earlier `#` stays in the path. + assert_eq!( + resolve(&app, &links, "./multi#hash.md#frag").await, + local_file_anchor(base_path.join("multi#hash.md"), "frag") + ); + // A trailing empty fragment (`file.md#`) resolves the file with no anchor. + assert_eq!( + resolve(&app, &links, "./other-file.md#").await, + local_file(base_path.join("other-file.md")) + ); + }); +} + +#[test] +fn test_fragment_split_preserves_line_number_routing() { + // The `#L100` line-number suffix must continue to route through the existing line/column + // path (handled by `CleanPathResult`), not be peeled as an anchor fragment. + App::test((), |mut app| async move { + let base = tempdir().unwrap(); + let base_path = base.path(); + touch(base_path.join("src/main.rs")).await; + let links = init_link_model(&mut app, Some(base_path)); + + assert_eq!( + resolve(&app, &links, "./src/main.rs#L100").await, + local_file_location(base_path.join("src/main.rs"), 100, None) + ); + assert_eq!( + resolve(&app, &links, "./src/main.rs#L100:50").await, + local_file_location(base_path.join("src/main.rs"), 100, Some(50)) + ); + }); +} + #[test] fn test_open_local_image_uses_system_generic_target() { App::test((), |mut app| async move { @@ -418,9 +628,9 @@ fn test_open_markdown_file_uses_viewer_when_preferred() { links .update(&mut app, |links, ctx| { - // The `./` in the link is important: `.md` is the TLD for Moldova, so this will be - // resolved as a web link otherwise. - let future = links.resolve_and_open("./README.md", ctx); + // With the ccTLD repair, a bare `README.md` that exists on disk resolves as a + // file rather than the `.md` Moldova domain, so no `./` prefix is needed here. + let future = links.resolve_and_open("README.md", ctx); ctx.await_spawned_future(future.future_id()) }) .await; @@ -428,9 +638,135 @@ fn test_open_markdown_file_uses_viewer_when_preferred() { let events = events.lock(); assert_eq!(events.len(), 1); match events.first() { - Some(LinkEvent::OpenFileNotebook { path, session }) => { + Some(LinkEvent::OpenFileNotebook { + path, + session, + anchor, + }) => { assert_eq!(path, &root.join("README.md")); - assert!(Arc::ptr_eq(&TEST_SESSION, session)); + assert!(session + .as_ref() + .is_some_and(|s| Arc::ptr_eq(&TEST_SESSION, s))); + assert_eq!(anchor, &None); + } + other => panic!("Expected OpenFileNotebook event, got {other:?}"), + } + }); +} + +#[test] +fn test_cross_document_fragment_threads_anchor_to_open_event() { + // A `file.md#section` cross-document link must carry its `section` anchor all the way to the + // emitted `OpenFileNotebook` event, so the destination notebook can perform a deferred + // scroll to the heading once it loads. + App::test((), |mut app| async move { + let base = tempdir().unwrap(); + let base_path = base.path(); + touch(base_path.join("other-file.md")).await; + let links = init_link_model(&mut app, Some(base_path)); + + let events = Arc::new(Mutex::new(vec![])); + { + let events = events.clone(); + app.update(|ctx| { + ctx.subscribe_to_model(&links, move |_, event, _| { + events.lock().push(event.clone()); + }) + }); + } + + links + .update(&mut app, |links, ctx| { + let future = links.resolve_and_open("other-file.md#target-section", ctx); + ctx.await_spawned_future(future.future_id()) + }) + .await; + + let events = events.lock(); + assert_eq!(events.len(), 1); + match events.first() { + Some(LinkEvent::OpenFileNotebook { + path, + session, + anchor, + }) => { + assert_eq!(path, &base_path.join("other-file.md")); + assert!(session + .as_ref() + .is_some_and(|s| Arc::ptr_eq(&TEST_SESSION, s))); + assert_eq!(anchor.as_deref(), Some("target-section")); + } + other => panic!("Expected OpenFileNotebook event, got {other:?}"), + } + }); +} + +#[test] +fn test_cross_document_link_resolves_without_session() { + // Regression (#13725): a standalone Markdown viewer tab — opened from Finder / `open -a Warp + // file.md` — has a valid base directory (the document's own dir) but *no* terminal session. + // Resolving a cross-document file link must still succeed against that base directory. Before + // the fix, `resolve` fell through to its session `match` and returned `Err(MissingContext)` + // for the no-session case even though the file existed, so every relative/cross-document link + // was a silent no-op in the standalone viewer while in-document `#fragment` links (which never + // touch the resolver) kept working. + App::test((), |mut app| async move { + let base = tempdir().unwrap(); + let base_path = base.path(); + touch(base_path.join("other-file.md")).await; + let links = init_link_model_no_session(&mut app, base_path); + + let resolved = links + .read(&app, |links, ctx| links.resolve("other-file.md", ctx)) + .await + .expect("cross-document link should resolve without a session"); + assert_eq!( + resolved, + local_file_no_session(base_path.join("other-file.md")) + ); + }); +} + +#[test] +fn test_cross_document_link_opens_in_viewer_without_session() { + // The end-to-end standalone-viewer path: `resolve_and_open` must emit `OpenFileNotebook` (with + // no session) so the workspace opens the target document. This is the seam the live GUI failure + // exercised — `resolve_and_open` silently drops a resolve `Err`, so a session-gated failure + // produced no event and no visible action. + App::test((), |mut app| async move { + let base = tempdir().unwrap(); + let base_path = base.path(); + touch(base_path.join("other-file.md")).await; + let links = init_link_model_no_session(&mut app, base_path); + + let events = Arc::new(Mutex::new(vec![])); + { + let events = events.clone(); + app.update(|ctx| { + ctx.subscribe_to_model(&links, move |_, event, _| { + events.lock().push(event.clone()); + }) + }); + } + + links + .update(&mut app, |links, ctx| { + let future = links.resolve_and_open("other-file.md#far-section", ctx); + ctx.await_spawned_future(future.future_id()) + }) + .await; + + let events = events.lock(); + assert_eq!(events.len(), 1); + match events.first() { + Some(LinkEvent::OpenFileNotebook { + path, + session, + anchor, + }) => { + assert_eq!(path, &base_path.join("other-file.md")); + assert!(session.is_none()); + assert_eq!(anchor.as_deref(), Some("far-section")); } other => panic!("Expected OpenFileNotebook event, got {other:?}"), } diff --git a/app/src/notebooks/notebook.rs b/app/src/notebooks/notebook.rs index a846bdd9aab..d848bb5622f 100644 --- a/app/src/notebooks/notebook.rs +++ b/app/src/notebooks/notebook.rs @@ -337,7 +337,7 @@ impl NotebookView { ctx.observe(&active_notebook_data, Self::handle_active_notebook_change); let window_id = ctx.window_id(); - let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::Active(window_id), ctx)); + let links = ctx.add_model(|ctx| NotebookLinks::new(SessionSource::active(window_id), ctx)); let title = ctx.add_typed_action_view(|ctx| { let appearance = Appearance::as_ref(ctx); diff --git a/app/src/pane_group/mod.rs b/app/src/pane_group/mod.rs index b63634e23de..e0d0989d7a7 100644 --- a/app/src/pane_group/mod.rs +++ b/app/src/pane_group/mod.rs @@ -549,8 +549,12 @@ pub enum Event { OpenFileInWarp { /// The file path to open. path: LocalOrRemotePath, - /// The session that the path was opened from. - session: Arc, + /// The session that the path was opened from, if any. `None` when the link was opened in a + /// standalone Markdown viewer tab, which has no terminal session. + session: Option>, + /// A `#fragment` anchor to scroll to once the destination Markdown notebook loads, if the + /// originating link carried one. `None` for a plain file open. + anchor: Option, }, OpenWarpDriveLink { open_warp_drive_args: OpenWarpDriveObjectArgs, @@ -1746,6 +1750,7 @@ impl PaneGroup { NotebookPaneSnapshot::LocalFileNotebook { path } => Box::new(FilePane::new( path.map(LocalOrRemotePath::Local), None, + None, #[cfg(feature = "local_fs")] None, ctx, @@ -5036,7 +5041,7 @@ impl PaneGroup { } }); - let file_pane = FilePane::new(Some(path), session, source, ctx); + let file_pane = FilePane::new(Some(path), session, None, source, ctx); let success = self.replace_pane(code_pane_id, file_pane, false, ctx); if !success { diff --git a/app/src/pane_group/pane/file_pane.rs b/app/src/pane_group/pane/file_pane.rs index 23f921b0bd5..5c8a74f8a84 100644 --- a/app/src/pane_group/pane/file_pane.rs +++ b/app/src/pane_group/pane/file_pane.rs @@ -40,9 +40,13 @@ impl FilePane { /// is `None`, the pane is created but left empty. For local paths without a target session, /// the pane waits for a local session to become active. Remote paths are loaded directly /// via the remote server. + /// + /// `anchor` is an optional `#fragment` to scroll to once the notebook's Markdown layout is + /// built — set when a cross-document link (`other-file.md#section`) opens this pane. pub fn new( path: Option, target_session: Option>, + anchor: Option, #[cfg(feature = "local_fs")] code_source: Option, ctx: &mut ViewContext, ) -> Self { @@ -53,6 +57,12 @@ impl FilePane { if let Some(path) = path { view.open(path, target_session, ctx); + // Queue the deferred scroll before the file finishes loading: the layout that + // resolves the heading offset happens asynchronously, and the pending anchor is + // drained on the first `LayoutUpdated`. + if let Some(anchor) = anchor { + view.queue_pending_anchor(anchor, ctx); + } } view diff --git a/app/src/pane_group/pane/notebook_pane.rs b/app/src/pane_group/pane/notebook_pane.rs index d13d2521e66..60e49400037 100644 --- a/app/src/pane_group/pane/notebook_pane.rs +++ b/app/src/pane_group/pane/notebook_pane.rs @@ -172,11 +172,16 @@ pub(super) fn subscribe_to_link_model( ctx: &mut ViewContext, ) { ctx.subscribe_to_model(handle, move |pane_group, _, event, ctx| match event { - LinkEvent::OpenFileNotebook { path, session } => { + LinkEvent::OpenFileNotebook { + path, + session, + anchor, + } => { // Opening local files is delegated to the parent workspace. ctx.emit(crate::pane_group::Event::OpenFileInWarp { path: crate::code::buffer_location::LocalOrRemotePath::Local(path.clone()), session: session.clone(), + anchor: anchor.clone(), }) } LinkEvent::OpenWarpDriveLink { diff --git a/app/src/pane_group/pane/terminal_pane.rs b/app/src/pane_group/pane/terminal_pane.rs index ae2071ba924..1d5f7eb3d2c 100644 --- a/app/src/pane_group/pane/terminal_pane.rs +++ b/app/src/pane_group/pane/terminal_pane.rs @@ -1097,7 +1097,10 @@ fn handle_terminal_view_event( Event::OpenFileInWarp { path, session } => { ctx.emit(pane_group::Event::OpenFileInWarp { path: LocalOrRemotePath::Local(path.clone()), - session: session.clone(), + // A terminal always has a session backing the file-path click. + session: Some(session.clone()), + // Terminal file-path clicks never carry a Markdown anchor fragment. + anchor: None, }); } #[cfg(feature = "local_fs")] diff --git a/app/src/workspace/home.rs b/app/src/workspace/home.rs index 7dbd0e8c375..3f4c457fa2b 100644 --- a/app/src/workspace/home.rs +++ b/app/src/workspace/home.rs @@ -21,6 +21,7 @@ Warp on Web can also be used by your teammates and peers who don't have Warp dow /// Create a static "home page" pane. pub fn create_home_pane(ctx: &mut ViewContext) -> Box { let pane = FilePane::new( + None, None, None, #[cfg(feature = "local_fs")] diff --git a/app/src/workspace/view.rs b/app/src/workspace/view.rs index 11ecae62f6d..d91f3ad9cb7 100644 --- a/app/src/workspace/view.rs +++ b/app/src/workspace/view.rs @@ -968,6 +968,24 @@ fn query_for_rewind_prefill(inputs: &[AIAgentInput]) -> Option { inputs.iter().find_map(AIAgentInput::display_query) } +/// Canonicalize a local file target so it compares equal to an open notebook's stored path +/// during dedup. Open notebooks record their *canonical* path on load (via `CanonicalizedPath`, +/// i.e. `dunce::canonicalize`), while a clicked link resolves to `base_directory.join(relative)` +/// — which keeps `.`/`..` components and, on macOS, the `/tmp` vs `/private/tmp` symlink alias. +/// Canonicalizing with the same function lets a self-referential link (`./this-doc.md`) and any +/// other spelling of the same file dedup to the already-open pane instead of opening a duplicate. +/// Remote paths and paths that can't be canonicalized (e.g. the file vanished between resolve and +/// open) are returned unchanged. +#[cfg(feature = "local_fs")] +fn canonicalize_local_path_for_dedup(path: LocalOrRemotePath) -> LocalOrRemotePath { + match &path { + LocalOrRemotePath::Local(local) => dunce::canonicalize(local) + .map(LocalOrRemotePath::Local) + .unwrap_or(path), + LocalOrRemotePath::Remote(_) => path, + } +} + /// Snapshot of a tab used to move it between workspaces or into a new window. /// Built by `Workspace::tab_transfer_info_at_index` and consumed by /// `insert_transferred_tab_at_index`. Captures the pane group handle, visual @@ -6277,6 +6295,7 @@ impl Workspace { LocalOrRemotePath::Local(path.clone()), session, layout, + None, ctx, ); } @@ -6390,7 +6409,7 @@ impl Workspace { // Jupyter notebook) instead of always opening remote // files as raw code in the editor. if let FileTarget::MarkdownViewer(layout) = target { - self.open_file_notebook(location.clone(), None, *layout, ctx); + self.open_file_notebook(location.clone(), None, *layout, None, ctx); } else { self.open_code( code_source, @@ -8483,8 +8502,17 @@ impl Workspace { path: LocalOrRemotePath, session: Option>, layout: EditorLayout, + anchor: Option, ctx: &mut ViewContext, ) { + // Canonicalize a local target before the dedup lookup. An open notebook stores its + // *canonical* path (recorded on load via `CanonicalizedPath`), but a link resolves to + // `base_directory.join(relative)`, which keeps `.`/`..` components and, on macOS, the + // `/tmp` vs `/private/tmp` symlink alias. Without canonicalizing, a self-referential + // link (`./this-doc.md`) wouldn't match its own open tab and would open a duplicate. + // Matching to canonical form makes self-reference — and any two spellings of the same + // file — dedup to the same pane. + let path = canonicalize_local_path_for_dedup(path); let existing_file_pane = { let pane_group = self.active_tab_pane_group(); pane_group @@ -8494,18 +8522,25 @@ impl Workspace { !pane_group.as_ref(ctx).is_pane_hidden_for_close(*pane_id) && file_view.as_ref(ctx).path() == Some(&path) }) - .map(|(pane_id, _)| pane_id) }; - if let Some(pane_id) = existing_file_pane { + if let Some((pane_id, file_view)) = existing_file_pane { self.active_tab_pane_group().update(ctx, |pane_group, ctx| { pane_group.focus_pane_by_id(pane_id, ctx); }); + // The tab is already open and laid out, so a cross-document anchor can scroll + // immediately — no deferral needed. A miss is a silent no-op. + if let Some(anchor) = anchor { + file_view.update(ctx, |view, ctx| { + view.scroll_to_anchor(&anchor, ctx); + }); + } return; } let pane = FilePane::new( Some(path), session, + anchor, #[cfg(feature = "local_fs")] None, ctx, @@ -16143,11 +16178,21 @@ impl Workspace { ); } #[cfg_attr(not(feature = "local_fs"), allow(unused_variables))] - pane_group::Event::OpenFileInWarp { path, session } => { + pane_group::Event::OpenFileInWarp { + path, + session, + anchor, + } => { #[cfg(feature = "local_fs")] { let layout = *EditorSettings::as_ref(ctx).open_file_layout.value(); - self.open_file_notebook(path.clone(), Some(session.clone()), layout, ctx); + self.open_file_notebook( + path.clone(), + session.clone(), + layout, + anchor.clone(), + ctx, + ); } } pane_group::Event::MoveToSpace { diff --git a/app/src/workspace/view_tests.rs b/app/src/workspace/view_tests.rs index d7400ddb205..c481f89ce1d 100644 --- a/app/src/workspace/view_tests.rs +++ b/app/src/workspace/view_tests.rs @@ -4490,3 +4490,54 @@ fn test_tools_panel_warp_drive_toggle_updates_available_views() { }); }); } + +#[cfg(feature = "local_fs")] +#[test] +fn test_canonicalize_local_path_for_dedup_normalizes_self_reference() { + // Self-referential dedup relies on `canonicalize_local_path_for_dedup` producing the same + // path an open notebook stores (which is canonicalized on load). This exercises the two + // ways a self-link's resolved path diverges from the stored canonical path: `.`/`..` + // components from `base_directory.join("./doc.md")`, and the macOS `/tmp` vs `/private/tmp` + // symlink alias this test environment actually hits. + let dir = tempfile::tempdir().expect("temp dir"); + let doc = dir.path().join("doc.md"); + std::fs::write(&doc, "# Doc\n").expect("write doc"); + + // The canonical form an open notebook would store for this file. + let canonical = dunce::canonicalize(&doc).expect("canonicalize doc"); + + // A `./`-relative resolution: base_directory + "./doc.md" keeps the `.` component. + let dot_relative = dir.path().join(".").join("doc.md"); + assert_eq!( + canonicalize_local_path_for_dedup(LocalOrRemotePath::Local(dot_relative)), + LocalOrRemotePath::Local(canonical.clone()), + "A './'-relative self-link must canonicalize to the stored canonical path" + ); + + // A `..`-round-trip resolution: base + "sub/../doc.md" (sub need not exist for the string + // form, but canonicalize requires real components, so create it). + std::fs::create_dir(dir.path().join("sub")).expect("mkdir sub"); + let dotdot_relative = dir.path().join("sub").join("..").join("doc.md"); + assert_eq!( + canonicalize_local_path_for_dedup(LocalOrRemotePath::Local(dotdot_relative)), + LocalOrRemotePath::Local(canonical.clone()), + "A '..'-round-trip self-link must canonicalize to the stored canonical path" + ); + + // The raw tempdir path itself (which on macOS is under `/var/folders/.../T` or `/tmp`, + // possibly a symlink) must also canonicalize to the same target — guarding the symlink-alias + // case the addendum called out. + assert_eq!( + canonicalize_local_path_for_dedup(LocalOrRemotePath::Local(doc.clone())), + LocalOrRemotePath::Local(canonical), + "The as-resolved path must canonicalize to the stored canonical path (symlink alias)" + ); + + // A path that can't be canonicalized (does not exist) is returned unchanged, not dropped. + let missing = dir.path().join("does-not-exist.md"); + assert_eq!( + canonicalize_local_path_for_dedup(LocalOrRemotePath::Local(missing.clone())), + LocalOrRemotePath::Local(missing), + "A non-existent path must pass through unchanged" + ); +} diff --git a/crates/markdown_parser/src/markdown_parser.rs b/crates/markdown_parser/src/markdown_parser.rs index 1e67e7f43da..4451a0f2faf 100644 --- a/crates/markdown_parser/src/markdown_parser.rs +++ b/crates/markdown_parser/src/markdown_parser.rs @@ -1035,6 +1035,28 @@ fn parse_inline<'a, E: ContextError<&'a str> + ParseError<&'a str>>( InlineToken::UnderlineEnd => { input = parse_underline(&mut state, remaining); } + InlineToken::HtmlAnchorStart { href, raw } => { + // Push the raw opening tag as the literal fallback (used if no `` closes it), + // mirroring how `LinkStart` stores its `[` placeholder, and stash the href on the + // delimiter for close-time styling. + let node_index = state.nodes.len(); + let mut delimiter = Delimiter::new( + node_index, + DelimiterKind::HtmlAnchorStart, + 1, + state + .nodes + .last() + .and_then(|fragment| fragment.text.chars().last()), + remaining.chars().next(), + ); + delimiter.anchor_href = Some(href); + state.push_closed_node(FormattedTextFragment::plain_text(raw)); + state.delimiters.push(delimiter); + } + InlineToken::HtmlAnchorEnd => { + input = parse_html_anchor(&mut state, remaining); + } } } @@ -1316,6 +1338,53 @@ fn parse_underline<'a>(state: &mut InlineState, remaining: &'a str) -> &'a str { remaining } +/// Resolve a raw HTML `` anchor when `` is encountered, using the same +/// backtracking approach as [`parse_link`]. Applies [`Hyperlink::Url`] styling to the fragments +/// between the opening tag and ``. +fn parse_html_anchor<'a>(state: &mut InlineState, remaining: &'a str) -> &'a str { + let Some((anchor_start_index, anchor_start)) = state + .delimiters + .iter() + .enumerate() + .rev() + .find(|(_, delimiter)| delimiter.kind == DelimiterKind::HtmlAnchorStart) + else { + // No opening tag — treat this as literal `` text. + state.push_text(""); + return remaining; + }; + + let anchor_start_node = anchor_start.node_index; + let href = anchor_start + .anchor_href + .clone() + .expect("HtmlAnchorStart delimiter must carry an href"); + + // Apply link styling to every fragment between the opening tag and ``. + state.backtrack_styles(anchor_start_node, |styles| { + styles.hyperlink = Some(Hyperlink::Url(href.clone())) + }); + + // Process inline styling within the anchor body, bounded by the opening tag. + process_emphasis(state, Some(anchor_start_index)); + + // Consume the opening delimiter and deactivate any prior link/anchor starts to prevent nesting. + state.delimiters.remove(anchor_start_index); + for delimiter in &mut state.delimiters[..anchor_start_index] { + if matches!( + delimiter.kind, + DelimiterKind::LinkStart | DelimiterKind::HtmlAnchorStart + ) { + delimiter.active = false; + } + } + + // Drop the placeholder fragment holding the raw opening tag; the styling now lives on the body. + state.remove_node(anchor_start_node); + state.last_node_closed = true; + remaining +} + /// Process emphasis delimiters on the state's delimiter stack, bounded by `stack_bottom`. /// /// This is approximately equivalent to the CommonMark [process emphasis](https://spec.commonmark.org/0.30/#phase-2-inline-structure) @@ -1549,6 +1618,8 @@ fn parse_inline_token<'a, E: ContextError<&'a str> + ParseError<&'a str>>( parse_inline_token_autolink, parse_inline_token_underline_start, parse_inline_token_underline_end, + parse_inline_token_html_anchor_start, + parse_inline_token_html_anchor_end, whitespace, text, // This _must_ be the last parser in the chain. It unconditionally consumes a single @@ -1654,6 +1725,80 @@ fn parse_inline_token_underline_end<'a, E: ContextError<&'a str> + ParseError<&' )(input) } +/// Parse a single `key="value"` or `key='value'` HTML attribute, returning `(key, value)`. +/// Whitespace-insensitive around `=`. Bare (valueless) attributes are not supported — every +/// attribute the anchor grammar cares about (`href`) is always quoted in hand-authored markup. +fn parse_html_attribute<'a, E: ContextError<&'a str> + ParseError<&'a str>>( + input: &'a str, +) -> IResult<&'a str, (&'a str, &'a str), E> { + let (input, key) = + take_while1(|c: char| c.is_ascii_alphanumeric() || c == '-' || c == '_')(input)?; + let (input, _) = space0(input)?; + let (input, _) = char('=')(input)?; + let (input, _) = space0(input)?; + let (input, value) = alt(( + delimited(char('"'), take_while(|c| c != '"'), char('"')), + delimited(char('\''), take_while(|c| c != '\''), char('\'')), + ))(input)?; + Ok((input, (key, value))) +} + +/// Parse an opening `` tag into an [`InlineToken::HtmlAnchorStart`]. Requires an +/// `href` attribute (the phase-1 hyperlink form); other attributes (`title`, `target`, `class`, +/// …) are accepted and discarded. An `` tag with no `href` (e.g. a bare `` anchor +/// target) does not match here — that is a separate phase-2 concern. +fn parse_inline_token_html_anchor_start<'a, E: ContextError<&'a str> + ParseError<&'a str>>( + input: &'a str, +) -> IResult<&'a str, InlineToken<'a>, E> { + context("html_anchor_start", |input: &'a str| { + let (rest, (raw, href)) = consumed(|input: &'a str| { + let (input, _) = tag_no_case("` or similar. + let (input, _) = space1(input)?; + + let mut input = input; + let mut href: Option<&'a str> = None; + loop { + let (next, _) = space0(input)?; + input = next; + if let Ok((next, _)) = char::<_, E>('>')(input) { + input = next; + break; + } + let (next, (key, value)) = parse_html_attribute(input)?; + if key.eq_ignore_ascii_case("href") { + href = Some(value); + } + input = next; + } + + match href { + Some(href) => Ok((input, href)), + None => Err(nom::Err::Error(make_error(input, ErrorKind::Tag))), + } + })(input)?; + + Ok(( + rest, + InlineToken::HtmlAnchorStart { + href: href.to_string(), + raw, + }, + )) + })(input) +} + +/// Parse a closing `` into an [`InlineToken::HtmlAnchorEnd`]. +fn parse_inline_token_html_anchor_end<'a, E: ContextError<&'a str> + ParseError<&'a str>>( + input: &'a str, +) -> IResult<&'a str, InlineToken<'a>, E> { + context( + "html_anchor_end", + map(tag_no_case(""), |_| InlineToken::HtmlAnchorEnd), + )(input) +} + /// Helper to parse a run of delimiters. fn parse_delimiter_run<'a, E: ContextError<&'a str> + ParseError<&'a str>>( kind: DelimiterKind, @@ -1700,6 +1845,11 @@ enum InlineToken<'a> { LinkEnd, /// A closing , which triggers underline parsing. UnderlineEnd, + /// An opening `` tag. Carries the parsed `href` value and the original tag text + /// (used as the literal fallback if no matching `` is found). + HtmlAnchorStart { href: String, raw: &'a str }, + /// A closing ``, which triggers anchor parsing. + HtmlAnchorEnd, } /// An entry in the [delimiter stack](https://spec.commonmark.org/0.30/#delimiter-stack) @@ -1720,6 +1870,10 @@ struct Delimiter { can_open: bool, /// Whether or not this delimiter can close a strong/emphasis range. can_close: bool, + /// For a [`DelimiterKind::HtmlAnchorStart`], the `href` value of the opening `` tag, + /// applied as a [`Hyperlink::Url`] when the matching `` is parsed. `None` for every + /// other delimiter kind. + anchor_href: Option, } impl Delimiter { @@ -1750,7 +1904,8 @@ impl Delimiter { && (!preceded_by_punctuation || (followed_by_whitespace || followed_by_punctuation)); let can_open = match kind { - DelimiterKind::LinkStart => false, + // Anchors, like links, are resolved explicitly rather than by emphasis flanking. + DelimiterKind::LinkStart | DelimiterKind::HtmlAnchorStart => false, DelimiterKind::Asterisk => left_flanking, DelimiterKind::Underscore => { left_flanking && (!right_flanking || preceded_by_punctuation) @@ -1761,7 +1916,7 @@ impl Delimiter { }; let can_close = match kind { - DelimiterKind::LinkStart => false, + DelimiterKind::LinkStart | DelimiterKind::HtmlAnchorStart => false, DelimiterKind::Asterisk => right_flanking, DelimiterKind::Underscore => { right_flanking && (!left_flanking || followed_by_punctuation) @@ -1778,6 +1933,7 @@ impl Delimiter { can_open, active: true, node_index, + anchor_href: None, } } @@ -1819,6 +1975,9 @@ enum DelimiterKind { LinkStart, Strikethrough, UnderlineStart, + /// The opening `` of a raw HTML anchor. Resolved explicitly when `` is + /// encountered (like `LinkStart`), so it never participates in emphasis flanking. + HtmlAnchorStart, } impl DelimiterKind { @@ -1832,6 +1991,7 @@ impl DelimiterKind { // tildes do not create strikethrough. DelimiterKind::Strikethrough => count <= 2, DelimiterKind::UnderlineStart => count == 1, + DelimiterKind::HtmlAnchorStart => count == 1, } } @@ -1842,6 +2002,10 @@ impl DelimiterKind { DelimiterKind::LinkStart => "[", DelimiterKind::Strikethrough => "~", DelimiterKind::UnderlineStart => "", + // An anchor's opening tag is variable-length, so its literal fallback text is stored + // on the placeholder fragment at parse time rather than reconstructed from a static + // string. `to_text`/`truncate_delimiters` are never invoked for this kind. + DelimiterKind::HtmlAnchorStart => "", } } } diff --git a/crates/markdown_parser/src/markdown_parser_tests.rs b/crates/markdown_parser/src/markdown_parser_tests.rs index d347c01c981..13c8316b42a 100644 --- a/crates/markdown_parser/src/markdown_parser_tests.rs +++ b/crates/markdown_parser/src/markdown_parser_tests.rs @@ -1981,6 +1981,112 @@ fn test_parse_empty_underline() { ) } +#[test] +fn test_parse_html_anchor_external_href() { + // Invariant 1: `` produces a Hyperlink::Url identical in shape to a markdown link. + assert_eq!( + parse_all("Visit Warp", parse_inline), + vec![FormattedTextFragment::hyperlink( + "Visit Warp", + "https://warp.dev" + )] + ); + // The markdown-native equivalent produces the same fragment. + assert_eq!( + parse_all("Visit Warp", parse_inline), + parse_all("[Visit Warp](https://warp.dev)", parse_inline), + ); +} + +#[test] +fn test_parse_html_anchor_fragment_href() { + // Invariant 2: an in-page `#fragment` href parses as Hyperlink::Url("#…"); resolution is + // tested separately in the editor model. + assert_eq!( + parse_all( + "Jump to Target Section", + parse_inline + ), + vec![FormattedTextFragment::hyperlink( + "Jump to Target Section", + "#target-section" + )] + ); +} + +#[test] +fn test_parse_html_anchor_single_quotes() { + assert_eq!( + parse_all("Warp", parse_inline), + vec![FormattedTextFragment::hyperlink("Warp", "https://warp.dev")] + ); +} + +#[test] +fn test_parse_html_anchor_ignores_other_attributes() { + // Invariant 8: attributes beyond `href` are parsed-but-ignored — no effect on output. + assert_eq!( + parse_all( + "Warp", + parse_inline + ), + vec![FormattedTextFragment::hyperlink("Warp", "https://warp.dev")] + ); + // `href` need not be the first attribute. + assert_eq!( + parse_all( + "Warp", + parse_inline + ), + vec![FormattedTextFragment::hyperlink("Warp", "https://warp.dev")] + ); +} + +#[test] +fn test_parse_html_anchor_inline_styling_in_body() { + // Invariant 9: inline styling inside the anchor body is preserved, and each styled fragment + // still carries the hyperlink. + let link = |text: &str| FormattedTextFragment { + text: text.to_string(), + styles: FormattedTextStyles { + hyperlink: Some(Hyperlink::Url("https://warp.dev".to_string())), + ..Default::default() + }, + }; + let mut bold_link = link("bold"); + bold_link.styles.weight = Some(CustomWeight::Bold); + assert_eq!( + parse_all( + "a **bold** link", + parse_inline + ), + vec![link("a "), bold_link, link(" link")] + ); +} + +#[test] +fn test_parse_html_anchor_unterminated_falls_back_to_text() { + // Invariant 10: a missing closing `` degrades to literal text without swallowing the + // rest of the paragraph. + assert_eq!( + parse_all("no close", parse_inline), + vec![FormattedTextFragment::plain_text( + "no close" + )] + ); + // A stray closing tag with no opener is literal text too. + assert_eq!( + parse_all("just here", parse_inline), + vec![FormattedTextFragment::plain_text("just here")] + ); + // An `` with no href at all is not a phase-1 hyperlink — it stays literal (its `` + // handling is a phase-2 concern). + assert_eq!( + parse_all("", parse_inline), + vec![FormattedTextFragment::plain_text("")] + ); +} + #[test] fn test_unordered_list_indentation_level_relative() { // Test that both 2-space and 4-space relative indentation produce the same structure diff --git a/specs/GH13725/product.md b/specs/GH13725/product.md new file mode 100644 index 00000000000..5a3d1869528 --- /dev/null +++ b/specs/GH13725/product.md @@ -0,0 +1,236 @@ +# PRODUCT.md — Markdown viewer: ``/`` anchor links + +Issue: https://github.com/warpdotdev/warp/issues/13725 + +Split from: #13652 (bulk raw-HTML-subset request, closed in favor of per-feature issues). +Sibling specs in the same split: `` sizing (#13721), raw HTML tables (#13726, +`specs/GH13652/tables/`), `
`/`` (#10259), `
` (#13732), `` +(#13733), ``/`` (#13734), `align` (#13735), ``/`` (#13736). + +## Summary + +Hand-built READMEs and docs commonly pair `
` (or `name="section"`) anchor +targets with `[Jump to Section](#section)` or `` links, either as a +manual table of contents or as inline cross-references. Warp's Markdown viewer supports +neither half of this pattern today: raw ``/`` tags render as literal text +(there is no HTML-tag token in the inline parser at all), and even the markdown-native +`[text](#fragment)` form — which **does** parse today as a link whose target is the literal +string `#fragment` — has nothing to resolve that fragment against, because no heading or +anchor in the document carries an id/slug of any kind. + +This request is therefore two related but separable capabilities: + +1. **Raw HTML anchor tags.** Recognize inline `` as a hyperlink (reusing + the existing `Hyperlink` link styling) and ``/`` as a named anchor + target attached to the surrounding content. +2. **Fragment resolution + scroll-to.** Give every heading an implicit anchor (a slug + derived from its text, GitHub-style), let an explicit `` register an anchor too, + and make any link whose target is `#fragment` — whether it came from `` or from + markdown-native `[text](#fragment)` — scroll the viewer to the matching anchor instead + of falling through to plain-URL handling. + +Capability (2) is the harder half and is also what markdown-native links need — a +`[text](#fragment)` link already parses correctly and gets no benefit from HTML anchor +parsing at all. The issue explicitly frames working in-document anchor links as a +prerequisite for any future table-of-contents feature (#13083, #4720). + +Figma: none provided. + +## Goals / Non-goals + +In scope: + +- Parse an inline `link text` as a hyperlink with the given text, styled + and clickable exactly like a markdown `[link text](…)` — including external URLs + (`https://…`) and in-page fragments (`#target`). +- Parse `` and `` (empty or self-closing, the common + hand-authored form) as a named anchor target at that point in the document. **As shipped, + the tag itself still renders as visible literal text** (e.g. `` appears + inline in the document) rather than rendering nothing the way GitHub/browsers do — see the + note under invariant 5 and #13982 for why, and for the deferred follow-up that would hide + it. +- Give every heading (`#`…`######`) an **implicit** anchor slug derived from its rendered + text, so `[Jump to Target Section](#target-section)` works against ordinary headings with + zero authoring effort — the common case the issue's test document exercises. +- Resolve a `#fragment` hyperlink click (from either an `` tag or a markdown + `[text](#fragment)` link) against the set of anchors in the current document — explicit + ``/`` targets and implicit heading slugs — and scroll the viewer so the + target is visible, instead of the current behavior (treated as an opaque URL). +- **Cross-document fragment links** — a link whose target combines a relative file path and + a fragment, e.g. `[text](other-file.md#section)`. Clicking it opens (or, if already open, + focuses) that file's Markdown-viewer tab and scrolls it to the matching anchor once the + document has loaded. A relative file link *without* a fragment already opens the target + today; this in-scope item is specifically the fragment half — carrying the `#section` + through the file-open flow and scrolling after load. **Delivered in this PR** alongside + phase 1 (see phasing), because it builds on the same-document slug resolver rather than + replacing it, and because implementation surfaced three latent resolution defects + (documented in the tech spec) that had to be repaired for even a fragment-less bare + `README.md` link to open reliably. +- A `#fragment` link with no matching anchor in the document degrades gracefully: it + remains a normally-styled, clickable-looking link, but clicking it is a no-op (no + navigation, no error, no crash) rather than attempting to open `#fragment` as a URL. +- `` attributes beyond `href` — `title`, `target`, `rel`, `class`, etc. — are + parsed-but-ignored: they don't break parsing, and they don't do anything (no `target` + window semantics, no HTML tooltip from `title`). + +Out of scope (explicit non-goals): + +- **Following a cross-document fragment link into a non-Markdown target, or into an + external editor.** The cross-document scroll-to-anchor (in-scope above, phase 3) applies + only when the target file opens in Warp's Markdown viewer. If the user's Markdown Viewer + preference is off — so the file opens in the code editor or an external app — the + fragment is dropped and only the file opens, matching how a plain relative file link + behaves today. Bridging a `#slug` anchor to a code-editor line is a separate concern. +- **Cross-document anchor scroll in the terminal (TUI) Markdown renderer.** Same rationale + as the same-document TUI non-goal below — the TUI has no scroll model or click-to-open + path. `other-file.md#section` in the TUI renders as inert styled text. +- **Slug-collision policy beyond "first wins."** If two headings produce the same slug + (e.g. two headings both literally titled "Overview"), only the first is addressable by + that slug — matching common Markdown-renderer behavior (GitHub disambiguates duplicates + by appending `-1`, `-2`, …; replicating that exact disambiguation scheme is left to the + tech spec's judgment, not mandated here). +- **`` tags with both `href` and `id`/`name` on the same element.** The issue's test + case and the common real-world pattern always use them separately (`` as a bare + target, `` as the link). Supporting both roles on one tag is not required. +- **Any other raw-HTML tag** (``, ``, `
`, etc.) — each has its own + spec in this split. +- **Editing/authoring affordances** — e.g. no "copy anchor link" UI, no auto-slug preview + while typing a heading. This is a *rendering/navigation* feature only. +- **URL scheme validation or link-target security beyond what markdown links already do.** + `` reuses the exact same `Hyperlink::Url` styling and click path as markdown + links; it inherits that trust boundary as-is rather than introducing a new one. +- **Anchor scroll-to in the terminal (TUI) Markdown renderer.** `` links will + *render* in the TUI viewer for free (it shares the parser), but clicking a `#fragment` + there does not scroll — the TUI has no scroll model or click-to-navigate path. Fragment + resolution is a GUI-viewer feature in this slice. + +## Behavior + +1. `Visit Warp` renders as a clickable link reading "Visit + Warp", visually and behaviorally identical to the markdown link + `[Visit Warp](https://warp.dev)`. + +2. `Jump to Target Section` renders as a clickable link. + Clicking it scrolls the viewer so the heading (or explicit anchor) matching + `target-section` is visible. This is the HTML-tag half of the issue's test case. + +3. The markdown-native equivalent, `[Jump to Target Section](#target-section)`, gets the + **same** scroll-to-target click behavior as invariant 2 — it already parses as a link + today; only fragment *resolution* is new. This is the contrasting case named explicitly + in the issue ("resolves as a plain URL hyperlink" today). + +4. A heading `## Target Section` is addressable by `#target-section` (its GitHub-style + slug: lowercased, spaces to hyphens, punctuation stripped) with no authoring effort — + no `` required. This is what makes invariants 2 and 3 work against the issue's + test document out of the box. + +5. `` (or ``) placed anywhere in + the document — most commonly immediately before a heading, as a hand-authored anchor — + registers `target-section` as a jump target. + + **Rendering, as shipped: the tag is visible, not hidden.** GitHub/browsers render a + content-less anchor tag as nothing at all; this PR does not replicate that. The phase-1 + inline parser only recognizes `` as a token (it requires an `href` attribute to + match at all), so a bare ``/`` falls through to literal text and appears + inline exactly as authored (``). Resolution (this invariant's + jump-target behavior) works regardless, because it's a live text scan over that same + literal content — see tech spec item 5. Making the tag disappear from the rendered view + requires representing it as first-class block metadata that still round-trips through + `to_markdown` on save (otherwise editing the document silently deletes the anchor) — a + genuine content-model change sized at 70-130+ call sites across the buffer/editor layer, + not a rendering tweak. That work is tracked as its own ticket, + [#13982](https://github.com/warpdotdev/warp/issues/13982), deliberately deferred so + maintainers can weigh the representation trade-offs before it's built, rather than shipping + a design nobody reviewed. + +6. Explicit ``/`` anchors and implicit heading slugs share **one namespace** — + there is no separate anchor-vs-heading priority tier. If both an explicit `` + and a heading whose implicit slug is also `x` exist, `#x` resolves to **whichever occurs + first in document order**, matching GitHub's single shared id space. This is the same + "first wins" rule invariant 4's heading-collision case already uses, just extended to + cover anchors and headings together rather than headings only. See tech spec item 5 for + the resolution mechanism. + +7. A `#fragment` link that matches nothing in the document remains a normal-looking, + clickable link. Clicking it does nothing observable (no scroll, no error dialog, no + attempt to open it as an external URL). This must not panic or freeze the viewer. + +8. Non-`href`/`id`/`name` attributes on `` (`title`, `target`, `rel`, `class`, inline + `style`, …) are accepted without breaking the parse; they have no behavioral effect. + +9. `` supports the same inline content markdown links do at minimum — plain text. + Bold/italic/code *inside* the anchor text is a nice-to-have the tech spec may size + separately; if infeasible in this slice, plain-text-only anchor content is an acceptable + MVP as long as it's called out. + +10. Malformed anchor tags (unterminated ``, `href`/`id` with no + value) degrade to literal text for that tag, without swallowing the rest of the + paragraph or document and without panicking. + +11. Clicking `[text](other-file.md#section)` opens `other-file.md` in the + Markdown viewer (or focuses its tab if already open — the same open/focus behavior a + plain `other-file.md` link has today) and, once that document has loaded, scrolls it to + the `section` anchor using the same slug resolution as a same-document jump. If + `other-file.md` opens but has no matching anchor, the outcome degrades to the + same-document miss (invariant 7): the file is shown, unscrolled, no error. If the file + itself cannot be resolved (does not exist relative to the document), clicking is a + no-op, matching a broken plain relative link today. + +12. **Self-referential relative links** — a relative link whose target resolves to the + *currently-open* document (`this-doc.md`, `./this-doc.md`, with or without a `#fragment`) — + keep focus on the same tab rather than opening a duplicate, and a fragment scrolls within + it. This reuses the same open/focus dedup a cross-document link uses; the only subtlety is + path equality: an open notebook stores its *canonical* path, while the link resolves to + `base_directory.join(relative)` (with `.`/`..` components and, on macOS, the `/tmp` vs + `/private/tmp` symlink alias), so the resolved target is canonicalized before the dedup + comparison. A self-link with a fragment scrolls immediately (the tab is already laid out); + a self-link without one just refocuses, no scroll. + +13. Every link-form's behavior is now fully specified — there are no undefined interim + states. Concretely, for a scheme-less relative-looking target: + - **Bare `file.md` that exists on disk** (no `./`, no `/`) resolves as a local file, not + a web URL — even though `.md`/`.dev`/`.com` are known public suffixes. This repairs a + latent collision: the pre-existing bare-domain heuristic classified `README.md`, + `notes.md`, etc. as domains (`.md` is Moldova's ccTLD) and opened them in the browser. + - **`./file.md`** always resolves as a file (the `./` prefix never parses as a domain); + unchanged. + - **`file.md#section`** splits the `#section` fragment off before file resolution, so the + file opens and the fragment drives the cross-document scroll. A trailing `#L100` + line-number suffix is *not* an anchor and continues to route through line-number + handling. + - **Bare `nonexistent.md` with no matching local file** falls through to the browser, + preserving the genuine bare-domain behavior (`warp.dev` still opens the browser). + - **A fragment or file miss** is inert per invariant 7 — no scroll, no error. + +## Suggested phasing + +The two capabilities compound in value but are separately shippable: + +- **Phase 1:** `` inline links (parsed via the existing `Hyperlink` link-styling + machinery) **and** heading auto-anchors + fragment click resolution + scroll-to. This + alone delivers the issue's headline case — an inline HTML link or a markdown + `[text](#heading)` link jumping to a heading — and is the highest-value slice because it + fixes markdown-native fragment links too, not just the new HTML tag. +- **Phase 2 (delivered with phase 1 in this PR):** Arbitrary ``/`` anchor + targets (anchors not attached to a heading). Completes the issue's + hand-built-table-of-contents use case for authors who anchor mid-paragraph. Pulled forward + from a deferred follow-up: since the phase-1 parser already leaves a bare ``/`` tag's raw markup (including the id/name value) intact in the buffer as literal text + — it never matches the `` delimiter grammar, which requires an `href` attribute — + resolution can reuse the same zero-cache, live-text-walk pattern phase 1 established for + headings, with no new content-model field or cached index. See tech spec item 5. + **Delivered with a caveat:** the anchor tag itself renders as visible literal text (see + invariant 5) rather than being hidden the way GitHub renders it — hiding it requires a + first-class, save-round-trippable content-model representation, sized and deferred to + [#13982](https://github.com/warpdotdev/warp/issues/13982). +- **Phase 3 (delivered with phase 1 in this PR):** Cross-document fragment links + (`other-file.md#section`). Built on phase 1's slug resolver: the file-open, tab-focus, and + dedup machinery already exists (a fragment-less relative link opens today), so this phase + adds fragment carry-through plus a deferred scroll after the target document loads. It was + pulled forward into this PR because implementation revealed that "a fragment-less relative + link opens today" was only *partly* true — three latent resolution defects (ccTLD + misclassification of bare `file.md`, literal `#fragment` breaking file stats, and a + standalone viewer tab lacking a base directory) meant a bare `README.md` link could + silently open the browser or no-op. Delivering the fragment feature required repairing + those, so the whole cross-document path ships together. See tech spec item 6a and the + resolution-repairs section. diff --git a/specs/GH13725/tech.md b/specs/GH13725/tech.md new file mode 100644 index 00000000000..c6e685667d7 --- /dev/null +++ b/specs/GH13725/tech.md @@ -0,0 +1,692 @@ +# TECH.md — Markdown viewer: ``/`` anchor links + +Product spec: `specs/GH13725/product.md` +GitHub issue: https://github.com/warpdotdev/warp/issues/13725 +Sibling specs in the same split: `specs/GH13652/tables/` (raw HTML tables), and specs to +follow for `` sizing, `
`/``, `
`, ``, ``/``, +`align`, ``/``. + +## Context + +This is two mostly-independent pieces of work bolted together by the issue, and they should +be evaluated separately because their feasibility differs enormously: + +- **`
` as a hyperlink tag: small.** Warp's inline parser has no HTML-tag concept at + all — `InlineToken` (`crates/markdown_parser/src/markdown_parser.rs:1684`) covers + `Delimiter`, `Text`, `BackslashEscape`, `HtmlEntity`, `CodeSpan`, `AutoLink`, `LinkEnd`, + `UnderlineEnd`. The **only** literal HTML tag special-cased anywhere in the inline grammar + is ``/`` (`parse_inline_token_underline_start`/`_end`, :1635/:1648), which is handled + exactly like a markdown delimiter pair (push a `Delimiter`/`UnderlineEnd` token, no + attribute parsing). `` is a direct structural analog: a start delimiter + carrying one piece of data (the `href` value) and a fixed end tag. The existing markdown + link path, `parse_link` (:1125) + `parse_link_target` (:1192), already builds + exactly the styling this needs — `styles.hyperlink = Some(Hyperlink::Url(url))` + (:1158) — so an HTML `` reader can reuse `Hyperlink::Url` as its output type; it only + needs a new *front door* into that styling, not a new link model. +- **Fragment resolution + scroll-to: small, because the scaffolding already exists.** A + markdown link `[text](#fragment)` **already parses today** — `parse_link_target` has no + opinion on what a link destination looks like beyond balanced parens/brackets, so + `#fragment` is accepted as a URL string exactly like `https://…` would be (confirmed at + `markdown_parser_tests.rs:1644-1647`, "Example 501": `[link](#fragment)` → + `Hyperlink::Url("#fragment")`). And the click path **already** treats a `#`-prefixed + `Hyperlink::Url` differently from an external URL: `maybe_open_url` + (`app/src/notebooks/editor/view.rs:1971`) routes it to `scroll_to_matching_header` rather + than the URL opener. The gap is narrow and specific: the matcher compares the fragment + against the heading's exact lowercased text instead of a GitHub-style slug, so a hyphenated + fragment misses a spaced heading. Fixing that one comparison is the core of the resolution + work — no new content-model field and no new click branch are required for headings. + - `FormattedTextHeader` (`crates/markdown_parser/src/lib.rs:303`) is + `{ heading_size: usize, text: FormattedTextInline }` — no id/slug field. But this no + longer matters for phase 1: heading matching happens in the editor at click time via + `find_matching_header`, which re-reads each heading's text out of the live buffer + (`content.text_in_range(...)`) on every click — it never consulted a parse-time slug + field, so none needs to be added to `FormattedTextHeader`. + - `grep -rn "slug" crates/markdown_parser/ crates/editor/` turns up no slug-*generation* + helper — the normalization function has to be written — but the *matching loop* it plugs + into already exists (`find_matching_header`, `app/src/notebooks/editor/model.rs:1351`), + so this is a small localized change, not new plumbing. + - **A `#`-fragment click path already exists in the Markdown viewer** — this is the key + fact that reshapes the scoping below. The viewer's click entry point is + `NotebookEditorView::maybe_open_url` + (`app/src/notebooks/editor/view.rs:1955`), and it **already branches on a leading `#`**: + at :1971-1983, `if url.starts_with('#')` it calls + `model.scroll_to_matching_header(&url, ctx)` and returns early on a hit, only falling + through to ordinary URL-open handling on a miss. So the fragment-scroll wiring the earlier + draft of this spec treated as net-new does not need to be built — it is live on master. + (The general `FormattedTextElement::register_default_click_handlers` helper at + `crates/warpui_core/src/elements/gui/formatted_text_element.rs:336` is used by many other + surfaces — settings pages, banners, modals, the changelog, AI views — but **not** by the + notebook Markdown viewer, and none of those callers has a `#`-fragment branch. It is not + the fix site; do not target it.) + - **The gap is the matching rule, not the scroll or the click branch.** + `scroll_to_matching_header` (`app/src/notebooks/editor/model.rs:1335`) delegates to + `find_matching_header` (:1351), which walks `content.outline_blocks()`, filters to + `BlockType::Text(BufferBlockStyle::Header { .. })`, and compares the fragment against + `heading.trim().to_lowercase()` (:1374) — i.e. the **exact lowercased heading text**, not + a GitHub-style slug. So `#target-section` (hyphenated) misses a heading titled + "Target Section" (spaced), which is precisely the issue's failing case. It does already + strip the `#` prefix and `urlencoding::decode` the fragment (:1352-1356), so URL-escaped + fragments are handled; only the text-vs-slug comparison is wrong. + - **The scroll primitive is already wired end-to-end.** On a match, + `scroll_to_matching_header` calls + `render_state.request_autoscroll_to(AutoScrollMode::PositionOffsetInViewportCenter(range.start))` + (`app/src/notebooks/editor/model.rs:1346`), backed by + `EditorRenderState::request_autoscroll_to` (`crates/editor/src/render/model/mod.rs:3054`, + with `PositionOffsetInViewportCenter` handled at :3661/:3712). Nothing new is needed on the + scroll side — a resolved heading range already scrolls today. + +This framing matters for scoping: the visible, "does it look done" work (parsing `` +text into a blue underlined link) is one piece. The part that makes the issue's headline test +case pass — `[Jump to Target Section](#target-section)` scrolling to the heading below it — is +**far smaller than the earlier draft claimed**: the click branch, the scroll call, and the +per-click heading iteration all already exist in `find_matching_header`. What's missing is slug +normalization inside that one function so a hyphenated fragment matches a spaced heading. + +## Feasibility summary + +- **(i) `` inline parsing → `Hyperlink::Url`: SMALL.** New `InlineToken` variant(s) + (or extend delimiter handling, mirroring ``) plus an attribute-extraction step for + `href`. Reuses 100% of the existing link styling/click/render path once the token exists. +- **(ii) ``/`` anchor targets: SMALL — delivered in this PR.** The zero-width + marker concept described in the original draft below turned out to be unnecessary. + Characterization of the phase-1 parser (`markdown_parser::parse_inline_token_html_anchor_start`, + `crates/markdown_parser/src/markdown_parser.rs:1750`) confirmed it only recognizes `` + as a delimiter pair — it *requires* an `href` attribute to match at all (:1776-1779: no `href` + → `Err`), so a bare ``/`` with no `href` never becomes that token. It + falls through the inline `alt` chain to plain `text`, and the tag's raw markup — including the + id/name value — survives **verbatim, as visible literal text**, in the buffer. This is already + a committed, passing assertion: `markdown_parser_tests::test_parse_html_anchor_unterminated_falls_back_to_text` + asserts `` parses to `FormattedTextFragment::plain_text("")`. + Because the id survives verbatim in the buffer, resolution doesn't need a new content-model + field or a parse-time anchor concept at all — it reuses the exact same live-text-walk pattern + `find_matching_header` already established for headings (item iii), just scanning for anchor + tags in the whole-buffer text instead of outline-block heading text. See item 5 for the + mechanism. (The original draft below, describing a `styles.anchor_id` fragment marker, is kept + for context but was **not** implemented — it solved a problem that characterization showed + doesn't exist for the empty/self-closing anchor form this feature scopes.) + + **Known gap, shipped as-is, deferred separately:** because the tag survives as literal text, + it also *renders* as literal text — `` is visible inline, unlike GitHub's + content-less-anchor-renders-nothing behavior. Investigating a hiding mechanism (a design + requiring the tag to become first-class block metadata that still re-serializes through + `to_markdown` on save, or it's silently deleted on the next edit) surfaced a genuine + content-model migration — every `BufferBlockStyle` variant and/or every `BufferText::BlockMarker` + call site (roughly 70-130 sites across `core.rs`, `edit.rs`, `buffer.rs`, `markdown.rs`, + `render/`, and hand-built test fixtures) would need touching, with no clearly-best + representation among the candidates. That's out of scope for this PR and tracked as + [#13982](https://github.com/warpdotdev/warp/issues/13982) — a design-discussion ticket, not a + quick follow-up patch, deliberately left unbuilt until maintainers weigh in on the + representation. +- **(iii) Heading slug matching + fragment click resolution: SMALL.** The click branch, the + scroll call, and the per-click heading walk all already exist in `find_matching_header` + (`app/src/notebooks/editor/model.rs:1351`). The only work is: + - Slug normalization (GitHub-style: lowercase, spaces→hyphens, strip punctuation). This is + a pure function with no existing code to build on, straightforward to unit test in + isolation. It gets applied inside `find_matching_header`: normalize the incoming fragment + and normalize each heading's text with the **same** function, then compare — replacing the + current `heading.trim().to_lowercase() == target` check (:1374). Both sides run through + one normalizer, so a hyphenated fragment matches a spaced heading. + - **No new anchor index.** `find_matching_header` already iterates `content.outline_blocks()` + and reads each heading's text live from the buffer on every click, so there is no + id→offset map to build, no place for it to live, and no cache-invalidation problem — the + function recomputes against the current buffer each time it runs. (Dedupe of collision + slugs, if wanted, is likewise "first match wins" as a natural consequence of the loop + returning on the first hit — no separate `-1`/`-2` bookkeeping is required for phase 1; + the product spec's non-goal already accepts first-wins.) + - Miss behavior (product invariant 7) is already correct: `find_matching_header` returns + `None` → `scroll_to_matching_header` returns `false` → `maybe_open_url` falls through. + The one nuance to preserve: on a miss today, `maybe_open_url` still hands `#fragment` to + the URL opener (view.rs:1984+), which for an in-document fragment is a no-op in practice + but should be confirmed not to surface a broken-link tooltip; if it does, the miss path + should early-return instead of falling through. Flag this for the implementer to verify + against invariant 7's "no error dialog" clause. + +- **(iv) Cross-document fragment navigation (`other-file.md#section`): MEDIUM — delivered in + this PR.** The file-open, tab-focus, and dedup were *supposed* to be free (a fragment-less + relative link opens the target in the Markdown viewer today, `resolve_and_open` → + `OpenFileNotebook` → `open_file_notebook`, `app/src/workspace/view.rs:8470`) — but that + baseline turned out to be broken in three ways that had to be repaired first (item 6b). The + feature work itself is bounded: split the `#section` off before file resolution, thread it to + the destination pane (mirroring the code editor's existing `line_and_column` plumbing), and + drain it as a **deferred scroll** once the new document parses — the one genuinely new + element, since there is no on-load hook to reuse. Full mechanism in **item 6a**; resolution + repairs in **item 6b**. + +This spec recommends implementing **(i) + (iii)** as phase 1 (matches the product spec's +phasing — this is the slice that fixes the issue's headline test case and also repairs +markdown-native `[text](#heading)` links, which get zero benefit from (ii) alone), +and **(iv)** delivered together with phase 1 in the same PR (see the amendment note below). +**(ii)** — arbitrary ``/`` markers — is deferred to a follow-up. + +> **Amendment (cross-document delivered with phase 1).** Item (iv) was originally sequenced +> as "phase 3, later." Implementation moved it into this PR because verifying the +> fragment-less baseline it was supposed to build on ("a bare relative link already opens +> the target today") turned out to be false in three ways — a bare `README.md` link could +> silently open the browser, and even a `./file.md#frag` link no-op'd. Those three +> resolution defects (documented in the new **Resolution repairs** section below) had to be +> fixed for the cross-document feature to work at all, so the feature and its repairs ship +> together. `` markers (item ii) remain the follow-up. + +## Proposed changes + +### 1. `text` inline token (phase 1) + +Add HTML-anchor delimiter tokens to `InlineToken` +(`crates/markdown_parser/src/markdown_parser.rs:1684`), following the `` precedent +exactly in shape but carrying data: + +- A start token that captures the `href` attribute value — unlike ``'s zero-data + `Delimiter { kind: UnderlineStart, count: 1 }`, this needs the URL string threaded through + to close-time, so it likely needs its own variant rather than reusing `Delimiter` verbatim + (e.g. `InlineToken::HtmlAnchorStart(String)` for the href, paired with a `HtmlAnchorEnd` + token on ``), or the URL could live in a small ad hoc attribute parser called at + `` applies `styles.hyperlink = Some(Hyperlink::Url(href))` to the fragments + between start and end — the exact same `backtrack_styles` call `parse_link` makes at + :1157-1158, just triggered by `` instead of `]`+`(url)`. +- A minimal attribute parser for the opening tag: extract `href="…"` (single/double-quoted), + tolerate and discard any other attributes (`title`, `target`, `rel`, `class`, …) per + product invariant 8 — this can be a small nom combinator scanning `key="value"` pairs + without needing a general HTML tokenizer (the existing paste-path parser in + `html_parser.rs` already depends on `html5ever`, but pulling that into the inline-token + grammar for a single-tag case is likely overkill; a purpose-built attribute scanner is + more consistent with how `` is handled today — call this choice out for maintainer + review, since `html5ever` is an available and more robust alternative if the ad hoc parser + proves fragile against real-world `` markup). +- Malformed input (unterminated ``) falls back to + literal text for the tag, matching how `parse_link` falls back to a literal `]` on failure + (:1170-1177) — product invariant 10. + +### 2. Heading slug normalization (phase 1) + +No parse-time change and **no field on `FormattedTextHeader`**. Heading matching happens in +the editor at click time via `find_matching_header` +(`app/src/notebooks/editor/model.rs:1351`), which already reads each heading's text out of +the live buffer (`content.text_in_range(...)`, :1371-1373) on every click. The fix is a single +slug normalizer applied on both sides of the comparison inside that function. + +**Slug algorithm — genuinely GitHub-compatible, not ASCII-only.** GitHub's heading slugger +(`gfm-auto-identifiers`, the same rule set GitHub Pages/`github/cmark-gfm` implement) does +**not** strip non-ASCII text: it lowercases (Unicode-aware), removes characters that are +punctuation/symbols per the Unicode categories it excludes, converts spaces to `-`, and +otherwise **preserves any Unicode alphanumeric/word character**, including accented Latin, +CJK, Cyrillic, etc. A heading `"Café Société"` slugs to `café-société`, not `caf-socit`; +`"日本語"` slugs to `日本語`. An ASCII-only `[a-z0-9 -]` filter silently breaks every non-English +README anchor, which is a real regression against the product goal, not a cosmetic gap. + +Concretely, the normalizer should: + +1. Lowercase the input (Unicode-aware lowercasing, e.g. Rust's `str::to_lowercase`, not an + ASCII-only lowercase — this already matters for e.g. Turkish/German/accented casing). +2. Strip characters that are ASCII punctuation/symbols outside `-` and whitespace (mirroring + GFM's exclusion set: roughly `!"#$%&'()*+,./:;<=>?@[\]^`{|}~`), while leaving all other + Unicode letters/digits/marks (CJK, accented Latin, etc.) untouched. This is the one place + the two algorithms diverge from a naive "strip non-ASCII" reading — the exclusion set is + defined by *punctuation class*, not by *ASCII range*. +3. Replace runs of whitespace with a single `-`, trim leading/trailing `-`. +4. Do **not** apply `user-content-` or any other prefix (see the Security section's note on + why Warp doesn't need GitHub's DOM-collision prefix). + +Add it as a small pure function (natural home: alongside `find_matching_header` in +`model.rs`, or a shared helper if a second caller appears). It should be unit-testable in +isolation on `&str → String`, with cases spanning plain ASCII, accented Latin, and CJK +headings (see the Testing section's updated case list) — the Unicode-preserving behavior is +exactly the kind of thing that regresses silently if only ASCII cases are tested. + +`find_matching_header` already normalizes the incoming fragment as `target = fragment +.strip_prefix('#')` → `urlencoding::decode` → `trim().to_lowercase()` (:1352-1357). Replace +that trailing `to_lowercase()` with the slug normalizer, and change the per-heading comparison +at :1374 from `heading.trim().to_lowercase() == target` to `slug(&heading) == target`. Both +sides then run through the same normalizer, so `#target-section` matches a heading "Target +Section". No document-wide pass and no prior-heading state are needed — first-match-wins for +collisions falls out of the loop returning on the first hit, which satisfies the product +non-goal's "first wins" dedupe stance. + +### 3. No anchor index (phase 1) + +There is deliberately no anchor index in phase 1. The earlier draft of this spec proposed a +document-scoped `HashMap` rebuilt per edit, and treated its storage and +invalidation lifecycle as the biggest unknown — but that entire structure is unnecessary +because `find_matching_header` already performs the id→offset lookup live: it iterates +`content.outline_blocks()`, filters to headers, and returns the matching header's +`start..end` range directly from the current buffer. Iterating a handful of outline blocks on +a click is cheap, and re-reading the buffer each time means there is nothing to keep in sync +with edits. **No new per-document cached state, no invalidation hook, no lifecycle question.** + +Phase 2 (``/`` markers) needs the parsed anchor positions to be reachable at +click time. It resolves this the same way phase 1 does — a live walk at click time, no cached +index — detailed concretely in item 5 below, including the precedence rule for an explicit +anchor and a heading slug that collide. + +### 4. Fragment-aware click resolution — already exists (phase 1) + +The click branch does **not** need to be built: `maybe_open_url` +(`app/src/notebooks/editor/view.rs:1955`) already does, on a leading `#` (:1971-1983), call +`scroll_to_matching_header`, which on a hit calls +`request_autoscroll_to(AutoScrollMode::PositionOffsetInViewportCenter(range.start))` +(`app/src/notebooks/editor/model.rs:1346`). The `#`-branch, the resolution call, and the +scroll are all live on master. Do **not** target +`FormattedTextElement::register_default_click_handlers` — that helper is used by unrelated +surfaces and is not on the Markdown viewer's click path. + +The only phase-1 behavior change here is downstream of the slug fix in item 2: + +- **Hit:** already correct — `find_matching_header` returns a range, + `scroll_to_matching_header` requests the autoscroll, `maybe_open_url` early-returns + (view.rs:1978-1982). Once item 2's slug normalization lands, headings that previously missed + now resolve. +- **Miss:** `find_matching_header` returns `None` → `scroll_to_matching_header` returns + `false` → `maybe_open_url` falls through to the ordinary URL path. Per product invariant 7 + this must be observably inert (no scroll, no error, no crash). Confirm the fall-through does + not raise a broken-link tooltip for an in-document `#fragment`; if it does, add an + early-return on the `#`-prefixed miss instead of falling through. This is the one spot the + implementer must check by hand against invariant 7. + +### Render surfaces (GUI vs. TUI) + +Master now has a second Markdown render surface — the TUI renderer at +`crates/warp_tui/src/tui_markdown.rs` — and it shares the same `markdown_parser` crate and the +same `FormattedText`/`Hyperlink` model as the GUI. This splits the feature cleanly: + +- **`` *parsing* (item 1) benefits both surfaces for free.** Because the new + `Hyperlink::Url` fragments are produced in the shared parser, the TUI renderer picks them up + with no TUI-specific code: `inline_spans` already reads + `fragment.styles.hyperlink → Some(Hyperlink::Url(url))` and renders the link text styled but + inert (`crates/warp_tui/src/tui_markdown.rs:194-196`). So an `` link will *display* + correctly in the TUI the moment the parser change lands. +- **Fragment *scroll* resolution (items 2–4) is GUI-only.** The whole resolution path lives in + `app/src/notebooks/editor/` (`maybe_open_url`, `scroll_to_matching_header`, + `find_matching_header`) — the TUI has no `maybe_open_url` equivalent and no scroll model to + drive, so there is nothing to hook. `#fragment` clicks resolving to a scroll are explicitly + **out of scope for the TUI surface** in this slice; the TUI simply renders the link as inert + styled text. Call this out so a reviewer doesn't read "shared parser" as "shared behavior." + +### 5. ``/`` anchor markers (phase 2) + +> **Superseded by what actually shipped.** This section is the pre-implementation design +> draft and is kept for historical context; it was **not** built. What shipped instead is a +> live text scan over the buffer (see item (ii) in the Feasibility summary above and item 3's +> "no anchor index" precedent) — no new fragment field, no zero-width construct. The one +> design point from this draft that *is* still an open, deferred problem is rendering: this +> draft's zero-width marker would have rendered nothing, matching GitHub; the shipped +> live-text-scan approach leaves the tag visible, because hiding it would require the very +> content-model investment ("no anchor index" i.e. no persisted representation) this section +> otherwise avoided. That trade-off — and the honest sizing of what a hiding fix would cost — +> is tracked as [#13982](https://github.com/warpdotdev/warp/issues/13982). + +Represent a bare anchor as a new zero-width construct — the cleanest option is a +`FormattedTextFragment` with empty `text` and a new style/marker field (e.g. +`styles.anchor_id: Option`), since fragments are already the unit `hyperlinks()` +and similar traversal helpers walk over; a marker-as-empty-fragment reuses that traversal +for free. An alternative is a dedicated `FormattedTextLine` variant, but that's a heavier +change (every line-level consumer would need a new match arm) for something that's +conceptually inline, not block-level — recommend the fragment-marker approach unless review +surfaces a reason `FormattedTextLine` fits better. + +`visible text` (both an id and content in one tag) is out of scope per product +non-goal — the phase-2 reader only needs to handle the empty/self-closing form +(`` or ``), simplifying the parser considerably (no need to also +apply link/text styling in this path). + +**Lookup strategy at click time: extend the same live walk, no cache.** `find_matching_header` +already re-walks `content.outline_blocks()` on every click rather than consulting a cached +index (item 3) — phase 2 extends that same function rather than introducing a second, +differently-shaped lookup: + +- `outline_blocks()` returns block-level markers (`BlockType::Text(BufferBlockStyle::Header { + .. })`, `CodeBlock`, `PlainText`, list/table markers, etc.) — it does not itself walk inline + fragments within a block, so it cannot see an `anchor_id` marker sitting mid-paragraph. + `find_matching_header` therefore needs a second pass alongside the header scan: for each + block returned by `outline_blocks()`, additionally read that block's fragments (via the same + `content` accessor the header path already uses to pull heading text, + `content.text_in_range`/the fragment-level equivalent) and check each fragment's + `styles.anchor_id` for a match, before or interleaved with the existing header-text + comparison. Concretely: rename/extend `find_matching_header` (or add a sibling + `find_matching_anchor` called first) so the combined resolver checks explicit anchors and + heading slugs in one pass over the document, still with no persisted state — the walk itself + is the "index," recomputed per click exactly as phase 1 established. +- This keeps phase 2 architecturally consistent with phase 1's core decision (item 3): no + `HashMap`, no invalidation hook, no lifecycle question. The only new + per-click cost is reading fragment styles in addition to block outlines, which is the same + order of cost as the existing header-text read. + +**Precedence when an explicit `` and a heading slug collide on the same fragment.** +Define one deterministic rule: **explicit anchors win.** Concretely, the combined resolver +checks explicit `anchor_id` fragments for an exact match against the (undecoded, as-authored) +id first; only if no explicit anchor matches does it fall back to the slug-normalized heading +comparison from item 2. Rationale: an `` is an author's literal, intentional target +string — it should never be shadowed by an incidental heading-slug collision the author didn't +control (e.g. two different headings elsewhere in the doc normalizing to the same slug, see +item 2's "first-wins" dedupe among headings). This rule composes cleanly with item 2's +first-occurrence-wins rule, which continues to apply only *among* headings when no explicit +anchor is present — i.e. the full ordering is: (1) exact match against an explicit ``/`` value, first occurrence wins if multiple share an id; (2) else, slug-normalized +match against a heading, first occurrence wins among headings. Both levels reuse the same +single-pass walk, so there is no separate index or second traversal for the fallback tier. + +### 6. Feature gating + +Recommend a **new** feature flag (e.g. `FeatureFlag::MarkdownAnchorLinks`) rather than +riding an existing one — unlike the tables spec, there's no existing "structural HTML" +flag this naturally extends, and gating separately lets phase 1 (headings + ``) ship +independently of phase 2 (``) if their cost estimates diverge during implementation. + +### 6b. Resolution repairs (delivered with cross-document navigation) + +Cross-document navigation (item 6a) assumed a fragment-less relative link already opens its +target today. Implementation found that assumption false in three independent ways, each +fixed in `app/src/notebooks/link.rs`. All three are in the same code path — `NotebookLinks::resolve` +— and all three block even the plain `[text](other-file.md)` case, so they are prerequisites, +not polish. + +**Repair 1 — ccTLD misclassification of bare `file.md`.** `resolve` (link.rs:150-169) applies +a bare-domain heuristic *before* file resolution: it takes the substring up to the first `/` +and, if `addr::parse_domain_name` reports a known public suffix with a root, treats the whole +target as `http://…`. Because `.md` is Moldova's ccTLD (and `.dev`, `.com`, … are TLDs), a bare +`README.md`/`notes.md`/`other-file.md` — no `./`, no `/` — is classified as a domain and opened +in the browser instead of the viewer. **Verified empirically** with `addr` 0.15.6: +`README.md`, `file.md`, `other-file.md`, `notes.md`, `warp.dev`, `google.com` all return +`true` from the heuristic; `./README.md`, `app/src/main.rs`, `index.html` return `false` (the +`./` prefix and multi-segment paths dodge it, and `.html` is not a known suffix — which is why +the existing `test_open_markdown_file_uses_viewer_when_preferred` had to spell its link +`./README.md`). *Fix:* before applying the heuristic, synchronously check whether the +scheme-less target resolves to an existing file relative to the base directory (reusing +`absolute_path_if_valid`, which already does a sync `fs::metadata` existence check). If it +does, resolve as a file; only fall through to the domain heuristic when there is no matching +local file — so `warp.dev` (no local file) still opens the browser, and a bare `nonexistent.md` +with nothing on disk also still does. Deterministic and file-existence-gated. + +**Repair 2 — literal `#fragment` breaks the file stat.** `CleanPathResult` strips `:line:col` +and `#L100` suffixes (`crates/warp_util/src/path.rs:47-57`) but **not** a bare `#section` +fragment, so `other-file.md#section` reaches `resolve_file` as a literal on-disk path, misses, +and `resolve_and_open`'s closure silently drops the error (link.rs:328-332) — today's no-op. +*Fix:* a `split_anchor_fragment` helper peels the trailing `#…` off before file resolution, +returning `(path, Option)`. It splits only the final `#` (so `weird#name.md#frag` +keeps `weird#name.md`), decodes the fragment with `urlencoding`, treats an empty fragment as +no anchor, and — critically — leaves a `#L[:]` suffix attached to the path so +the existing line-number routing in `CleanPathResult` is not regressed. The split runs *after* +the explicit-URL branch (so a real `https://…#frag` keeps its fragment) and feeds repair 1's +file check. + +**Repair 3 — standalone viewer tab lacks a base directory.** A cross-document link clicked +from *within* an open notebook already gets the right base dir: `FileNotebookView::open_local` +→ `set_context` sets `SessionSource::Target { base_directory: }` +(`app/src/notebooks/file/mod.rs`), so this repair is not needed for the primary in-app click +path. The gap is a standalone viewer tab opened with no session (`open -a Warp file.md`): with +no session, `SessionSource::Active` fell back to the window's active-session cwd, which is +absent, so even `./file.md` resolved to `MissingContext`. *Fix:* `SessionSource::Active` now +carries an optional `document_dir`; `base_directory()` prefers the active session's local cwd +and falls back to the document's own parent directory. `open_local`'s no-session branch seeds +that fallback from the file's parent. The document knows where it lives even when the window +doesn't. (A truly session-less tab still cannot resolve files that require a `Session` object +for other reasons; surfacing resolution failures non-silently — `resolve_and_open` swallows +errors at link.rs:328-332 — is noted as a **follow-up**, since no cheap existing tooltip +affordance is on this path and the spec forbids building new UI here.) + +### 6a. Cross-document fragment navigation (delivered in this PR) + +**Size: MEDIUM.** The file-open half is free — a fragment-less relative link +(`other-file.md`) already opens the target in the Markdown viewer today, including +tab-focus and dedup. What's missing is carrying the `#section` through that flow and +scrolling after the *new* document parses. This is bounded, well-scoped plumbing, not new +targeting machinery — but it is more than a single parameter through one existing call, and +it needs a new piece of deferred state on the destination editor because the scroll target +can't be applied until the document is loaded and there is no on-load hook today. + +**What already works (verified on master).** Clicking a relative link with no fragment +routes through `maybe_open_url` (`app/src/notebooks/editor/view.rs:1955`); because it does +not start with `#`, it goes to `NotebookLinks::resolve_and_open` +(`app/src/notebooks/link.rs:323`). `resolve` (:128) resolves the relative path against the +session's base directory (:211-222), `resolve_file` (:235) confirms it exists on disk, and +`open` (:258) emits `LinkEvent::OpenFileNotebook` for a Markdown target (:282/:290). That +event is consumed at `app/src/pane_group/pane/notebook_pane.rs:175`, re-emitted as +`Event::OpenFileInWarp`, and handled by `Workspace::open_file_notebook` +(`app/src/workspace/view.rs:8470`) — which **already de-dupes an open pane and focuses it** +(:8489-8494) or opens a new tab/split (:8503-8520). So open, focus, and dedup for the target +document are live; only the fragment is lost. + +> **Dedup needs canonicalization for self-reference (and symlink aliases).** The dedup +> compares `file_view.path()` against the resolved link path, but an open notebook stores its +> *canonical* path (recorded on load via `CanonicalizedPath`/`dunce::canonicalize`), while the +> link resolves to `base_directory.join(relative)` — which keeps `.`/`..` components and, on +> macOS, the `/tmp` vs `/private/tmp` symlink alias. Without normalization a self-referential +> link (`./this-doc.md`, the same file that's already open) fails to match its own pane and +> opens a duplicate. Fix: canonicalize the resolved local target with the *same* +> `dunce::canonicalize` before the dedup comparison (extracted as +> `canonicalize_local_path_for_dedup`, unit-tested against the `.`/`..`/symlink-alias shapes). +> A self-link with a fragment then hits the already-open branch and scrolls immediately; a +> self-link without one just refocuses. This is invariant 12. + +**Why the fragment is lost today.** The path is cleaned by +`CleanPathResult::with_line_and_column_number` (`crates/warp_util/src/path.rs:158`), whose +`LINE_AND_COLUMN_REGEX` (:47) strips `:line[:col]`, `[l, c]`, and `#L100`-style suffixes +(:48-56) but **not** a bare `#section` fragment. So `other-file.md#section` survives cleaning +intact, is resolved as a literal on-disk path, misses (`ResolveError::FileNotFound`), and +`resolve_and_open`'s closure silently drops the error (:328-332) — the current no-op. + +**The precedent for "open a file AND position the viewport" already exists — for the code +editor.** `LinkTarget::LocalFile` carries `line_and_column` (link.rs:35) end-to-end, and +`add_tab_for_code_file` (`app/src/workspace/view.rs:12828`) threads it through to position the +code editor. The Markdown-notebook path is the gap: `add_tab_for_file_notebook` (:12767) and +`open_file_notebook` (:8470) carry only a `path`, no viewport target, and `OpenFileNotebook` +(link.rs:441) has no fragment field. + +**Concrete mechanism (three localized changes):** + +1. **Split the fragment before file resolution.** In `NotebookLinks::resolve` + (link.rs:128), for a link that is neither a parseable URL nor a bare `#fragment`, peel a + trailing `#…` off the string before it reaches `CleanPathResult`, keeping it as an + `Option anchor` alongside the cleaned path. (A bare `#fragment` is still handled + earlier by `maybe_open_url`'s `starts_with('#')` branch — this split only affects strings + that have a path *and* a fragment.) Once the fragment is removed, `resolve_file` finds the + real file and the existing open/focus flow runs unchanged. + +2. **Thread the fragment to the destination pane**, mirroring how `line_and_column` already + rides the code path. Add `anchor: Option` to `LinkTarget::LocalFile` (link.rs:33), + to `LinkEvent::OpenFileNotebook` (link.rs:441), to `pane_group::Event::OpenFileInWarp` + (`app/src/pane_group/mod.rs:549`), and through `open_file_notebook` (view.rs:8470) into the + `FilePane`/notebook it constructs. This is additive field-plumbing along an existing event + chain — the analog of the code editor's `line_and_column`, which already proves the shape + works end-to-end. + +3. **Apply the scroll after the destination document loads — the one genuinely new piece.** + The same-document jump can call `scroll_to_matching_header` immediately because the buffer + is already parsed. A freshly opened notebook is not: the offset the slug resolves to does + not exist until parse completes, and there is **no on-load callback in the notebook-open + flow to hang the scroll on** (`open_file_notebook` constructs the pane and returns; nothing + fires when its content finishes parsing). The destination editor model therefore needs a + small piece of **deferred-scroll state** — a `pending_anchor: Option` set at + construction — that is consumed once, on the first successful parse/layout, by calling the + existing `scroll_to_matching_header` (`app/src/notebooks/editor/model.rs:1335`) with the + pending fragment and then clearing it. This reuses phase 1's resolver verbatim; the only + new logic is *when* to call it. The implementer must locate the model's + content-ready/relayout point (the notebook already rebuilds layout on content load — e.g. + the `rebuild_layout` path around `model.rs:1315`) and drain `pending_anchor` there. If no + anchor matches after load, draining is a no-op — identical to the same-document miss + (product invariant 7). + +**Resolution reuse.** The slug comparison is entirely unchanged: the destination scroll runs +through the same `find_matching_header` (`app/src/notebooks/editor/model.rs:1351`) the +same-document jump uses, so phase 1's slug normalizer (item 2) is the cross-document resolver +too — no second matcher, no divergence. This is the concrete sense in which phase 1 "leaves +room" for cross-document navigation: the target document's resolver is the *same function*, +invoked after open instead of in place. + +**Non-Markdown / external-editor targets.** If the Markdown Viewer preference is off, `open` +routes the file to `open_file` → the code editor or system handler (link.rs:284), which has +no slug concept. Per the product non-goal, the fragment is simply dropped in that case: the +file opens, unscrolled. Only the `OpenFileNotebook` branch carries the anchor. + +### 7. Security + +`` reuses `Hyperlink::Url` verbatim — no new trust boundary, no script/event-handler +attributes are read (only `href`; all others parsed-but-discarded per product invariant 8). +Fragment resolution never leaves the document (no network, no file access) — a `#fragment` +click either scrolls within the current buffer or is a no-op. ``/`` values +(phase 2) are used only as in-document lookup keys, never interpolated into a URL, path, or +shell context. + +**On GitHub's `user-content-` prefix (intentionally not replicated).** GitHub's renderer +prefixes the *rendered DOM ids* it emits for headings and `` anchors with +`user-content-` (to avoid collisions with GitHub's own page-chrome ids), while leaving the +`href="#…"` fragments authors write unprefixed; a small piece of client JS bridges the two at +click time. Warp has no such collision surface and no DOM: resolution happens **in-process**, +comparing the fragment against slugs computed live from heading text (item 2), not against any +emitted id attribute. So Warp deliberately does **not** add a `user-content-` prefix on either +side — there is nothing for it to disambiguate against, and adding it would only break parity +with the plain `#slug` fragments authors actually write. Worth stating explicitly so a +reviewer familiar with GitHub's scheme doesn't flag its absence as a bug. + +## Testing and validation + +**Validation is committed, tracked test cases — not the probe file.** All cases below live in +tracked test modules (`markdown_parser_tests.rs`, editor-model tests) that CI runs today and +will run against the implementation PR; nothing in this spec's validation depends on a file +that isn't checked in. + +**Historical context on the probe file.** The master checkout currently carries an *untracked* +scratch file, `crates/markdown_parser/src/html_tag_support_tests.rs` (wired via `#[path] mod +html_tag_support_tests;` at `markdown_parser.rs:1998-1999`, itself marked "DO NOT COMMIT" in +its header comment), whose two anchor probes — +`test_raw_html_anchor_href_not_parsed_as_hyperlink` and +`test_raw_html_anchor_id_not_registered_as_target` — assert today's **no-op** status quo (raw +`` produces no hyperlink style; raw `` registers no target). These were exploratory +probes used to confirm current behavior while scoping this spec, not a test suite the +implementation is meant to build on. The implementation PR must **not** leave these assertions +in place or rely on the untracked file continuing to exist — it must add the committed positive +assertions listed below to `markdown_parser_tests.rs` directly (`` *does* produce a +`Hyperlink::Url`; `` *does* register an anchor), which subsumes and inverts what the probe +file was checking. If the probe file is still present at implementation time, delete it or fold +any remaining useful cases into the committed suite — it should not ship as project state. + +### Parser unit tests (`crates/markdown_parser/src/markdown_parser_tests.rs`) + +- `Visit Warp` → `Hyperlink::Url` fragment identical in + shape to the equivalent markdown link (invariant 1). +- `Jump` → `Hyperlink::Url("#target")` fragment (invariant 2) — + parsing only; resolution is tested separately below. +- Attributes beyond `href` (`title`, `target="_blank"`, `class="x"`) parsed-but-ignored, no + effect on output (invariant 8). +- Unterminated `` / missing closing `` → literal text fallback, rest of + paragraph intact (invariant 10). +- Slug normalizer (item 2), tested as a pure `&str → String` function, table-driven over: + - Plain ASCII text, text with punctuation/mixed case, multi-space runs → normalized slug + (invariant 4). + - **Unicode headings, to lock in genuine GitHub-compatibility (not ASCII-only):** + accented Latin (`"Café Société"` → `café-société`), CJK (`"日本語"` → `日本語`, unchanged — + no word characters to strip and no ASCII case to fold), and a mixed-script heading + (e.g. `"Section 日本語 Café"` → `section-日本語-café`). These guard against silently + reintroducing an ASCII-only filter. + - First-wins collision behavior is covered by the resolution tests below, not the + normalizer itself. +- (Delivered, revised from the original phase-2 draft) `` / `` + → parses as a single literal-text fragment (`FormattedTextFragment::plain_text`), **not** a + zero-width marker — characterization confirmed the phase-1 `` grammar requires + `href` to match, so an id-only tag falls through to plain text and stays visible (invariant + 5's rendering caveat, deferred to #13982). *Landed* as + `test_parse_html_anchor_unterminated_falls_back_to_text`'s trailing case in + `markdown_parser_tests.rs`. +- (Phase 2) `text` (both id and content on one tag) — confirm documented + behavior (out of scope; assert it doesn't panic, even if unspecified which role wins). + +### Resolution tests (`app/src/notebooks/editor/` — `find_matching_header` / `model.rs`) + +These target the matcher directly, since that is where the phase-1 change lives. + +- Heading `## Target Section` + fragment `#target-section` → `find_matching_header` returns + the heading's range; today (exact-text match) it returns `None`. This is the regression the + slug normalizer fixes (invariants 2, 3, 4). +- Heading with a non-English title (e.g. `## Café Société`) + fragment `#café-société` → + resolves, exercising the Unicode-preserving normalizer end-to-end through the matcher, not + just the pure function in isolation. +- `` and markdown `[text](#slug)` targeting the same heading resolve to the + same range (invariant 3 — the two syntaxes are equivalent because both reach + `find_matching_header` via the same `#`-branch in `maybe_open_url`). +- Fragment with no matching heading → `find_matching_header` returns `None`, + `scroll_to_matching_header` returns `false`, no panic (invariant 7). +- First-wins collision: two headings normalizing to the same slug → the fragment resolves to + the first, exercised by asserting the returned range is the earlier heading's. +- (Phase 2) Explicit `` sharing a fragment/document with a heading whose implicit + slug is also `x` → the combined resolver's explicit-anchor-first pass (item 5) returns the + anchor's position, not the heading's, per the defined precedence rule (invariant 6). +- (Phase 2) Explicit anchor lookup when no heading collides: `` alone resolves + via the fragment-styles pass added to the combined resolver (item 5), independent of the + header-outline pass. +- (Phase 2) Two `` markers with the same id → first occurrence wins, mirroring the + heading first-wins rule. +- (Cross-doc, delivered) Fragment-split in `resolve`: `other-file.md#section` yields cleaned + path `other-file.md` + anchor `section`; `other-file.md` (no fragment) yields no anchor; + `other-file.md#L10` still routes to the existing line-number path, not the anchor path + (guard against regressing the `#L`-suffix handling in `CleanPathResult`). *Landed* as + `test_split_fragment_before_file_resolution`, `test_fragment_split_preserves_line_number_routing`, + and the pure-function `test_split_anchor_fragment_pure` (covers multiple `#`, empty + fragment, URL-decode, `#L` vs `#License`/`#L10x`). +- (Repair 1, delivered) ccTLD classification matrix: a bare `README.md`/`notes.md` that + exists on disk resolves as a file; `warp.dev` and a bare `nonexistent.md` with no local + file still resolve as URLs. *Landed* as `test_bare_markdown_file_prefers_local_file_over_cctld`; + the existing viewer test was also switched from `./README.md` to bare `README.md` to assert + the repair end-to-end. +- (Cross-doc, delivered) A resolved `LinkTarget::LocalFile` for a `#section` link carries the + anchor through to `OpenFileNotebook` (assert on the emitted event). *Landed* as + `test_cross_document_fragment_threads_anchor_to_open_event`. +- (Cross-doc, delivered) Deferred-scroll drain: a `pending_anchor` set before layout resolves + through the same slug matcher on drain (hit → scroll, cleared one-shot; miss → silent + no-op). *Landed* as `test_pending_anchor_drains_and_scrolls_on_match` and + `test_pending_anchor_miss_is_silent_no_op` in the editor-model tests. + +### Integration / manual + +Per CONTRIBUTING, before/after screenshots plus a short recording reproducing the issue's +motivating test document verbatim: the raw-HTML `` jump, the +markdown-native `[Jump to Target Section](#target-section)` jump (contrast case — today +resolves as a plain URL), the external `` link, and the `` marker preceding the heading. Cross-document (delivered) +additionally: clicking `[text](other-file.md#section)` opens/focuses `other-file.md` and +lands on its `section` heading after load; a bare `[text](other-file.md)` link opens the +target in the viewer (not the browser — the ccTLD repair); re-clicking when the tab is +already open focuses and re-scrolls rather than opening a duplicate; a `#section` with no +matching heading in the opened file shows the file unscrolled with no error. Confirm scroll lands the heading in +view (the existing call uses `AutoScrollMode::PositionOffsetInViewportCenter`, i.e. the target +is centered rather than top-aligned — sanity-check that this reads well for the anchor-jump +case, since it is the behavior already shipped for other `#`-fragment jumps and changing it +would affect them too). + +## Risks and follow-ups + +- **No anchor-index lifecycle risk in phase 1.** An earlier draft of this spec called a + per-document id→offset map's storage and invalidation the single biggest unknown. That risk + is now moot: phase 1 builds no index (item 3). `find_matching_header` resolves live against + the current buffer per click, so there is nothing to store, sync, or invalidate. The only + residual judgment call is cosmetic — whether the existing center-in-viewport autoscroll + reads well for anchor jumps (noted in the manual-testing section). +- **The one behavioral check the implementer must not skip** is the miss path (item 4): today + a `#`-fragment that resolves to nothing falls through to the URL opener. Confirm that + fall-through is observably inert (no broken-link tooltip) per invariant 7, and add an + early-return if it is not. This is the sole place phase-1 correctness is not already + guaranteed by existing master behavior. +- **``'s attribute parser (item 1) needs a maintainer call between a purpose-built + scanner and reusing `html5ever`.** The purpose-built path matches the `` precedent's + spirit (minimal, inline-grammar-native) but is more exposed to malformed real-world HTML + than a real parser; `html5ever` is already a dependency of the crate (via + `html_parser.rs`) so there's no new-dependency cost either way. +- **Interaction with the HTML-table spec:** an ``/`` inside a table cell should + work automatically once cell inline content is parsed via the same `parse_phrasing_content` + path the tables spec already plans to reuse — verify once both land, no explicit design + change anticipated here. +- **Cross-document fragment links are delivered in this PR (item 6a), with three resolution + repairs (item 6b).** The original spec assumed a fragment-less relative link already + opens/focuses the target Markdown-viewer tab today. That was only partly true: a bare + `file.md` (no `./`) misrouted to the browser (ccTLD collision), a `#fragment` broke the file + stat, and a standalone viewer tab lacked a base directory. Repairing those was a prerequisite + for the feature, so the two ship together. The cross-document jump resolves against the + *destination* document's buffer using the **same** `find_matching_header` slug resolver, + invoked after open via a `pending_anchor` drained on first `LayoutUpdated`; the dedup case + (tab already open) scrolls immediately. See items 6a/6b and product invariant 11. +- **Non-silent resolution failures (follow-up).** `resolve_and_open` swallows resolution + errors (link.rs closure). A genuinely session-less standalone tab still can't resolve files + that need a `Session` object, and today that failure is silent. Surfacing it (e.g. a + broken-link tooltip) is deferred — no cheap existing affordance is on this path, and the + spec forbids building new UI in this slice. +- **``/`` explicit anchor markers (follow-up, was "phase 2").** Not in this PR. + Heading auto-anchors cover the common case; item 5's design stands for the follow-up.