feat: make editor extensions ssr#3673
Draft
danilowoz wants to merge 1 commit into
Draft
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 6ca1bed The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Running ultrareview automatically — This PR refactors the core serialization pipeline, modifies public APIs (composeReactEmail, SerializerPlugin), adds caching with WeakMap, and introduces a new ComposeContext type — a bug here could silently produce incorrect email HTML for all server-side exports.. I'll post findings when complete. |
commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Ultrareview completed in 31m 37s
12 issues found across 20 files
Confidence score: 3/5
- In
benchmarks/editor-ssr/src/run.ts, parity mismatches are only logged so the benchmark can still exit successfully and publishfalseresults, which undermines the main correctness signal for this PR — makemainfail when any fixture mismatch is detected before merging. - Also in
benchmarks/editor-ssr/src/run.ts, theEditoris torn down beforecomposeReactEmailfinishes, creating a race where serializer/template code can read a destroyed editor and produce flaky parity or throughput numbers — await compose completion before teardown. benchmarks/editor-ssr/tsconfig.jsoncurrently drops Node ambient types, sosrc/run.tstype-checking fails onprocessandnode:*imports; merging as-is risks a broken benchmark build/check step — restore Node types inheritance (or explicitly include them) before merge.- In
packages/editor/src/core/serializer/compose-react-email.tsx, custom content-mode serializers callingeditor.isActive(...)can throw a rawTypeErrorinstead of migration guidance, which makes integration failures harder for consumers to diagnose — classify/interceptisActiveas editor-only in the proxy path before shipping.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="benchmarks/editor-ssr/bench_notebook.md">
<violation number="1" location="benchmarks/editor-ssr/bench_notebook.md:173">
P3: The notebook currently describes the extension list as immutable, but the finalized API detects mutations and re-resolves the schema. This conflicts with both the checked-in changeset and the SSR regression test, so it can mislead maintainers about the cache contract. Updating this review note to describe snapshot validation and re-resolution would keep the benchmark record aligned with the shipped behavior.</violation>
</file>
<file name="benchmarks/editor-ssr/src/cold-start-worker.mjs">
<violation number="1" location="benchmarks/editor-ssr/src/cold-start-worker.mjs:2">
P3: The cold-start result is labeled as a newsletter compose, but this worker serializes a much smaller four-block document. That makes the published `composeMs` unsuitable for comparing the advertised newsletter scenario; consider either using the newsletter fixture's shape here or renaming the sample/benchmark description to a small email compose.</violation>
</file>
<file name="packages/editor/src/core/serializer/compose-react-email.tsx">
<violation number="1" location="packages/editor/src/core/serializer/compose-react-email.tsx:113">
P2: Content-mode custom serializers that call `editor.isActive(...)` get an unhelpful `TypeError` rather than the intended migration guidance, because `isActive` is not classified as an editor-only member and the proxy returns `undefined` for it. Adding this public Editor method to the guarded set would preserve the actionable SSR migration error.</violation>
</file>
<file name="apps/docs/editor/api-reference/compose-react-email.mdx">
<violation number="1" location="apps/docs/editor/api-reference/compose-react-email.mdx:113">
P3: Custom node renderers can otherwise be written with the expectation that inline styles have already been merged into the `style` prop. The compose pipeline passes theme styles only; the built-in renderers merge `node.attrs.style` themselves. Suggest clarifying that this merge is renderer responsibility.</violation>
</file>
<file name="packages/editor/src/plugins/email-theming/extension.tsx">
<violation number="1" location="packages/editor/src/plugins/email-theming/extension.tsx:216">
P3: Editing with a config-object theme now rebuilds all configured panel overrides on every preview update even though this branch only needs the base theme. Reading `configuredTheme.extends` directly preserves the theme choice while avoiding repeated panel/input cloning during editor transactions.</violation>
</file>
<file name="benchmarks/editor-ssr/src/run.ts">
<violation number="1" location="benchmarks/editor-ssr/src/run.ts:4">
P3: Custom agent: **Initialisms and Acronyms Naming Conventions**
The new local identifier uses the all-caps `URL` acronym (`fileURLToPath`), which breaks the repository naming rule for identifiers. Aliasing this import to `fileUrlToPath` and using that alias would keep the Node API while preserving the required camel-cased acronym form.</violation>
<violation number="2" location="benchmarks/editor-ssr/src/run.ts:19">
P2: The editor benchmark tears down its `Editor` before `composeReactEmail` has completed. That can make the editor-mode parity/throughput path observe a destroyed editor when a serializer/BaseTemplate reads the legacy `editor` prop during rendering, and it differs from the cold-start benchmark's lifecycle. Await the compose promise inside the `try` so destruction occurs only after serialization finishes.</violation>
<violation number="3" location="benchmarks/editor-ssr/src/run.ts:80">
P2: A headless/editor output mismatch is only logged, so this advertised parity gate still exits successfully and can publish benchmark results marked with `false`. Fail `main` when any fixture does not match, after retaining the per-fixture diagnostic from `checkParity()`.</violation>
<violation number="4" location="benchmarks/editor-ssr/src/run.ts:142">
P3: The reported `outputBytes` value is a UTF-16 character count rather than the UTF-8 payload size. Unicode email content therefore makes the output-size benchmark undercount bytes; use `Buffer.byteLength` to match the section label and persisted field name.</violation>
</file>
<file name="benchmarks/editor-ssr/tsconfig.json">
<violation number="1" location="benchmarks/editor-ssr/tsconfig.json:8">
P2: The benchmark's TypeScript configuration excludes the Node ambient types inherited from its React-library base config, so type-checking `src/run.ts` fails on `process` and the `node:*` imports even though `@types/node` is installed. Including `node` while retaining the existing Vitest globals lets the SSR benchmark be type-checked.</violation>
</file>
<file name="packages/editor/src/extensions/global-content.ts">
<violation number="1" location="packages/editor/src/extensions/global-content.ts:85">
P2: Custom agent: **Initialisms and Acronyms Naming Conventions**
The new public helper breaks the project's acronym casing convention: `getGlobalContentFromJSON` leaves `JSON` fully uppercase. Naming it `getGlobalContentFromJson` keeps the API camel-cased consistently with the rule and avoids introducing a differently cased public identifier.</violation>
</file>
<file name="benchmarks/editor-ssr/src/fixtures.ts">
<violation number="1" location="benchmarks/editor-ssr/src/fixtures.ts:1">
P3: Custom agent: **Initialisms and Acronyms Naming Conventions**
The new fixture types expose the `JSON` initialism in all caps (`JSONContent`), which conflicts with the repository naming rule for identifiers. Aliasing the import to `JsonContent` and updating its local annotations keeps the TipTap API import intact while making the local type name consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| 'getJSON', | ||
| 'getHTML', | ||
| 'getText', | ||
| 'getAttributes', |
Contributor
There was a problem hiding this comment.
P2: Content-mode custom serializers that call editor.isActive(...) get an unhelpful TypeError rather than the intended migration guidance, because isActive is not classified as an editor-only member and the proxy returns undefined for it. Adding this public Editor method to the guarded set would preserve the actionable SSR migration error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/core/serializer/compose-react-email.tsx, line 113:
<comment>Content-mode custom serializers that call `editor.isActive(...)` get an unhelpful `TypeError` rather than the intended migration guidance, because `isActive` is not classified as an editor-only member and the proxy returns `undefined` for it. Adding this public Editor method to the guarded set would preserve the actionable SSR migration error.</comment>
<file context>
@@ -49,17 +51,108 @@ interface ComposeReactEmailResult {
+ 'getJSON',
+ 'getHTML',
+ 'getText',
+ 'getAttributes',
+]);
+
</file context>
Suggested change
| 'getAttributes', | |
| 'getAttributes', | |
| 'isActive', |
| // 1. Parity: the headless path must produce byte-identical output to the | ||
| // editor path for the same document. Anything else is a correctness bug, | ||
| // not a performance trade-off. | ||
| const parity = await checkParity(); |
Contributor
There was a problem hiding this comment.
P2: A headless/editor output mismatch is only logged, so this advertised parity gate still exits successfully and can publish benchmark results marked with false. Fail main when any fixture does not match, after retaining the per-fixture diagnostic from checkParity().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/editor-ssr/src/run.ts, line 80:
<comment>A headless/editor output mismatch is only logged, so this advertised parity gate still exits successfully and can publish benchmark results marked with `false`. Fail `main` when any fixture does not match, after retaining the per-fixture diagnostic from `checkParity()`.</comment>
<file context>
@@ -0,0 +1,157 @@
+ // 1. Parity: the headless path must produce byte-identical output to the
+ // editor path for the same document. Anything else is a correctness bug,
+ // not a performance trade-off.
+ const parity = await checkParity();
+ results.parity = parity;
+ console.log('\n## Parity (editor path vs content path)\n');
</file context>
| function composeViaEditor(doc: (typeof fixtures)[keyof typeof fixtures]) { | ||
| const editor = new Editor({ extensions, content: doc }); | ||
| try { | ||
| return composeReactEmail({ editor }); |
Contributor
There was a problem hiding this comment.
P2: The editor benchmark tears down its Editor before composeReactEmail has completed. That can make the editor-mode parity/throughput path observe a destroyed editor when a serializer/BaseTemplate reads the legacy editor prop during rendering, and it differs from the cold-start benchmark's lifecycle. Await the compose promise inside the try so destruction occurs only after serialization finishes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/editor-ssr/src/run.ts, line 19:
<comment>The editor benchmark tears down its `Editor` before `composeReactEmail` has completed. That can make the editor-mode parity/throughput path observe a destroyed editor when a serializer/BaseTemplate reads the legacy `editor` prop during rendering, and it differs from the cold-start benchmark's lifecycle. Await the compose promise inside the `try` so destruction occurs only after serialization finishes.</comment>
<file context>
@@ -0,0 +1,157 @@
+function composeViaEditor(doc: (typeof fixtures)[keyof typeof fixtures]) {
+ const editor = new Editor({ extensions, content: doc });
+ try {
+ return composeReactEmail({ editor });
+ } finally {
+ editor.destroy();
</file context>
| "exclude": ["dist", "build", "node_modules"], | ||
| "compilerOptions": { | ||
| "target": "esnext", | ||
| "noUncheckedIndexedAccess": true, |
Contributor
There was a problem hiding this comment.
P2: The benchmark's TypeScript configuration excludes the Node ambient types inherited from its React-library base config, so type-checking src/run.ts fails on process and the node:* imports even though @types/node is installed. Including node while retaining the existing Vitest globals lets the SSR benchmark be type-checked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/editor-ssr/tsconfig.json, line 8:
<comment>The benchmark's TypeScript configuration excludes the Node ambient types inherited from its React-library base config, so type-checking `src/run.ts` fails on `process` and the `node:*` imports even though `@types/node` is installed. Including `node` while retaining the existing Vitest globals lets the SSR benchmark be type-checked.</comment>
<file context>
@@ -0,0 +1,15 @@
+ "exclude": ["dist", "build", "node_modules"],
+ "compilerOptions": {
+ "target": "esnext",
+ "noUncheckedIndexedAccess": true,
+ "resolveJsonModule": true,
+ "moduleResolution": "Bundler",
</file context>
| * editor. Pure and cache-free, so it is safe for server workloads that | ||
| * serialize many documents concurrently. | ||
| */ | ||
| export function getGlobalContentFromJSON( |
Contributor
There was a problem hiding this comment.
P2: Custom agent: Initialisms and Acronyms Naming Conventions
The new public helper breaks the project's acronym casing convention: getGlobalContentFromJSON leaves JSON fully uppercase. Naming it getGlobalContentFromJson keeps the API camel-cased consistently with the rule and avoids introducing a differently cased public identifier.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/extensions/global-content.ts, line 85:
<comment>The new public helper breaks the project's acronym casing convention: `getGlobalContentFromJSON` leaves `JSON` fully uppercase. Naming it `getGlobalContentFromJson` keeps the API camel-cased consistently with the rule and avoids introducing a differently cased public identifier.</comment>
<file context>
@@ -57,6 +62,35 @@ export function getGlobalContent(key: string, editor: Editor): unknown | null {
+ * editor. Pure and cache-free, so it is safe for server workloads that
+ * serialize many documents concurrently.
+ */
+export function getGlobalContentFromJSON(
+ key: string,
+ doc: JSONContent,
</file context>
Comment on lines
+113
to
114
| 1. **Resolves styles** — calls `serializerPlugin.getNodeStyles(node, depth, context)` to get | ||
| theme styles, then merges any inline styles from the node's attributes |
Contributor
There was a problem hiding this comment.
P3: Custom node renderers can otherwise be written with the expectation that inline styles have already been merged into the style prop. The compose pipeline passes theme styles only; the built-in renderers merge node.attrs.style themselves. Suggest clarifying that this merge is renderer responsibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/docs/editor/api-reference/compose-react-email.mdx, line 113:
<comment>Custom node renderers can otherwise be written with the expectation that inline styles have already been merged into the `style` prop. The compose pipeline passes theme styles only; the built-in renderers merge `node.attrs.style` themselves. Suggest clarifying that this merge is renderer responsibility.</comment>
<file context>
@@ -74,9 +110,9 @@ plugin is found, styles default to `{}` and the built-in `DefaultBaseTemplate` i
It recursively walks the ProseMirror document. For each node it:
-1. **Resolves styles** — calls `serializerPlugin.getNodeStyles(node, depth, editor)` to get
+1. **Resolves styles** — calls `serializerPlugin.getNodeStyles(node, depth, context)` to get
theme styles, then merges any inline styles from the node's attributes
-2. **Renders unknown nodes as `null`** — if the node type isn't registered or isn't an
</file context>
Suggested change
| 1. **Resolves styles** — calls `serializerPlugin.getNodeStyles(node, depth, context)` to get | |
| theme styles, then merges any inline styles from the node's attributes | |
| 1. **Resolves styles** — calls `serializerPlugin.getNodeStyles(node, depth, context)` to get | |
| theme styles and passes them to the node renderer, which can merge inline styles from its attributes |
| globalStyles: PanelGroup[] | null, | ||
| ): EditorTheme { | ||
| if (isThemeConfig(configuredTheme)) { | ||
| return resolveThemeConfig(configuredTheme).baseTheme; |
Contributor
There was a problem hiding this comment.
P3: Editing with a config-object theme now rebuilds all configured panel overrides on every preview update even though this branch only needs the base theme. Reading configuredTheme.extends directly preserves the theme choice while avoiding repeated panel/input cloning during editor transactions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/plugins/email-theming/extension.tsx, line 216:
<comment>Editing with a config-object theme now rebuilds all configured panel overrides on every preview update even though this branch only needs the base theme. Reading `configuredTheme.extends` directly preserves the theme choice while avoiding repeated panel/input cloning during editor transactions.</comment>
<file context>
@@ -187,19 +192,117 @@ function resolveThemeConfig(config: EditorThemeInput): {
+ globalStyles: PanelGroup[] | null,
+): EditorTheme {
+ if (isThemeConfig(configuredTheme)) {
+ return resolveThemeConfig(configuredTheme).baseTheme;
+ }
+ if (configuredTheme === 'basic' || configuredTheme === 'minimal') {
</file context>
Suggested change
| return resolveThemeConfig(configuredTheme).baseTheme; | |
| return configuredTheme.extends ?? 'minimal'; |
| content: doc, | ||
| extensions, | ||
| }); | ||
| sizes[name] = unformattedHtml.length; |
Contributor
There was a problem hiding this comment.
P3: The reported outputBytes value is a UTF-16 character count rather than the UTF-8 payload size. Unicode email content therefore makes the output-size benchmark undercount bytes; use Buffer.byteLength to match the section label and persisted field name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/editor-ssr/src/run.ts, line 142:
<comment>The reported `outputBytes` value is a UTF-16 character count rather than the UTF-8 payload size. Unicode email content therefore makes the output-size benchmark undercount bytes; use `Buffer.byteLength` to match the section label and persisted field name.</comment>
<file context>
@@ -0,0 +1,157 @@
+ content: doc,
+ extensions,
+ });
+ sizes[name] = unformattedHtml.length;
+ }
+ results.outputBytes = sizes;
</file context>
| @@ -0,0 +1,160 @@ | |||
| import type { JSONContent } from '@tiptap/core'; | |||
Contributor
There was a problem hiding this comment.
P3: Custom agent: Initialisms and Acronyms Naming Conventions
The new fixture types expose the JSON initialism in all caps (JSONContent), which conflicts with the repository naming rule for identifiers. Aliasing the import to JsonContent and updating its local annotations keeps the TipTap API import intact while making the local type name consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/editor-ssr/src/fixtures.ts, line 1:
<comment>The new fixture types expose the `JSON` initialism in all caps (`JSONContent`), which conflicts with the repository naming rule for identifiers. Aliasing the import to `JsonContent` and updating its local annotations keeps the TipTap API import intact while making the local type name consistent.</comment>
<file context>
@@ -0,0 +1,160 @@
+import type { JSONContent } from '@tiptap/core';
+
+/**
</file context>
| import { spawnSync } from 'node:child_process'; | ||
| import { mkdirSync, writeFileSync } from 'node:fs'; | ||
| import { dirname, resolve } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; |
Contributor
There was a problem hiding this comment.
P3: Custom agent: Initialisms and Acronyms Naming Conventions
The new local identifier uses the all-caps URL acronym (fileURLToPath), which breaks the repository naming rule for identifiers. Aliasing this import to fileUrlToPath and using that alias would keep the Node API while preserving the required camel-cased acronym form.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/editor-ssr/src/run.ts, line 4:
<comment>The new local identifier uses the all-caps `URL` acronym (`fileURLToPath`), which breaks the repository naming rule for identifiers. Aliasing this import to `fileUrlToPath` and using that alias would keep the Node API while preserving the required camel-cased acronym form.</comment>
<file context>
@@ -0,0 +1,157 @@
+import { spawnSync } from 'node:child_process';
+import { mkdirSync, writeFileSync } from 'node:fs';
+import { dirname, resolve } from 'node:path';
+import { fileURLToPath } from 'node:url';
+import { composeReactEmail } from '@react-email/editor/core';
+import { StarterKit } from '@react-email/editor/extensions';
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Make the email serialization pipeline SSR-capable.
composeReactEmailcan now render stored TipTap JSON with your extensions on any server, noEditoror DOM required, and includes aformatoption to skip Prettier when not needed.New Features
composeReactEmail({ content, extensions })returnshtml,text, andunformattedHtml;{ editor }still works.format(defaulttrue) skips Prettier whenfalse;html === unformattedHtmlin that mode.ComposeContext({ doc, schema, extensions });getEmailThemingaccepts either an editor or a context.editor.extensionManager.extensions) are rejected.getGlobalContentFromJSONand exportedEmailThemingResult.Migration
contextingetNodeStylesandBaseTemplate. Legacy editor-based plugins continue working in{ editor }mode; accessing editor internals in{ content }mode throws a clear error.contentand the originalextensionslist; do not mixeditorwithcontent/extensions, and do not pass pre-resolved arrays.format: falseon server send paths to avoid the Prettier cost.Written for commit 6ca1bed. Summary will update on new commits.