Skip to content

[MC-458-K] fix(frontend): read concept notes by the concept uid, not member csv ids#649

Merged
TrueNorth49 merged 2 commits into
mainfrom
mc-458-k-notes-uid
Jun 3, 2026
Merged

[MC-458-K] fix(frontend): read concept notes by the concept uid, not member csv ids#649
TrueNorth49 merged 2 commits into
mainfrom
mc-458-k-notes-uid

Conversation

@TrueNorth49

Copy link
Copy Markdown
Collaborator

Problem

Compare's per-concept note box resolves its text via conceptNoteKeys, which returned the concept's underlying member csv ids (e.g. ["311"] for bird, ["328"] for cloud). But MC-458-E uid-migrated concept_notes (it's in ROOT_CONCEPT_KEYED_BLOCKS), so the server serves notes keyed by the concept identity uid (c-311, c-328). The box looked up the member id, found nothing, and fell back to the stale localStorage cache — so a note showed under the wrong concept (cold under cloud) or appeared blank (bird), even though concept_notes on the server was correct (verified: 0 mis-keyed).

This is the notes-equivalent of MC-458-J (the cognate keying fix): an MC-458 reader still using csv-id/member keys instead of the concept uid.

Fix

Add noteKeysForConcept(concept) (in speakerElicitedConcepts.ts) that leads with concept.key (the uid) and keeps the member csv ids after it as a legacy read-fallback and for merged-member save-sync. ConceptNotesBox/resolveServerNote returns the first non-empty match, so it now finds the uid-keyed note; concept.key-first means the current note wins over any stale member-id/localStorage entry. ParseUI uses the helper instead of the inline "variant csv ids only" form.

conceptUnderlyingKeys is intentionally left unchanged — it omits concept.key for grouped concepts to avoid concept_id collisions in concept_id-keyed lookups (sort keys, elicited filters). The note store is uid-keyed, so that concern doesn't apply; hence a dedicated helper rather than perturbing the shared one.

No data change: concept_notes is already correctly uid-keyed; this makes the reader use that key.

Tests

  • noteKeysForConcept — uid-first for identity/grouped concepts, all members included, mergedKeys, singleton, dedupe.
  • ConceptNotesBox — a uid-keyed note (c-328) resolves and is preferred over a stale member-id (328) localStorage entry.
  • npx vitest run1205 passed, 9 skipped, 0 failed. tsc --noEmit clean. npm run build clean.

Scope / follow-ups

Converges the note reader onto the uid. Same broader convergence noted on #648 applies (single key-mint chokepoint, etc.). A "Reload identity/notes from disk" affordance (raised earlier) would remove the restart needed when notes are repaired out-of-band.

🤖 Generated with Claude Code

…member csv ids

Compare's concept-note box resolves the note via conceptNoteKeys, which returned
the underlying member csv ids (e.g. ["311"], ["328"]). But MC-458-E uid-migrated
concept_notes (it is in ROOT_CONCEPT_KEYED_BLOCKS), so the server serves notes
keyed by the concept identity uid (c-311, c-328). The box looked up the member id,
found nothing, and fell back to the stale localStorage cache — so notes showed
under the wrong concept (cold under cloud) or blank (bird), even though the data
was correct.

Fix: add noteKeysForConcept(concept) which leads with concept.key (the uid) and
keeps the member csv ids after it as a legacy read-fallback / merged-member sync.
ConceptNotesBox/resolveServerNote returns the first non-empty match, so it now
finds the uid-keyed note. This is the notes-equivalent of the MC-458-J cognate
keying fix.

conceptUnderlyingKeys is unchanged: it intentionally omits concept.key for
grouped concepts to avoid concept_id-collision in concept_id-keyed lookups; the
note store is uid-keyed so that concern does not apply here.

Tests: noteKeysForConcept (uid-first, members, merged, dedupe) + a ConceptNotesBox
case proving a uid-keyed note resolves over a stale member-id localStorage entry.
Full vitest 1205 passed; tsc --noEmit clean; build clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@TrueNorth49 TrueNorth49 added bugfix Bug fix MC-458 Unify Compare/Annotate grouping on concept_identity labels Jun 3, 2026
@TrueNorth49

Copy link
Copy Markdown
Collaborator Author

Mandatory PR Review Checklist — PR #649 (MC-458-K)

Per pr-review-checklist.md.

1. Scope Confirmation & Sufficiency

  • git diff origin/main...HEAD --stat: 4 files, +77/−5 — speakerElicitedConcepts.ts (+23, new helper), ParseUI.tsx (use helper, +6/−5), noteKeysForConcept.test.ts (+38), ConceptNotesBox.test.tsx (+10).
  • PR claim: make the concept-note reader key on the uid. Diff matches: adds noteKeysForConcept (uid-first) and points conceptNoteKeys at it; no other behavior touched.
  • Sufficient: fixes the root cause (reader used member csv ids; server serves uid-keyed notes). The data is already correct, so this is the complete display fix. Verdict: scope matched & sufficient.

2. Test Execution

  • npx vitest run136 files, 1205 passed / 9 skipped / 0 failed (was 1198; +7 new). 0 new failures.
  • tsc --noEmit clean; npm run build ✓ built (pre-existing chunk-size notice only).
  • Backend pytest / ruff: N/A — no python/ files changed.
  • Coverage of changed lines: noteKeysForConcept unit-tested (uid-first, members, mergedKeys, singleton, dedupe); ConceptNotesBox test proves a uid-keyed note (c-328) resolves over a stale member-id (328) localStorage entry — i.e. it would fail under the old behavior.

3. Critical Systems Impact & Regression Check

  • Changed files: a pure lib helper + its single call site + two test files. No stores, save/load, waveform, contracts, or server files.
  • Annotation persistence / waveform / tiers — NO REGRESSION (untouched).
  • Concept-notes read/write (UI state) — CORRECTLY EXTENDED. Note save still writes to every key in conceptNoteKeys; now those lead with the uid (+ member ids), so writes land on the uid (matching the served store) and stay back-compatible. ConceptNotesBox.test.tsx (11) green.
  • conceptUnderlyingKeys — NO REGRESSION. Left unchanged; still omits concept.key for grouped concepts (its 14 non-test call sites — sort keys, elicited filters — rely on that to avoid concept_id collisions). The new helper is separate precisely so that contract is untouched.

4. Interactive Surface Verification

  • The concept-note textarea (ConceptNotesBox) is the only affected control. Read resolves via resolveServerNote(data, conceptNoteKeys) → now finds the uid-keyed note; save (useEnrichmentStore.save) writes the patch across the same keys. Status: correctly modified. No buttons/handlers changed. ConceptNotesBox.test.tsx exercises read + save + localStorage paths.

5. Side-Effect, Dependency & Performance

  • Only consumer of noteKeysForConcept: ParseUI.tsx:1316. Pure function (Set-dedup over ≤ a few keys); no perf impact, no hooks/deps changes, no shared-state mutation.
  • Save now also writes member-id keys; on next load the uid migration promotes those to the uid (same note) — redundant but harmless, and keeps legacy/merged members in sync. No data loss.

6. CI Gates & Merge Hygiene

  • gh pr checks 649: Backend CI pass · Frontend CI pass · Schema Compatibility pass · Parity Diff Harness pass (4/4).
  • Base main; mergeable: MERGEABLE; mergeStateStatus: BLOCKED = awaiting required human approval (not a failing gate). Worktree clean.

7. Overall Integrity Verdict

SAFE TO MERGE (pending human approval — agents don't merge).

  • Scope matched; tests 1205/0; no critical-system regressions; note-box read/write correctly converged to the uid.
  • After merge: the running app (restart picks up the merged FE) + a refresh will show each concept's own note (the concept_notes data is already correctly uid-keyed). The stale localStorage no longer masks anything once the uid note resolves.
  • Follow-ups: the broader MC-458 convergence (single key-mint chokepoint) and a "reload notes/identity from disk" affordance (to avoid restarts when data is repaired out-of-band).

8. Communication, Language & Docs Hygiene

  • 8.1/8.2: title, body, comments describe the generic mechanism (uid vs member-id note keying); no language/speaker/corpus framing; plain prose. No edit needed.
  • 8.3 docs: the keying rule is documented in the helper docstring + the call-site comment (in-diff). The user-facing persistence doc (docs/data-persistence-model.md) already describes concept_notes; the note-reader-keys-on-uid detail and the operational repair live in the parse-restore-concept-notes skill (updated alongside, outside the repo since .claude/ is gitignored). Docs verdict: present & compliant.

The retained comment still said "merged → underlying member ids", which
contradicts the fix. Replaced with the uid-first description.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@TrueNorth49

Copy link
Copy Markdown
Collaborator Author

Code review — PR #649 (review skill)

Overview

Fixes the concept-note box reading notes under the wrong key. concept_notes is uid-keyed post MC-458-E (server uid-migrates it), but conceptNoteKeys fed ConceptNotesBox the member csv ids, so the lookup missed and fell back to stale localStorage (cold-under-cloud, blank bird). Adds noteKeysForConcept(concept) (uid-first, members as legacy fallback) and uses it. +77/−5 across 4 files (helper + call site + 2 test files); the data is unchanged (already correct).

Correctness

  • resolveServerNote returns the first non-empty match, so leading with concept.key (uid) makes the current uid-keyed note win — including over a stale member-id/localStorage entry. ✓
  • noteKeysForConcept Set-dedups, trims, and includes mergedKeys + variants + mergedVariants, mirroring conceptUnderlyingKeys's sources but adding concept.key. ✓
  • conceptUnderlyingKeys correctly left untouched (it omits concept.key to avoid concept_id collisions in sort/elicited lookups — a different contract). Good call splitting into a separate helper rather than overloading it.

Conventions / quality

  • Matches the MC-458-J pattern (reader converged onto the uid). Helper is pure, documented, and unit-tested in isolation — the right altitude.
  • Applied during review (§8 corrective): the retained comment above conceptNoteKeys still read "merged → underlying member ids", contradicting the fix — rewritten to the uid-first description (commit 90408ab).

Test coverage

  • noteKeysForConcept: uid-first, multi-member, mergedKeys, singleton, dedupe.
  • ConceptNotesBox: a uid-keyed note (c-328) resolves over a stale member-id (328) localStorage entry — fails under the old behavior, so it's a real regression guard.
  • Full vitest 1205 passed / 0 failed; tsc --noEmit clean; npm run build clean; CI 4/4 green.

Performance / security

  • Pure function over ≤ a few keys; no perf impact. No security surface (client-side keying only).

Risks / suggestions

  • Low risk. Minor: save now also writes member-id keys (then re-promoted to uid on next load) — redundant but harmless and keeps legacy/merged members synced; acceptable.
  • Broader follow-up (not this PR): this is the 3rd reader stranded on csv-id keys post-uid-migration — a single key-mint chokepoint + a "every decision key is a uid" guard test would close the class.

Verdict: approve / safe to merge (pending required human approval; not merging). This complements the mandatory 8-section checklist review posted above.

@TrueNorth49 TrueNorth49 merged commit 08d9f27 into main Jun 3, 2026
4 checks passed
@TrueNorth49 TrueNorth49 deleted the mc-458-k-notes-uid branch June 3, 2026 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Bug fix MC-458 Unify Compare/Annotate grouping on concept_identity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant