fix(frontend): guard chart tooltip canvas lifecycle

[riderx]

Summary (AI generated)

• skip custom chart overlay drawing when the chart canvas has already been disconnected
• reuse the connected-canvas check in the chart tooltip service instead of assuming the canvas is still alive
• add unit coverage for disconnected canvases in the vertical hover line and today line plugins

Motivation (AI generated)

PostHog reported a TypeError: Cannot read properties of null (reading 'save') from the chart tooltip bundle. Our custom Chart.js plugins were still attempting to draw after the canvas lifecycle had already ended, which can happen during teardown or navigation.

Business Impact (AI generated)

This removes a chart crash from statistics screens, especially around fast navigation or mobile teardown paths, which keeps analytics views stable and reduces noisy client-side errors in PostHog.

Test Plan (AI generated)

• bunx vitest run tests/chart-plugins.unit.test.ts
• bunx eslint src/services/chartTooltip.ts tests/chart-plugins.unit.test.ts
• bun typecheck

Generated with AI

Summary by CodeRabbit

• Bug Fixes

  • Improved chart rendering stability by skipping drawing when the chart's canvas or rendering context is not connected, preventing spurious tooltip/line rendering and errors.

• Tests

  • Added unit tests covering plugin behavior when the canvas connection is absent to ensure no drawing or state changes occur.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fd52d9f6-9307-470d-8bc8-ece7edb0c142

📥 Commits

Reviewing files that changed from the base of the PR and between 8fbab4f and 36abe6f.

📒 Files selected for processing (1)

• tests/chart-plugins.unit.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/chart-plugins.unit.test.ts

---

📝 Walkthrough

Walkthrough

Adds a non-exported helper that checks whether a Chart's canvas (or its context's canvas) is connected, switches chart-update eligibility to use that helper, and adds early-return guards in two plugin draw hooks; tests added to assert no drawing occurs when canvas.isConnected is false.

Changes

Cohort / File(s)	Summary
Canvas validation & plugins src/services/chartTooltip.ts	Added hasConnectedCanvas(chart) helper; updated canSafelyUpdateChart() to use it; added early-return guards in verticalLinePlugin.afterDatasetsDraw and todayLinePlugin.afterDatasetsDraw so drawing is skipped when canvas/context is disconnected.
Plugin unit tests tests/chart-plugins.unit.test.ts	Imported vi from vitest; added tests asserting verticalLinePlugin.afterDatasetsDraw and todayLinePlugin.afterDatasetsDraw do not throw and do not call ctx.save when canvas.isConnected is false.

Sequence Diagram(s)

(No sequence diagrams generated — changes are small, localized guards and tests.)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• fix(frontend): reduce noisy PostHog client errors #1918: Modifies the same src/services/chartTooltip.ts to introduce canvas/context guard logic and early-return checks in plugin draw hooks.

Suggested reviewers

• Ari4ka

Poem

🐰 I sniff the canvas, gentle and light,
If not connected, I skip the night,
No frantic saves or phantom line,
The plot stays calm, the pixels fine,
A quiet hop — rendering done right. 🎨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: adding guards for the chart tooltip canvas lifecycle to prevent errors when canvas is disconnected.
Description check	✅ Passed	The description includes a summary of changes, motivation, and test plan section with test execution steps. However, it lacks screenshots/videos section and the standard checklist with validation items.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/posthog-chart-canvas-guard

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codspeed-hq]

Merging this PR will not alter performance

✅ 28 untouched benchmarks

---

_{Comparing codex/posthog-chart-canvas-guard (36abe6f) with main (b8a984c)}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

🧹 Nitpick comments (1)

tests/chart-plugins.unit.test.ts (1)

48-48: Use it.concurrent for these two independent unit tests.

Line [48] and Line [75] can run in parallel and should use it.concurrent(...) to align with test-suite conventions.

♻️ Suggested patch

-  it('skips drawing the vertical hover line when the canvas is disconnected', () => {
+  it.concurrent('skips drawing the vertical hover line when the canvas is disconnected', () => {
@@
-  it('skips drawing the today line when the canvas is disconnected', () => {
+  it.concurrent('skips drawing the today line when the canvas is disconnected', () => {

As per coding guidelines, tests/**/*.{ts,js}: Use it.concurrent() instead of it() when possible to run tests in parallel within the same file, maximizing parallelism for faster CI/CD.

Also applies to: 75-75

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/chart-plugins.unit.test.ts` at line 48, Two independent test cases
should run in parallel: replace the two `it(...)` blocks with
`it.concurrent(...)` for the test titled "skips drawing the vertical hover line
when the canvas is disconnected" and the other `it` at the reported location
(line ~75) so both tests use `it.concurrent(...)`; locate the `it` calls by
their test titles in tests/chart-plugins.unit.test.ts and change their callers
from `it` to `it.concurrent` (no other logic changes).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/chart-plugins.unit.test.ts`:
- Line 48: Two independent test cases should run in parallel: replace the two
`it(...)` blocks with `it.concurrent(...)` for the test titled "skips drawing
the vertical hover line when the canvas is disconnected" and the other `it` at
the reported location (line ~75) so both tests use `it.concurrent(...)`; locate
the `it` calls by their test titles in tests/chart-plugins.unit.test.ts and
change their callers from `it` to `it.concurrent` (no other logic changes).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5d05fac2-e69c-4c15-9c11-04e7c1ede7e4

📥 Commits

Reviewing files that changed from the base of the PR and between b8a984c and 8fbab4f.

📒 Files selected for processing (2)

• src/services/chartTooltip.ts
• tests/chart-plugins.unit.test.ts

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud