chore: DH-24027: Addressing some npm package gaps identified with CLI work by bmingles · Pull Request #2803 · deephaven/web-client-ui

bmingles · 2026-11-09T20:33:24Z

This PR addresses some gaps identified during CLI work that allow less custom code in downstream consumers

@deephaven/jsapi-nodejs: features the CLI needed, so it can drop its own download
code
- Restrict where code is loaded from: isTrustedUrl is checked on the first URL and
  on every redirect. The CLI uses this to allow only HTTPS, or plain HTTP to a local
  server.
- Follow redirects: with a limit, as fetch does. Before, a redirect response's body
  was saved as the module.
- Fail clearly on HTTP errors and timeouts: they reject with HttpError /
  DownloadTimeoutError instead of saving an error page as the module, or rejecting
  with undefined.
- Configurable timeout: for large downloads like the ~5 MB irisapi, where the fixed
  5s was too short.
- Cancellation: an AbortSignal stops an in-flight download or a pending retry.
- Download options through the loaders: loadModules / loadDhModules now pass trust
  checks, timeout, cancellation and a custom logger through to each download.
  Before, they always used the defaults and logged to console.
@deephaven/jsapi-utils: auth handler helpers that work in Node
- Auth handler constants and fetchAuthHandlers(client): usable from Node, so the CLI
  no longer needs its own copies. Before, they lived in React/app packages.
- AuthHandlers parsing matches the server: accepts commas and/or spaces, so handlers
  listed as a, b are no longer misread.

semanticdiff-com · 2026-11-09T20:33:29Z

Review changes with

Changed Files

File	Status
packages/jsapi-nodejs/src/loaderUtils.ts	52% smaller
packages/auth-plugins/src/AuthHandlerTypes.ts	43% smaller
packages/jsapi-nodejs/src/serverUtils.ts	21% smaller
packages/jsapi-nodejs/node-tests/loaderUtils.test.mjs	1% smaller
packages/app-utils/src/plugins/PluginUtils.ts	0% smaller
packages/jsapi-nodejs/README.md	Unsupported file format
packages/jsapi-nodejs/src/errorUtils.ts	0% smaller
packages/jsapi-nodejs/src/serverUtils.test.ts	0% smaller
packages/jsapi-utils/src/AuthUtils.test.ts	0% smaller
packages/jsapi-utils/src/AuthUtils.ts	0% smaller
packages/jsapi-utils/src/index.ts	0% smaller

codecov · 2026-11-09T20:36:40Z

Codecov Report

❌ Patch coverage is 91.07143% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.89%. Comparing base (fe173e9) to head (a771a71).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/jsapi-nodejs/src/serverUtils.ts	93.25%	6 Missing ⚠️
packages/jsapi-nodejs/src/loaderUtils.ts	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2803      +/-   ##
==========================================
+ Coverage   53.61%   53.89%   +0.27%     
==========================================
  Files         830      831       +1     
  Lines       47936    48062     +126     
  Branches    12566    12601      +35     
==========================================
+ Hits        25703    25905     +202     
+ Misses      22213    22106     -107     
- Partials       20       51      +31

Flag	Coverage Δ
unit	`53.89% <91.07%> (+0.27%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

Removes urlToStorageDirName so loadModules keeps the existing cache directory names. Adds tests pinning urlToDirectoryName's output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

The server sends the raw AuthHandlers property and accepts commas or spaces as separators, so space-separated handlers were returned as a single entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

Checks the scheduled retry delay and that abort clears the pending retry timer, rather than relying on wall-clock bounds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

- Reject instead of throwing uncaught when isTrustedUrl or the request setup throws during a redirect or retry - Treat any HTTP error for dh-internal.js as missing, not just 404 - Log the original url when retrying, since that is what is retried - Skip errorTransform and download transforms once aborted Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

Other statuses, such as a proxy 5xx, indicate a real failure rather than the removed file, so they are rethrown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

Copilot

🟡 Changes recommended

Discarded responses can leave sockets open and keep the CLI running indefinitely, while response-stream network errors reject downloads without using remaining retries.

2 open issues

What changed in this PR

This PR expands shared JS API utilities so downstream CLI consumers need less custom download and authentication code.

Changes:

Adds configurable download trust checks, redirects, timeouts, cancellation, logging, and typed errors.
Passes download options through module loaders and adds downloader and loader tests.
Moves authentication helpers into jsapi-utils, preserves existing exports, and supports comma- or whitespace-separated handlers.

File	Description
`packages/jsapi-utils/src/index.ts`	Exports shared authentication helpers.
`packages/jsapi-utils/src/AuthUtils.ts`	Adds authentication constants, parsing, and fetching.
`packages/jsapi-utils/src/AuthUtils.test.ts`	Tests handler parsing and fetching.
`packages/jsapi-nodejs/src/serverUtils.ts`	Implements configurable download behavior.
`packages/jsapi-nodejs/src/serverUtils.test.ts`	Tests downloads, redirects, errors, retries, and cancellation.
`packages/jsapi-nodejs/src/loaderUtils.ts`	Forwards download options and checks cancellation before processing results.
`packages/jsapi-nodejs/src/errorUtils.ts`	Adds untrusted-URL and download-timeout errors.
`packages/jsapi-nodejs/README.md`	Documents download options and errors.
`packages/jsapi-nodejs/node-tests/loaderUtils.test.mjs`	Adds native loader download and cancellation coverage.
`packages/auth-plugins/src/AuthHandlerTypes.ts`	Re-exports shared authentication constants.
`packages/app-utils/src/plugins/PluginUtils.ts`	Uses and re-exports the shared handler parser.

_{Review effort: Balanced}

A request superseded by a redirect could fail the active download when its discarded body stalled, and a request whose download had settled never closed its socket. Timeouts now always close their own request, and only the current request can fail or retry the download. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

Copilot

🟡 Changes recommended

Package-root imports fail on supported Node 16 runtimes because a transitive dependency extends the unavailable global CustomEvent, preventing the helpers from running.

1 open issue

Root import crashes on Node 16 without global CustomEvent · New

2 resolved since last review

_{Review effort: Balanced}

mattrunyon

I mostly reviewed source code and not tests

mattrunyon · 2026-11-11T00:16:15Z

+  /** Socket idle timeout in milliseconds per request. Defaults to 5000. */
+  timeoutMs?: number;


Is this timeout without any messages or timeout for the entire request? For one request or is it supposed to include any retries?

I updated the comment to clarify. It's per request and resets on data received. Retries and redirects each count as a request.

mattrunyon · 2026-11-11T00:19:18Z

+ * Download the text content of a URL using `node:http` / `node:https`. These
+ * are used instead of `fetch` so that proxy and certificate handling patched
+ * into the Node agents (e.g. by VS Code) still applies.


You might still be able to use fetch if you explicitly set the agent from undici. But not sure and I don't know if it buys us anything else that the node http methods don't have

Claude was being "helpful" by adding that comment, but the historical reason that I used node:http was that I copy / modified the logic from existing code. This was also before undici fetch was a thing on my radar. Possible we could get fetch to work, but I'd consider it out of scope and trying to avoid changing things that are already working in this PR

mattrunyon · 2026-11-11T00:55:50Z

+          // A throw here would escape the response callback as an uncaught
+          // exception instead of rejecting the download.
+          let nextUrl: URL;
+          try {
+            nextUrl = new URL(location, currentUrl);
+          } catch {
+            fail(
+              new HttpError(
+                statusCode,
+                `Invalid redirect location "${location}" from "${currentUrl}"`
+              )
+            );
+            return;
+          }


So fetch should be able to follow redirects on its own. Could be a benefit of switching to it if the "VSCode injects agent info" or w/e it was is feasible w/ fetch.

I don't think the isTrustedUrl would work for following the redirects though if it's meant to be rechecked. Was there a specific ask or purpose for that check?

The CLI has specific rules for which URLs it supports so it had to re-implement a lot of this code. The isTrustedUrl was the agent request to keep logic upstream while allowing consumers to customize which redirects they support.

mattrunyon · 2026-11-11T01:07:09Z

+            statusCode === 404
+              ? `File not found: "${currentUrl}"`
+              : `HTTP ${statusCode} ${
+                  STATUS_CODES[statusCode] ?? ''
+                }: "${currentUrl}"`


I don't know if the 404 handling specifically gives any additional benefit. Could just simplify to the one status code message without the special 404. The code is Not Found already

It's just keeping what the 404 message was before the change.

mattrunyon · 2026-11-11T18:53:40Z

+      req
+        .on('timeout', () => {
+          // Only the current request can fail the download. A request that
+          // was superseded by a redirect, or whose download already settled,
+          // is just closed.
+          if (!settled && req === activeRequest) {
+            logger.error('Failed download of url:', currentUrl);
+            fail(new DownloadTimeoutError(currentUrl, timeoutMs));


I get the idea of this, but I'm surprised a request can timeout after it would have returned w/ a redirect or returned w/ a body

After a redirect we call res.resume() to discard the old response's body, so its socket stays open until that body is fully read. If the body stalls, that request can time out after we've already started the next request. The guard closes that old request without failing the current download.

mattrunyon · 2026-11-11T18:55:13Z

+        .on('error', e => {
+          // A superseded request must not start a second retry chain.
+          if (settled || req !== activeRequest) {


Same thing about a request that redirected/has a body later throwing an error

You're right for the redirect case: once a response has arrived, Node reports socket failures on the response, not as 'error' on the request, so a redirected request won't reach this handler. The case it guards is destroy() on a request that hasn't had a response yet: onAbort rejects and then destroys (line 122), and the timeout handler fails and then destroys (line 254). Both fire 'error' after the download has finished, and without the settled check that would schedule a retry. Added tests for both and updated the if check and comment on this block

mattrunyon · 2026-11-11T19:00:02Z

+export function getAuthHandlers(
+  authConfigValues: Map<string, string>
+): string[] {


Just curious why this is exported. I assume we'd want the public API to just be fetchAuthHandlers and there's not a reason to use this. Just for testing? Not a big deal

It was previously exported from PluginUtils. It's now re-exported to avoid api change. I updated the re-export with @deprecated which I had previously forgotten.

Copilot

🔵 Needs a closer look

Synchronous cancellation by a custom retry logger can still schedule another request and keep the process alive through the retry delay, requiring a post-logger settled check and regression test.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Prevent retry scheduling after logger-triggered cancellation

packages/jsapi-nodejs/src/serverUtils.ts:264

If a custom logger synchronously calls controller.abort() while logging a retry, this code still schedules another request after cancellation and can keep the process alive for the retry delay. Recheck settled after the logger call before scheduling the timer, and add a test for synchronous logger-triggered cancellation.

1 resolved since last review

Root import crashes on Node 16 without global CustomEvent

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

Human assessment is needed because cancellation after a redirect may leave earlier streaming requests open, keeping the CLI alive despite the abort.

No open issues 🎉

_{Review effort: Balanced}

Copilot

🟢 Approval recommended

No active review issues remain for the shared download and authentication utility changes.

No open issues 🎉

_{Review effort: Balanced}

Addressing some npm package gaps identified with CLI work (#DH-24027)

c528580

bmingles added 8 commits November 9, 2026 16:22

fix: Reject invalid redirect Location instead of throwing uncaught

9c08d64

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

refactor: Keep urlToDirectoryName for module storage dirs

5387c78

Removes urlToStorageDirName so loadModules keeps the existing cache directory names. Adds tests pinning urlToDirectoryName's output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

test: Use urlToDirectoryName in node tests and update plan

060fa62

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

fix: Split AuthHandlers on whitespace as well as commas

316c7a3

The server sends the raw AuthHandlers property and accepts commas or spaces as separators, so space-separated handlers were returned as a single entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

test: Assert retry timers directly instead of elapsed time

88b746e

Checks the scheduled retry delay and that abort clears the pending retry timer, rather than relying on wall-clock bounds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

fix: Only treat a 404 for dh-internal.js as missing

cca3914

Other statuses, such as a proxy 5xx, indicate a real failure rather than the removed file, so they are rethrown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

chore: Remove DH-24027 plan doc

6a0b30b

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (#DH-24027)

bmingles marked this pull request as ready for review November 10, 2026 18:42

Copilot AI balanced review requested due to automatic review settings November 10, 2026 18:42

Copilot started reviewing on behalf of bmingles November 10, 2026 18:43

View session

Copilot AI reviewed Nov 10, 2026

View reviewed changes

Comment thread packages/jsapi-nodejs/src/serverUtils.ts Resolved

Comment thread packages/jsapi-nodejs/src/serverUtils.ts Resolved

Copilot AI balanced review requested due to automatic review settings November 10, 2026 20:50

Copilot started reviewing on behalf of bmingles November 10, 2026 20:51

View session

Copilot AI reviewed Nov 10, 2026

View reviewed changes

Comment thread packages/jsapi-utils/src/index.ts Resolved

bmingles requested review from a team and mattrunyon and removed request for a team November 10, 2026 22:12

mattrunyon requested changes Nov 11, 2026

View reviewed changes

Addressed review comments (#DH-24027)

b8c48e9

Copilot AI balanced review requested due to automatic review settings November 12, 2026 01:23

Copilot started reviewing on behalf of bmingles November 12, 2026 01:23

View session

Copilot AI reviewed Nov 12, 2026

View reviewed changes

Mark re-export as @deprecated (#DH-24027)

d94da09

Copilot AI balanced review requested due to automatic review settings November 12, 2026 01:40

Copilot started reviewing on behalf of bmingles November 12, 2026 01:41

View session

Copilot AI reviewed Nov 12, 2026

View reviewed changes

Added tests for timeout and abort case and simplified check (#DH-24027)

a771a71

Copilot AI balanced review requested due to automatic review settings November 12, 2026 02:46

Copilot started reviewing on behalf of bmingles November 12, 2026 02:47

View session

bmingles requested a review from mattrunyon November 12, 2026 02:48

Copilot AI reviewed Nov 12, 2026

View reviewed changes

		/** Socket idle timeout in milliseconds per request. Defaults to 5000. */
		timeoutMs?: number;

Conversation

bmingles commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

semanticdiff-com Bot commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

mattrunyon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmingles Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmingles Nov 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

bmingles commented Nov 9, 2026 •

edited

Loading

semanticdiff-com Bot commented Nov 9, 2026 •

edited

Loading

codecov Bot commented Nov 9, 2026 •

edited

Loading

bmingles Nov 11, 2026 •

edited

Loading

bmingles Nov 12, 2026 •

edited

Loading