Skip to content

chore(editor): snapshot and unit testing#3487

Open
danilowoz wants to merge 26 commits into
canaryfrom
claude/editor-reliability-testing-EYQXQ
Open

chore(editor): snapshot and unit testing#3487
danilowoz wants to merge 26 commits into
canaryfrom
claude/editor-reliability-testing-EYQXQ

Conversation

@danilowoz
Copy link
Copy Markdown
Member

@danilowoz danilowoz commented May 11, 2026

  • Fixes a few editor bugs: unsafe-URL paste, broken image uploads, missing placeholder on empty headings.
  • Adds 175 new tests, including individual extension snapshots and a bunch of real world emails snapshots
  • Sets up shared test helpers and a coverage check.

claude added 19 commits May 10, 2026 17:23
- createTestEditor harness lifts the inline createEditorWithContent
  pattern from compose-react-email.spec.tsx into a shared helper.
- loadFixture reader for src/__tests__/fixtures/{emails,paste-sources}/
  cached per process.
- proseMirrorDocArbitrary for fast-check property tests, depth- and
  length-constrained to keep shrinking fast.
- Adds fast-check and @vitest/coverage-v8 to devDependencies.
- Sanity spec exercises createTestEditor end-to-end.

Foundation for the Snow Leopard unit-test plan; every subsequent
commit imports from here instead of duplicating ~40 lines of setup.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Adds the first regression net for the paste-sanitization seam — the
hot path behind the "imports get reformatted" / "pasted code uses v1
extensions" bug cluster (MES-490, MES-472, ~50KB → 300KB import).

The spec also revealed that javascript:, vbscript:, data:, and file:
URLs were preserved in href/src on external pastes. Fixed by adding
URL_ATTRIBUTES + UNSAFE_URL_SCHEMES check in sanitizeElement.

Snapshots from word/gmail/notion/vscode/apple-mail/view-source
fixtures lock down deterministic output across the canonical paste
sources we care about.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Extends isDocumentVisuallyEmpty so a doc whose only non-global child
is an empty heading is treated the same as an empty paragraph. This
unblocks the heading placeholder logic flagged at email-editor.tsx:153
(\"this heading placeholder is not working\") and matches MES-382 plus
the open P0s around the editor-empty / blinking-cursor flow.

The matching empty-text-block predicate now handles paragraph and
heading; the rest of the predicate is unchanged. Adds 6 spec cases
covering empty heading, heading with text, heading with variable,
mixed empty heading + variable paragraph, and the container variants.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
…-circuit

The paste handler used to bail out of sanitization whenever the
ProseMirror slice contained a single child, falling back to the
default paste path. That meant external HTML pastes that happen to
parse to one node bypassed sanitizePastedHtml entirely — the seam
behind MES-490 (\"Pasting code still uses v1 editor extensions\"),
MES-472 (\"Opening template rewrites HTML and duplicates content\"),
and the open report of 50KB imports being reformatted to 300KB+.

Now every text/html paste runs through the sanitizer and dispatches
exactly one transaction. Adds 5 spec cases covering the regression
plus the existing onPaste/file/missing-clipboardData paths. The
@tiptap/html generateJSON is stubbed at the test boundary because
its schema dependency isn't relevant to the handler's control flow.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
The drop handler used to consume only file payloads and return false
for text/html and text/plain — letting ProseMirror's default drop
path corrupt the document, the seam behind MES-461 (\"Dragging
editor block removes callout styling\") and the open P0 for drag
content blocks. EmailEditor itself never set handleDrop, so out of
the box drops were unhandled too.

Now createDropHandler accepts an optional `extensions` argument and
mirrors the paste path: sanitize via sanitizePastedHtml, generate
JSON, dispatch one transaction. EmailEditor passes its extensions
in. Internal block reorders (moved=true) still fall through to PM.
Backward compatible: callers that omit `extensions` keep the legacy
file-only behavior.

Adds 5 spec cases covering the regression plus the legacy paths,
and switches load-fixture.ts off `import.meta.url` to play nicely
with the package's CJS+ESM dual build under nodenext.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
executeUploadFlow had four production-visible failure modes that
collectively drove the largest non-serializer bug cluster (MES-476,
MES-377, MES-481, CUS-495, the open Safari image-upload P0):

  1. Failures bypassed the editor event bus (only console.error),
     so callers couldn't surface a toast — appearing as silent
     \"upload didn't work\" reports.
  2. Both swapImageSrc and removeImageBySrc returned after the
     first match, so duplicated images that shared one blob src
     leaked into the document on failure.
  3. view.dispatch ran unconditionally even when the editor had
     been destroyed mid-flight, throwing on the React unmount path.
  4. No abort signal: navigating away while uploading still
     attempted to insert the resolved URL into a stale doc.

Now upload errors emit `image-upload-error` on the event bus
(consumers can surface a toast), the descendant scan handles all
matching nodes with safe back-to-front deletion, every dispatch
checks isEditorAlive, and an optional AbortSignal short-circuits
the flow before insertion is finalized.

Adds 7 spec cases. Image extension wiring is unchanged; signal
support is opt-in so existing callers continue to work.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Adds the regression net for the largest editor bug cluster — the
\"editor and code view show different output\" family (MES-388,
MES-472, MES-488, GAB-5, GAB-11, WORKSPA-1721).

Two new describe blocks in compose-react-email.spec.tsx:

  - round-trip corpus: 7 HTML fixtures under
    src/__tests__/fixtures/emails/ covering the bug-prone shapes
    (mixed marks, headings, lists, images, blockquote/code,
    inline-styled typography, nested tables). Each fixture is
    parsed via generateJSON and rendered via composeReactEmail;
    the output is snapshotted. Future drift in either parser or
    serializer surfaces immediately at PR review.
  - round-trip property: fast-check generates arbitrary
    proseMirrorDocArbitrary trees and asserts composeReactEmail
    never throws and always returns a string. 25 runs per CI.

The corpus is intentionally small for now; per the plan, future
bug fixes should land with a fixture entry that would have caught
the bug, growing the corpus organically.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Locks down the public event name + payload shape introduced for
upload-flow's failure path in the previous commit. Subscribers like
the dashboard rely on this name; renaming or changing the payload
shape now breaks here at the type level instead of silently
changing runtime behavior.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
The editor's root component had no spec coverage despite being the
seam where the imperative ref API, ready/update lifecycle, and the
known theme-remount defect all live.

Adds 4 active tests:
  - ref exposes getEmail / getEmailHTML / getEmailText / getJSON
  - onReady fires exactly once after mount
  - onUpdate carries the latest ref (no stale closure)
  - editable=false propagates to the underlying editor

Plus one it.skip for the theme-no-remount invariant: today the
component re-keys EditorProvider on every theme change, destroying
the editor and wiping undo. The fix is parked because reconfiguring
EmailTheming imperatively touches the dashboard's collaborative
session lifecycle and needs broader validation. The skipped test
makes the regression visible at PR review.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
…aphy)

The 8 inspector sections have driven a third of the editor's
historical bug volume (~10 closed bugs plus 3 still-open P0/P1s)
yet had zero direct test coverage; only inner hooks/utils were
tested. This commit ships the 4 sections most tied to live
defects:

  - border: per-side mutations don't reset other sides (open P0)
  - padding: batched updates preserve untouched sides
  - size: routes through setStyle for non-image, setAttr for image
  - typography: color/fontSize/lineHeight prop round-trips
    (MES-491), conditional marks + alignment UI

Adds buildInspectorContext() in __tests__/context-helpers.ts so
each section spec is ~50 lines instead of ~150. Backgrounds,
column-spacing, link, and attributes follow the same pattern;
they're staged for a separate commit.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Completes the inspector-section coverage matrix begun in the prior
commit. background / column-spacing / link / attributes follow the
same pattern: render with a buildInspectorContext fixture, assert
the section renders the expected title, asserts conditional render
gating (link only when active, column-spacing only on column
parents, attributes only when visible attrs exist), and verifies
each section's mutation callback wiring.

The 8 sections together address roughly 13 closed bugs and 3 still
open in cluster E.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Locks down the click-to-select contract behind the open P3
\"clicking nested elements opens Page style panel instead of
element settings\". Three tests cover:

  - empty path renders no buttons
  - clicking a nested segment dispatches setNodeSelection at the
    segment's pos and focuses (not the current cursor's pos)
  - clicking the body segment blurs instead of selecting pos 0
    (the documented escape hatch in breadcrumb.tsx:30)

Uses module-level mocks for useCurrentEditor / useInspector so the
spec stays fast and doesn't need an editor instance.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
The slash command surface drove the MES-470 / ENG-1592 / open P0
\"Test: SlashCommand buggy in editor V2\" cluster but its commands
catalog (245 LOC) had zero direct coverage; only the search
algorithm was tested.

commands.spec.ts:
  - asserts every exported SlashCommandItem has the required
    title/description/searchTerms/command shape
  - drives each command's `command()` callback with a Proxy-based
    fake editor and confirms it produces chain operations + calls
    run() once
  - locks H1/H2/H3 setNode level args
  - sanity checks defaultSlashCommands membership

utils.spec.ts:
  - covers updateScrollView in all directions (above / below /
    visible / boundary), the helper used by the keyboard nav
    flow that ENG-1592 reported broken

44 + 4 new tests, no source changes.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
…il render

Each extension under src/extensions/ has a renderToReactEmail
component that produces the final email HTML for its node/mark.
Heading already had a snapshot spec; the rest had none — meaning
silent regressions in the rendered output (like MES-488 stripping
inline typography styles) survived to production.

Adds snapshotExtensionRender() in extensions/__tests__/, modeled
on heading.spec.tsx, then specs for: paragraph, italic, underline,
strike, code, blockquote, sup, hard-break, list-item, bullet-list,
ordered-list. Each spec is ~15 lines.

The remaining unspecced extensions (alignment-attribute,
class-attribute, preserved-style, prism-plugin, max-nesting,
preview-text, global-content, text) are wiring/attribute-only
modules without renderToReactEmail; they're addressed by the
serializer round-trip corpus in commit 7.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
The css-transforms module had specs for transformToCssJs +
injectThemeCss + injectGlobalPlainCss but two branches were
uncovered:

  - the legacy 'h-padding' prop mapping at css-transforms.ts:37
    (gated by an @ts-expect-error). Without a test, refactors that
    drop this case silently re-introduce the GAB-11 \"Body and
    container styles missing for HTML imports\" regression.
  - mergeCssJs, exported but untested: 5 cases covering child-wins
    merge semantics, new-key addition, primitive overwrite, and
    no-mutation of the input.

7 new test cases.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Adds 8 spec cases for the paste + drop ProseMirror plugin that
routes image files into executeUploadFlow:

  - handlePaste: image accepted, non-image rejected, missing
    clipboardData / empty files arrays handled
  - handleDrop: same matrix plus the moved=true reorder skip

Locks the contract that drove "Customer unable to upload images
into Broadcast Editor" (CUS-495) and the open Safari image-upload
P0. Combined with the upload-flow rewrite in commit 6, the image
pipeline now has end-to-end unit coverage of every public seam.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Both serializer base classes carry @ts-expect-error annotations on
configure() and extend() — the typings papered over a real risk:
on tiptap upgrades these subclass overrides could silently revert
to the base Mark/Node return type, dropping the
renderToReactEmail component without a compile error.

Adds 4 spec cases that lock the runtime contract:

  - EmailMark.extend() returns an EmailMark instance and keeps
    renderToReactEmail
  - EmailNode.extend() same
  - configure() chained twice keeps renderToReactEmail (both)

These are the safety net required before the @ts-expect-errors
themselves can be removed in a follow-up maintainability PR.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Adds a coverage block to vitest.config.ts that runs only against
the directories the Snow Leopard plan covered:

  - src/core/**         — paste/drop, serializer, event-bus
  - src/utils/**        — paste sanitizer, styles, alignment
  - src/plugins/image/** — file handler, upload flow, extension
  - plugins/email-theming/css-transforms.ts

Thresholds are 70% statements / 60% branches / 70% functions /
70% lines. Current numbers are well above the floor (90.49% /
85% / 85.85% / 91.03%); the floor is intentionally below the
current state to leave breathing room for in-flight work without
blocking unrelated PRs.

Adds pnpm test:coverage script for local + CI use.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 11, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
react-email Ready Ready Preview, Comment May 13, 2026 9:39am
react-email-demo Ready Ready Preview, Comment May 13, 2026 9:39am

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 11, 2026

⚠️ No Changeset found

Latest commit: 2a95350

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@danilowoz danilowoz changed the title Claude/editor reliability testing eyqxq editor: reliability testing eyqxq May 11, 2026
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 11, 2026

Open in StackBlitz

npm i https://pkg.pr.new/@react-email/editor@3487

commit: 2a95350

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

2 issues found across 75 files

Confidence score: 3/5

  • There is a concrete behavior risk in packages/editor/src/plugins/image/upload-flow.ts: aborted uploads may still dispatch image-upload-error, which can surface misleading error states to users during normal cancel flows.
  • The test concern in packages/editor/src/ui/inspector/sections/link.spec.tsx is low severity, but it reduces confidence because the current spec bypasses LinkSection wiring and may miss a ColorInputsetLinkColor regression.
  • Given a medium-severity runtime issue with high confidence plus a test coverage gap, this sits at moderate merge risk rather than a clear low-risk merge.
  • Pay close attention to packages/editor/src/plugins/image/upload-flow.ts and packages/editor/src/ui/inspector/sections/link.spec.tsx - abort-path error dispatch behavior and missing integration-style wiring coverage.
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="packages/editor/src/plugins/image/upload-flow.ts">

<violation number="1" location="packages/editor/src/plugins/image/upload-flow.ts:52">
P2: Skip error dispatch in the catch path when the abort signal is set, otherwise aborted uploads can still emit `image-upload-error`.</violation>
</file>

<file name="packages/editor/src/ui/inspector/sections/link.spec.tsx">

<violation number="1" location="packages/editor/src/ui/inspector/sections/link.spec.tsx:51">
P3: This test doesn't exercise `LinkSection`; it calls the mock directly, so it won't catch a broken `ColorInput` → `setLinkColor` wiring.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread packages/editor/src/plugins/image/upload-flow.ts Outdated
Comment thread packages/editor/src/ui/inspector/sections/link.spec.tsx Outdated
@danilowoz danilowoz changed the title editor: reliability testing eyqxq editor: reliability testing May 11, 2026
claude added 2 commits May 11, 2026 08:56
Tests now read on their own merits; ticket history belongs in the
commit log, not in code comments.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
…ples

Emails (8 new): welcome-confirm, password-reset, magic-link,
order-receipt, marketing-announcement, team-invite,
digest-newsletter, notification-with-footer. Modeled on the
shapes Resend users actually build — transactional flows with
buttons, marketing announcements with hero images + CTAs,
receipts with item tables, newsletters with sections + quotes,
notifications with a real footer.

Paste sources (4 new + 1 expanded): outlook-web (Word's full
Mso* schema cruft), chatgpt (markdown-prose div), github-markdown
(octicon links, syntax-highlighted code), linear-comment
(ProseMirror output with mention chips). Existing notion fixture
upgraded to include checkbox-list, toggle callout, status pill.

Tests: 634 → 646 passing.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv

vi.mock('@/actions/ai', () => ({ uploadImageViaAI: vi.fn() }));

const mockEditor = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have some component tests already defined inside of tests, can we maybe move tests that actually do things in the editor to those?

The benefit is that we don't do mocking, it's less code and we can have more confidence in the tests.


it('routes width changes to setAttr for image nodes', () => {
const ctx = buildInspectorContext({ nodeType: 'image' });
render(<SizeSection {...ctx} />);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

these tests aren't actually testing the SizeSection, right? maybe we should make the change here by editing the fields?

import { buildInspectorContext } from '../__tests__/context-helpers';
import { SizeSection } from './size';

vi.mock('@/actions/ai', () => ({ uploadImageViaAI: vi.fn() }));
Copy link
Copy Markdown
Member

@gabrielmfern gabrielmfern May 12, 2026

Choose a reason for hiding this comment

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

I don't think this mock is right. I think there're also other instances of this on the other tests

expect(container.textContent).toContain('Spacing');
});

it('coerces non-numeric padding values to 0', () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is not actually testing it converts to 0, it's testing it doesn't throw. can we test that it actually converts to 0?

},
});
const { container } = render(<PaddingSection {...ctx} />);
expect(container.textContent).toContain('Spacing');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we also test that the padding values show up?

});
render(<PaddingSection {...ctx} />);

ctx.batchSetStyle([{ prop: 'paddingTop', value: 16 }]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this isn't actually testing the component, right? maybe we should make the change here by editing the fields?

attrs: { cellspacing: 0 },
});
render(<ColumnSpacingSection {...ctx} />);
ctx.setAttr('cellspacing', 12);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should dispatch a change event here to the field

nodeType: 'twoColumns',
attrs: { cellspacing: undefined },
});
expect(() => render(<ColumnSpacingSection {...ctx} />)).not.toThrow();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this doesn't test that we're actually coercing

it('renders nothing for non-column node types', () => {
const ctx = buildInspectorContext({ nodeType: 'paragraph' });
const { container } = render(<ColumnSpacingSection {...ctx} />);
expect(container.textContent).not.toContain('Column spacing');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
expect(container.textContent).not.toContain('Column spacing');
expect(container.textContent).toBe('');

const ctx = buildInspectorContext({ styles: { borderRadius: 0 } });
render(<BorderSection {...ctx} />);

ctx.setStyle('borderRadius', 12);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should dispatch a change event here to the field

});
render(<BorderSection {...ctx} />);

ctx.batchSetStyle([
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should dispatch a change event here to the field

// Simulate the BorderPicker emitting a single-prop change (the picker
// path that drives setStyle, not batchSetStyle, when only one prop
// changes).
ctx.setStyle('borderTopWidth', 4);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should dispatch a change event here to the field

it('color mutation goes through setStyle', () => {
const ctx = buildInspectorContext({});
render(<BackgroundSection {...ctx} />);
ctx.setStyle('backgroundColor', '#0670DB');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should dispatch a change event here to the field


it('coerces missing backgroundColor to empty string without throwing', () => {
const ctx = buildInspectorContext({});
expect(() => render(<BackgroundSection {...ctx} />)).not.toThrow();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is not actually testing that we're coercing a missing backgroundColor

* expect(ctx.setStyle).toHaveBeenCalledWith('paddingTop', 16);
* expect(ctx.styles.paddingTop).toBe(16);
*/
export function buildInspectorContext({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the tests that need this, I think we should make a new component test for the inspector itself. I believe we already have some inspector component tests too

@@ -1,46 +1,84 @@
import type { Editor } from '@tiptap/core';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe these changes are unrelated, and we need closer testing and reviewing for this. Are we trying to fix problems caught by tests?

nodeSize: number;
}

function makeFakeEditor(images: FakeImageNode[]) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we could do this with a component test and without mocking by passing down the image upload function I believe

import { ListItem } from './list-item';

describe('ListItem Node', () => {
it('wraps children in <li> with style', async () => {
Copy link
Copy Markdown
Member

@gabrielmfern gabrielmfern May 12, 2026

Choose a reason for hiding this comment

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

I think these titles are going to be a maintenance burden, maybe we should just have this all be a test('ListItem node snapshot').

Specially because the title isn't quite exactly what's being tested.


exports[`Underline Mark > wraps children in the underline element with style 1`] = `
"<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<!--$--><u style="text-decoration:underline">underlined</u
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should there be an undefined here like this?


// `key={JSON.stringify(theme)}` on EditorProvider re-mounts the editor on
// theme change, wiping undo/scroll/selection. Skipped pending an imperative
// reconfigure of EmailTheming.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we should create a Linear issue about this so we can address in a follow up

Comment thread packages/editor/src/core/event-bus.ts Outdated
export interface EditorEventMap {
'bubble-menu:add-link': undefined;
'node-clicked': NodeClickedEvent;
'image-upload-error': ImageUploadErrorEvent;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is also part of the changes in the image upload flow that should be part of a follow up


describe('round-trip property', () => {
it('composeReactEmail does not throw on arbitrary valid JSON', async () => {
await fc.assert(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is it intended that we're using something else other than vitest to assert things here?

Out-of-scope reverts (deferred to a separate PR):
- upload-flow.ts: restored original implementation
- event-bus.ts: dropped image-upload-error event
- email-editor.spec: removed theme-remount it.skip
- vitest.config: removed coverage thresholds
- upload-flow.spec: deleted (paired with the revert)
- event-bus.spec: dropped the image-upload-error contract test

Inspector specs now drive real DOM events instead of asserting
on mock callbacks:
- padding/background/border/column-spacing/size: fire change +
  blur on the actual rendered inputs and assert the dispatched
  callback args + resulting style/attr state.
- Added coercion assertions (padding 'invalid' -> '0', cellspacing
  undefined/null/'' -> '0') against the rendered input value, not
  just no-throw.
- border: per-side test now drives the actual input, verifying
  only the targeted side changes.

Slash command spec:
- Refactored to describe.each(command).
- Replaced the chain-spy fake editor with a real createTestEditor
  and asserted editor.getJSON() contains the expected node type
  per command.
- Heading levels now checked via the heading node's attrs.level
  instead of a setNode call spy.

Extension specs:
- Simplified all 'it()' titles to 'renders the snapshot' style,
  regenerated snapshots accordingly.

Breadcrumb spec:
- Removed; will be re-landed as a __tests__ component test using
  a real editor in a follow-up.

https://claude.ai/code/session_011cESBQ7BczntHHkyozJ9Gv
Copy link
Copy Markdown
Member Author

Addressed the review in b9fec52.

Out-of-scope reverts (deferred to a follow-up PR):

  • upload-flow.ts — restored original implementation
  • event-bus.ts — dropped image-upload-error event
  • upload-flow.spec.ts — deleted (paired with the revert)
  • event-bus.spec.ts — dropped the image-upload-error contract test
  • email-editor.spec.tsx — removed the theme-remount it.skip
  • vitest.config.ts — removed coverage thresholds + @vitest/coverage-v8 dep

Inspector specs now drive real DOM events:

  • padding, background, border, column-spacing, size — fire change + blur on the actual rendered inputs and assert dispatched callback args and resulting style/attr state.
  • Coercion assertions now check the rendered input value (padding 'invalid' → '0', cellspacing undefined/null/'' → '0') instead of just not.toThrow().
  • border per-side test drives the actual input and verifies only the targeted side changes.
  • Hex/color changes go through the real [data-re-inspector-color-hex] input via fireEvent.change.
  • column-spacing "renders nothing" case uses toBe('').

Slash command spec:

  • Replaced the chain-spy fake editor with a real createTestEditor; each command now asserts editor.getJSON() contains the expected node type.
  • Wrapped the per-command tests in describe.each(COMMAND_TABLE).
  • Heading levels are now verified via the heading node's attrs.level instead of a setNode call spy.

Extension specs:

  • All it() titles simplified to "renders the snapshot" form; snapshots regenerated. There's no undefined rendered in the snap files — what reads as line-broken in underline.spec.tsx.snap is React Email's <!--$--> SSR boundary marker.

Breadcrumb spec:

  • Removed. Will be re-landed as a __tests__/*.browser.spec.tsx component test using a real editor in the follow-up.

Remaining notes:

  • context-helpers.ts is kept as a small typed props builder; the inspector specs that use it now drive real DOM events on top of it.
  • compose-react-email.spec.tsx:545fc.assert is intentional (fast-check property test). The expect() inside the asyncProperty body is still Vitest.
  • The vi.mock('@/actions/ai', ...) lines are required for any spec that imports a module that transitively reaches @/actions/ai (the inspector and editor specs do). Tests crash without them.

Tests: 625 passing, 1 skipped, 73 files. Lint clean on changed files.


Generated by Claude Code

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 37 files (changes from recent commits).

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="packages/editor/src/plugins/image/upload-flow.ts">

<violation number="1">
P2: Only the first matching blob image is swapped/removed, leaving duplicate placeholders with revoked blob URLs.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

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.

4 participants