Skip to content

revert 63814385d1304896c0a269629236df1271261b4c #274256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+11 −132
Draft

revert 63814385d1304896c0a269629236df1271261b4c #274256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 0 additions & 4 deletions src/vs/base/common/async.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,10 +313,6 @@ export class SequencerByKey<TKey> {
return newPromise;
}

peek(key: TKey): Promise<unknown> | undefined {
return this.promiseMap.get(key) || undefined;
}

keys(): IterableIterator<TKey> {
return this.promiseMap.keys();
}
Expand Down
19 changes: 8 additions & 11 deletions src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingCheckpointTimelineImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The return type was changed from string to void, removing the ability for callers to get the checkpoint ID. However, there's no indication that all call sites have been updated. If callers need the checkpoint ID (which seems likely for the navigateToCheckpoint API), they must now call getCheckpointIdForRequest separately. Consider whether this is an intentional API breaking change or if the return value should be restored.

Copilot uses AI. Check for mistakes.
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();
Expand All @@ -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<void> {
Expand All @@ -193,7 +190,7 @@ export class ChatEditingCheckpointTimelineImpl implements IChatEditingCheckpoint
public async redoToNextCheckpoint(): Promise<void> {
const targetEpoch = this._willRedoToEpoch.get();
if (targetEpoch) {
await this._navigateToEpoch(targetEpoch + 1);
return this._navigateToEpoch(targetEpoch);
}
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 2 additions & 6 deletions src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,6 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio
}

public async getSnapshotModel(requestId: string, undoStop: string | undefined, snapshotUri: URI): Promise<ITextModel | null> {
await this._baselineCreationLocks.peek(snapshotUri.path);

const content = await this._timeline.getContentAtStop(requestId, snapshotUri, undoStop);
if (content === undefined) {
return null;
Expand Down Expand Up @@ -440,6 +438,8 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio
this._onDidDispose.dispose();
}

private _streamingEditLocks = new SequencerByKey</* URI */ string>();
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The _streamingEditLocks field is declared after the dispose() method (line 439) but is used in the startStreamingEdits method further down. For better code organization, private fields should be declared together with other fields near the top of the class, not interspersed with methods. Consider moving this declaration up to line 135 where it was originally located, grouped with other fields like _baselineCreationLocks.

Copilot uses AI. Check for mistakes.

private get isDisposed() {
return this._state.get() === ChatEditingSessionState.Disposed;
}
Expand All @@ -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);
Expand Down
112 changes: 1 addition & 111 deletions src/vs/workbench/contrib/chat/test/browser/chatEditingCheckpointTimeline.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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
Expand Down
Loading