Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/vs/base/browser/positron/renderHtml.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,12 @@ export const renderHtml = (html: string, opts: HTMLRendererOptions = {}): React.

if (node.type === 'text') {
// Create <span> 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') {
Expand Down
26 changes: 25 additions & 1 deletion src/vs/base/common/htmlParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,29 @@ const voidElements: Record<string, boolean> = {
'wbr': true
};

/**
* Checks if a node is nested inside a <pre> element.
* In <pre> 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.
Comment on lines +89 to +94
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

The O(n) tree traversal for every text node could impact performance with deeply nested HTML structures. Consider tracking pre-element state during parsing to avoid repeated traversals.

Copilot uses AI. Check for mistakes.

*
* @param node The node to check (typically parent of a text node)
* @returns true if node is inside a <pre> 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.
*
Expand Down Expand Up @@ -355,7 +378,8 @@ export function parseHtml(html: string): Array<HtmlNode> {

// 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 <pre> elements where it's semantically significant
if (whitespaceRE.test(content) && !isInsidePreElement(current)) {
content = ' ';
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -36,18 +35,25 @@ export class PositronNotebookMarkdownCell extends PositronNotebookCellGeneral im
);
}

toggleEditor(): void {
async toggleEditor(): Promise<void> {
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<ICodeEditor | undefined> {
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();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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<string> {
const m = new marked.Marked(
markedHighlight({
async: true,
async highlight(code: string, lang: string): Promise<string> {
if (!lang) {
return escape(code);
}

await extensionService.whenInstalledExtensionsRegistered();

const languageId = languageService.getLanguageIdByLanguageName(lang)
?? languageService.getLanguageIdByLanguageName(lang.split(/\s+|:|,|(?!^)\{|\?]/, 1)[0]);
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

The regular expression has an unmatched closing bracket ] without a corresponding opening bracket. This will cause a syntax error. The regex should be /\s+|:|,|(?!^)\{|\?/ (remove the ]).

Suggested change
?? languageService.getLanguageIdByLanguageName(lang.split(/\s+|:|,|(?!^)\{|\?]/, 1)[0]);
?? languageService.getLanguageIdByLanguageName(lang.split(/\s+|:|,|(?!^)\{|\?/, 1)[0]);

Copilot uses AI. Check for mistakes.


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.
Comment on lines +46 to +52
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

This duplicated code creates maintainability concerns. Consider extracting the shared functionality into a common utility module that both files can import, rather than duplicating the implementation.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't expect to change their implementation, it shouldn't cause bad conflicts to add an export to the upstream code. But if we don't really need to stay in sync with any upstream changes, copy pasting is fine too

*/
function markedHighlight(options: marked.MarkedOptions & {
highlight: (code: string, lang: string) => string | Promise<string>;
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> | 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 `<pre><code${classAttr}>${escaped ? text : escape(text)}\n</code></pre>`;
},
},
};
}

function updateToken(token: any) {
return (code: string) => {
if (typeof code === 'string' && code !== token.text) {
token.escaped = true;
token.text = code;
}
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
*
Expand Down Expand Up @@ -133,13 +134,22 @@ export function useCellEditorWidget(cell: PositronNotebookCellGeneral) {
const disposable = autorun(reader => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I recently realized that autorun also has an initial run on definition. Where we don't want that we might prefer something like runOnChange

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 };
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -29,14 +31,21 @@ export function NotebookMarkdownCell({ cell }: { cell: PositronNotebookMarkdownC
{editorShown ? <CellEditorMonacoWidget cell={cell} /> : null}
</div>
<div className='cell-contents positron-notebook-cell-outputs'>
<div className='positron-notebook-markup-rendered' onDoubleClick={() => {
cell.toggleEditor();
}}>
<div
className='positron-notebook-markup-rendered'
onDoubleClick={(e) => {
// 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 ?
<Markdown content={markdownString} />
: <div className='empty-output-msg'>
Empty markup cell. {editorShown ? '' : 'Double click to edit'}
Empty markup cell. {editorShown ? '' : 'Double click to edit.'}
</div>
}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,11 @@ export function NotebookCellMoreActionsMenu({
const buttonRef = useRef<HTMLButtonElement>(null);
const [isMenuOpen, setIsMenuOpen] = React.useState(false);
const commandService = usePositronReactServicesContext().commandService;
const showMoreActionsMenu = () => {
const showMoreActionsMenu = (e: React.MouseEvent<HTMLButtonElement>) => {
// Prevent click from bubbling to cell wrapper which would deselect the cell
e.preventDefault();
e.stopPropagation();

if (!buttonRef.current) {
return;
}
Expand Down