mirror of
https://github.com/stablyai/orca.git
synced 2026-10-31 00:57:45 +00:00
fix(editor): keep notebook cell undo across cell and tab switches (#34952)
Notebook cell models were anonymous and disposed whenever a cell lost focus or the notebook view unmounted on a tab switch, so Undo did nothing after returning to a cell. Give cells that have a stable nbformat id a vscode-userdata model URI, scoped to the view, so Monaco's model service keeps and restores the disposed model's undo stack. Monaco restores only when the text is unchanged and bounds retained history to its closed-file budget. Id-less cells keep anonymous models because their position-based key can move to a different cell, and a duplicate id falls back to an anonymous model instead of colliding. Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
This commit is contained in:
@@ -82,7 +82,7 @@ vi.mock('@/lib/monaco-setup', async () => {
|
||||
})
|
||||
|
||||
function notebookContent(
|
||||
cells: { id: string; source: string; kind?: 'code' | 'markdown' }[],
|
||||
cells: { id: string | null; source: string; kind?: 'code' | 'markdown' }[],
|
||||
language = 'python'
|
||||
): string {
|
||||
return JSON.stringify({
|
||||
@@ -90,7 +90,7 @@ function notebookContent(
|
||||
nbformat_minor: 5,
|
||||
metadata: { language_info: { name: language } },
|
||||
cells: cells.map(({ id, source, kind }) => ({
|
||||
id,
|
||||
...(id === null ? {} : { id }),
|
||||
cell_type: kind ?? 'code',
|
||||
metadata: {},
|
||||
execution_count: null,
|
||||
@@ -126,6 +126,7 @@ function NotebookCells({
|
||||
return (
|
||||
<IpynbCellSource
|
||||
key={key}
|
||||
modelKey={cell.id === null ? null : JSON.stringify([modelScope, cell.id])}
|
||||
cell={cell}
|
||||
source={
|
||||
hasIpynbSourceDraft(editing.sourceDrafts, key)
|
||||
@@ -163,7 +164,10 @@ function appendText(model: editor.ITextModel, text: string): void {
|
||||
})
|
||||
}
|
||||
|
||||
// Monaco keeps disposed undo history globally, so each test gets its own model URIs.
|
||||
let modelScope = 0
|
||||
beforeEach(() => {
|
||||
modelScope += 1
|
||||
vi.useFakeTimers()
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
@@ -314,3 +318,118 @@ describe('active notebook cell external reload', () => {
|
||||
expect(onContentChange).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe('notebook cell undo lifetime', () => {
|
||||
const callbacks = () => ({
|
||||
onContentChange: vi.fn<(content: string) => void>(),
|
||||
onDirtyStateHint: vi.fn<(dirty: boolean) => void>()
|
||||
})
|
||||
|
||||
async function undo(model: editor.ITextModel): Promise<void> {
|
||||
await act(async () => {
|
||||
await model.undo()
|
||||
})
|
||||
}
|
||||
|
||||
it('restores cell undo after switching cells and after the notebook view remounts', async () => {
|
||||
const handlers = callbacks()
|
||||
const content = notebookContent([
|
||||
{ id: 'a', source: 'first source' },
|
||||
{ id: 'b', source: 'second source' }
|
||||
])
|
||||
const firstView = render(<NotebookCells content={content} {...handlers} />)
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'first source' }), { button: 0 })
|
||||
appendText(activeModel(), '!')
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'second source' }), { button: 0 })
|
||||
appendText(activeModel(), '?')
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'first source!' }), { button: 0 })
|
||||
await undo(activeModel())
|
||||
expect(activeModel().getValue()).toBe('first source')
|
||||
expect(screen.getByRole('button', { name: 'second source?' })).toBeDefined()
|
||||
|
||||
const edited = handlers.onContentChange.mock.calls.at(-1)?.[0] ?? ''
|
||||
firstView.unmount()
|
||||
render(<NotebookCells content={edited} {...handlers} />)
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'second source?' }), { button: 0 })
|
||||
await undo(activeModel())
|
||||
expect(activeModel().getValue()).toBe('second source')
|
||||
})
|
||||
|
||||
it('drops cell undo when the source changed while the cell was inactive', async () => {
|
||||
const handlers = callbacks()
|
||||
const { rerender } = render(
|
||||
<NotebookCells
|
||||
content={notebookContent([
|
||||
{ id: 'a', source: 'original' },
|
||||
{ id: 'b', source: 'other' }
|
||||
])}
|
||||
{...handlers}
|
||||
/>
|
||||
)
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'original' }), { button: 0 })
|
||||
appendText(activeModel(), '!')
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'other' }), { button: 0 })
|
||||
rerender(
|
||||
<NotebookCells
|
||||
content={handlers.onContentChange.mock.calls.at(-1)?.[0] ?? ''}
|
||||
{...handlers}
|
||||
/>
|
||||
)
|
||||
rerender(
|
||||
<NotebookCells
|
||||
content={notebookContent([
|
||||
{ id: 'a', source: 'from disk' },
|
||||
{ id: 'b', source: 'other' }
|
||||
])}
|
||||
{...handlers}
|
||||
/>
|
||||
)
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'from disk' }), { button: 0 })
|
||||
await undo(activeModel())
|
||||
expect(activeModel().getValue()).toBe('from disk')
|
||||
})
|
||||
|
||||
it('does not carry undo to another id-less cell that moves into the same position', async () => {
|
||||
const handlers = callbacks()
|
||||
const { rerender } = render(
|
||||
<NotebookCells
|
||||
content={notebookContent([
|
||||
{ id: null, source: 'text' },
|
||||
{ id: null, source: 'text!' }
|
||||
])}
|
||||
{...handlers}
|
||||
/>
|
||||
)
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'text' }), { button: 0 })
|
||||
appendText(activeModel(), '!')
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'text!' }), { button: 0 })
|
||||
rerender(
|
||||
<NotebookCells
|
||||
content={handlers.onContentChange.mock.calls.at(-1)?.[0] ?? ''}
|
||||
{...handlers}
|
||||
/>
|
||||
)
|
||||
rerender(
|
||||
<NotebookCells content={notebookContent([{ id: null, source: 'text!' }])} {...handlers} />
|
||||
)
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'text!' }), { button: 0 })
|
||||
handlers.onContentChange.mockClear()
|
||||
await undo(activeModel())
|
||||
expect(activeModel().getValue()).toBe('text!')
|
||||
expect(handlers.onContentChange).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('opens a cell whose id is duplicated in the notebook', () => {
|
||||
render(
|
||||
<NotebookCells
|
||||
content={notebookContent([
|
||||
{ id: 'dup', source: 'first' },
|
||||
{ id: 'dup', source: 'second' }
|
||||
])}
|
||||
{...callbacks()}
|
||||
/>
|
||||
)
|
||||
fireEvent.mouseDown(screen.getByRole('button', { name: 'first' }), { button: 0 })
|
||||
expect(liveModels.map((model) => model.getValue())).toEqual(['first', 'second'])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -37,6 +37,8 @@ export function IpynbMarkdownCell({ source }: { source: string }): React.JSX.Ele
|
||||
}
|
||||
|
||||
type IpynbCellSourceProps = {
|
||||
/** Null for cells without a stable id, whose position-based key can move to another cell. */
|
||||
modelKey: string | null
|
||||
cell: IpynbCell
|
||||
source: string
|
||||
active: boolean
|
||||
@@ -140,6 +142,7 @@ function IpynbCodePreview({
|
||||
}
|
||||
|
||||
function IpynbSourceEditor({
|
||||
modelKey,
|
||||
cell,
|
||||
source,
|
||||
pressedAt,
|
||||
@@ -172,7 +175,16 @@ function IpynbSourceEditor({
|
||||
return
|
||||
}
|
||||
const { lineHeight, paddingX, paddingY } = CODE_LAYOUT
|
||||
const model = monaco.editor.createModel(source, cell.language)
|
||||
// Monaco restores undo for disposed models with this scheme; no other Monaco feature special-cases it.
|
||||
const uri =
|
||||
modelKey === null
|
||||
? null
|
||||
: monaco.Uri.from({ scheme: 'vscode-userdata', path: `/${modelKey}` })
|
||||
const model = monaco.editor.createModel(
|
||||
source,
|
||||
cell.language,
|
||||
uri && !monaco.editor.getModel(uri) ? uri : undefined
|
||||
)
|
||||
const editorInstance = monaco.editor.create(container, {
|
||||
dropIntoEditor: { enabled: false },
|
||||
model,
|
||||
|
||||
@@ -24,6 +24,7 @@ function cell(kind: IpynbCell['kind'], source: string, outputs: IpynbOutput[] =
|
||||
function renderSource(target: IpynbCell, onActivate = vi.fn()) {
|
||||
render(
|
||||
<IpynbCellSource
|
||||
modelKey={null}
|
||||
cell={target}
|
||||
source={target.source}
|
||||
active={false}
|
||||
|
||||
@@ -234,6 +234,9 @@ export default function IpynbViewer({
|
||||
</div>
|
||||
<div className="min-w-0 flex-1">
|
||||
<IpynbCellSource
|
||||
modelKey={
|
||||
cell.id === null ? null : JSON.stringify([fileId, scrollCacheKey, cell.id])
|
||||
}
|
||||
cell={cell}
|
||||
source={source}
|
||||
active={editingCellKey === cellKey}
|
||||
|
||||
Reference in New Issue
Block a user