diff --git a/apps/web/e2e/sprint-47-trace-run-stability.spec.ts b/apps/web/e2e/sprint-47-trace-run-stability.spec.ts new file mode 100644 index 0000000..7e1f05b --- /dev/null +++ b/apps/web/e2e/sprint-47-trace-run-stability.spec.ts @@ -0,0 +1,180 @@ +import { expect, test } from "@playwright/test"; + +test.describe("Sprint 47 — Trace Run Stability", () => { + test.setTimeout(60_000); + + async function loadTraceExample( + page: import("@playwright/test").Page, + exampleId: string, + ) { + await page.goto("/#/load"); + const card = page.getByTestId(`example-card-${exampleId}`); + await expect(card).toBeVisible({ timeout: 15000 }); + await card.click(); + await expect(page.getByTestId("debugger-page")).toBeVisible({ + timeout: 15000, + }); + } + + async function openSettings(page: import("@playwright/test").Page) { + await page.getByTestId("drawer-tab-settings").click(); + await expect(page.getByTestId("settings-tab")).toBeVisible(); + } + + async function setAutoContinuePolicy( + page: import("@playwright/test").Page, + policy: "always_continue" | "continue_when_trace_matches" | "never", + ) { + await openSettings(page); + await page.getByTestId(`auto-continue-radio-${policy}`).click(); + await expect( + page.getByTestId(`auto-continue-radio-${policy}`), + ).toBeChecked(); + } + + test("trace-001: run with never auto-continue does not crash", async ({ + page, + }) => { + // Collect console errors and page crashes + const errors: string[] = []; + page.on("pageerror", (err) => errors.push(err.message)); + + await loadTraceExample(page, "trace-001"); + + // Set instruction stepping + never auto-continue + await openSettings(page); + await page.getByTestId("stepping-radio-instruction").click(); + await expect(page.getByTestId("stepping-radio-instruction")).toBeChecked(); + await page.getByTestId("auto-continue-radio-never").click(); + await expect(page.getByTestId("auto-continue-radio-never")).toBeChecked(); + + // Click Run — should stop at the first host call + await page.getByTestId("run-button").click(); + + // Wait for the PVM to reach host call state + await expect(page.getByTestId("pvm-status-typeberry")).toHaveText( + "Host Call", + { timeout: 15000 }, + ); + + // App must not have crashed — debugger page still visible, no error boundary + await expect(page.getByTestId("debugger-page")).toBeVisible(); + await expect(page.getByTestId("error-boundary")).not.toBeVisible(); + + // No unhandled JS errors + expect(errors).toEqual([]); + }); + + test("io-trace: run with always_continue completes without crash", async ({ + page, + }) => { + const errors: string[] = []; + page.on("pageerror", (err) => errors.push(err.message)); + + await loadTraceExample(page, "io-trace"); + await setAutoContinuePolicy(page, "always_continue"); + + // Click Run — should auto-continue through all host calls + await page.getByTestId("run-button").click(); + + // Wait for execution to finish or at least run for a while + const completeBadge = page.getByTestId("execution-complete-badge"); + const pauseBtn = page.getByTestId("pause-button"); + + // Either execution completes or we wait and pause + try { + await completeBadge.waitFor({ state: "visible", timeout: 15000 }); + } catch { + // If not complete yet, pause and verify app is still alive + if (await pauseBtn.isVisible()) { + await pauseBtn.click({ force: true }); + } + } + + // App must still be functional + await expect(page.getByTestId("debugger-page")).toBeVisible(); + await expect(page.getByTestId("error-boundary")).not.toBeVisible(); + await expect(page.getByTestId("pvm-status-typeberry")).toBeVisible(); + + expect(errors).toEqual([]); + }); + + test("trace-001: run with continue_when_trace_matches completes without crash", async ({ + page, + }) => { + const errors: string[] = []; + page.on("pageerror", (err) => errors.push(err.message)); + + await loadTraceExample(page, "trace-001"); + await setAutoContinuePolicy(page, "continue_when_trace_matches"); + + await page.getByTestId("run-button").click(); + + // With trace matching, it should auto-continue through matching host calls + const completeBadge = page.getByTestId("execution-complete-badge"); + const pauseBtn = page.getByTestId("pause-button"); + + try { + await completeBadge.waitFor({ state: "visible", timeout: 20000 }); + } catch { + if (await pauseBtn.isVisible()) { + await pauseBtn.click({ force: true }); + } + } + + // If there were JS errors, log them for debugging + if (errors.length > 0) { + console.log("Page errors captured:", errors); + } + + // App must still be functional + await expect(page.getByTestId("debugger-page")).toBeVisible({ + timeout: 10000, + }); + await expect(page.getByTestId("error-boundary")).not.toBeVisible(); + + expect(errors).toEqual([]); + }); + + test("pending changes only visible when paused at host call", async ({ + page, + }) => { + await loadTraceExample(page, "trace-001"); + await setAutoContinuePolicy(page, "never"); + + // Run to first host call + await page.getByTestId("run-button").click(); + await expect(page.getByTestId("pvm-status-typeberry")).toHaveText( + "Host Call", + { timeout: 15000 }, + ); + + // Pending changes should be visible (paused at host call with trace proposal) + // Wait for the 300ms debounce + await page.waitForTimeout(400); + const pending = page.getByTestId("pending-changes"); + // Pending may or may not exist depending on whether the proposal has data, + // but if it does exist it must only exist while paused at a host call + const isPendingVisible = await pending.isVisible().catch(() => false); + + if (isPendingVisible) { + // Verify it shows register or gas changes + const hasRegisters = await page + .getByTestId("pending-register-writes") + .isVisible() + .catch(() => false); + const hasGas = await page + .getByTestId("pending-gas-change") + .isVisible() + .catch(() => false); + expect(hasRegisters || hasGas).toBe(true); + } + + // Resume by stepping — pending changes should disappear + await page.getByTestId("next-button").click(); + + // After stepping, lifecycle is no longer paused_host_call + // so pending changes must not be visible + await expect(pending).not.toBeVisible({ timeout: 5000 }); + }); +}); diff --git a/apps/web/src/App.tsx b/apps/web/src/App.tsx index 00edc08..6f8c72f 100644 --- a/apps/web/src/App.tsx +++ b/apps/web/src/App.tsx @@ -3,6 +3,7 @@ import { AppsSidebar, Content, Header } from "@fluffylabs/shared-ui"; import { initManifest } from "@pvmdbg/content"; import { type ReactNode, useEffect, useRef, useState } from "react"; import { Navigate, Route, Routes, useNavigate } from "react-router"; +import { ErrorBoundary } from "./components/ErrorBoundary"; import { DebuggerSettingsProvider } from "./context/debugger-settings"; import { OrchestratorProvider } from "./context/orchestrator"; import { useOrchestrator } from "./hooks/useOrchestrator"; @@ -97,13 +98,15 @@ export default function App() {
- - - } /> - } /> - } /> - - + + + + } /> + } /> + } /> + + +
diff --git a/apps/web/src/components/ErrorBoundary.tsx b/apps/web/src/components/ErrorBoundary.tsx new file mode 100644 index 0000000..c26eaac --- /dev/null +++ b/apps/web/src/components/ErrorBoundary.tsx @@ -0,0 +1,55 @@ +import { Component, type ErrorInfo, type ReactNode } from "react"; + +interface Props { + children: ReactNode; +} + +interface State { + error: Error | null; +} + +/** + * Catches uncaught render errors and displays a recovery UI instead of + * crashing the entire app to a blank page. + */ +export class ErrorBoundary extends Component { + state: State = { error: null }; + + static getDerivedStateFromError(error: Error): State { + return { error }; + } + + componentDidCatch(error: Error, info: ErrorInfo) { + console.error("ErrorBoundary caught:", error, info.componentStack); + } + + render() { + if (this.state.error) { + return ( +
+

+ Something went wrong +

+ 
+            {this.state.error.message}
+
+ +
+ ); + } + return this.props.children; + } +} diff --git a/apps/web/src/components/debugger/BottomDrawer.tsx b/apps/web/src/components/debugger/BottomDrawer.tsx index ba8b5a6..1758167 100644 --- a/apps/web/src/components/debugger/BottomDrawer.tsx +++ b/apps/web/src/components/debugger/BottomDrawer.tsx @@ -40,6 +40,7 @@ interface BottomDrawerProps { storageTable: UseStorageTable; pendingChanges: UsePendingChanges; snapshotVersion: number; + isRunning: boolean; } export function BottomDrawer({ @@ -51,12 +52,14 @@ export function BottomDrawer({ storageTable, pendingChanges, snapshotVersion, + isRunning, }: BottomDrawerProps) { const { activeTab, height, setActiveTab, setHeight } = useDrawer(); const { activeHostCall } = useHostCallState( hostCallInfo, selectedPvmId, snapshots, + isRunning, ); const dragRef = useRef<{ startY: number; startH: number } | null>(null); diff --git a/apps/web/src/components/debugger/RegistersPanel.tsx b/apps/web/src/components/debugger/RegistersPanel.tsx index 2acc161..c14fcce 100644 --- a/apps/web/src/components/debugger/RegistersPanel.tsx +++ b/apps/web/src/components/debugger/RegistersPanel.tsx @@ -202,7 +202,7 @@ export function RegistersPanel({ /> ))} - {pendingChanges.pending && ( + {lifecycle === "paused_host_call" && pendingChanges.pending && ( )} diff --git a/apps/web/src/hooks/useHostCallState.test.tsx b/apps/web/src/hooks/useHostCallState.test.tsx new file mode 100644 index 0000000..8cd48e1 --- /dev/null +++ b/apps/web/src/hooks/useHostCallState.test.tsx @@ -0,0 +1,128 @@ +import type { + HostCallInfo, + MachineStateSnapshot, + PvmLifecycle, +} from "@pvmdbg/types"; +import { renderHook } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { describe, expect, it } from "vitest"; +import { DrawerProvider } from "../components/debugger/DrawerContext"; +import { useHostCallState } from "./useHostCallState"; + +function makeSnapshot( + overrides?: Partial, +): MachineStateSnapshot { + return { + pc: 0, + gas: 1_000_000n, + status: "ok", + registers: Array(13).fill(0n), + ...overrides, + }; +} + +function makeSnapshots( + entries: Array<[string, PvmLifecycle, Partial?]>, +): Map { + return new Map( + entries.map(([id, lifecycle, overrides]) => [ + id, + { snapshot: makeSnapshot(overrides), lifecycle }, + ]), + ); +} + +function makeHostCallInfo(pvmId: string): HostCallInfo { + return { + pvmId, + hostCallIndex: 1, + hostCallName: "fetch", + currentState: makeSnapshot({ status: "host" }), + }; +} + +function wrapper({ children }: { children: ReactNode }) { + return {children}; +} + +describe("useHostCallState", () => { + it("returns active host call when paused at host call", () => { + const info = makeHostCallInfo("typeberry"); + const hostCallInfo = new Map([["typeberry", info]]); + const snapshots = makeSnapshots([["typeberry", "paused_host_call"]]); + + const { result } = renderHook( + () => useHostCallState(hostCallInfo, "typeberry", snapshots, false), + { wrapper }, + ); + + expect(result.current.activeHostCall).toBe(info); + }); + + it("returns null when lifecycle is not paused_host_call", () => { + const info = makeHostCallInfo("typeberry"); + const hostCallInfo = new Map([["typeberry", info]]); + const snapshots = makeSnapshots([["typeberry", "paused"]]); + + const { result } = renderHook( + () => useHostCallState(hostCallInfo, "typeberry", snapshots, false), + { wrapper }, + ); + + expect(result.current.activeHostCall).toBeNull(); + }); + + it("returns null when isRunning is true even if paused at host call", () => { + const info = makeHostCallInfo("typeberry"); + const hostCallInfo = new Map([["typeberry", info]]); + const snapshots = makeSnapshots([["typeberry", "paused_host_call"]]); + + const { result } = renderHook( + () => useHostCallState(hostCallInfo, "typeberry", snapshots, true), + { wrapper }, + ); + + expect(result.current.activeHostCall).toBeNull(); + }); + + it("returns null when no selectedPvmId", () => { + const hostCallInfo = new Map(); + const snapshots = makeSnapshots([["typeberry", "paused_host_call"]]); + + const { result } = renderHook( + () => useHostCallState(hostCallInfo, null, snapshots, false), + { wrapper }, + ); + + expect(result.current.activeHostCall).toBeNull(); + }); + + it("returns null when hostCallInfo has no entry for the selected PVM", () => { + const hostCallInfo = new Map(); + const snapshots = makeSnapshots([["typeberry", "paused_host_call"]]); + + const { result } = renderHook( + () => useHostCallState(hostCallInfo, "typeberry", snapshots, false), + { wrapper }, + ); + + expect(result.current.activeHostCall).toBeNull(); + }); + + it("transitions from active to null when isRunning becomes true", () => { + const info = makeHostCallInfo("typeberry"); + const hostCallInfo = new Map([["typeberry", info]]); + const snapshots = makeSnapshots([["typeberry", "paused_host_call"]]); + + const { result, rerender } = renderHook( + ({ isRunning }) => + useHostCallState(hostCallInfo, "typeberry", snapshots, isRunning), + { wrapper, initialProps: { isRunning: false } }, + ); + + expect(result.current.activeHostCall).toBe(info); + + rerender({ isRunning: true }); + expect(result.current.activeHostCall).toBeNull(); + }); +}); diff --git a/apps/web/src/hooks/useHostCallState.ts b/apps/web/src/hooks/useHostCallState.ts index d033afb..3726a40 100644 --- a/apps/web/src/hooks/useHostCallState.ts +++ b/apps/web/src/hooks/useHostCallState.ts @@ -23,15 +23,22 @@ export function useHostCallState( string, { snapshot: MachineStateSnapshot; lifecycle: PvmLifecycle } >, + isRunning: boolean, ): HostCallState { const { openToTab } = useDrawer(); + // Suppress host call state while running. During auto-continue the rAF + // can fire between host-call detection and resume (Worker postMessage is + // a macrotask). Exposing the transient paused_host_call to React causes + // the HostCallTab to mount and trigger cascading state updates that + // exceed React's max update depth. const activeHostCall = useMemo(() => { + if (isRunning) return null; if (!selectedPvmId) return null; const entry = snapshots.get(selectedPvmId); if (entry?.lifecycle !== "paused_host_call") return null; return hostCallInfo.get(selectedPvmId) ?? null; - }, [hostCallInfo, selectedPvmId, snapshots]); + }, [hostCallInfo, selectedPvmId, snapshots, isRunning]); // Auto-open drawer when transitioning from no host call to active host call. const prevRef = useRef(null); diff --git a/apps/web/src/hooks/useOrchestratorState.ts b/apps/web/src/hooks/useOrchestratorState.ts index 83335c5..4be2c82 100644 --- a/apps/web/src/hooks/useOrchestratorState.ts +++ b/apps/web/src/hooks/useOrchestratorState.ts @@ -136,6 +136,19 @@ export function useOrchestratorState(): OrchestratorReactiveState { pendingStepDone.current = true; versionRef.current += 1; + // Clear host call info when the PVM is no longer paused at a host call + // (e.g. after auto-continue resumes). Without this, stale entries + // persist and cause "Pending changes" to show during continuous execution. + if (lifecycle !== "paused_host_call") { + const hcBase = pendingHostCallInfo.current ?? new Map(); + if (hcBase.has(pvmId)) { + hcBase.delete(pvmId); + } + // Always set pendingHostCallInfo so the flush clears any stale + // React state even when no new hostCallPaused event fired. + pendingHostCallInfo.current = hcBase; + } + if (lifecycle === "paused") { const errBase = pendingPerPvmErrors.current ?? new Map(); if (errBase.has(pvmId)) { diff --git a/apps/web/src/pages/DebuggerPage.tsx b/apps/web/src/pages/DebuggerPage.tsx index 1a254dc..c02da88 100644 --- a/apps/web/src/pages/DebuggerPage.tsx +++ b/apps/web/src/pages/DebuggerPage.tsx @@ -279,6 +279,7 @@ function DebuggerPageInner() { storageTable={storageTable} pendingChanges={pendingChanges} snapshotVersion={snapshotVersion} + isRunning={isRunning} /> } /> diff --git a/reference/pvm-debugger b/reference/pvm-debugger index ab9390d..710f0ca 160000 --- a/reference/pvm-debugger +++ b/reference/pvm-debugger @@ -1 +1 @@ -Subproject commit ab9390da861febbef81806837fe23bec793b9abe +Subproject commit 710f0ca21f64aac4fc513771de48fdaf15e315a3 diff --git a/reference/shared-ui b/reference/shared-ui index 1286a6b..e5cf075 160000 --- a/reference/shared-ui +++ b/reference/shared-ui @@ -1 +1 @@ -Subproject commit 1286a6ba524ec169180c664fa4c02be6f8e44512 +Subproject commit e5cf07574fbcbb5352740874d3ddeb861882880a diff --git a/spec/ui/sprint-47-pending-changes-fix-and-error-boundary.md b/spec/ui/sprint-47-pending-changes-fix-and-error-boundary.md new file mode 100644 index 0000000..127ff42 --- /dev/null +++ b/spec/ui/sprint-47-pending-changes-fix-and-error-boundary.md @@ -0,0 +1,67 @@ +# Sprint 47 — Pending Changes Fix and Error Boundary + +Status: Implemented + +## Goal + +Fix "Pending Host Call Changes" banner appearing during continuous execution (when PVM is running, not stopped at a host call). Fix crash (React error #185 — Maximum update depth exceeded) when running trace examples with auto-continue. Add an error boundary so uncaught render errors show a recovery UI instead of crashing the app to a blank page. + +## Prior Sprint Dependencies + +- Sprint 41: pending changes system, `usePendingChanges` hook +- Sprint 42: host call UX redesign, auto-apply flow + +## What Works After This Sprint + +### Stale Host Call Info Cleanup + +1. **`hostCallInfo` entries cleared after resume.** In `useOrchestratorState`, when `onStateChanged` fires with a lifecycle other than `"paused_host_call"`, the PVM's entry is deleted from `pendingHostCallInfo`. This ensures stale host call info does not persist in React state after auto-continue resumes a host call during the run loop. + +2. **Flush always propagates clearing.** When a PVM transitions away from `paused_host_call`, `pendingHostCallInfo.current` is always set (even to an empty Map) so the next rAF flush replaces any stale React state. Previously, if `pendingHostCallInfo.current` was null (already flushed) and the React state had a stale entry, it would never be cleared. + +### Pending Changes Display Guard + +3. **Lifecycle-gated rendering.** `RegistersPanel` now only renders the `` component when `lifecycle === "paused_host_call"` in addition to `pendingChanges.pending` being non-null. Previously, the component rendered whenever `pending` was non-null, which could happen when stale `hostCallInfo` entries leaked through during continuous execution. + +4. **No visible change during manual host-call pauses.** When the user is genuinely paused at a host call (manual stepping or auto-continue policy stops), the lifecycle is `"paused_host_call"` and the banner renders as before. The existing E2E tests and unit tests continue to pass unchanged. + +### Crash Fix — Suppress Host Call State While Running + +5. **`useHostCallState` returns null while running.** The `activeHostCall` useMemo returns null when `isRunning` is true. This prevents transient `paused_host_call` state from reaching the HostCallTab during auto-continue, which would trigger cascading setState calls and `orchestrator.setGas()` async operations that exceed React's max update depth. + +6. **Root cause.** The PVM adapter runs in a Web Worker. Worker `postMessage` is a macrotask, so `requestAnimationFrame` callbacks can fire between the orchestrator's host-call detection (`adapter.step()`) and resume (`adapter.setRegisters()/setMemory()/setGas()`). When the rAF fires in this window, React renders with `paused_host_call` lifecycle and `hostCallInfo` entry. The HostCallTab mounts, its render-time setState fires (×4), its useEffect applies effects which call `pendingChanges.setRegister/setGas` (more setState), and `orchestrator.setGas()` emits another `pvmStateChanged` event. With 43+ log host calls in trace-001, these cascading updates exceed React's 50-render limit. + +7. **`isRunning` threaded through BottomDrawer.** `DebuggerPage` passes `isRunning` to `BottomDrawer`, which passes it to `useHostCallState`. The hook suppresses all host-call-related state and drawer auto-opening while the PVM is running. + +### Error Boundary + +8. **`ErrorBoundary` component.** A new class component `ErrorBoundary` catches uncaught render errors via `getDerivedStateFromError` and `componentDidCatch`. It displays the error message in a styled container with a "Reload" button that navigates to `#/load` and reloads the page. + +9. **Wraps the app content.** The `ErrorBoundary` is placed in `App.tsx` around `` and ``, inside the `OrchestratorProvider` and layout shell. Any rendering crash in the debugger page, load page, or restore gate is caught and displayed instead of showing a blank page. + +## Bug Details + +The root cause of the spurious "Pending changes" banner was a two-part issue: + +- **Missing cleanup:** The `onHostCallPaused` event handler in `useOrchestratorState` added entries to `pendingHostCallInfo`, but no code path ever removed them when the host call was resumed. During the run loop, `resumeHostCall` emits `pvmStateChanged(paused)` but nothing cleared the host call info entry. The stale entry survived across rAF flushes. + +- **Missing guard:** `RegistersPanel` rendered `` whenever `pendingChanges.pending` was non-null. Since `usePendingChanges` derived `pending` from `hostCallInfo.get(selectedPvmId)`, and the stale entry was never removed, `pending` was always non-null after the first host call. + +## Files Changed + +| File | Changes | +|------|---------| +| `apps/web/src/hooks/useOrchestratorState.ts` | `onStateChanged` handler clears `pendingHostCallInfo` entry when lifecycle is not `paused_host_call`. Forces `pendingHostCallInfo.current` assignment so rAF flush propagates clearing | +| `apps/web/src/hooks/useHostCallState.ts` | `activeHostCall` returns null when `isRunning` is true. Accepts `isRunning` parameter | +| `apps/web/src/components/debugger/BottomDrawer.tsx` | Accepts and passes `isRunning` prop to `useHostCallState` | +| `apps/web/src/pages/DebuggerPage.tsx` | Passes `isRunning` to `BottomDrawer` | +| `apps/web/src/components/debugger/RegistersPanel.tsx` | `PendingChanges` rendering gated on `lifecycle === "paused_host_call"` | +| `apps/web/src/components/ErrorBoundary.tsx` | **New.** React error boundary with error display and reload button | +| `apps/web/src/App.tsx` | Wraps routes in `` | +| `apps/web/e2e/sprint-47-trace-run-stability.spec.ts` | **New.** E2E tests for trace run stability: crash reproduction, pending changes guard | + +## Implementation Notes + +- The `useHostCallState` hook (used by `BottomDrawer`) already checked `entry?.lifecycle !== "paused_host_call"` before returning host call info, so it was unaffected by the stale entries. The bug only manifested through `usePendingChanges`, which accessed `hostCallInfo` directly without a lifecycle check. +- The rAF buffering in `useOrchestratorState` means that during a run loop batch, multiple `onHostCallPaused` + `onStateChanged(paused)` pairs fire synchronously. By the time the rAF flushes, the last `onStateChanged(paused)` has already cleared the entry, so React never sees the intermediate `paused_host_call` state during continuous execution. +- The `ErrorBoundary` is a class component because React's error boundary API (`getDerivedStateFromError`, `componentDidCatch`) is only available as class component methods.