Skip to content

Commit

Permalink
[ui] Initialize CodeMirror on a div instead of textarea (#16878)
Browse files Browse the repository at this point in the history
## Summary & Motivation

Handling a bug reported by a user:
https://dagster.slack.com/archives/C01U954MEER/p1695890039223599.

When viewing the "Configuration" page under "Deployment", the read-only
CodeMirror renders correctly. If you navigate away, however, the
CodeMirror remains rendered in the DOM, though unstyled and jammed into
the top of the page.

This is because we're using `CodeMirror.fromTextArea`, which creates a
target div for the CodeMirror instance independent of the `textarea` we
render for the component. This div is uncontrolled by React, so when we
navigate away, the React-controlled textarea is unmounted, but the
CodeMirror target div remains in the DOM.

To resolve this, use the CodeMirror constructor API instead. In this
way, we can pass a div to serve as the CM target, which will be
unmounted correctly when navigating away.

Also switch to using a `useRef` instead of the ref function, which is
currently being executed every time any of the prop values changes --
which is a lot. The ref function re-execution leads to
mounting/unmounting the rendered node, which isn't useful to us and
could be problematic.

The CodeMirror instantiation now takes place within in an effect
instead.

## How I Tested These Changes

View Configuration page, then navigate to Daemons. Verify that the
CodeMirror doesn't linger in the DOM.

View Launchpad, verify that editable CodeMirrors continue to behave
correctly.
  • Loading branch information
hellendag authored Sep 28, 2023
1 parent c9cbe64 commit 538df91
Showing 1 changed file with 33 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,62 +21,53 @@ interface Props {

export const RawCodeMirror = (props: Props) => {
const {value, options, handlers} = props;
const cm = React.useRef<CodeMirror.EditorFromTextArea | null>(null);
const target = React.useRef<HTMLDivElement>(null);
const cm = React.useRef<CodeMirror.Editor | null>(null);

React.useEffect(() => {
if (value !== cm.current?.getValue()) {
cm.current?.setValue(value);
}
}, [value]);

const ref = React.useCallback(
(node: HTMLTextAreaElement | null) => {
if (!node) {
return;
}
React.useEffect(() => {
if (!target.current || cm.current) {
return;
}

if (cm.current) {
return;
}
cm.current = CodeMirror(target.current, {value, ...options});

cm.current = CodeMirror.fromTextArea(node, {
value,
...options,
});
// Wait a moment for the DOM to settle, then call refresh to ensure that all
// CSS has finished loading. This allows CodeMirror to correctly align elements,
// including the cursor.
setTimeout(() => {
cm.current?.refresh();
}, REFRESH_DELAY_MSEC);

// Wait a moment for the DOM to settle, then call refresh to ensure that all
// CSS has finished loading. This allows CodeMirror to correctly align elements,
// including the cursor.
setTimeout(() => {
cm.current?.refresh();
}, REFRESH_DELAY_MSEC);

if (!handlers) {
return;
}
if (!handlers) {
return;
}

if (handlers.onChange) {
cm.current.on('change', handlers.onChange);
}
if (handlers.onChange) {
cm.current.on('change', handlers.onChange);
}

if (handlers.onBlur) {
cm.current.on('blur', handlers.onBlur);
}
if (handlers.onBlur) {
cm.current.on('blur', handlers.onBlur);
}

if (handlers.onCursorActivity) {
cm.current.on('cursorActivity', handlers.onCursorActivity);
}
if (handlers.onCursorActivity) {
cm.current.on('cursorActivity', handlers.onCursorActivity);
}

if (handlers.onKeyUp) {
cm.current.on('keyup', handlers.onKeyUp);
}
if (handlers.onKeyUp) {
cm.current.on('keyup', handlers.onKeyUp);
}

if (handlers.onReady) {
handlers.onReady(cm.current);
}
},
[value, options, handlers],
);
if (handlers.onReady) {
handlers.onReady(cm.current);
}
}, [handlers, options, value]);

React.useEffect(() => {
// Check current options and update if necessary.
Expand All @@ -90,5 +81,5 @@ export const RawCodeMirror = (props: Props) => {
}
}, [options]);

return <textarea ref={ref} />;
return <div ref={target} />;
};

1 comment on commit 538df91

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-oh3ou6ohe-elementl.vercel.app

Built with commit 538df91.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.