feat(sandbox): add provider dashboard links#654
Open
hreiten wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds a Modal sandbox dashboard URL builder, persists and broadcasts dashboard URLs from the lifecycle manager during spawn/restore/resume, extends shared types and session state, updates WebSocket handling to set/clear the URL, and renders it as an external link in the SandboxStatus UI. ChangesSandbox Dashboard URL Display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/hooks/use-session-socket.ts (1)
391-409:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear stale
sandboxDashboardUrlon all spawn/failure transitions, not onlysandbox_spawning+ terminal status messages.
SandboxLifecycleManagerbroadcastssandbox_status: "spawning"on new spawn/restore and may emit onlysandbox_erroron failures. In those paths, this reducer can keep an old dashboard URL visible.Suggested fix
case "sandbox_status": { - const isTerminal = - data.status === "stale" || data.status === "stopped" || data.status === "failed"; + const shouldClearAccessState = + data.status === "spawning" || + data.status === "stale" || + data.status === "stopped" || + data.status === "failed"; setSessionState((prev) => prev ? { ...prev, sandboxStatus: data.status, - ...(isTerminal && { + ...(shouldClearAccessState && { codeServerUrl: undefined, codeServerPassword: undefined, tunnelUrls: undefined, ttydUrl: undefined, ttydToken: undefined, sandboxDashboardUrl: undefined, }), } : null ); break; } @@ case "sandbox_error": console.error("Sandbox error:", data.error); - setSessionState((prev) => (prev ? { ...prev, sandboxStatus: "failed" } : null)); + setSessionState((prev) => + prev + ? { + ...prev, + sandboxStatus: "failed", + codeServerUrl: undefined, + codeServerPassword: undefined, + tunnelUrls: undefined, + ttydUrl: undefined, + ttydToken: undefined, + sandboxDashboardUrl: undefined, + } + : null + ); break;Also applies to: 478-481
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/hooks/use-session-socket.ts` around lines 391 - 409, Update the reducer logic in the socket handler so the old sandboxDashboardUrl is cleared on spawn and failure transitions: in the "sandbox_status" case (the setSessionState updater) include sandboxDashboardUrl: undefined whenever data.status === "spawning" or when the existing isTerminal branch runs (status in "stale","stopped","failed"); likewise, in the "sandbox_error" handler (the other reducer block around lines ~478-481) add sandboxDashboardUrl: undefined to the fields that are cleared so any dashboard URL is removed on error-only failure paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/web/src/hooks/use-session-socket.ts`:
- Around line 391-409: Update the reducer logic in the socket handler so the old
sandboxDashboardUrl is cleared on spawn and failure transitions: in the
"sandbox_status" case (the setSessionState updater) include sandboxDashboardUrl:
undefined whenever data.status === "spawning" or when the existing isTerminal
branch runs (status in "stale","stopped","failed"); likewise, in the
"sandbox_error" handler (the other reducer block around lines ~478-481) add
sandboxDashboardUrl: undefined to the fields that are cleared so any dashboard
URL is removed on error-only failure paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1513d7c-cbe4-4432-bab0-c2e6f1772014
📒 Files selected for processing (9)
packages/control-plane/src/sandbox/client.test.tspackages/control-plane/src/sandbox/client.tspackages/control-plane/src/sandbox/lifecycle/manager.test.tspackages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/session/durable-object.tspackages/shared/src/types/index.tspackages/web/src/app/(app)/session/[id]/page.tsxpackages/web/src/hooks/use-session-socket.test.tsxpackages/web/src/hooks/use-session-socket.ts
hreiten
force-pushed
the
feat/sandbox-dashboard-url
branch
from
8a4a6ec to
883369c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/hooks/use-session-socket.test.tsx (1)
486-491: ⚡ Quick winConsider waiting for URLs to be set before asserting they're cleared.
This test combines socket open, subscription, access messages, and spawning status in a single
act()call without verifying that the URLs were actually set before being cleared. While the test would fail if clearing logic is broken, it's less clear and robust than the previous test (lines 443-454), which explicitly waits for the URLs to be set before clearing them.♻️ Suggested test structure for improved clarity
act(() => { socket.open(); socket.receive(createSubscribedMessage()); sendSandboxAccessMessages(socket, "old-sandbox"); +}); + +await waitFor(() => { + expect(result.current.sessionState?.sandboxDashboardUrl).toBe( + "https://provider.example/old-sandbox" + ); + expect(result.current.sessionState?.codeServerUrl).toBe("https://code.example/old-sandbox"); +}); + +act(() => { socket.receive({ type: "sandbox_status", status: "spawning" }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/hooks/use-session-socket.test.tsx` around lines 486 - 491, Split the combined act() into two steps so the test first opens the socket and sends subscription/access messages and then waits for the URLs to be set before triggering the spawning status; specifically call socket.open(), socket.receive(createSubscribedMessage()), and sendSandboxAccessMessages(socket, "old-sandbox") inside the first act(), then use an async wait (e.g. waitFor or findBy* against whatever component state/selectors expose the sandbox URLs) to assert the URLs were set, and only after that call socket.receive({ type: "sandbox_status", status: "spawning" }) in a second act() and assert the URLs were cleared. Ensure the test references the same helpers (socket.open, createSubscribedMessage, sendSandboxAccessMessages) so the sequence remains identical but with an explicit wait between setting and clearing assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/web/src/hooks/use-session-socket.test.tsx`:
- Around line 486-491: Split the combined act() into two steps so the test first
opens the socket and sends subscription/access messages and then waits for the
URLs to be set before triggering the spawning status; specifically call
socket.open(), socket.receive(createSubscribedMessage()), and
sendSandboxAccessMessages(socket, "old-sandbox") inside the first act(), then
use an async wait (e.g. waitFor or findBy* against whatever component
state/selectors expose the sandbox URLs) to assert the URLs were set, and only
after that call socket.receive({ type: "sandbox_status", status: "spawning" })
in a second act() and assert the URLs were cleared. Ensure the test references
the same helpers (socket.open, createSubscribedMessage,
sendSandboxAccessMessages) so the sequence remains identical but with an
explicit wait between setting and clearing assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b728a824-4af8-4d05-9efb-8f2ae9f4628e
📒 Files selected for processing (9)
packages/control-plane/src/sandbox/client.test.tspackages/control-plane/src/sandbox/client.tspackages/control-plane/src/sandbox/lifecycle/manager.test.tspackages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/session/durable-object.tspackages/shared/src/types/index.tspackages/web/src/app/(app)/session/[id]/page.tsxpackages/web/src/hooks/use-session-socket.test.tsxpackages/web/src/hooks/use-session-socket.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/control-plane/src/sandbox/client.test.ts
- packages/shared/src/types/index.ts
- packages/control-plane/src/sandbox/client.ts
- packages/web/src/app/(app)/session/[id]/page.tsx
- packages/web/src/hooks/use-session-socket.ts
- packages/control-plane/src/sandbox/lifecycle/manager.test.ts
- packages/control-plane/src/session/durable-object.ts
- packages/control-plane/src/sandbox/lifecycle/manager.ts
hreiten
force-pushed
the
feat/sandbox-dashboard-url
branch
from
883369c to
1ce5a29
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sandbox_dashboard_urlserver message and session state fieldWhy
Modal exposes useful per-sandbox diagnostics in its dashboard, but the web UI currently has no way to link users from a session to the corresponding provider sandbox. This makes debugging stuck, slow, or failed sandboxes harder than it needs to be.
The implementation keeps the shared protocol provider-neutral and confines Modal URL construction to the control-plane Modal path. Existing connected clients receive the URL as soon as the provider object ID is known, while newly subscribed clients receive it through the hydrated session state.
Prior upstream work checked
upstream/mainbefore opening this PR; this branch is one commit ahead and zero commits behindupstream/mainforsandboxDashboardUrl,sandbox_dashboard_url, andmodal_sandbox_url; no existing implementation was presentmainand does not add new Terraform/env wiringDetails
buildModalSandboxDashboardUrlreturnsnullunless the required Modal workspace and provider object ID are present.SandboxLifecycleManagercentralizes provider object ID persistence and broadcasts the dashboard URL only when configured.SessionDOwires the builder only for the Modal backend.Tests
npm run build -w @open-inspect/sharednpm test -w @open-inspect/control-planenpm test -w @open-inspect/webnpm run typecheck -w @open-inspect/control-planenpm run typecheck -w @open-inspect/webnpm run lintnpm run typechecknpm testgit diff --checkSummary by CodeRabbit
New Features
Tests