Skip to content

fix(ws): restore live websocket URL helpers - #11465

Closed
RohitChauhan13 wants to merge 2 commits into
diegosouzapw:release/v3.8.51from
RohitChauhan13:fix/restore-live-ws-export-helpers
Closed

RohitChauhan13 wants to merge 2 commits into
diegosouzapw:release/v3.8.51from
RohitChauhan13:fix/restore-live-ws-export-helpers

Conversation

@RohitChauhan13

Copy link
Copy Markdown

Fix PR: Restore live WebSocket URL export compatibility

Summary

This fix resolves the build error caused by a stale import in the live dashboard hook:

The fix restores the expected helper API while preserving the newer runtime URL behavior for LIVE_WS_PUBLIC_URL and NEXT_PUBLIC_LIVE_WS_PUBLIC_URL.


Root cause

The module src/shared/utils/wsPath.ts was refactored to expose deriveLiveWsPath() and resolveLiveWsPublicUrl(), but the live dashboard hook and the regression tests were still importing older helpers:

  • resolveLiveWsUrl
  • sanitizeLiveWsPort

That mismatch caused the build-time import error:

Export resolveLiveWsUrl doesn't exist in target module


Files changed

What was added

The module now exports:

  • sanitizeLiveWsPort(port)
  • resolveLiveWsUrl({ ... })
  • deriveLiveWsPath(publicUrl?)
  • resolveLiveWsPublicUrl(env?)
  • getLiveWsPath()

This keeps compatibility with the dashboard hook while preserving the runtime-aware reverse-proxy logic introduced for the websocket handshake fix.


Why this is correct

The restored helper functions implement the intended behavior:

  1. Prefer an explicit wsUrl when the caller passes one.
  2. Honor the public WebSocket URL from the handshake response.
  3. Replace the default port with the runtime-reported port when valid.
  4. Preserve the path from the handshake when it is a valid websocket path.
  5. Keep the fallback default URL when the handshake data is invalid or absent.
  6. Continue accepting LIVE_WS_PUBLIC_URL as the runtime override while keeping NEXT_PUBLIC_LIVE_WS_PUBLIC_URL as a fallback.

This matches the design expected by the dashboard and prevents the regression behind reverse-proxy deployments.


Validation

I validated the fix with the existing targeted regression tests:

Command run:

cd 'C:\ROHIT\OmniRoute'; node --import tsx/esm --test tests/unit/live-ws-url-11331.test.ts tests/unit/live-ws-public-url-11331.test.ts

Result:

  • 18 tests passed
  • 0 failed

This command covers:


Restore sanitizeLiveWsPort() and resolveLiveWsUrl() exports that were
missing after the runtime public URL refactor, fixing a build error
where useLiveDashboard.ts could not resolve these named imports.
@ntdatt812

Copy link
Copy Markdown
Contributor

I hit this from the other side — the production build is red on every open PR right now — and had a restore ready before I found yours. Yours is better than mine, so I am dropping mine. Two things from the verification that may be worth having on the record.

Your patch passes the pinned test. tests/unit/live-ws-url-11331.test.ts is present on the release/v3.8.51 tip (6435f618f) but cannot even load there:

SyntaxError: The requested module '../../src/shared/utils/wsPath.ts'
does not provide an export named 'resolveLiveWsUrl'

With your diff applied to that tip: 11 pass, 0 fail, ✔ sanitizeLiveWsPort and ✔ resolveLiveWsUrl.

On the root cause — the helpers were not dropped by a refactor. They were added by #11388, which merged into release/v3.8.50 (merge commit f93fecd86) touching four files:

changelog.d/fixes/11388-live-ws-handshake-port.md
src/hooks/useLiveDashboard.ts
src/shared/utils/wsPath.ts
tests/unit/live-ws-url-11331.test.ts

Three of the four are on the v3.8.51 tip. src/shared/utils/wsPath.ts is not, and f93fecd86 is not an ancestor of that tip — git diff 6435f618f:src/shared/utils/wsPath.ts release/v3.8.50:src/shared/utils/wsPath.ts is exactly +58 -0, that one hunk and nothing else. So the branch was assembled per-file rather than merged, and this PR lost one file of four. Worth a glance at whether anything else came across the same way.

Your version is stricter than the original in four places, and none of them is covered by the tests. I diffed the two implementations by running both:

input original (v3.8.50) this PR
sanitizeLiveWsPort("0x10") 16 null
sanitizeLiveWsPort("1e3") 1000 null
resolveLiveWsUrl({explicit: "http://weird"}) "http://weird" falls back to the default
resolveLiveWsUrl({handshakeUrl: "https://nope"}) "https://nope" falls back to the default

All four are improvements — the original happily handed an http:// URL to a WebSocket client and read "0x10" as a port, because it used bare Number() and a truthiness check on explicit. I wrote that original, so this is not a complaint.

But the 11 existing assertions pass either way, which means nothing stops the strictness being refactored back out. If you want it pinned, four one-liners in live-ws-url-11331.test.ts would do it. Happy to send them as a follow-up once this lands, or you can fold them in here.

@RohitChauhan13

Copy link
Copy Markdown
Author

I hit this from the other side — the production build is red on every open PR right now — and had a restore ready before I found yours. Yours is better than mine, so I am dropping mine. Two things from the verification that may be worth having on the record.

Your patch passes the pinned test. tests/unit/live-ws-url-11331.test.ts is present on the release/v3.8.51 tip (6435f618f) but cannot even load there:

SyntaxError: The requested module '../../src/shared/utils/wsPath.ts'
does not provide an export named 'resolveLiveWsUrl'

With your diff applied to that tip: 11 pass, 0 fail, ✔ sanitizeLiveWsPort and ✔ resolveLiveWsUrl.

On the root cause — the helpers were not dropped by a refactor. They were added by #11388, which merged into release/v3.8.50 (merge commit f93fecd86) touching four files:

changelog.d/fixes/11388-live-ws-handshake-port.md
src/hooks/useLiveDashboard.ts
src/shared/utils/wsPath.ts
tests/unit/live-ws-url-11331.test.ts

Three of the four are on the v3.8.51 tip. src/shared/utils/wsPath.ts is not, and f93fecd86 is not an ancestor of that tip — git diff 6435f618f:src/shared/utils/wsPath.ts release/v3.8.50:src/shared/utils/wsPath.ts is exactly +58 -0, that one hunk and nothing else. So the branch was assembled per-file rather than merged, and this PR lost one file of four. Worth a glance at whether anything else came across the same way.

Your version is stricter than the original in four places, and none of them is covered by the tests. I diffed the two implementations by running both:

input original (v3.8.50) this PR
sanitizeLiveWsPort("0x10") 16 null
sanitizeLiveWsPort("1e3") 1000 null
resolveLiveWsUrl({explicit: "http://weird"}) "http://weird" falls back to the default
resolveLiveWsUrl({handshakeUrl: "https://nope"}) "https://nope" falls back to the default
All four are improvements — the original happily handed an http:// URL to a WebSocket client and read "0x10" as a port, because it used bare Number() and a truthiness check on explicit. I wrote that original, so this is not a complaint.

But the 11 existing assertions pass either way, which means nothing stops the strictness being refactored back out. If you want it pinned, four one-liners in live-ws-url-11331.test.ts would do it. Happy to send them as a follow-up once this lands, or you can fold them in here.

Thanks for the detailed verification and for pointing out the untested stricter behavior. I’ve added regression coverage for all four cases you mentioned:

  • Hexadecimal port strings (0x10)
  • Scientific-notation port strings (1e3)
  • Invalid explicit HTTP URLs
  • Invalid handshake HTTPS URLs

The test suite now passes with 15/15 tests passing, 0 failures.

@ntdatt812

Copy link
Copy Markdown
Contributor

Confirmed on my side. Applied your head (1e133b0cf) to the release/v3.8.51 tip (6435f618f) and ran the file:

✔ sanitizeLiveWsPort
✔ resolveLiveWsUrl
ℹ tests 15   ℹ pass 15   ℹ fail 0

I also checked the two port tests actually hold the line rather than just passing, by reverting sanitizeLiveWsPort to the loose original (Number() on any string, integer + range check) and re-running:

✖ rejects hexadecimal numeric strings
✖ rejects scientific-notation numeric strings
ℹ pass 13   ℹ fail 2

So those two are load-bearing — the strictness cannot be refactored back out silently now.

I did not get a clean mutation on the URL-guard half; my substitution did not match your code shape and I did not want to hand-edit your logic to force it. Worth saying rather than implying I checked all four.

This unblocks the production build on every open PR, so from my side it is good to go.

@RohitChauhan13

Copy link
Copy Markdown
Author

Confirmed on my side. Applied your head (1e133b0cf) to the release/v3.8.51 tip (6435f618f) and ran the file:

✔ sanitizeLiveWsPort
✔ resolveLiveWsUrl
ℹ tests 15   ℹ pass 15   ℹ fail 0

I also checked the two port tests actually hold the line rather than just passing, by reverting sanitizeLiveWsPort to the loose original (Number() on any string, integer + range check) and re-running:

✖ rejects hexadecimal numeric strings
✖ rejects scientific-notation numeric strings
ℹ pass 13   ℹ fail 2

So those two are load-bearing — the strictness cannot be refactored back out silently now.

I did not get a clean mutation on the URL-guard half; my substitution did not match your code shape and I did not want to hand-edit your logic to force it. Worth saying rather than implying I checked all four.

This unblocks the production build on every open PR, so from my side it is good to go.

Thanks again for verifying the fix. Since this issue currently affects the production build and can cause problems for new users using the affected functionality, could you please help get this PR merged as soon as possible? This would prevent other users from running into the same issue. Thanks!

@ntdatt812 ntdatt812 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 should be straight about what I can and cannot do here: I have no write access to this repo, so I cannot merge this or put it in the queue. Approving is the whole of my leverage, and I have done that above.

What I can add is the part a maintainer will want before queueing it, stated as scope rather than as a blanket "LGTM":

  • Verified: your head 1e133b0cf applied to the release/v3.8.51 tip 6435f618f gives 15 pass / 0 fail. The two port assertions are load-bearing — reverting sanitizeLiveWsPort to the loose original turns exactly those two red. Without this branch the pinned test file cannot even load on that tip (does not provide an export named 'resolveLiveWsUrl'), so the failure is not subtle.
  • Not verified by me: the URL-guard half, in the mutation sense. The assertions pass; I did not prove they would fail against a looser implementation.
  • Blast radius: restores one file, src/shared/utils/wsPath.ts, that #11388 added and the per-file assembly of this release branch dropped. Nothing else on the tip imports a symbol this changes.

On timing — merges here go through the Mergify queue, and both of its checks are currently neutral, meaning it has not been queued rather than that anything failed. That step needs someone with write access; the semgrep scan is already green. Nothing further is needed from either of us to make it mergeable.

If it helps make the case: this is not only a new-user problem. Every open PR targeting release/v3.8.51 currently inherits the broken production build, so a red run on any of them says nothing about that PR until this lands.

@RohitChauhan13

Copy link
Copy Markdown
Author

I should be straight about what I can and cannot do here: I have no write access to this repo, so I cannot merge this or put it in the queue. Approving is the whole of my leverage, and I have done that above.

What I can add is the part a maintainer will want before queueing it, stated as scope rather than as a blanket "LGTM":

  • Verified: your head 1e133b0cf applied to the release/v3.8.51 tip 6435f618f gives 15 pass / 0 fail. The two port assertions are load-bearing — reverting sanitizeLiveWsPort to the loose original turns exactly those two red. Without this branch the pinned test file cannot even load on that tip (does not provide an export named 'resolveLiveWsUrl'), so the failure is not subtle.
  • Not verified by me: the URL-guard half, in the mutation sense. The assertions pass; I did not prove they would fail against a looser implementation.
  • Blast radius: restores one file, src/shared/utils/wsPath.ts, that fix(dashboard): honour the live WebSocket port the handshake reports (#11331) #11388 added and the per-file assembly of this release branch dropped. Nothing else on the tip imports a symbol this changes.

On timing — merges here go through the Mergify queue, and both of its checks are currently neutral, meaning it has not been queued rather than that anything failed. That step needs someone with write access; the semgrep scan is already green. Nothing further is needed from either of us to make it mergeable.

If it helps make the case: this is not only a new-user problem. Every open PR targeting release/v3.8.51 currently inherits the broken production build, so a red run on any of them says nothing about that PR until this lands.

Thanks for the detailed verification and for approving the PR.

I also completed the URL-guard mutation check locally to cover the part you could not verify:

  • Temporarily removed the ws:// / wss:// guards from both explicit and handshakeUrl.
  • Reran tests/unit/live-ws-url-11331.test.ts.
  • The two URL regression tests failed as expected:
    • rejects an explicit non-websocket URL
    • rejects a handshake URL that is not a websocket URL
  • Result with the looser implementation: 13 passed / 2 failed.
  • Restored the URL guards and reran the suite: 15 passed / 0 failed.

So the URL-guard assertions are load-bearing as well.

Thanks again for the approval and for providing the verification details for the maintainer.

@ntdatt812

Copy link
Copy Markdown
Contributor

Reproduced your mutation independently, and it holds — same two, same counts:

✖ rejects an explicit non-websocket URL
✖ rejects a handshake URL that is not a websocket URL
ℹ tests 15   ℹ pass 13   ℹ fail 2

I dropped only the two scheme guards inside resolveLiveWsUrl (if (trimmed.startsWith("ws://") || …) → if (trimmed)), leaving the identical-looking guard in resolveLiveWsPublicUrl and the one in deriveLiveWsPath untouched, so the two failures are attributable to the guards under test rather than to a wider edit. Restored: 15 pass, 0 fail. All four of the behaviours I flagged as untested are now pinned.

More important: the base moved under this PR, so my earlier "nothing further is needed" is out of date. The tip is now a179ffed5, and 04dba0460 (#11434) landed on src/shared/utils/wsPath.ts itself — it added resolveLiveWsPublicUrl and rewired getLiveWsPath to use it. A base commit touching the exact file a PR restores is worth re-measuring rather than assuming.

Re-measured on the new tip:

Also worth correcting something I wrote earlier, since it will read as odd to anyone checking now: I said wsPath.ts was not on the tip. The file was always there — what the per-file assembly dropped was #11388's additions to it, the sanitizeLiveWsPort and resolveLiveWsUrl block. The diff I quoted (+58 -0) was measuring that, but the sentence overstated it. The consequence is unchanged: those two exports are still absent from a179ffed5, so the pinned test still cannot load on the tip today, and this PR is still what fixes it.

@RohitChauhan13

Copy link
Copy Markdown
Author

Reproduced your mutation independently, and it holds — same two, same counts:

✖ rejects an explicit non-websocket URL
✖ rejects a handshake URL that is not a websocket URL
ℹ tests 15   ℹ pass 13   ℹ fail 2

I dropped only the two scheme guards inside resolveLiveWsUrl (if (trimmed.startsWith("ws://") || …) → if (trimmed)), leaving the identical-looking guard in resolveLiveWsPublicUrl and the one in deriveLiveWsPath untouched, so the two failures are attributable to the guards under test rather than to a wider edit. Restored: 15 pass, 0 fail. All four of the behaviours I flagged as untested are now pinned.

More important: the base moved under this PR, so my earlier "nothing further is needed" is out of date. The tip is now a179ffed5, and 04dba0460 (#11434) landed on src/shared/utils/wsPath.ts itself — it added resolveLiveWsPublicUrl and rewired getLiveWsPath to use it. A base commit touching the exact file a PR restores is worth re-measuring rather than assuming.

Re-measured on the new tip:

Also worth correcting something I wrote earlier, since it will read as odd to anyone checking now: I said wsPath.ts was not on the tip. The file was always there — what the per-file assembly dropped was #11388's additions to it, the sanitizeLiveWsPort and resolveLiveWsUrl block. The diff I quoted (+58 -0) was measuring that, but the sentence overstated it. The consequence is unchanged: those two exports are still absent from a179ffed5, so the pinned test still cannot load on the tip today, and this PR is still what fixes it.

Thanks for re-checking the mutation independently and for verifying the PR against the updated base.

Glad to see that all four regression behaviours are now confirmed as load-bearing, and that the merged result on a179ffed5 is clean with no duplicate declarations or merge conflicts.

Also thanks for clarifying the earlier wsPath.ts wording — the file itself was present, while the additions from #11388 were missing.

Everything looks good from my side. Hopefully it can now be queued through Mergify and merged. Thanks again for the thorough verification and approval.

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for catching this — closing in favor of #11502, which already landed the identical fix and merged today: resolveLiveWsUrl and sanitizeLiveWsPort are both present again in src/shared/utils/wsPath.ts on the current release/v3.8.51 tip, alongside the newer resolveLiveWsPublicUrl/deriveLiveWsPath.

No distinct value remains to carry forward — same root cause (the #11331 fix's exports getting overwritten by a same-day competing PR's hunk), same fix shape. Appreciate the independent catch.

@RohitChauhan13

Copy link
Copy Markdown
Author

Thanks for catching this — closing in favor of #11502, which already landed the identical fix and merged today: resolveLiveWsUrl and sanitizeLiveWsPort are both present again in src/shared/utils/wsPath.ts on the current release/v3.8.51 tip, alongside the newer resolveLiveWsPublicUrl/deriveLiveWsPath.

No distinct value remains to carry forward — same root cause (the #11331 fix's exports getting overwritten by a same-day competing PR's hunk), same fix shape. Appreciate the independent catch.

Thanks for the update and for confirming that the same fix has landed in #11502.

Glad the root cause and the fix were independently confirmed. Appreciate the thorough verification and review throughout this PR.

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