Skip to content

Conversation

softwarenerd
Copy link
Contributor

@softwarenerd softwarenerd commented May 17, 2025

Description

This PR addresses #397 by adding a thumbnail cache feature to plots. Now, when Positron is reloaded, the Plots pane will show thumbnails for all the current plots.

Here's a screen recording of this in action:

Screen.Recording.2025-05-16.at.7.02.52.PM.mov

Tests:
@:plots

Release Notes

New Features

  • N/A

Bug Fixes

QA Notes

Please note that #7584 has not been addressed yet. It is a separate bug unrelated to this PR.

Copy link

github-actions bot commented May 17, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:plots

readme  valid tags

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a thumbnail cache feature so that the plots pane can restore thumbnails after a reload.

  • Introduce getCachedPlotThumbnailURI in the service interface and stub in tests
  • Implement storage, retrieval, and cleanup of cached thumbnail descriptors in PositronPlotsService
  • Update thumbnail components to initialize from cache before the plot renders again

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/vs/workbench/services/positronPlots/test/common/testPositronPlotsService.ts Added stub getCachedPlotThumbnailURI to the test service
src/vs/workbench/services/positronPlots/common/positronPlots.ts Added getCachedPlotThumbnailURI to the public service interface
src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts Implemented caching logic: store on save, load on init, and provide retrieval method
src/vs/workbench/contrib/positronPlots/browser/components/webviewPlotThumbnail.tsx Updated WebviewPlotThumbnail to initialize from cache and subscribe/unsubscribe correctly
src/vs/workbench/contrib/positronPlots/browser/components/dynamicPlotThumbnail.tsx Updated DynamicPlotThumbnail to initialize from cache (needs cleanup for its listener)
Comments suppressed due to low confidence (2)

src/vs/workbench/services/positronPlots/test/common/testPositronPlotsService.ts:216

  • Add unit tests to verify that thumbnail URIs are correctly stored to and retrieved from the cache, including edge cases with missing or invalid entries.
getCachedPlotThumbnailURI(plotId: string) {

src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts:96

  • [nitpick] Rename thumbnailURI to thumbnailUri to match camelCase conventions used elsewhere (e.g., plotClient.thumbnailUri).
readonly thumbnailURI: string;

Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

LGTM!

Nit (performance) -- since we are caching the full resolution plot (for PlotClients), we could use that to cache to seed the PlotClient with an initial render. i.e. when we list plots from the runtimes, we could match each up with its cached render and set that as its last render, so that (if dimensions haven't changed) we don't need to ask the runtime to render again if dimensions haven't changed.

@softwarenerd softwarenerd merged commit 74fd52a into main May 20, 2025
8 checks passed
@softwarenerd softwarenerd deleted the fix/plots-pane-loses-thumbnails-on-reload branch May 20, 2025 00:23
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants