From d24230076101d1d844a0a7bd23b4a05507d8ced8 Mon Sep 17 00:00:00 2001 From: justschen Date: Thu, 30 Oct 2025 22:42:44 -0700 Subject: [PATCH] revert commit --- src/vs/base/common/async.ts | 4 - .../chatEditingCheckpointTimelineImpl.ts | 19 ++- .../browser/chatEditing/chatEditingSession.ts | 8 +- .../chatEditingCheckpointTimeline.test.ts | 112 +----------------- 4 files changed, 11 insertions(+), 132 deletions(-) diff --git a/src/vs/base/common/async.ts b/src/vs/base/common/async.ts index c37f2e3433f5a..7f44d84b8c43f 100644 --- a/src/vs/base/common/async.ts +++ b/src/vs/base/common/async.ts @@ -313,10 +313,6 @@ export class SequencerByKey { return newPromise; } - peek(key: TKey): Promise | undefined { - return this.promiseMap.get(key) || undefined; - } - keys(): IterableIterator { return this.promiseMap.keys(); } diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingCheckpointTimelineImpl.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingCheckpointTimelineImpl.ts index a904ab1304d55..bfe41e79e762b 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingCheckpointTimelineImpl.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingCheckpointTimelineImpl.ts @@ -76,7 +76,7 @@ export class ChatEditingCheckpointTimelineImpl implements IChatEditingCheckpoint const operations = this._operations.read(reader); const checkpoints = this._checkpoints.read(reader); - const previousOperationEpoch = findLast(operations, op => op.epoch < currentEpoch)?.epoch || 0; + const previousOperationEpoch = operations.findLast(op => op.epoch <= currentEpoch)?.epoch || 0; const previousCheckpointIdx = findLastIdx(checkpoints, cp => cp.epoch < previousOperationEpoch); if (previousCheckpointIdx === -1) { return undefined; @@ -158,11 +158,10 @@ export class ChatEditingCheckpointTimelineImpl implements IChatEditingCheckpoint this.createCheckpoint(undefined, undefined, 'Initial State', 'Starting point before any edits'); } - public createCheckpoint(requestId: string | undefined, undoStopId: string | undefined, label: string, description?: string): string { + public createCheckpoint(requestId: string | undefined, undoStopId: string | undefined, label: string, description?: string): void { const existingCheckpoints = this._checkpoints.get(); - const existing = existingCheckpoints.find(c => c.undoStopId === undoStopId && c.requestId === requestId); - if (existing) { - return existing.checkpointId; + if (existingCheckpoints.some(c => c.undoStopId === undoStopId && c.requestId === requestId)) { + return; } const checkpointId = generateUuid(); @@ -179,8 +178,6 @@ export class ChatEditingCheckpointTimelineImpl implements IChatEditingCheckpoint this._checkpoints.set([...existingCheckpoints, checkpoint], tx); this._currentEpoch.set(checkpoint.epoch + 1, tx); }); - - return checkpointId; } public async undoToLastCheckpoint(): Promise { @@ -193,7 +190,7 @@ export class ChatEditingCheckpointTimelineImpl implements IChatEditingCheckpoint public async redoToNextCheckpoint(): Promise { const targetEpoch = this._willRedoToEpoch.get(); if (targetEpoch) { - await this._navigateToEpoch(targetEpoch + 1); + return this._navigateToEpoch(targetEpoch); } } @@ -203,7 +200,7 @@ export class ChatEditingCheckpointTimelineImpl implements IChatEditingCheckpoint throw new Error(`Checkpoint ${checkpointId} not found`); } - return this._navigateToEpoch(targetCheckpoint.epoch + 1); + return this._navigateToEpoch(targetCheckpoint.epoch); } public getContentURIAtStop(requestId: string, fileURI: URI, stopId: string | undefined): URI { @@ -238,13 +235,13 @@ export class ChatEditingCheckpointTimelineImpl implements IChatEditingCheckpoint const currentCheckpoints = this._checkpoints.get(); const operations = this._operations.get(); - const insertAt = findLastIdx(operations, op => op.epoch < currentEpoch); + const insertAt = operations.findLastIndex(op => op.epoch <= currentEpoch); operations[insertAt + 1] = operation; operations.length = insertAt + 2; // Truncate any operations beyond this point // If we undid some operations and are dropping them out of history, also remove // any associated checkpoints. - const newCheckpoints = currentCheckpoints.filter(c => c.epoch < currentEpoch); + const newCheckpoints = currentCheckpoints.filter(c => c.epoch <= currentEpoch || c.epoch >= operation.epoch); transaction(tx => { if (newCheckpoints.length !== currentCheckpoints.length) { this._checkpoints.set(newCheckpoints, tx); diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts index d3cfc236c68d4..d83b1381cb56c 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts @@ -312,8 +312,6 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio } public async getSnapshotModel(requestId: string, undoStop: string | undefined, snapshotUri: URI): Promise { - await this._baselineCreationLocks.peek(snapshotUri.path); - const content = await this._timeline.getContentAtStop(requestId, snapshotUri, undoStop); if (content === undefined) { return null; @@ -440,6 +438,8 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio this._onDidDispose.dispose(); } + private _streamingEditLocks = new SequencerByKey(); + private get isDisposed() { return this._state.get() === ChatEditingSessionState.Disposed; } @@ -454,10 +454,6 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio const sequencer = new ThrottledSequencer(15, 1000); sequencer.queue(() => startPromise.p); - // Lock around creating the baseline so we don't fail to resolve models - // in the edit pills if they render quickly - this._baselineCreationLocks.queue(resource.path, () => startPromise.p); - this._streamingEditLocks.queue(resource.toString(), async () => { if (!this.isDisposed) { await this._acceptStreamingEditsStart(responseModel, inUndoStop, resource); diff --git a/src/vs/workbench/contrib/chat/test/browser/chatEditingCheckpointTimeline.test.ts b/src/vs/workbench/contrib/chat/test/browser/chatEditingCheckpointTimeline.test.ts index 25fc09cf6236e..d9993666715fe 100644 --- a/src/vs/workbench/contrib/chat/test/browser/chatEditingCheckpointTimeline.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/chatEditingCheckpointTimeline.test.ts @@ -597,7 +597,7 @@ suite('ChatEditingCheckpointTimeline', function () { // Verify we're at the start of req1, which has epoch 2 (0 = initial, 1 = baseline, 2 = start checkpoint) const state = timeline.getStateForPersistence(); - assert.strictEqual(state.currentEpoch, 3); // Should be at the "Start req1" checkpoint epoch + assert.strictEqual(state.currentEpoch, 2); // Should be at the "Start req1" checkpoint epoch }); test('operations use incrementing epochs', function () { @@ -1022,116 +1022,6 @@ suite('ChatEditingCheckpointTimeline', function () { // Should not have changed assert.strictEqual(stateBefore.currentEpoch, stateAfter.currentEpoch); }); - - test('orphaned operations and checkpoints are removed after undo and new changes', async function () { - const uri = URI.parse('file:///test.txt'); - - // Create the file first - const createEpoch = timeline.incrementEpoch(); - - timeline.recordFileOperation(createFileCreateOperation( - uri, - 'req1', - createEpoch, - 'initial content' - )); - - timeline.createCheckpoint('req1', undefined, 'Start req1'); - - // First set of changes - timeline.recordFileOperation(createTextEditOperation( - uri, - 'req1', - timeline.incrementEpoch(), - [{ range: new Range(1, 1, 1, 16), text: 'first edit' }] - )); - - timeline.createCheckpoint('req1', 'stop1', 'First Edit'); - - timeline.recordFileOperation(createTextEditOperation( - uri, - 'req1', - timeline.incrementEpoch(), - [{ range: new Range(1, 1, 1, 11), text: 'second edit' }] - )); - - timeline.createCheckpoint('req1', 'stop2', 'Second Edit'); - - // Verify we have 3 operations (create + 2 edits) and 4 checkpoints (initial, start, stop1, stop2) - let state = timeline.getStateForPersistence(); - assert.strictEqual(state.operations.length, 3); - assert.strictEqual(state.checkpoints.length, 4); - - // Undo to stop1 (before second edit) - await timeline.navigateToCheckpoint(timeline.getCheckpointIdForRequest('req1', 'stop1')!); - - // Record a new operation - this should truncate the "second edit" operation - // and remove the stop2 checkpoint - timeline.recordFileOperation(createTextEditOperation( - uri, - 'req1', - timeline.incrementEpoch(), - [{ range: new Range(1, 1, 1, 11), text: 'replacement edit' }] - )); - - timeline.createCheckpoint('req1', 'stop2-new', 'Replacement Edit'); - - // Verify the orphaned operation and checkpoint are gone - state = timeline.getStateForPersistence(); - assert.strictEqual(state.operations.length, 3, 'Should still have 3 operations (create + first + replacement)'); - assert.strictEqual(state.checkpoints.length, 4, 'Should have 4 checkpoints (initial, start, stop1, stop2-new)'); - - // Verify the third operation is the replacement, not the original second edit - const thirdOp = state.operations[2]; - assert.strictEqual(thirdOp.type, FileOperationType.TextEdit); - if (thirdOp.type === FileOperationType.TextEdit) { - assert.strictEqual(thirdOp.edits[0].text, 'replacement edit'); - } - - // Verify the stop2-new checkpoint exists, not stop2 - const stop2NewCheckpoint = timeline.getCheckpointIdForRequest('req1', 'stop2-new'); - const stop2OldCheckpoint = timeline.getCheckpointIdForRequest('req1', 'stop2'); - assert.ok(stop2NewCheckpoint, 'New checkpoint should exist'); - assert.strictEqual(stop2OldCheckpoint, undefined, 'Old orphaned checkpoint should be removed'); - - // Now navigate through the entire timeline to verify consistency - const initialCheckpoint = state.checkpoints[0]; - const startCheckpoint = timeline.getCheckpointIdForRequest('req1', undefined)!; - const stop1Checkpoint = timeline.getCheckpointIdForRequest('req1', 'stop1')!; - const stop2NewCheckpointId = timeline.getCheckpointIdForRequest('req1', 'stop2-new')!; - - // Navigate to initial to clear everything - await timeline.navigateToCheckpoint(initialCheckpoint.checkpointId); - assert.strictEqual(fileContents.has(uri), false); - - // Navigate to start - file should be created - await timeline.navigateToCheckpoint(startCheckpoint); - assert.strictEqual(fileContents.get(uri), 'initial content'); - - // Navigate to stop1 - first edit should be applied - await timeline.navigateToCheckpoint(stop1Checkpoint); - assert.strictEqual(fileContents.get(uri), 'first edit'); - - // Navigate to stop2-new - replacement edit should be applied, NOT the orphaned "second edit" - await timeline.navigateToCheckpoint(stop2NewCheckpointId); - assert.strictEqual(fileContents.get(uri), 'replacement edit'); - - // Navigate back to start - await timeline.navigateToCheckpoint(startCheckpoint); - assert.strictEqual(fileContents.get(uri), 'initial content'); - - // Navigate forward through all checkpoints again to ensure redo works correctly - await timeline.navigateToCheckpoint(stop1Checkpoint); - assert.strictEqual(fileContents.get(uri), 'first edit'); - - await timeline.navigateToCheckpoint(stop2NewCheckpointId); - assert.strictEqual(fileContents.get(uri), 'replacement edit', 'Orphaned edit should never reappear'); - - // Go back to initial and forward again to thoroughly test - await timeline.navigateToCheckpoint(initialCheckpoint.checkpointId); - await timeline.navigateToCheckpoint(stop2NewCheckpointId); - assert.strictEqual(fileContents.get(uri), 'replacement edit', 'Content should still be correct after full timeline traversal'); - }); }); // Mock notebook service for tests that don't need notebook functionality