fix: clear stale host call info and add error boundary#5
Merged
Conversation
Host call info entries were never removed from React state after auto-continue resumed them, causing "Pending changes" to display during continuous execution. Fix by clearing entries in onStateChanged when lifecycle leaves paused_host_call, and guarding PendingChanges render on lifecycle === "paused_host_call". Add ErrorBoundary to catch uncaught render errors gracefully instead of crashing to a blank page. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes introduce an error boundary component to catch rendering failures in the app's routing layer and fix a state synchronization issue where pending host call changes were displayed incorrectly during continuous execution. Updates ensure pending changes only render when lifecycle is paused and stale state is cleared when execution resumes. Changes
Sequence DiagramssequenceDiagram
participant App as App
participant EB as ErrorBoundary
participant Routes as Routes/Components
App->>EB: Render with children
EB->>Routes: Render children
Routes-->>EB: Throws Error
EB->>EB: getDerivedStateFromError()<br/>componentDidCatch()
EB->>EB: Display error UI
Note over EB: Error message + Reload button
EB->>EB: onClick: navigate to `#/load`
EB->>EB: window.location.reload()
sequenceDiagram
participant Orch as Orchestrator
participant Hook as useOrchestratorState
participant State as Hook State
participant Panel as RegistersPanel
Orch->>Hook: pvmStateChanged event
Hook->>Hook: Check lifecycle value
alt lifecycle !== "paused_host_call"
Hook->>State: Remove hostCallInfo for pvmId
Hook->>State: Assign updated Map
end
Hook->>Panel: Update pendingHostCallInfo state
Panel->>Panel: Check lifecycle === "paused_host_call"<br/>AND pendingChanges.pending
alt Conditions met
Panel->>Panel: Render PendingChanges
else
Panel->>Panel: Hide PendingChanges
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/src/components/ErrorBoundary.tsx (2)
41-44: Remove redundant state reset before full reloadLine 42 is unnecessary because Line 44 triggers a full page reload immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ErrorBoundary.tsx` around lines 41 - 44, Remove the redundant this.setState({ error: null }) inside the ErrorBoundary onClick handler: since window.location.reload() (called after setting window.location.hash) performs a full page reload, delete the this.setState call in the onClick callback to avoid unnecessary state mutation; locate the onClick handler in ErrorBoundary.tsx (the anonymous function containing this.setState, window.location.hash = "#/load", window.location.reload) and remove only the this.setState({ error: null }) line.
39-47: Import and use shared-uiButtoncomponent instead of a local<button>elementFor consistency with the rest of the codebase, use the
Buttoncomponent from@fluffylabs/shared-ui, which is already widely imported throughout the app (e.g.,LoadPage.tsx,DebuggerPage.tsx,ExecutionControls.tsx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ErrorBoundary.tsx` around lines 39 - 47, Replace the local <button> inside the ErrorBoundary render with the shared Button component: add an import for Button from "@fluffylabs/shared-ui" at the top of the file, then swap the element used in the ErrorBoundary component (the block that calls this.setState({ error: null }); window.location.hash = "#/load"; window.location.reload();) to use <Button> and forward the same onClick handler, type prop (if the shared Button accepts it) and className/visual props so behavior and styling remain identical; ensure the Button receives the same children content and that the import name matches the component reference in the file.spec/ui/sprint-47-pending-changes-fix-and-error-boundary.md (1)
14-55: Add an explicit verification checklist sectionThe doc captures behavior and pitfalls well, but it should include concrete verification steps (including edge-case runs) as an explicit checklist for this sprint’s acceptance criteria.
As per coding guidelines
spec/**/*.md: Mark spec files as implemented by addingStatus: Implementedto the top and enhance acceptance criteria with edge cases, pitfalls, and verification steps discovered during implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/ui/sprint-47-pending-changes-fix-and-error-boundary.md` around lines 14 - 55, Add "Status: Implemented" at the top of the spec and append an explicit "Verification Checklist" section that enumerates concrete acceptance steps and edge-case checks; specifically, include tests to reproduce the stale hostCallInfo leak (continuous run loop with auto-continue), manual host-call pause/resume behavior, rAF flush propagation when pendingHostCallInfo.current is null vs set, RegistersPanel rendering gating on lifecycle === "paused_host_call", and the ErrorBoundary display/reload behavior. Ensure the checklist references the documented areas ("What Works After This Sprint", "Bug Details", and the new ErrorBoundary) and includes pass/fail criteria for each item so reviewers can mark the sprint as accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/hooks/useOrchestratorState.ts`:
- Around line 143-149: The flush is creating an empty Map (new Map()) which can
wipe other PVM entries; instead, initialize the temporary map from the latest
committed host-call map and then delete only the target pvmId. Concretely, when
building hcBase, use the committed map (e.g.,
pendingHostCallInfoCommitted.current) or fallback to pendingHostCallInfo.current
to clone existing entries, then call hcBase.delete(pvmId) and assign
pendingHostCallInfo.current = hcBase so only that pvmId is removed and unrelated
paused host-call entries are preserved.
---
Nitpick comments:
In `@apps/web/src/components/ErrorBoundary.tsx`:
- Around line 41-44: Remove the redundant this.setState({ error: null }) inside
the ErrorBoundary onClick handler: since window.location.reload() (called after
setting window.location.hash) performs a full page reload, delete the
this.setState call in the onClick callback to avoid unnecessary state mutation;
locate the onClick handler in ErrorBoundary.tsx (the anonymous function
containing this.setState, window.location.hash = "#/load",
window.location.reload) and remove only the this.setState({ error: null }) line.
- Around line 39-47: Replace the local <button> inside the ErrorBoundary render
with the shared Button component: add an import for Button from
"@fluffylabs/shared-ui" at the top of the file, then swap the element used in
the ErrorBoundary component (the block that calls this.setState({ error: null
}); window.location.hash = "#/load"; window.location.reload();) to use <Button>
and forward the same onClick handler, type prop (if the shared Button accepts
it) and className/visual props so behavior and styling remain identical; ensure
the Button receives the same children content and that the import name matches
the component reference in the file.
In `@spec/ui/sprint-47-pending-changes-fix-and-error-boundary.md`:
- Around line 14-55: Add "Status: Implemented" at the top of the spec and append
an explicit "Verification Checklist" section that enumerates concrete acceptance
steps and edge-case checks; specifically, include tests to reproduce the stale
hostCallInfo leak (continuous run loop with auto-continue), manual host-call
pause/resume behavior, rAF flush propagation when pendingHostCallInfo.current is
null vs set, RegistersPanel rendering gating on lifecycle ===
"paused_host_call", and the ErrorBoundary display/reload behavior. Ensure the
checklist references the documented areas ("What Works After This Sprint", "Bug
Details", and the new ErrorBoundary) and includes pass/fail criteria for each
item so reviewers can mark the sprint as accepted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cf3dcae-5447-44cc-b5a3-15e5a94c9a02
📒 Files selected for processing (5)
apps/web/src/App.tsxapps/web/src/components/ErrorBoundary.tsxapps/web/src/components/debugger/RegistersPanel.tsxapps/web/src/hooks/useOrchestratorState.tsspec/ui/sprint-47-pending-changes-fix-and-error-boundary.md
The reference/pvm-debugger submodule pointed to a commit that no longer exists on the remote, breaking Netlify builds. Update both submodules to current main branch heads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for pvm-debugger ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
The HostCallTab mounted during auto-continue when the rAF fired between host-call detection and resume (Worker postMessage is a macrotask). Its render-time setState and async orchestrator.setGas() calls created cascading updates that exceeded React's max update depth (error #185). Fix by returning null from useHostCallState when isRunning is true, preventing the transient paused_host_call state from reaching React. Add e2e tests that reproduce the crash with trace examples. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verifies that activeHostCall is suppressed when isRunning is true, preventing the max-update-depth crash during auto-continue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
hostCallInfoentries were never cleared after auto-continue resumed host calls. The stale entries causedusePendingChangesto always show the "Pending Host Call Changes" banner even while the PVM was running. Fixed by clearing entries inonStateChangedwhen lifecycle leavespaused_host_call, and gating<PendingChanges>render onlifecycle === "paused_host_call".ErrorBoundary: Uncaught render errors previously crashed the app to a blank page (no error boundary existed). Added one around the app routes with a recovery UI.Test plan
🤖 Generated with Claude Code