From 68034bd6146629bcd4b78cef34b519dbd24b0131 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 14 Oct 2025 11:47:09 -0400 Subject: [PATCH 1/4] Fix bug where selection/editing never went to markdown cells when the editor needed to be instantiated first causing selection to jump to first cell --- .../PositronNotebookMarkdownCell.ts | 16 +++++++++++----- .../notebookCells/CellEditorMonacoWidget.tsx | 12 +++++++++++- .../notebookCells/NotebookCellWrapper.tsx | 3 ++- .../notebookCells/NotebookMarkdownCell.tsx | 17 +++++++++++++---- 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookMarkdownCell.ts b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookMarkdownCell.ts index 2a1e58818ff9..69d00f84c04f 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookMarkdownCell.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookMarkdownCell.ts @@ -12,7 +12,6 @@ import { PositronNotebookInstance } from '../PositronNotebookInstance.js'; import { IPositronNotebookMarkdownCell } from './IPositronNotebookCell.js'; import { ICodeEditor } from '../../../../../editor/browser/editorBrowser.js'; import { INotebookExecutionStateService } from '../../../notebook/common/notebookExecutionStateService.js'; -import { CellSelectionType } from '../selectionMachine.js'; export class PositronNotebookMarkdownCell extends PositronNotebookCellGeneral implements IPositronNotebookMarkdownCell { @@ -36,18 +35,25 @@ export class PositronNotebookMarkdownCell extends PositronNotebookCellGeneral im ); } - toggleEditor(): void { + async toggleEditor(): Promise { const editorStartingOpen = this.editorShown.get(); - this.editorShown.set(!editorStartingOpen, undefined); - // Make sure cell stays selected if we're closing the editor if (editorStartingOpen) { - this.select(CellSelectionType.Normal); + // Closing the editor - exit editing mode and return to selected state + this._instance.selectionStateMachine.exitEditor(this); + this.editorShown.set(false, undefined); + } else { + // Opening the editor - enter editing mode through the selection machine + // This will properly handle state transitions and focus management + await this._instance.selectionStateMachine.enterEditor(this); } } override async showEditor(): Promise { this.editorShown.set(true, undefined); await waitForState(this._editor, (editor) => editor !== undefined); + // Wait for the text model to be loaded before returning + // This ensures the editor is fully ready for focus operations + await this.getTextEditorModel(); return super.showEditor(); } diff --git a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellEditorMonacoWidget.tsx b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellEditorMonacoWidget.tsx index 684b84498dca..afc192a54292 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellEditorMonacoWidget.tsx +++ b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellEditorMonacoWidget.tsx @@ -23,6 +23,7 @@ import { PositronNotebookCellGeneral } from '../PositronNotebookCells/PositronNo import { usePositronReactServicesContext } from '../../../../../base/browser/positronReactRendererContext.js'; import { autorun } from '../../../../../base/common/observable.js'; import { POSITRON_NOTEBOOK_CELL_EDITOR_FOCUSED } from '../ContextKeysManager.js'; +import { SelectionState } from '../selectionMachine.js'; /** * @@ -133,13 +134,22 @@ export function useCellEditorWidget(cell: PositronNotebookCellGeneral) { const disposable = autorun(reader => { cell.editorFocusRequested.read(reader); const editor = cell.editor; + // Check if THIS cell is still the one being edited + // This prevents stale focus requests when user rapidly navigates between cells + const state = instance.selectionStateMachine.state.read(reader); + const shouldFocus = state.type === SelectionState.EditingSelection && state.selected === cell; + + if (!shouldFocus) { + return; + } + if (editor) { editor.focus(); } }); return () => disposable.dispose(); - }, [cell]); + }, [cell, instance.selectionStateMachine]); return { editorPartRef }; } diff --git a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookCellWrapper.tsx b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookCellWrapper.tsx index 404583131ca0..e0823095b903 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookCellWrapper.tsx +++ b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookCellWrapper.tsx @@ -110,10 +110,11 @@ export function NotebookCellWrapper({ cell, actionBarChildren, children }: { return; } + // If already selected, do nothing - maintain selection invariant if (selectionStatus === CellSelectionStatus.Selected) { - cell.deselect(); return; } + const addMode = e.shiftKey || e.ctrlKey || e.metaKey; selectionStateMachine.selectCell(cell, addMode ? CellSelectionType.Add : CellSelectionType.Normal); }} diff --git a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx index fd1416d68bc5..72bce1a6d287 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx +++ b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx @@ -15,9 +15,11 @@ import { useObservedValue } from '../useObservedValue.js'; import { Markdown } from './Markdown.js'; import { NotebookCellWrapper } from './NotebookCellWrapper.js'; import { PositronNotebookMarkdownCell } from '../PositronNotebookCells/PositronNotebookMarkdownCell.js'; +import { useNotebookInstance } from '../NotebookInstanceProvider.js'; export function NotebookMarkdownCell({ cell }: { cell: PositronNotebookMarkdownCell }) { + const notebookInstance = useNotebookInstance(); const markdownString = useObservedValue(cell.markdownString); const editorShown = useObservedValue(cell.editorShown); @@ -29,14 +31,21 @@ export function NotebookMarkdownCell({ cell }: { cell: PositronNotebookMarkdownC {editorShown ? : null}
-
{ - cell.toggleEditor(); - }}> +
{ + // Prevent bubbling to wrapper's onClick and default browser behavior + e.stopPropagation(); + e.preventDefault(); + // Enter edit mode for this cell + notebookInstance.selectionStateMachine.enterEditor(cell); + }} + > { markdownString.length > 0 ? :
- Empty markup cell. {editorShown ? '' : 'Double click to edit'} + Empty markup cell. {editorShown ? '' : 'Double click to edit.'}
}
From aa61ed054618a79c7eba31c5ae06670893b4eda8 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 14 Oct 2025 13:14:07 -0400 Subject: [PATCH 2/4] Stop propagation on the more actions button which was causing weird selection behavior. --- .../notebookCells/actionBar/NotebookCellMoreActionsMenu.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/NotebookCellMoreActionsMenu.tsx b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/NotebookCellMoreActionsMenu.tsx index d0ba565a43ee..75d595f8f7b2 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/NotebookCellMoreActionsMenu.tsx +++ b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/NotebookCellMoreActionsMenu.tsx @@ -39,7 +39,11 @@ export function NotebookCellMoreActionsMenu({ const buttonRef = useRef(null); const [isMenuOpen, setIsMenuOpen] = React.useState(false); const commandService = usePositronReactServicesContext().commandService; - const showMoreActionsMenu = () => { + const showMoreActionsMenu = (e: React.MouseEvent) => { + // Prevent click from bubbling to cell wrapper which would deselect the cell + e.preventDefault(); + e.stopPropagation(); + if (!buttonRef.current) { return; } From 7bb88fbfc1c173e71e9eed9d2eb4586898bcfc30 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Wed, 15 Oct 2025 14:37:54 -0400 Subject: [PATCH 3/4] Preserve whitespace in text nodes and add check for 
 elements in HTML parsing

---
 src/vs/base/browser/positron/renderHtml.tsx |  7 +++---
 src/vs/base/common/htmlParser.ts            | 26 ++++++++++++++++++++-
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/vs/base/browser/positron/renderHtml.tsx b/src/vs/base/browser/positron/renderHtml.tsx
index 6a04ce10d843..ecbde4f318b2 100644
--- a/src/vs/base/browser/positron/renderHtml.tsx
+++ b/src/vs/base/browser/positron/renderHtml.tsx
@@ -69,11 +69,12 @@ export const renderHtml = (html: string, opts: HTMLRendererOptions = {}): React.
 
 		if (node.type === 'text') {
 			// Create  elements to host the text content.
-			if (node.content && node.content.trim().length > 0) {
+			// Preserve all text content, including whitespace-only nodes, as they may be
+			// semantically significant (e.g., spaces between inline elements).
+			if (node.content && node.content.length > 0) {
 				return React.createElement('span', {}, node.content);
 			}
-			// Text nodes with no content (or only whitespae content) are
-			// currently ignored.
+			// Only ignore truly empty text nodes (null, undefined, or empty string).
 			return undefined;
 		} else if (node.type === 'tag' && node.children) {
 			if (node.children.length === 1 && node.children[0].type === 'text') {
diff --git a/src/vs/base/common/htmlParser.ts b/src/vs/base/common/htmlParser.ts
index eacdb65deb39..138436afa179 100644
--- a/src/vs/base/common/htmlParser.ts
+++ b/src/vs/base/common/htmlParser.ts
@@ -84,6 +84,29 @@ const voidElements: Record = {
 	'wbr': true
 };
 
+/**
+ * Checks if a node is nested inside a 
 element.
+ * In 
 elements, whitespace must be preserved per HTML spec.
+ *
+ * This is an O(n) operation, where n is the depth of the node in the tree which
+ * is not super ideal and could be optimized with additional state tracking if
+ * needed but until we see performance issues the encapsulation provided by
+ * this implementation is worth it.
+ *
+ * @param node The node to check (typically parent of a text node)
+ * @returns true if node is inside a 
 element
+ */
+function isInsidePreElement(node: HtmlNode | undefined): boolean {
+	let current = node;
+	while (current) {
+		if (current.name === 'pre') {
+			return true;
+		}
+		current = current.parent;
+	}
+	return false;
+}
+
 /**
  * Parses a single HTML tag.
  *
@@ -355,7 +378,8 @@ export function parseHtml(html: string): Array {
 
 				// If a node is nothing but whitespace, collapse it as the spec states:
 				// https://www.w3.org/TR/html4/struct/text.html#h-9.1
-				if (whitespaceRE.test(content)) {
+				// However, preserve whitespace inside 
 elements where it's semantically significant
+				if (whitespaceRE.test(content) && !isInsidePreElement(current)) {
 					content = ' ';
 				}
 

From 6a07722bc8c091eac25d40e31fcc8cb61389a48a Mon Sep 17 00:00:00 2001
From: Nick Strayer 
Date: Wed, 15 Oct 2025 16:40:24 -0400
Subject: [PATCH 4/4] Replace old async command-based markdown rendering with a
 direct call to marked.js. Fixes styles etc..

---
 .../browser/markdownRenderer.ts               | 96 +++++++++++++++++++
 .../browser/notebookCells/Markdown.tsx        |  4 +-
 2 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 src/vs/workbench/contrib/positronNotebook/browser/markdownRenderer.ts

diff --git a/src/vs/workbench/contrib/positronNotebook/browser/markdownRenderer.ts b/src/vs/workbench/contrib/positronNotebook/browser/markdownRenderer.ts
new file mode 100644
index 000000000000..1ce0110069f1
--- /dev/null
+++ b/src/vs/workbench/contrib/positronNotebook/browser/markdownRenderer.ts
@@ -0,0 +1,96 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (C) 2025 Posit Software, PBC. All rights reserved.
+ *  Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { escape } from '../../../../base/common/strings.js';
+import { ILanguageService } from '../../../../editor/common/languages/language.js';
+import { tokenizeToString } from '../../../../editor/common/languages/textToHtmlTokenizer.js';
+import * as marked from '../../../../base/common/marked/marked.js';
+import { IExtensionService } from '../../../services/extensions/common/extensions.js';
+
+/**
+ * Renders markdown with theme-aware syntax highlighting for Positron notebooks.
+ * Unlike renderMarkdownDocument, this doesn't sanitize aggressively since notebook
+ * content is trusted and needs to support local image paths.
+ */
+export async function renderNotebookMarkdown(
+	content: string,
+	extensionService: IExtensionService,
+	languageService: ILanguageService
+): Promise {
+	const m = new marked.Marked(
+		markedHighlight({
+			async: true,
+			async highlight(code: string, lang: string): Promise {
+				if (!lang) {
+					return escape(code);
+				}
+
+				await extensionService.whenInstalledExtensionsRegistered();
+
+				const languageId = languageService.getLanguageIdByLanguageName(lang)
+					?? languageService.getLanguageIdByLanguageName(lang.split(/\s+|:|,|(?!^)\{|\?]/, 1)[0]);
+
+				return tokenizeToString(languageService, code, languageId);
+			}
+		})
+	);
+
+	return await m.parse(content, { async: true });
+}
+
+/**
+ * Helper for marked.js syntax highlighting integration.
+ *
+ * NOTE: This code is duplicated from markdownDocumentRenderer.ts (MarkedHighlight namespace)
+ * which itself copied it from https://github.com/markedjs/marked-highlight.
+ *
+ * We duplicate it here rather than modifying upstream code to avoid merge conflicts.
+ * The MarkedHighlight namespace is private in upstream, so we can't import it.
+ *
+ * If VSCode upstream exports this utility in the future, we should use that instead.
+ */
+function markedHighlight(options: marked.MarkedOptions & {
+	highlight: (code: string, lang: string) => string | Promise;
+	async?: boolean;
+}): marked.MarkedExtension {
+	if (!options || typeof options.highlight !== 'function') {
+		throw new Error('Must provide highlight function');
+	}
+
+	return {
+		async: !!options.async,
+		walkTokens(token: marked.Token): Promise | void {
+			if (token.type !== 'code') {
+				return;
+			}
+
+			if (options.async) {
+				return Promise.resolve(options.highlight(token.text, token.lang || '')).then(updateToken(token));
+			}
+
+			const code = options.highlight(token.text, token.lang || '');
+			if (code instanceof Promise) {
+				throw new Error('markedHighlight is not set to async but the highlight function is async.');
+			}
+			updateToken(token)(code);
+		},
+		renderer: {
+			code({ text, lang, escaped }: marked.Tokens.Code) {
+				const classAttr = lang ? ` class="language-${escape(lang)}"` : '';
+				text = text.replace(/\n$/, '');
+				return `
${escaped ? text : escape(text)}\n
`;
+			},
+		},
+	};
+}
+
+function updateToken(token: any) {
+	return (code: string) => {
+		if (typeof code === 'string' && code !== token.text) {
+			token.escaped = true;
+			token.text = code;
+		}
+	};
+}
diff --git a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/Markdown.tsx b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/Markdown.tsx
index 1984ebec0d6e..e0a8d1c699ec 100644
--- a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/Markdown.tsx
+++ b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/Markdown.tsx
@@ -16,6 +16,8 @@ import { ExternalLink } from '../../../../../base/browser/ui/ExternalLink/Extern
 import { localize } from '../../../../../nls.js';
 import { createCancelablePromise, raceTimeout } from '../../../../../base/common/async.js';
 import { usePositronReactServicesContext } from '../../../../../base/browser/positronReactRendererContext.js';
+import { renderNotebookMarkdown } from '../markdownRenderer.js';
+import { IExtensionService } from '../../../../services/extensions/common/extensions.js';
 
 /**
  * Component that render markdown content from a string.
@@ -56,7 +58,7 @@ function useMarkdown(content: string): MarkdownRenderResults {
 	React.useEffect(() => {
 
 		const conversionCancellablePromise = createCancelablePromise(() => raceTimeout(
-			services.commandService.executeCommand('markdown.api.render', content),
+			renderNotebookMarkdown(content, services.get(IExtensionService), services.languageService),
 			5000,
 		));