Fix Positron notebook markdown cell focus bugs #9949

nstrayer · 2025-10-14T17:16:40Z

Summary

This is a general bug-bash pr for markdown rendering in positron notebooks. It addresses two focus-related bugs and improves markdown syntax highlighting in Positron notebooks.

Double-clicking to edit markdown cells loses focus (Creating a new markdown cell does not default to edit mode #9877): When double-clicking a collapsed markdown cell to enter edit mode, focus would jump to the top of the notebook instead of entering the editor. This was caused by a race condition where the Monaco editor's model wasn't fully loaded before attempting to focus.
Ellipsis button click causes focus jump (Clicking on the ellipses for a collapsed markdown cell brings you to the top of the notebook #9909): Clicking the ellipsis (more actions) button on a collapsed markdown cell would cause focus to jump to the top of the notebook. This occurred because the button's click event wasn't stopped from propagating to the cell wrapper, which triggered cell deselection logic.
*Markdown syntax highlighting: Markdown code blocks in notebook cells now use VSCode's tokenization system with .mtk* classes that automatically match the current theme etc.. This avoids needing to call to an async command and speeds up rendering/ makes it more debugable.

Old

New look

Release Notes

New Features

N/A

Bug Fixes

N/A

QA Notes

@:positron-notebooks

Markdown cell focus behavior:

Test double-clicking and pressing Enter on collapsed markdown cells to enter edit mode
Verify focus stays in the editor and doesn't jump to the top of the notebook
Test rapid cell navigation while markdown editor is loading

Markdown cell action buttons:

Test clicking the ellipsis (...) button and other action buttons on collapsed markdown cells
Verify focus remains stable and menus appear correctly

Markdown syntax highlighting:

Test markdown cells with code blocks in various languages (Python, R, JavaScript, etc.)
Switch between different themes (Dark+, Light+, custom themes)
Verify syntax highlighting colors update to match the current theme
Compare with Monaco editor syntax highlighting to ensure colors match
Test that local image references in markdown cells still render correctly

github-actions · 2025-10-14T17:17:01Z

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

^readme ^{valid tags}

Copilot

Pull Request Overview

This PR fixes two focus-related bugs with markdown cells in Positron notebooks: double-clicking to edit markdown cells losing focus (#9877) and ellipsis button clicks causing focus jumps (#9909). The fixes address race conditions in event propagation and editor initialization.

Key changes:

Enhanced the selection state machine with better focus management and race condition prevention
Fixed ellipsis button event propagation to prevent unwanted cell deselection
Improved markdown cell editor initialization to ensure proper focus timing

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/e2e/tests/notebook/notebook-focus-and-selection.test.ts	Unskipped a test that was previously failing due to the focus bugs
src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts	Enhanced selection state machine with improved focus management and race condition prevention
src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/NotebookCellMoreActionsMenu.tsx	Added event propagation prevention to ellipsis button click handler
src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx	Removed double-click handler from markdown cell rendering
src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellEditorMonacoWidget.tsx	Added selection state validation to prevent stale focus requests
src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookInstance.ts	Simplified cell creation logic to use the enhanced selection state machine
src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookMarkdownCell.ts	Enhanced editor initialization with proper async handling and text model loading
src/vs/workbench/contrib/positronNotebook/browser/IPositronNotebookInstance.ts	Removed obsolete setEditingCell method from interface

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx

Copilot

Pull Request Overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Copilot · 2025-10-15T20:50:12Z

src/vs/workbench/contrib/positronNotebook/browser/markdownRenderer.ts

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


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 · 2025-10-15T20:50:12Z

src/vs/workbench/contrib/positronNotebook/browser/markdownRenderer.ts

+ * 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.


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 · 2025-10-15T20:50:12Z

src/vs/base/common/htmlParser.ts

+ * 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.


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.

cindyytong · 2025-10-16T15:07:54Z

Humble request where there are UI changes can you pls include a video for me to review. This may also help QA. My code reviews won't be great :)

Thanks

… editor needed to be instantiated first causing selection to jump to first cell

…election behavior.

… HTML parsing

… to marked.js. Fixes styles etc..

Copilot

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

seeM

Code changes look great! I remember we spoke a long time ago about focus being a real pain point across notebook UIs, so this is very important

seeM · 2025-10-17T09:41:21Z

src/vs/workbench/contrib/positronNotebook/browser/markdownRenderer.ts

+ * 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.


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

seeM · 2025-10-17T09:43:01Z

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellEditorMonacoWidget.tsx

 	// Watch for editor focus requests from the cell
 	React.useLayoutEffect(() => {
 		// Subscribe to focus request signal - triggers whenever requestEditorFocus() is called
 		const disposable = autorun(reader => {


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

cindyytong

Nice! ty for the screenshots

nstrayer requested review from cindyytong, Copilot and midleman October 14, 2025 17:16

Copilot AI reviewed Oct 14, 2025

View reviewed changes

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx Outdated Show resolved Hide resolved

nstrayer force-pushed the positron-nb-markdown-expand-bug branch 2 times, most recently from b6feb92 to 6445435 Compare October 15, 2025 20:48

nstrayer requested review from Copilot and seeM October 15, 2025 20:48

Copilot AI reviewed Oct 15, 2025

View reviewed changes

nstrayer force-pushed the positron-nb-markdown-expand-bug branch from 6445435 to 51fc434 Compare October 16, 2025 14:52

nstrayer added 4 commits October 16, 2025 12:07

Fix bug where selection/editing never went to markdown cells when the…

68034bd

… editor needed to be instantiated first causing selection to jump to first cell

Stop propagation on the more actions button which was causing weird s…

aa61ed0

…election behavior.

Preserve whitespace in text nodes and add check for <pre> elements in…

7bb88fb

… HTML parsing

Replace old async command-based markdown rendering with a direct call…

6a07722

… to marked.js. Fixes styles etc..

nstrayer force-pushed the positron-nb-markdown-expand-bug branch from 51fc434 to 6a07722 Compare October 16, 2025 16:09

nstrayer requested a review from Copilot October 16, 2025 16:17

Copilot AI reviewed Oct 16, 2025

View reviewed changes

seeM approved these changes Oct 17, 2025

View reviewed changes

cindyytong approved these changes Oct 17, 2025

View reviewed changes

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

Fix Positron notebook markdown cell focus bugs #9949

Are you sure you want to change the base?

Fix Positron notebook markdown cell focus bugs #9949

Conversation

nstrayer commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Old

New look

Release Notes

New Features

Bug Fixes

QA Notes

Uh oh!

github-actions bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

cindyytong commented Oct 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

seeM left a comment

Choose a reason for hiding this comment

Uh oh!

seeM Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

seeM Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

cindyytong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nstrayer commented Oct 14, 2025 •

edited

Loading

github-actions bot commented Oct 14, 2025 •

edited

Loading