Skip to content

Conversation

seeM
Copy link
Contributor

@seeM seeM commented Aug 22, 2025

Addresses #8733.

Release Notes

New Features

  • N/A

Bug Fixes

  • N/A

QA Notes

@:notebooks @:assistant

@seeM seeM requested review from nstrayer and austin3dickey August 22, 2025 14:32
@seeM seeM added the needs automated test Issue is candidate for automated test label Aug 22, 2025
Copy link

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:notebooks @:assistant

readme  valid tags

nstrayer
nstrayer previously approved these changes Aug 22, 2025
Copy link
Contributor

@nstrayer nstrayer left a comment

Choose a reason for hiding this comment

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

This is awesome. I tested it out and it works fantastic. This makes the notebooks way more easy to use now that I've forgotten how to write python code the old-fashioned way!

Comment on lines 66 to 80
// --- Start Positron ---
// Find the Positron notebook instance for the given URI and containing the given editor.
for (const notebookInstance of positronNotebookService.getInstances()) {
if (isEqual(notebookInstance.uri, data.notebook)) {
const candidate = `<positron-notebook>${notebookInstance.id}#${uri}`;
if (!fallback) {
fallback = candidate;
}
if (notebookInstance.cells.get().some(cell => cell.editor === editor)) {
return candidate;
}
}
}
// --- End Positron ---
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could put this type of thing into the instance itself. The logic would probably be the same just a different location.

Copy link
Contributor Author

@seeM seeM Aug 28, 2025

Choose a reason for hiding this comment

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

I've moved a bunch of it to the service and instance

Comment on lines +15 to +18
* TODO: Some notebook functionality (possibly debugging and outlines) require that the editor control
* also have a `notebookEditor: INotebookEditor` property. We'll need to investigate what that unlocks,
* whether to implement INotebookEditor, or find a different solution.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is something i've been mildly dreading implementing, glad we have a scaffold now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent a bunch of time looking into INotebookEditor, and I'm not totally convinced we should implement it. Worth a discussion though

Comment on lines 54 to 63
notebookInstance.selectionStateMachine.onNewState((state) => {
if (state.type === SelectionState.EditingSelection) {
this._activeCodeEditor = state.selectedCell.editor;
} else if (state.type === SelectionState.NoSelection) {
this._activeCodeEditor = undefined;
} else {
this._activeCodeEditor = state.selected[0]?.editor;
}
})
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious how you find the selection state machine. You're the first person outside of me to touch it and it is a bit... different than what vscode does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The selection state machine is :chef-kissed-fingers:

@@ -5,8 +5,9 @@

import { VSBuffer } from '../../../../base/common/buffer.js';
import { Disposable } from '../../../../base/common/lifecycle.js';
import { ISettableObservable } from '../../../../base/common/observableInternal/base.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this path change recently? I keep needing to make this change everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so yea

austin3dickey
austin3dickey previously approved these changes Aug 25, 2025
Copy link
Contributor

@austin3dickey austin3dickey left a comment

Choose a reason for hiding this comment

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

lovely!

@seeM seeM dismissed stale reviews from austin3dickey and nstrayer via b978511 August 28, 2025 14:17
@seeM seeM force-pushed the feature/positron-notebooks-inline-chat branch from a3b992e to b978511 Compare August 28, 2025 14:17
@seeM seeM force-pushed the feature/positron-notebooks-inline-chat branch from ad1d8bb to d70dd4c Compare August 28, 2025 15:57
@seeM seeM merged commit de66a3f into main Aug 28, 2025
9 checks passed
@seeM seeM deleted the feature/positron-notebooks-inline-chat branch August 28, 2025 16:56
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs automated test Issue is candidate for automated test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants