Skip to content
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"@dnd-kit/sortable": "^10.0.0",
"@dnd-kit/utilities": "^3.2.2",
"@mantine/hooks": "^9.0.0",
"@monaco-editor/react": "^4.7.0",
"@octokit/rest": "^22.0.0",
"@reduxjs/toolkit": "^1.9.7",
"@sentry/react": "^10.5.0",
Expand Down Expand Up @@ -77,6 +78,7 @@
"lz-string": "^1.4.4",
"mdast-util-from-markdown": "^2.0.0",
"mdast-util-to-hast": "^13.0.0",
"monaco-editor": "^0.55.1",
"normalize.css": "^8.0.1",
"phaser": "~3.90.0",
"query-string": "^9.0.0",
Expand Down
20 changes: 16 additions & 4 deletions src/commons/editor/EditorContainer.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import _ from 'lodash';
import { useContext } from 'react';
import { lazy, Suspense, useContext } from 'react';
import { flagMonacoEditorEnable } from 'src/features/monaco/flagMonacoEditorEnable';

import { useFeature } from '../featureFlags/useFeature';
import type { EditorTabState } from '../workspace/WorkspaceTypes';
import { WorkspaceSettingsContext } from '../WorkspaceSettingsContext';
import Editor, { type EditorProps, type EditorTabStateProps } from './Editor';
import EditorTabContainer from './tabs/EditorTabContainer';

const MonacoEditor = lazy(() => import('./MonacoEditor'));

type OwnProps = {
baseFilePath?: string;
isFolderModeEnabled: boolean;
Expand Down Expand Up @@ -34,13 +38,21 @@ export const convertEditorTabStateToProps = (
};

const createNormalEditorTab =
(editorProps: Omit<EditorProps, keyof EditorTabStateProps>) =>
(editorProps: Omit<EditorProps, keyof EditorTabStateProps>, useMonacoEditor: boolean) =>
(editorTabStateProps: EditorTabStateProps) => {
return <Editor {...editorProps} {...editorTabStateProps} />;
const editorPropsWithTab = { ...editorProps, ...editorTabStateProps };
return useMonacoEditor ? (
<Suspense fallback={null}>
<MonacoEditor {...editorPropsWithTab} />
</Suspense>
) : (
<Editor {...editorPropsWithTab} />
);
};

const EditorContainer: React.FC<EditorContainerProps> = (props: EditorContainerProps) => {
const [workspaceSettings] = useContext(WorkspaceSettingsContext)!;
const useMonacoEditor = useFeature(flagMonacoEditorEnable);
const {
baseFilePath,
isFolderModeEnabled,
Expand All @@ -52,7 +64,7 @@ const EditorContainer: React.FC<EditorContainerProps> = (props: EditorContainerP
} = props;
editorProps.editorBinding = workspaceSettings.editorBinding;

const createEditorTab = createNormalEditorTab(editorProps);
const createEditorTab = createNormalEditorTab(editorProps, useMonacoEditor);

if (activeEditorTabIndex === null) {
return (
Expand Down
73 changes: 73 additions & 0 deletions src/commons/editor/MonacoEditor.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { Card } from '@blueprintjs/core';
import MonacoReactEditor from '@monaco-editor/react';
import { useCallback } from 'react';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Add useMemo to the React imports to support memoizing the editor options.

Suggested change
import { useCallback } from 'react';
import { useCallback, useMemo } from 'react';


import { EditorProps } from './Editor';
import { SOURCE_MONACO_THEME } from './monaco/setupMonaco';

const languageByExtension: Record<string, string> = {
c: 'c',
cc: 'cpp',
cpp: 'cpp',
css: 'css',
h: 'cpp',
html: 'html',
java: 'java',
js: 'javascript',
json: 'json',
jsx: 'javascript',
py: 'python',
ts: 'typescript',
tsx: 'typescript'
};

const getLanguage = ({ filePath, mode }: Pick<EditorProps, 'filePath' | 'mode'>): string => {
if (mode) {
return mode;
}
const extension = filePath?.split('.').pop()?.toLowerCase();
return (extension && languageByExtension[extension]) || 'javascript';
};

const MonacoEditor: React.FC<EditorProps> = props => {
const handleChange = useCallback(
(value: string | undefined, event: unknown) => {
const newValue = value ?? '';
props.handleEditorValueChange(props.editorTabIndex, newValue);
props.handleUpdateHasUnsavedChanges?.(true);
props.onChange?.(newValue, event);
},
[props]
);
Comment on lines +33 to +41
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The useCallback hook uses props as a dependency. Since props is a new object created on every render in the parent EditorContainer, this callback will be recreated on every render, defeating the purpose of memoization. It is better to destructure the specific functions and values needed from props and use them as dependencies.

Suggested change
const handleChange = useCallback(
(value: string | undefined, event: unknown) => {
const newValue = value ?? '';
props.handleEditorValueChange(props.editorTabIndex, newValue);
props.handleUpdateHasUnsavedChanges?.(true);
props.onChange?.(newValue, event);
},
[props]
);
const { handleEditorValueChange, editorTabIndex, handleUpdateHasUnsavedChanges, onChange } = props;
const handleChange = useCallback(
(value: string | undefined, event: unknown) => {
const newValue = value ?? '';
handleEditorValueChange(editorTabIndex, newValue);
handleUpdateHasUnsavedChanges?.(true);
onChange?.(newValue, event);
},
[handleEditorValueChange, editorTabIndex, handleUpdateHasUnsavedChanges, onChange]
);


return (
<Card className="Editor">
<div className="row editor-react-ace" data-testid="Editor" id="editor-react-ace">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using a hardcoded id="editor-react-ace" can lead to duplicate IDs in the DOM if multiple editor tabs are rendered simultaneously. Consider removing the ID if it's not strictly required for external scripts or global CSS targeting.

Suggested change
<div className="row editor-react-ace" data-testid="Editor" id="editor-react-ace">
<div className="row editor-react-ace" data-testid="Editor">

<MonacoReactEditor
className="react-ace"
height="100%"
language={getLanguage(props)}
onChange={handleChange}
options={{
fontFamily: "'Inconsolata', 'Consolas', monospace",
fontSize: 17,
folding: false,
glyphMargin: false,
hover: { enabled: false },
lineHeight: 17,
lineNumbersMinChars: 4,
minimap: { enabled: false },
readOnly: props.sessionDetails?.readOnly ?? false,
renderLineHighlight: 'none',
scrollBeyondLastLine: false,
}}
Comment on lines +51 to +63
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The options object is created inline on every render. This can cause the MonacoReactEditor component to perform unnecessary internal updates. Consider memoizing this object using useMemo.

Suggested change
options={{
fontFamily: "'Inconsolata', 'Consolas', monospace",
fontSize: 17,
folding: false,
glyphMargin: false,
hover: { enabled: false },
lineHeight: 17,
lineNumbersMinChars: 4,
minimap: { enabled: false },
readOnly: props.sessionDetails?.readOnly ?? false,
renderLineHighlight: 'none',
scrollBeyondLastLine: false,
}}
options={useMemo(() => ({
fontFamily: "'Inconsolata', 'Consolas', monospace",
fontSize: 17,
folding: false,
glyphMargin: false,
hover: { enabled: false },
lineHeight: 17,
lineNumbersMinChars: 4,
minimap: { enabled: false },
readOnly: props.sessionDetails?.readOnly ?? false,
renderLineHighlight: 'none',
scrollBeyondLastLine: false,
}), [props.sessionDetails?.readOnly])}

theme={SOURCE_MONACO_THEME}
value={props.editorValue}
width="100%"
/>
</div>
</Card>
);
};

export default MonacoEditor;
98 changes: 98 additions & 0 deletions src/commons/editor/__tests__/EditorContainer.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import { fireEvent, render, screen } from '@testing-library/react';
import { Provider } from 'react-redux';
import { mockInitialStore } from 'src/commons/mocks/StoreMocks';
import {
defaultWorkspaceSettings,
WorkspaceSettingsContext
} from 'src/commons/WorkspaceSettingsContext';
import { flagMonacoEditorEnable } from 'src/features/monaco/flagMonacoEditorEnable';
import { vi } from 'vitest';

import EditorContainer, { EditorContainerProps } from '../EditorContainer';

vi.mock('../MonacoEditor', () => ({
default: (props: any) => (
<textarea
data-testid="MonacoEditorMock"
onChange={event => props.handleEditorValueChange(props.editorTabIndex, event.target.value)}
readOnly={props.sessionDetails?.readOnly ?? false}
value={props.editorValue}
/>
)
}));

const createProps = (overrides: Partial<EditorContainerProps> = {}): EditorContainerProps => ({
activeEditorTabIndex: 0,
editorSessionId: '',
editorTabs: [
{
breakpoints: [],
editorTabIndex: 0,
editorValue: 'const x = 1;',
filePath: '/program.js',
highlightedLines: []
}
],
editorVariant: 'normal',
handleDeclarationNavigate: () => {},
handleEditorEval: () => {},
handleEditorUpdateBreakpoints: () => {},
handleEditorValueChange: () => {},
handlePromptAutocomplete: () => {},
isEditorAutorun: false,
isFolderModeEnabled: false,
removeEditorTabByIndex: () => {},
sessionDetails: null,
setActiveEditorTabIndex: () => {},
...overrides
});

const renderEditorContainer = (
props: EditorContainerProps,
featureFlags: Record<string, any> = {}
) => {
const store = mockInitialStore({
featureFlags: {
modifiedFlags: featureFlags
}
});

return render(
<Provider store={store}>
<WorkspaceSettingsContext.Provider value={[defaultWorkspaceSettings, () => {}]}>
<EditorContainer {...props} />
</WorkspaceSettingsContext.Provider>
</Provider>
);
};

test('EditorContainer renders Ace editor path when Monaco flag is off', () => {
renderEditorContainer(createProps());
expect(screen.getByTestId('Editor')).toBeTruthy();
expect(screen.queryByTestId('MonacoEditorMock')).toBeNull();
});

test('EditorContainer renders Monaco editor path when Monaco flag is on', async () => {
renderEditorContainer(createProps(), {
[flagMonacoEditorEnable.flagName]: true
});
expect(await screen.findByTestId('MonacoEditorMock')).toBeTruthy();
});

test('Monaco editor path forwards value changes with the active editor tab index', async () => {
const handleEditorValueChange = vi.fn();
renderEditorContainer(
createProps({
handleEditorValueChange
}),
{
[flagMonacoEditorEnable.flagName]: true
}
);

fireEvent.change(await screen.findByTestId('MonacoEditorMock'), {
target: { value: 'const y = 2;' }
});

expect(handleEditorValueChange).toHaveBeenCalledWith(0, 'const y = 2;');
});
90 changes: 90 additions & 0 deletions src/commons/editor/__tests__/MonacoEditor.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import { fireEvent, render, screen } from '@testing-library/react';
import { vi } from 'vitest';

import { EditorProps } from '../Editor';
import MonacoEditor from '../MonacoEditor';

vi.mock('monaco-editor', () => ({
editor: {
defineTheme: vi.fn()
}
}));

vi.mock('@monaco-editor/react', () => ({
default: (props: any) => (
<textarea
data-theme={props.theme}
data-testid="MonacoReactEditorMock"
onChange={event => props.onChange(event.target.value, { source: 'test' })}
readOnly={props.options?.readOnly ?? false}
value={props.value}
/>
),
loader: {
config: vi.fn()
}
}));

const createProps = (overrides: Partial<EditorProps> = {}): EditorProps => ({
breakpoints: [],
editorSessionId: '',
editorTabIndex: 0,
editorValue: 'const x = 1;',
handleDeclarationNavigate: () => {},
handleEditorEval: () => {},
handleEditorUpdateBreakpoints: () => {},
handleEditorValueChange: () => {},
handlePromptAutocomplete: () => {},
highlightedLines: [],
isEditorAutorun: false,
sessionDetails: null,
...overrides
});

test('MonacoEditor renders the Monaco React editor wrapper', () => {
render(<MonacoEditor {...createProps()} />);
const editor = screen.getByTestId('MonacoReactEditorMock') as HTMLTextAreaElement;
expect(screen.getByTestId('Editor')).toBeTruthy();
expect(editor.value).toBe('const x = 1;');
expect(editor.dataset.theme).toBe('source');
});

test('MonacoEditor forwards changes to workspace handlers', () => {
const handleEditorValueChange = vi.fn();
const handleUpdateHasUnsavedChanges = vi.fn();
const onChange = vi.fn();

render(
<MonacoEditor
{...createProps({
handleEditorValueChange,
handleUpdateHasUnsavedChanges,
onChange
})}
/>
);

fireEvent.change(screen.getByTestId('MonacoReactEditorMock'), {
target: { value: 'const y = 2;' }
});

expect(handleEditorValueChange).toHaveBeenCalledWith(0, 'const y = 2;');
expect(handleUpdateHasUnsavedChanges).toHaveBeenCalledWith(true);
expect(onChange).toHaveBeenCalledWith('const y = 2;', { source: 'test' });
});

test('MonacoEditor passes session readonly state to Monaco', () => {
render(
<MonacoEditor
{...createProps({
sessionDetails: {
docId: 'doc-id',
owner: false,
readOnly: true
}
})}
/>
);

expect(screen.getByTestId('MonacoReactEditorMock').hasAttribute('readonly')).toBe(true);
});
Loading