feat(plugin): send object payload in onPageRendered with front matter (#1095)#1118
Conversation
Change onPageRendered payload contract from raw HTML string to page object with html, frontMatter, url, path fields. Only html in the return is applied back — other fields are read-only context for conditional processing. 7 failing pipeline tests + 3 registry-level contract tests covering: payload shape validation, conditional transforms via front matter, read-only frontMatter/url/path, correct url and path values, empty frontMatter handling, html apply-back through the pipeline, and incremental rebuild with new callback signature. Refs #1095 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix stale section heading "Per-page hooks (HTML string)" → "Per-page hooks (page object)" in PLAN.md - Replace unchecked type assertion on frontMatter with checked assertion using Expect(fmOk).To(BeTrue()) in crosscutting_test.go - Rename misleading test "page with no custom front matter has empty frontMatter object" → "page with minimal front matter receives frontMatter as an object" in hooks_test.go Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…spec test: onPageRendered object payload with front matter (issue #1095)
…#1095) Change onPageRendered from sending a raw HTML string to sending {html, frontMatter, url, path}. Only the html field in the return is applied back — other fields are read-only context for conditional processing. Updated both Build() and BuildIncremental() paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for alloyssg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthrough
ChangesonPageRendered contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BuildPipeline
participant OnPageRendered
participant RenderedPage
BuildPipeline->>OnPageRendered: Send html, frontMatter, url, path
OnPageRendered-->>BuildPipeline: Return object containing html
BuildPipeline->>RenderedPage: Apply returned html
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/pipeline/build.go (2)
918-921: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLegacy string-return hooks silently no-op with no diagnostic.
Since the
onPageRenderedcontract changed from(html) => stringto(page) => object(issue#1095), a plugin still returning a bare string will failtoGoMapand be silently dropped here — the transform simply stops applying with no log message to explain why. Given this is a documented breaking change, a warning on extraction failure would help plugin authors diagnose the migration.🩹 Proposed diagnostic
for i, result := range results { - if html, ok := extractPageRenderedHTML(result); ok { - pages[i].SetRenderedBody([]byte(html)) + if html, ok := extractPageRenderedHTML(result); ok { + pages[i].SetRenderedBody([]byte(html)) + } else if result != nil { + log.Printf("warning: onPageRendered result for %s did not match the expected page-object shape; ignoring", pages[i].RelPath) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pipeline/build.go` around lines 918 - 921, Update the results loop around extractPageRenderedHTML to emit a warning when extraction fails, including enough context to identify the invalid legacy string-return hook result. Preserve the existing SetRenderedBody behavior when extraction succeeds, and do not alter page processing otherwise.
888-899: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPayload construction duplicated with
incremental.go; consider a shared helper.This exact
html/frontMatter/url/pathmap literal (with the nil-frontMatter guard) is repeated verbatim ininternal/pipeline/incremental.go. SinceextractPageRenderedHTMLwas already factored into a shared helper for the result side, a symmetrical helper for payload construction would prevent the two call sites from drifting when the payload shape changes again.♻️ Proposed helper (in hooks.go)
// buildPageRenderedPayload builds the onPageRendered hook payload for a page. func buildPageRenderedPayload(page *content.Page) map[string]interface{} { fm := page.FrontMatter if fm == nil { fm = map[string]interface{}{} } return map[string]interface{}{ "html": page.HTML(), "frontMatter": fm, "url": page.URL, "path": page.RelPath, } }- for i, page := range pages { - fm := page.FrontMatter - if fm == nil { - fm = map[string]interface{}{} - } - payloads[i] = map[string]interface{}{ - "html": page.HTML(), - "frontMatter": fm, - "url": page.URL, - "path": page.RelPath, - } - } + for i, page := range pages { + payloads[i] = buildPageRenderedPayload(page) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pipeline/build.go` around lines 888 - 899, The payload map construction is duplicated between the build and incremental flows. Add a shared buildPageRenderedPayload helper in hooks.go containing the nil-frontMatter guard and html/frontMatter/url/path fields, then replace both call sites with this helper so payload behavior remains identical and shape changes stay centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plans/IMPLEMENTATION.md`:
- Line 642: The implementation guide must match the current payload contract in
internal pipeline Build. Update the documented onPageRendered instructions
around HookRenderedPayload and the payload construction/apply-back behavior to
reflect the actual map[string]interface{} implementation, or change the
implementation to construct plugin.HookRenderedPayload if that is the intended
contract; keep the guide and Build behavior consistent.
In `@plans/PLAN.md`:
- Line 2472: Clarify the onPageRendered return contract in the hook table to
state whether both the full page object and an object containing only html are
supported; explicitly note that, regardless of shape, only the returned object's
html field is applied back to the rendered page.
- Line 2355: Update hook request construction in the WASM hook dispatch logic
around the code sending requests from internal/plugin/wasm.go so map/object
payloads merge their fields alongside the event key, while primitive payloads
remain wrapped under payload. Preserve the documented event name and ensure
returned payload handling still works for both envelope forms.
---
Nitpick comments:
In `@internal/pipeline/build.go`:
- Around line 918-921: Update the results loop around extractPageRenderedHTML to
emit a warning when extraction fails, including enough context to identify the
invalid legacy string-return hook result. Preserve the existing SetRenderedBody
behavior when extraction succeeds, and do not alter page processing otherwise.
- Around line 888-899: The payload map construction is duplicated between the
build and incremental flows. Add a shared buildPageRenderedPayload helper in
hooks.go containing the nil-frontMatter guard and html/frontMatter/url/path
fields, then replace both call sites with this helper so payload behavior
remains identical and shape changes stay centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2201d2b8-6a73-482c-8459-f7e1b2a2a7bc
📒 Files selected for processing (8)
internal/pipeline/build.gointernal/pipeline/hooks.gointernal/pipeline/hooks_test.gointernal/pipeline/incremental.gointernal/pipeline/incremental_hooks_test.goplans/IMPLEMENTATION.mdplans/PLAN.mdtest/integration/crosscutting_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/page-rendered-payload.md:
- Line 5: Update the onPageRendered hook chaining described in the change note
so frontMatter, url, and path cannot be mutated and propagated between hooks.
Preserve html as the only mutable value applied from hook results, either by
stripping metadata before passing each result onward or deep-freezing the
metadata before dispatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e3c92cd-87c9-46f2-9db0-4034f2f80d3a
📒 Files selected for processing (1)
.changeset/page-rendered-payload.md
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Code Review Results
Scope: internal/pipeline/build.go, hooks.go, incremental.go, hooks_test.go, incremental_hooks_test.go, test/integration/crosscutting_test.go, plans/PLAN.md, plans/IMPLEMENTATION.md, .changeset/page-rendered-payload.md
Intent: Change onPageRendered hook payload from raw HTML string to { html, frontMatter, url, path } object. Only html in the return is applied back. Both Build() and BuildIncremental() paths updated identically.
P1 -- High
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 1 | build.go:890, incremental.go:497 |
Missing convertOrderedMaps() on FrontMatter. fm := page.FrontMatter passes the raw map which can contain *ordered.Map values from JSON data files via cascade. Every other hook that sends FrontMatter (onContentTransformed at hooks.go:283, onPagesReady at hooks.go:486, onContentLoaded at hooks.go:201) calls convertOrderedMaps(). IMPLEMENTATION.md explicitly requires it (issue #463): "ordered.Map.MarshalJSON is expensive (13.7% of CPU in profiling)." Fix: fm := convertOrderedMaps(page.FrontMatter) in both files. |
correctness, performance, maintainability, adversarial | 100 | safe_auto |
| 2 | internal/plugin/payload.go (missing), build.go:894, incremental.go:501 |
Missing HookRenderedPayload typed struct. IMPLEMENTATION.md line 642: "The developer must create a HookRenderedPayload struct in internal/plugin/payload.go with JSON tags html, frontMatter, url, path." Implementation uses raw map[string]interface{} instead. All 6 other hooks use typed structs (HookTransformPayload, HookPagesReadyPayload, etc.). This also eliminates the DRY violation — the same 11-line payload map literal is duplicated verbatim in build.go and incremental.go. Fix: Create the struct, use plugin.HookRenderedPayload{HTML: page.HTML(), FrontMatter: convertOrderedMaps(page.FrontMatter), URL: page.URL, Path: page.RelPath} in both call sites. |
correctness, maintainability | 100 | manual |
P2 -- Moderate
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 3 | hooks.go:516-522 |
Old-format plugins fail silently. A plugin written for the old API returns a string. extractPageRenderedHTML → toGoMap() fails the type assertion → returns ("", false) → SetRenderedBody is skipped. The plugin's transformation is silently dropped with no warning or error. For a documented breaking change, the failure mode should be visible. Consider logging a warning when toGoMap fails (the result is non-nil but not a map), e.g., "onPageRendered: plugin returned %T instead of map — ignoring (old string API was removed in v0.X)". |
adversarial | 90 | manual |
Pre-existing Issues (not blocking)
| # | File | Issue | Reviewer | Confidence |
|---|---|---|---|---|
| 4 | internal/plugin/wasm.go:827-830 |
WASM CallHook always wraps as {event, payload} instead of merging event key into map payloads per spec. PLAN.md says map payloads should get {"event": "onPageRendered", "html": "...", ...} but the code wraps all payloads as {"event": name, "payload": payload}. Pre-existing — affects all map-payload hooks (onContentTransformed, etc.), not specific to this PR. |
adversarial | 85 |
| 5 | incremental.go:509 |
BuildIncremental logs onPageRendered errors as warnings while Build treats them as fatal. Pre-existing divergence, but the new object payload makes malformed returns more likely. |
adversarial | 90 |
Agent-Native Gaps
Docs and example plugins still show the old string API:
/docs/content/hooks/index.md:326-332— old(html) => html.replace(...)signature/docs/content/plugins/index.md:70— old inline example/docs/plugins/shiki.js:25,/docs/plugins/prettify.js:6— live example plugins use old format
Per project rules, docs are separate PRs. Open an issue for the architect to update these before release.
Learnings & Past Solutions
The convertOrderedMaps requirement is a known pattern established in issue #571 (ordered map type preservation through hook serialization). The performance cost was profiled in issue #463 at 13.7% CPU. Every FrontMatter-sending hook follows this pattern — this PR is the only exception.
Coverage
- Testing gaps (for architect — open issue):
extractPageRenderedHTMLedge cases are untested (nil return, missinghtmlkey, non-stringhtml, old string format return).BuildIncrementalobject payload shape is not verified at field level. No test exercises FrontMatter containing*ordered.Mapvalues throughonPageRendered. - Residual risks: FrontMatter reference sharing —
fm := page.FrontMatterpasses a reference, not a copy. Go-registered hooks could mutate page FrontMatter in-place.convertOrderedMapscreates a copy when conversion is needed, but passes through the original reference when no*ordered.Mapvalues exist. Low risk since onPageRendered is post-render. - Suppressed: 0 findings below anchor 75.
Verdict: Not ready
Reasoning: Two P1 findings must be fixed before merge. Finding #1 (
convertOrderedMaps) is a performance regression confirmed by 4 independent reviewers at 100% confidence — the spec explicitly requires it, all other hooks follow the pattern, and profiling data shows 13.7% CPU overhead without it. Finding #2 (missingHookRenderedPayloadstruct) is a direct spec deviation — IMPLEMENTATION.md explicitly says to create it, and the raw map approach blocks sonic JIT optimization while duplicating code across two files. Both fixes are straightforward and follow established patterns already present in the codebase. Fix order: #2 first (struct creation naturally resolves the DRY issue), then #1 (addconvertOrderedMapsinside the struct construction).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review responseAll fixes pushed in cf57ec4. Full test suite passes with P1 #1 — Missing
|
…ndered payload (#1120) - extractPageRenderedHTML edge cases: nil result, missing html key, non-string html, old string format return — all preserve original HTML - BuildIncremental field-level payload verification: validates all 4 fields (html, frontMatter, url, path) across initial and cached rebuilds - Nested YAML front matter with *ordered.Map values through onPageRendered: proves convertOrderedMaps flattens deeply nested maps for plugins - Updated PLAN.md and IMPLEMENTATION.md with edge-case documentation Refs #1120 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Edge-case coverage added (issue #1120)Pushed 6762534 with 6 new tests covering the gaps identified in human review:
|
| Test | Plugin returns | Expected behavior |
|---|---|---|
| nil return | undefined (no return) |
Original HTML preserved, no warning |
| Missing html key | { transformed: true } |
Original HTML preserved, migration warning |
| Non-string html | { html: 42 } |
Original HTML preserved, migration warning |
| Old string format | return page.html + '...' |
Original HTML preserved, migration warning |
BuildIncremental field-level payload (1 test)
Validates all 4 fields (html, frontMatter, url, path) with correct values across both nil-cache and cached incremental rebuilds. Verifies updated front matter is reflected in the payload after content changes.
*ordered.Map front matter (1 test)
3-level nested YAML front matter (metadata.author.social.twitter) proves convertOrderedMaps recursively flattens *ordered.Map values for plugin access.
PLAN.md and IMPLEMENTATION.md updated with edge-case documentation.
Full suite: 404 pipeline + 60 integration = 464 passing with -race.
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Re-review after commits cf57ec4 and 6762534
Previous review was against ffc9a25. Two commits have landed since.
Finding resolution
| # | Original finding | Status |
|---|---|---|
| 1 | Missing convertOrderedMaps() on FrontMatter |
✅ Fixed — buildPageRenderedPayload calls convertOrderedMaps(page.FrontMatter) at hooks.go:516 |
| 2 | Missing HookRenderedPayload typed struct + DRY |
✅ Fixed — Struct added to payload.go:27-32 with correct JSON tags. Shared buildPageRenderedPayload helper eliminates DRY violation. Both build.go:890 and incremental.go:496 use the helper. |
| 3 | Old-format plugins fail silently | ✅ Fixed — Migration warning logged at build.go:912 and incremental.go:506 when result is non-nil but not a map with an html key |
New code quality check
The fix commits are clean:
buildPageRenderedPayload(hooks.go:514-526): Returnsplugin.HookRenderedPayload(typed struct). CallsconvertOrderedMapsbefore nil guard — correct order (convert first, then fall back to empty map if result is nil). Matches the pattern used byonContentTransformed.- Migration warning (
build.go:911-912,incremental.go:505-506): Guards withresult != nilso nil/undefined returns don't trigger false warnings. Warning text includes%Tand mentions "object API" — actionable for plugin authors. HookRenderedPayload(payload.go:24-32): JSON tags match spec (html,frontMatter,url,path). Consistent withHookTransformPayloadstructure.- Edge-case tests (
hooks_test.go,incremental_hooks_test.go): 6 new tests covering nil return, missing html key, non-string html, old string format, nested ordered.Map front matter, and BuildIncremental field-level verification. All gaps flagged in the previous review are now covered. - Spec updates: IMPLEMENTATION.md updated with edge-case handling docs (issue #1120). PLAN.md updated with return value edge cases section and BuildIncremental parity note.
Remaining pre-existing issues (not blocking)
- WASM
CallHookalways wraps as{event, payload}instead of merging — pre-existing, affects all hooks. BuildIncrementallogs hook errors as warnings vsBuildtreating them as fatal — pre-existing divergence.
Verdict: Ready to merge
Reasoning: All three findings from the initial review are resolved. The typed
HookRenderedPayloadstruct matches the spec.convertOrderedMapsis called correctly. Migration warnings surface old-format plugin returns. Edge-case test coverage is comprehensive. Specs are updated. The two pre-existing issues are unrelated to this PR.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/pipeline/hooks_test.go (1)
3009-3106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the migration warning is actually logged.
Each of these three tests (missing
htmlkey, non-stringhtml, old string-format return) documents in its failure message that "the pipeline logs a migration warning," but none of them capture/assert on the actual log output — they only verify HTML preservation. Since logging the migration warning is an explicit implementation goal for this PR (per the PR objectives), consider capturinglogoutput (e.g., redirect vialog.SetOutputto a buffer) and asserting the warning text appears, to close the gap between the documented behavior and what's actually verified.🧪 Example: capturing log output
var buf bytes.Buffer log.SetOutput(&buf) defer log.SetOutput(os.Stderr) result, err := pipeline.BuildWithContent(cfg, contentMap) Expect(err).NotTo(HaveOccurred()) Expect(buf.String()).To(ContainSubstring("onPageRendered"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pipeline/hooks_test.go` around lines 3009 - 3106, Update the three tests for missing HTML, non-string HTML, and old string-format returns to capture logger output around pipeline.BuildWithContent, restore the logger destination afterward, and assert the captured warning contains the onPageRendered migration-warning text. Keep the existing HTML preservation assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/pipeline/hooks_test.go`:
- Around line 3009-3106: Update the three tests for missing HTML, non-string
HTML, and old string-format returns to capture logger output around
pipeline.BuildWithContent, restore the logger destination afterward, and assert
the captured warning contains the onPageRendered migration-warning text. Keep
the existing HTML preservation assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ff7d329-4f55-4a14-8524-0874bc812fba
📒 Files selected for processing (4)
internal/pipeline/hooks_test.gointernal/pipeline/incremental_hooks_test.goplans/IMPLEMENTATION.mdplans/PLAN.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plans/IMPLEMENTATION.md
1 participant
Summary
onPageRenderedhook payload from a raw HTML string to{ html, frontMatter, url, path }— plugins can now conditionally process pages based on front matter without parsing HTML markershtmlfield in the return is applied back;frontMatter,url, andpathare read-only contextBuild()andBuildIncremental()paths identically, using a sharedextractPageRenderedHTMLhelper that handles bothmap[string]interface{}and*ordered.Mapreturn types from the QuickJS bridgeTest plan
onPageRendered object payload with front matter)onPageRendered fires during BuildIncremental)go test -race ./...Closes #1095
🤖 Generated with Claude Code
Summary by CodeRabbit
onPageRenderedplugin hook to receive a per-page object containinghtml,frontMatter,url, andpath(instead of raw HTML).{ html }is applied back (other fields are read-only).BuildandBuildIncremental, including edge-case return behavior.{ html }-only application, and robustness against malformed/legacy hook results.