Repository navigation
Capture LinkElem as a fourth links family, and --input/--inputs-file - #3
Merged
Merged
Conversation
The typst-cnd CLI had no sys.inputs support at all. Add it via the same Dict that Typst's own convention uses, plus a file-based alternative for data too large for a comfortable argv: - `--input key=value` (mirrors typst-cli, repeatable) - `--inputs-file <path>`: a JSON object of string values, merged into the same inputs map rather than a second one. `--input` wins on a key collision (documented in its help text and covered by a test). CndWorld::new keeps its old signature and behavior unchanged (empty inputs by default); CndWorld::new_with_inputs is the new entry point the CLI uses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review fix round 1 on the --inputs-file work:
- inputs_file_rejects_non_string_value asserted stderr.contains('k'),
which passes against almost any error text. Pin the exact quoted-key
substring instead (accounting for the double Debug-escaping main()'s
error printing applies). Tighten the sibling non-object assertion from
"object" to "json object" the same way.
- load_inputs_file now rejects an empty key, matching parse_sys_input_pair
(--input already rejected one) so both flags feeding the same inputs
map behave consistently. Covered by a new test.
- Document, without changing, the pre-existing trim asymmetry: --input
trims key/value whitespace (typed by hand), --inputs-file does not
(values are often serialized JSON where whitespace can matter).
- Correct the duplication comment: parse_sys_input_pair isn't reachable
from typst-cli because that crate has no [lib] target at all, not
because the original is merely private.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…024) Maps a realized `LinkElem` per the three-row design: a URL destination becomes a `links` entry (`href`, `text_span`); a label/location destination is a cross-reference with a custom body and folds into the existing `refs`/`set_ref` machinery; a page/point position has no label and is dropped. `LinkElem` is Locatable, so its own Tag::Start/Tag::End already bracket the realized `LinkMarker` at the same location — this is captured via the existing cite/footnote `open_frame` bracket, not the ref/LinkMarker pending-position mechanism, which stays untouched (it pairs a *different* marker's rendered text and was never touched by links here). Adds `NodeLink`/`NodeBase.links` to the typst-cnd model (cnd-sdk's Task 1 shape, mirrored per ADR 0019's no-shared-code rule), a universal bare-traverse fallback for links inside non-flat nodes (list items, mirroring `ref_targets`), and a null-span rule for a marker that opens and closes with no rendered text in between (an empty-bodied link). New fixture `examples/links.typ` pins the case that motivated this design: a link whose body opens with a ref (`#link(url)[@Label ...]`). The fork now needs a pin bump in services/indexer before consumers can read `links` (see the existing `chore(indexer): pin typst-cnd to ...` commit shape). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- extract.rs: flush a still-open `Link` frame at the end of `extract_with_markers` with a degenerate `[start,start)` span rather than silently dropping the edge (Link-only; cite/footnote drop behavior is unchanged and still pinned by existing fixtures). Verified against real compiles that this does NOT rescue a body that forces its own paragraph break (`#link(url)[first\n\nsecond]`, `#block[..]`, `#image(..)` bodies all behave the same): typst-realize hoists a tag pair straddling a group boundary out of both resulting groups, so neither node's walk ever sees either tag. Documented as a known, currently-uncovered gap rather than claiming the flush closes it — corrects the review's suggested-fix framing against what actually reproduces. - model.rs: bump CND_VERSION to "0.4.0" (the `links` family is a 0.4.0 format change; both callers compare against the constant itself, so this is zero-churn). - document_suite.rs: rename the link-capture test off "pins marker pairing" (that collision cannot happen under open_frame capture) to state what it actually guards against; add a fixture-backed assertion pinning the multi-paragraph-body gap above. - links.typ: comment explaining why the fixture's two headings are split (works around a pre-existing, unrelated bug in refs.rs::ref_edges_from_content, filed separately, not fixed here); add the multi-paragraph-body case. - refs.rs: guard `LinkDest::Label` against a bibliography key, mirroring resolve_refs's existing guard (defensive; resolve_label already returns None for one today). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comments-only, no behavior change (confirmed: cargo test -p typst-cnd
unchanged, 29/29).
The Link-only flush in extract.rs::extract_with_markers does fire, on
the right node, with a null span — verified by a re-review that built
both pre- and post-fix binaries and diffed real output. My prior
comments claimed the opposite ("there is no open frame for this flush,
or any per-node fix, to reach") and attributed the failure mode to
"paragraph grouping hoists a tag pair out of both resulting groups" for
all three probes (two-paragraph body, #block, #image) uniformly. Both
claims were wrong:
- The flush rescues a link whose Tag::Start is interior to the
enclosing paragraph group's trigger range (leading text before the
link in the same paragraph). It only fails to reach a
paragraph-initial link, whose Tag::Start sits before any group's
trigger range and stays in the outer flow — per
typst-realize/src/lib.rs::finish_grouping, PAR keeps tags (tags:
true) and includes one only when interior to the trigger range or
adjacent to it, which a paragraph-initial link's tags satisfy on
neither side. Nothing is hoisted.
- #block/#image bodies are a distinct case with the same visible
effect for an unrelated reason: BlockElem/ImageElem are Interrupt for
PAR, so no paragraph group forms around the link at the flow level at
all, and the block's own inner paragraph never sees the link's tags.
Also corrected: the Link-only scoping reason for cite/footnote cited an
existing fixture that does not exist; replaced with the real reason
(their own TagElem-unwrapping collectors already cover them
universally).
The three fixed spots: extract.rs's doc comment on the flush,
links.typ's comment on the multi-paragraph-body fixture (rescoped to
the paragraph-initial shape it actually exercises), and the matching
test comment in document_suite.rs. The test assertions themselves are
unchanged — they were already correctly labelled as pinning a known
gap.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d it did CI sets RUSTFLAGS: -Dwarnings, so every warning is a build error. Two reviews of this branch concluded otherwise — one read local clippy output without the flag and called the dead-code notices pre-existing, the other stated the clippy step does not pass -D warnings. Both were wrong, and the branch went red on main's own workflow. tests/common/mod.rs gains allow(dead_code): every integration test binary compiles the whole module, so adding cli_inputs.rs — which uses almost none of it — turned the rest into dead code there. The helpers are not unused, they are unused *in that binary*. refs.rs gains a LinkEdge alias beside the RefEdge one already there, which is what the file's own convention says to do and what silences clippy::type_complexity. Verified locally with the flags CI actually uses: RUSTFLAGS=-Dwarnings cargo clippy -p typst-cnd --all-targets --all-features clean, and the same for cargo test — 38 passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three assertions checked that the error names the offending file by
comparing stderr against the path we hold. main() prints each
SourceDiagnostic with {:?}, and the derived Debug escapes the message's
own backslashes — so on Windows stderr carries C:\\Users\\... while the
value has single separators. On Unix the two are identical, which is why
this passed locally and on Linux CI and failed only on windows-latest.
Same escaping quirk a fix round already hit on this branch for the
quoted key; it was fixed there for the key and not noticed for the path,
because no Unix path has a backslash to reveal it.
The assertion keeps its intent — the error must name the path — by
escaping the expected value the same way rather than falling back to
matching the file name alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two units, both additive to the CND emitter and its CLI.
linkscaptureFor each realised
LinkElemin a text-bearing node: aUrldestination becomes alinksentry ({href, text_span}); aLabel/Locationdestination —#link(<label>)[body]— becomes arefsentry, because it is a cross-reference to a node with a custom body; aPositiondestination is dropped, being neither durable nor labelled.CND_VERSIONmoves to0.4.0, matching the format release this shape comes from (cnd-sdkv0.6.0).The capture goes through the existing
open_framebracket, which is keyed on theLinkElem's ownLocationat both ends and is structurally disjoint from thepending_refs/link_for_refmechanism. An earlier instruction asked for that mechanism to be re-keyed onmarker_loc; it turned out not to be implementable as written —LinkMarkercarries onlybodyandalt,PendingRefonlylabelandstart, and the two share no key.A known gap, documented in three places and pinned by a fixture whose assertion says so: a link that opens its paragraph and whose body then splits across a paragraph or block boundary loses its edge. A link with leading text before it on the same line is captured, with a null span. The difference is whether the
Tag::Startis interior to the enclosing paragraph group's trigger range; closing it properly is a flow-level change rather than a local one.Also pre-existing and left alone, with a comment at the fixture that trips it:
refs.rs::ref_edges_from_content's position-correlation heuristic tracks onlyHeadingElem, so it misattributes spanless ref edges.--inputand--inputs-file--inputs-file <path>loads a JSON object of string values intosys.inputs, for blobs too large for argv.--input key=valuehad to come with it: this CLI had nosys.inputssupport at all, so there was no map to merge into.--inputwins on a key collision, documented in both flags' help and covered by a test.Verification
cargo test -p typst-cnd: 29document_suite, 6cli_inputs, 3 unit — 38 passing, no existing fixture moved. Reviewed per unit and then as a whole branch; the final review returned no Critical and no Important.🤖 Generated with Claude Code