Skip to content

Conversation

@lightninglu10
Copy link
Contributor

@lightninglu10 lightninglu10 commented Dec 10, 2025

Inject window config and refactor HMR wrapping

This PR moves repo/branch propagation from DOM attributes to a global window.CODEPRESS_CONFIG and refactors the HMR provider injection to wrap the default export component externally.

Key Changes:

  • SWC: Add inject_config to set window.CODEPRESS_CONFIG once per build; remove repo/branch JSX attribute code paths.
  • SWC: Replace return-statement wrapping with external wrapper of the default export component using __CPRefreshProvider.
  • Esbuild/Babel: Prepend config injection, and stop adding repo/branch HTML attributes.
  • Version bump to 0.9.0.

Review Notes:

  • Ensure CSP compatibility; avoid unsafe-eval patterns.
  • Verify no identifier collisions for modules exporting default identifiers or anonymous functions.

@lightninglu10 lightninglu10 self-assigned this Dec 10, 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 11 must-fix issues and left 0 helpful notes inline.

Here's the quick rundown:

❌ Decision: REQUEST CHANGES
The SWC config injection uses new Function (unsafe-eval), which will break under common CSPs, and the wrapper logic redeclares an existing default export identifier (export default App), causing a correctness error. These are blocking issues that must be fixed before merge.

🚧 Needs a bit of love

The most critical issue is the pervasive use of new Function/eval to inject config and metadata, which violates CSP (unsafe-eval) and will throw at construction time, causing runtime failures before any inner try/catch can run. Replace these with static, AST-generated statements that guard on typeof window !== 'undefined' and assign to window.CODEPRESS_CONFIG via Object.assign, inserted appropriately after the directive prologue. Config injection is also incorrectly gated by a shared, never-reset GLOBAL_ATTRIBUTES_ADDED flag, leading to skipped or nondeterministic injection across builds; use a dedicated per-transform/per-build flag instead. Additionally, the default-export wrapper logic can introduce duplicate bindings or name collisions (e.g., reusing App), so emit an anonymous default wrapper or a distinct identifier only when an original name exists, and ensure the esbuild plugin injects the config once per build rather than into every module to avoid bundle bloat and repeated work.

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.

Review Summary

This PR centralizes repo/branch configuration into window.CODEPRESS_CONFIG across SWC, Babel, and esbuild integrations, and refactors HMR to wrap the default export component with __CPRefreshProvider instead of rewriting return sites. It also updates the internal CodePress review workflow to v4 and bumps the package version to 0.9.0.

Reviewed areas

  • SWC transform (config injection and CPRefreshProvider wrapping)
  • Babel and esbuild config injection paths
  • Removal of DOM attribute-based repo/branch propagation
  • GitHub Actions workflow and package metadata

Blocking issues (🔴 REQUIRED)

  • SWC: inject_config and inject_metadata_map both use new Function(...) to eval generated code, which violates CSP (requires 'unsafe-eval') and will be blocked in many production environments. This can break module evaluation and leave window.CODEPRESS_CONFIG unset. These sites must emit direct AST-based statements instead of eval.
  • SWC: inject_config is gated by and mutates GLOBAL_ATTRIBUTES_ADDED, conflating config injection with historical DOM-attribute state. This global flag is never reset and may cause config injection to be skipped across modules/builds.
  • SWC: The RefreshProviderWrapper wrapper currently reuses the original identifier name in some ExportDefaultExpr cases and names the wrapper App when no original_ident is present. Both patterns can introduce duplicate bindings or name collisions in the module scope.
  • Esbuild: The module-level window.CODEPRESS_CONFIG snippet is prefixed into every transformed module, causing bundle bloat and repeated runtime work instead of a single, build-scoped injection.

These are substantial security, correctness, and performance concerns for consumers with CSP enabled or large codebases. Once the eval usage is removed, the config gating is decoupled from GLOBAL_ATTRIBUTES_ADDED (or otherwise made robust), wrapper naming is made collision-safe, and esbuild’s config injection is de-duplicated or centralized, the overall direction of the PR looks solid.

@lightninglu10 lightninglu10 merged commit be8c2ce into main Dec 10, 2025
7 checks passed
@lightninglu10 lightninglu10 deleted the github-in-js branch December 10, 2025 06:53
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