|
6 | 6 | **PR:** [#235](https://github.com/quarto-dev/q2/pull/235) |
7 | 7 | **Branch:** `feature/bd-ky14a-pampa-hash-fileids` (this branch) |
8 | 8 | **Author of this note:** investigation on 2026-06-15 |
| 9 | +**Related in-flight PR:** [#286](https://github.com/quarto-dev/q2/pull/286) |
| 10 | +(block-editing epic) — see §"Entanglement with #286 and the `d === 0` |
| 11 | +assumption" and §"Recommended sequencing". |
| 12 | + |
| 13 | +## TL;DR sequencing recommendation |
| 14 | + |
| 15 | +**Wait for #286 (block-editing epic) to land before merging bd-ky14a.** |
| 16 | +The root entanglement is not a merge conflict — it is a *semantic* |
| 17 | +assumption, "FileId 0 = the document", that #260 wove into the q2-preview |
| 18 | +block-editing source index (`buildSourceIndex` filters `entry.d === 0`; |
| 19 | +lookups pass `d: 0`; the Rust siKey is `"0:{start}-{end}:{file_id}"`). |
| 20 | +bd-ky14a turns `d` from `0` into a hash, which would **silently empty the |
| 21 | +source index** and break block editing / targeted edits in q2-preview. |
| 22 | +#286 is *actively extending* that same `d`-keyed machinery right now, so |
| 23 | +the assumption is a moving target. Land #286 first, let the siKey surface |
| 24 | +settle, then rebase bd-ky14a once and fix the `d`-scheme in one pass. |
| 25 | +(Note: waiting does **not** simplify the hardest *merge conflict* — |
| 26 | +`engine_execution.rs` — which comes from #260, already on `main`.) |
9 | 27 |
|
10 | 28 | ## Why this document exists |
11 | 29 |
|
@@ -150,6 +168,77 @@ Two things still need an explicit audit before declaring this safe: |
150 | 168 | - The dead `FileId(0)` argument and its "single-file document" comment |
151 | 169 | should be cleaned up (or made real) so the next reader isn't misled. |
152 | 170 |
|
| 171 | +> **The `apply_node_edit` literal is the *least* of it.** The serious |
| 172 | +> version of this assumption lives on the **frontend**, in the q2-preview |
| 173 | +> block-editing source index — see the next section. That is the one that |
| 174 | +> silently breaks. |
| 175 | +
|
| 176 | +### 2b. Entanglement with #260 + #286 — the `d === 0` source-index assumption |
| 177 | + |
| 178 | +This is the entanglement that motivates **waiting for #286** (see |
| 179 | +§"Recommended sequencing"). It is broader than `apply_node_edit` and it |
| 180 | +is not a merge conflict — it is a live, working assumption that bd-ky14a |
| 181 | +silently violates. |
| 182 | + |
| 183 | +**Root cause (already on `main`, from #260).** The q2-preview block |
| 184 | +editor builds a source index keyed by a "siKey" string of the form |
| 185 | +`"<t>:<r0>-<r1>:<d>"`, where `d` is the SourceInfo file_id. In |
| 186 | +`ts-packages/preview-renderer/src/q2-preview/sourceIndex.ts`, |
| 187 | +`buildSourceIndex` only indexes blocks whose pool entry has **`d === 0`**: |
| 188 | + |
| 189 | +```ts |
| 190 | +// sourceIndex.ts (origin/main, from #260) |
| 191 | +const entry = pool[Number(s)]; |
| 192 | +if (entry?.t === 0 && entry.d === 0) { // ← hardcoded "primary file is 0" |
| 193 | + const key = serializeSourceEntry(entry); // "0:<r0>-<r1>:0" |
| 194 | + index.set(key, { sourceNode: block, … }); |
| 195 | +} |
| 196 | +``` |
| 197 | + |
| 198 | +Under bd-ky14a, every `Original` block's `d` is `hash(filename) ≠ 0`, so |
| 199 | +**no block satisfies `entry.d === 0` → the source index is empty →** every |
| 200 | +hover/click/edit in q2-preview resolves to nothing. The targeted-edit |
| 201 | +feature (#260) silently stops working. No test in pampa/quarto-core |
| 202 | +catches this; it only shows up in a real q2-preview browser session. |
| 203 | + |
| 204 | +**What #286 adds.** #286 (`feature/block-editing-improvements`, the |
| 205 | +epic-closing PR) is **purely additive on the Rust side** (a new |
| 206 | +`crates/pampa/src/regenerate_nested_buffers.rs` + a WASM binding + tests) |
| 207 | +and does **not** touch the FileId machinery, `apply_node_edit`, |
| 208 | +`node_lookup`, the JSON reader, or `engine_execution`. But it *extends the |
| 209 | +same `d`-keyed siKey scheme* with more consumers that bake in `d = 0`: |
| 210 | + |
| 211 | +- `regenerate_nested_buffers.rs` emits map keys as |
| 212 | + `format!("0:{start}-{end}:{file_id}")` with the comment |
| 213 | + *"d=file_id (always 0 for single-file docs)"*. |
| 214 | +- Its Rust tests hardcode the expected key as `format!("0:{start}-{end}:0")` |
| 215 | + (`regenerate_nested_buffers_tests.rs:69,316`) — these **fail** the moment |
| 216 | + `file_id` becomes a hash. |
| 217 | +- The frontend depth-cursor lookups pass a literal `d: 0` |
| 218 | + (`PreviewRoot.tsx:747`, `useBlockEditHover.tsx:88`: |
| 219 | + `serializeSourceEntry({ t: 0, r: [...], d: 0 })`), so even if the Rust |
| 220 | + map were re-keyed by hash, the frontend would still ask for `:0` and miss. |
| 221 | + |
| 222 | +So #286 does not *create* the entanglement (it's #260's), but it **widens |
| 223 | +the blast radius** and keeps the siKey surface in active flux. |
| 224 | + |
| 225 | +**The clean fix (for the eventual bd-ky14a rebase).** Make the block |
| 226 | +editor scheme-agnostic instead of assuming `0`: |
| 227 | + |
| 228 | +- Drop the `&& entry.d === 0` filter in `buildSourceIndex` — it already has |
| 229 | + `entry.d` in hand; index every Original block under its real `d`. |
| 230 | +- Replace the hardcoded `d: 0` at lookup sites with the real file_id the |
| 231 | + frontend already knows (the block's pool entry `d`). |
| 232 | +- Re-key `regenerate_nested_buffers` by the real `file_id` (it already |
| 233 | + does — the bug is only in the *tests'* hardcoded `:0` and the frontend |
| 234 | + lookups), and update its tests. |
| 235 | + |
| 236 | +This fix is small and well-contained, but it touches `sourceIndex.ts`, |
| 237 | +`PreviewRoot.tsx`, `useBlockEditHover.tsx`, and the |
| 238 | +`regenerate_nested_buffers` tests — **all files #286 is rewriting.** Doing |
| 239 | +it before #286 lands guarantees a conflict and a re-audit. Hence the |
| 240 | +sequencing recommendation. |
| 241 | + |
153 | 242 | ### 3. The 61 snapshot conflicts are NOT mechanical |
154 | 243 |
|
155 | 244 | The PR description frames the snapshot churn as a pure `"d": 0 → "d": <hash>` |
@@ -238,6 +327,30 @@ multi-engine loop and won't exercise it. |
238 | 327 | round-trip on a document whose primary FileId is a hash (not 0), proving |
239 | 328 | the targeted-edit path still locates the destination block. |
240 | 329 |
|
| 330 | +**A new e2e check is required** for §2b: a real q2-preview browser |
| 331 | +session proving the block-editing source index is non-empty and a |
| 332 | +targeted edit resolves when the primary file's `d` is a hash. This is the |
| 333 | +only thing that catches the "empty source index" regression — no Rust or |
| 334 | +vitest unit test does. |
| 335 | + |
| 336 | +## Recommended sequencing |
| 337 | + |
| 338 | +1. **Let #286 land first.** It is `MERGEABLE`, freshly rebased on `main`, |
| 339 | + and a draft. Its Rust side is purely additive; the bulk is q2-preview |
| 340 | + TS. Once it merges, the q2-preview siKey / source-index surface is |
| 341 | + stable and the full set of `d === 0` consumers is visible in one tree. |
| 342 | +2. **Then rebase bd-ky14a onto the post-#286 `main`.** Do the `d`-scheme |
| 343 | + fix from §2b *once*, against settled code, instead of chasing a moving |
| 344 | + target. |
| 345 | +3. What waiting does **not** buy you: the `engine_execution.rs` conflict |
| 346 | + (§1) is from #260/bd-5yff4 and already on `main`; it is unaffected by |
| 347 | + #286 either way. Plan to solve it regardless. |
| 348 | + |
| 349 | +(If for some reason bd-ky14a must go first, the cost is that #286 will |
| 350 | +have to absorb the `d`-scheme change mid-flight — re-keying its siKeys, |
| 351 | +fixing its hardcoded `:0` tests, and re-running its browser e2e. That is |
| 352 | +strictly more total work and lands on the #286 author. Not recommended.) |
| 353 | + |
241 | 354 | ## Suggested merge strategy (for when the go-ahead comes) |
242 | 355 |
|
243 | 356 | 1. Branch a working branch off `feature/bd-ky14a-pampa-hash-fileids` |
|
0 commit comments