feat(template): implement Go html/template engine#5
Conversation
Implement the TemplateEngine interface using Go's html/template package.
Supports {{ .page.title }} expressions, {{ if }}/{{ else }} conditionals,
{{ range }} loops, {{ block }}/{{ define }} layout inheritance, and raw
HTML content injection via template.HTML conversion.
7/7 GoEngine tests pass. Template package now at 120/120.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zeroedin
left a comment
There was a problem hiding this comment.
Code Review: feat(template): implement Go html/template engine
Clean, minimal implementation. 120/120 template tests pass, integration tests improved (7/16, up from 6/16 on main), go vet clean. The GoEngine is now fully functional for the test suite's expectations.
Summary
| Area | Verdict |
|---|---|
| Parse / Render | ✅ Correct — wraps html/template properly |
AddFilter → FuncMap |
✅ Correctly wires FilterFunc into the template's FuncMap |
AddTag |
|
markHTMLSafe |
|
| Test results | ✅ Verified 120/120 |
| Integration regressions | ✅ None — net +1 pass over main |
Overall this is good to merge. Two items worth addressing — one now (the markHTMLSafe depth issue) and one deferred (AddTag).
See inline comments for details.
- Make markHTMLSafe recursive so nested HTML fields (page.content, page.summary) are converted to template.HTML at any depth - Implement AddTag by wrapping TagFunc in a variadic string function and registering it in FuncMap, matching Go template call syntax - Add doc comment on AddFilter noting parse-time binding constraint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All three review comments addressed in commit 8f55831:
|
Reviewer items: - Use MarkdownOptions.Hooks field instead of separate function (#1) - Tests use []byte(input) matching RenderMarkdown signature (#2) - Heading ID uses slugify: "My Section" → "my-section" (#3) - Protocol-relative URLs intentionally not external for v1 (#4) - Render hooks follow engine choice (Liquid vs Go templates) (#5) - Add blockquote and table tests (#6) Copilot items: - Clarify Alloy registers renderers, not goldmark auto-detecting - Add template tag escaping note for custom hook output structures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ack guards - #1 P1: matchPageGlob handles zero-segment ** match (e.g. /blog/**/index matches /blog/index) - #4 P2: data/pageFields use !== undefined instead of || null to preserve empty arrays - #5 P2: fireContentTransformedHooks writeback guarded by scope.WantsField checks - #6 P2: parseScopeJSON warns on non-string taxonomy term values - #9 P3: computeUnionScope has explicit PagesScopeNone case with comment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Restore IMPLEMENTATION.md from base branch to remove unrelated changes accidentally bundled from feature/plugin-sources-spec (P1 #1, #2, P2 #3) - Batch test: assert per-page uniqueness via data-page attribute with page.path (P2 #4) - Pre-layout test: assert nesting order with combined substring <div class="layout-wrapper"><article class="rewritten"> (P2 #5) - Add html-only return test: sparse {path, html} entries without frontMatter to catch implementations that gate html merge-back on frontMatter presence (CodeRabbit) 6 red tests total (5 original + 1 new). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
P1 #1: Fix all RenderedContent lookups — virtual pages from addPages have non-empty RelPath, so renderedContentKey() returns RelPath (e.g. "_virtual/demos/button.html"), not URL. Changed all 13 assertions from URL keys to RelPath keys. P1 #2: Fix negative assertion for removed virtual pages — was checking NotTo(HaveKey("/demos/card/")) which passes vacuously. Changed to NotTo(HaveKey("_virtual/demos/card.html")). P2 #3: Fix output file test — delete the output file between builds so the post-build-2 read proves build 2 actually wrote it, not just that build 1's file persists on disk. P2 #4: Fix IMPLEMENTATION.md guidance gap — step 2 now explicitly states to add newly identified virtual page RelPaths to renderRelPaths, covering the initial build (nil cache) where step 3 is a no-op. P3 #5: Skipped — markdown: false documents intent matching the rhds plugin pattern; harmless and may be used by future features. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- P1 #1: LoadCache now propagates non-IsNotExist I/O errors instead of swallowing them (matches docstring contract) - P1 #2: Response body capped at 1 MB via io.LimitReader to prevent unbounded memory allocation from malicious/corrupted responses - P2 #3: Version printed once in --check up-to-date case (was printing twice); error/update-available cases print version first then status - P2 #4: CacheDir falls back to os.TempDir() instead of relative path when os.UserHomeDir() fails - P2 #5: SaveCache errors printed as warning to stderr instead of silently discarded - P3 #6: User-Agent header set on GitHub API requests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
P1 #1: Fix duplicate step 19 in PLAN.md pipeline stages — renumber steps 20-25 after inserting onFormatRendered at step 19. Update hook table step numbers (onAssetProcess 22+, onBuildComplete 25). P1 #2: Add content mutation write-back assertion to single non-HTML page test — verifies page.SetRenderedBody path is wired, not just FormatBodies. P2 #3: Fix test count 12 → 14 in IMPLEMENTATION.md (was 13, now 14 with new test). P2 #4: Add test for HTML in non-first position of outputs array (outputs: ["json", "html"]) — catches page.Outputs[0] == "html" implementations. P2 #5: Add second plugin to read-only fields test that verifies frontMatter immutability end-to-end — throws if first plugin's frontMatter mutation was applied back. P3 #6: Add BuildIncremental parity note for onFormatRendered in both PLAN.md and IMPLEMENTATION.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix test count 8→9 in IMPLEMENTATION.md (#1) - Pick single outbound approach: GetOwnPropertyNames + property walk (#2) - Use extractGoMap helper in first test instead of inline logic (#3) - Inline toOrderedMap into extractGoMap (#4) - Extract buildBenchHTML helper to deduplicate benchmark HTML construction (#5) - Add path assertion to HookTransformPayload test (#11) - Add toc outbound verification to HookTransformPayload result (#12) - Add url, path, frontMatter assertions to identity return test (#13) - Fix 800KB benchmark: reduce to 4300 iterations for ~800KB actual size (#14) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- P1 #6: Updated summary table (Phase 5: ~129, cumulative downstream) - P2 #1: Added multi-byte character table entry (CJK, emoji) — validates X-Body-Length is byte count not rune count - P2 #7: Added \r\n suffix to X-Body-Length assertion to prevent decimal-prefix false match (50 matching 500) - P3 #2: Use parsed headers map from parseSplitFrame, assert header value matches actual raw body byte count - P3 #3: Added empty split field test — falls back to single-header - P3 #4: Added dual-key map priority test — html over content - P3 #5: Added caller map immutability guard test 12 total tests (was 8). Test counts updated in IMPLEMENTATION.md (section header, node.go description, impl guidance, summary table). PLAN.md updated with empty-field fallback, dual-key priority, and map immutability constraints. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- P0 #1: Update bridge.js inbound parser to handle split-body frames (X-Body-Length/X-Body-Field headers) and outbound sendMessage to produce split-body frames for hook results with html/content fields - P0 #2-3: Add bounds and negative-value validation on Content-Length and X-Body-Length in both DecodeMessage and Send before slicing or allocating - P1 #4: Require X-Body-Field header when X-Body-Length is present; error on empty field name - P1 #5: Restore stdout pollution detection — first header line in Send must start with "Content-Length:", rejecting "debug: ..." lines - P1 #6: Switch stripField to JSON round-trip (marshal struct → unmarshal to map → delete key) per IMPLEMENTATION.md spec, so new struct fields are automatically included - P1 #7: DecodeMessage split-body injection prefers Result map, falls back to Payload map when Result is nil, per spec - P2 #8: Use len(fieldValue) directly in EncodeMessage instead of allocating []byte(fieldValue) copy for byte-length measurement Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
P2 #1: Hoist setupQuickJSWithHook to outer Describe scope, remove duplicate setupQuickJSWithHookFor1186. Both #1180 and #1186 sections now share the same helper. P2 #2: Extract checkGlobalIsFunction closure — three existence tests are now one-liners calling the shared helper. P2 #3: Add *qjs.Value lifetime note to IMPLEMENTATION.md — stored function references must be Free()d in Close() before context teardown. P2 #4: Add lazy FM test for HookFormatRenderedPayload (onFormatRendered) to verify setPayloadFrontMatter is wired consistently across all three payload types. P2 #9 (addendum): Fix null sentinel collision in __installLazyFM and __installLazyTOC JS snippets — use unique sentinel object instead of null so page.frontMatter = null sticks. Add corresponding test. P3 #5: Add configurable: true test for lazy frontMatter property. P3 #6: Add lazy TOC setter test for parity with frontMatter setter. P3 #7: Add no-globals disambiguation test — verifies invokeHookFastPath uses ctx.Invoke (no __callInput/__callHookName globals) vs ctx.Eval. Test count: 9 → 14 (7 RED, 7 GREEN). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- P1 #1: Fix hook chain misrouting onContentTransformed payloads — check for toc key alongside html to disambiguate payloadTransform vs payloadRendered, preventing silent TOC data loss in multi-hook chains - P1 #2: Log marshal errors in lazy __resolveFM/__resolveTOC callbacks instead of silently returning empty object/undefined - P2 #3: Use NewFloat64 for int values in callHookMapPayload to avoid int32 narrowing truncation on timestamps and large counts - P2 #4: Wire up SetRenderedHTML at 3 hook apply-back call sites in hooks.go (onContentTransformed html/string, onPageRendered html) - P2 #5: Log warning on marshal failure in callHookMapPayload default case instead of silent continue - P2 #6: Document thread-safety assumption on convertedFrontMatter in-place mutation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
TemplateEngineinterface using Go'shtml/templatepackage{{ .page.title }}expressions,{{ if }}/{{ else }}conditionals,{{ range }}loops{{ block }}/{{ define }}layout inheritance with default contenttemplate.HTMLconversion for thecontentfieldAddFilterregisters functions into the template'sFuncMapTest results
Test plan
go test ./internal/template/...— 120/120 passgo vet ./...— clean🤖 Generated with Claude Code