-
Notifications
You must be signed in to change notification settings - Fork 121
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]); | ||||||
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. The regular expression has an unmatched closing bracket
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
|
||||||
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
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. 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. Positive FeedbackNegative Feedback 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. If we don't expect to change their implementation, it shouldn't cause bad conflicts to add an |
||||||
*/ | ||||||
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 |
---|---|---|
|
@@ -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.
Copilot uses AI. Check for mistakes.