fix(Mermaid): improve preformance when editing mermaid diagram in wysiwyg mode#1158
Open
d3m1d0v wants to merge 3 commits into
Open
fix(Mermaid): improve preformance when editing mermaid diagram in wysiwyg mode#1158d3m1d0v wants to merge 3 commits into
d3m1d0v wants to merge 3 commits into
Conversation
Reviewer's GuideImproves Mermaid WYSIWYG editing performance by debouncing textarea updates and deferring/canceling Mermaid rendering, plus adjusts Playwright waits for multiple loaders and adds minimal styling for new loading states. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
d3m1d0v
commented
Jun 23, 2026
|
|
|
|
d3m1d0v
force-pushed
the
fix-mermaid-lags
branch
from
c330ed3 to
b200fe0
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
MermaidPreviewtheuseEffectsetsupdatingtotrueunconditionally but never resets it ifmermaidInstanceis falsy (the early return inrunskips thesetUpdating(false)paths), which can leave the overlay stuck in a loading state; consider short‑circuiting beforesetUpdating(true)or explicitly resettingupdatingwhenmermaidInstanceis not available. - The new
allMaphelper indemo/tests/playwright/core/wait.tscallslocator.all()and thenPromise.allover the resulting list; if the intent is to wait for all current and future loaders to disappear, this will only cover the current snapshot—consider whether you instead want to wait on a single locator with a composite selector or poll untilcount()is zero. - In
DelayedTextArea, theonChangeProxyrelies onevent.type === 'click'andevent.target.value === ''whenhasClearis set; it might be safer to narrow the event target type and/or rely on the component’s clear callback semantics (if available) to avoid edge cases where non‑clear clicks or different event targets could satisfy this condition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MermaidPreview` the `useEffect` sets `updating` to `true` unconditionally but never resets it if `mermaidInstance` is falsy (the early return in `run` skips the `setUpdating(false)` paths), which can leave the overlay stuck in a loading state; consider short‑circuiting before `setUpdating(true)` or explicitly resetting `updating` when `mermaidInstance` is not available.
- The new `allMap` helper in `demo/tests/playwright/core/wait.ts` calls `locator.all()` and then `Promise.all` over the resulting list; if the intent is to wait for all current and future loaders to disappear, this will only cover the current snapshot—consider whether you instead want to wait on a single locator with a composite selector or poll until `count()` is zero.
- In `DelayedTextArea`, the `onChangeProxy` relies on `event.type === 'click'` and `event.target.value === ''` when `hasClear` is set; it might be safer to narrow the event target type and/or rely on the component’s clear callback semantics (if available) to avoid edge cases where non‑clear clicks or different event targets could satisfy this condition.
## Individual Comments
### Comment 1
<location path="packages/editor/src/extensions/additional/Mermaid/MermaidNodeView/MermaidView.tsx" line_range="44-45" />
<code_context>
+ let cancelled = false;
+ setUpdating(true);
+
+ const run = async () => {
+ if (!mermaidInstance) return;
+ try {
+ // Validates syntax and throws error if text is invalid
</code_context>
<issue_to_address>
**issue (bug_risk):** The `updating` flag can get stuck as `true` when `mermaidInstance` is falsy.
Because `setUpdating(true)` is called before `run`, and `run` returns early when `mermaidInstance` is falsy, `setUpdating(false)` is never reached in that case (e.g., when the instance becomes `null`/`undefined` after a prop change). Ensure `updating` is reset by handling the `!mermaidInstance` case explicitly in the effect, or by using a `finally` block so `setUpdating(false)` always runs when the effect finishes (and isn’t cancelled).
</issue_to_address>
### Comment 2
<location path="packages/editor/src/react-utils/hooks/useDelayedValue.ts" line_range="25-33" />
<code_context>
+) {
+ const [currentValue, setCurrentValue] = useState<TValue>(value);
+
+ const timeoutRef = useRef<number | undefined>(undefined);
+
+ const delayedOnChange = useCallback(
+ (nextValue: TValue) => {
+ setCurrentValue(nextValue);
+
+ window.clearTimeout(timeoutRef.current);
+
+ timeoutRef.current = window.setTimeout(() => {
+ onChange(nextValue);
+ }, delay);
</code_context>
<issue_to_address>
**issue (bug_risk):** Timeout created in `useDelayedValue` is never cleared on unmount.
Because the timeout is only cleared when scheduling a new one, a pending timeout can still fire after the component unmounts and call `onChange`. Add a `useEffect` cleanup that calls `window.clearTimeout(timeoutRef.current)` to prevent callbacks from running after unmount and to avoid potential leaks.
</issue_to_address>
### Comment 3
<location path="packages/editor/src/extensions/additional/Mermaid/MermaidNodeView/MermaidView.tsx" line_range="85-92" />
<code_context>
return (
- <div className={b('Preview')}>
+ <div className={b('Preview', {loading: !svg, updating})}>
{svg ? <div className="mermaid" dangerouslySetInnerHTML={{__html: svg}} /> : <Loader />}
+ <Overlay visible={updating}>
+ <Loader />
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Two loaders can be rendered at the same time when `updating` is true and `svg` is falsy.
With `svg` falsy and `updating` true, both the base `Loader` and the overlay `Loader` render. If only one spinner should be visible, adjust the conditions—for example, hide the base loader while `updating`, or only show the overlay during updates.
```suggestion
return (
<div className={b('Preview', {loading: !svg, updating})}>
{svg ? (
<div className="mermaid" dangerouslySetInnerHTML={{__html: svg}} />
) : (
!updating && <Loader />
)}
<Overlay visible={updating}>
<Loader />
</Overlay>
</div>
);
```
</issue_to_address>
### Comment 4
<location path="packages/editor/src/react-utils/components/DelayedTextArea.tsx" line_range="12" />
<code_context>
+
+import {useDelayedValue} from '../hooks/useDelayedValue';
+
+type DelayedTextAreaProps = Required<Pick<TextAreaProps, 'onUpdate' | 'value'>> &
+ Omit<TextAreaProps, 'onUpdate' | 'defaultValue'> & {delay?: number};
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the wrapper by tightening the props typing and composing the clear-logic with the original onChange handler to make behavior more explicit and easier to follow.
You can reduce complexity in this wrapper while keeping the behavior by simplifying the props typing and making the clear logic less “hidden” and more composable.
**1. Simplify `DelayedTextAreaProps`**
The current `Required<Pick<...>> & Omit<...>` shape adds cognitive load. You can keep `value` and `onUpdate` required without the indirection:
```ts
type DelayedTextAreaProps = TextAreaProps & {
value: NonNullable<TextAreaProps['value']>;
onUpdate: NonNullable<TextAreaProps['onUpdate']>;
delay?: number;
};
```
This removes the need to reason about `Pick`/`Omit` and still guarantees `value`/`onUpdate` are present.
**2. Make `onChange` composition explicit**
Right now `onChangeProxy` completely replaces `TextArea`’s `onChange` except in the clear case, which is non-obvious and brittle. You can compose the clear special-case with the original `onChange` so consumers still get their handler called for all events:
```ts
function DelayedTextAreaComponent(
props: DelayedTextAreaProps,
ref: React.Ref<HTMLSpanElement>,
) {
const {value, onUpdate, delay, onChange, hasClear, ...textAreaProps} = props;
const {currentValue: textInputValue, delayedOnChange} = useDelayedValue(
value,
onUpdate,
delay,
);
const handleChange: TextAreaProps['onChange'] = (event) => {
// Always call the original onChange
onChange?.(event);
// Special-case native clear
if (hasClear && event.type === 'click' && event.target.value === '') {
onUpdate('');
}
};
return (
<TextArea
{...textAreaProps}
hasClear={hasClear}
ref={ref}
onChange={handleChange}
value={textInputValue}
onUpdate={delayedOnChange}
/>
);
}
```
This keeps the clear behavior, but:
- The prop surface is simpler to read.
- The interaction between `onChange`/`onUpdate` is explicit and local.
- Consumers don’t lose `onChange` events except in a hidden branch.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 Sourcery
Improve Mermaid diagram editing performance and responsiveness in the editor and adjust related tests and styles.
New Features:
Bug Fixes:
Enhancements: