Skip to content

fix(editor): preserve custom HTML by blocking lossy visual conversion - #362

Open
heyramzi wants to merge 6 commits into
useplunk:nextfrom
heyramzi:fix/protect-custom-html-mode-switch
Open

fix(editor): preserve custom HTML by blocking lossy visual conversion#362
heyramzi wants to merge 6 commits into
useplunk:nextfrom
heyramzi:fix/protect-custom-html-mode-switch

Conversation

@heyramzi

Copy link
Copy Markdown

Summary

Prevent templates with unsupported custom HTML from switching into the visual editor, where the markup would be normalized into a lossy TipTap representation.

Problem

The editor already detects custom HTML and warns that visual mode cannot preserve it faithfully. But the current dialog still offers a destructive Switch Anyway path, which makes it too easy to mutate production email markup accidentally.

This is especially risky for templates that depend on:

  • inline styles
  • CSS classes
  • custom attributes
  • table-based layout
  • wrapper elements that are valid for email HTML but not represented in the visual editor

What changed

  • extracted mode-switch decisions into a small pure helper
  • kept HTML mode authoritative for unsupported templates
  • removed the destructive Switch Anyway path from the custom HTML warning dialog
  • clarified dialog copy so the user understands why the template remains locked to HTML mode
  • added focused tests covering initial mode detection and blocked visual-mode switching for custom HTML

Why this approach

The safe path already exists: users can continue editing in HTML mode and still preview the rendered email without converting the source into visual-editor content. Blocking the destructive transition preserves the original template instead of asking users to make an unsafe choice.

Verification

  • yarn vitest run --config ./vitest.mode-guards.config.cjs apps/web/__tests__/EmailEditor/modeGuards.test.ts

Note: the repository's default Vitest setup requires a database and runs global DB initialization for all suites, so I used a minimal temporary config to verify this pure unit test in isolation.

@driaug

Copy link
Copy Markdown
Member

The reason the destructive action is there in case there is an edge where a template gets incorrectly identified but is parseable.

This would allow the user to switch between modes, in your solution this is no longer possible. To completely lose a template you would need to press the destructive button AND save the template. You can always refresh the page to discard the changes.

Instead of blocking the HTML→Visual switch for custom HTML templates,
allow the switch but snapshot the original HTML. A revert banner lets
the user restore the original markup if the visual conversion dropped
elements. This addresses the maintainer's concern about false positives
in custom HTML detection while still protecting against accidental loss.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@heyramzi

Copy link
Copy Markdown
Author

Good point about false positives in the detection. I've revised the approach:

Before: Block the switch entirely when custom HTML is detected.
Now: Allow the switch, but snapshot the original HTML first. A small banner appears at the top of the visual editor with a one-click "Revert to HTML" button that restores the exact original markup.

This way:

  • Users can always switch modes (your concern about incorrectly flagged templates)
  • The original HTML is never lost until the user explicitly dismisses the banner or saves (my concern about accidental destruction)
  • No modal dialog, no friction for the happy path
  • Detection accuracy becomes less critical since the action is always reversible

The snapshot is cleared when switching back to HTML mode or when the user dismisses the banner.

heyramziand others added 4 commits June 26, 2026 13:59
Cleared all CVE-class advisories surfaced by `yarn npm audit` via yarn
resolutions (range-scoped) plus direct dev-dep bumps. Applied from an
isolated worktree at HEAD because the main working tree holds an unrelated
yarn->pnpm migration WIP.
Audit: 4 critical / 15 high / 22 moderate / 8 low -> only 1 low residual.
Resolutions / bumps:
- next ^16.2.6 (critical/high/moderate; direct)
- vitest ^4.1.0, turbo ^2.9.14, form-data ^4.0.6 (direct dev deps)
- shell-quote ^1.9.0, fast-xml-parser ^5.7.0 (critical)
- effect ^3.21.4, fast-uri ^4.0.0, multer ^2.2.0, nodemailer ^9.0.1,
ws ^8.21.0, serialize-javascript ^7.0.5, undici ^6.27.0, vite ^7.3.5,
picomatch ^4.0.4, minimatch (3.1.4/9.0.7/10.2.3), path-to-regexp
(0.1.13/8.4.0), @isaacs/brace-expansion ^5.0.1 (high)
- dompurify ^3.4.11, ajv ^8.18.0, brace-expansion (1.1.13/2.0.3),
ip-address ^10.2.0, js-yaml ^4.2.0, markdown-it ^14.2.0, postcss ^8.5.10,
qs ^6.15.3, tar ^7.5.17, uuid ^11.1.1, yaml ^2.8.3, esbuild ^0.28.1
(moderate/low)
Residual: @babel/core <=7.29.0 (low) - fix exists only in 8.0 major, which
would force a breaking babel-ecosystem upgrade; left unbumped to keep build
green. Deprecation-only notices (@react-email/*, @overnightjs/core,
@types/dompurify, glob, etc.) are not CVEs and were not touched.
Build: `yarn build` (turbo, 9/9 tasks) passes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regenerated from vibe-kit/ai-doc/references/agents-core.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four type-evidence rules ported from dmmulroy/anti-slop ship in 2.8.4:
no-chained-type-assertions, no-conditional-empty-object-spread and
no-known-value-widening at warn, require-safety-comment-for-type-assertion
off and opt-in.
Every no-known-value-widening site here is fixed by keeping inference,
using satisfies, or naming the real contract. No assertions were added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds fallow@^3.18.0 with .fallowrc.json (workspaces apps/*, packages/*;
explicit entry for the Express api/smtp apps and the wiki's docs-generation
scripts, which aren't reached by any Next.js route convention) and the five
fallow:* scripts.
Note on this diff's shape: apps/api, apps/landing, apps/web, apps/wiki,
packages/email, packages/ui, the root package.json, and pnpm-lock.yaml were
already carrying a staged (uncommitted) yarn-to-pnpm package-manager
migration before this session touched them. That migration is unrelated to
fallow and is not this commit's work; it is included here only because git
has no way to commit a package.json edit without also committing whatever
else was already staged in that same file. Untouched migration-only files
(yarn.lock, .impeccable.md, .yarn/releases, packages/db, packages/eslint-
config, packages/shared, packages/types, pnpm-workspace.yaml, renovate.json,
DESIGN.md, CLAUDE.md) were left exactly as they were, unstaged/untracked,
for that migration to be reviewed and committed on its own.
The fallow-attributable dependency changes, verified by grep against the
whole repo (not just each app) before touching anything:
Removed (zero imports anywhere):
- apps/api: @react-email/render, body-parser, mailchecker
- apps/web: @tiptap/extension-image (apps/landing already declares its own
copy), @tiptap/extension-table/-cell/-header/-row, react-intl
- apps/wiki: shiki (fumadocs-core already depends on it directly)
- packages/email: @react-email/tailwind (@react-email/components already
depends on it directly), @react-email/preview-server (deprecated, and
react-email 6.x doesn't depend on it — a leftover from an older CLI)
- packages/ui: @dnd-kit/core|modifiers|sortable, @number-flow/react,
@radix-ui/react-accordion|alert-dialog|menubar|navigation-menu|
scroll-area|slider, date-fns, embla-carousel-react, next-themes,
react-day-picker, sonner (apps/web and apps/landing already declare
their own copies), @tailwindcss/forms (apps/web and apps/landing already
declare their own copies) — shadcn-scaffolded primitives never wired
into a page
- root: form-data (was a yarn "resolutions" pin for a CVE; the new pnpm
overrides block dropped it, so it's now dead weight), supertest and its
@types/supertest (a comment mentions it, no file imports it)
Added (real imports, wrong or missing package.json):
- apps/api: react, zod (used directly; apps/api never declared either)
- apps/web: @hookform/resolvers (8 files; only packages/ui had it),
tailwindcss-animate (globals.css `@plugin`; only packages/ui had it)
- apps/landing: swr, framer-motion, zod (all three imported directly;
only apps/web declared them)
- apps/wiki: class-variance-authority, tailwind-merge (only packages/ui
had them)
- root (devDependencies, test-only): bcrypt, bullmq, dayjs, ioredis — the
root vitest suite's test/helpers/*.ts and test/mocks/*.ts import these
directly; only apps/api declared them, which doesn't help imports
resolved from the repo root under pnpm's non-hoisted linking. This one
is likely a real regression from the yarn migration (yarn hoists
everything to the root node_modules by default; pnpm doesn't).
ignoreDependencies, not touched (real but invisible to a static import
scan): @eslint/js and typescript-eslint, imported only from apps/api's and
apps/smtp's eslint.config.mjs, and dotenv, imported only from apps/wiki's
generate-openapi.js build script — all three resolve via Node's
parent-directory node_modules walk-up to the root's own devDependency,
matching this monorepo's existing deliberate convention of keeping shared
tooling (eslint, typescript, prettier, turbo) declared only at the root.
ignoreExports: apps/wiki/app/llms.mdx/**/route.ts's generateStaticParams —
a Next.js App Router build-time hook fallow's Next.js rule recognizes on
page.tsx/layout.tsx but not on a route.ts handler.
ignoreUnresolvedImports: '../../services/ses' in
WorkflowExecutionService.integration.test.ts — a stale vi.mock path (the
real file is ./SESService.ts exporting sendRawEmail, not an `ses` object).
Left for a human: fixing the path alone wouldn't fix the mismatched mock
shape, and this repo's test suite is already fully red for an unrelated
reason (Prisma client not generated), so I can't verify a fix here.
Left alone (reported, not fixed): 1 circular dependency among
EmailService/EventService/WorkflowExecutionService (warn-level,
pre-existing) — a real refactor, out of scope for a dead-code pass. 332
health/complexity findings and 26.3% duplicated lines were baselined
as-is (pre-existing, not dead code).
Gates: `pnpm test:run` and `pnpm lint` were already fully red before this
change (Prisma client not generated; apps/wiki's eslint config can't
resolve @plunk/eslint-config) — both pre-existing and unrelated to fallow,
unchanged by this commit. `fallow dead-code` and `fallow audit` are clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for freeto 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

@heyramzi@driaug