Skip to content

fix(ui): harden board canvas against React #185 render-loop crash#1746

Open
kgabryje wants to merge 3 commits into
mainfrom
fix-canvas-185-crash
Open

fix(ui): harden board canvas against React #185 render-loop crash#1746
kgabryje wants to merge 3 commits into
mainfrom
fix-canvas-185-crash

Conversation

@kgabryje

@kgabryje kgabryje commented Jul 1, 2026

Copy link
Copy Markdown
Member

Problem

React #185 ("Maximum update depth exceeded") can fire on the board canvas
when the browser tab regains visibility after an OS desktop/space switch —
the ResizeObserver re-measures and a render loop starts (issues #1738, #1740).
Because the only ErrorBoundary was the global one, this white-screened the
entire app.

What changed

Deliverable 1 — canvas-scoped ErrorBoundary (primary, 7f4d79e1 + test 8570e41c)

Deliverable 2 — reduce the loop surface (secondary, e2904863)

  • The two always-mounted zone-toolbar <ColorPicker>s rebuilt their presets
    arrays on every render, churning antd's picker + rc-resize-observer internals.
  • Memoized the derived color palette and both presets arrays so references
    stay stable across renders. Kept the ColorPickers always-mounted (CSS-only
    visibility) to avoid reintroducing the mount/unmount flicker the current code
    guards against.

Verification

  • pnpm typecheck (agor-ui): 0 errors after building the @agor-live/client
    and @agor/core workspace deps (fresh worktree had no dist).
  • biome check on changed files: clean.
  • vitest (ErrorBoundary + SessionCanvas): 25 passed (23 existing + 2 new).

Not done / honest caveats

  • I did not reproduce the live ResizeObserver fix: prevent empty AgentChain from rendering #185 oscillation loop in
    jsdom — that timing-dependent loop isn't reliably reproducible in a unit test.
    Deliverable 1 is verified defense-in-depth (the boundary provably contains a
    render crash); Deliverable 2 is a targeted reduction of a known re-render
    source, not a proven root-cause fix.
  • The two deliverables are separate commits so they can be split into separate
    PRs if desired (the test commit belongs with Deliverable 1).

🤖 Generated with Claude Code

kgabryje and others added 3 commits July 1, 2026 12:28
React #185 ("Maximum update depth exceeded") can fire on the board
canvas when the tab regains visibility after an OS space/desktop switch
(ResizeObserver re-measurement). The only ErrorBoundary was the global
one, so a canvas render loop white-screened the entire app.

Add a canvas-scoped ErrorBoundary around <SessionCanvas> with a new
"canvas" variant: it contains the crash to the canvas subtree, keeps the
app shell (header, panels, nav) alive, and offers an in-place "Reload
canvas" reset instead of a full page reload. resetKey clears a stuck
error when the user switches boards. Crash reporting reuses the existing
crashReport helpers so auto-issue reporting is preserved.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The two zone-toolbar ColorPickers are always mounted (visibility is
CSS-only to avoid mount/unmount flicker), so a fresh `presets` array on
every render churns antd's picker internals and its rc-resize-observer
machinery — feeding the ResizeObserver re-measurement loop behind React
#185. Memoize the derived color palette and both `presets` arrays so
their references stay stable across renders.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Verifies the canvas-variant boundary contains a child render error while
the surrounding shell stays mounted, and that the "Reload canvas" action
re-mounts the subtree in place once the error condition clears.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kgabryje

kgabryje commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

🤖 Automated review — Claude Opus 4.8

Verdict: Approve (defense-in-depth). This hardens the blast radius of the #1738/#1740 render-loop crash; it isn't (and doesn't claim to be) a root-cause fix. Reviewed against head 8570e41c.

Deliverable 1 — canvas ErrorBoundary (correct)

  • <ErrorBoundary variant="canvas" resetKey={currentBoard?.board_id}> around <SessionCanvas>. resetKey is honored via getDerivedStateFromProps, so switching boards clears a stuck error. Error boundaries do catch React fix: prevent empty AgentChain from rendering #185 (that's how UI crash: unknown component — Minified React error #185; visit https://react.dev/errors/185 for the full messa #1738 captured a component stack), so the crash is genuinely contained to the canvas panel — the app shell survives instead of white-screening.
  • Report path preserved: there's no automatic crash-filer — the prod reports were filed by a human via "Report on GitHub", and CanvasCrashScreen reuses the same buildGitHubIssueUrl / buildMarkdownReport helpers. No telemetry regression.
  • sessionCanvasRef still lands on SessionCanvas (the boundary only wraps it), so the imperative ref / getViewportCenter path is unaffected.
  • Tests cover shell-survival + in-place reset.

Deliverable 2 — ColorPicker preset memoization (correct)

  • Memoizes colors + both presets arrays with correct deps (token, colors, recentColors). Removes a real per-render allocation on the always-mounted pickers. Kept always-mounted (CSS visibility) to avoid the documented flicker — right call.

Notes (non-blocking)

LGTM to merge as hardening once CI is green.

@kgabryje kgabryje added the ai-reviewed-claude-opus-4-8 Reviewed by Claude Opus 4.8 agent label Jul 1, 2026
@kgabryje

kgabryje commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

🤖 Automated review — Codex (GPT-5.5)

Verdict: Mergeable as defense-in-depth hardening — no correctness/regression blockers. (head 8570e41c)

Verified:

  • ref={sessionCanvasRef} still targets SessionCanvas, not the boundary; on a canvas crash React clears the ref and callers already optional-chain → degrades safely.
  • Required providers (ConnectionProvider, CanvasNavigationProvider, AppActionsProvider) remain above the new boundary — no context regression.
  • resetKey={board_id} clears a caught error on board switch (getDerivedStateFromProps); in-place "Reload canvas" recovers without a full page reload.
  • variant union widening ('scoped' | 'canvas' | 'global') is backward-compatible; existing global/scoped usages unaffected.
  • Memo deps (colors, borderColorPresets, backgroundColorPresets) complete; no stale-closure risk.

Minor (non-blocking):

  • The CanvasCrashScreen comment "auto-issue reporting is preserved" slightly overstates — it's a prefilled GitHub-issue link, not automatic filing. Suggest rewording (e.g. "issue-report link is preserved").
  • The index.ts re-export of CanvasCrashScreen is unused (ErrorBoundary imports it directly) — can drop.
  • Optional: scalar token-field deps for getColorPalette instead of [token].

@kgabryje kgabryje added the ai-reviewed-gpt-5-5 Reviewed by GPT-5.5 AI reviewer label Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed-claude-opus-4-8 Reviewed by Claude Opus 4.8 agent ai-reviewed-gpt-5-5 Reviewed by GPT-5.5 AI reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants