Positron notebooks - Add new functionality to edit tool for moving cells. #11023
+401
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nstrayer
changed the title
|
E2E Tests 🚀 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds notebook cell reordering capabilities to the Positron Assistant, allowing users to reorganize cells through natural language commands like "move all imports to the top cell" or "move the third cell to the bottom."
Key changes include:
- New
positron.notebooks.moveCellandpositron.notebooks.reorderCellsAPIs for single and batch cell reordering - Extension of
EditNotebookCellsToolwith a 'reorder' operation that accepts eitherfromIndex/toIndexparameters or anewOrderpermutation array - Comprehensive validation for indices and permutations with clear error messages
- Cycle-based algorithm for efficient permutation reordering with minimal cell moves
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/api/common/positron/extHostNotebookFeatures.ts | Added moveCell and reorderCells methods to the extension host API implementation |
| src/vs/workbench/api/common/positron/extHost.positron.protocol.ts | Added protocol method signatures for $moveCell and $reorderCells |
| src/vs/workbench/api/common/positron/extHost.positron.api.impl.ts | Wired up the new notebook methods to the extension API factory |
| src/vs/workbench/api/browser/positron/mainThreadNotebookFeatures.ts | Implemented main thread handlers for cell moving/reordering with validation, cycle-based algorithm, and text model integration |
| src/positron-dts/positron.d.ts | Added TypeScript definitions for the new notebook APIs |
| extensions/positron-assistant/src/tools/notebookUtils.ts | Added validatePermutation function for validating cell reordering arrays |
| extensions/positron-assistant/src/tools/notebookTools.ts | Extended EditNotebookCellsTool to support 'reorder' operation with single move and batch reorder capabilities |
| extensions/positron-assistant/package.json | Updated tool schema to document the new reorder operation parameters |
Comment on lines
+90
to
+141
| export function validatePermutation( | ||
| newOrder: number[], | ||
| cellCount: number | ||
| ): PermutationValidation { | ||
| // Check length matches | ||
| if (newOrder.length !== cellCount) { | ||
| return { | ||
| valid: false, | ||
| error: `Permutation length (${newOrder.length}) must match cell count (${cellCount})` | ||
| }; | ||
| } | ||
|
|
||
| // Handle empty notebook case | ||
| if (cellCount === 0) { | ||
| return { valid: true, isIdentity: true }; | ||
| } | ||
|
|
||
| // An identity permutation is just one where each index maps to itself. Aka a no-op. | ||
| let isIdentity = true; | ||
|
|
||
| // Check if any indices are outside of the valid range | ||
| for (const [i, index] of newOrder.entries()) { | ||
| if (!Number.isInteger(index) || index < 0 || index >= cellCount) { | ||
| return { | ||
| valid: false, | ||
| error: `Invalid index in permutation: ${index} (must be integer between 0 and ${cellCount - 1})` | ||
| }; | ||
| } | ||
|
|
||
| // Check for identity at the same time | ||
| if (index !== i) { | ||
| isIdentity = false; | ||
| } | ||
| } | ||
|
|
||
| // If identity permutation, no need to check further | ||
| if (isIdentity) { | ||
| return { valid: true, isIdentity: true }; | ||
| } | ||
|
|
||
| // Make sure there are no duplicates and all indices are present | ||
| const uniqueIndices = new Set(newOrder); | ||
|
|
||
| if (uniqueIndices.size !== cellCount) { | ||
| return { | ||
| valid: false, | ||
| error: 'Invalid permutation: must contain each index from 0 to cellCount-1 exactly once' | ||
| }; | ||
| } | ||
|
|
||
| return { valid: true, isIdentity: false }; | ||
| } |
There was a problem hiding this comment.
The validatePermutation function lacks test coverage. Given that this validation is critical for ensuring data integrity during cell reordering operations, comprehensive tests should be added to verify:
- Empty arrays (cellCount = 0)
- Identity permutations
- Valid permutations of various sizes
- Invalid cases: wrong length, out-of-range indices, non-integer values, duplicate indices
- Edge cases like single cell notebooks
Consider adding a test file extensions/positron-assistant/src/test/notebookUtils.test.ts with comprehensive test cases for this function.
Comment on lines
+459
to
+499
| // Apply the reordering as a series of move operations | ||
| // We use a cycle-based approach to minimize moves: | ||
| // For each cycle in the permutation, we perform cycle_length - 1 moves | ||
| const currentOrder = [...Array(cellCount).keys()]; // [0, 1, 2, ..., n-1] | ||
| const visited = new Set<number>(); | ||
|
|
||
| for (let startPos = 0; startPos < cellCount; startPos++) { | ||
| if (visited.has(startPos) || newOrder[startPos] === currentOrder[startPos]) { | ||
| visited.add(startPos); | ||
| continue; | ||
| } | ||
|
|
||
| // Follow the cycle | ||
| let pos = startPos; | ||
| while (!visited.has(pos)) { | ||
| visited.add(pos); | ||
| const targetCellOriginalIndex = newOrder[pos]; | ||
| const currentPosOfTargetCell = currentOrder.indexOf(targetCellOriginalIndex); | ||
|
|
||
| if (currentPosOfTargetCell !== pos) { | ||
| // Move the cell from currentPosOfTargetCell to pos | ||
| textModel.applyEdits([{ | ||
| editType: CellEditType.Move, | ||
| index: currentPosOfTargetCell, | ||
| length: 1, | ||
| newIdx: pos | ||
| }], true, undefined, () => undefined, undefined, computeUndoRedo); | ||
|
|
||
| // Update our tracking of current positions | ||
| const movedValue = currentOrder.splice(currentPosOfTargetCell, 1)[0]; | ||
| currentOrder.splice(pos, 0, movedValue); | ||
| } | ||
|
|
||
| // Find the next position in the cycle | ||
| const nextPos = newOrder.indexOf(currentOrder[pos], pos + 1); | ||
| if (nextPos === -1 || visited.has(nextPos)) { | ||
| break; | ||
| } | ||
| pos = nextPos; | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The cycle-based reordering algorithm is clever but complex. While the comment at lines 460-461 provides a brief explanation, additional inline comments explaining the algorithm's logic would improve maintainability.
Consider adding comments that explain:
- Why
currentOrder.indexOf(targetCellOriginalIndex)is used at line 476 - What
currentOrder.indexOf(currentOrder[pos], pos + 1)achieves at line 493 - An example showing how a simple permutation like
[2, 0, 1]is processed through the cycles
This would help future maintainers understand the non-trivial algorithm without needing to reverse-engineer the logic.
Comment on lines
+423
to
+440
| // Validate the permutation | ||
| if (newOrder.length !== cellCount) { | ||
| throw new Error(`Invalid newOrder length: ${newOrder.length}. Must match cell count: ${cellCount}`); | ||
| } | ||
|
|
||
| // Check that it's a valid permutation (each index 0 to n-1 appears exactly once) | ||
| const seen = new Set<number>(); | ||
| for (const index of newOrder) { | ||
| if (!Number.isInteger(index) || index < 0 || index >= cellCount) { | ||
| throw new Error(`Invalid index in newOrder: ${index}. Must be between 0 and ${cellCount - 1}`); | ||
| } | ||
| if (seen.has(index)) { | ||
| throw new Error(`Duplicate index in newOrder: ${index}. Each index must appear exactly once`); | ||
| } | ||
| seen.add(index); | ||
| } | ||
|
|
||
| // Check if this is a no-op (identity permutation) |
There was a problem hiding this comment.
The validation logic here is duplicated from the validatePermutation function in extensions/positron-assistant/src/tools/notebookUtils.ts. This creates a maintenance burden where changes to validation rules need to be made in two places.
Consider removing this validation from $reorderCells and relying solely on the validation in notebookUtils.ts before the API is called, or extracting this into a shared utility that can be used on both the extension side and the main thread side if defense-in-depth validation is desired.
Suggested change
| // Validate the permutation | |
| if (newOrder.length !== cellCount) { | |
| throw new Error(`Invalid newOrder length: ${newOrder.length}. Must match cell count: ${cellCount}`); | |
| } | |
| // Check that it's a valid permutation (each index 0 to n-1 appears exactly once) | |
| const seen = new Set<number>(); | |
| for (const index of newOrder) { | |
| if (!Number.isInteger(index) || index < 0 || index >= cellCount) { | |
| throw new Error(`Invalid index in newOrder: ${index}. Must be between 0 and ${cellCount - 1}`); | |
| } | |
| if (seen.has(index)) { | |
| throw new Error(`Duplicate index in newOrder: ${index}. Each index must appear exactly once`); | |
| } | |
| seen.add(index); | |
| } | |
| // Check if this is a no-op (identity permutation) | |
| // Validate the permutation length | |
| if (newOrder.length !== cellCount) { | |
| throw new Error(`Invalid newOrder length: ${newOrder.length}. Must match cell count: ${cellCount}`); | |
| } | |
| // If this is a no-op (identity permutation), do nothing |
seeM
approved these changes
Dec 12, 2025
Contributor
seeM
left a comment
seeM
left a comment
There was a problem hiding this comment.
Very nice! I wonder if the two different modes of using reorder could confuse LLMs but we can test that out
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #10553.
Summary
This PR adds notebook cell reordering capabilities to the Positron Assistant, enabling users to reorganize cells through natural language prompts like "move all imports to the top cell" or "move the third cell to the bottom."
move-demo.mov
The implementation extends the
EditNotebookCellsToolwith a newreorderoperation that supports both single cell moves (viafromIndex/toIndex) and full permutation reordering (vianewOrderarray). The feature includes validation for permutations and indices, confirmation prompts for the user, and integration with the notebook editor through thepositron.notebooks.reorderCellsAPI.Release Notes
New Features
Bug Fixes
QA Notes
@:positron-notebooks @:assistant