Skip to content

fix: support library downloads with virtual fs uris#1910

Draft
smorrisj wants to merge 6 commits into
mainfrom
bug/web-table-dl
Draft

fix: support library downloads with virtual fs uris#1910
smorrisj wants to merge 6 commits into
mainfrom
bug/web-table-dl

Conversation

@smorrisj

@smorrisj smorrisj commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary:
Library downloads failed in virtual file systems (e.g., vscode-local:// scheme from code-server's "Show Local" folder) fail because the code only supported local file system paths. Additionally, large table downloads could cause memory exhaustion due to missing backpressure handling in the streaming logic.

Add browser download support for non-file schemes, and fix the underlying stream backpressure issue to prevent memory problems on large libraries.

[index.ts]

  • Added streamTableToBrowserDownload() method with single-use token authentication, HTTP server, and browser download pipeline for virtual file systems
  • Refactor download command to detect file scheme and route to appropriate handler

[LibraryModel.ts]

  • Replace forEach with for...of loop to enable async/await backpressure handling
  • Add drain event waiting when write returns false to prevent memory exhaustion on large tables
  • Remove unused TableRow import

Testing:
See added unit tests. Additionally built a vsce package and tested in analytics pro and workbench.

TODOS

  • Make the streaming timeout configurable.

@smorrisj smorrisj changed the title fix: support table download with virtual fs uris fix: support library downloads with virtual fs uris Jun 16, 2026
smorrisj added 3 commits June 17, 2026 08:50
Signed-off-by: Joe Morris <Joe.Morris@sas.com>
Signed-off-by: Joe Morris <Joe.Morris@sas.com>
Signed-off-by: Joe Morris <Joe.Morris@sas.com>

@scottdover scottdover left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand all of browserDownload, but it looks okay to me. And it looks like we either use browser download or what we had before, so I'm good w/ it :)

const DOWNLOAD_TOKEN_PARAM = "token";
const DEFAULT_DOWNLOAD_FILENAME = "table.csv";

export function isValidDownloadToken(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't love that we're only exposing these functions so that we can test them. I'd prefer just running all tests through what's available in the public interface, but I'm guessing it's likely easier this way and requires less setup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants