edits: fix 'restore checkpoint' leaving the request #274542
Conversation
- For restoring to a request, we actually want to restore the request's initial checkpoint but put our pointer before it, so that further requests overwrite that request checkpoint. - We previously didn't discard undone checkpoints when new ones were applied - Refined the behavior of disablement to more explicitly provide the right UX (show up to and including the last checkpoint that contains any applied operation)
| const firstUnappliedOperationIdx = operations.findIndex(op => op.epoch >= currentEpoch); | ||
| if (firstUnappliedOperationIdx === -1) { | ||
| return []; | ||
| } | ||
|
|
||
| const lastAppliedOperation = firstUnappliedOperationIdx > 0 ? operations[firstUnappliedOperationIdx - 1].epoch : 0; |
There was a problem hiding this comment.
nit: find is a bit nicer unless you'd using the index?
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the checkpoint timeline system to fix issues with request disablement tracking and epoch management. The key change is to track only "visible" operations and checkpoints (those before the current epoch) when creating new checkpoints or recording operations, ensuring proper cleanup of undone history.
Key changes:
- Modified
requestDisablementto use operation-based tracking instead of epoch-based tracking - Refactored
recordFileOperationto use_getVisibleOperationsAndCheckpointshelper - Added
_getVisibleOperationsAndCheckpointsmethod to filter out future operations/checkpoints - Updated
createCheckpointto filter operations/checkpoints before adding new ones - Enhanced
navigateToCheckpointto handle baseline restoration for request start checkpoints
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| chatEditingCheckpointTimelineImpl.ts | Core implementation changes for checkpoint/operation visibility tracking and request disablement logic |
| chatEditingCheckpointTimeline.test.ts | Rewrote test to verify request disablement through undo/redo sequences; removed unused autorun import; fixed epoch expectation comment |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingCheckpointTimelineImpl.ts:87
- Grammatical error in the comment: 'that checkpoint at the checkpoint' should be 'that checkpoint and the checkpoint'.
// If we're backing up to a checkpoint and there are no other operations between
// that checkpoint at the checkpoint marking the start of the request, undo the
// entire request.
| for (let i = checkpoints.length - 1; i >= 0; i--) { | ||
| const { undoStopId, requestId, epoch } = checkpoints[i]; | ||
| if (epoch < currentEpoch) { | ||
| if (epoch < lastAppliedOperation) { |
There was a problem hiding this comment.
The comment on line 136 says 'disable any until the one that contains the last applied operation', but the condition on line 142 breaks when epoch < lastAppliedOperation. This appears to be checking if the epoch is before the last applied operation, not 'until the one that contains'. The comment should be clarified to say 'disable any checkpoints that come after the last applied operation' to match the logic where we iterate backwards and break when we reach checkpoints before the last applied operation.
initial checkpoint but put our pointer before it, so that further
requests overwrite that request checkpoint.
right UX (show up to and including the last checkpoint that contains
any applied operation)