-
Couldn't load subscription status.
- Fork 123
Fix Positron notebook markdown cell focus bugs #9949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
68034bd
aa61ed0
7bb88fb
6a07722
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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]); | ||||||
|
||||||
| ?? languageService.getLanguageIdByLanguageName(lang.split(/\s+|:|,|(?!^)\{|\?]/, 1)[0]); | |
| ?? languageService.getLanguageIdByLanguageName(lang.split(/\s+|:|,|(?!^)\{|\?/, 1)[0]); |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recently realized that |
||
| 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 }; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
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.