feat: link #Heading section references in annotation text, and a skill that uses them - #1419
Open
centraldogma99 wants to merge 6 commits into
Open
feat: link #Heading section references in annotation text, and a skill that uses them#1419centraldogma99 wants to merge 6 commits into
#Heading section references in annotation text, and a skill that uses them#1419centraldogma99 wants to merge 6 commits into
Conversation
An annotation that names another section in prose ("this contradicts
3.1") hands the reader the cross-referencing work the annotation surface
exists to remove: they have to hold the comment in their head while they
go find 3.1 in the document. Comment text rendered as a bare string had
no way to close that gap.
Writing `backnotprop#3.1 Single source of truth` in a comment now renders that
heading text as a link that scrolls the document to it.
Resolution is exact match against the document's own headings, so there
is no grammar of section numbers to define or get wrong — `3.1`, `A.`
and `IV.` are all just heading text. That is also what keeps the common
non-references inert: `#fff` and `backnotprop#123` would need a heading literally
named `fff` / `123`, and `C#` (hash welded to a word) plus `# ` (a
markdown heading) are skipped before lookup. Backtick spans are never
scanned, longest match wins, and a title used by two headings resolves
to neither.
Additive for hosts: the panel parses only when `onNavigateAnchor` and a
non-empty heading index are both present, so a host passing nothing gets
the same single `<p>` as before. `ViewerHandle.scrollToAnchor` exposes
the callback `Viewer` already had; `HtmlViewer` implements it as a no-op
because raw HTML carries no heading blocks. `CodeAnnotationCard` is
untouched — review annotations address files and lines.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lz3aBzrpHocVFU94YN7QBr
Drives the external-annotations API so an agent's answer lands as markup on the user's document — inline comments and strikethrough edits pinned to the phrases they concern — instead of prose in chat that the reader then has to match against the file by hand. The mechanics are not currently discoverable. `plannotator annotate` blocks until the human decides, so an agent that launches it in the foreground cannot post into the session it just started; `plannotator sessions` prints the base URL to stderr, so a pipe without `2>&1` reads empty; and `DELETION` discards its `text`, so an agent that puts its suggested replacement there watches it vanish. Each of those is a silent failure. The skill is only that contract — what to say about a document stays with the request that asked for it. Shipped locked (`disable-model-invocation: true`) like every other skill here, and registered in the three installers' extras list so the "Make any skills callable by the model?" prompt offers it. The two historical cleanup lists are deliberately untouched: a name in those is a name the installer deletes. `SKILL.test.ts` pins the claims that can go stale silently — the endpoint path, the annotation types the plan transformer accepts, and that the export still drops `DELETION`'s text — against their sources. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lz3aBzrpHocVFU94YN7QBr
An external annotation is pinned by searching the rendered DOM for its `originalText`. The text an agent has to quote comes from `/api/plan`, which serves the markdown SOURCE — so `**Install** now`, ``Setup `bun` `` and `[Setup](./s.md)` are faithful quotes of phrases that never appear in the DOM, because the renderer consumed that syntax before rendering. The failure is silent in the worst way: the server accepts the annotation, it appears in the panel, and only the highlight is missing. The one trace is a `console.warn`. Half of the divergence was already handled — a miss retries with the renderer's plain-text transforms, so `---` and `:rocket:` re-bind — and that partial coverage is what made it read as random: dashes work, backticks do not. `findTextInDOM` becomes an explicit four-rung ladder (literal, plain-text transforms, markup stripped, both), each rung tried only when the cheaper one missed. The stripper is `slugifyHeading`'s, moved to `inlineTransforms` and shared verbatim rather than reimplemented, so anchors and text restore cannot disagree about what markup is; slug output is unchanged, pinned by the existing tests plus new rows. The strip is deliberately crude — it drops every `*_`~`, so `snake_case` becomes `snakecase`. That is safe here because a needle that survives the literal rung never reaches it. The DOM test is mutation-checked: without the new rungs, the emphasis, code-span and link cases each fail while the two pre-existing rungs continue to pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UiYakvfsUMxQk6z3d1vBrF
Two more ways an external annotation is accepted, listed, and silently never highlighted — both found on the same document, both invisible except for a `console.warn`. A table row quoted from the source (`| a | b |`) has pipes and cell padding that exist nowhere in the DOM: the renderer makes sibling `<td>`s with no separator text at all. The restore ladder could not reach it, because every rung rewrites only the NEEDLE, and this divergence is on the haystack side too — `` `JIRA_*` `` renders as `JIRA_*` while the strip rung has already taken the `_*` off the needle. A last-resort rung now drops markdown noise from BOTH sides (whitespace included, since the row's own spaces are what the DOM lacks) and maps the match back through an index map, so the rendered range is still exact. The other is text the renderer injects for itself: an ambiguous code-file link appends a match-count `<sup>`, so `` `config.ts` `` becomes `config.ts7` in `textContent` and no faithful quote can ever match it. That `<sup>` is now `aria-hidden` — the count is already in the `title`, and it is not document text — and restore reads the DOM through a TreeWalker that honors that flag rather than through `textContent`. Marking chrome aria-hidden is the general fix: the next decoration is skipped by construction. Verified end to end on the reported document: 11 of 11 annotations bind, where the table row missed before. Both rungs are mutation-checked — reverting the canonical rung fails only the table-row case, reverting the aria-hidden filter only the decoration case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U6xpqeyB2T3UGowSAZvNRt
A section reference is written by someone reading the document, so the
heading text they copy is the RENDERED text. The index was built from
`block.content` — raw markdown — so every heading carrying inline markup
was referenceable only by a spelling nobody ever sees:
heading "**Install** now" → #Install now plain text
heading "Setup `bun`" → #Setup bun plain text
heading "[Setup](./s.md)" → #Setup plain text
heading "Part 1 --- end" → #Part 1 — end plain text
heading ":rocket: Launch" → #🚀 Launch plain text
A code-span heading was unreferenceable outright: typing the raw form
does not help either, because `splitOnCodeSpans` removes the backticked
run from the comment before lookup ever happens.
Each heading is now registered under both spellings — the raw text and
`transformPlainText(stripInlineMarkdown(...))`, the same approximation of
the renderer's output that the annotation restore ladder uses, so the two
cannot disagree about what markup is. The raw key stays, since an agent
quoting `/api/plan` sees the source rather than the page.
The ambiguity rule now compares anchors rather than counting insertions:
anchors are already deduplicated per heading, so an anchor identifies its
heading, one heading registering two spellings is not a collision, and
two DIFFERENT headings that collapse to the same text (`Install` and
`**Install**`) still drop the key rather than resolving to the first.
Mutation-checked: without the second spelling, all five rows above and
the two-headings-collapse case fail; the pre-existing tests do not move.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UiYakvfsUMxQk6z3d1vBrF
…arkup An external annotation quotes the markdown SOURCE — that is what `/api/plan` serves — and is pinned by searching the RENDERED DOM. Bridging the two by approximating the renderer's inverse with regexes was never going to end: each markdown construct bought a rung, and the ladder grew to four while still missing everything where the DOM is the side that diverges. Rewriting the needle cannot remove text the haystack GAINED. A bulleted item is both failures at once: the source's `- ` is nowhere in the page, and the page's `•` is nowhere in the source. So stop approximating. `utils/renderedText` runs the real renderer over the quote — `parseMarkdownToBlocks` → `BlockRenderer` → `renderToStaticMarkup` into an inert `<template>` — and compares its text. Four approximation rungs collapse into one, and the markdown feature added next is covered the day it renders rather than the day someone notices a missing highlight. Both sides are then read through one `visibleText` walker that skips text the renderer DREW rather than text the document contains. Two flags, because they are two different claims: `aria-hidden` on the ambiguous code-file link's match-count badge, which repeats its own `title`; `data-decorative` on the list glyphs, which must stay audible — these list items are divs with no list semantics, so the bullet is the only thing marking them as a list, and hiding it from assistive tech to fix an anchoring bug would be a bad trade. The ordered numeral is why this matters beyond tidiness: CommonMark renumbers `1. / 2. / 5.` as 1, 2, 3, so the numeral the reader sees is in neither the source nor a quote rendered on its own. What survives is a two-rung ladder plus one last resort: the literal quote, the rendered quote, and a match that drops markdown noise and whitespace from both sides for a quote cutting a block in half (`Install** now`) — the one shape a faithful render genuinely cannot reproduce. `stripInlineMarkdown` leaves restore and now serves `slugifyHeading` alone, which stays pure and DOM-free on purpose: its output is anchor ids that live in URLs. Verified end to end by quoting all 110 block lines of the document that reported the bug: 76/110 anchored before, 110/110 after, every one of the 34 misses a bulleted item. Each mechanism is mutation-checked in the ladder test, whose harness renders the document through Viewer's own parser, grouping and BlockRenderer rather than hand-written DOM — approximating the renderer in the test would be the same mistake one level up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U6xpqeyB2T3UGowSAZvNRt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You have probably hit this
You are reading a review in Plannotator. A comment says:
So you hold that sentence in your head, scroll off to find 3.1, read it, scroll back, and reconstruct what the comment meant. Then the next comment mentions 3.2 and you do it again.
Every other part of this surface exists to spare you exactly that. An inline annotation is a pointer — click it in the panel and the document scrolls to the phrase. But the moment a comment refers to a different section, the pointer stops at the comment's own anchor and the rest is manual. Comment text renders as a bare string (
<p>{annotation.text}</p>), so3.1is inert.It bites hardest on
GLOBAL_COMMENT, which by definition is anchored to no phrase at all, and on any remark whose whole point is that two sections disagree — the case where you most need both ends and have neither.What this does
Writing
#followed by a heading's exact text in a comment renders it as a link that scrolls the document to that heading.Resolution is exact match against the document's own headings. That is the whole design: there is no grammar of section numbers to define, so
3.1,A.,IV.and an unnumbered## Authenticationare all just heading text, and nothing has to be taught about numbering schemes that vary per document.It is also what keeps the common non-references inert, without a denylist:
#fff,#123fff/123C##welded to a word is part of that word# Heading#is markdown heading intent`#Overview`Longest match wins, so a document with both
3.1and3.1 Scoperesolves#3.1 Scopeto the longer one. Trailing text stays outside the link, which is what makes agglutinative languages work without tokenizing (#3.1 단일 진실 원천의 전제→ the의is not part of the link).Full heading text is deliberate over a bare number: if the link ever fails to resolve, the sentence still reads.
3.1alone does not.Host compatibility
Additive, and inert unless opted into:
AnnotationPanelonNavigateAnchor?: (hash: string) => void— the panel parses only when this AND a non-empty heading index are both present. A host passing nothing renders the same single<p>as before. The panel already receivedblocks, so the index costs the host nothing.ViewerHandle.scrollToAnchor(hash) => boolean— exposes the callbackVieweralready had internally for in-document anchor links. Required rather than optional on purpose: the interface is implemented only inside this package (Viewer,HtmlViewer) and consumed everywhere else through a ref, so requiring it costs no host anything while forcing both implementations to stay in step.HtmlViewerimplements it as() => false— raw HTML pages have no markdown heading blocks to resolve against.utils/sectionRefs—buildSectionRefIndex/parseSectionRefs, both pure and DOM-free.CodeAnnotationCardis untouched: review annotations address files and lines, and a review surface has no heading blocks.Documented in
packages/ui/README.mdandpackages/ui/HANDOFF.mdfollowing the existing seam convention. No version bump — I left that to the maintainer; nothing underpackages/corechanged, so it should be a ui-only publish.Second commit: the
plannotator-markupextra skillThe same problem from the other end. An agent that wants to answer as markup on a document has to drive the external-annotations API, and three of its properties fail silently today:
plannotator annotateblocks until the human decides, so an agent that launches it in the foreground can never post into the session it just started.plannotator sessionsprints the base URL to stderr, so a pipe without2>&1reads empty and the flow stalls at step 2.DELETIONdiscards itstext(the export writes a fixed sentence), so an agent that puts a suggested replacement there watches it disappear.The skill is only that contract — launch, discover, read, POST, clean up on re-run. What to say about a document stays with the request that asked for it; there is no review checklist or perspective baked in.
Conventions followed:
apps/skills/extra/, notcore/— opt-in, not default-installed.disable-model-invocation: true) like every other skill here, per the locked-by-default rule stated inapps/skills/core/plannotator/agents/openai.yaml.EXTRA_SKILL_NAMES,$extraSkillNames, the.cmdpicker) so the "Make any skills callable by the model?" prompt offers it. The two historical cleanup lists are deliberately untouched — a name in those is a name the installer deletes.SKILL.test.ts, following the freshness-guard patternplannotator-skill-reference.test.tsuses for the core skill. It pins the endpoint path, the annotation typestransformPlanInputaccepts, and the fact that the export still dropsDELETION's text, each against its source. Mutation-checked: removing the DELETION warning, or inventing a fourth annotation type, each fails exactly one test.AGENTS.md's project structure (CLAUDE.mdis a symlink to it).Verification
Local run of the CI steps:
bun run check:release-versionv0.27.9)bash apps/pi-extension/vendor.shbun run typecheck(10 tsconfigs, incl.strict-consumer)bun testDOM_TESTS=1seam batchNew unit tests (
packages/ui/utils/sectionRefs.test.ts, 11 cases) pin every row of the table above plus longest-match, unicode headings, and the trailing-text rule. Mutation-checked: dropping the duplicate-title guard, flipping longest-match to shortest, and removing code-span protection each fail exactly one test.Verified in a real browser on an annotate session, not just in tests:
#없는섹션/#fffas plain text, trailing particle outside the link.getBoundingClientRect().topfrom 2781px → 417px, with the section visible on screen afterwards.Pre-existing test failure, unrelated to this change
packages/ui/components/Viewer.consumer.test.tsx→ "readOnly fenced code never opens composers or mutates the rendered code" fails underDOM_TESTS=1. It assertscode.innerHTMLequals the plain source, but Shiki has highlighted it into token spans by then.I confirmed this is pre-existing by stashing this branch and running the same test on a clean tree — it fails identically there. Not touched here.