fix(toolbar): stop finder unique() throwing on 0-match candidates - #64166
Draft
posthog[bot] wants to merge 1 commit into
Draft
fix(toolbar): stop finder unique() throwing on 0-match candidates#64166posthog[bot] wants to merge 1 commit into
posthog[bot] wants to merge 1 commit into
Conversation
A candidate selector that matches zero nodes (a DOM race during hover, or a class scoped to a shadow root / iframe) made the vendored finder's unique() throw. The throw is caught in computeElementQuery, which reports it via captureToolbarException as pure error-tracking noise. Treat a 0-match as non-unique so every caller (the main loop, optimize(), and fallback()) skips it and falls back to a positional path instead. Generated-By: PostHog Code Task-Id: 3ffe9680-6d40-429a-a480-d8022369d278
Contributor
|
Reviews (1): Last reviewed commit: "fix(toolbar): stop finder unique() throw..." | Re-trigger Greptile |
Contributor
|
Size Change: -2.09 kB (0%) Total Size: 63.7 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
Contributor
🕸️ Eager graphHow much code each root forces the browser to download and decode through static imports — the regression class total bundle size can't see.
✅ Largest files eagerly reachable from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly reachable from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.5 KiB | ../node_modules/.pnpm/posthog-js@1.407.2/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 236.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 226.1 KiB | ../node_modules/.pnpm/posthog-js@1.407.2/node_modules/posthog-js/dist/module.js |
| 167.1 KiB | src/queries/validators.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.8 KiB | src/lib/api.ts |
| 94.0 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Posted automatically by check-eager-graph · sizes are input-source bytes from the esbuild metafile · part of #32479
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
Contributor
Author
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
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
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.
Problem
The toolbar's selector generator was logging a caught exception into error tracking as noise. A new issue (
Can't select any node with this selector: .text-primary-foreground) appeared fromtoolbar.js, surfaced as a fresh fingerprint after the finder was vendored (9823b19e14) and the minified line/column offsets shifted. Users are not affected — the exception is caught — but the team gets a new active issue each time the bundle changes.The throw originates in
unique()in the vendored@medv/finderwhen a generated candidate selector matches 0 nodes viaquerySelectorAll(a DOM race during hover, or a shadow-DOM / iframe-scoped class). It is caught incomputeElementQuery, which returnsundefinedand reports it viacaptureToolbarException— so it is pure error-tracking noise, not user-facing breakage.Changes
Treat a 0-match in
unique()as non-unique (returnfalse) rather than throwing. A selector that matches zero nodes is by definition not a unique match, and every caller already handles the non-unique case gracefully: the main candidate loop skips it and continues,optimize()skips that optimization, andfallback()returnsundefinedso its callers degrade to a positional path. This removes the throw at its single source, which also closes the latent case wherefallback()'s ownunique()call could throw on a positional path that briefly matches nothing.Added a regression test that forces every class-based candidate down the 0-match path while leaving the positional fallback resolvable, asserting
finder()returns a working selector instead of throwing. Verified the test fails against the old throwing code and passes with the fix.How did you test this code?
I'm an agent. I ran the vendored finder Jest suite (
frontend/src/toolbar/vendor/finder/index.test.ts) — all 7 tests pass, including the new regression test. I confirmed the new test fails against the pre-fix (throwing) code, then passes once the fix is restored. No manual browser testing was performed.🤖 Agent context
Autonomy: Fully autonomous
Authored by PostHog Code from an inbox report. The root cause was traced to the unguarded normal-path throw in
unique()(the prior vendoring commit added a positionalfallback()only for the combinatorial-cap and timeout branches). Two fixes were considered: wrapping the main loop'sunique()call withfallback(), or suppressing the specific exception incaptureToolbarException. I chose to fix the root cause inunique()instead — it is the single throw site shared by the loop,optimize(), andfallback(), so returningfalseon a 0-match resolves all of them with the smallest change and no behavioral regression.Created with PostHog Code from an inbox report.