feat(pipeline): implement per-instance SSR Phase 2 render model#137
Conversation
BuildPhase2 now scans intermediate HTML for custom elements, deduplicates instances by hash(tag + attributes), pipes each unique instance to the ssr.render command via stdin/stdout, and stamps the SSR'd output back into the page HTML. Pages without custom elements pass through unchanged with no render command invocations. Uses existing ssr.ScanComponents, DeduplicateInstances, InsertMarkers, and StampBack building blocks from the ssr package. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zeroedin
left a comment
There was a problem hiding this comment.
Review: PR #137 — feat(pipeline): implement per-instance SSR Phase 2 render model
Scope: `internal/pipeline/build.go` — 74 added, 13 removed
What changed
`BuildPhase2` rewritten from a single batch `exec.Command` to the per-instance model specified in PR #135:
- Iterate pages: For each page in `intermediateHTML`
- Scan: `ssr.ScanComponents(html)` finds custom element tags
- Skip if empty: No custom elements → page passes through unchanged (no command invoked)
- Deduplicate: `ssr.DeduplicateInstances(instances)` by `hash(tag + attrs)`
- Mark: `ssr.InsertMarkers(html, unique)` wraps instances with `` comments
- Render: `renderViaStdin(parts, rawElement)` pipes each unique instance to the render command, reads SSR'd HTML from stdout
- Stamp back: `ssr.StampBack(markedHTML, ssrResults)` replaces markers with SSR'd output
Two new helper functions:
- `buildRawElement(inst)` — reconstructs `` from `ComponentInstance`, sorted attributes for determinism
- `renderViaStdin(cmdParts, rawHTML)` — `exec.Command` with `cmd.Stdin`, captures stdout/stderr
Resolves the PR #130 architectural gap
The old `BuildPhase2` ran a batch command and returned the input unchanged — the caller wrote stale data to `page.RenderedBody`. Now `BuildPhase2` builds a new `result` map with correctly transformed HTML per page. The caller at `build.go:565-574` copies `finalHTML[page.RelPath]` into `page.RenderedBody`, which now contains the actual SSR'd content.
Analysis
-
Correct: `buildRawElement` excludes slot content (light DOM children) — only `` is piped to the render command. This matches PLAN.md §8: "Slot content lives in the light DOM and is projected by the browser — it does NOT affect the shadow root output."
-
Correct: Attribute sorting (`sort.Strings(keys)`) matches the same sorting in `ssr.computeInstanceHash`, so the element piped to the render command is consistent with the dedup hash.
-
Correct: Error message includes the component tag name: `fmt.Errorf("exec ssr.render command %q for <%s>: %w", parts[0], inst.Tag, err)` — good for debugging which component failed SSR.
-
Correct: The "no custom elements" path returns the original HTML string unchanged (no marker insertion, no command invocation). This is what the new TDD test from PR #135 expects.
Things to note
1. One render process per unique instance: Each `renderViaStdin` call spawns a new `exec.Command`. For ~100 unique instances, that's ~100 process spawns. The spec mentions the `ssr.serve` long-running subprocess as a performance optimization for this — without it, process startup overhead will dominate. Not a correctness issue, but worth tracking.
2. Attribute value escaping in `buildRawElement`: The function uses `fmt.Fprintf(&b, " %s="%s"", k, inst.Attrs[k])`. Attribute values come from `attrRe` which matches `([^"]*)` — so they can't contain double quotes (the regex excludes them). Safe for now, but if attribute parsing ever changes to support single-quoted or unquoted attributes, this would need escaping.
3. Per-page iteration, not global dedup: The function deduplicates instances within each page but not across pages. If `` appears on 100 pages with the same attributes, it gets rendered 100 times (once per page), not once globally. The spec says "Identical instances across all pages SSR once" — so cross-page dedup should be a follow-up. This would be a straightforward lift: collect all instances from all pages first, dedup globally, render unique set once, then stamp back per page.
Verdict
LGTM — the per-instance model is correctly wired. Resolves the stale-return-value gap from PR #130. The cross-page dedup gap is worth a follow-up issue but doesn't block this PR.
Summary
Closes #136. Closes #131.
Rewrites
BuildPhase2from a single batch command invocation to a per-instance stdin/stdout render model, per PLAN.md §6:ssr.ScanComponentshash(tag + attributes)viassr.DeduplicateInstancesssr.InsertMarkersssr.rendercommand viacmd.Stdin, reading SSR'd output fromcmd.Stdoutssr.StampBackNote on SSRConfig.Build → Render rename
The issue requested renaming
SSRConfig.BuildtoSSRConfig.Render. The tests (immutable) useBuild:field name, and the test comment at line 130 explicitly notes "field rename is implementation work tracked in the dev issue, not spec." Left the Go struct field asBuildto avoid breaking tests — error messages reference "ssr.render" instead.Test plan
🤖 Generated with Claude Code