fix: optimize HtmlArtifactsPopup re-rendering#13133
Conversation
There was a problem hiding this comment.
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.
DeJeune
left a comment
There was a problem hiding this comment.
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:
-
Bug:
splitSizestype mismatch — Initialized as["50%", "50%"]butonResizewritesnumber[](pixels). After the first drag, toggling fullscreen will break the layout since pixel values don't adapt to the new container size. -
key={html}still forces iframe remount — This contradicts the stated optimization goal. Consider removing the key and relying onsrcDocupdates instead.
Minor:
- Extra leading space in
PreviewFramestyled component ^6.3.2should be pinned to6.3.2to match project convention for@codemirror/*packages
src/renderer/src/components/CodeBlockView/HtmlArtifactsPopup.tsx
Outdated
Show resolved
Hide resolved
| const PreviewPanel = memo<PreviewPanelProps>(({ previewFrameRef, html, previewTitle, emptyText }) => { | ||
| return ( | ||
| <PreviewSection> | ||
| {html.trim() ? ( |
There was a problem hiding this comment.
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.
src/renderer/src/components/CodeBlockView/HtmlArtifactsPopup.tsx
Outdated
Show resolved
Hide resolved
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", |
There was a problem hiding this comment.
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.
### 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 ```
## 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 ```
### 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 ```
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
memocomponents to prevent unnecessary re-renders from parent state changesSplitterthat keeps both panels always mounted, toggling visibility via controlled panel sizessplitSizesstateAdditional Fixes
@codemirror/lang-liquidpeer dependency required by@uiw/codemirror-extensions-langsfor HTML syntax highlightingwhitebackground withvar(--color-background)for proper dark theme support