Skip to content

fix: optimize HtmlArtifactsPopup re-rendering#13133

Merged
DeJeune merged 3 commits intoCherryHQ:mainfrom
kovsu:fix-preview
Mar 2, 2026
Merged

fix: optimize HtmlArtifactsPopup re-rendering#13133
DeJeune merged 3 commits intoCherryHQ:mainfrom
kovsu:fix-preview

Conversation

@kovsu
Copy link
Copy Markdown
Collaborator

@kovsu kovsu commented Mar 2, 2026

Problem

Switching between code/preview/split modes in HtmlArtifactsPopup caused full component unmount/remount due to the switch-based renderContent(), leading to iframe reloads and CodeMirror re-initialization.

Solution

  • Extract CodePanel and PreviewPanel into memo components to prevent unnecessary re-renders from parent state changes
  • Replace switch-based rendering with a controlled Splitter that keeps both panels always mounted, toggling visibility via controlled panel sizes
  • Remember user's custom split position via splitSizes state

Additional Fixes

  • Add missing @codemirror/lang-liquid peer dependency required by @uiw/codemirror-extensions-langs for HTML syntax highlighting
  • Replace hardcoded white background with var(--color-background) for proper dark theme support

@kovsu kovsu requested a review from EurFelux March 2, 2026 09:52
Copy link
Copy Markdown
Collaborator

@EurFelux EurFelux left a comment

Choose a reason for hiding this comment

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

Overall looks good.

Positives:

  • The refactor keeps code/preview panels mounted across mode switches, which avoids unnecessary iframe and editor remounts.
  • Split size persistence improves usability in split mode.
  • Theme background update removes hardcoded white and better aligns with dark mode support.

I did not find blocking correctness or security issues in this change set.

Copy link
Copy Markdown
Collaborator

@DeJeune DeJeune left a comment

Choose a reason for hiding this comment

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

Good approach — keeping both panels always mounted via a single Splitter with controlled sizes is the right idea to avoid iframe/CodeMirror teardown on mode switch. The dark theme fix and missing peer dep addition are welcome.

Issues to address:

  1. Bug: splitSizes type mismatch — Initialized as ["50%", "50%"] but onResize writes number[] (pixels). After the first drag, toggling fullscreen will break the layout since pixel values don't adapt to the new container size.

  2. key={html} still forces iframe remount — This contradicts the stated optimization goal. Consider removing the key and relying on srcDoc updates instead.

Minor:

  • Extra leading space in PreviewFrame styled component
  • ^6.3.2 should be pinned to 6.3.2 to match project convention for @codemirror/* packages

const PreviewPanel = memo<PreviewPanelProps>(({ previewFrameRef, html, previewTitle, emptyText }) => {
return (
<PreviewSection>
{html.trim() ? (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggestion: key={html} forces React to destroy and recreate the iframe every time html changes, which contradicts the goal of keeping panels always mounted to avoid iframe reloads. The memo wrapper on PreviewPanel also becomes ineffective since any html change triggers a full remount via the key.

If the intent is to update the preview without remounting, consider removing key={html} and updating srcDoc directly — the browser will re-render the iframe content when srcDoc changes without a full DOM teardown.

package.json Outdated
"@cherrystudio/extension-table-plus": "workspace:^",
"@cherrystudio/openai": "6.15.0",
"@codemirror/lang-json": "6.0.1",
"@codemirror/lang-liquid": "^6.3.2",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: Other @codemirror/* packages in this project use pinned exact versions (e.g., "6.0.1", "6.8.5"). Consider pinning this to "6.3.2" instead of "^6.3.2" for consistency.

@kovsu kovsu requested a review from DeJeune March 2, 2026 12:38
Copy link
Copy Markdown
Collaborator

@DeJeune DeJeune left a comment

Choose a reason for hiding this comment

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

LGTM

@DeJeune DeJeune merged commit 46ffa81 into CherryHQ:main Mar 2, 2026
9 checks passed
@kovsu kovsu deleted the fix-preview branch March 2, 2026 14:22
GeorgeDong32 pushed a commit that referenced this pull request Mar 3, 2026
### What this PR does

Before this PR:
`@uiw/codemirror-extensions-langs`, `@uiw/codemirror-themes-all`, and
`@uiw/react-codemirror` were pinned at version 4.25.1 to work around a
phantom dependency issue.

After this PR:
- All three `@uiw/codemirror-*` packages are upgraded to 4.25.7 with
exact version pinning (no `^` prefix). The phantom dependency issue has
been resolved upstream, making this upgrade safe.
- Added `@codemirror/state` to `pnpm.overrides` to resolve version
duplication, and updated the overrides for `@codemirror/view`,
`@codemirror/language`, and `@codemirror/lint` to their latest versions.

Fixes #12829

### Why we need it and why it was done in this way

The following tradeoffs were made:
- Exact version pinning (removing `^`) is used to prevent unexpected
automatic upgrades that could reintroduce dependency issues.
- `pnpm.overrides` is used to enforce a single version of each
`@codemirror/*` core package across the entire dependency tree. Without
this, multiple versions coexist due to different sub-dependencies
resolving independently, which causes TypeScript errors because
CodeMirror uses private class properties for nominal typing.
- After the update, `@codemirror/state` has multiple dependency versions
(6.5.3 and 6.5.4 coexisting), which declare different types for the same
type name, causing TypeScript errors. Therefore, an override was added
for this dependency.

The following alternatives were considered:
- Using `^4.25.7` (range specifier) was considered but rejected in favor
of exact pinning for more predictable dependency resolution.
- Considered removing the override, but dependency analysis is difficult
to handle because the previous override has already left a fixed version
in the lock file. Deleting the lock file and regenerating it would cause
extensive changes.

Links to places where the discussion took place:
- #12829 — original issue that pinned versions to 4.25.1
- #13133 — also referenced the phantom dependency problem
- uiwjs/react-codemirror#760

### Breaking changes

None.

### Special notes for your reviewer

The `@codemirror/*` override versions are updated as follows:

| Package | Old | New |
|---|---|---|
| `@codemirror/state` | _(none)_ | 6.5.4 |
| `@codemirror/view` | 6.38.1 | 6.39.16 |
| `@codemirror/language` | 6.11.3 | 6.12.2 |
| `@codemirror/lint` | 6.8.5 | 6.9.5 |

### Checklist

- [x] PR: The PR description is expressive enough and will help future
contributors
- [x] Code: [Write code that humans can
understand](https://en.wikiquote.org/wiki/Martin_Fowler#code-for-humans)
and [Keep it simple](https://en.wikipedia.org/wiki/KISS_principle)
- [x] Refactor: You have [left the code cleaner than you found it (Boy
Scout
Rule)](https://learning.oreilly.com/library/view/97-things-every/9780596809515/ch08.html)
- [x] Upgrade: Impact of this change on upgrade flows was considered and
addressed if required
- [ ] Documentation: A [user-guide update](https://docs.cherry-ai.com)
was considered and is present (link) or not required. Check this only
when the PR introduces or changes a user-facing feature or behavior.
- [ ] Self-review: I have reviewed my own code (e.g., via
[`/gh-pr-review`](/.claude/skills/gh-pr-review/SKILL.md), `gh pr diff`,
or GitHub UI) before requesting review from others

### Release note

```release-note
NONE
```
EurFelux pushed a commit that referenced this pull request Mar 3, 2026
## Problem
Switching between code/preview/split modes in HtmlArtifactsPopup caused
full component unmount/remount due to the switch-based
`renderContent()`, leading to iframe reloads and CodeMirror
re-initialization.

## Solution
- Extract CodePanel and PreviewPanel into `memo` components to prevent
unnecessary re-renders from parent state changes
- Replace switch-based rendering with a controlled `Splitter` that keeps
both panels always mounted, toggling visibility via controlled panel
sizes
- Remember user's custom split position via `splitSizes` state

## Additional Fixes
- Add missing `@codemirror/lang-liquid` peer dependency required by
`@uiw/codemirror-extensions-langs` for HTML syntax highlighting
- Replace hardcoded `white` background with `var(--color-background)`
for proper dark theme support
```release-note
fix: optimize HtmlArtifactsPopup re-rendering
```
EurFelux added a commit that referenced this pull request Mar 3, 2026
### What this PR does

Before this PR:
`@uiw/codemirror-extensions-langs`, `@uiw/codemirror-themes-all`, and
`@uiw/react-codemirror` were pinned at version 4.25.1 to work around a
phantom dependency issue.

After this PR:
- All three `@uiw/codemirror-*` packages are upgraded to 4.25.7 with
exact version pinning (no `^` prefix). The phantom dependency issue has
been resolved upstream, making this upgrade safe.
- Added `@codemirror/state` to `pnpm.overrides` to resolve version
duplication, and updated the overrides for `@codemirror/view`,
`@codemirror/language`, and `@codemirror/lint` to their latest versions.

Fixes #12829

### Why we need it and why it was done in this way

The following tradeoffs were made:
- Exact version pinning (removing `^`) is used to prevent unexpected
automatic upgrades that could reintroduce dependency issues.
- `pnpm.overrides` is used to enforce a single version of each
`@codemirror/*` core package across the entire dependency tree. Without
this, multiple versions coexist due to different sub-dependencies
resolving independently, which causes TypeScript errors because
CodeMirror uses private class properties for nominal typing.
- After the update, `@codemirror/state` has multiple dependency versions
(6.5.3 and 6.5.4 coexisting), which declare different types for the same
type name, causing TypeScript errors. Therefore, an override was added
for this dependency.

The following alternatives were considered:
- Using `^4.25.7` (range specifier) was considered but rejected in favor
of exact pinning for more predictable dependency resolution.
- Considered removing the override, but dependency analysis is difficult
to handle because the previous override has already left a fixed version
in the lock file. Deleting the lock file and regenerating it would cause
extensive changes.

Links to places where the discussion took place:
- #12829 — original issue that pinned versions to 4.25.1
- #13133 — also referenced the phantom dependency problem
- uiwjs/react-codemirror#760

### Breaking changes

None.

### Special notes for your reviewer

The `@codemirror/*` override versions are updated as follows:

| Package | Old | New |
|---|---|---|
| `@codemirror/state` | _(none)_ | 6.5.4 |
| `@codemirror/view` | 6.38.1 | 6.39.16 |
| `@codemirror/language` | 6.11.3 | 6.12.2 |
| `@codemirror/lint` | 6.8.5 | 6.9.5 |

### Checklist

- [x] PR: The PR description is expressive enough and will help future
contributors
- [x] Code: [Write code that humans can
understand](https://en.wikiquote.org/wiki/Martin_Fowler#code-for-humans)
and [Keep it simple](https://en.wikipedia.org/wiki/KISS_principle)
- [x] Refactor: You have [left the code cleaner than you found it (Boy
Scout
Rule)](https://learning.oreilly.com/library/view/97-things-every/9780596809515/ch08.html)
- [x] Upgrade: Impact of this change on upgrade flows was considered and
addressed if required
- [ ] Documentation: A [user-guide update](https://docs.cherry-ai.com)
was considered and is present (link) or not required. Check this only
when the PR introduces or changes a user-facing feature or behavior.
- [ ] Self-review: I have reviewed my own code (e.g., via
[`/gh-pr-review`](/.claude/skills/gh-pr-review/SKILL.md), `gh pr diff`,
or GitHub UI) before requesting review from others

### Release note

```release-note
NONE
```
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.

3 participants