Skip to content

fix: guard Output page against stale async chart renders (#101) - #252

Open
Sanjay Singh (san360) wants to merge 1 commit into
mainfrom
san360-fix-output-render-error
Open

fix: guard Output page against stale async chart renders (#101)#252
Sanjay Singh (san360) wants to merge 1 commit into
mainfrom
san360-fix-output-render-error

Conversation

@san360

Copy link
Copy Markdown
Contributor

Summary

Fixes #101 — the Output page's intermittent ⚠️ Failed to render Output — Cannot read properties of undefined/null (reading …) error boundary crash.

Root cause

createChart() in src/webview/shared.ts passed document.getElementById(canvasId) straight into new Chart(...) with no null check. renderOutput/renderProductionTab/renderTokenUsageTab in src/webview/page-output.ts render a spinner, await rpc(...), then re-render tab markup and call createChart(...) — with no cancellation of stale in-flight renders across that async gap.

If the user switched nav pages, toggled the Code Output ↔ Token Usage tab, or clicked date-range buttons rapidly while a request was still in flight, the stale promise would eventually resolve and either:

  • find its expected canvas missing from the live DOM (Chart.js dereferences the null element and throws), or
  • find the canvas still present but belonging to a different tab that has since been rendered, silently clobbering it with stale data.

Fix (two layers, per the repro/fix notes in #101)

  1. createChart (src/webview/shared.ts) — now checks document.getElementById(canvasId) for null and returns null with a console.warn instead of handing a null element to new Chart(...). Return type updated to Chart | null; no call site used the return value, so this is a safe, non-breaking change.
  2. page-output.ts — added a module-level renderGeneration counter, bumped once per render attempt (initial mount, tab switch, or range change) in renderActiveTab(). renderProductionTab/renderTokenUsageTab capture the generation at entry and bail out immediately after their await rpc(...) if a newer render has since started, so a slow/superseded response never touches the DOM again.

Testing

Ran the full repo check suite from the worktree:

  • npm run typecheck
  • npm run lint ✅ (0 errors; pre-existing warnings only, none introduced)
  • npm run spellcheck
  • npm run knip
  • npm test (vitest) — 1341 passed; 7 failures in src/core/github-app-analytics.test.ts are pre-existing and unrelated (verified by stashing this change and re-running the same file — identical failures on main).
  • npm run build + npx playwright test (full e2e suite) — 37 passed, 7 skipped (pre-existing feature-flag gating), 0 failures, including all applicable tests/e2e/output.spec.ts cases.

Notes

  • No dependency, config, or public API changes.
  • createChart's new Chart | null return type doesn't affect any existing caller (none of the ~30 call sites across the webview capture the return value).

- createChart() now bails out with a warning instead of crashing when its
  canvas isn't in the live DOM (Chart.js dereferences a null/undefined
  element and throws otherwise).
- page-output.ts tracks a render generation counter; renderProductionTab
  and renderTokenUsageTab bail out after their await if a newer render
  (tab switch, range change, or re-navigation) has since started, so a
  slow response can no longer overwrite the current tab's DOM.

Fixes intermittent 'Failed to render Output' error boundary crashes when
switching pages/tabs/ranges while Output is still loading.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

⚠️ Deprecation Warning: The deny-licenses option is deprecated for possible removal in the next major release. For more information, see issue 997.

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Output page intermittently shows "Failed to render Output — Cannot read properties of null (reading 'id')"

2 participants