Add support for markdown cells in positron notebooks #2563

nstrayer · 2024-03-28T17:03:13Z

Intent

Add the ability to create and edit markdown cells in notebooks.

Approach

Leveraged existing markdown parser from the markdown-language-features extension.

Added new component for displaying images (DeferredImage) from the local file-system in react-land.
- Uses new extension positron-notebooks to do so with the 'positronNotebookHelpers.convertImageToBase64' command that converts image to data-url.
- Updated the renderHtml() function to allow for the usage of custom components for a tag type. This is used to pass <DeferredImage/> component instead of plain <img/>.
Added <Markdown/> component that takes a string of markdown text and renders it into react land with images done using the <DeferredImage/> component
Markup cells default to closed but can be opened via double-clicking or a "show editor" button.

Addresses #2350

seeM

(Partial review since I had to head out, will complete tomorrow.)

Very nice! It mostly worked well in my testing. A few rough edges I encountered and some thoughts below, but happy to get :

For some reason the "Hide Editor" button didn't initially work for me, but then started working and I'm not sure what changed.
I also managed to make my notebook tab go blank although I'm not sure how. I think I tried to close the tab (but it didn't actually close) then created a markdown cell and added an image to it.
Links are not yet clickable.
We'll want to decide which approach we take in terms of showing a side-by-side preview versus previewing when the user tries to "run" the cell, but we can handle that in future.

We can address some of these in follow-ups of course.

src/vs/workbench/contrib/positronNotebook/browser/AddCellButtons.tsx

extensions/positron-notebooks/package.json

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/interfaces.ts

src/vs/base/browser/renderHtml.tsx

extensions/positron-notebooks/src/extension.ts

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

extensions/positron-notebooks/src/extension.ts

extensions/positron-notebooks/extension.webpack.config.js

extensions/positron-notebooks/package.json

extensions/positron-notebooks/src/extension.ts

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

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

src/vs/base/browser/ui/ExternalLink/ExternalLink.tsx

seeM · 2024-04-01T10:50:41Z

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

+
+	React.useEffect(() => {
+		const timeoutMs = 3000;
+		const convertCommand = commandWithTimeout<unknown>(


Does the onError part of this work i.e. can you catch errors in the extension host by try-catching executeCommand?

I'm still able to see errors in the console if they aren't caught in the command.

Something weird also happens if I use a URL e.g. https://posit.co/wp-content/uploads/2023/03/home-hero-connect-e1689269684616.jpg. It displays in the preview, but also shows an error in the console:

Error: ENOENT: no such file or directory, open '/Users/seem/posit/demos/https:/posit.co/wp-content/uploads/2023/03/home-hero-connect-e1689269684616.jpg'

Errors thrown in the extension context don't seem to trigger errors in the ui-thread. So in that case onError() really only catches errors where no data is returned. Probably worth renaming or consolidating that logic.

Cleaned up this logic in b7402a8 to make it more obvious what was going on. Unhandled errors in the extension will now most likely trigger a timeout now, which seems reasonable.

ff2afc6 Lets remote images pass through (Based on if it starts with http or https which I think is safe enough.

extensions/positron-notebooks/src/extension.ts

…ells.

…cell.

…rvable renderedHtml field for the markup cells.

…tton

…s hidden.

…ontent and double-click status

…ed markup view. Also fix distracting action button outlines.

…ndering HTML

…rs extension

…ld that is used for image loading. Also update modalDialogs to use this new approach.

Co-authored-by: Jonathan <[email protected]> Signed-off-by: Nick Strayer <[email protected]>

…y passing info about failure as object

…and also added a timeout.

…and utilize in the markdown and image rendering.

…led after timeout

…ack to being more typical promise format

…ssages

softwarenerd

I found just one small thing.

src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookComponent.tsx

extensions/positron-notebooks/src/extension.ts

nstrayer · 2024-04-01T19:15:07Z

src/vs/workbench/contrib/positronNotebook/common/utils/promiseWithTimeout.ts

+export function promiseWithTimeout<T>(promise: Promise<T>, timeoutMs: number, cancelToken: CancellationToken): Promise<T> {
+
+	return new Promise((resolve, reject) => {
+		cancelToken.onCancellationRequested(() => {
+			reject(new Error('Promise cancelled'));
+		});
+
+		const timeout = setTimeout(() => {
+			reject(new Error(`Promise timed out after ${timeoutMs}ms`));
+		}, timeoutMs);
+
+		promise.then((res) => {
+			clearTimeout(timeout);
+			resolve(res);
+		}).catch((err) => {
+			clearTimeout(timeout);
+			reject(err);
+		});
+	});
+}


@jmcphers Is this reinventing a wheel from somewhere in the codebase already?

It is. I think you want raceTimeout here.

jmcphers · 2024-04-01T19:21:43Z

src/vs/workbench/contrib/positronNotebook/common/utils/promiseWithTimeout.ts

+export function promiseWithTimeout<T>(promise: Promise<T>, timeoutMs: number, cancelToken: CancellationToken): Promise<T> {
+
+	return new Promise((resolve, reject) => {
+		cancelToken.onCancellationRequested(() => {
+			reject(new Error('Promise cancelled'));
+		});
+
+		const timeout = setTimeout(() => {
+			reject(new Error(`Promise timed out after ${timeoutMs}ms`));
+		}, timeoutMs);
+
+		promise.then((res) => {
+			clearTimeout(timeout);
+			resolve(res);
+		}).catch((err) => {
+			clearTimeout(timeout);
+			reject(err);
+		});
+	});
+}


It is. I think you want raceTimeout here.

extensions/positron-notebooks/src/extension.ts

nstrayer requested review from jmcphers, seeM and softwarenerd March 28, 2024 17:03

seeM reviewed Mar 28, 2024

View reviewed changes

jmcphers requested changes Mar 28, 2024

View reviewed changes

nstrayer force-pushed the feature/notebooks/markdown-cells branch from b8b36df to f2fcdfb Compare March 29, 2024 17:26

seeM reviewed Apr 1, 2024

View reviewed changes

nstrayer and others added 23 commits April 1, 2024 10:49

Add info about cell type to the PositronNotebookCell class

370a180

Add support for plain "error" notebook messages

85ccfe5

Restructure notebook cells to differentiate between markup and code c…

360e71a

…ells.

Distinguish between view and text model for cells better.

0a94da3

Add support for rendering (to html string) the content of a markdown …

76e7241

…cell.

Add render support for markdown.

304a73d

Create two distinct classes for code and markup cells and add an obse…

8d4e8f2

…rvable renderedHtml field for the markup cells.

Further separate code and markup cell models and rendering

f898401

Add new button for creating markup cell next to code cell creation bu…

4d6bfcb

…tton

Add option for showing and hiding the editor for a markup cell

aa47cd3

Fix overflowing action-bar border when in collapsed markup cell mode

0d0c189

Add message about empty markup cell so user's not super confused

138a2a3

Add logic to hide the cell outlines in markdown cells if the editor i…

d7c4ec3

…s hidden.

Switch off of notebook skeleton for the markup cell, too

4b041a2

markdown now renders in webview so we can successfully open images.

84c9349

Add communication from webview to outside with info about height of c…

cbb7546

…ontent and double-click status

mend

461dff9

Make action bar look a bit better when overlapping content in collaps…

80a56a4

…ed markup view. Also fix distracting action button outlines.

mend

97ccbe2

Add new skeleton for extension used for converting images to data urls

cf62989

Add ability to specify custom components for a given tag name when re…

e0c17f4

…ndering HTML

Render images by converting to data-url using positron notebook helpe…

71d3005

…rs extension

Add placeholder image for when images are not yet loaded

936032a

nstrayer and others added 12 commits April 1, 2024 10:49

Add ability to open links using opener service

6227d04

Consolidate html link clicking logic into same componentOverrides fie…

dd3f53f

…ld that is used for image loading. Also update modalDialogs to use this new approach.

Update extensions/positron-notebooks/extension.webpack.config.js

35e8e80

Co-authored-by: Jonathan <[email protected]> Signed-off-by: Nick Strayer <[email protected]>

Update extensions/positron-notebooks/src/extension.ts

38654e2

Co-authored-by: Jonathan <[email protected]> Signed-off-by: Nick Strayer <[email protected]>

Declare command in contributes section.

b7cc4eb

Be better about mime-type mapping

d5211f6

Pass responsibilities for handling bad conversions to the front-end b…

8f82d6e

…y passing info about failure as object

Localize everything!

cb92717

Make error handing handle entire markdown rendering command pipeline …

d9d176d

…and also added a timeout.

Remove left-over comments in tsconfig from original copy location

f4b4bc9

Add helper function for the task of running a command with a timeout …

9b34e68

…and utilize in the markdown and image rendering.

Restructure timeout command function to not allow callbacks to be cal…

0d5a5e3

…led after timeout

nstrayer force-pushed the feature/notebooks/markdown-cells branch from 62800ce to 0d5a5e3 Compare April 1, 2024 14:50

nstrayer added 8 commits April 1, 2024 11:49

Update timeout command to use existing cancel token class and to go b…

b7402a8

…ack to being more typical promise format

Dont attempt to convert remote images

ff2afc6

Only show most recent error message for conversion and clear stale me…

a51d379

…ssages

Don't pass source to error state to avoid failed GET request

1ea9e77

Move renderHtml() into positron subfolder

fbf825a

Fix name of script containing promiseWithTimeout function

ea726b7

Add slightly better error message in timeout case

be302cb

Add styles for rendering and error messages in markdown component

af266b0

softwarenerd approved these changes Apr 1, 2024

View reviewed changes

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

nstrayer added 2 commits April 1, 2024 14:58

Fix utterly unneccesary string concatenation.

a59f4a7

Fix case-sensitivity of mime-type checking.

b829e7f

nstrayer commented Apr 1, 2024

View reviewed changes

extensions/positron-notebooks/src/extension.ts Show resolved Hide resolved

nstrayer commented Apr 1, 2024

View reviewed changes

jmcphers approved these changes Apr 1, 2024

View reviewed changes

Replace custom promiseWithTimeout() with existing functions

1182b4f

nstrayer merged commit 0aa7255 into main Apr 1, 2024
1 check passed

nstrayer deleted the feature/notebooks/markdown-cells branch April 1, 2024 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for markdown cells in positron notebooks #2563

Add support for markdown cells in positron notebooks #2563

nstrayer commented Mar 28, 2024

seeM left a comment

seeM Apr 1, 2024

seeM Apr 1, 2024

nstrayer Apr 1, 2024

nstrayer Apr 1, 2024

nstrayer Apr 1, 2024

softwarenerd left a comment

nstrayer Apr 1, 2024

jmcphers Apr 1, 2024

jmcphers Apr 1, 2024

Add support for markdown cells in positron notebooks #2563

Add support for markdown cells in positron notebooks #2563

Conversation

nstrayer commented Mar 28, 2024

Intent

Approach

seeM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarenerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment