fix: reduce perf comparator false positives - #860
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 26 minutes and 48 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/testing/compare-perf-script.test.ts, packages/app/src/testing/perf-metrics.test.ts, packages/app/src/testing/perf-metrics.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request enhances the performance comparison logic by introducing a confirmation mechanism that filters failures based on previous run results. Key changes include updating the compare-perf.ts script to extract specific failure keys, increasing the interaction median threshold from 10ms to 20ms, and adding comprehensive tests for the new confirmation workflow. Feedback from the reviewer highlights an opportunity to improve readFailureScope by capturing 'hard failures' (such as missing scenarios) from the top-level failures array to prevent incorrect passes. Additionally, it was recommended to deduplicate comparisonKeys in comparePerfBaselines to ensure the output summary remains concise and free of duplicate entries.
Perf delta summaryComparator: pass
|
Summary
interaction_ms_medianfloor from10msto20msso single-frame-ish deltas do not block the gate.profile:scenario:metrickeys instead of any failure in the same scenario.Why
Issue #736 showed two structural false positives in the perf comparator: the default interaction median threshold treated below-frame timing noise as a blocking regression, and the confirm pass could fail when a different metric regressed in the same scenario. This keeps the gate blocking only when the same metric failure repeats, while still failing corrupted or incomplete confirm artifacts.
Related Issue
Closes #736.
Human Review Status
Pending
Review Focus
Please check the boundary between metric-key filtering and hard missing-scenario failures, especially the requested scenario case where base and head are both absent.
Risk Notes
Behavior risk: this intentionally stops blocking on default
interaction_ms_mediandeltas at or below20ms, and on confirm-stage failures for different metrics in the same scenario. Catastrophic thresholds, low-end warning behavior, the perf scenario list, and the #746/#853 probe-window guard are unchanged.Skipped conditional checklist items:
Pre-fix workflow context only: the most recent 50
perf-probe-baselineruns sampled before this PR showed 38 success, 11 failure, and 1 run with no conclusion. This PR still relies on post-merge/PR CI to measure whether the false-positive rate drops in practice.How To Verify
Screenshots or Recordings
Not applicable: no visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.