revert 63814385d1304896c0a269629236df1271261b4c #274256
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes epoch semantics in the chat editing checkpoint timeline system to correctly handle navigation and operation recording. The changes ensure that navigating to a checkpoint means the state is at that checkpoint (not after it), and operations are properly truncated when recording new changes after an undo.
Key Changes
- Changed epoch comparison logic from
<to<=in multiple places to align with the invariant that_currentEpochequal to an operation's epoch means the operation is not applied - Updated navigation methods to use checkpoint epochs directly instead of
epoch + 1 - Fixed checkpoint filtering logic when recording operations to preserve checkpoints at or after the new operation
- Removed unused
peek()method fromSequencerByKeyand related lock usage inChatEditingSession - Removed an obsolete test case that tested orphaned operation removal behavior
- Fixed test assertion to expect epoch 2 instead of 3
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
chatEditingCheckpointTimelineImpl.ts |
Core epoch logic fixes - changed comparisons from < to <=, updated navigation to use checkpoint epochs directly, and fixed checkpoint filtering during operation recording |
chatEditingSession.ts |
Removed unused _baselineCreationLocks usage and moved _streamingEditLocks declaration to a more appropriate location |
async.ts |
Removed unused peek() method from SequencerByKey class |
chatEditingCheckpointTimeline.test.ts |
Fixed test assertion and removed obsolete test case for orphaned operations |
| this._onDidDispose.dispose(); | ||
| } | ||
|
|
||
| private _streamingEditLocks = new SequencerByKey</* URI */ string>(); |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| 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 { |
There was a problem hiding this comment.
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.
1 participant
No description provided.