|
| 1 | +# Phase 9 follow-up: thread user_grammars through RenderToHtmlRenderer (bd-izfv) |
| 2 | + |
| 3 | +**Date:** 2026-05-10 |
| 4 | +**Beads:** bd-izfv (P3, open → set to in_progress on `main` once user agrees) |
| 5 | +**Worktree:** `.worktrees/bd-izfv-thread-user-grammars` (branch `beads/bd-izfv-thread-user-grammars`, based on `main` @ `0fa2655d`) |
| 6 | +**Status:** Design questions answered (2026-05-10) — implementation not yet started. **Wait for user go-ahead before starting Phase 0.** |
| 7 | + |
| 8 | +## Triage verdict |
| 9 | + |
| 10 | +**Ready to design.** Bug is reproducible at HEAD via the e2e suite |
| 11 | +(playwright `highlighting/03-user-grammar-toml.qmd [html]` — observed |
| 12 | +`<code>name = "example" count = 42</code>` instead of the expected |
| 13 | +`<pre class="sourceCode toml">…hl-string…hl-number…hl-operator</pre>`). |
| 14 | +The fix site is unambiguous (the explicit `let _ = user_grammars;` at |
| 15 | +`crates/wasm-quarto-hub-client/src/lib.rs:1443` and the |
| 16 | +`RenderContext` construction at |
| 17 | +`crates/quarto-core/src/project/pass2_renderer.rs:336`). The only |
| 18 | +non-trivial decision is the lifetime model for the provider when a |
| 19 | +single `RenderToHtmlRenderer` is used across multiple pages — see |
| 20 | +design questions below. |
| 21 | + |
| 22 | +## Issue context |
| 23 | + |
| 24 | +bd-izfv was filed by Carlos on 2026-04-29 as a P3 follow-up at |
| 25 | +Phase-9 close-out (bd-ayj6). Description verbatim: |
| 26 | + |
| 27 | +> Today render_page_in_project's project-rendering branch drops |
| 28 | +> user_grammars on the floor — the RenderToHtmlRenderer constructs |
| 29 | +> its own per-page RenderContext and there's no path to attach |
| 30 | +> user_grammars there. Single-file projects still wire user_grammars |
| 31 | +> correctly via render_single_doc_to_response. |
| 32 | +> |
| 33 | +> Threading user grammars through the renderer is straightforward: |
| 34 | +> add a field to RenderToHtmlRenderer holding an |
| 35 | +> Arc<Mutex<Option<JsUserGrammars>>> (or similar) and have its |
| 36 | +> render() method install the provider on the per-page RenderContext. |
| 37 | +> |
| 38 | +> Filed as discovered-from-bd-ayj6 and tagged P3 since most |
| 39 | +> user-grammar use is for code-block highlighting which still works |
| 40 | +> on the active page's first render — the gap is just for project |
| 41 | +> renders where the hub-client passes a grammars handle but it gets |
| 42 | +> ignored. |
| 43 | +
|
| 44 | +The "still works on the active page's first render" claim was true |
| 45 | +at filing time but is no longer accurate: the hub-client now |
| 46 | +unconditionally routes through `renderPageInProject` |
| 47 | +(`hub-client/src/services/wasmRenderer.ts:944`), so any document |
| 48 | +with a `_quarto.yml` ancestor — including the highlighting smoke |
| 49 | +fixtures — loses user grammars. |
| 50 | + |
| 51 | +That's why the e2e suite picked it up: this is the only smoke-all |
| 52 | +fixture that needs a user-supplied grammar AND lives inside a |
| 53 | +project directory. |
| 54 | + |
| 55 | +## Dependency graph |
| 56 | + |
| 57 | +``` |
| 58 | +bd-izfv: Phase 9 follow-up: thread user_grammars through RenderToHtmlRenderer |
| 59 | + ├── parent-child → bd-0tr6 (Website projects epic) |
| 60 | + └── discovered-from → bd-ayj6 ([websites phase 9] Hub-client project rendering, closed) |
| 61 | +``` |
| 62 | + |
| 63 | +- **discovered-from bd-ayj6**: filed as a known follow-up while |
| 64 | + closing Phase 9. The `let _ = user_grammars;` block in the |
| 65 | + project-render path is the literal physical evidence — Carlos |
| 66 | + left a comment marking this exact deferral. |
| 67 | +- **parent-child bd-0tr6**: the website-projects epic. This issue |
| 68 | + doesn't block any of its remaining phases, but it does block |
| 69 | + shipping highlighting end-to-end through the hub-client preview. |
| 70 | + Whether that pressure changes the priority is a question for |
| 71 | + the user. |
| 72 | +- No `blocks` edges. The e2e fixture has been failing silently on |
| 73 | + `cargo xtask verify --e2e` since Phase 9 closed — that's the |
| 74 | + *only* user-facing surfacing of the bug today, and `--e2e` is not |
| 75 | + in CI. So there's no production pressure unless we count the |
| 76 | + documentation site (which would render TOML or other unusual |
| 77 | + languages through user grammars in a website project — bd-tr81). |
| 78 | + |
| 79 | +## What the code looks like today |
| 80 | + |
| 81 | +Reproducible at HEAD. Full notes in |
| 82 | +`thread-user-grammars-renderer-investigation/repro-and-codepath.md` |
| 83 | +(committed alongside this plan). The short version: |
| 84 | + |
| 85 | +1. **Single-file path** at |
| 86 | + `crates/wasm-quarto-hub-client/src/lib.rs:1365`: |
| 87 | + ```rust |
| 88 | + if let Some(provider) = user_grammars { |
| 89 | + ctx.user_grammar_provider = Some(Box::new(provider)); |
| 90 | + } |
| 91 | + ``` |
| 92 | + ✅ wired correctly. |
| 93 | + |
| 94 | +2. **Project path** at |
| 95 | + `crates/wasm-quarto-hub-client/src/lib.rs:1443`: |
| 96 | + ```rust |
| 97 | + // Note: `user_grammars` is currently dropped on the orchestrator |
| 98 | + // path … sub-phase 9.4 follow-up — file as bd-XXXX on close-out. |
| 99 | + let _ = user_grammars; |
| 100 | + ``` |
| 101 | + ❌ explicitly dropped. This is bd-izfv. |
| 102 | + |
| 103 | +3. **Pass-2 renderer** at |
| 104 | + `crates/quarto-core/src/project/pass2_renderer.rs:336`: |
| 105 | + ```rust |
| 106 | + let mut ctx = RenderContext::new(project, doc_info, format, &binaries)…; |
| 107 | + ctx.project_index = Some(index); |
| 108 | + ctx.resource_resolver = Some(resolver.clone()); |
| 109 | + ``` |
| 110 | + The fix site is here — needs `ctx.user_grammar_provider = Some(…)`. |
| 111 | + |
| 112 | +4. **Provider type** at `crates/quarto-core/src/render.rs:164`: |
| 113 | + ```rust |
| 114 | + pub user_grammar_provider: Option<Box<dyn quarto_highlight::UserGrammarProvider>>, |
| 115 | + ``` |
| 116 | + The trait object isn't `Clone`. Multi-page rendering through one |
| 117 | + `RenderToHtmlRenderer` requires either consuming the provider on |
| 118 | + first call, or changing the storage to a shared/cloneable shape. |
| 119 | + |
| 120 | +## Proposed phases |
| 121 | + |
| 122 | +Decisions baked in (see § Open design questions): provider stored as |
| 123 | +`Option<Rc<RefCell<dyn UserGrammarProvider>>>`; `JsUserGrammars` |
| 124 | +unchanged (no `Clone` derive); both Rust-side unit test and e2e |
| 125 | +fixture used as regression tests. |
| 126 | + |
| 127 | +- **Phase 0 — Failing tests first (TDD).** Two tests, both expected |
| 128 | + to fail at HEAD before phase 1. |
| 129 | + - **Rust unit:** new file under `crates/quarto-core/tests/` |
| 130 | + (e.g. `render_to_html_user_grammars.rs`) that drives |
| 131 | + `RenderToHtmlRenderer::with_user_grammars(...)` with a stub |
| 132 | + `UserGrammarProvider` returning a known-shape `data-hl-spans` |
| 133 | + JSON for a single class, then asserts the span appears in the |
| 134 | + rendered HTML. Fixture is a tiny in-memory project with a |
| 135 | + `_quarto.yml` and one `.qmd` containing a fenced block in the |
| 136 | + stub class. |
| 137 | + - **E2E:** the existing |
| 138 | + `crates/quarto/tests/smoke-all/highlighting/03-user-grammar/03-user-grammar-toml.qmd` |
| 139 | + fixture already fails the suite — no new fixture needed. |
| 140 | + - Confirm both fail before proceeding. |
| 141 | +- **Phase 1 — Migrate `user_grammar_provider` to `Rc<RefCell<…>>`.** |
| 142 | + - Change field type at `crates/quarto-core/src/render.rs:164`: |
| 143 | + ```rust |
| 144 | + pub user_grammar_provider: Option<Rc<RefCell<dyn quarto_highlight::UserGrammarProvider>>>, |
| 145 | + ``` |
| 146 | + - Update every call site that currently writes |
| 147 | + `Some(Box::new(provider))` — at minimum: |
| 148 | + - `crates/wasm-quarto-hub-client/src/lib.rs:1365` (single-doc path) |
| 149 | + - `crates/wasm-quarto-hub-client/src/lib.rs:1189` (`render_qmd_content`) |
| 150 | + - `crates/wasm-quarto-hub-client/src/lib.rs:1068` (`render_qmd`) |
| 151 | + - any other constructors / tests caught by the compiler. |
| 152 | + - Update every read site that calls |
| 153 | + `provider.contains(...)` / `provider.highlight(...)` to go |
| 154 | + through `borrow()` / `borrow_mut()`. The walker in |
| 155 | + `quarto-highlight` is the main consumer — chase the type |
| 156 | + error from the field rename. |
| 157 | +- **Phase 2 — Wire `RenderToHtmlRenderer`.** |
| 158 | + - Add a field |
| 159 | + `user_grammars: Option<Rc<RefCell<dyn UserGrammarProvider>>>` |
| 160 | + on `RenderToHtmlRenderer` (`crates/quarto-core/src/project/pass2_renderer.rs:279`). |
| 161 | + - Add a builder `with_user_grammars(self, ...) -> Self`. |
| 162 | + - In `render()` (line 336), set |
| 163 | + `ctx.user_grammar_provider = self.user_grammars.clone();` |
| 164 | + after the existing `project_index` / `resource_resolver` lines. |
| 165 | +- **Phase 3 — Update the WASM entry point.** |
| 166 | + - In `render_project_active_page_to_response` |
| 167 | + (`crates/wasm-quarto-hub-client/src/lib.rs:1416`), replace |
| 168 | + `let _ = user_grammars;` with |
| 169 | + ```rust |
| 170 | + let renderer = RenderToHtmlRenderer::new("/.quarto/project-artifacts"); |
| 171 | + let renderer = if let Some(p) = user_grammars { |
| 172 | + renderer.with_user_grammars(Rc::new(RefCell::new(p))) |
| 173 | + } else { |
| 174 | + renderer |
| 175 | + }; |
| 176 | + ``` |
| 177 | + (or inline; whichever reads cleaner). |
| 178 | + - Verify Phase-0 unit test now passes. |
| 179 | +- **Phase 4 — Verify end-to-end through Playwright.** Re-run |
| 180 | + `cargo xtask verify --e2e` and confirm the |
| 181 | + `highlighting/03-user-grammar-toml.qmd` fixture passes. Capture |
| 182 | + the rendered HTML snippet in this plan as the end-to-end- |
| 183 | + verification artifact (per CLAUDE.md § "End-to-end verification |
| 184 | + before declaring success"). |
| 185 | + |
| 186 | + Note: the hub-client e2e workflow (`chore/e2e-ci`, merged into |
| 187 | + `main` shortly before this work began) currently *skips* this |
| 188 | + fixture via `SKIP_WASM_UNSUPPORTED` in |
| 189 | + `hub-client/e2e/helpers/smokeAllDiscovery.ts`. To get an honest |
| 190 | + end-to-end signal, you must remove that skip entry **before** the |
| 191 | + Phase-4 verification run — otherwise Playwright will report |
| 192 | + "skipped" instead of running the assertions. See Phase 5 for the |
| 193 | + exact edit. |
| 194 | + |
| 195 | +- **Phase 5 — Close-out (must include re-enabling the e2e fixture).** |
| 196 | + - **Re-enable the e2e fixture.** In |
| 197 | + `hub-client/e2e/helpers/smokeAllDiscovery.ts`, delete the |
| 198 | + `'highlighting/03-user-grammar/03-user-grammar-toml.qmd'` |
| 199 | + entry from the `SKIP_WASM_UNSUPPORTED` map (added in |
| 200 | + `chore/e2e-ci` commit `ed4b1f02`). The map should be empty |
| 201 | + after removal — that's fine; leave the declaration in place |
| 202 | + so future WASM-gap entries have an obvious slot. If you |
| 203 | + prefer to remove the map entirely, also drop the |
| 204 | + corresponding `SKIP_WASM_UNSUPPORTED.get(relPath)` lookup in |
| 205 | + `shouldSkip()` and revert the `shouldSkip(runConfig, relPath)` |
| 206 | + signature change (call site in |
| 207 | + `hub-client/e2e/smoke-all.spec.ts:42`). |
| 208 | + - Re-run `cargo xtask verify --e2e` after the skip removal to |
| 209 | + confirm `highlighting/03-user-grammar-toml.qmd` now *runs* and |
| 210 | + *passes* (not skipped, not failed). |
| 211 | + - Remove the `let _ = user_grammars;` deferral comment in |
| 212 | + `lib.rs` (it stops being true). |
| 213 | + - `br close bd-izfv --reason "Threaded user_grammars through RenderToHtmlRenderer (Rc<RefCell<…>>); re-enabled e2e fixture by dropping SKIP_WASM_UNSUPPORTED entry"`. |
| 214 | + - `br sync --flush-only && git add .beads/ && git commit` from `main`. |
| 215 | + |
| 216 | +## Design decisions (answered 2026-05-10) |
| 217 | + |
| 218 | +1. **Provider lifetime model — share via interior mutability (B).** |
| 219 | + `RenderContext::user_grammar_provider` becomes |
| 220 | + `Option<Rc<RefCell<dyn UserGrammarProvider>>>`. The pipeline is |
| 221 | + `?Send`, so `Rc` is correct on wasm32 and on native pollster. |
| 222 | + Provider is shared across all pages a renderer touches — |
| 223 | + future multi-page render modes work without a second migration. |
| 224 | + Costs a one-line rewrite at every existing |
| 225 | + `Some(Box::new(provider))` call site (~half-dozen sites + tests). |
| 226 | + |
| 227 | +2. **`JsUserGrammars` `Clone` derive — skip.** Not needed under |
| 228 | + model (B): the `Rc` shares the underlying value without cloning |
| 229 | + `JsUserGrammars` itself. If a later use case needs it, derive |
| 230 | + then. |
| 231 | + |
| 232 | +3. **Test infrastructure — both.** |
| 233 | + - **Rust unit test** under `crates/quarto-core/tests/` driving |
| 234 | + `RenderToHtmlRenderer` directly with a stub |
| 235 | + `UserGrammarProvider` returning canned `data-hl-spans`. Fast, |
| 236 | + deterministic, catches the wiring bug independent of any real |
| 237 | + tree-sitter grammar. Lives in CI's default test suite. |
| 238 | + - **E2E**: the existing |
| 239 | + `highlighting/03-user-grammar-toml.qmd` smoke fixture (already |
| 240 | + failing today via `cargo xtask verify --e2e`) doubles as the |
| 241 | + integration check. Caveat: `--e2e` is not in default CI, so |
| 242 | + the unit test is the load-bearing one for regression |
| 243 | + prevention. |
| 244 | + |
| 245 | +4. **Priority — left as P3 by default.** Open question: does the |
| 246 | + user want it bumped now that the hub-client routes all |
| 247 | + `_quarto.yml`-rooted documents through the project path? Not |
| 248 | + blocking design; can be revisited at implementation time. |
| 249 | + |
| 250 | +## Phase 4 end-to-end verification artifact |
| 251 | + |
| 252 | +Re-ran the e2e suite for the `highlighting/03-user-grammar-toml.qmd` |
| 253 | +fixture from the worktree on `beads/bd-izfv-thread-user-grammars` |
| 254 | +(rebased onto `chore/e2e-ci` to pick up the smoke-all e2e |
| 255 | +infrastructure). After landing both this branch's Rust changes |
| 256 | +*and* the test-harness improvements documented in Phase 5: |
| 257 | + |
| 258 | +Invocation: |
| 259 | + |
| 260 | +```sh |
| 261 | +cd hub-client |
| 262 | +npx playwright test --grep "user-grammar" smoke-all.spec.ts |
| 263 | +``` |
| 264 | + |
| 265 | +Result: `1 passed`. Inspecting the iframe innerHTML inside the |
| 266 | +live preview at assertion time confirms the user grammar is |
| 267 | +applied: |
| 268 | + |
| 269 | +```html |
| 270 | +<pre class="sourceCode toml" data-sid="248" data-loc="0:36:1-40:1"><code><span class="hl-property"><span class="hl-type">name</span> <span class="hl-operator">=</span> <span class="hl-string">"example"</span></span> |
| 271 | +<span class="hl-property"><span class="hl-type">count</span> <span class="hl-operator">=</span> <span class="hl-number">42</span></span></code></pre> |
| 272 | +``` |
| 273 | + |
| 274 | +All four `ensureFileRegexMatches` patterns (`<pre class="sourceCode |
| 275 | +toml"`, `hl-string">"example"</span>`, |
| 276 | +`hl-number">42</span>`, `hl-operator">=</span>`) match. |
| 277 | + |
| 278 | +Two test-harness gaps surfaced during this phase, fixed in the |
| 279 | +same branch: |
| 280 | + |
| 281 | +- `smoke-all.spec.ts` previously read every fixture file via |
| 282 | + `readFileSync(path, 'utf-8')` and forwarded it with |
| 283 | + `contentType: 'text' as const`, which corrupts binary grammar |
| 284 | + `.wasm` payloads before they reach Automerge → VFS. Discovery |
| 285 | + (`smokeAllDiscovery.ts`) now detects binary extensions |
| 286 | + (`.wasm`, common image and font types), reads them as bytes, |
| 287 | + encodes base64, and tags `contentType: 'binary'` with a |
| 288 | + matching `mimeType`. `smoke-all.spec.ts` forwards both fields |
| 289 | + to `createProjectOnServer`. Output was inspected via the |
| 290 | + iframe innerHTML before the fix (no `sourceCode toml` class, |
| 291 | + no `hl-*` spans) and after the fix (the snippet above). |
| 292 | +- `previewExtraction.ts::getPreviewHtml` did a re-render via |
| 293 | + `renderToHtml({ documentPath })` without forwarding a |
| 294 | + `userGrammars` context, so user-grammar fixtures got |
| 295 | + unhighlighted output from the helper even when the live |
| 296 | + preview rendered correctly. The helper now derives a |
| 297 | + `userGrammars` context from VFS state — stripping the |
| 298 | + `/project/` prefix `vfs_list_files` returns to match |
| 299 | + `discoverUserGrammars`'s project-relative shape, and wiring |
| 300 | + binary/text resolvers back through `vfsReadBinaryFile` / |
| 301 | + `vfsReadFile`. |
| 302 | + |
| 303 | +## Risks / tradeoffs |
| 304 | + |
| 305 | +- **Public API ripple.** Changing the type of |
| 306 | + `RenderContext::user_grammar_provider` from `Option<Box<…>>` to |
| 307 | + `Option<Rc<RefCell<…>>>` is a breaking change for any out-of-tree |
| 308 | + consumer of `RenderContext`. There are none today, but the field |
| 309 | + is `pub`. The compiler will catch every internal call site at |
| 310 | + rename time. |
| 311 | +- **No CI coverage for `--e2e`.** Even after the fix, the e2e |
| 312 | + fixture itself won't be caught by default CI — the Phase-0 Rust |
| 313 | + unit test is what guards against regression. Worth a separate |
| 314 | + follow-up beads to either gate a smoke subset of `--e2e` on PR |
| 315 | + builds, or to expand the Rust integration tests further. Out of |
| 316 | + scope for bd-izfv proper. |
| 317 | +- **`Rc<RefCell<…>>` borrow discipline.** The walker that calls |
| 318 | + `provider.contains(...)` / `provider.highlight(...)` must not |
| 319 | + hold a borrow across reentrant calls. The trait's `&mut self` on |
| 320 | + `highlight` already pushes implementations toward short-lived |
| 321 | + borrows, but watch for any code that grabs the provider once and |
| 322 | + iterates — convert to per-call borrow there. |
0 commit comments