fix(performanceTimer): work in frames #4834
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the performanceTimer fails to work correctly in iframe environments and improves logging detail for rule gathering performance. The changes rename performance markers from "axe" to "audit" and enhance the gather performance log to include rule ID and node count information.
- Renamed performance markers from
mark_axe_starttomark_audit_startto fix iframe compatibility - Enhanced gather performance logging to include rule ID and node count for better debugging visibility
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/core/utils/performance-timer.js | Updates performance marker name from 'mark_axe_start' to 'mark_audit_start' to resolve iframe issues |
| lib/core/base/rule.js | Improves gather performance logging to include rule ID and node count with template literal formatting |
| const { startTime } = | ||
| window.performance.getEntriesByName('mark_axe_start')[0] || | ||
| window.performance.getEntriesByName('mark_audit_start')[0]; |
There was a problem hiding this comment.
This error is kinda making me wonder if we should rethink not having tests for this. WDYT?
There was a problem hiding this comment.
I agree; I think tests would be a good idea (especially since performanceTimer is a documented part of our API).
| const { startTime } = | ||
| window.performance.getEntriesByName('mark_axe_start')[0] || | ||
| window.performance.getEntriesByName('mark_audit_start')[0]; |
There was a problem hiding this comment.
I agree; I think tests would be a good idea (especially since performanceTimer is a documented part of our API).
| @@ -90,11 +90,12 @@ const performanceTimer = (() => { | |||
| ) { | |||
| // only output measures that were started after axe started, otherwise | |||
| // we get measures made by the page before axe ran (which is confusing) | |||
| const axeStart = | |||
| window.performance.getEntriesByName('mark_axe_start')[0]; | |||
| const { startTime } = | |||
There was a problem hiding this comment.
nit: A few lines up, this call is defensive about the possibility of window.performance not existing at all; should it also be a bit more defensive about the possibility of the mark_*_start events not being available (eg, assert with a meaningful message that start or auditStart must be called first)?
| @@ -90,11 +90,12 @@ const performanceTimer = (() => { | |||
| ) { | |||
There was a problem hiding this comment.
nit: The actual functionality here feels unclear from the name/comment. For L81:
/**
* @member {Function} logMeasures Logs previously captured measures in chronological order.
* Starts from the most recent start()/auditStart() call.
* @param {String|undefined} measureName If provided, will only log up to the first matching measure.
*/
| const axeStart = | ||
| window.performance.getEntriesByName('mark_axe_start')[0]; | ||
| const { startTime } = | ||
| window.performance.getEntriesByName('mark_axe_start')[0] || |
There was a problem hiding this comment.
I don't think this actually does the right thing in the event that you're in an environment with more than one mark, since we never seem to clear marks/measures out from previous runs. I think what you'd actually want here is "grab the most recent (not 0th) of each type of mark, and if both exist, use the one with later start_time)".
Turns out our performanceTimer errors when running in iframes. I also added some detail to gather so we could see what rule is being gathered.