Repository navigation
fix(desktop): honor display and stream options on create() and stream.start() - #2145
PrabalParihar wants to merge 1 commit into
Conversation
….start()
JS SDK:
- Sandbox.create() read `display` from its second argument, so
Sandbox.create({ display: ':1' }) started Xvfb and set DISPLAY on :0.
Read it from the resolved sandbox options, which covers both call forms.
- VNCServer built its noVNC command in the constructor with the default
ports, so stream.start({ vncPort, port }) started x11vnc on the new
rfbport but noVNC still on 6080 -> localhost:5900, and the readiness
check on the new port timed out. Build the command in start(), after
the ports are set (the Python SDK already does this).
Python SDK:
- _VNCServer.start() keeps auth enabled across restarts but only made a
password when require_auth was passed again, so a second start() ran
`x11vnc -storepasswd None` with -usepw and get_auth_key() raised.
Generate the password whenever auth is enabled (as the JS SDK does).
|
We require contributors to sign our Contributor License Agreement, and we don't have @PrabalParihar on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
🦋 Changeset detectedLatest commit: 9208692 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
TASTE.md review: checked parity (T-1, T-1d, T-2), API shape and options (T-3, T-3a, T-10, T-20, T-22, T-23), defaults and config (T-47, T-50), shell composition (T-43), and errors (T-59, T-62, T-63) against the changed lines. No violations found: the fixes bring JS create()/stream.start() and Python _VNCServer.start() into line with each other without adding to the public surface.
There was a problem hiding this comment.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Three options the desktop SDKs already accept were being dropped: in JS, create({ display: ':1' }) still started on :0 and stream.start() ignored custom ports, then failed to start noVNC; in Python, restarting an authenticated stream saved the password as the literal None. I fixed those and lined each SDK up with the other where it already worked.
Three places where the desktop SDKs drop an option the caller passed. I found them while comparing the JS and Python
create()andstream.start()paths.Problem
JS:
Sandbox.create({ display: ':1' })starts the desktop on:0.create()accepts(opts)or(template, opts)and resolves them intosandboxOpts, but it read the display from the raw second argument:const display = opts?.display || ':0'. With the options-only form that argument isundefined. So Xvfb,xdpyinfo,DISPLAYandsandbox.displayall end up on:0, whileresolutionanddpifrom the same object are applied.Sandbox.create('desktop', { display: ':1' })works.JS:
stream.start({ vncPort, port })doesn't move noVNC.VNCServerbuiltnovncCommandonce in its constructor, whilevncPort/portstill held the defaults (5900/6080).start()updates the ports afterwards, so x11vnc gets-rfbport 5901and the stream URL points at 6081, but noVNC still runs--vnc localhost:5900 --listen 6080. The readiness check then waits on:6081, andstart()throwsCould not start noVNC server. The Python SDK builds this command insidestart(), so it isn't affected.Python: restarting the stream with auth enabled stores the password
None._VNCServer.start()keeps_novnc_auth_enabledon across restarts (require_auth or self._novnc_auth_enabled), but it only made a password whenrequire_auth=Truewas passed again. Sostream.start(require_auth=True), thenstream.stop(), thenstream.start()runsx11vnc -storepasswd None ~/.vnc/passwdwith-usepw. The stream's password becomes the literal stringNone, andget_auth_key()raises. The JS SDK generates the password whenever auth is enabled.Change
create(): readdisplayfrom the resolvedsandboxOpts.VNCServer: build the noVNC command instart(), after the ports are set, as the Python SDK does. The unusednovncCommandfield is removed._VNCServer.start(): generate the password whenever auth is enabled, as the JS SDK does.@e2b/desktopand@e2b/desktop-python.Usage is the same; these calls now do what they say:
Tests
packages/desktop-js/tests/readiness.test.ts: a newtest.eachcovering bothcreate()call forms. It mocksBaseSandbox.createthe same way the existing startup-failure test does, then checks thatDISPLAYis:1in the envs and that_startgets:1. The options-only case fails on main and passes here.packages/desktop-js/tests/stream.test.ts(new): it runs_start()with recordedcommands.runand a mockedwaitAndVerify, then callsstream.start({ vncPort: 5901, port: 6081 }). It checks the x11vnc-rfbport, the noVNC--vnc/--listenports, the port the readiness check waits on, and the stream URL. A second test checks that the defaults stay 5900/6080. The custom-ports test fails on main (noVNC getslocalhost:5900 --listen 6080) and passes here.packages/desktop-python/tests/test_stream.py(new): runs start with auth, then stop, then start again. It checks that a fresh password is stored and returned, with no-storepasswd None. A second test checks that a start without auth stays-nopw. The restart test fails on main (get_auth_key()raises) and passes here.vitest run tests/stream.test.ts tests/readiness.test.ts tests/client.test.ts: 18 passed.pnpm typecheck,pnpm lint(oxlint) andprettier --check src testsare clean.pytest tests/test_stream.py tests/test_readiness.py tests/test_client.py: 15 passed.ruff check,ruff format --checkandty check e2b_desktopare clean.apps,mouse,screen,test_controls) because they need anE2B_API_KEY.This change was made with AI assistance; the tests listed above were run locally before opening this PR.