Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
489 changes: 489 additions & 0 deletions plans/sandboxed-e2e-test-runtime.md

Large diffs are not rendered by default.

10 changes: 9 additions & 1 deletion src/atoms/testRuntimeAtoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export type TestRunPhase =
| "setup" // first-run Playwright bootstrap streaming
| "running" // playwright test executing
| "stopping" // Stop pressed; killing the Playwright process tree
| "cleaning-up"; // tests gone; isolation teardown still restoring the app
| "cleaning-up"; // tests gone; isolated provider data is still being removed
Comment thread
azizmejri1 marked this conversation as resolved.

export interface TestRunState {
phase: TestRunPhase;
Expand Down Expand Up @@ -72,6 +72,12 @@ export interface TestRunState {
* a run completes.
*/
isolation?: TestIsolation;
/**
* Whether the run executed in an isolated sandbox. Drives the cleanup copy:
* the fallback path (Docker/cloud runtime, or the user's opt-out) creates no
* workspace, so there is no sandbox to claim Dyad is deleting.
*/
sandboxed?: boolean;
startedAt?: number;
}

Expand Down Expand Up @@ -275,6 +281,8 @@ export const applyTestRunStartedAtom = atom(
),
runError: undefined,
isolation: undefined,
// Reported by the main process once the run has picked its path.
sandboxed: undefined,
startedAt: startedAt ?? Date.now(),
}),
});
Expand Down
29 changes: 29 additions & 0 deletions src/components/SandboxedE2eTestsSwitch.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { useSettings } from "@/hooks/useSettings";
import { Label } from "@/components/ui/label";
import { Switch } from "@/components/ui/switch";

/**
* Escape hatch for the sandboxed E2E runtime. Stored inverted
* (`disableSandboxedE2eTests`) so the sandbox stays the default for everyone
* who never opens this, and only an explicit opt-out falls back to running
* against the normal preview.
*/
export function SandboxedE2eTestsSwitch() {
const { settings, updateSettings } = useSettings();
const enabled = !settings?.disableSandboxedE2eTests;

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.

P2: While the settings query is loading, settings is null, so enabled evaluates to !undefined and the switch renders ON even for users who have explicitly set disableSandboxedE2eTests: true. This briefly promises sandboxing that the user turned off (the same inverted-read footgun TestsPanel.tsx already guards against), and a toggle during that window writes a value derived from the not-yet-loaded state. Disable the switch until settings have loaded.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/SandboxedE2eTestsSwitch.tsx, line 13:

<comment>While the settings query is loading, `settings` is null, so `enabled` evaluates to `!undefined` and the switch renders ON even for users who have explicitly set `disableSandboxedE2eTests: true`. This briefly promises sandboxing that the user turned off (the same inverted-read footgun `TestsPanel.tsx` already guards against), and a toggle during that window writes a value derived from the not-yet-loaded state. Disable the switch until settings have loaded.</comment>

<file context>
@@ -0,0 +1,29 @@
+ */
+export function SandboxedE2eTestsSwitch() {
+  const { settings, updateSettings } = useSettings();
+  const enabled = !settings?.disableSandboxedE2eTests;
+  return (
+    <div className="flex items-center space-x-2">
</file context>

return (
<div className="flex items-center space-x-2">
<Switch
id="enable-sandboxed-e2e-tests"
aria-label="Run E2E Tests in an Isolated Sandbox"
checked={enabled}
onCheckedChange={(checked) => {
updateSettings({ disableSandboxedE2eTests: !checked });
}}
/>
<Label htmlFor="enable-sandboxed-e2e-tests">
Run E2E Tests in an Isolated Sandbox
</Label>
</div>
);
}
19 changes: 12 additions & 7 deletions src/components/chat/CancellationBanner.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ vi.mock("react-i18next", () => ({
({
stoppingGeneration: "Stopping…",
cancellationEndingTestRun: "Ending the test run.",
cancellationRestoringTestApp:
"Restoring your app's database and preview. This can take a while.",
cancellationRemovingTestDatabase:
"Removing the temporary test database. This can take a while.",
cancellationCleaningTestData:
"Cleaning up the test data from this run.",
"Cleaning up this run's test sandbox and data.",
})[key] ?? key,
}),
}));
Expand Down Expand Up @@ -87,21 +87,26 @@ describe("CancellationBanner", () => {
});

expect(
screen.getByText(/Restoring your app's database and preview/),
screen.getByText(/Removing the temporary test database/),
).toBeTruthy();
expect(screen.getByText(/can take a while/)).toBeTruthy();
// The run had its own sandbox and its own server; the user's preview and
// `.env.local` were never touched, so nothing is being restored.
expect(screen.queryByText(/Restoring/i)).toBeNull();
});

it("does not claim a restore on the Supabase path", () => {
// That teardown only deletes the temporary test user — no env swap, no
// dev-server restart, nothing the user sees.
// That teardown only deletes the temporary test user and the run's sandbox
// copy — no env swap, no dev-server restart, nothing the user sees.
renderBanner({
phase: "cleaning-up",
isolationMode: "supabase-test-user",
source: "agent",
});

expect(screen.getByText(/Cleaning up the test data/)).toBeTruthy();
expect(
screen.getByText(/Cleaning up this run's test sandbox/),
).toBeTruthy();
expect(screen.queryByText(/Restoring/)).toBeNull();
});

Expand Down
16 changes: 9 additions & 7 deletions src/components/chat/CancellationBanner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ import {
* Pinned above the composer while a stopped turn settles.
*
* Stopping is not instant. The agent awaits its in-flight tool, and a
* `run_tests` call first kills the Playwright process tree and then runs an
* isolation teardown that accepts no AbortSignal — restoring `.env.local`,
* restarting the dev server and deleting the temporary Neon branch, whose
* delete retries with backoff. That wait can pass a minute, and the composer
* stays locked for all of it.
* `run_tests` call first kills the Playwright process tree and then runs a
* teardown that accepts no AbortSignal — deleting the temporary Neon branch,
* whose delete retries with backoff, and removing the run's sandbox copy of the
* app. That wait can pass a minute, and the composer stays locked for all of
* it. The user's own `.env.local` and preview are never touched.
*
* The transcript's inline status card scrolls out of view; this stays fused to
* the composer the user just clicked Stop in, so the wait is never unexplained.
Expand All @@ -38,8 +38,10 @@ export function CancellationBanner({ appId }: { appId?: number | null }) {
? null
: runState.phase === "cleaning-up"
? runState.isolation?.mode === "neon-branch"
? t("cancellationRestoringTestApp")
: t("cancellationCleaningTestData")
? t("cancellationRemovingTestDatabase")
: runState.sandboxed
? t("cancellationCleaningTestSandbox")
: t("cancellationCleaningTestData")
: runState.phase === "stopping"
? t("cancellationEndingTestRun")
: null;
Expand Down
111 changes: 95 additions & 16 deletions src/components/preview_panel/TestsPanel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ const mocks = vi.hoisted(() => ({
appUrl: "http://localhost:32100" as string | null,
previewUrl: "http://localhost:32100/" as string | null,
previewUrlSource: "dyad" as "none" | "dyad" | "app",
app: { id: 1, testingEnabled: true } as Record<string, unknown>,
settings: {} as Record<string, unknown>,
}));

vi.mock("@/ipc/types", () => ({
Expand All @@ -52,11 +54,11 @@ vi.mock("@/lib/toast", () => ({
}));

vi.mock("@/hooks/useLoadApp", () => ({
useLoadApp: () => ({ app: { id: 1, testingEnabled: true } }),
useLoadApp: () => ({ app: mocks.app }),
}));

vi.mock("@/hooks/useSettings", () => ({
useSettings: () => ({ settings: {}, updateSettings: vi.fn() }),
useSettings: () => ({ settings: mocks.settings, updateSettings: vi.fn() }),
}));

vi.mock("@/hooks/useRunApp", () => ({
Expand Down Expand Up @@ -131,6 +133,8 @@ describe("TestsPanel", () => {
mocks.appUrl = "http://localhost:32100";
mocks.previewUrl = "http://localhost:32100/";
mocks.previewUrlSource = "dyad";
mocks.app = { id: 1, testingEnabled: true };
mocks.settings = {};
mocks.listAppTests.mockResolvedValue({
specs: [
{
Expand Down Expand Up @@ -314,6 +318,43 @@ describe("TestsPanel", () => {
).toBe(true);
});

it("runs sandboxed tests without the preview being up", async () => {
// A sandboxed run serves its own copy of the app on its own port. Requiring
// the user's preview would block the whole point of the feature — and would
// contradict the panel's own "your preview keeps running" disclosure.
mocks.appUrl = null;

renderPanel();

await screen.findByText("signup.spec.ts");
expect(screen.queryByText("Start the app to run tests.")).toBeNull();
expect(
(
screen.getByRole("button", {
name: "Run all tests",
}) as HTMLButtonElement
).disabled,
).toBe(false);
});

it("still requires the preview when the run is not sandboxed", async () => {
// The fallback path runs Playwright against the user's preview, so the
// gate is still correct there.
mocks.appUrl = null;
mocks.settings = { disableSandboxedE2eTests: true };

renderPanel();

expect(await screen.findByText("Start the app to run tests.")).toBeTruthy();
expect(
(
screen.getByRole("button", {
name: "Run all tests",
}) as HTMLButtonElement
).disabled,
).toBe(true);
});

describe("stopping a run", () => {
/** Put the panel's app into `phase` as if a run had reached it. */
function setPhase(
Expand Down Expand Up @@ -391,42 +432,80 @@ describe("TestsPanel", () => {
expect(mocks.showError).toHaveBeenCalledWith(error);
});

it("names the Neon teardown, which restarts the preview", () => {
it("tells Neon users their preview keeps its real database", async () => {

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.

P3: These three disclosure tests — "tells Neon users their preview keeps its real database", "drops the sandbox disclosure when the sandbox is turned off", and "promises no sandbox while settings are still loading" — live inside describe("stopping a run"), but none of them touches stopping, the stop button, or the cleaning-up phase. They verify run-gate/disclosure rendering and belong at the top level alongside "runs sandboxed tests without the preview being up" and "still requires the preview...", not buried in the stopping block.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/preview_panel/TestsPanel.test.tsx, line 454:

<comment>These three disclosure tests — "tells Neon users their preview keeps its real database", "drops the sandbox disclosure when the sandbox is turned off", and "promises no sandbox while settings are still loading" — live inside `describe("stopping a run")`, but none of them touches stopping, the stop button, or the `cleaning-up` phase. They verify run-gate/disclosure rendering and belong at the top level alongside "runs sandboxed tests without the preview being up" and "still requires the preview...", not buried in the stopping block.</comment>

<file context>
@@ -391,42 +451,92 @@ describe("TestsPanel", () => {
     });
 
-    it("names the Neon teardown, which restarts the preview", () => {
+    it("tells Neon users their preview keeps its real database", async () => {
+      // The pre-sandbox copy promised a double preview restart. Nothing
+      // restarts any more, so that disclosure would now be a lie.
</file context>

// The pre-sandbox copy promised a double preview restart. Nothing
// restarts any more, so that disclosure would now be a lie.
mocks.app = { id: 1, testingEnabled: true, neonProjectId: "neon-proj" };
renderPanel();

expect(
await screen.findByText(/Your preview keeps running against your real/),
).toBeTruthy();
expect(screen.queryByText(/restart the preview/i)).toBeNull();
});

it("drops the sandbox disclosure when the sandbox is turned off", async () => {
mocks.app = { id: 1, testingEnabled: true, neonProjectId: "neon-proj" };
mocks.settings = { disableSandboxedE2eTests: true };
renderPanel();

await screen.findByText("signup.spec.ts");
expect(screen.queryByText(/Your preview keeps running/)).toBeNull();
});

it("names the Neon teardown without promising a restore", () => {
// This is the wait that can pass a minute (the branch delete retries with
// backoff), and it visibly reloads the user's preview. Calling it
// "Running…" — as the panel used to — reads as a hang.
// backoff). Calling it "Running…" — as the panel used to — reads as a
// hang, but the run had its own sandbox and its own server, so nothing
// of the user's is being put back either.
const { store } = renderPanel();
setPhase(store, {
phase: "cleaning-up",
isolation: { mode: "neon-branch" },
});

expect(
screen.getByText(/Restoring your database and preview/),
screen.getByText(/Removing the temporary test database/),
).toBeTruthy();
expect(screen.queryByText(/Restoring/i)).toBeNull();
expect(
screen.getByRole("button", { name: "Restoring your app" }).textContent,
).toContain("Restoring…");
screen.getByRole("button", {
name: "Removing the temporary test database",
}).textContent,
).toContain("Cleaning up…");
});

it("does not promise a database restore on the Supabase path", () => {
// That teardown only deletes the temporary test user. It never swaps
// `.env.local` and never restarts the app, so the Neon copy would lie.
it("names the sandbox cleanup on the Supabase path", () => {
// That teardown only deletes the temporary test user and the run's
// sandbox copy. It never swaps `.env.local` and never restarts the app.
const { store } = renderPanel();
setPhase(store, {
phase: "cleaning-up",
isolation: { mode: "supabase-test-user" },
sandboxed: true,
});

expect(screen.getByText(/Cleaning up the test data/)).toBeTruthy();
expect(screen.queryByText(/Restoring your database/)).toBeNull();
expect(screen.getByText(/Cleaning up the test sandbox/)).toBeTruthy();
expect(screen.queryByText(/Restoring/i)).toBeNull();
expect(
screen.getByRole("button", { name: "Cleaning up test data" })
.textContent,
).toContain("Cleaning up…");
expect(
screen.queryByRole("button", { name: "Restoring your app" }),
).toBeNull();
});

it("claims no sandbox when the run never took one", () => {
// The fallback path (docker/cloud runtime, or the opt-out) creates no
// workspace, so naming one would be the same inaccurate cleanup copy the
// Neon "restoring your preview" wording was.
const { store } = renderPanel();
setPhase(store, {
phase: "cleaning-up",
isolation: { mode: "supabase-test-user" },
sandboxed: false,
});

expect(screen.getByText(/Cleaning up the test data/)).toBeTruthy();
expect(screen.queryByText(/test sandbox/)).toBeNull();
});

it("does not carry a completed run's stop latch into the next run", () => {
Expand Down
Loading
Loading