perf(plugin): lazy frontMatter and TOC serialization in QuickJS fast path#1200
Conversation
…#1187) 14 spec tests under "Lazy frontMatter and TOC serialization (issue #1187)": frontMatter (9 tests): - Lazy accessor detection + self-caching on HookRenderedPayload (RED) - Write-before-read (setter replaces accessor without TypeError) - Never-read (lazy getter inert when unused) - Reading twice returns cached value (self-caching) - Consecutive calls resolve correct frontMatter per call - Nil frontMatter → empty object (not null, not undefined) - Empty frontMatter → empty object - Lazy accessor on HookFormatRenderedPayload (RED) - Lazy accessor on HookTransformPayload (RED) TOC (5 tests): - Lazy accessor detection + self-caching on HookTransformPayload (RED) - Write-before-read (setter replaces accessor without TypeError) - Never-read (lazy getter inert when unused) - Nil TOC → property absent (no lazy getter installed) - Empty TOC → property absent (no lazy getter installed) 4 tests fail RED (accessor property detection), 10 pass as regression guards. Refs #1187 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Hoist setupQuickJSWithHook to parent Describe scope (P2: was duplicated at lines 1635 and 2252, now shared by 31 call sites) - Add TOC "reading twice returns cached array reference" test - Add TOC "consecutive hook calls resolve correct TOC per call" test - Update Phase 5A test count from 93 to 109 (+16 from this PR) - Update Phase 5 total and running total accordingly - Update IMPLEMENTATION.md test coverage description to 16 tests Refs #1187 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
test: spec tests for lazy frontMatter and TOC serialization (#1187)
…path (#1187) Replace eager JSON marshal of frontMatter and TOC on every hook call with lazy getters backed by Go callbacks. The JSON work only runs when the plugin actually reads page.frontMatter or page.toc. Getter self-replaces with a cached data property on first access; setter supports page.frontMatter = {...} without TypeError. 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. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughQuickJS hook payloads now lazily serialize ChangesLazy payload serialization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant HookPayload
participant QuickJSRuntime
participant LazyAccessor
participant Plugin
HookPayload->>QuickJSRuntime: store pending frontMatter or TOC
HookPayload->>LazyAccessor: install lazy property
Plugin->>LazyAccessor: read property
LazyAccessor->>QuickJSRuntime: invoke resolver callback
QuickJSRuntime-->>LazyAccessor: return serialized JSON
LazyAccessor-->>Plugin: cache and return value
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/plugin/wasm.go (1)
531-562: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPending state is never cleared, so a retained payload resolves another page's frontMatter/TOC.
Resolution is deferred to property-read time, but
pendingFM/pendingTOCkeep the previous call's values afterinvokeHookFastPathreturns. A plugin that stashes the payload (module-scope var, closure, deferred callback) and readspage.frontMatterduring a later hook call gets the current page's data instead of its own — silent cross-page contamination. IMPLEMENTATION.md step 6 specifies clearing this state; that step is missing here.🛠️ Proposed fix
if len(v.TOC) > 0 { r.pendingTOC = v.TOC installFn := r.ctx.Global().GetPropertyStr("__installLazyTOC") defer installFn.Free() if _, err := r.ctx.Invoke(installFn, r.ctx.Global(), obj); err != nil { return nil, fmt.Errorf("hook %q: installing lazy toc getter: %w", name, err) } } + defer func() { + r.pendingFM = nil + r.pendingTOC = nil + }() return r.invokeHookFastPath(name, obj) }Clearing turns the stale-read into an empty object / empty array rather than another page's data. If a hard guarantee is wanted, capture a per-call generation counter in the installer and have the resolvers reject mismatched generations.
🤖 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/plugin/wasm.go` around lines 531 - 562, Update callHookTransformPayload to clear pendingFM and pendingTOC after invokeHookFastPath completes, including when the hook returns an error, so retained payloads cannot resolve another page’s data. Preserve the existing setup and return behavior while ensuring cleanup runs for every call.
🧹 Nitpick comments (1)
internal/plugin/wasm.go (1)
32-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider typing
pendingTOCas[]content.TOCEntry.
interface{}loses type safety and lets a typed-nil slice defeat thetoc == nilcheck in__resolveTOC. The plan (IMPLEMENTATION.md step 1) specifies the concrete slice type.🤖 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/plugin/wasm.go` around lines 32 - 33, Change pendingTOC to []content.TOCEntry and update the related __resolveTOC handling to use that concrete type, preserving nil checks so a nil TOC is recognized correctly. Follow the concrete slice type specified in IMPLEMENTATION.md step 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 `@internal/plugin/wasm.go`:
- Around line 145-167: Update the Marshal error branches in the __resolveFM and
__resolveTOC callbacks to return the encountered error instead of an empty JSON
fallback, allowing it to surface as a JavaScript exception and propagate through
CallHook while preserving the existing nil-data defaults.
- Around line 171-198: Update both __installLazyFM and __installLazyTOC so their
accessor descriptors explicitly set enumerable to true, and add enumerable: true
to each self-replacing data-property descriptor in the getter and setter paths.
Preserve the existing lazy resolution and replacement behavior while ensuring
frontMatter and toc remain visible to Object.keys, spread, and JSON.stringify.
In `@plans/IMPLEMENTATION.md`:
- Around line 730-735: Align the implementation and plan for pending TOC state:
update the QuickJSRuntime.pendingTOC declaration to use []content.TOCEntry and
clear pendingFM and pendingTOC immediately after invokeHookFastPath returns, or
revise the plan to match the existing behavior. Keep the pending state scoped to
each hook invocation and avoid retaining stale references.
---
Outside diff comments:
In `@internal/plugin/wasm.go`:
- Around line 531-562: Update callHookTransformPayload to clear pendingFM and
pendingTOC after invokeHookFastPath completes, including when the hook returns
an error, so retained payloads cannot resolve another page’s data. Preserve the
existing setup and return behavior while ensuring cleanup runs for every call.
---
Nitpick comments:
In `@internal/plugin/wasm.go`:
- Around line 32-33: Change pendingTOC to []content.TOCEntry and update the
related __resolveTOC handling to use that concrete type, preserving nil checks
so a nil TOC is recognized correctly. Follow the concrete slice type specified
in IMPLEMENTATION.md step 1.
🪄 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: 706437af-d20b-46d4-ae21-64c44b97463f
📒 Files selected for processing (4)
internal/plugin/wasm.gointernal/plugin/wasm_test.goplans/IMPLEMENTATION.mdplans/PLAN.md
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Code Review Results
Scope: internal/plugin/wasm.go (implementation), internal/plugin/wasm_test.go (test helper hoist + 16 new spec tests), plans/PLAN.md, plans/IMPLEMENTATION.md
Intent: Replace eager JSON serialization of frontMatter and TOC on every hook call with lazy Object.defineProperty getters backed by Go callbacks, deferring the JSON work to when plugins actually read the properties.
P1 — High
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 1 | wasm.go:511,525,547 |
Missing pendingFM/pendingTOC cleanup after invokeHookFastPath — IMPLEMENTATION.md line 735 step 6 explicitly requires: "After invokeHookFastPath returns, set r.pendingFM = nil and r.pendingTOC = nil to avoid stale references." None of the three fast-path callers clear these fields. Not a data-corruption bug (each call overwrites before JS runs, mutex is held), but a spec deviation that retains Go heap objects after the last hook call and leaves __resolveFM()/__resolveTOC() (global JS functions) returning stale data if called outside a getter context. Fix: add r.pendingFM = nil and r.pendingTOC = nil to the existing defer block in invokeHookFastPath at line 570. |
correctness, simplicity, maintainability, standards | 95 | Fix before merge |
P2 — Moderate
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 2 | wasm.go:151-153,163-165 |
Silent error swallowing in __resolveFM/__resolveTOC callbacks — When jsonCodec.Marshal fails, these return empty JSON ("{}" / "[]") with nil error. Pre-PR, setPayloadFrontMatter propagated the error: return fmt.Errorf("marshaling frontMatter: %w", err). The lazy path silently degrades. Practically unlikely (YAML-parsed maps always marshal), but makes marshal failures invisible. Add log.Printf("warning: lazy frontMatter marshal failed: %v", err) before the fallback return on both error branches. |
correctness, simplicity, maintainability | 70 | Fix before merge |
| 3 | wasm.go:33 |
pendingTOC typed as interface{} instead of spec's []content.TOCEntry — IMPLEMENTATION.md line 730 specifies the concrete type. Using interface{} loses compile-time type safety and is inconsistent with pendingFM (which uses its concrete type). Importing the content package adds one import line. |
standards, maintainability | 75 | Fix before merge |
P3 — Advisory
| # | File | Issue | Reviewer | Confidence | Route |
|---|---|---|---|---|---|
| 4 | wasm.go:541,555 |
Global property lookup per hook call for installer functions — GetPropertyStr("__installLazyFM") crosses the WASM boundary on every fast-path hook call (~2460 times on 820 pages × 3 hooks). The function references are constant after Init(). Caching them as struct fields would eliminate these lookups, but adds lifecycle management in Close(). Estimated savings: 2-12ms per site build — negligible vs the 3.2s saved by the lazy optimization itself. Author's call. |
performance | 75 | Optional |
| 5 | wasm.go:543,557 |
ctx.Invoke return value discarded without Free() — normalizeJsValue (qjs eval.go:72) returns the raw *Value on success. The discarded values are undefined (no explicit JS return), which are immediate values in QuickJS — Free() is a no-op on them. Not a real leak, but explicit Free() would match the codebase convention. |
correctness | 50 | Optional |
Coverage
- Testing gaps: No test verifies that
pendingFM/pendingTOCare nilled after hook calls (the "consecutive calls" tests pass because the fields are overwritten, not because they were cleared). No test covers thejsonCodec.Marshalfailure path. Both would be architect issues if needed. - Residual risks:
__resolveFM()and__resolveTOC()are global JS functions — a plugin could call them directly outside a hook getter. With cleanup (finding #1 fixed), they'd return safe fallback values. Without cleanup, they'd return stale data from the previous call. - Benchmark gap: Existing benchmarks test the full hook path but don't isolate lazy-vs-eager getter cost. A benchmark comparing a plugin that reads frontMatter vs one that doesn't would quantify the savings and catch regressions. Open an issue if desired.
Verdict: Ready with fixes
Reasoning: The lazy getter pattern is clean, well-tested (16 spec tests), and delivers a major performance win (3.2s → 36ms for frontMatter serialization). The three required fixes are small: (1) add 2 lines of nil-out in the existing defer block to match the spec, (2) add
log.Printfon 2 error branches to restore observability, (3) changependingTOCto[]content.TOCEntryto match the spec and gain type safety. All three are under 10 lines of change total. The advisory items are genuine but non-blocking.
- Clear pendingFM/pendingTOC after invokeHookFastPath returns (P1 #1, IMPLEMENTATION.md step 6) - Add log.Printf warning on marshal error paths in __resolveFM/__resolveTOC callbacks (P2 #2) - Type pendingTOC as []content.TOCEntry instead of interface{} (P2 #3) - Add enumerable: true to all lazy getter descriptors so frontMatter/toc remain visible to Object.keys, spread, and JSON.stringify Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review responseAll fixes pushed in ea09278. P1 #1 — Missing pendingFM/pendingTOC cleanupFixed. Added P2 #2 — Silent error swallowing in resolve callbacksFixed. Added P2 #3 — pendingTOC typed as interface{}Fixed. Changed to P3 #4 — Global property lookup per hook callSkipped. Estimated savings 2-12ms vs 3.2s saved — not worth the lifecycle management overhead. Can revisit if profiling shows it matters. P3 #5 — ctx.Invoke return value not Free'dSkipped. Return values are CodeRabbit: Non-enumerable accessorsFixed. Added Testing gaps (advisory)
|
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Re-review: All findings addressed ✅
Commit reviewed: ea09278 fix: address review feedback on PR #1200
Previous findings — status
| # | Finding | Status |
|---|---|---|
| P1 #1 | Missing pendingFM/pendingTOC cleanup |
✅ Fixed — r.pendingFM = nil and r.pendingTOC = nil added to invokeHookFastPath defer block (line 578-579). Matches IMPLEMENTATION.md step 6. |
| P2 #2 | Silent error swallowing in resolve callbacks | ✅ Fixed — log.Printf added on both marshal error branches (lines 153, 166). |
| P2 #3 | pendingTOC typed as interface{} |
✅ Fixed — changed to []content.TOCEntry (line 34), content import added (line 16). Matches IMPLEMENTATION.md spec. |
Additional change: enumerable: true on property descriptors
The fix commit also adds enumerable: true to all Object.defineProperty calls — both the initial accessor and the self-replacing data property. This is correct: the old SetPropertyStr("frontMatter", ...) created enumerable properties by default, so JSON.stringify(page) and Object.keys(page) included them. Without enumerable: true, Object.defineProperty defaults to enumerable: false, which would have been a subtle behavioral regression. Good catch.
Verification
- No test file changes — all 16 spec tests still apply as written
- No plan file changes
contentimport is the only new import, required for the typedpendingTOCfield- No
encoding/jsonusage
Verdict: Ready
Reasoning: All three required fixes are correctly applied. The
enumerable: trueaddition is a proactive correctness improvement that prevents a subtle behavioral regression. The implementation now matches the spec, maintains observability on error paths, and properly cleans up Go heap references after hook calls.
Merge origin/main into feature/quickjs-outbound-fastpath. Resolves conflicts between this PR's targeted extraction/BatchCallHook/map fast path and main's lazy FM/TOC (PR #1200) and Node IPC framing (PR #1192). Resolution strategy: - wasm.go: take main's lazy callback approach (return JSON strings, parse in JS getter) + add __callHookByName pre-compiled function, targeted extractors, callHookLocked/BatchCallHook, callHookMapPayload - wasm_test.go: keep this branch's spec tests (identity return asserts pipeline-consumed fields only, not all fields) - plans/: take main's version (architect domain) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
jsonCodec.Marshal+ParseJSONoffrontMatterandTOCon every hook call with lazyObject.definePropertygetters backed by Go callbacks (__resolveFM,__resolveTOC)page.frontMatterorpage.toc— most hooks never access these, saving ~3.2s on the RHDS benchmarkpage.frontMatter = {...}without TypeError__installLazyFM/__installLazyTOChelper functions called viactx.Invoke(notEval) on the hot pathTest results
All 324 plugin spec tests pass with
-race, including 18 new lazy serialization tests from issue #1187:Full suite:
go test -race ./...— all packages pass.Closes #1187
Summary by CodeRabbit