-
Notifications
You must be signed in to change notification settings - Fork 31
detector service for breakage reporting #2052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for content-scope-scripts ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Temporary Branch UpdateThe temporary branch has been updated with the latest changes. Below are the details:
Please use the above install command to update to the latest version. |
[Beta] Generated file diffTime updated: Wed, 12 Nov 2025 15:46:34 GMT Android
File has changed Apple
File has changed Chrome-mv3File has changed FirefoxFile has changed IntegrationFile has changed WindowsFile has changed |
madblex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. I added some comments and a couple more here:
- I'd like to see how this integrates with content features, especially broker protection, i.e. the equivalent of this
- I see we removed
WebInterferenceDetectioncontent feature. Is it because you don't think we'll need a standalone feature for this? i.e. the detectors will only be called from other features. - I see we're going the functional route (from classes). Not feeling strongly (I prefer functional most of the time), I just believe the trade-off is that interfaces will be more difficult to enforce. I think classes provide a bit of a better structure, but this is opinionated.
| const bundledFeatureNames = typeof importConfig.injectName === 'string' ? platformSupport[importConfig.injectName] : []; | ||
|
|
||
| // Initialize detectors early so they're available when features init | ||
| initDetectors(args.bundledConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is running on a critical path, I believe we should surround it with try/catch and fire a pixel on error here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 good catch
| * @param {boolean} [options._autoRun] - Internal flag indicating auto-run (gates apply) | ||
| * @returns {Promise<Record<string, any>>} Object mapping detector IDs to their data | ||
| */ | ||
| export async function getDetectorBatch(detectorIds, options = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use a better name here, currently it's a bit misleading. getDetectorBatch makes me think we're just getting the detectors based on args, not that they'll also be running. Suggestions:
runDetectors / getDetectorsData, etc.
Since this is also a "public" service to the repo, I'd interject the word web to make it clear what these detectors are about, i.e. runWebDetectors / getWebDetectorsData or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point. I'll go with getDetectorsData. I don't think we need web prefix as we're not following that naming scheme elsewhere (e.g., getJsPerformanceMetrics, getExpandedPerformanceMetrics)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we still be needing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I can drop this
| @@ -0,0 +1,62 @@ | |||
| import { isVisible, queryAllSelectors } from '../utils/detection-utils.js'; | |||
| const DEFAULT_CONFIG = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add to DEFAULT_DETECTOR_SETTINGS?
|
Asana Task/Github Issue:
Description
Building on #2048 with a more generalize framework for breakage reports.
New detector service:
shouldRun()checks (only during auto-run)Changed:
See the injected/src/detectors/README.md for more details
Testing Steps
Checklist
Please tick all that apply: