Repository navigation
fix(export): image headings name the resolved file; diff comments sort after the document - #1700
Merged
Merged
Conversation
…t after the document Two export bugs found by the 0.28.1 smoke tests. Image source in the heading: the composer quote for a text-less media pin resolves the file through src, a lazy data-src over a data: placeholder, srcset, a video's <source> and poster (descRawSource in the bridge), but the export read only the src attribute, so a srcset-only image exported as a bare "Feedback on the <img> element" and a lazy one as "(data:image/gif)". The bridge now records the file it names as elementContext.sourceName (validated in core's parseHtmlElementContext, capped, additive), and the export heading reads it, falling back to the src attribute for contexts saved before. One resolution, in the bridge: the heading and the quote cannot disagree. The bridge also resolves <picture><source srcset><img>, which named nothing before. Diff comment order: version-diff comments (blockId diff-block-N) are not in the document's blocks, so they ranked -1 and printed FIRST. N indexes the diff against the selected base and nothing maps it back to a block, so they now print together after the document's other comments, in diff order. A list with no diff comments sorts exactly as before (fuzzed against the old comparator). The [In diff content] label is unchanged.
…ole context sourceName was in neither shed order, so an image pin whose context only fit without it (a long live-app route) passed the bridge but was dropped whole by core's validator. Append it to both orders, last: the export heading falls back to the src attribute without it. Core's order is now the exported ELEMENT_CONTEXT_SHED_ORDER and a test keeps the bridge's CTX_SHED_ORDER equal to it.
1 task
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.
Two low-severity export bugs from the 0.28.1 smoke tests, targeting 0.28.2.
1. Image heading names the wrong file, or none
The composer quote for a text-less media pin resolves the file through
src, a lazydata-srcover adata:placeholder,srcset, a video's<source>, andposter(descRawSourcein the bridge). The export read only thesrcattribute.Fix. The bridge now records the file it names as
elementContext.sourceName, and the export heading reads that field. The value is the bridge's owndescSourceName, after the samectxScrubUrlscrub. Core'sparseHtmlElementContextvalidates it and caps it at 80 characters. The field is optional and additive. Contexts saved without it fall back to the scrubbedsrcattribute, which is the old behavior. The resolution happens only in the bridge, so the heading and the quote cannot name different files.I chose this over adding
srcset/data-src/posterto the attribute allowlist because the export never sees child elements: a video's<source>and<picture><source>cannot be rebuilt from the element's own attributes. The allowlist is unchanged.The bridge also resolves
<picture><source srcset><img>now (the first candidate). Before, neither the quote nor the export named a file for it.<img src="data:image/gif…" data-src="img/real-photo.png?v=3">Feedback on the <img> element (data:image/gif)Feedback on the <img> element (real-photo.png)<img srcset="img/hero-480.jpg 480w, …">Feedback on the <img> elementFeedback on the <img> element (hero-480.jpg)<picture><source srcset="img/pic.avif"><img></picture>Feedback on the <img> element(the quote had no file either)Feedback on the <img> element (pic.avif)2. Version-diff comments printed first
A diff-view comment has
blockIddiff-block-N. That id is not inblocks, sosortAnnotationsInDocumentOrderranked it -1, ahead of everything else. N indexes the diff against the selected base. Nothing on the annotation maps it back to a document block, and the export has no base version to recompute the diff from.Fix. Diff comments now print together after the document's other comments, ordered by diff index. The diff walks the document, so diff order is also document order among the diff comments. The comparator is unchanged for every non-diff pair, so a document with no diff comments exports byte-identically. A test fuzzes this against the pre-fix comparator over 200 random annotation sets that include blockless ids.
[In diff content]is kept.Tests
packages/ui/utils/parser.diffOrder.test.ts(new, pure): root and linked-document order, with and without blocks; a global comment keeps its place; fuzzed byte-identity with no diff comments.packages/ui/utils/parser.test.ts: the heading usessourceNamefor srcset-only and lazy images; a name equal to the file is not repeated.packages/ui/components/html-viewer/srcdoc.test.ts(already in DOM_TESTS): runs the real bridge on srcset-only, lazydata-src,<picture>, videoposter, video<source>andsrcwith a query. For each case it pins the element, passes the posted context throughparseHtmlElementContext, exports it, and asserts that the heading names the same file as the composer quote and that no query secret survives.Verification
bun run typecheckpasses.bun test packages/ui packages/editor packages/core: 1694 pass, 0 fail.test.yml(DOM_TESTS=1 --isolate): 1152 pass, 0 fail.scripts/dom-test-allowlist.test.tspasses.apps/guides-show check:manifestis in sync; the guide viewer build is unchanged.plannotator annotatesession, using the built hook HTML:data-srcimage and a<picture>. Each pin's composer quote and the submitted/api/feedbackheading both namehero-480.jpg,real-photo.pngandpic.avif.[In diff content].Notes
HtmlElementContext.sourceName?and its validation inpackages/core/html-anchor.ts. UI code reads the field untyped and does not need a new core to compile. A host on an older published core's validator drops the field, and its export falls back tosrcas today. Publish core before ui as usual.packages/editor/App.tsx,annotateSubmission.tsandapps/hook/server/host-result.ts.BRIDGE_PROTOCOL_VERSIONbump): the context field is additive in both directions.