Skip to content

Commit 639accc

Browse files
authored
Merge pull request #316 from quarto-dev/feature/bd-w0c6d38k-revealjs-crossref
fix(revealjs): resolve cross-references + enforce a transform-phase ordering invariant
2 parents 0940c94 + ebe1707 commit 639accc

54 files changed

Lines changed: 1246 additions & 53 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

CLAUDE.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,10 @@ The render pipeline has a **profile checkpoint** between `MetadataMergeStage` an
366366

367367
Q2 emits HTML **directly from the Pandoc AST** and has **no post-Pandoc DOM-mutation stage**. Quarto 1, by contrast, leans heavily on DOM postprocessors (functions that parse the rendered HTML into a `Document` and mutate it — e.g. reveal's `applyStretch`, which hoists a stretched `<img>` to be a direct child of its `<section>`). **When porting such a feature, re-express it as an AST transform** (operate on `Block`/`Inline` so the writer emits the right HTML the first time) rather than adding a DOM postprocessor. A DOM-postprocessor stage is a large new architectural surface with no current consumer; do not introduce one without an extremely strong, explicitly-discussed reason. Worked example: revealjs auto-stretch unwraps a `Paragraph[Image]` into a `Plain[Image]` in `crates/quarto-core/src/revealjs/auto_stretch.rs` to get `section > img.r-stretch`, instead of mutating the DOM after the fact (bd-zkstclhl).
368368

369+
### Transform pipeline phases — format-agnostic semantics before format-specific presentation
370+
371+
The single transform pipeline (`build_transform_pipeline` in `crates/quarto-core/src/pipeline.rs`) runs every output format, splicing format-specific transforms in at chosen positions. It must visit transforms in **non-decreasing phase rank**: `Normalization` → `Crossref` → `Navigation` → `Finalization`. The hard rule: **a format-specific *presentation* transform that consumes semantic structure (a float, caption, number, or resolved `@ref`) must run in `Finalization`, after `crossref-render` — never earlier.** Each `AstTransform` declares its phase via `fn phase(&self) -> TransformPhase` (defaults to `Unclassified`; every pipeline member overrides it). The invariant is enforced by `test_build_transform_pipeline_phase_ordering`, which is format-neutral (loops over format strings, no `is_revealjs` branch). When adding a new format-specific late transform, add it via a named seam like `reveal_finalization_transforms`/`footer_render_stage`, not an inline `if is_revealjs { … }` at an arbitrary position. This is why a revealjs presentation transform (auto-stretch) once silently broke crossrefs by running before they were numbered (bd-w0c6d38k). Full contract + author rule + the preview-pipeline shape contract: `claude-notes/designs/transform-pipeline-phases.md`.
372+
369373
## hub-client Commit Instructions
370374

371375
**IMPORTANT**: When making commits that include changes to `hub-client/`, you MUST also update `hub-client/changelog.md`.
Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
# Transform pipeline phases & the format-agnostic-first invariant
2+
3+
**Status:** Implemented under bd-w0c6d38k (2026-06-18). `TransformPhase` and the
4+
defaulted `phase()` method live on `AstTransform` (`crates/quarto-core/src/transform.rs`);
5+
all render-pipeline transforms are classified; the invariant is enforced by
6+
`test_build_transform_pipeline_phase_ordering` in `pipeline.rs`. The first
7+
consumer of the seam is the revealjs auto-stretch move (auto-stretch now runs in
8+
`Finalization`, after `crossref-render`).
9+
10+
**Audience:** anyone adding or reordering an `AstTransform` in
11+
`build_transform_pipeline` (`crates/quarto-core/src/pipeline.rs`), and especially
12+
anyone adding a new output format (`dashboard`, `typst`, `pdf`, …) with its own
13+
format-specific transforms.
14+
15+
---
16+
17+
## Why this exists
18+
19+
Q2 renders every output format through **one** transform pipeline
20+
(`build_transform_pipeline`, `pipeline.rs:1078`), with format-specific transforms
21+
spliced in at chosen positions (today via inline `if is_revealjs { … }`). The
22+
pipeline mixes two fundamentally different kinds of work:
23+
24+
1. **Format-agnostic semantic normalization** — establishing *what the document
25+
means*: callouts, theorems, proofs, floats (Figure/Table), equations, and the
26+
crossref index/resolve that numbers them and rewrites `@refs`.
27+
2. **Format-specific presentation** — reshaping the resolved document for *one*
28+
output: revealjs slide construction, single-image auto-stretch, etc.
29+
30+
These have a hard dependency direction: **presentation depends on semantics, not
31+
the other way around.** A revealjs transform that hoists a `Figure` to a bare
32+
`<img>` is only correct *after* that `Figure` has been recognized, numbered, and
33+
resolved as a crossref float.
34+
35+
When that direction is violated, the failure is silent and format-specific. The
36+
motivating incident (bd-w0c6d38k): `RevealAutoStretchTransform` (`pipeline.rs:1148`)
37+
ran *before* `FloatRefTargetSugarTransform` (`:1166`) and the crossref phase
38+
(`:1170`). On a single-image slide it hoisted `![cap](x){#fig-1}` (a Pandoc
39+
`Figure`) into a bare `Plain[Image]` before the float was ever registered — so
40+
`@fig-1` rendered unresolved and a sibling figure was mis-numbered. Every test
41+
passed; only `format: revealjs` was affected, and only for that figure syntax.
42+
43+
---
44+
45+
## The phase model
46+
47+
Every transform belongs to exactly one phase. Phases are **totally ordered**;
48+
their order is the contract.
49+
50+
| rank | `TransformPhase` | purpose | examples |
51+
|------|------------------|---------|----------|
52+
| 1 | `Normalization` | format-agnostic semantic sugar **and** format-specific *structural scaffolding* that does not consume crossref semantics | `callout`, `theorem-sugar`, `proof-sugar`, `float-ref-target-sugar`, `equation-label`, `metadata-normalize`, `footnotes`, `example-embed`; **`reveal-columns`, `reveal-slides`, `reveal-footnotes`, `reveal-footer-alias`** |
53+
| 2 | `Crossref` | assign section-scoped numbers; rewrite `@refs` to custom nodes | `crossref-index`, `crossref-resolve` |
54+
| 3 | `Navigation` | TOC / navbar / sidebar / page-nav / footer / listings generate + render | `toc-*`, `navbar-*`, `sidebar-*`, `footer-generate`, `listing-*` |
55+
| 4 | `Finalization` | render custom nodes to writer-visible shapes, then format presentation that *consumes* those shapes, then resource/attribution baking | `link-rewrite`, `appendix-structure`, **`crossref-render`**, **`reveal-auto-stretch`** (after the move), `code-block-render`, `resource-collector`, `attribution-render` |
56+
57+
Two subtleties this model encodes:
58+
59+
- **Crossref spans two phases.** Index + resolve happen in `Crossref` (rank 2);
60+
the conversion of crossref custom nodes into real `Figure`/`Div`/`Span`/`Link`
61+
shapes happens in `crossref-render` (`pipeline.rs:1259`), which is `Finalization`
62+
(rank 4). This is *why* a presentation transform that needs the final `Figure`
63+
(auto-stretch) belongs in `Finalization`, after `crossref-render` — not in
64+
`Crossref`.
65+
- **Not all format-specific transforms are late.** `reveal-slides`/`reveal-columns`
66+
build the `<section>` scaffolding from raw blocks and metadata; they do **not**
67+
read or mutate floats/captions/numbers, so they correctly live in
68+
`Normalization`. The distinction is *what a transform consumes*, not merely that
69+
it is format-specific (see the author rule below).
70+
71+
---
72+
73+
## The invariant
74+
75+
> **Format-agnostic semantic structure is fully established before any transform
76+
> that consumes it.** Concretely, in the built pipeline the transforms' phase
77+
> ranks are **non-decreasing by position** (a transform never precedes one of a
78+
> lower rank).
79+
80+
Because `crossref-render` is `Finalization` and a presentation transform that
81+
hoists a crossref float is also `Finalization`, the rank order alone forbids the
82+
bug: a `Finalization` transform cannot sit among `Normalization` transforms ahead
83+
of the `Crossref` phase.
84+
85+
### Author rule (which phase do I pick?)
86+
87+
When adding a format-specific transform, ask: **does it read or mutate a structure
88+
produced by the Crossref phase — a float, a caption, a number, or a resolved
89+
`@ref`?**
90+
91+
- **Yes** → phase `Finalization`, and it must run *after* `crossref-render`. (This
92+
is auto-stretch.)
93+
- **No, it only builds format scaffolding from raw blocks/metadata** → phase
94+
`Normalization` is acceptable. (This is `reveal-slides`.)
95+
96+
If unsure, choose `Finalization`. Running a presentation transform too late is at
97+
worst a missed optimization; running it too early silently corrupts semantics.
98+
99+
---
100+
101+
## Enforcement
102+
103+
### `phase()` on the `AstTransform` trait
104+
105+
`AstTransform` (`crates/quarto-core/src/transform.rs:69`) gains a method:
106+
107+
```rust
108+
fn phase(&self) -> TransformPhase { TransformPhase::Unclassified }
109+
```
110+
111+
It **defaults to `Unclassified`** rather than being required-with-no-default. The
112+
reason is honesty: a handful of `AstTransform` impls are *not* part of the ordered
113+
multi-format render pipeline — engine-stage transforms (`JupyterTransform`), the
114+
stage-level `AttributionGenerateTransform`, and in-module test doubles. Forcing
115+
them to fabricate a render-pipeline phase would be a lie. Instead, `Unclassified`
116+
is the honest default for "not a member of the ordered pipeline," and **every
117+
transform that actually appears in `build_transform_pipeline` overrides it** with
118+
a real phase. The anti-drift guarantee is preserved by the test, not the compiler:
119+
the ordering test **fails if any transform in the built pipeline reports
120+
`Unclassified`** (exhaustiveness), so a new pipeline transform that forgets to
121+
classify itself is caught loudly. This mirrors the existing deny-list-name
122+
validator (`pipeline.rs:1344`), which is likewise a test, not a compiler check.
123+
124+
Cost: one extra vtable slot per transform *type* (a fn pointer in static rodata);
125+
**zero per-instance memory and zero render-time cost**`phase()` is called only
126+
by the ordering test, never on the render hot path. (`is_format_specific()` may be
127+
added later as a sibling method if a real consumer appears; the ordering invariant
128+
needs only `phase()`.)
129+
130+
`TransformPhase` derives `PartialOrd`/`Ord` from declaration order; `Unclassified`
131+
is declared **last** so a leaked value sorts high, though the exhaustiveness check
132+
rejects it before the monotonicity comparison runs.
133+
134+
### The ordering-invariant test
135+
136+
A test over the **real** `build_transform_pipeline` (the analogue of the existing
137+
*analysis*-pipeline test, `pipeline.rs:1851`) that:
138+
139+
1. builds the pipeline for **each supported format string** (`html`, `revealjs`,
140+
and `dashboard`/`typst`/`pdf` as they land — a list, not an `is_revealjs`
141+
branch, so new formats are covered without new assertions);
142+
2. reads each transform's `phase()`;
143+
3. asserts ranks are **non-decreasing by position**;
144+
4. on failure, names the offending transform and its neighbours.
145+
146+
This test **fails on today's code** (auto-stretch is `Finalization`-rank sitting
147+
before the `Crossref` phase) and **passes after auto-stretch moves** past
148+
`crossref-render` — so it is simultaneously the red/green TDD test for the
149+
structural fix and the permanent guardrail. It is format-neutral by construction:
150+
it never mentions `revealjs`, only phases.
151+
152+
---
153+
154+
## The preview-pipeline shape contract (the anti-recurrence rule)
155+
156+
The bug had a deeper enabler worth stating as its own rule.
157+
158+
The preview pipeline (`build_q2_preview_transform_pipeline`, `pipeline.rs:1387`) is
159+
**the render pipeline minus a deny-list** (`Q2_PREVIEW_TRANSFORM_EXCLUDED`,
160+
`:1348`), which removes `"crossref-render"` (`:1374`) so crossref custom nodes
161+
survive for React. That subtraction means a crossref float has **different
162+
concrete AST shapes in the two pipelines** after the crossref phase: a real
163+
`Figure` in native render, a `CustomNode("FloatRefTarget")` in preview. Faced with
164+
that divergence, the original author placed auto-stretch *before* the sugar
165+
transforms — the only point where both pipelines still show a plain `Figure` — and
166+
accepted not stretching crossref figures. That early placement is what created the
167+
ordering inversion.
168+
169+
**Rule:** *removing (or adding) a transform in the preview deny-list must not
170+
create a post-crossref AST-shape divergence that forces a presentation transform
171+
to run earlier in the render pipeline.* If preview and render must differ in
172+
post-crossref shape, the presentation transform must handle **both** shapes (e.g.
173+
recognize the custom-node form), or the preview must be brought back onto the
174+
common shape — never the reverse, where the render pipeline contorts to match
175+
preview's reduced shape. Tracking the preview's own crossref story is bd-zecehtnc.
176+
177+
---
178+
179+
## Worked example: the auto-stretch inversion
180+
181+
| | before (buggy) | after (fixed) |
182+
|---|---|---|
183+
| `reveal-auto-stretch` declared phase | `Finalization` | `Finalization` |
184+
| its position | `pipeline.rs:1148`, **before** `Crossref` (`:1170`) | after `crossref-render` (`:1259`) |
185+
| `![cap](x){#fig-1}` at that point | a plain `Figure` → hoisted to bare `<img>`; never registered as a float | a fully-rendered `Figure(id=fig-1)` with caption "Figure 1: …" → hoisted to `section > img.r-stretch` + `<p class="caption">` |
186+
| ordering test | **fails** (rank 4 before rank 2) | **passes** (ranks non-decreasing) |
187+
| `@fig-1` in output | unresolved `?fig-1?` | `Figure 1` |
188+
189+
---
190+
191+
## References
192+
193+
- Pipeline: `crates/quarto-core/src/pipeline.rs``build_transform_pipeline`
194+
(`:1078`), phase headers (`NORMALIZATION` `:1090`, `CROSSREF` `:1169`,
195+
`NAVIGATION` `:1173`, `FINALIZATION` `:1249`), `crossref-render` (`:1259`),
196+
`Q2_PREVIEW_TRANSFORM_EXCLUDED` (`:1348`), preview builder (`:1387`), existing
197+
analysis-pipeline ordering test (`:1851`), `footer_render_stage` format seam
198+
(`:1070`).
199+
- Trait: `crates/quarto-core/src/transform.rs:69` (`AstTransform`).
200+
- Auto-stretch transform + its ordering rationale: `crates/quarto-core/src/revealjs/auto_stretch.rs`.
201+
- Plan: `claude-notes/plans/2026-06-18-revealjs-crossref.md` (bd-w0c6d38k);
202+
follow-ups bd-4ly7ne01 (bare-table desugar), bd-zecehtnc (preview crossref).
203+
- Crossref design epic: `claude-notes/plans/2026-04-15-crossref-design.md` (bd-jsbg).

0 commit comments

Comments
 (0)