amadan
Act on the whole-branch review
Three required fixes, and a claim I made that was not true. A test called TestPageOneDoesNotDisturbPDFExport could not fail. It exported a document, ran PageOne, exported again and compared bytes — but the two calls share no state, so only a package-level global could ever have reddened it. It stayed green while a deliberate cursor shift inside the sink reddened the agreement test. Two comments and the first commit message cited it as what held the sink additive. It never was. Deleted, and the comments now say what actually covers this: the exporter's own suite, and the agreement test for the direction that can drift. The "lists are covered by it instead" pretext is gone too — list geometry is held by nothing, and that is now written down rather than implied away. Transient failures were cached as permanent ones. Store.JSON runs on the request's context, so a viewer closing the tab during a cold render stamped "no thumbnail" onto every document still being laid out, each keeping it until its next edit — with an Error log per document for what is just a disconnect. A cancelled or timed-out request now logs at debug and caches nothing; a corrupt log still caches, because that is a fact about the document. The cache wiped itself when full, which combined with the render budget to leave a large instance permanently cold: wipe, warm forty, overflow, wipe. Every row past the fortieth would have drawn the empty box on every render, for ever. It now evicts one entry instead, and two tests cover the overflow and the re-cache paths. Also from the review: PDF and PageOne now share one layout.document instead of duplicating the first-page setup, so a change to what a first page holds reaches the thumbnail; the dead "failed" flag is gone, as is the double cache lookup; the claim that a thumbnail costs one page of work is corrected to name what is and is not bounded; and the bar comments no longer describe a weight difference no renderer resolves at 0.4px. Two new end-to-end tests, both seen to fail. Editing a document changes its thumbnail — the cache's central claim, previously covered only by unit tests on a map; keying the cache on a constant reddens it. And a thumbnail carries none of the document's own words, which turns the argument the caching rests on into a gate rather than a comment. That second one is the Sheets session's idea, from building the same feature tonight; its first mutation failed for the wrong reason (a 500 from a bad template variable) and was redone as a real leak that renders.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014JM96sbqzZLJgqYmAs4j5y
docs/superpowers/specs/2026-08-30-docs-design.md+1 −1internal/docs/list_test.go+131 −0internal/docs/preview.go+86 −46internal/docs/preview_internal_test.go+40 −3internal/export/pageone.go+25 −31internal/export/pageone_test.go+16 −22internal/export/pdf.go+46 −20
CI passed — run details
Intent
<task-notification> <task-id>a90915f2c2ffc00a0</task-id> <tool-use-id>toolu_01V1Jo3NXbpbPYLaBLFYh8as</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/e1e74ec8-0576-474e-8564-a71365101816/tasks/a90915f2c2ffc00a0.output</output-file> <status>completed</status> <summary>Agent "Fable whole-branch review" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>Review complete. I read the branch as one diff (`origin/main...HEAD`, now three commits — `2f7e80c` landed mid-review and only adds a cost comment to `preview.go`), traced every `stopped`/`sink` path in `pdf.go`, read `ApplyUpdate`/`replay`/`JSON`, `show.html`, `render_test.go`, the spec sections and AGENTS.md, and ran two experiments in a throwaway copy under `/tmp/claude` (nothing in the worktree was touched; `git status` is clean). ## Findings, most serious first **1. Transient errors are cached as permanent failures.** `thumbnail()` (`internal/docs/preview.go`) negative-caches *any* error from `Store.JSON`, keyed to the current `updated_at`. `JSON` runs on `r.Context()`, so a viewer who leaves the page mid-render (cold cache, 40 replays) turns `context.Canceled` into up to 40 documents with no thumbnail until each is next edited — plus an `Error` log line per document for a client disconnect. Same for a deadline or a transient `SQLITE_BUSY`. The comment's "will not fix itself before its updated_at changes" is only true of a corrupt log. Fix: do not `put` when `ctx.Err() != nil` (or `errors.Is(err, context.Canceled/DeadlineExceeded)`); arguably drop the negative cache entirely, since the budget already bounds a broken document to one ~2ms attempt per render. **2. `TestPageOneDoesNotDisturbPDFExport` cannot fail, and two comments plus the commit message lean on it.** It compares `PDF(doc)` before and after a `PageOne(doc)` that shares no state with it; only a global could make it red. Experiment: I inserted `l.cursor += 100` inside the `if l.sink != nil` block — `TestPageOneMatchesTheFirstPageOfThePDF` went red, `DoesNotDisturb` stayed green. This is AGENTS.md's "verification that cannot distinguish no from I can't see it". Commit `748a3a0` says it "holds the sink to changing no PDF byte" — it does not; `pdf.go`'s `sink` field comment says it "is what holds that" — it is not. The sink *is* additive by inspection (`sink` nil and `stopAfterPage` 0 in `PDF()`, so no guard ever fires), and the agreement test is the real guard. Fix: delete the test, rewrite the two comments, and lose the "no-list caveat" pretext that lists are "covered by DoesNotDisturb instead" (they are covered by nothing). **3. The cache bound and the budget combine so a large instance never warms.** `maxPreviews = 512` is global across all owners and drops the whole map on overflow; `thumbnailBudget = 40` caps misses per render. Once the instance's listed documents exceed 512, every render past the wipe restarts cold and everything beyond row 40 draws the empty box — on every render, forever. The comment's "cost of being wrong is one page of layout per document on the next render" is false because the budget caps that render at 40. Fix: evict one random entry on insert (`for k := range p.by { delete(p.by, k); break }` — O(1), bounded, no LRU), or raise the bound to a few thousand (an entry is ~40 bars × 32B ≈ 1.3KB). **4. `PageOne` duplicates `PDF()`'s first-page setup.** Title-as-h1 then root walk is copied, so a change to `PDF()`'s page one (a date line, say) reaches `PageOne` only if it happens to add a text run to the agreement fixture. Fix: one unexported `(l *layout) document(doc Document)` called by both — six lines. Also note `pageone.go`'s comment argues the title *must* be laid ("a page one that omitted it would not be the page one the exporter produces") while its only caller passes `Parse("", …)` to strip it; make the two comments say the same thing (PageOne is page one of a PDF of *this* Document; the preview hands it a title-less one on purpose). **5. "One page of work regardless of length" is true of the layout only.** `Store.JSON` replays and serialises the whole document, `export.Parse` decodes all of it, and `paragraph()` tokenises (measures every word of) the paragraph in progress before it draws a line, so one enormous pasted paragraph is measured in full. Measured on the seeds: JSON 0.5–3ms, layout 0.3–2ms per document — the same order. The third commit's comment gets this right ("grows with a document's update log between snapshots"); commit `748a3a0`, `pageone.go` and the budget comment's opening do not. Fix the wording. **6. Dead state and a double lookup.** `previewEntry.failed` is written (`put`) and never read — `get` returns `(nil, true)` because `bars` is nil, not because it consults `failed`. Delete the field and the parameter (or give it a meaning per finding 1). `thumbnailer` calls `get`, then `thumbnail` calls `get` again on the same key; put the budget inside `thumbnail`. **7. Spec standing and duplication.** AGENTS.md: "A spec, before it becomes a plan… Reviewing it after implementation means reviewing the code's rationalisation of it." This branch amends §11.4, §13 item 9 and adds a §14 row in the same diff as the implementation — exactly that case. The content is honest (it keeps the original ruling, says what shipped, keeps raster as a trigger) and this review is the review it needed, so I'd accept it — but trim the §14 row to one line like every other row, and stop restating the "23pt to the pixel" argument: it appears in eight places (both commit messages, §11.4, §13, §14, `pageone.go`, `preview.go`, `handlers.go`, `list.html`). One home (§11.4), pointers elsewhere. **8. The bar comments over-promise, and one test misapplies WCAG.** At 34px for 792pt a body line box is 0.72px and its bar 0.39px; a heading bar 0.54px. I screenshotted the six seed thumbnails at real 26x34 in chromium: they work — rows survive as light-grey lines, headings as gaps, paragraph ends as ragged edges — but "a heading draws heavier… the real ratio" and "minBarPt keeps the faintest line visible" describe geometry no renderer resolves at this size. `TestAHeadingBarIsTallerThanABodyBar` cites "colour never carries meaning alone" for an `aria-hidden` decoration: WCAG does not apply to it, and 0.15px would not satisfy it if it did. Drop the WCAG sentence; keep the geometry test. **9. No end-to-end freshness test.** Nothing edits a document and asserts its thumbnail changed. The cache is correct today because `ApplyUpdate` inserts and commits the update *before* bumping `updated_at` (I checked: a render racing between the two caches the new content under the old key, and the bump then orphans that key; the reverse order would be a permanent stale) and because the driver is modernc (`sql.Open("sqlite")` in rastrillo `db.go`), which round-trips nanosecond timestamps as text. Both are invariants a ten-line test would pin: seed, GET `/`, count `<rect>`s; `ApplyUpdate` a second update; GET `/`; assert the count changed. The one stale case D4 already accepts (insert committed, bump failed, client never retries) should be named in the new invariant comment. **10. Minor.** List markers never reach the sink, so a list thumbnails as indented lines with no bullets — fine, but say so in `LineBox`'s comment. The `TestPageOneStopsAtTheEndOfTheFirstPage` upper bound of 60 is loose against ~39 actual. ## What I attacked and found sound The `stopped` guards are complete: `block`, `drawLine`, `advance` all return early; `newPage()` from inside `drawLine` sets `stopped` on the second call and `drawLine` returns before capture, draw or cursor move; `list()`'s marker goes to `capturedPage`, which is only ever page one; the quote rule sits after the guard. The title decision matches `show.html` (title lives in `docs-shell-title` in the brand block, never on the paper). Markup: only ints reach the SVG through `html/template`, `viewBox` survives (asserted by the e2e test), the grammar test only forbids `class="rst-`, `aria-hidden` with no role is the correct choice, `--rst-text-muted` is defined in all three rastrillo themes, both row loops share `docRow.Thumb`, and `searchJSON` is the palette endpoint and needs none. No new user-visible strings. Budget 40 is defensible: my measurement is 1.5–4ms per miss (≈60–160ms cold), consistent with the third commit's 1.9ms. ## Verdict **MERGE WITH CHANGES.** Required before merge: findings 1 (don't cache context/transient errors), 2 (delete the vacuous test and the comments and message that cite it), 3 (evict-one instead of drop-all). Strongly recommended: 4, 5, 6, 9. Optional tidy-ups: 7, 8, 10. Sanity check: re-read the ask (single-diff, adversarial, no file edits) — `git status` in the worktree is clean; all experiments ran in `/tmp/claude/docs-bench`, a copy. result: Branch `list-thumbnails` is MERGE WITH CHANGES — three required fixes (transient errors cached as permanent failures, a PDF-unchanged test that cannot fail, cache wipe × budget leaves large instances permanently cold), the sink itself and the markup are sound.</result> <usage><subagent_tokens>151939</subagent_tokens><tool_uses>53</tool_uses><duration_ms>674059</duration_ms></usage> </task-notification>
Attributed to this commit: 63.3 g CO₂e (4829310 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on list-thumbnails, 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/specs/2026-08-30-docs-design.md b/docs/superpowers/specs/2026-08-30-docs-design.md| index aacca1a..7173fb4 100644 |
| --- a/docs/superpowers/specs/2026-08-30-docs-design.md |
| +++ b/docs/superpowers/specs/2026-08-30-docs-design.md |
| @@ -1062,7 +1062,7 @@ amendment. Pagination and PDF still do.)* |
| | Pageless mode | Long-document editing proves painful in page view | |
| | Embedded live Sheets ranges | Both apps stable, and the seam between two collaboration models designed deliberately | |
| | Templates | Somebody has made the same document three times | |
| -| Rasterized thumbnails (§11.4's `internal/raster/`) | A thumbnail is wanted at a size where glyphs are legible. The document list ships a real one already — `export.PageOne` reports where the PDF exporter's own layout put every line of page one, and the row draws those line boxes as SVG. At 26×34 that IS the rendering: a page is ~23pt to the pixel, so a glyph is a fraction of one and rasterizing it would cost a great deal to draw a smudge. What is elided is the glyphs, not the layout | |
| +| Rasterized thumbnails (§11.4's `internal/raster/`) | A thumbnail is wanted at a size where glyphs are legible. The list already draws real page-one line boxes — §11.4 | |
| --- |
diff --git a/internal/docs/list_test.go b/internal/docs/list_test.go| index 4209e7f..41fe248 100644 |
| --- a/internal/docs/list_test.go |
| +++ b/internal/docs/list_test.go |
| @@ -162,6 +162,137 @@ func TestAnEmptyDocumentGetsNoThumbnail(t *testing.T) { |
| } |
| } |
| +// Editing a document changes its thumbnail on the next list render. |
| +// |
| +// This is the cache's central claim and every other test of it is a |
| +// unit test on a map. It holds through the real stack for two reasons |
| +// that are worth pinning here rather than trusting: ApplyUpdate bumps |
| +// updated_at, which is the cache key, so an edit makes the old entry |
| +// unreachable with nobody calling an invalidate; and the sqlite driver |
| +// round-trips that timestamp with enough precision that the two |
| +// versions do not collide. |
| +// |
| +// The one stale case this does not cover, because it is accepted |
| +// rather than prevented: if the update commits and the updated_at bump |
| +// then fails while the process lives, the row has new content under |
| +// its old key. The client's save queue retries, and a restart clears |
| +// the cache regardless. |
| +func TestEditingADocumentChangesItsThumbnail(t *testing.T) { |
| + app, _ := docs.NewForTest(t) |
| + ctx := context.Background() |
| + owner := app.TestPerson(t, "reader@example.com") |
| + doc, err := app.Store().Create(ctx, owner, "Notes") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + cookies := signInCookies(t, app, "reader@example.com") |
| + |
| + rects := func() int { |
| + req := httptest.NewRequest(http.MethodGet, "/", nil) |
| + for _, c := range cookies { |
| + req.AddCookie(c) |
| + } |
| + rec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(rec, req) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("status %d", rec.Code) |
| + } |
| + return strings.Count(rec.Body.String(), "<rect") |
| + } |
| + |
| + before := rects() |
| + |
| + // A real update through the real path, which is what bumps |
| + // updated_at. |
| + if _, err := app.Store().ApplyUpdate(ctx, doc, docs.ReadFragmentFixture(t, "hello")); err != nil { |
| + t.Fatalf("ApplyUpdate: %v", err) |
| + } |
| + |
| + after := rects() |
| + if after == before { |
| + t.Errorf("the list drew %d line boxes before the edit and %d after — the thumbnail did not follow the document", before, after) |
| + } |
| +} |
| + |
| +// A thumbnail must carry no word of the document it depicts. |
| +// |
| +// This is the property the whole cache argument rests on — a preview |
| +// is geometry, so caching one holds no prose in memory and would hold |
| +// none at rest either. That argument was made in a comment and |
| +// believed; this makes it a gate. Suggested by the Sheets session, |
| +// which built the same feature tonight and made the equivalent check |
| +// there. |
| +// |
| +// Docs cannot make the stronger claim Sheets can. Its preview never |
| +// reads a cell at all, so plaintext cannot reach the drawing code; our |
| +// text does enter the function — PageOne measures every word of the |
| +// paragraph it is breaking — and what holds is that only the geometry |
| +// comes back out. That is exactly the kind of claim worth a test |
| +// rather than a comment. |
| +// |
| +// The second assertion matters as much as the first: without it this |
| +// passes by drawing nothing at all. |
| +func TestAThumbnailCarriesNoneOfTheDocumentsWords(t *testing.T) { |
| + app, _ := docs.NewForTest(t) |
| + ctx := context.Background() |
| + owner := app.TestPerson(t, "reader@example.com") |
| + if err := docs.Seed(ctx, app.Store(), owner); err != nil { |
| + t.Fatalf("Seed: %v", err) |
| + } |
| + cookies := signInCookies(t, app, "reader@example.com") |
| + |
| + req := httptest.NewRequest(http.MethodGet, "/", nil) |
| + for _, c := range cookies { |
| + req.AddCookie(c) |
| + } |
| + rec := httptest.NewRecorder() |
| + app.Handler().ServeHTTP(rec, req) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("status %d", rec.Code) |
| + } |
| + body := rec.Body.String() |
| + |
| + if !strings.Contains(body, "<rect") { |
| + t.Fatal("no thumbnail was drawn, so this test would pass without checking anything") |
| + } |
| + |
| + // Words from the seeded documents' bodies. The list shows titles, |
| + // never content, so none of these belongs anywhere on this page — |
| + // and certainly not inside a thumbnail. |
| + for _, canary := range []string{ |
| + "Millbrook", "allotment", "printer", "waterworks", "committee", |
| + } { |
| + if strings.Contains(strings.ToLower(body), strings.ToLower(canary)) { |
| + // Titles legitimately contain some of these, so report only |
| + // what leaked into an <svg>. |
| + for _, svg := range svgFragments(body) { |
| + if strings.Contains(strings.ToLower(svg), strings.ToLower(canary)) { |
| + t.Errorf("a thumbnail carries the document's own word %q", canary) |
| + } |
| + } |
| + } |
| + } |
| +} |
| + |
| +// svgFragments returns every <svg>...</svg> on the page. |
| +func svgFragments(body string) []string { |
| + var out []string |
| + rest := body |
| + for { |
| + i := strings.Index(rest, "<svg") |
| + if i < 0 { |
| + return out |
| + } |
| + rest = rest[i:] |
| + j := strings.Index(rest, "</svg>") |
| + if j < 0 { |
| + return append(out, rest) |
| + } |
| + out = append(out, rest[:j]) |
| + rest = rest[j:] |
| + } |
| +} |
| + |
| func TestEmptyListSaysSo(t *testing.T) { |
| app, _ := docs.NewForTest(t) |
| cookies := signInCookies(t, app, "reader@example.com") |
diff --git a/internal/docs/preview.go b/internal/docs/preview.go| index 597d415..c3c3986 100644 |
| --- a/internal/docs/preview.go |
| +++ b/internal/docs/preview.go |
| @@ -2,6 +2,7 @@ package docs |
| import ( |
| "context" |
| + "errors" |
| "sync" |
| "github.com/carlosframework/docs/internal/export" |
| @@ -44,14 +45,20 @@ type previewBar struct { |
| // in for its lines. |
| // |
| // The height of a bar is derived from the type size rather than from |
| -// the line box, so a heading draws heavier than body text and the |
| -// difference is the real ratio between them. It is deliberately taller |
| -// than the text's own x-height: at this scale a bar of literally |
| -// correct height is a third of a pixel, and the sub-pixel coverage a |
| -// browser resolves that to varies enough between renderers to be worth |
| -// not depending on. minBarPt is the floor that keeps the faintest line |
| -// visible; it is a legibility decision, and the only one here that is |
| -// not simply reported from the layout. |
| +// the line box, so a heading's bar is proportionally taller than a |
| +// body line's, and minBarPt keeps a very small one from collapsing to |
| +// nothing. Both are legibility decisions and the only numbers here not |
| +// reported from the layout. |
| +// |
| +// Be honest about the scale they operate at. 34px for 792pt makes a |
| +// body line box 0.72px and its bar about 0.39px; a heading's is about |
| +// 0.54px. No renderer resolves that difference as "heavier" in any |
| +// literal sense — what actually reaches the eye is a band of sub-pixel |
| +// coverage, and headings read as slightly darker rows within it. It |
| +// works, checked in a browser at 1x and 2x against the seeded |
| +// documents, but the mechanism is antialiasing rather than a drawn |
| +// weight, and a future change here should be judged in a browser |
| +// rather than by the arithmetic. |
| func bars(lines []export.LineBox) []previewBar { |
| const ( |
| barOfSize = 0.66 // of the type size, roughly its cap height |
| @@ -117,38 +124,48 @@ func bars(lines []export.LineBox) []previewBar { |
| // bumps the column, not the struct it was handed. |
| type previews struct { |
| mu sync.Mutex |
| - // by is keyed on Document.ID. Bounded rather than evicted in LRU |
| - // order: at this size the bookkeeping costs more than the misses |
| - // it would save, and a miss is one page of layout. |
| + // by is keyed on Document.ID, and is bounded — see maxPreviews for |
| + // how it is trimmed and why a wipe was the wrong answer. |
| by map[int64]previewEntry |
| } |
| type previewEntry struct { |
| updatedAtNanos int64 |
| - // bars is nil for a document with nothing on its first page, which |
| - // is a real answer and is cached like any other — an empty |
| - // document must not be replayed on every list render. |
| + // bars is nil for a document with nothing on its first page, and |
| + // for one whose structure would not read at this version. Both are |
| + // settled answers meaning "draw nothing", and both are cached: an |
| + // empty document must not be replayed on every list render, and a |
| + // document whose log will not replay would otherwise be retried on |
| + // every page view for as long as it stays broken, turning one bad |
| + // row into a permanent error-log fountain. |
| + // |
| + // There is no separate "failed" flag. There was one, written and |
| + // never read — get answered from bars being nil either way — so it |
| + // recorded a distinction nothing acted on. |
| bars []previewBar |
| - // failed records that this document's structure could not be read |
| - // at this version. Cached deliberately: a document whose log will |
| - // not replay would otherwise be retried on every page view for as |
| - // long as it stays broken, turning one bad row into a permanent |
| - // error-log fountain, and it will not fix itself before its |
| - // updated_at changes. |
| - failed bool |
| } |
| -// maxPreviews bounds the map. Past it the whole map is dropped rather |
| -// than trimmed: this is a cache in front of a bounded amount of work, |
| -// so the cost of being wrong is one page of layout per document on the |
| -// next render, and that is not worth a heap of bookkeeping to avoid. |
| +// maxPreviews bounds the map. |
| +// |
| +// Eviction is one arbitrary entry per insert, not a wipe. A wipe was |
| +// wrong in a way worth recording, because it only misbehaves at scale: |
| +// combined with thumbnailBudget, an instance whose listed documents |
| +// exceed this bound would drop everything, warm at most a budget's |
| +// worth on the next render, overflow, and drop it again — so every row |
| +// past the fortieth would draw the empty box on every render, for |
| +// ever. Evicting one keeps the map full of *something* and lets a |
| +// large list warm over successive renders. |
| +// |
| +// Arbitrary rather than least-recently-used on purpose: Go's map |
| +// iteration gives a usable arbitrary key for free, and at this size |
| +// the difference between evicting the coldest entry and evicting a |
| +// random one is not worth the bookkeeping an LRU needs. |
| const maxPreviews = 512 |
| func newPreviews() *previews { return &previews{by: map[int64]previewEntry{}} } |
| // get returns the cached bars for doc at its current updated_at, and |
| -// whether there was a usable entry. A cached failure returns (nil, |
| -// true): the answer is known and it is "draw nothing". |
| +// whether there was a usable entry. |
| func (p *previews) get(id int64, updatedAtNanos int64) ([]previewBar, bool) { |
| p.mu.Lock() |
| defer p.mu.Unlock() |
| @@ -159,34 +176,59 @@ func (p *previews) get(id int64, updatedAtNanos int64) ([]previewBar, bool) { |
| return e.bars, true |
| } |
| -func (p *previews) put(id int64, updatedAtNanos int64, b []previewBar, failed bool) { |
| +func (p *previews) put(id int64, updatedAtNanos int64, b []previewBar) { |
| p.mu.Lock() |
| defer p.mu.Unlock() |
| - if len(p.by) >= maxPreviews { |
| - p.by = map[int64]previewEntry{} |
| + if _, replacing := p.by[id]; !replacing { |
| + for len(p.by) >= maxPreviews { |
| + for k := range p.by { |
| + delete(p.by, k) |
| + break |
| + } |
| + } |
| } |
| - p.by[id] = previewEntry{updatedAtNanos: updatedAtNanos, bars: b, failed: failed} |
| + p.by[id] = previewEntry{updatedAtNanos: updatedAtNanos, bars: b} |
| } |
| -// thumbnail returns the bars for one document's first page, from the |
| -// cache when it can and by laying the page out when it cannot. |
| +// thumbnail lays out one document's first page and caches the result. |
| +// The caller has already missed the cache; thumbnailer is what checks |
| +// it, so this does not look again. |
| // |
| // It never returns an error. A document whose structure will not read |
| -// gets no thumbnail and the row renders the empty box — the failure is |
| -// logged once per version and cached, because the list is the one page |
| -// that must work, and a single unreplayable document turning it into a |
| -// 500 would be a worse bug than the one it is reporting. |
| +// gets no thumbnail and the row renders the empty box, because the |
| +// list is the one page that must work and a single unreplayable |
| +// document turning it into a 500 would be a worse bug than the one it |
| +// is reporting. |
| +// |
| +// WHICH failures are remembered is the subtle part. A document whose |
| +// log is corrupt is remembered, so it is not retried on every page |
| +// view for as long as it stays broken. A request that was CANCELLED is |
| +// not: Store.JSON runs on the request's own context, so a viewer who |
| +// closes the tab during a cold render — up to a budget's worth of |
| +// replays — would otherwise stamp "no thumbnail" onto every document |
| +// still being laid out, and each would keep it until its next edit. A |
| +// timeout and a busy database are the same shape. Those say nothing |
| +// about the document, so they are logged at a lower level and left |
| +// uncached, and the next render simply tries again. |
| func (h *handlers) thumbnail(ctx context.Context, doc *Document) []previewBar { |
| nanos := doc.UpdatedAt.UnixNano() |
| - if b, ok := h.previews.get(doc.ID, nanos); ok { |
| - return b |
| + |
| + // remember records a settled "draw nothing" for this version. A |
| + // context that is already done means the answer is about the |
| + // request, not the document, so nothing is written down. |
| + remember := func(what string, err error) []previewBar { |
| + if ctx.Err() != nil || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { |
| + h.logger.Debug("docs: list thumbnail abandoned with the request", "doc_id", doc.ID, "stage", what, "err", err) |
| + return nil |
| + } |
| + h.logger.Error("docs: "+what+" for a list thumbnail", "doc_id", doc.ID, "err", err) |
| + h.previews.put(doc.ID, nanos, nil) |
| + return nil |
| } |
| structure, err := h.store.JSON(ctx, doc) |
| if err != nil { |
| - h.logger.Error("docs: read structure for a list thumbnail", "doc_id", doc.ID, "err", err) |
| - h.previews.put(doc.ID, nanos, nil, true) |
| - return nil |
| + return remember("read structure", err) |
| } |
| // Deliberately no title. export.PageOne lays out whatever Title |
| // the Document carries, because the PDF exporter puts one at the |
| @@ -201,13 +243,11 @@ func (h *handlers) thumbnail(ctx context.Context, doc *Document) []previewBar { |
| // have. |
| parsed, err := export.Parse("", structure) |
| if err != nil { |
| - h.logger.Error("docs: parse structure for a list thumbnail", "doc_id", doc.ID, "err", err) |
| - h.previews.put(doc.ID, nanos, nil, true) |
| - return nil |
| + return remember("parse structure", err) |
| } |
| b := bars(export.PageOne(parsed)) |
| - h.previews.put(doc.ID, nanos, b, false) |
| + h.previews.put(doc.ID, nanos, b) |
| return b |
| } |
diff --git a/internal/docs/preview_internal_test.go b/internal/docs/preview_internal_test.go| index 9f91b53..a04708b 100644 |
| --- a/internal/docs/preview_internal_test.go |
| +++ b/internal/docs/preview_internal_test.go |
| @@ -69,7 +69,7 @@ func TestATinyLineStillDrawsSomething(t *testing.T) { |
| func TestAChangedUpdatedAtMissesTheCache(t *testing.T) { |
| p := newPreviews() |
| want := []previewBar{{X: 1, Y: 2, W: 3, H: 4}} |
| - p.put(7, 1000, want, false) |
| + p.put(7, 1000, want) |
| if got, ok := p.get(7, 1000); !ok || len(got) != 1 || got[0] != want[0] { |
| t.Errorf("same version did not hit: got %+v ok=%v", got, ok) |
| @@ -87,7 +87,7 @@ func TestAChangedUpdatedAtMissesTheCache(t *testing.T) { |
| // laid out again on every single list render. |
| func TestACachedFailureIsAHit(t *testing.T) { |
| p := newPreviews() |
| - p.put(7, 1000, nil, true) |
| + p.put(7, 1000, nil) |
| got, ok := p.get(7, 1000) |
| if !ok { |
| t.Fatal("a cached failure missed, so it will be retried on every render") |
| @@ -100,7 +100,7 @@ func TestACachedFailureIsAHit(t *testing.T) { |
| func TestTheCacheIsBounded(t *testing.T) { |
| p := newPreviews() |
| for i := int64(0); i < maxPreviews+10; i++ { |
| - p.put(i, 1, []previewBar{{X: 1}}, false) |
| + p.put(i, 1, []previewBar{{X: 1}}) |
| } |
| p.mu.Lock() |
| n := len(p.by) |
| @@ -109,3 +109,40 @@ func TestTheCacheIsBounded(t *testing.T) { |
| t.Errorf("cache holds %d entries, past its %d bound", n, maxPreviews) |
| } |
| } |
| + |
| +// Overflowing the bound must not empty it. This is the bug an earlier |
| +// version had: the map was wiped when full, and combined with |
| +// thumbnailBudget that left an instance with more documents than the |
| +// bound permanently cold — everything past the budget drew the empty |
| +// box on every render, for ever, because each render wiped what the |
| +// last one warmed. |
| +func TestOverflowingTheCacheKeepsItWarm(t *testing.T) { |
| + p := newPreviews() |
| + for i := int64(0); i < maxPreviews*2; i++ { |
| + p.put(i, 1, []previewBar{{X: 1}}) |
| + } |
| + p.mu.Lock() |
| + n := len(p.by) |
| + p.mu.Unlock() |
| + if n < maxPreviews { |
| + t.Errorf("cache holds only %d entries after overflowing — it was emptied rather than trimmed", n) |
| + } |
| +} |
| + |
| +// Re-caching the same document must not evict anything: a list that |
| +// re-renders the same rows would otherwise churn the map for no reason. |
| +func TestReplacingAnEntryEvictsNothing(t *testing.T) { |
| + p := newPreviews() |
| + for i := int64(0); i < maxPreviews; i++ { |
| + p.put(i, 1, []previewBar{{X: 1}}) |
| + } |
| + for i := int64(0); i < maxPreviews; i++ { |
| + p.put(i, 2, []previewBar{{X: 2}}) |
| + } |
| + p.mu.Lock() |
| + n := len(p.by) |
| + p.mu.Unlock() |
| + if n != maxPreviews { |
| + t.Errorf("cache holds %d entries after re-caching the same documents, want %d", n, maxPreviews) |
| + } |
| +} |
diff --git a/internal/export/pageone.go b/internal/export/pageone.go| index 6987980..8cd6c2b 100644 |
| --- a/internal/export/pageone.go |
| +++ b/internal/export/pageone.go |
| @@ -7,14 +7,8 @@ import "github.com/carlosframework/docs/internal/pdf" |
| // of the page. |
| // |
| // It exists for the document list's thumbnails (internal/docs's |
| -// preview.go). Spec §6.4 and decision 11 ask for "real rendered |
| -// page-one thumbnails", and §11.4 is right that a *raster* one needs a |
| -// pipeline this app does not have — glyph rasterization, image decode, |
| -// PNG composition. What §11.4 did not anticipate is that at 26x34 px a |
| -// page is about twenty-three points to the pixel, so a glyph is a |
| -// fraction of a pixel wide and rasterizing one would render a grey |
| -// smudge at enormous cost. At that size the line boxes ARE the |
| -// rendering, and this reports the real ones. |
| +// preview.go); spec §11.4 is where the argument for drawing line boxes |
| +// rather than rasterizing them lives, and it is not repeated here. |
| // |
| // Real is the whole point, and it is why this is a sink on the |
| // exporter's own layout rather than a second, simpler one beside it. |
| @@ -27,13 +21,22 @@ import "github.com/carlosframework/docs/internal/pdf" |
| // from the first, and drift is exactly what the list would show. |
| // |
| // What is elided, and should be said plainly: the glyphs. This draws |
| -// where the text is, not the text. When the layout engine and |
| -// internal/raster land (spec §13), the honest upgrade is to keep this |
| -// call and rasterize what it reports. |
| +// where the text is, not the text. |
| // |
| -// Cost is one page of layout regardless of the document's length — |
| -// stopAfterPage ends it once the first page is full — which is what |
| -// makes it affordable on a page that renders a row per document. |
| +// This is page one of a PDF of THIS Document, title included — it runs |
| +// the same layout.document PDF does, so whatever the exporter puts on a |
| +// first page is what this reports. A caller that wants the paper |
| +// without the title passes a Document with an empty Title; the document |
| +// list does exactly that, and preview.go says why. |
| +// |
| +// What it bounds is the LAYOUT: stopAfterPage ends that after the first |
| +// page, so a four-hundred-page document costs one page of it. The rest |
| +// of the work is not bounded and it would be wrong to imply otherwise — |
| +// the caller has already replayed and serialised the whole document to |
| +// get here, Parse decodes all of it, and the paragraph in progress is |
| +// measured in full before its first line is drawn. Measured on the seed |
| +// documents, the two halves are the same order: about 0.5-3ms to reach |
| +// the tree and 0.3-2ms to lay the page out. |
| func PageOne(doc Document) []LineBox { |
| var lines []LineBox |
| l := &layout{ |
| @@ -43,23 +46,7 @@ func PageOne(doc Document) []LineBox { |
| sink: func(b LineBox) { lines = append(lines, b) }, |
| } |
| l.newPage() |
| - |
| - // The title is laid out exactly as PDF does it, for the reason PDF |
| - // does it: it is not part of the CRDT, and a page one that omitted |
| - // it would not be the page one the exporter produces. Keep these |
| - // two in step — TestPageOneMatchesTheFirstPageOfThePDF is what |
| - // notices if they stop being. |
| - if doc.Title != "" { |
| - l.block(Node{ |
| - Type: "heading", |
| - Attrs: Attrs{Level: 1}, |
| - Content: []Node{{Type: "text", Text: doc.Title}}, |
| - }, blockContext{scale: 1}) |
| - } |
| - |
| - for _, n := range doc.Root.Content { |
| - l.block(n, blockContext{scale: 1}) |
| - } |
| + l.document(doc) |
| return lines |
| } |
| @@ -67,6 +54,13 @@ func PageOne(doc Document) []LineBox { |
| // the page. It carries no text: a consumer of this is drawing where the |
| // words are, and the words themselves never leave the layout. |
| // |
| +// Lines only. A list's bullets and numbers are drawn beside their |
| +// item's first line rather than as lines of their own, so they never |
| +// reach the sink and a list reports as indented lines with no markers. |
| +// At thumbnail scale a marker would be well under a pixel, so this is |
| +// not worth fixing — but it is worth knowing before someone reads a |
| +// list preview as a run of short paragraphs. |
| +// |
| // That is a property worth keeping rather than an accident. It is what |
| // lets the document list cache a preview in memory without holding any |
| // of the document's prose, and it is why a preview built from these is |
diff --git a/internal/export/pageone_test.go b/internal/export/pageone_test.go| index 370167f..2a99e46 100644 |
| --- a/internal/export/pageone_test.go |
| +++ b/internal/export/pageone_test.go |
| @@ -125,26 +125,19 @@ func TestPageOneReportsAlignment(t *testing.T) { |
| } |
| } |
| -// PageOne must not change what PDF draws. The guard is cheap: the same |
| -// document exported before and after a PageOne run has to produce |
| -// identical bytes. |
| -func TestPageOneDoesNotDisturbPDFExport(t *testing.T) { |
| - js := `{"type":"doc","content":[ |
| - {"type":"heading","attrs":{"level":2},"content":[{"type":"text","text":"Heading"}]}, |
| - {"type":"bullet_list","content":[ |
| - {"type":"list_item","content":[{"type":"paragraph","content":[{"type":"text","text":"An item."}]}]} |
| - ]} |
| - ]}` |
| - doc := mustParse(t, "Notes", js) |
| - |
| - before, _ := mustPDF(t, doc) |
| - _ = export.PageOne(doc) |
| - after, _ := mustPDF(t, doc) |
| - |
| - if string(before) != string(after) { |
| - t.Error("a PageOne run changed what PDF export produces") |
| - } |
| -} |
| +// A test named TestPageOneDoesNotDisturbPDFExport stood here. It |
| +// exported a document, ran PageOne, exported it again and compared the |
| +// bytes — and it could not fail. PDF and PageOne build separate layout |
| +// values sharing no state, so only a package-level global could ever |
| +// have reddened it; a deliberate `l.cursor += 100` inside drawLine's |
| +// sink block left it green while reddening the agreement test below. |
| +// |
| +// Deleted rather than repaired, because a check that cannot |
| +// distinguish "no" from "I can't see it" is worse than no check: two |
| +// comments and a commit message cited it as the thing holding the sink |
| +// additive, which it never was. The exporter's own suite covers its |
| +// output, and TestPageOneMatchesTheFirstPageOfThePDF covers the |
| +// direction that can actually drift. |
| // The test PageOne's own comment promises, and the one that makes "the |
| // same layout, not a second opinion" a mechanism rather than a claim: |
| @@ -160,8 +153,9 @@ func TestPageOneDoesNotDisturbPDFExport(t *testing.T) { |
| // to the LEFT of its item's text (in the list's own indent), so the |
| // leftmost run on that line is the marker rather than the line itself, |
| // and this test's "leftmost run starts where the line starts" would be |
| -// comparing the wrong two numbers. Lists are covered by |
| -// TestPageOneDoesNotDisturbPDFExport instead, which compares bytes. |
| +// comparing the wrong two numbers. That is a real gap, named rather |
| +// than papered over — a list's own geometry is not held by anything |
| +// here, and the pretext that another test covered it was false. |
| func TestPageOneMatchesTheFirstPageOfThePDF(t *testing.T) { |
| js := `{"type":"doc","content":[ |
| {"type":"heading","attrs":{"level":2},"content":[{"type":"text","text":"A heading that is long enough to be interesting"}]}, |
diff --git a/internal/export/pdf.go b/internal/export/pdf.go| index dad8254..53f3d95 100644 |
| --- a/internal/export/pdf.go |
| +++ b/internal/export/pdf.go |
| @@ -271,16 +271,38 @@ func PDF(doc Document) ([]byte, Report, error) { |
| } |
| l.b.Title = doc.Title |
| l.newPage() |
| + l.document(doc) |
| - // The title is set as a level-1 heading at the top of the first |
| - // page. It is not part of the CRDT and so not part of the box tree |
| - // the screen renders — the screen puts it in the shell bar beside |
| - // the brand link, which is chrome around the paper rather than on |
| - // it (it was a page header until the chrome moved to the topbar |
| - // shell; either way it is not on the paper). A PDF has no |
| - // chrome: a file that does not say which document it is, is worse |
| - // than one that occasionally repeats a heading its author also |
| - // typed. Markdown does the same thing for the same reason. |
| + out, err := l.b.Bytes() |
| + if err != nil { |
| + return nil, Report{}, err |
| + } |
| + return out, l.report(), nil |
| +} |
| + |
| +// document lays out a whole Document: its title, then its content. |
| +// |
| +// Shared by PDF and PageOne rather than written out in both, so a |
| +// change to what a first page contains reaches the thumbnail too. The |
| +// two did duplicate it, which meant something added here would have |
| +// reached the thumbnail only if it happened to move a run in the |
| +// agreement test's fixture. |
| +// |
| +// The title is set as a level-1 heading at the top of the first page. |
| +// It is not part of the CRDT and so not part of the box tree the |
| +// screen renders — the screen puts it in the shell bar beside the |
| +// brand link, which is chrome around the paper rather than on it (it |
| +// was a page header until the chrome moved to the topbar shell; either |
| +// way it is not on the paper). A PDF has no chrome: a file that does |
| +// not say which document it is, is worse than one that occasionally |
| +// repeats a heading its author also typed. Markdown does the same |
| +// thing for the same reason. |
| +// |
| +// A caller wanting the paper WITHOUT that chrome — the document list's |
| +// thumbnail does, and preview.go says why — passes a Document with an |
| +// empty Title, which is why this branches rather than always laying |
| +// one. |
| +func (l *layout) document(doc Document) { |
| if doc.Title != "" { |
| l.block(Node{ |
| Type: "heading", |
| @@ -288,16 +310,9 @@ func PDF(doc Document) ([]byte, Report, error) { |
| Content: []Node{{Type: "text", Text: doc.Title}}, |
| }, blockContext{scale: 1}) |
| } |
| - |
| for _, n := range doc.Root.Content { |
| l.block(n, blockContext{scale: 1}) |
| } |
| - |
| - out, err := l.b.Bytes() |
| - if err != nil { |
| - return nil, Report{}, err |
| - } |
| - return out, l.report(), nil |
| } |
| type layout struct { |
| @@ -331,10 +346,21 @@ type layout struct { |
| capturedTop float64 |
| // sink, when set, is told where every line box landed as it is |
| - // placed (pageone.go). PDF export leaves it nil and is byte-for-byte |
| - // unaffected — TestPageOneDoesNotDisturbPDFExport is what holds |
| - // that, because "additive" is a claim about code and that is a |
| - // claim about output. |
| + // placed (pageone.go). PDF export leaves it nil, and leaves |
| + // stopAfterPage 0, so no guard below ever fires and no PDF byte |
| + // moves. |
| + // |
| + // That is a claim by inspection, and it is worth being straight |
| + // about what does and does not hold it. A test that exported the |
| + // same document before and after a PageOne run was written for |
| + // exactly this and then deleted: the two calls share no state, so |
| + // only a package-level global could ever have made it red. It |
| + // passed unchanged while a deliberate `l.cursor += 100` inside |
| + // this block reddened the agreement test — a check that could not |
| + // distinguish "no" from "I can't see it" (AGENTS.md). What covers |
| + // the exporter is its own suite, which measures real output, and |
| + // TestPageOneMatchesTheFirstPageOfThePDF for the reverse |
| + // direction. |
| sink func(LineBox) |
| // pages counts the pages started so far, and stopAfterPage — zero |