#8030: add slow/bad selector analysis warnings

[twschiller]

What does this PR do?

• Closes Add selector analysis warnings #8030
• Adds selector analysis warnings:
  • Invalid selector: Error
  • Selector with random characters: Warning
  • Selector that might slow down the page: Warning

Reviewer Notes

• Vendor check fails due to fixed typo. That particular file is one we've made some modifications to
• Strict null checks not possible due to dependency on the registry

Discussion

• Uses the pre-existing randomness detection we had prototyped for the "Exclude Random Classes" skunkworks feature (which has never been turned on for all users)
• JQuery doesn't report what's invalid about the selector when it throws its error. Otherwise it'd be nice to explain where the syntax/invalid selector is
• Based on dogfooding, we might choose to make the randomness check an info instead of warning

Demo

Future Work

• Generalize the schema recursion to be available in other analyses. (We have a generic visitExpression, but that is not schema-aware)
• Consider warning on use of known unstable selectors:

  pixiebrix-extension/src/utils/inference/selectorInference.ts

  Line 79 in 641e539

  const UNSTABLE_SELECTORS = [

• Consider warning on excessive structural selectors (e.g., :nth-child)
• Consider info on using contains (because it's not robust to translation)

Checklist

• Add tests and/or storybook stories
• Designate a primary reviewer: @fungairino

[twschiller]

Just fixing a spelling mistake

[codecov]

Codecov Report

Attention: Patch coverage is 98.41270% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 73.09%. Comparing base (dfe97b3) to head (3a9d4c6).
Report is 15 commits behind head on main.

❗ Current head 3a9d4c6 differs from pull request most recent head b456572. Consider uploading reports for the commit b456572 to get more accurate results

Files	Patch %	Lines
src/analysis/analysisVisitors/selectorAnalysis.ts	99.08%	1 Missing ⚠️
src/utils/domUtils.ts	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8031      +/-   ##
==========================================
+ Coverage   72.98%   73.09%   +0.10%     
==========================================
  Files        1307     1308       +1     
  Lines       40588    40711     +123     
  Branches     7538     7574      +36     
==========================================
+ Hits        29625    29756     +131     
+ Misses      10963    10955       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fungairino]

It seems like some of the visiting logic could be refactored to a common util as it's not specific to selector analysis.

[twschiller]

There's some assumptions in there, e.g., only checking string typed values that aren't general. It will be good to generalize in the future in a base class. See PR's discussion section, we'll likely want a base class implementation that provides a visitValueWithSchema or similar method

[fungairino]

likely want a base class implementation that provides a visitValueWithSchema or similar method

That's what I had in mind, yea.

[fungairino]

nit: Looks like checkTrigger is very similar to this method and we could extract some common logic or helper methods.

[twschiller]

Agree there's duplication, but there's enough differences (in the path, message, and etc.) I think that abstracting it will be harder to read vs. just repeating the lines of code. If we add any other brick types, would definitely look to abstract

[github-actions]

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.