fix: 8 tuning session bugs — data loss, stale state, missing TF metrics#432
fix: 8 tuning session bugs — data loss, stale state, missing TF metrics#432
Conversation
Main process: - archiveSession() now copies verificationTransferFunctionMetrics and recommendationTraces to history records (Flash Tune data was lost) - updateRecordVerification() accepts TF metrics parameter - TUNING_UPDATE_HISTORY_VERIFICATION handler passes TF metrics through IPC/preload: - updateHistoryVerification signature includes TF metrics parameter Renderer: - isReanalyze reset after use (prevented quality gate on 2nd attempt) - pendingVerification cleared on: wizard exit, session dismiss, phase change - verificationPickerLogId cleared on dismiss - commitVerification passes TF metrics to history record updates - All isReanalyze references use local snapshot (not stale state) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes multiple tuning-session data flow issues across renderer ↔ preload ↔ main so verification metrics (including Flash Tune transfer-function verification) and recommendation telemetry are preserved and can be re-applied during history reanalysis, while also reducing stale renderer state during verification flows.
Changes:
- Extend
updateHistoryVerificationIPC/API to include transfer-function verification metrics end-to-end (renderer → preload → main → history storage). - Archive additional session fields into history (
verificationTransferFunctionMetrics,recommendationTraces) and add a regression test. - Reset several renderer-side verification states on dismiss/phase changes and avoid stale React state reads during verification analysis.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/types/ipc.types.ts | Adds TF verification metrics parameter to updateHistoryVerification API type. |
| src/renderer/App.tsx | Clears more verification UI state on certain transitions; passes TF metrics through history-update path; snapshots isReanalyze to avoid stale reads. |
| src/preload/index.ts | Forwards new TF verification metrics arg through IPC invoke. |
| src/main/storage/TuningHistoryManager.ts | Archives TF verification metrics + recommendation traces; supports updating TF verification metrics on a specific record. |
| src/main/storage/TuningHistoryManager.test.ts | Adds regression test ensuring new fields are archived. |
| src/main/ipc/handlers/tuningHandlers.ts | Extends IPC handler to accept and persist TF verification metrics for record updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Clear stale verification state when session changes (disconnect, reset, new session) | ||
| const sessionPhase = tuning.session?.phase; | ||
| useEffect(() => { | ||
| setPendingVerification(null); |
There was a problem hiding this comment.
The new sessionPhase effect only clears pendingVerification, but other verification-related UI state can remain set across session/profile changes (e.g., verificationPickerLogId keeps VerificationSessionModal open, and reanalyzeHistoryRecordId/isReanalyze can leak into the next flow). Consider clearing verificationPickerLogId, reanalyzeHistoryRecordId, and isReanalyze in the same effect so any session change fully resets verification UI state.
| setPendingVerification(null); | |
| setPendingVerification(null); | |
| setVerificationPickerLogId(null); | |
| setReanalyzeHistoryRecordId(null); | |
| setIsReanalyze(false); |
There was a problem hiding this comment.
Fixed in 3851eab — useEffect now clears all verification state together.
| setErasedForPhase(null); | ||
| setPendingVerification(null); | ||
| setVerificationPickerLogId(null); | ||
| setIsReanalyze(false); |
There was a problem hiding this comment.
In the 'dismiss' action you clear pendingVerification/verificationPickerLogId/isReanalyze, but reanalyzeHistoryRecordId is left intact. If the user previously started a history reanalysis (sets reanalyzeHistoryRecordId) and then dismisses/resets, the next verification analysis can incorrectly be treated as a history update and overwrite the wrong record. Clear reanalyzeHistoryRecordId here as part of the reset.
| setIsReanalyze(false); | |
| setIsReanalyze(false); | |
| setReanalyzeHistoryRecordId(null); |
There was a problem hiding this comment.
Fixed in 3851eab — reanalyzeHistoryRecordId added to dismiss cleanup.
Address CodePilot feedback: - useEffect on sessionPhase clears verificationPickerLogId, reanalyzeHistoryRecordId, isReanalyze (not just pendingVerification) - dismiss handler also clears reanalyzeHistoryRecordId 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
Comprehensive review found 8 bugs in tuning session data flow. Fixes:
Data loss (main process):
archiveSession()now copiesverificationTransferFunctionMetrics(Flash Tune TF verification was permanently lost)archiveSession()now copiesrecommendationTraces(telemetry data was lost)updateRecordVerification()+ IPC handler accept TF metrics for history reanalysiscommitVerification()passes TF metrics toupdateHistoryVerification()callStale state (renderer):
isReanalyzereset after use — prevented quality gate from showing on 2nd verification attemptpendingVerificationcleared on: wizard exit, session dismiss, session phase changeverificationPickerLogIdcleared on session dismissisReanalyzereferences use local snapshot to prevent reading stale React stateTest plan
🤖 Generated with Claude Code