fix(frontend): reduce noisy PostHog client errors#1918
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe pull request adds detection and suppression for "known crawler noise" errors (specifically "Object Not Found Matching Id" patterns) in window error event handlers and PostHog exception suppression logic. Additionally, chart plugin rendering logic and tooltip management are enhanced with canvas context validation helpers to prevent errors when the rendering context is unavailable. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/stale-asset-errors.unit.test.ts (2)
3-3: Use~/alias for the service import.Replace
../src/services/staleAssetErrorswith the project alias form.🔧 Suggested change
-import { getErrorMessage, isKnownCrawlerNoiseErrorMessage, isStaleAssetErrorMessage, shouldSuppressPostHogExceptionEvent } from '../src/services/staleAssetErrors' +import { getErrorMessage, isKnownCrawlerNoiseErrorMessage, isStaleAssetErrorMessage, shouldSuppressPostHogExceptionEvent } from '~/services/staleAssetErrors'As per coding guidelines "
**/*.ts: Use path alias~/to map tosrc/for cleaner imports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/stale-asset-errors.unit.test.ts` at line 3, Update the import in the test to use the project path alias instead of a relative path: replace the import source '../src/services/staleAssetErrors' with the alias form '~/services/staleAssetErrors' so the imported symbols get resolved via the src/ alias; keep the imported names getErrorMessage, isKnownCrawlerNoiseErrorMessage, isStaleAssetErrorMessage, and shouldSuppressPostHogExceptionEvent unchanged.
22-27: Convert this new matcher test toit.concurrent().This case is independent and can run in parallel safely.
⚡ Suggested change
- it('matches the known crawler-only Object Not Found noise seen in PostHog', () => { + it.concurrent('matches the known crawler-only Object Not Found noise seen in PostHog', () => {As per coding guidelines "
tests/**/*.{ts,js}: Useit.concurrent()instead ofit()when possible to run tests in parallel within the same file, maximizing parallelism for faster CI/CD".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/stale-asset-errors.unit.test.ts` around lines 22 - 27, The test using it(...) is safe to run in parallel; change the test declaration to it.concurrent(...) for the case that asserts isKnownCrawlerNoiseErrorMessage outputs (replace the current it('matches the known crawler-only Object Not Found noise seen in PostHog', ...) with it.concurrent(...)). Ensure the test function name and assertions (isKnownCrawlerNoiseErrorMessage calls) remain unchanged so only the test runner call is updated.tests/chart-plugins.unit.test.ts (2)
3-4: Switch test imports to the~/alias.Use the frontend alias instead of relative
../src/...paths for TypeScript consistency.🔧 Suggested change
-import { inlineAnnotationPlugin } from '../src/services/chartAnnotations' -import { todayLinePlugin, verticalLinePlugin } from '../src/services/chartTooltip' +import { inlineAnnotationPlugin } from '~/services/chartAnnotations' +import { todayLinePlugin, verticalLinePlugin } from '~/services/chartTooltip'As per coding guidelines "
**/*.ts: Use path alias~/to map tosrc/for cleaner imports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/chart-plugins.unit.test.ts` around lines 3 - 4, Replace the relative imports in tests/chart-plugins.unit.test.ts with the project path alias by changing the import sources for inlineAnnotationPlugin, todayLinePlugin, and verticalLinePlugin to use the `~/` alias (e.g., import { inlineAnnotationPlugin } from '~/services/chartAnnotations' and import { todayLinePlugin, verticalLinePlugin } from '~/services/chartTooltip') so the tests use the TypeScript frontend alias mapping to src/.
7-82: Run these independent tests concurrently.These three plugin guard tests are isolated and can use
it.concurrent()to improve CI throughput.⚡ Suggested change
- it('skips drawing the vertical hover line when the chart context is gone', () => { + it.concurrent('skips drawing the vertical hover line when the chart context is gone', () => { @@ - it('skips drawing the today line when the chart context is gone', () => { + it.concurrent('skips drawing the today line when the chart context is gone', () => { @@ - it('skips inline annotations when the chart context is gone', () => { + it.concurrent('skips inline annotations when the chart context is gone', () => {As per coding guidelines "
tests/**/*.{ts,js}: Useit.concurrent()instead ofit()when possible to run tests in parallel within the same file, maximizing parallelism for faster CI/CD".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/chart-plugins.unit.test.ts` around lines 7 - 82, Replace the three synchronous tests that call verticalLinePlugin.afterDatasetsDraw, todayLinePlugin.afterDatasetsDraw, and inlineAnnotationPlugin.afterDatasetsDraw with concurrent tests by changing it(...) to it.concurrent(...); ensure the test bodies and assertions remain unchanged so each test still calls the same plugin method with the same mock chart context and uses expect(...).not.toThrow().src/services/chartTooltip.ts (1)
24-29: ExtractgetCanvasContextinto a shared helper to avoid drift.The same helper now exists in both
src/services/chartTooltip.tsandsrc/services/chartAnnotations.ts. Centralizing it keeps guard behavior consistent.♻️ Proposed refactor
- function getCanvasContext(ctx: unknown): CanvasRenderingContext2D | null { - if (ctx && typeof (ctx as CanvasRenderingContext2D).save === 'function') - return ctx as CanvasRenderingContext2D - - return null - } + import { getCanvasContext } from '~/services/chartCanvas'// src/services/chartCanvas.ts export function getCanvasContext(ctx: unknown): CanvasRenderingContext2D | null { if (ctx && typeof (ctx as CanvasRenderingContext2D).save === 'function') return ctx as CanvasRenderingContext2D return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/chartTooltip.ts` around lines 24 - 29, Extract the duplicated getCanvasContext function into a single shared helper module by creating and exporting function getCanvasContext(ctx: unknown): CanvasRenderingContext2D | null with the same guard (checking ctx && typeof (ctx as CanvasRenderingContext2D).save === 'function'), then remove the duplicate implementations from both modules that currently define getCanvasContext and update those modules to import and use the shared getCanvasContext export; keep the signature and behavior identical so callers in chartTooltip and chartAnnotations continue to work without further 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 `@src/services/chartTooltip.ts`:
- Around line 24-29: Extract the duplicated getCanvasContext function into a
single shared helper module by creating and exporting function
getCanvasContext(ctx: unknown): CanvasRenderingContext2D | null with the same
guard (checking ctx && typeof (ctx as CanvasRenderingContext2D).save ===
'function'), then remove the duplicate implementations from both modules that
currently define getCanvasContext and update those modules to import and use the
shared getCanvasContext export; keep the signature and behavior identical so
callers in chartTooltip and chartAnnotations continue to work without further
changes.
In `@tests/chart-plugins.unit.test.ts`:
- Around line 3-4: Replace the relative imports in
tests/chart-plugins.unit.test.ts with the project path alias by changing the
import sources for inlineAnnotationPlugin, todayLinePlugin, and
verticalLinePlugin to use the `~/` alias (e.g., import { inlineAnnotationPlugin
} from '~/services/chartAnnotations' and import { todayLinePlugin,
verticalLinePlugin } from '~/services/chartTooltip') so the tests use the
TypeScript frontend alias mapping to src/.
- Around line 7-82: Replace the three synchronous tests that call
verticalLinePlugin.afterDatasetsDraw, todayLinePlugin.afterDatasetsDraw, and
inlineAnnotationPlugin.afterDatasetsDraw with concurrent tests by changing
it(...) to it.concurrent(...); ensure the test bodies and assertions remain
unchanged so each test still calls the same plugin method with the same mock
chart context and uses expect(...).not.toThrow().
In `@tests/stale-asset-errors.unit.test.ts`:
- Line 3: Update the import in the test to use the project path alias instead of
a relative path: replace the import source '../src/services/staleAssetErrors'
with the alias form '~/services/staleAssetErrors' so the imported symbols get
resolved via the src/ alias; keep the imported names getErrorMessage,
isKnownCrawlerNoiseErrorMessage, isStaleAssetErrorMessage, and
shouldSuppressPostHogExceptionEvent unchanged.
- Around line 22-27: The test using it(...) is safe to run in parallel; change
the test declaration to it.concurrent(...) for the case that asserts
isKnownCrawlerNoiseErrorMessage outputs (replace the current it('matches the
known crawler-only Object Not Found noise seen in PostHog', ...) with
it.concurrent(...)). Ensure the test function name and assertions
(isKnownCrawlerNoiseErrorMessage calls) remain unchanged so only the test runner
call is updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a99aa33d-7264-4c7e-b0bc-0585d7e552a1
📒 Files selected for processing (6)
src/main.tssrc/services/chartAnnotations.tssrc/services/chartTooltip.tssrc/services/staleAssetErrors.tstests/chart-plugins.unit.test.tstests/stale-asset-errors.unit.test.ts
Ari4ka
approved these changes
Apr 17, 2026
3 tasks
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
2 participants
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 (AI generated)
Object Not Found Matching Id:*client errors before they reach PostHogbefore_sendsuppression aligned with the same crawler-noise matchernull.savecrashesMotivation (AI generated)
PostHog error tracking is currently dominated by two frontend-only problems: crawler-generated noise from email link scanning, and chart plugin redraws that run after the canvas context is gone. Those issues hide real regressions and waste time during triage.
Business Impact (AI generated)
This reduces false-positive production noise in PostHog so real customer-facing errors stay visible, and it removes a real chart crash affecting mobile users on stats and bundles views. Cleaner monitoring improves support response and lowers the risk of missing genuine regressions.
Test Plan (AI generated)
bunx vitest run tests/stale-asset-errors.unit.test.ts tests/chart-plugins.unit.test.tsbun run typecheckGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests