Conversation
When a verification flight has poor or fair data quality (< 60/100), show a warning modal with failing sub-scores and two options: - "Fly Again" — discard and let user retry with better stick inputs - "Accept Anyway" — proceed with low-quality verification data Previously the app silently accepted any verification data regardless of quality, which could produce unreliable convergence detection and tune quality scores. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a “verification flight quality gate” to prevent completing a tuning session with low-quality verification data by showing a warning modal that summarizes overall/sub-score quality and lets the user retry or proceed.
Changes:
- Introduces
VerificationQualityWarningmodal to present low verification data quality details and actions. - Captures
dataQualityfrom PID/filter verification analysis results and blocks completion onfair/pooruntil user decision. - Adds unit tests for the new warning modal component.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/renderer/components/TuningHistory/VerificationQualityWarning.tsx |
New modal UI to warn about low verification data quality and show failing sub-scores. |
src/renderer/components/TuningHistory/VerificationQualityWarning.test.tsx |
Unit tests covering rendering, sub-score filtering, callbacks, and dialog role. |
src/renderer/App.tsx |
Adds pending-verification state + commit helper, gates verification completion on dataQuality.tier, and wires the modal into the app flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const tierLabel = dataQuality.tier === 'poor' ? 'Poor' : 'Fair'; | ||
| const tierColor = dataQuality.tier === 'poor' ? '#e74c3c' : '#f39c12'; |
There was a problem hiding this comment.
DataQualityScore['tier'] includes 'excellent' and 'good', but this component maps any non-'poor' tier to 'Fair' (and uses the fair color). This would mislabel higher-quality tiers if the component is ever rendered with them; consider deriving the label/color from the actual tier (or asserting/handling unexpected tiers explicitly).
| const tierLabel = dataQuality.tier === 'poor' ? 'Poor' : 'Fair'; | |
| const tierColor = dataQuality.tier === 'poor' ? '#e74c3c' : '#f39c12'; | |
| const tierDisplay: Record<DataQualityScore['tier'], { label: string; color: string }> = { | |
| poor: { label: 'Poor', color: '#e74c3c' }, | |
| fair: { label: 'Fair', color: '#f39c12' }, | |
| good: { label: 'Good', color: '#27ae60' }, | |
| excellent: { label: 'Excellent', color: '#2ecc71' }, | |
| }; | |
| const { label: tierLabel, color: tierColor } = tierDisplay[dataQuality.tier]; |
There was a problem hiding this comment.
Fixed — using exhaustive tier→label/color map in 22f0dcf.
| const handleQualityGateAccept = async () => { | ||
| if (!pendingVerification) return; | ||
| try { | ||
| setAnalyzingVerification(true); | ||
| await commitVerification( | ||
| pendingVerification.verificationMetrics, | ||
| pendingVerification.verificationPidMetrics, | ||
| pendingVerification.verificationTFMetrics, | ||
| pendingVerification.historyRecordId, | ||
| pendingVerification.isReanalyze | ||
| ); | ||
| } catch (err) { | ||
| toast.error(err instanceof Error ? err.message : 'Failed to complete verification'); | ||
| } finally { | ||
| setPendingVerification(null); | ||
| setAnalyzingVerification(false); | ||
| } |
There was a problem hiding this comment.
handleQualityGateAccept can be triggered multiple times (e.g., double-clicking “Accept Anyway”), which could run commitVerification concurrently and perform duplicate updates. Consider guarding against re-entry (disable modal buttons while accepting, clear pendingVerification before awaiting, or add an in-flight flag).
There was a problem hiding this comment.
Fixed — pendingVerification is cleared before await in 22f0dcf.
| historyRecordId, | ||
| isReanalyze | ||
| ); | ||
| setErasedForPhase(null); |
There was a problem hiding this comment.
commitVerification already calls setErasedForPhase(null), so calling it again after await commitVerification(...) is redundant.
| setErasedForPhase(null); |
Address CodePilot feedback: - Use exhaustive tier→label/color map instead of ternary - Clear pendingVerification before await to prevent double-click - Remove redundant setErasedForPhase (already in commitVerification) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Test plan
VerificationQualityWarningcomponent (renders, callbacks, sub-scores)🤖 Generated with Claude Code