Skip to content

fix(ui): finish confidence-band unification + close estimatePlr audit item - #60

Merged
slittycode merged 2 commits into
mainfrom
claude/review-main-post-merge-1gq4F
May 17, 2026
Merged

fix(ui): finish confidence-band unification + close estimatePlr audit item#60
slittycode merged 2 commits into
mainfrom
claude/review-main-post-merge-1gq4F

Conversation

@slittycode

Copy link
Copy Markdown
Owner

Post-merge review of bc79247 (the #54+#55+#56+#58 fix stack) surfaced two follow-up items. This PR closes both.

1. Confidence-band unification (audit #56 follow-up)

apps/ui/src/components/analysisResultsViewModel.ts carried a second source of truth for the canonical band thresholds defined in services/sessionMusician/confidenceBand.ts:

  • export type ConfidenceLevel = "High" | "Moderate" | "Low"
  • Two hardcoded confidence >= 0.8 ? "High" : confidence >= 0.5 ? "Moderate" : "Low" ternaries in buildMelodyInsights (transcription + melody paths)
  • confidenceLabel: ConfidenceLevel field on MelodyInsightsViewModel

The original merge deferred this rollout deliberately ("once producers see the new bands in context"). Now that bands ship on chips, badges, and citation pills across the same surface, the two melody-row renderers migrate too:

  • Sonic Element melody row: "High (78%)""Solid scaffold (78%)"
  • Melody Patch parameter row: "78% (High)""78% (Solid scaffold)"

Both now compute the label via getConfidenceBand(confidence).label. Threshold definition lives in one place.

Test added: positive assertion that averageConfidence: 0.83 lands on the Solid scaffold band via getConfidenceBand — guards against silent threshold drift.

2. estimatePlr boundary review (audit 2026-05-16 §4)

The 2026-05-16 nightly audit flagged estimatePlr in apps/ui/src/services/mixDoctor.ts as "needs human review" — the truePeak − lufsIntegrated fallback when phase1.plr is missing is technically a re-computation of a Phase 1 schema value.

Decision: keep the fallback, document why. Verified that:

  • Backend's analyze_plr in apps/backend/analyze_core.py:624-635 computes round(true_peak − lufs_integrated, 2) — byte-identical to the UI fallback
  • Backend emits plr in both --fast (analyze.py:1385) and full (analyze.py:1775) paths
  • Fallback fires only when phase1.plr is null/undefined (legacy snapshots replayed from runtime SQLite that predate the field) and lands on the same number the backend would have
  • Result is consumed locally for advisory scoring; never written back into the Phase 1 snapshot

Documented inline above estimatePlr so the next nightly audit doesn't re-flag, and added a Resolution subsection to audits/nightly-2026-05-16.md pointing back to the comment.

Verification

  • npm run lint --prefix apps/ui → clean (tsc --noEmit)
  • npm run test:unit --prefix apps/ui603/603 pass (14.75s)
  • npm run build --prefix apps/ui → green (4.26s)
  • Backend tests (run pre-change): 654/654 pass

Test plan

  • tsc passes
  • vitest unit suite passes
  • vite build passes
  • new positive test asserts band label for known confidence value
  • (optional) Playwright smoke if reviewer wants extra coverage on rendered text

Diff size

4 files changed, 26 insertions(+), 16 deletions(-)


Generated by Claude Code

… item

Two post-merge follow-ups from the review of bc79247:

1. Confidence-band unification (audit #56 follow-up): the three-level
   `ConfidenceLevel` enum ("High" | "Moderate" | "Low") and its two
   hardcoded `>= 0.8 ? : >= 0.5 ?` ternaries in `buildMelodyInsights`
   were a second source of truth for the canonical band thresholds in
   `services/sessionMusician/confidenceBand.ts`. The original merge
   deferred this rollout deliberately ("once producers see the new
   bands in context"); now that bands ship on chips, badges, and
   citation pills, the melody Sonic Element row and Melody Patch
   parameter row migrate too. They now render canonical band labels
   ("Solid scaffold (78%)", etc.) so the threshold definition lives in
   one place only.

   Drops: `ConfidenceLevel` type, `confidenceLabel` field on
   `MelodyInsightsViewModel`, two ternary literals. Adds: positive
   test that `averageConfidence: 0.83` lands on the `Solid scaffold`
   band via `getConfidenceBand`.

2. `estimatePlr` boundary review (audit 2026-05-16 §4): documented why
   the `truePeak − lufsIntegrated` fallback in
   `apps/ui/src/services/mixDoctor.ts` is not a competing source of
   truth. Backend's `analyze_plr` computes the same value byte-for-byte
   and emits `plr` in both `--fast` and full paths; fallback only fires
   on legacy snapshots that predate the field, and lands on the same
   number the backend would have. Inline comment + audit-doc resolution
   subsection so the next nightly does not re-flag it.

Verified: tsc clean, 603/603 vitest pass, build green (4.26s).

@slittycode slittycode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE

Summary

Closes two deferred items from the audit stack: removes the duplicate three-level confidence vocabulary from analysisResultsViewModel.ts in favor of the canonical four-band ladder in confidenceBand.ts, and documents the estimatePlr fallback so the nightly audit doesn't keep re-flagging a deliberate design decision. Both items are handled cleanly. Tests pass, tsc is clean.

Findings

No blocking or should-fix findings.

Worth considering (not blocking): The new test at analysisResultsViewModel.test.ts:753 calls getConfidenceBand directly rather than asserting the rendered string ("Solid scaffold (83%)") that appears in getSonicMeasurements and buildMelodyPatchParameters. It's a good threshold-drift guard, but it doesn't cover the actual string-interpolation call sites added in this PR. The risk is low — trivial formatting — but a future regression in either of those two .label usages would go unnoticed until manual QA. Acceptable as-is given the diff size.

Test results

603 / 603 pass (run locally, 14.82s). tsc --noEmit clean. Both match what the PR description claims.

Phase boundary check

Clean.

estimatePlr is correctly documented and correctly scoped: it only fires on legacy snapshots predating the plr field, it re-derives the exact value analyze_plr would have produced (round(true_peak − lufs_integrated, 2)), and its output is consumed locally for advisory scoring — never written back into the Phase 1 snapshot. The backend line references in the comment (analyze_core.py:624-635, analyze.py:1385, analyze.py:1775) all check out.

ConfidenceLevel and confidenceLabel were UI-layer constructs, not part of the Phase 1 measurement schema. Their removal is a view-model cleanup, not a contract change.


Generated by Claude Code

PR #60 review noted that the threshold-drift guard didn't cover the
actual string-interpolation sites added by the migration. Adds two
focused tests:

1. buildSonicElementCards (transcription path) → asserts that the
   melodicArp card's "Transcription" measurement renders
   "Solid scaffold (83%)" — exercises the Sonic Element row template
   at analysisResultsViewModel.ts:572.

2. buildPatchCards (MIDI Clip Guide synthetic fallback) → asserts
   that the melody-confidence parameter renders
   "72% (Workable draft)" — exercises the patch-parameter row
   template at analysisResultsViewModel.ts:1099.

A future regression in either `.label` interpolation now fails a
specific test rather than waiting for manual QA.

605/605 pass (was 603).
@slittycode
slittycode merged commit adb715e into main May 17, 2026
2 checks passed
@slittycode
slittycode deleted the claude/review-main-post-merge-1gq4F branch May 17, 2026 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants