amadan
fix: renderer review round 1 — phone reflow resets every template grid
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01563XGCgxcSZToEXwuTBsKE
internal/slides/render/render.go+2 −0internal/slides/render/render_test.go+98 −0internal/slides/static/slides.css+10 −1
Intent
<task-notification> <task-id>a95c2d280e95b7d3d</task-id> <tool-use-id>toolu_01ChHKKmhLkCs2MzfvdS9BSt</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/2420c44b-a27a-4e0d-bbe7-8b3ca0464c21/tasks/a95c2d280e95b7d3d.output</output-file> <status>completed</status> <summary>Agent "Implement Task 4: renderer" 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>Status: done — implemented, tested, `make ci` green, committed. Commit: `85633a0` — `feat: HTML slide renderer, six themes, slides.css` Tests: 19 in `internal/slides/render` (18 goldens across 9 slides × 2 viewports, plus the three named tests and 15 others) — all PASS; `make ci` green. Concerns: - `meta.aspect` not honoured (CSS hard-codes 16/9); `/assets/<id>` is the slice-2 placeholder; nothing links `slides.css` yet (left `layout.html` alone — another agent is in this worktree). - CSS has not been seen in a browser; `image-full`'s scrim and the canvas overflow want a look on first render. - A custom slide background colour flattens both accents to the (recomputed, readable) foreground unless the deck sets `meta.accent`. - `--slides-accent-2` is a sixth variable beyond the brief's five, because `candy` is specified with two accents. Report: `/home/paulca/amadan.net/oficina/slides/.claude/worktrees/slice-1/.superpowers/sdd/2026-09-07-slice-1-live/task-4-report.md`</result> <usage><subagent_tokens>232252</subagent_tokens><tool_uses>73</tool_uses><duration_ms>1564266</duration_ms></usage> </task-notification>
<task-notification> <task-id>a89581dcec8e0ea55</task-id> <tool-use-id>toolu_01PL15aT6PgMwbjNVUmzHgHZ</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/2420c44b-a27a-4e0d-bbe7-8b3ca0464c21/tasks/a89581dcec8e0ea55.output</output-file> <status>completed</status> <summary>Agent "Review Task 4 (spec + quality)" 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>I have enough evidence. Here is the review. ### Spec Compliance All eleven §2.4 block types render in both viewports (verified directly in goldens and via `TestEveryBlockTypeRenders`); all seven flow templates plus canvas and canvas-reflow are covered in `render_test.go`'s `goldenNames` (`internal/slides/render/render_test.go:231-244`), 9 slides × 2 viewports = 18 goldens matching the file list on disk. `meta.aspect` not being honoured, flagged by the implementer as a concern, is **not actually a gap**: spec §9 lists "4:3 and custom aspect" under "Deliberately not built yet," trigger "A person asks; the key already exists" (`docs/superpowers/specs/2026-09-07-slides-design.md:488`). Hard-coding `16/9` is correct for this slice. `TestThemeContrast` (`render_test.go:529-562`) and `TestContrastFunctionItself` (`render_test.go:613-632`) really do compute WCAG 2.2 relative luminance from first principles (not a lookup table), and are cross-checked against four independently-known ratios (black/white 21:1, `#767676`/white ~4.54:1, etc.). I independently recomputed the plum-background/paper-theme case (`section-desktop.golden.html`) in Python and confirmed both `--slides-accent` (2.75:1) and `--slides-accent-2` (2.67:1) genuinely fail 3:1 against `#2b1030` and correctly collapse to the recomputed foreground — the contrast math is sound, not just self-consistent. Escaping checked and verified safe: block text/hrefs (`safeHref` + contextual `html/template`, `render.go:834-846`), alt text (`altFor`, `render.go:608-619`, always emitted, never omitted), theme ids (resolved against a fixed map before use, `render.go:191-196`), language names in `class` (`langPattern` allowlist before the string ever reaches the attribute, `render.go:643-645`). Every attribute value and the whole `style` string independently pass through `template.HTMLEscapeString` in `openTag` (`render.go:862-883`) even where callers have already allowlisted the content — genuine defense in depth. `TestMetaOverridesAreSanitised` and `TestShapeColoursAreSanitised` both probe with realistic injection payloads (`url(...)`, `<script>`, `expression(...)`, path traversal) and I confirmed none survive. The sixth theme variable `--slides-accent-2` is justified: `candy` is specified as needing two accents, and giving every theme one keeps `Slide`'s output uniform. Not a spec violation — an additive, well-documented decision. **One genuine spec violation, not caught by any test:** the `@container slide (max-width: 600px)` block (`internal/slides/static/slides.css:490-562`) never resets `grid-template-columns` or `grid-template-areas` on `[slides-slide][data-template="two-column"]` (set at `slides.css:173-180` to `1fr 1fr` / two explicit columns). It only sets `[slides-region] { grid-area: auto; }` (`slides.css:509-511`). Since the explicit 2-column/2-row track from `two-column`'s rule is never removed, the three regions (heading, body, aside), now placed by grid auto-placement instead of by name, fill that grid row-major: heading→(row1,col1), body→(row1,col2), aside→(row2,col1) — side-by-side and out of reading order, not the single stacked column §2.3 and the CSS's own header comment ("Every template collapses to one column, in document order," `slides.css:508`) promise. Every other template has only one explicit column, so only `two-column` is affected. This is a real, deterministic CSS-grid behavior, not a hypothetical — but nothing in the test suite would catch it since the tests only assert markup, never rendered layout. ### Strengths - SVG self-closing-tag bug (arrow head swallowed as a child in foreign content) and the `--slides-bg` double-emission bug were caught and fixed during the implementer's own golden read-through — evidence of real verification, not just green tests. - The accent-flattening-on-custom-background behavior is a deliberate, correctly implemented, and honestly documented tradeoff (§8's "no exemptions" wins over decorative color), not a bug. - `TestPhoneReflowOfCanvasSlideOrdersByZThenPosition` is a strong test: it constructs a genuine z-then-top-left tie-break case and checks ordering, absence of canvas positioning, and survival of hidden blocks with their attributes, all in one test. - Columns/cycle handling (`done`/`claimed` maps, `render.go:286-306`) is correct and tested for both the shared-child and two-block-cycle cases. - `themes.go` and `render.go` are cleanly separated: palette data and contrast arithmetic in one file, tree-building in the other, with no bleed between them. ### Issues #### Critical None. #### Important - `internal/slides/static/slides.css:490-562` (two-column, phone breakpoint): grid collapse is incomplete for the `two-column` template — see Spec Compliance above. Needs a reset of `grid-template-columns: 1fr` (or equivalent) for `[slides-slide][data-template="two-column"]` inside the `@container` block. Uncaught by the current test suite because it only checks markup, not computed layout; worth a note for whoever adds a browser/visual check later. #### Minor - `internal/slides/render/render.go` is 1136 lines in one file. Not disorganized (clear `── section ──` banners throughout), but a future split (e.g. block renderers vs. element/prop helpers) would ease navigation. - Nested-list indent is only exercised at `indent:1` in the fixture; `indent:2`/`3` and the CSS clamp at `n>3` (`render.go:542-546`) have no direct test, though the clamp logic is simple enough that this is low risk. - `rows` prop on `table` blocks is accepted in `props` but never read (`cols` alone drives layout) — harmless, but worth a one-line comment noting it's intentionally unused (view sends it, renderer infers row count from `cells`/`cols` instead). ### Assessment **Task quality:** Needs fixes **Reasoning:** The renderer itself (Go side: markup construction, escaping, contrast math, phone-layer/canvas/reflow logic, theme resolution) is thorough, well-tested, and I could not find a real spec violation or XSS gap in it — the implementer's self-reported "concerns" turn out to be either non-issues (`meta.aspect`, correctly deferred per §9) or deliberate, documented, correctly-implemented tradeoffs (accent flattening). However, the CSS has one concrete, verifiable violation of an explicit brief requirement — the `two-column` template does not actually collapse to a single column at the phone breakpoint, because the explicit 2-column grid track set outside the container query is never reset inside it. This is a real bug a sighted phone reader would hit, it contradicts the stylesheet's own documented intent, and it slipped through only because the test suite (reasonably, given Go's tooling) checks markup and not computed CSS layout. It's a small, well-scoped fix, but it does need to be fixed before this task is signed off as meeting §2.3/§8's phone-reflow requirement.</result> <usage><subagent_tokens>131539</subagent_tokens><tool_uses>24</tool_uses><duration_ms>226244</duration_ms></usage> </task-notification>
Attributed to this commit: 20.3 g CO₂e (2623682 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on worktree-slice-1, 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/internal/slides/render/render.go b/internal/slides/render/render.go| index 39e74d3..6f5e98a 100644 |
| --- a/internal/slides/render/render.go |
| +++ b/internal/slides/render/render.go |
| @@ -722,6 +722,8 @@ func (c *renderer) shapeBlock(b deck.Block) template.HTML { |
| func (c *renderer) tableBlock(b deck.Block) template.HTML { |
| cells := cellsProp(b.Props) |
| + // A deck may also carry a "rows" prop; it is never read here, because |
| + // row count is derived below from len(cells) and cols. |
| cols := int(numProp(b.Props, "cols", 0)) |
| if cols < 1 { |
| cols = len(cells) |
diff --git a/internal/slides/render/render_test.go b/internal/slides/render/render_test.go| index e097fbb..b9fe9a8 100644 |
| --- a/internal/slides/render/render_test.go |
| +++ b/internal/slides/render/render_test.go |
| @@ -6,6 +6,7 @@ import ( |
| "math" |
| "os" |
| "path/filepath" |
| + "regexp" |
| "strconv" |
| "strings" |
| "testing" |
| @@ -864,3 +865,100 @@ func TestPhoneViewportIsMarked(t *testing.T) { |
| t.Errorf("the phone render is not marked: %s", phone) |
| } |
| } |
| + |
| +// TestPhoneBreakpointCollapsesEveryTemplateToOneColumn guards against a |
| +// regression where the `@container slide (max-width: 600px)` block reset |
| +// `[slides-region] { grid-area: auto }` but left a template's explicit |
| +// `grid-template-columns` in place. `two-column` sets its slide to |
| +// `1fr 1fr`; resetting only the regions' grid-area does not remove that |
| +// track, so the browser auto-places body and aside side by side instead |
| +// of stacking them in document order (spec §2.3). |
| +// |
| +// A Go test cannot lay out CSS grid or run a browser, so this is a |
| +// text/structural assertion against slides.css rather than a rendered |
| +// one: it parses the stylesheet as plain text, finds the phone |
| +// container block, and checks that some rule inside it collapses the |
| +// slide itself (not just its regions) to a single column in a way that |
| +// covers `two-column` — the one template with a multi-track grid. It |
| +// cannot verify the visual result, only that the reset exists; a |
| +// human (or a browser-based check) still has to look at the phone |
| +// breakpoint to be sure grid-template-rows and the rest don't leave a |
| +// stray multi-track behind. |
| +func TestPhoneBreakpointCollapsesEveryTemplateToOneColumn(t *testing.T) { |
| + css, err := os.ReadFile(filepath.Join("..", "static", "slides.css")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + const marker = "@container slide (max-width: 600px) {" |
| + start := strings.Index(string(css), marker) |
| + if start == -1 { |
| + t.Fatal("no phone container block (@container slide (max-width: 600px)) in slides.css") |
| + } |
| + body := string(css)[start+len(marker):] |
| + |
| + // Walk to the matching close brace by counting depth, so a nested |
| + // rule's own "}" doesn't end the block early. |
| + depth := 1 |
| + end := -1 |
| + for i, r := range body { |
| + switch r { |
| + case '{': |
| + depth++ |
| + case '}': |
| + depth-- |
| + if depth == 0 { |
| + end = i |
| + } |
| + } |
| + if end != -1 { |
| + break |
| + } |
| + } |
| + if end == -1 { |
| + t.Fatal("phone container block in slides.css is never closed") |
| + } |
| + block := body[:end] |
| + |
| + // Split the block into its sibling rules (selector { declarations }). |
| + // None of the rules inside this block nest further, so a non-greedy |
| + // match between braces is enough. |
| + rulePattern := regexp.MustCompile(`(?s)([^{}]+)\{([^{}]*)\}`) |
| + rules := rulePattern.FindAllStringSubmatch(block, -1) |
| + if len(rules) == 0 { |
| + t.Fatal("found no rules inside the phone container block") |
| + } |
| + |
| + // A rule "covers" two-column if its selector isn't scoped to a |
| + // different, specific template (an image-full/quote/etc selector |
| + // would not apply to a two-column slide) and either names |
| + // two-column directly or applies to every flow slide generically |
| + // (e.g. via [data-layout="flow"], with no data-template filter). |
| + coversTwoColumn := func(selector string) bool { |
| + if strings.Contains(selector, `data-template="two-column"`) { |
| + return true |
| + } |
| + if strings.Contains(selector, "data-template=") { |
| + return false // scoped to some other, specific template |
| + } |
| + return strings.Contains(selector, "slides-slide") |
| + } |
| + |
| + singleColumn := regexp.MustCompile(`grid-template-columns\s*:\s*(1fr|minmax\(0,\s*1fr\)|none)\s*;`) |
| + areasReset := regexp.MustCompile(`grid-template-areas\s*:\s*none\s*;`) |
| + |
| + found := false |
| + for _, r := range rules { |
| + selector, decls := r[1], r[2] |
| + if !coversTwoColumn(selector) { |
| + continue |
| + } |
| + if singleColumn.MatchString(decls) && areasReset.MatchString(decls) { |
| + found = true |
| + break |
| + } |
| + } |
| + if !found { |
| + t.Errorf("no rule in the phone container block resets both grid-template-columns to a single track and grid-template-areas to none for two-column (or every flow slide); two-column's desktop `1fr 1fr` track survives the breakpoint, so body and aside sit side by side instead of stacking") |
| + } |
| +} |
diff --git a/internal/slides/static/slides.css b/internal/slides/static/slides.css| index 21b3a92..b3748d2 100644 |
| --- a/internal/slides/static/slides.css |
| +++ b/internal/slides/static/slides.css |
| @@ -505,7 +505,16 @@ |
| --slides-gap: max(0.8rem, 4cqw); |
| } |
| - /* Every template collapses to one column, in document order. */ |
| + /* Every template collapses to one column, in document order. Both |
| + halves matter: `grid-area: auto` alone is not enough, because a |
| + template that still carries an explicit multi-column track (only |
| + `two-column` does, at 1fr 1fr) auto-places its items into that |
| + track instead of stacking them — the region rule below undoes the |
| + named placement, this one undoes the columns underneath it. */ |
| + [slides-slide][data-layout="flow"] { |
| + grid-template-columns: 1fr; |
| + grid-template-areas: none; |
| + } |
| [slides-region] { |
| grid-area: auto; |
| } |