Skip to content

Commit 35f4405

Browse files
mscolnickmscolnick
authored
fix: [rtc] edge cases for auto-run and focus (#4861)
1 parent 5c6766e commit 35f4405

File tree

26 files changed

+507
-205
lines changed

26 files changed

+507
-205
lines changed

‎frontend/src/components/editor/Cell.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,10 @@ export interface CellHandle {
338338
* The CodeMirror editor view.
339339
*/
340340
editorView: EditorView;
341+
/**
342+
* The CodeMirror editor view, or null if it is not yet mounted.
343+
*/
344+
editorViewOrNull: EditorView | null;
341345
}
342346

343347
export interface CellProps
@@ -418,6 +422,9 @@ const CellComponent = (
418422
get editorView() {
419423
return derefNotNull(editorView);
420424
},
425+
get editorViewOrNull() {
426+
return editorView.current;
427+
},
421428
}),
422429
[editorView],
423430
);

‎frontend/src/components/editor/cell/code/cell-editor.tsx

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { EditorView, ViewPlugin } from "@codemirror/view";
55
import React, { memo, useEffect, useRef, useMemo } from "react";
66

77
import { setupCodeMirror } from "@/core/codemirror/cm";
8-
import { getFeatureFlag } from "@/core/config/feature-flag";
98
import useEvent from "react-use-event-hook";
109
import { notebookAtom, useCellActions } from "@/core/cells/cells";
1110
import type { CellRuntimeState, CellData } from "@/core/cells/types";
@@ -43,6 +42,7 @@ import { useDeleteCellCallback } from "../useDeleteCell";
4342
import { useSaveNotebook } from "@/core/saving/save-component";
4443
import { DelayMount } from "@/components/utils/delay-mount";
4544
import { Button } from "@/components/ui/button";
45+
import { isRtcEnabled } from "@/core/rtc/state";
4646

4747
export interface CellEditorProps
4848
extends Pick<CellRuntimeState, "status">,
@@ -239,7 +239,7 @@ const CellEditorInternal = ({
239239
saveOrNameNotebook,
240240
]);
241241

242-
const rtcEnabled = getFeatureFlag("rtc_v2");
242+
const rtcEnabled = isRtcEnabled();
243243
const handleInitializeEditor = useEvent(() => {
244244
// If rtc is enabled, use collaborative editing
245245
if (rtcEnabled) {
@@ -365,8 +365,10 @@ const CellEditorInternal = ({
365365
]);
366366

367367
// Auto-focus. Should focus newly created editors.
368+
// We don't focus if RTC is enabled, since other players will be creating editors
368369
const shouldFocus =
369-
editorViewRef.current === null || serializedEditorState !== null;
370+
(editorViewRef.current === null || serializedEditorState !== null) &&
371+
!isRtcEnabled();
370372
useEffect(() => {
371373
// Perf:
372374
// We don't pass this in from the props since it causes lots of re-renders for unrelated cells
@@ -548,6 +550,6 @@ function WithWaitUntilConnected<T extends {}>(
548550
return WaitUntilConnectedComponent;
549551
}
550552

551-
export const CellEditor = getFeatureFlag("rtc_v2")
553+
export const CellEditor = isRtcEnabled()
552554
? WithWaitUntilConnected(memo(CellEditorInternal))
553555
: memo(CellEditorInternal);

‎frontend/src/components/editor/cell/collapse.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,9 @@ export const CollapseToggle: React.FC<Props> = (props) => {
3333
return (
3434
<Button variant="text" size="icon" onClick={props.onClick}>
3535
<Tooltip content={props.isCollapsed ? "Expand" : "Collapse"}>
36-
<Arrow isCollapsed={props.isCollapsed} />
36+
<span>
37+
<Arrow isCollapsed={props.isCollapsed} />
38+
</span>
3739
</Tooltip>
3840
</Button>
3941
);

‎frontend/src/components/editor/chrome/wrapper/footer-items/rtc-status.tsx

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,22 +5,21 @@ import { useAtomValue, useAtom } from "jotai";
55
import { UsersIcon } from "lucide-react";
66
import type React from "react";
77
import { FooterItem } from "../footer-item";
8-
import { getFeatureFlag } from "@/core/config/feature-flag";
98
import {
109
Popover,
1110
PopoverContent,
1211
PopoverTrigger,
1312
} from "@/components/ui/popover";
1413
import { Input } from "@/components/ui/input";
1514
import { useState } from "react";
16-
import { usernameAtom } from "@/core/rtc/state";
15+
import { isRtcEnabled, usernameAtom } from "@/core/rtc/state";
1716

1817
export const RTCStatus: React.FC = () => {
1918
const connectedDoc = useAtomValue(connectedDocAtom);
2019
const [username, setUsername] = useAtom(usernameAtom);
2120
const [open, setOpen] = useState(false);
2221

23-
if (!getFeatureFlag("rtc_v2")) {
22+
if (!isRtcEnabled()) {
2423
return null;
2524
}
2625

‎frontend/src/components/editor/output/MarimoErrorOutput.tsx

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ export const MarimoErrorOutput = ({
142142

143143
if (syntaxErrors.length > 0 || unknownErrors.length > 0) {
144144
messages.push(
145-
<li key="syntax-unknown">
145+
<div key="syntax-unknown">
146146
{syntaxErrors.map((error, idx) => (
147147
<p key={`syntax-${idx}`}>{error.msg}</p>
148148
))}
@@ -155,13 +155,13 @@ export const MarimoErrorOutput = ({
155155
cellId={cellId}
156156
/>
157157
)}
158-
</li>,
158+
</div>,
159159
);
160160
}
161161

162162
if (setupErrors.length > 0) {
163163
messages.push(
164-
<li key="setup-refs">
164+
<div key="setup-refs">
165165
<p className="text-muted-foreground font-medium">
166166
The setup cell cannot be run because it has references.
167167
</p>
@@ -206,13 +206,13 @@ export const MarimoErrorOutput = ({
206206
.
207207
</p>
208208
</Tip>
209-
</li>,
209+
</div>,
210210
);
211211
}
212212

213213
if (cycleErrors.length > 0) {
214214
messages.push(
215-
<li key="cycle">
215+
<div key="cycle">
216216
<p className="text-muted-foreground font-medium">
217217
This cell is in a cycle.
218218
</p>
@@ -255,13 +255,13 @@ export const MarimoErrorOutput = ({
255255
.
256256
</p>
257257
</Tip>
258-
</li>,
258+
</div>,
259259
);
260260
}
261261

262262
if (multipleDefsErrors.length > 0) {
263263
messages.push(
264-
<li key="multiple-defs">
264+
<div key="multiple-defs">
265265
<p className="text-muted-foreground font-medium">
266266
This cell redefines variables from other cells.
267267
</p>
@@ -320,13 +320,13 @@ export const MarimoErrorOutput = ({
320320
<span>to experiment without restrictions on variable names.</span>
321321
</div>
322322
</Tip>
323-
</li>,
323+
</div>,
324324
);
325325
}
326326

327327
if (importStarErrors.length > 0) {
328328
messages.push(
329-
<li key="import-star">
329+
<div key="import-star">
330330
{importStarErrors.map((error, idx) => (
331331
<p key={`import-star-${idx}`} className="text-muted-foreground">
332332
{error.msg}
@@ -361,13 +361,13 @@ export const MarimoErrorOutput = ({
361361
.
362362
</p>
363363
</Tip>
364-
</li>,
364+
</div>,
365365
);
366366
}
367367

368368
if (deleteNonlocalErrors.length > 0) {
369369
messages.push(
370-
<li key="delete-nonlocal">
370+
<div key="delete-nonlocal">
371371
{deleteNonlocalErrors.map((error, idx) => (
372372
<div key={`delete-nonlocal-${idx}`}>
373373
{`The variable '${error.name}' can't be deleted because it was defined by another cell (`}
@@ -385,13 +385,13 @@ export const MarimoErrorOutput = ({
385385
order. Try refactoring so that you can delete variables in the cells
386386
that create them.
387387
</Tip>
388-
</li>,
388+
</div>,
389389
);
390390
}
391391

392392
if (interruptionErrors.length > 0) {
393393
messages.push(
394-
<li key="interruption">
394+
<div key="interruption">
395395
{interruptionErrors.map((_, idx) => (
396396
<p key={`interruption-${idx}`}>
397397
{"This cell was interrupted and needs to be re-run."}
@@ -400,22 +400,22 @@ export const MarimoErrorOutput = ({
400400
{cellId && (
401401
<AutoFixButton errors={interruptionErrors} cellId={cellId} />
402402
)}
403-
</li>,
403+
</div>,
404404
);
405405
}
406406

407407
if (exceptionErrors.length > 0) {
408408
messages.push(
409-
<li key="exception">
409+
<div key="exception">
410410
{exceptionErrors.map((error, idx) => (
411411
<li className="my-2" key={`exception-${idx}`}>
412412
{error.raising_cell == null ? (
413-
<li>
413+
<div>
414414
<p className="text-muted-foreground">{error.msg}</p>
415415
<div className="text-muted-foreground mt-2">
416416
See the console area for a traceback.
417417
</div>
418-
</li>
418+
</div>
419419
) : (
420420
<div>
421421
{error.msg}
@@ -431,13 +431,13 @@ export const MarimoErrorOutput = ({
431431
</Tip>
432432
)}
433433
{cellId && <AutoFixButton errors={exceptionErrors} cellId={cellId} />}
434-
</li>,
434+
</div>,
435435
);
436436
}
437437

438438
if (strictExceptionErrors.length > 0) {
439439
messages.push(
440-
<li key="strict-exception">
440+
<div key="strict-exception">
441441
{strictExceptionErrors.map((error, idx) => (
442442
<li className="my-2" key={`strict-exception-${idx}`}>
443443
{error.blamed_cell == null ? (
@@ -458,24 +458,24 @@ export const MarimoErrorOutput = ({
458458
? "Ensure that the referenced cells define the required variables, or turn off strict execution."
459459
: "Something is wrong with your declarations. Fix any discrepancies, or turn off strict execution."}
460460
</Tip>
461-
</li>,
461+
</div>,
462462
);
463463
}
464464

465465
if (internalErrors.length > 0) {
466466
messages.push(
467-
<li key="internal">
467+
<div key="internal">
468468
{internalErrors.map((error, idx) => (
469469
<p key={`internal-${idx}`}>{error.msg}</p>
470470
))}
471471
{cellId && <AutoFixButton errors={internalErrors} cellId={cellId} />}
472-
</li>,
472+
</div>,
473473
);
474474
}
475475

476476
if (ancestorPreventedErrors.length > 0) {
477477
messages.push(
478-
<li key="ancestor-prevented">
478+
<div key="ancestor-prevented">
479479
{ancestorPreventedErrors.map((error, idx) => (
480480
<div key={`ancestor-prevented-${idx}`}>
481481
{error.msg}
@@ -495,13 +495,13 @@ export const MarimoErrorOutput = ({
495495
{cellId && (
496496
<AutoFixButton errors={ancestorPreventedErrors} cellId={cellId} />
497497
)}
498-
</li>,
498+
</div>,
499499
);
500500
}
501501

502502
if (ancestorStoppedErrors.length > 0) {
503503
messages.push(
504-
<li key="ancestor-stopped">
504+
<div key="ancestor-stopped">
505505
{ancestorStoppedErrors.map((error, idx) => (
506506
<div key={`ancestor-stopped-${idx}`}>
507507
{error.msg}
@@ -511,7 +511,7 @@ export const MarimoErrorOutput = ({
511511
{cellId && (
512512
<AutoFixButton errors={ancestorStoppedErrors} cellId={cellId} />
513513
)}
514-
</li>,
514+
</div>,
515515
);
516516
}
517517

@@ -534,7 +534,7 @@ export const MarimoErrorOutput = ({
534534
>
535535
{title}
536536
<div>
537-
<ul className="flex flex-col gap-8">{renderMessages()}</ul>
537+
<div className="flex flex-col gap-8">{renderMessages()}</div>
538538
</div>
539539
</Alert>
540540
);

‎frontend/src/core/cells/__tests__/cells.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,10 @@ describe("cell reducer", () => {
111111
state = reducer(state, action);
112112
for (const [cellId, handle] of Object.entries(state.cellHandles)) {
113113
if (!handle.current) {
114+
const view = createEditor(state.cellData[cellId as CellId].code);
114115
const handle: CellHandle = {
115-
editorView: createEditor(state.cellData[cellId as CellId].code),
116+
editorView: view,
117+
editorViewOrNull: view,
116118
};
117119
state.cellHandles[cellId as CellId] = { current: handle };
118120
}

‎frontend/src/core/cells/cells.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ import {
5050
import { isEqual, zip } from "lodash-es";
5151
import { isErrorMime } from "../mime";
5252
import { extractAllTracebackInfo, type TracebackInfo } from "@/utils/traceback";
53+
import { isRtcEnabled } from "../rtc/state";
5354

5455
export const SCRATCH_CELL_ID = "__scratch__" as CellId;
5556
export const SETUP_CELL_ID = "setup" as CellId;
@@ -832,9 +833,12 @@ const {
832833
}
833834

834835
// Update codemirror if mounted
835-
const cellHandle = nextState.cellHandles[cellId].current;
836-
if (cellHandle?.editorView) {
837-
updateEditorCodeFromPython(cellHandle.editorView, code);
836+
// If RTC is enabled, the editor view will already be updated, so we don't need to do this
837+
if (!isRtcEnabled()) {
838+
const cellHandle = nextState.cellHandles[cellId].current;
839+
if (cellHandle?.editorViewOrNull) {
840+
updateEditorCodeFromPython(cellHandle.editorViewOrNull, code);
841+
}
838842
}
839843

840844
return {
@@ -1002,7 +1006,7 @@ const {
10021006
// browser fails to scrollIntoView an element at the end of a long page
10031007
if (index === column.length - 1) {
10041008
const cellId = column.last();
1005-
state.cellHandles[cellId].current?.editorView.focus();
1009+
state.cellHandles[cellId].current?.editorViewOrNull?.focus();
10061010
} else {
10071011
const nextCellId = column.atOrThrow(index);
10081012
focusAndScrollCellIntoView({

‎frontend/src/core/codemirror/copilot/__tests__/getCode.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ function createMockEditorView(code: string) {
6060

6161
return {
6262
editorView: view,
63+
editorViewOrNull: view,
6364
};
6465
}
6566

‎frontend/src/core/codemirror/format.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,12 +105,15 @@ export async function formatSQL(editor: EditorView) {
105105
});
106106

107107
// Update editor with formatted SQL
108+
const doc = editor.state.doc;
109+
110+
// Noop if the code is the same
111+
if (doc.toString() === formattedSQL) {
112+
return;
113+
}
114+
108115
editor.dispatch({
109-
changes: {
110-
from: 0,
111-
to: editor.state.doc.length,
112-
insert: formattedSQL,
113-
},
116+
changes: { from: 0, to: doc.length, insert: formattedSQL },
114117
effects: [formattingChangeEffect.of(true)],
115118
});
116119
}

0 commit comments

Comments
 (0)