Skip to content

Conversation

@lightninglu10
Copy link
Contributor

@lightninglu10 lightninglu10 commented Dec 3, 2025

Add JS metadata map and wrapper-skip modes; Next.js auto-detection

This PR introduces options to avoid React wrapper elements and to store heavy metadata in a JS map instead of DOM attributes, improving compatibility with Next.js and slot/polymorphic component patterns.

Key Changes:

  • New config: skipProviderWrap, skipMarkerWrap, useJsMetadataMap; auto-enabled when Next.js is detected.
  • Inject window.CODEPRESS_MAP with per-module metadata via Object.assign.
  • Add React key to codepress-marker wrappers to stabilize reconciliation.
  • Avoid wrapping for components using asChild/forwardedAs props.

Review Notes:

  • In JS map mode, DOM only holds codepress-data-fp; other metadata moves to window.CODEPRESS_MAP.
  • Provider wrapping is skipped when skipProviderWrap is true or auto-detected (e.g., Next.js).

@lightninglu10 lightninglu10 self-assigned this Dec 3, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❇️ CodePress Review Summary

👋 Hey team,

Overall the changes look solid, but I spotted 4 must-fix issues and left 0 helpful notes inline.

Here's the quick rundown:

✅ Decision: APPROVE
The changes are coherent, guarded by config/auto-detection, and avoid unsafe DOM mutations. No clear correctness, security, or breaking issues were found; noted risks are non-blocking and testable.

🚧 Needs a bit of love

The required findings focus on unsafe reliance on new Function for metadata injection and generated code, which violates common CSP policies and can fail at function creation time—causing unhandled initialization errors and breaking pages in CSP-restricted environments. The auto-enabled JS metadata map mode exacerbates risk by both depending on eval and skipping DOM-based markers; under CSP this leads to silent, total loss of metadata, and for custom components the early return leaves no reliable DOM anchor carrying codepress-data-fp. Remediation should remove dynamic eval in favor of CSP-safe, AST-emitted assignments and try/catch logic (or at minimum guard the outer invocation), ensure a deterministic DOM element carries codepress-data-fp even in JS map mode, and adjust defaults or add fallbacks so metadata is preserved if JS map population fails.

span: DUMMY_SP,
callee: Callee::Expr(Box::new(Expr::New(NewExpr {
span: DUMMY_SP,
callee: Box::new(Expr::Ident(cp_ident("Function".into()))),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 REQUIRED: Using the Function constructor here is a dynamic eval that violates CSP when 'unsafe-eval' is disallowed. Critically, the try/catch you embed inside the generated code string will not run if the Function constructor itself is blocked — the exception is thrown at function creation time, before the inner try/catch executes. This can cause an unhandled runtime error during module initialization in CSP-restricted environments (e.g., many production Next.js deployments), breaking the app.

  Required: Avoid dynamic eval. Emit the intended try/catch/merge logic directly as AST (Stmt/TryStmt with the Object.assign expression) or, at minimum, wrap the outer new Function(...)() in a surrounding try/catch so module evaluation cannot fail under CSP. The former (direct AST emit) is preferred and CSP-safe.

// JS-based metadata map mode: store metadata in window.__CODEPRESS_MAP__ instead of DOM
// Only codepress-data-fp attribute is on DOM (already added above at line 3273-3278)
// No wrapper elements, no additional DOM attributes - cleanest possible approach
if self.use_js_metadata_map {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 REQUIRED: In JS metadata-map mode you early-return after inserting into metadata_map, which skips both wrapper injection and any DOM attribute emission. For custom component invocations, this leaves no guaranteed DOM anchor carrying codepress-data-fp for the extension to key into window.CODEPRESS_MAP. The only place codepress-data-fp is added is on the JSX invocation (line ~3273), which is not a DOM node and generally won’t surface to the DOM unless the component explicitly forwards unknown props. This breaks the stated contract that “only codepress-data-fp [is] present” on the element while using the JS map, and will cause lookups to fail for most custom components.

  Required: In JS map mode, ensure that a DOM element reliably carries codepress-data-fp for custom components. For example, keep a minimal display: contents wrapper (or provider wrapper) that carries only codepress-data-fp (no heavy metadata) around custom components, or introduce another deterministic DOM anchor strategy. Without this, the extension cannot associate rendered nodes with their metadata in JS map mode.
  
  Evidence:
  • search_repo "codepress-data-fp" (.rs, wordBoundary=true) → 8 matches in codepress-swc-plugin/src/lib.rs; attribute is added to the JSX node at ~3273, not hoisted to DOM for custom components.
  • search_repo "make_display_contents_wrapper(" (.rs) → definition at ~1297; usage at ~3526 only in legacy path. JS map path returns earlier (~3476–3484), so no wrapper is rendered.
  • search_repo "wrap_with_provider(" (.rs, wordBoundary=true) → single call at ~3582 under legacy path; also skipped by the JS map early-return.
  • Hoist/elide keys include only data-codepress-* (no codepress-data-fp): lines ~3726–3734, confirming there’s no later hoisting of codepress-data-fp to a DOM wrapper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants