From cdf7acf9e818478ace59005d840723f3fd50885a Mon Sep 17 00:00:00 2001 From: Xmon Dai Date: Tue, 22 Sep 2026 04:13:09 +0800 Subject: [PATCH] fix(editor): stop external file syncs from stranding the dirty marker External writes to an open file (AI agent, git checkout, scripts) refreshed the buffer correctly but left the tab showing "modified": the global content change listener recomputed isDirty on every setValue with no way to tell a disk sync apart from a user edit, and the clean-up markAsSaved ran in a microtask that an unmount race could skip entirely. - MonacoModelManager gains a beginExternalSync/endExternalSync bracket: while open, content changes skip the dirty recompute and the transient dirty-changed broadcast; unbalanced endExternalSync calls are no-ops so suppression can never get stuck on. - updateModelContent(markAsSaved=true) brackets its setValue and broadcasts dirty-changed(false), making the clean transition visible to consumers the same way setValue made the dirty one visible. - CodeEditor brackets the programmatic setValue in applyExternalContentToModel and settles markAsSaved synchronously in the disk-sync and encoding-reload paths instead of deferring to a microtask. Closes #3165 --- .../tools/editor/components/CodeEditor.tsx | 32 ++-- .../services/MonacoModelManager.test.ts | 180 ++++++++++++++++++ .../editor/services/MonacoModelManager.ts | 62 +++++- 3 files changed, 260 insertions(+), 14 deletions(-) create mode 100644 src/web-ui/src/tools/editor/services/MonacoModelManager.test.ts diff --git a/src/web-ui/src/tools/editor/components/CodeEditor.tsx b/src/web-ui/src/tools/editor/components/CodeEditor.tsx index 026f99531f..288c72f713 100644 --- a/src/web-ui/src/tools/editor/components/CodeEditor.tsx +++ b/src/web-ui/src/tools/editor/components/CodeEditor.tsx @@ -371,7 +371,14 @@ const CodeEditor: React.FC = ({ const previousLoadingState = isLoadingContentRef.current; isLoadingContentRef.current = true; - model.setValue(nextContent); + // Programmatic disk sync: bracket the write so the model manager does not + // flag the model dirty for content nobody typed (issue #3165). + monacoModelManager.beginExternalSync(); + try { + model.setValue(nextContent); + } finally { + monacoModelManager.endExternalSync(); + } setIndentation(applyModelIndentation(model, latestEditorConfigRef.current ?? {}, true)); queueMicrotask(() => { @@ -404,14 +411,17 @@ const CodeEditor: React.FC = ({ if (pos && editorRef.current) { editorRef.current.setPosition(pos); } + // Settle the saved state synchronously: deferring it to a microtask let + // an unmount race skip markAsSaved and strand stale saved metadata after + // the disk sync (issue #3165). + if (modelRef.current && filePath) { + savedVersionIdRef.current = modelRef.current.getAlternativeVersionId(); + monacoModelManager.markAsSaved(modelKey); + } onContentChange?.(fileContent, false); reportFileMissingFromDisk(false); queueMicrotask(() => { isLoadingContentRef.current = false; - if (modelRef.current && !isUnmountedRef.current && filePath) { - savedVersionIdRef.current = modelRef.current.getAlternativeVersionId(); - monacoModelManager.markAsSaved(modelKey); - } }); }, [applyExternalContentToModel, filePath, modelKey, onContentChange, reportFileMissingFromDisk, updateLargeFileMode] @@ -1424,12 +1434,12 @@ const CodeEditor: React.FC = ({ } log.warn('Failed to sync disk version after encoding change', err); } - queueMicrotask(() => { - if (modelRef.current && !isUnmountedRef.current) { - savedVersionIdRef.current = modelRef.current.getAlternativeVersionId(); - monacoModelManager.markAsSaved(modelKey); - } - }); + // Same unmount race as the disk-sync path: settle the saved state + // synchronously right after the bracketed content write (issue #3165). + if (modelRef.current) { + savedVersionIdRef.current = modelRef.current.getAlternativeVersionId(); + monacoModelManager.markAsSaved(modelKey); + } } catch (err) { if (isLikelyFileNotFoundError(err)) { reportFileMissingFromDisk(true); diff --git a/src/web-ui/src/tools/editor/services/MonacoModelManager.test.ts b/src/web-ui/src/tools/editor/services/MonacoModelManager.test.ts new file mode 100644 index 0000000000..ec5eb0c6b3 --- /dev/null +++ b/src/web-ui/src/tools/editor/services/MonacoModelManager.test.ts @@ -0,0 +1,180 @@ +// @vitest-environment jsdom + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type * as Monaco from 'monaco-editor'; +import { setMonacoRuntime } from './monacoRuntime'; + +vi.mock('@/shared/utils/logger', () => ({ + createLogger: () => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }), +})); + +import { monacoModelManager } from './MonacoModelManager'; + +type ChangeListener = () => void; + +class FakeModel { + private value: string; + private versionId = 1; + private listeners: ChangeListener[] = []; + public readonly uri: { toString: () => string }; + + constructor(value: string, uriString: string) { + this.value = value; + this.uri = { toString: () => uriString }; + } + + getValue(): string { + return this.value; + } + + setValue(next: string): void { + if (next === this.value) return; + this.value = next; + this.versionId += 1; + this.listeners.forEach(listener => listener()); + } + + getAlternativeVersionId(): number { + return this.versionId; + } + + onDidChangeContent(listener: ChangeListener): { dispose: () => void } { + this.listeners.push(listener); + return { + dispose: () => { + const index = this.listeners.indexOf(listener); + if (index > -1) this.listeners.splice(index, 1); + }, + }; + } +} + +function createFakeMonaco(): typeof Monaco { + const models = new Map(); + + const Uri = { + file: (path: string) => ({ toString: () => `file://${path}` }), + parse: (value: string) => ({ toString: () => value }), + }; + + const editor = { + getModel: (uri: { toString: () => string }) => models.get(uri.toString()) ?? null, + createModel: (content: string, _language: string, uri: { toString: () => string }) => { + const model = new FakeModel(content, uri.toString()); + models.set(uri.toString(), model); + return model; + }, + onWillDisposeModel: () => ({ dispose: () => {} }), + }; + + return { Uri, editor } as unknown as typeof Monaco; +} + +interface DirtyEvent { + filePath: string; + isDirty: boolean; +} + +function listenDirtyEvents(): { events: DirtyEvent[]; dispose: () => void } { + const events: DirtyEvent[] = []; + const handler = (event: Event) => { + const detail = (event as CustomEvent).detail; + events.push({ filePath: detail.filePath, isDirty: detail.isDirty }); + }; + window.addEventListener('monaco-model-dirty-changed', handler as EventListener); + return { + events, + dispose: () => window.removeEventListener('monaco-model-dirty-changed', handler as EventListener), + }; +} + +let dirtyListener: { events: DirtyEvent[]; dispose: () => void }; + +beforeEach(() => { + setMonacoRuntime(createFakeMonaco()); + dirtyListener = listenDirtyEvents(); +}); + +afterEach(() => { + dirtyListener.dispose(); + setMonacoRuntime(null); +}); + +describe('MonacoModelManager external sync dirty state (issue #3165)', () => { + it('updateModelContent with markAsSaved=true leaves the model clean', () => { + const filePath = '/repo/external-sync-saved.ts'; + const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'const a = 1;'); + expect(monacoModelManager.getModelMetadata(filePath)?.isDirty).toBe(false); + + // External program rewrote the file on disk; the sync path pushes the new + // disk truth into the open model and marks it saved. + monacoModelManager.updateModelContent(filePath, 'const a = 2;', true); + + expect(model.getValue()).toBe('const a = 2;'); + const metadata = monacoModelManager.getModelMetadata(filePath); + expect(metadata?.isDirty).toBe(false); + }); + + it('updateModelContent with markAsSaved=true broadcasts a clean dirty-changed event', () => { + const filePath = '/repo/external-sync-broadcast.ts'; + monacoModelManager.getOrCreateModel(filePath, 'typescript', 'const b = 1;'); + dirtyListener.events.length = 0; + + monacoModelManager.updateModelContent(filePath, 'const b = 2;', true); + + // Consumers that track the dirty dot must see the model end up clean; a + // transient dirty flip without a clean follow-up strands the marker. + const mine = dirtyListener.events.filter(event => event.filePath === filePath); + expect(mine.length).toBeGreaterThan(0); + expect(mine[mine.length - 1].isDirty).toBe(false); + expect(mine.some(event => event.isDirty)).toBe(false); + }); + + it('a bracketed external sync does not flip the dirty flag or broadcast transient dirt', () => { + const filePath = '/repo/external-sync-bracket.ts'; + const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'const c = 1;'); + dirtyListener.events.length = 0; + + monacoModelManager.beginExternalSync(); + try { + model.setValue('const c = 2;'); + } finally { + monacoModelManager.endExternalSync(); + } + monacoModelManager.markAsSaved(filePath); + + expect(monacoModelManager.getModelMetadata(filePath)?.isDirty).toBe(false); + const mine = dirtyListener.events.filter(event => event.filePath === filePath); + expect(mine.some(event => event.isDirty)).toBe(false); + expect(mine[mine.length - 1]?.isDirty).toBe(false); + }); + + it('still marks real user edits outside a sync bracket as dirty', () => { + const filePath = '/repo/user-edit-control.ts'; + const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'const d = 1;'); + dirtyListener.events.length = 0; + + // Control: an edit that is not an external sync must keep the old behavior. + model.setValue('const d = 2; // user typed'); + + expect(monacoModelManager.getModelMetadata(filePath)?.isDirty).toBe(true); + const mine = dirtyListener.events.filter(event => event.filePath === filePath); + expect(mine.some(event => event.isDirty)).toBe(true); + }); + + it('an unbalanced endExternalSync never leaves suppression stuck on', () => { + const filePath = '/repo/unbalanced-bracket.ts'; + const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'const e = 1;'); + + monacoModelManager.endExternalSync(); // no matching begin: must be a no-op + dirtyListener.events.length = 0; + model.setValue('const e = 2; // user typed'); + + expect(monacoModelManager.getModelMetadata(filePath)?.isDirty).toBe(true); + }); +}); diff --git a/src/web-ui/src/tools/editor/services/MonacoModelManager.ts b/src/web-ui/src/tools/editor/services/MonacoModelManager.ts index ac833634e2..f468f1cc48 100644 --- a/src/web-ui/src/tools/editor/services/MonacoModelManager.ts +++ b/src/web-ui/src/tools/editor/services/MonacoModelManager.ts @@ -83,6 +83,14 @@ class MonacoModelManager { private globalListenersInstalled = false; + /** + * Depth counter for programmatic external-sync brackets. While non-zero, + * content changes are disk-sync writes (issue #3165), so the dirty state is + * owned by the sync caller (markAsSaved / updateModelContent) instead of the + * change listener. A counter keeps nested brackets balanced. + */ + private externalSyncDepth = 0; + private constructor() {} public static getInstance(): MonacoModelManager { @@ -213,6 +221,13 @@ class MonacoModelManager { model: monaco.editor.ITextModel ): void { const listener = model.onDidChangeContent(() => { + // Inside an external-sync bracket the write is the disk truth, not a user + // edit: skip the dirty recompute and the transient dirty broadcast so the + // tab never flashes "modified" for a programmatic sync (issue #3165). + // The bracket caller settles the final state (markAsSaved / saved flag). + if (this.externalSyncDepth > 0) { + return; + } const metadata = this.modelMetadata.get(uriString); if (metadata) { const currentVersionId = model.getAlternativeVersionId(); @@ -345,15 +360,37 @@ class MonacoModelManager { const wasEmpty = !metadata || metadata.originalContent === ''; const isFirstContentSet = wasEmpty && content.length > 0; - model.setValue(content); + // markAsSaved callers push the disk truth into an open model (issue + // #3165): bracket the write so the change listener does not recompute the + // dirty flag or broadcast a transient "modified" state in between. + if (markAsSaved) { + this.beginExternalSync(); + } + try { + model.setValue(content); + } finally { + if (markAsSaved) { + this.endExternalSync(); + } + } if (metadata) { if (markAsSaved) { metadata.savedVersionId = model.getAlternativeVersionId(); metadata.originalContent = content; metadata.isDirty = false; + metadata.lastAccessedAt = Date.now(); + + window.dispatchEvent(new CustomEvent('monaco-model-dirty-changed', { + detail: { + uri: uriString, + filePath: metadata.filePath, + isDirty: false + } + })); + } else { + metadata.lastAccessedAt = Date.now(); } - metadata.lastAccessedAt = Date.now(); } this.markLoadingComplete(uriString); @@ -368,6 +405,25 @@ class MonacoModelManager { } } + /** + * Open a programmatic external-sync bracket. While it is open, model content + * changes skip the dirty recompute and the transient dirty broadcast + * (issue #3165). Pair every begin with an end in a finally block. + */ + public beginExternalSync(): void { + this.externalSyncDepth += 1; + } + + /** + * Close an external-sync bracket. Unbalanced calls (more ends than begins) + * are no-ops so suppression can never get stuck on and swallow real edits. + */ + public endExternalSync(): void { + if (this.externalSyncDepth > 0) { + this.externalSyncDepth -= 1; + } + } + public markAsSaved(filePath: string, savedContent?: string, savedVersionId?: number): void { const uri = this.normalizeUri(filePath); const uriString = uri.toString(); @@ -389,7 +445,7 @@ class MonacoModelManager { })); } } - + public getModelMetadata(filePath: string): ModelMetadata | undefined { if (!getMonacoRuntime()) { return undefined;