amadan
Plan 7 — images, links and tables
Paul, after pasting a Google Doc and finding the table flattened: "make
a plan to handle images, links, and tables. Images can be rendered
immediately and uploaded async to object storage".
Spec §11.2 already commits to all three, so the scope is not the
question. What the plan works out is the one safety decision two of them
depend on, and the storage seam the app does not have yet.
The decision is D1. fragment_html's output is template.HTML written
unescaped, and it is safe today because no byte from a document reaches
attribute position — marks are attribute-free, align is a key into a
closed table, level is clamped. Tables keep that property, because
colspan and rowspan are integers. Links and images spend it, because
href, src, alt and title are strings from an update that never went
through ProseMirror's schema. So the plan buys it back once and
deliberately — an escaper, a scheme allowlist, a URL that fails it
rendering as text rather than as a dead anchor, and adversarial tests —
before either caller lands. That is why links come before images and why
Task 4 lands with no caller at all.
D4 is the ordering trick images need. The src in the CRDT has to be
stable and app-owned, so it is /d/{sqid}/blobs/{plaintext sha-256},
hashed in the browser before the upload starts. The browser therefore
knows the final URL up front, inserts the node with its real src
immediately, and shows a blob: URL through a NodeView until the upload
lands — no placeholder attribute and no rewrite pass. §11.5 requires
blobs.Sealed, whose address is the CIPHERTEXT's hash, so a small owned
document_blobs table maps one to the other and carries the per-document
authorisation with it.
D6 is the risk that cannot be designed away. A Google Docs paste gives a
googleusercontent URL, not bytes, and fetching it from the browser is
subject to CORS with no canvas fallback (a cross-origin draw taints the
canvas). This slice degrades loudly per §11.2 rather than smuggling in a
server-side fetcher, which is an SSRF surface and its own decision. It
should be tested against a real Doc before Task 7 is built, because the
answer changes the deliverable.
Also recorded: prosemirror-tables rather than a hand-rolled node set,
with column resizing off so colwidth stays null and the wire stays as
simple as it is today; and a coordination note, since discussion #20 is
building DOCX import, which produces these same nodes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AmyRH8GkGkt5ay8gDAUKNm
1 file changed,
+306
−0
docs/superpowers/plans/2026-09-02-images-links-tables.md+306 −0
CI cancelled — superseded by a newer push — run details
Intent
Attributed to this commit: 51.2 g CO₂e (5659557 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on worktree-paste-fidelity, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/docs/superpowers/plans/2026-09-02-images-links-tables.md b/docs/superpowers/plans/2026-09-02-images-links-tables.md| new file mode 100644 |
| index 0000000..a8c69a6 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/2026-09-02-images-links-tables.md |
| @@ -0,0 +1,306 @@ |
| +# Plan 7 — images, links and tables: what a pasted document keeps |
| + |
| +Brief, from Paul, 2026-09-02: *"make a plan to handle images, links, and |
| +tables. Images can be rendered immediately and uploaded async to object |
| +storage"* — after pasting a Google Doc and finding the table flattened |
| +and the formatting gone. |
| + |
| +Spec §11.2 already commits to all three: *"The schema covers headings and |
| +named styles, lists, tables, images, links, headers and footers, page |
| +setup and comments."* So none of this is a new decision about scope. What |
| +it is, is one real decision about safety that two of the three depend on, |
| +and a storage seam the app does not yet have. |
| + |
| +## What is actually broken today |
| + |
| +Verified against `origin/main` by running Google-Docs clipboard HTML |
| +through the shipped schema and the real wazero core. |
| + |
| +**Tables are not in the schema at all.** A pasted table becomes one |
| +top-level paragraph per cell, in row-major order, the grid gone. The |
| +When/What/Who table in Paul's rollout doc came out as six consecutive |
| +paragraphs. `core/src/fragment.rs`, `internal/export/markdown.go` and |
| +`internal/export/pdf.go` know nothing about tables either. |
| + |
| +**Links and images are half-present, which is worse than absent.** |
| +`link` and `image` come from `prosemirror-schema-basic`, so they are in |
| +the *browser* schema. A paste writes them into the CRDT. Then |
| +`fragment.rs` drops them: `link` is not in `MARKS` (`fragment.rs:77-87`), |
| +so the read view renders the anchor text with no anchor, and `image` hits |
| +the unknown-element arm (`fragment.rs:203-204`) and renders as **nothing |
| +at all**. Both exporters are equally blind. This is the module comment's |
| +own "silence here is data loss", happening now. |
| + |
| +## D1 — the safety property, and what it costs to spend it |
| + |
| +This is the decision everything else waits on. |
| + |
| +`fragment_html`'s output is returned as `template.HTML` and written |
| +unescaped by `views/show.html`. It is safe today because of a property |
| +the module states and maintains: **no byte from a document reaches |
| +attribute position**. Marks are attribute-free and render as fixed |
| +literals; the one node attribute that exists, `paragraph.align`, is only |
| +ever a key into a closed table whose right-hand side is a literal |
| +(`fragment.rs`'s `ALIGNMENTS`); `heading.level` is parsed as an integer |
| +and clamped. The `MARKS` comment says plainly what adding a valued mark |
| +would mean: |
| + |
| +> `POST /d/{sqid}/updates` accepts arbitrary client-supplied Yjs bytes |
| +> that never went through ProseMirror's schema, so the value would be |
| +> genuinely arbitrary. Adding one is a real decision about attribute |
| +> escaping and (for `href`) a scheme allowlist, not a row in this table. |
| + |
| +**Tables do not spend this property.** `colspan` and `rowspan` are |
| +integers, clamped exactly as `heading.level` already is. A table can be |
| +added without touching the escaping question at all. |
| + |
| +**Links and images do spend it.** `href`, `src`, `alt` and `title` are |
| +strings chosen by whoever posted the update. So this plan buys the |
| +property back deliberately, once, before either lands: |
| + |
| +1. An `attr_escaped(&str) -> String` in `fragment.rs` escaping `&`, `<`, |
| + `>`, `"` and `'` — in that order, ampersand first. |
| +2. A **scheme allowlist** for `href` and `src`: `http`, `https`, `mailto` |
| + for links; app-relative paths only for images (D4 makes every image |
| + src app-relative, so `src` never carries a scheme). Parsing is |
| + deliberately strict and case-folded, and rejects anything with a |
| + control character or whitespace before the colon — `java\tscript:` is |
| + the classic bypass. |
| +3. **A URL that fails the allowlist renders as text, not as a dead |
| + anchor.** Degrading loudly (§11.2) beats an anchor that looks live and |
| + is not. |
| +4. Tests in `core/tests/fragment.rs` that are adversarial rather than |
| + illustrative: `"><script>`, `javascript:`, `JaVaScRiPt:`, |
| + `java\u{0000}script:`, a scheme-relative `//evil`, and a `title` |
| + containing a quote. |
| + |
| +Doing this once, with its own tests, is the whole reason links come |
| +before images: by the time images need `alt` escaped, the machinery and |
| +the tests exist. |
| + |
| +## D2 — tables are `prosemirror-tables`, not a hand-rolled node set |
| + |
| +`prosemirror-tables` is the same team's package, is what TipTap's table |
| +extension wraps, and brings the parts that are genuinely hard: cell |
| +selection, the column/row commands, and the fixups that keep a table |
| +rectangular after every edit. Writing those by hand is how you get a |
| +document model that can represent a broken grid. |
| + |
| +It contributes `table`, `table_row`, `table_cell`, `table_header`, with |
| +`colspan`, `rowspan` and `colwidth` on cells. |
| + |
| +**Column resizing is off in this slice.** `columnResizing` is what |
| +populates `colwidth`, and `colwidth` is an *array* of numbers or null — |
| +a different shape from every attribute the wire currently carries, and |
| +one `fragment.rs` would have to validate element by element. Leaving |
| +resizing off keeps `colwidth` null, y-prosemirror skips null attributes |
| +entirely, and the wire stays as simple as it is today. Resizing can come |
| +later without a migration, exactly as §11.2 requires of footnotes. |
| + |
| +The vocabulary then has to be added in the four places that cannot import |
| +each other — the same shape of coupling `fixtures/align.mjs` documents, |
| +though a weaker one: these are node **names**, which every consumer |
| +already handles by name, not a closed set of **values** that four tables |
| +must keep in agreement. |
| + |
| +## D3 — a link is a mark with an href, and the read view renders it |
| + |
| +`link` is already in the browser schema and already round-trips through |
| +the CRDT. What is missing is the reader. Once D1 is done this is a row in |
| +`MARKS` — except that `MARKS` is a table of literal open/close strings |
| +and `link` needs to compute its open tag, so the table grows a variant |
| +for a mark that takes an attribute rather than getting a second table. |
| + |
| +Rendered as `<a href="…" rel="noopener noreferrer">`. No `target`: a |
| +document is a page, and opening a new tab is the reader's choice, not the |
| +author's. |
| + |
| +`title` is escaped and passed through. Everything else on the mark is |
| +dropped. |
| + |
| +## D4 — an image renders immediately and uploads in the background |
| + |
| +Paul's shape, and the ordering constraint it creates is the interesting |
| +part. |
| + |
| +**Where the bytes come from is not one case but two.** |
| + |
| +- *A file dropped or pasted from the filesystem* arrives as a `File` on |
| + the clipboard. We have the bytes. |
| +- *An image pasted out of Google Docs* does **not**. Google's clipboard |
| + HTML carries `<img src="https://lh7-…googleusercontent.com/…">` — a |
| + remote, expiring, credentialed URL. We have an address, not an image. |
| + D6 is about that. |
| + |
| +**The src that goes into the CRDT must be app-relative and stable.** Not |
| +a `blob:` URL, which is per-tab and dies with the tab; not a |
| +googleusercontent URL, which expires and leaks the reader's request to |
| +Google. So every image node's `src` is `/d/{sqid}/blobs/{hash}`, where |
| +`hash` is the **SHA-256 of the plaintext bytes**, computed in the browser |
| +with `crypto.subtle.digest` before the upload starts. |
| + |
| +That last part is what makes "render immediately" work without a rewrite |
| +pass. The browser knows the final URL up front, so it inserts the node |
| +with its real `src` straight away and shows the local `blob:` URL through |
| +a NodeView until the upload lands. No placeholder attribute, no second |
| +edit to rewrite, and other tabs converge on the same node. |
| + |
| +**The plaintext hash cannot be the storage key.** Spec §11.5 requires all |
| +blob storage to go through `blobs.Sealed`, and says: *"The address |
| +becomes the ciphertext's hash, so dedup is per key and `Ref.Size` is the |
| +sealed size."* Sealing is not deterministic, so the stored key is not |
| +predictable from the plaintext. A small owned table closes the gap: |
| + |
| + document_blobs(document_id, plain_hash, blob_hash, size, created_at) |
| + |
| +`plain_hash` is what the URL carries and what the CRDT holds; `blob_hash` |
| +is the `blobs.Ref.Hash` that `Store.Get` wants. The row is also what |
| +makes authorisation work: `GET /d/{sqid}/blobs/{hash}` is inside the |
| +existing owner-scoped group in `app.go`, so an image is exactly as |
| +reachable as the document that references it — by construction, not by a |
| +second check somebody has to remember. |
| + |
| +**Failure is not allowed to be silent.** The document holds a `src` the |
| +moment the node is inserted, so a permanently failed upload leaves a |
| +reference to bytes that will never exist. `fixtures/save-queue.mjs` was |
| +written for exactly this class of bug (its F1 comment: a rejected batch |
| +that retried forever and absorbed every later edit), and the image |
| +uploader reuses its retry-then-drop shape. On permanent failure the node |
| +is replaced by its alt text and the person is told which image did not |
| +make it. |
| + |
| +## D5 — what each renderer does with an image |
| + |
| +- **Read view** (`fragment.rs`): `<img src="…" alt="…">`, both escaped, |
| + `src` required to be app-relative. No dimensions in this slice, so the |
| + page reflows as images load; `loading="lazy"` and an aspect-ratio box |
| + are a later refinement, not a correctness question. |
| +- **Markdown** (`markdown.go`): ``, which is lossless within |
| + the subset §11.3 promises. |
| +- **PDF** (`pdf.go`): the real work. JPEG bytes embed directly as |
| + `DCTDecode` and PNG needs decode-and-reflate, which is `image/png` from |
| + the standard library plus a `FlateDecode` stream — no rasterizer, so |
| + §11.4's `internal/raster` is **not** pulled in by this. Scaling is |
| + to the text column width, preserving aspect ratio. |
| +- **A blob the store does not have** renders as the alt text everywhere, |
| + never as a broken image icon. |
| + |
| +## D6 — the Google Docs image, and the risk this plan cannot design away |
| + |
| +Getting bytes for a `googleusercontent.com` src is the part most likely |
| +to fail, and it should be stated rather than discovered. |
| + |
| +- **Fetching it from the browser** is subject to CORS. If Google does not |
| + send permissive headers the fetch fails, and the canvas fallback fails |
| + too — an image drawn from a cross-origin source without CORS taints the |
| + canvas and `toBlob` throws. |
| +- **Fetching it from the server** works and is an SSRF surface: a |
| + document could name `http://169.254.169.254/…` and have the app fetch |
| + it. Doing this safely means a host allowlist, no redirect following to |
| + a private range, a size cap and a timeout. |
| + |
| +**This slice tries the browser fetch and degrades loudly if it fails.** |
| +The image does not come in, and the paste report names it — which is |
| +§11.2's rule, and is honest. Server-side fetching is a separate decision |
| +with its own threat model, and it should not be smuggled in as an |
| +implementation detail of a paste. |
| + |
| +This is the risk to check first with a real Google Doc, because if the |
| +browser fetch does work the whole question goes away, and if it does not |
| +then Paul should decide between "images pasted from Docs do not come in" |
| +and "the app gets a fetcher". |
| + |
| +## D7 — order, and why |
| + |
| +**Tables, then links, then images.** Tables need no new safety property |
| +and no new storage, so they ship first and independently. Links buy the |
| +escaping machinery of D1 and pay for it with adversarial tests. Images |
| +need D1 done, plus a new table, a new route, a new upload path and PDF |
| +work — and they carry D6, the only genuinely open risk here. |
| + |
| +Each is a branch that can merge on its own. |
| + |
| +## The shape of the work |
| + |
| +### Task 1 — tables in the schema and on the wire |
| + |
| +`fixtures/editor-entry.js` (add the four node types from |
| +`prosemirror-tables`, its plugin and its keymap; column resizing off), |
| +`fixtures/package.json`, `fixtures/tables.mjs` if the node specs need |
| +more than a line each, `fixtures/tables.test.mjs`, the gate. |
| + |
| +### Task 2 — tables in the three renderers |
| + |
| +`core/src/fragment.rs`: `node_html` arms for the four names, with |
| +`colspan`/`rowspan` parsed and clamped the way `heading.level` is; |
| +`node_text` treating a row as a line and a cell as a tab-separated field; |
| +`node_json` emitting the numeric attrs. `core/tests/fragment.rs`. |
| +`internal/export/markdown.go`: GFM tables, with the alignment row. |
| +`internal/export/pdf.go`: column widths from the text column, cell |
| +wrapping, and a rule for a row that will not fit on the page. |
| + |
| +### Task 3 — tables on screen |
| + |
| +`internal/docs/views/show.html`: table CSS in the `[docs-text]` |
| +vocabulary, borders that read as a document rather than a spreadsheet, |
| +and `overflow-x: auto` on a wrapper so a wide table scrolls inside the |
| +page rather than making the page scroll. `fixtures/toolbar.mjs`: insert |
| +table, add/remove row and column. WCAG 2.2 AA, and every string a |
| +catalog key. |
| + |
| +### Task 4 — attribute escaping and the scheme allowlist |
| + |
| +`core/src/fragment.rs` only, plus `core/tests/fragment.rs`. Lands with no |
| +caller, deliberately: the machinery and its adversarial tests are |
| +reviewable on their own, and the next two tasks are then small. |
| + |
| +### Task 5 — links reach the reader |
| + |
| +`fragment.rs`'s `MARKS` grows a valued variant; `markdown.go` emits |
| +`[text](href)`; `pdf.go` draws the link text with the document's link |
| +styling and, if the writer supports it, a `/Link` annotation. |
| +`fixtures/toolbar.mjs` gains add/remove link. Spec §6.3's toolbar list |
| +already names link. |
| + |
| +### Task 6 — the blob store and the route |
| + |
| +`internal/docs/models.go` and a migration for `document_blobs`; |
| +`blobs.Sealed` over `blobs.S3FromEnv()` in `cmd/docs/main.go`, falling |
| +back to `blobs.Dir` for local runs; `POST /d/{sqid}/blobs` and |
| +`GET /d/{sqid}/blobs/{hash}` inside the owner-scoped group in `app.go`, |
| +with a size cap and an accepted-type list. The §12 plaintext-grep gate |
| +already covers bucket objects and will cover these. |
| + |
| +### Task 7 — the image, end to end |
| + |
| +`fixtures/image-upload.mjs` (hash, upload, retry-then-drop, reusing |
| +save-queue's shape) and its test; a NodeView in `editor-entry.js` showing |
| +the local preview until the upload lands; `fragment.rs`, `markdown.go` |
| +and `pdf.go` per D5; the paste report for what did not come in. |
| + |
| +### Task 8 — the gate |
| + |
| +Every new JS module gets a `node --test` file in `.amadan/ci.d/10-gate`, |
| +the way `paste.test.mjs` was added on this branch. The wire fixtures are |
| +the contract and must not change (they will not: no existing node's shape |
| +is altered). |
| + |
| +## Risks |
| + |
| +| Risk | Where it bites | What to do | |
| +|---|---|---| |
| +| Google's images are unfetchable from the browser (CORS) | D6, Task 7 | Test it with a real Doc **before** building Task 7. It changes the deliverable. | |
| +| Escaping is got subtly wrong | D1, Task 4 | Adversarial tests, and Fable reviews Task 4 on its own before anything calls it. | |
| +| `prosemirror-tables` bloats the committed bundle | Task 1 | Measure. The bundle is committed and spec §6.5 forbids a build step, so growth is visible in the diff. | |
| +| PDF table layout is open-ended | Task 2 | Cap the first slice: no cell merging across pages, a row that will not fit starts a new page, and say so. | |
| +| A permanently failed upload leaves a dangling src | D4, Task 7 | Retry-then-drop, then replace the node with its alt text and tell the person. | |
| +| Two branches inventing the same vocabulary | All | Discussion #20 (`worktree-import-documents`) is building DOCX/Markdown/PDF **import**, which produces these same nodes. Agree the node names and attrs there before either side writes them. | |
| + |
| +## What this plan does not do |
| + |
| +Headers and footers, page setup, comments, footnotes and tracked changes |
| +— all §11.2 or §14, none of them touched here. Column resizing (D2). |
| +Image dimensions and lazy loading (D5). Server-side image fetching (D6). |
| +Underline, strikethrough, colour and highlight, which are a separate |
| +question about the attribute-free `MARKS` discipline and not about these |
| +three nodes. |