fix(desktop): use searchParams not string concat for terminal WS URL by saddlepaddle · Pull Request #4159 · superset-sh/superset

saddlepaddle · 2026-05-07T02:11:11Z

Summary

#4149 appended ?themeType=... to a URL that already had ?token=<jwt>, producing ?token=jwt?themeType=light. Two failures:

Auth breaks for every terminal. Server's c.req.query(\"token\") (packages/host-service/src/app.ts:143) reads everything after the first ?token= up to the next & — so the parsed token is eyJ...?themeType=light, which fails validateToken(). Every WS upgrade gets 401. This is the "every terminal unreachable" symptom that surfaced on the desktop-v1.8.5 draft.
The themeType forwarding doesn't work. Server's c.req.query(\"themeType\") returns null because there's no real &themeType=... in the URL — it's part of the token value. So the host-side respawn fallback that fix(terminal): respawn lost sessions on attach; route WS errors to log #4149 was trying to make theme-aware silently uses defaults.

Fix

Build the URL via URL.searchParams so the existing token param is preserved and themeType lands as a real query parameter:

const baseWebsocketUrl = useWorkspaceWsUrl(\`/terminal/\${terminalId}\`);
const themedUrl = new URL(baseWebsocketUrl);
themedUrl.searchParams.set(\"themeType\", themeType);
const websocketUrl = themedUrl.toString();

Preserves the PR's design intent (`baseWebsocketUrl` stays stable so theme toggles don't trigger reconnects via the effect deps; `websocketUrl` is read off the ref inside the effect).

Test plan

Verified locally: terminals connect again
Confirmed via Node URL parser that the malformed string is parsed as token=\"eyJ...?themeType=light\", themeType=null
Re-tag desktop-v1.8.5 after merge, confirm the new draft build shows working terminals

Summary by cubic

Fixes terminal WebSocket URL building in Desktop by using URL.searchParams instead of string concatenation. Preserves the existing token and correctly forwards themeType, restoring auth and theme-aware respawn.

Bug Fixes
- Build terminal WS URL via URL.searchParams to append themeType without corrupting token.
- Terminals connect again (no 401). Server now receives a real themeType query param.

^{Written for commit 8b3502b. Summary will update on new commits.}

Summary by CodeRabbit

Refactor
- Improved internal WebSocket URL construction for terminal components to use standard URL handling methods.

#4149 appended `?themeType=...` to a URL that already had `?token=<jwt>`, producing `?token=jwt?themeType=light`. Hono's `c.req.query("token")` then read the JWT with `?themeType=light` appended, failing `validateToken()` -> 401 on every WS upgrade. As a bonus, `c.req.query("themeType")` returned null, so the themeType forwarding this PR was trying to enable was dead code. Build the URL via URL.searchParams so the existing token param is preserved and themeType lands as a real query parameter.

coderabbitai · 2026-05-07T02:11:30Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 027e217e-f029-4275-bb65-7693b1d2697b

📥 Commits

Reviewing files that changed from the base of the PR and between 72e84fc and 8b3502b.

📒 Files selected for processing (1)

apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx

📝 Walkthrough

Walkthrough

The TerminalPane component's WebSocket URL construction was refactored to use the URL API with searchParams instead of string interpolation with encodeURIComponent. The computed URL is still assigned to the websocketUrl ref, and all downstream connection logic remains unchanged.

Changes

WebSocket URL Construction

Layer / File(s)	Summary
URL Building Refactor `apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx`	WebSocket URL created via `new URL(baseWebsocketUrl)` and `searchParams.set("themeType", themeType)` instead of string interpolation with `encodeURIComponent`. Ref synchronization and mount/reconnect logic remain unchanged.

Estimated Code Review Effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A URL doth change its form today,
From strings that splice in tangled way,
To params set with modern grace—
The WebSocket finds its rightful place! ✨

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch oval-sock

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

greptile-apps · 2026-05-07T02:13:24Z

Greptile Summary

This PR fixes a URL construction bug introduced in #4149 where appending ?themeType=... via string concatenation to a base WebSocket URL that already contained query parameters produced a malformed double-? URL. This corrupted the auth parameter value on the server side, causing every terminal WebSocket upgrade to fail and themeType to be unparseable.

Replaces string concatenation with new URL(baseWebsocketUrl).searchParams.set("themeType", themeType), correctly appending themeType as a proper ampersand-separated query parameter.
Preserves the intentional design where only baseWebsocketUrl (without themeType) drives the reconnect effect dependency, so theme toggles do not tear down live shells.

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted fix to a broken URL construction that was already causing 100% terminal connection failures.

The three-line change is correct: useWorkspaceWsUrl already returns a valid absolute URL string (it uses URL internally), so constructing new URL(baseWebsocketUrl) is safe. searchParams.set handles encoding automatically and places themeType as a proper ampersand-separated parameter rather than a second question mark. The intentional ref-based design that keeps themeType out of the reconnect effect dependencies is preserved unchanged.

No files require special attention — the change is confined to three lines within TerminalPane.tsx and has no ripple effects on other modules.

Important Files Changed

Filename	Overview
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx	Replaces string concatenation for WebSocket URL construction with URL.searchParams.set(), correctly placing themeType as a proper query parameter without breaking the existing auth parameter.

Sequence Diagram

sequenceDiagram
    participant C as TerminalPane (renderer)
    participant H as useWorkspaceWsUrl
    participant S as host-service (server)

    Note over C,H: Before fix - broken URL construction
    H->>C: ws://host/terminal/id with auth param already set
    C->>C: string concat appends second question mark to URL
    C->>S: WS upgrade with malformed double-question-mark URL
    S->>S: auth param value gets corrupted - validation fails
    S->>S: themeType query returns null
    S-->>C: 401 Unauthorized - terminal unreachable

    Note over C,H: After fix - correct URL construction
    H->>C: ws://host/terminal/id with auth param already set
    C->>C: new URL(base).searchParams.set adds themeType properly
    C->>S: WS upgrade with both params as separate query entries
    S->>S: auth param parsed correctly
    S->>S: themeType parsed correctly
    S-->>C: 101 Switching Protocols - terminal connected

_{Reviews (1): Last reviewed commit: "fix(desktop): use searchParams not strin..." | Re-trigger Greptile}

github-actions · 2026-05-07T02:13:58Z

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

✅ Neon database branch

Thank you for your contribution! 🎉

saddlepaddle merged commit a5a3143 into main May 7, 2026
9 of 10 checks passed

MocA-Love mentioned this pull request May 8, 2026

upstream merge 2026-05-08 PR 13: host-service shell env probe + typo MocA-Love/superset#460

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(desktop): use searchParams not string concat for terminal WS URL#4159

fix(desktop): use searchParams not string concat for terminal WS URL#4159
saddlepaddle merged 1 commit into
mainfrom
oval-sock

saddlepaddle commented May 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 7, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Estimated Code Review Effort

Poem

Uh oh!

greptile-apps Bot commented May 7, 2026

Important Files Changed

Uh oh!

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

saddlepaddle commented May 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Test plan

Summary by cubic

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated Code Review Effort

Poem

Uh oh!

greptile-apps Bot commented May 7, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

github-actions Bot commented May 7, 2026

🧹 Preview Cleanup Complete

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

saddlepaddle commented May 7, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 7, 2026 •

edited

Loading