-
-
Notifications
You must be signed in to change notification settings - Fork 167
feat(speakers): stop a re-diarization from silently reattributing people, and remember the review #475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(speakers): stop a re-diarization from silently reattributing people, and remember the review #475
Changes from all commits
f283b36
ff8bcc8
737ffce
6e0b862
8035d65
c3db7ae
95ac550
8f24072
43217fe
29c5c42
cbe5c25
aad6ad5
702a8dc
66b6c59
6aff5b6
b5f1c58
eafb592
978b4f4
5930f9d
b70bbcc
522fd12
e87236f
e803724
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -883,6 +883,9 @@ function install({ ipcMain }) { | |||||
| cluster.confirmed_by_user = null; | ||||||
| cluster.confirmed_person_id = null; | ||||||
| } | ||||||
| // "A human kept this generic" is superseded by "a human says it is | ||||||
| // several people" -- same transition the real CLI performs. | ||||||
| cluster.review_state = null; | ||||||
| } else if (cluster.prevSuggestion) { | ||||||
| Object.assign(cluster, cluster.prevSuggestion); | ||||||
| delete cluster.prevSuggestion; | ||||||
|
|
@@ -896,6 +899,27 @@ function install({ ipcMain }) { | |||||
| }; | ||||||
| }, | ||||||
|
|
||||||
| // Mutates the same speakerState the suggestions echo is built from, so | ||||||
| // a spec sees the marking the way the real backend delivers it: written | ||||||
| // to the sidecar, read back on the next suggest-speakers. The real CLI | ||||||
| // also clears it on a confirm and on a mixed marking (both are stronger | ||||||
| // statements about the same cluster); those transitions live in the two | ||||||
| // handlers above so the mock cannot drift from that contract. | ||||||
| 'set-cluster-review-state': async (_event, params) => { | ||||||
| const { channel, diarizationSpeakerId, generic } = params || {}; | ||||||
| const cluster = (speakerState.suggestions[channel] || {})[diarizationSpeakerId]; | ||||||
| if (!cluster) { | ||||||
| return { success: false, error: `No cluster ${diarizationSpeakerId} in ${channel}` }; | ||||||
| } | ||||||
| cluster.review_state = generic ? 'generic' : null; | ||||||
| return { | ||||||
| success: true, | ||||||
| resolved_diarization_speaker_id: diarizationSpeakerId, | ||||||
| fragment_ids: [diarizationSpeakerId], | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The mock response can misrepresent which raw clusters were updated when a row is merged, so T1 can pass while production behavior differs. Returning all fragment ids (primary plus Prompt for AI agents
Suggested change
|
||||||
| review_state: cluster.review_state, | ||||||
| }; | ||||||
| }, | ||||||
|
|
||||||
| 'speaker-naming-status': async (_event, meetingStem) => { | ||||||
| const clusters = Object.values(speakerState.suggestions).flatMap((c) => Object.values(c)); | ||||||
| const countable = clusters.filter((c) => !c.contains_multiple_speakers); | ||||||
|
|
@@ -987,6 +1011,9 @@ function install({ ipcMain }) { | |||||
| // already hold a cluster of this meeting by id, never by display | ||||||
| // name (a rename can leave two profiles reading alike). | ||||||
| confirmed_person_id: person.person_id, | ||||||
| // Naming the row supersedes "a human kept this generic" -- the real | ||||||
| // CLI clears it across the whole fragment set on every confirm. | ||||||
| review_state: null, | ||||||
| }; | ||||||
|
|
||||||
| return { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,9 +14,10 @@ import { | |
| useGetSpeakerSampleAudio, | ||
| useDeletePersonProfile, | ||
| useMarkSpeakerCluster, | ||
| useSetClusterReviewState, | ||
| meetingStemFromSummaryFile, | ||
| } from '@/hooks/useSpeakerSuggestions'; | ||
| import type { PersonProfile, SpeakerSuggestion } from '@/lib/ipc'; | ||
| import type { PersonProfile, SpeakerSuggestion, StaleAssignment } from '@/lib/ipc'; | ||
|
|
||
| interface SpeakerReviewPanelProps { | ||
| summaryFile: string; | ||
|
|
@@ -84,6 +85,68 @@ export function orderProfilesForRow<T extends { display_name: string; person_id: | |
| }); | ||
| } | ||
|
|
||
| /** Did a human look at this row and choose to leave it unnamed? | ||
| * | ||
| * Read out of the query payload rather than component state, which is the | ||
| * whole reason the backend records it: held in the component, the decision | ||
| * died on the next remount and every row the reviewer had already dealt | ||
| * with came back. | ||
| * | ||
| * Compares against the one value this build knows. A newer build writing a | ||
| * state this one has never heard of reads as "not reviewed" -- the safe | ||
| * direction, because the alternative is presenting an undo for a decision | ||
| * this build cannot describe. */ | ||
| export function isKeptGeneric(suggestion: SpeakerSuggestion): boolean { | ||
| return suggestion.review_state === 'generic'; | ||
| } | ||
|
|
||
| /** Whether the "keep generic" button belongs on this row at all. | ||
| * | ||
| * Not on a confirmed row and not on one marked as several people: both are | ||
| * decided, and parking a decided row would say two contradictory things | ||
| * about the same cluster. (Until this slice the button rendered on both, | ||
| * because it sat outside the conditional that hides the naming actions.) | ||
| * It stays on a row that is already kept generic -- that click is the undo, | ||
| * and it is the only way back. */ | ||
| export function showsKeepGenericButton(suggestion: SpeakerSuggestion): boolean { | ||
| return !suggestion.confirmed_by_user && !suggestion.contains_multiple_speakers; | ||
| } | ||
|
|
||
| /** Whether this row still offers Approve / Change / New person. | ||
| * | ||
| * Withheld on a mixed row because the backend refuses to enroll it, and on | ||
| * a row kept generic because parking a row has to mean the same thing here | ||
| * as marking one does: the reviewer said they are done with it. Leaving the | ||
| * naming actions there produced a row that said "you decided not to name | ||
| * this speaker" with three ways to name it beside the sentence, and offered | ||
| * to "Reopen" something that was never closed. Reopening is one click, and | ||
| * it brings them all back. */ | ||
| export function showsNamingActions(suggestion: SpeakerSuggestion): boolean { | ||
| return !suggestion.contains_multiple_speakers && !isKeptGeneric(suggestion); | ||
| } | ||
|
|
||
| /** The one meeting-level sentence for confirmations a re-diarization | ||
| * orphaned, or null when there are none. | ||
| * | ||
| * Deliberately says the assignments are gone rather than the people: their | ||
| * voice evidence is untouched and still scores candidates everywhere. And | ||
| * deliberately does not promise completeness -- someone whose cluster id | ||
| * the new run no longer produces cannot be listed here at all (see | ||
| * suggest-speakers, where this list is built). */ | ||
| export function staleAssignmentNotice(stale: StaleAssignment[] | undefined): string | null { | ||
| if (!stale || stale.length === 0) return null; | ||
| const names = stale.map((s) => s.display_name); | ||
| const listed = | ||
| names.length === 1 | ||
| ? names[0] | ||
| : `${names.slice(0, -1).join(', ')} and ${names[names.length - 1]}`; | ||
| return ( | ||
| `This meeting was analysed again, so the speakers were renumbered. ` | ||
| + `${listed} ${names.length === 1 ? 'is' : 'are'} no longer linked to any speaker below. ` | ||
| + `Nothing was deleted -- assign them again to restore the link.` | ||
| ); | ||
| } | ||
|
|
||
| // "mic" is always the device owner's own recording side (in-person audio); | ||
| // "system" is loopback capture of the other call participant(s) -- see | ||
| // determine_recording_type (src/speaker_suggestions.py) for the same | ||
|
|
@@ -229,8 +292,8 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan | |
| const confirmSpeaker = useConfirmSpeaker(); | ||
| const deleteProfile = useDeletePersonProfile(); | ||
| const markCluster = useMarkSpeakerCluster(); | ||
| const setReviewState = useSetClusterReviewState(); | ||
|
|
||
| const [dismissed, setDismissed] = React.useState<Set<string>>(new Set()); | ||
| const [expanded, setExpanded] = React.useState<Set<string>>(new Set()); | ||
| const [changeOpenFor, setChangeOpenFor] = React.useState<string | null>(null); | ||
| const [newPersonRow, setNewPersonRow] = React.useState<Row | null>(null); | ||
|
|
@@ -302,19 +365,23 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan | |
| const alreadyInMeeting = new Set( | ||
| rows.map((row) => row.suggestion.confirmed_person_id).filter((id): id is string => !!id), | ||
| ); | ||
| const notDismissed = rows.filter((row) => !dismissed.has(rowKey(row))); | ||
| // A row a human has explicitly marked stays in the main list even if its | ||
| // turn shape also matches the artifact heuristic -- hiding it behind | ||
| // "Show N filtered rows" would bury the undo for a deliberate action | ||
| // behind a toggle the user has no reason to open. | ||
| // behind a toggle the user has no reason to open. Same for a row kept | ||
| // generic: the marking IS a deliberate action, and its undo lives on the | ||
| // row itself. | ||
| const isFiltered = (row: Row) => | ||
| row.suggestion.is_likely_artifact && !row.suggestion.contains_multiple_speakers; | ||
| const artifactRows = notDismissed.filter(isFiltered); | ||
| const primaryRows = notDismissed.filter((row) => !isFiltered(row)); | ||
| const visibleRows = showFiltered ? notDismissed : primaryRows; | ||
| row.suggestion.is_likely_artifact | ||
| && !row.suggestion.contains_multiple_speakers | ||
| && !isKeptGeneric(row.suggestion); | ||
| const artifactRows = rows.filter(isFiltered); | ||
| const primaryRows = rows.filter((row) => !isFiltered(row)); | ||
| const visibleRows = showFiltered ? rows : primaryRows; | ||
| const recordingAvailable = suggestionsQuery.data?.recording_available ?? false; | ||
| const staleNotice = staleAssignmentNotice(suggestionsQuery.data?.stale_assignments); | ||
|
|
||
| if (!suggestionsQuery.data || notDismissed.length === 0) return null; | ||
| if (!suggestionsQuery.data || rows.length === 0) return null; | ||
|
|
||
| const duplicateProfile = newPersonName.trim() | ||
| ? (profilesQuery.data ?? []).find((p) => namesCollide(p.display_name, newPersonName)) | ||
|
|
@@ -383,6 +450,20 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan | |
| > | ||
| Speakers | ||
| </h2> | ||
| {/* Above the rows and above the speaker-count note, because it | ||
| explains why the rows below look unfamiliar: without it, a | ||
| reviewer returning to a re-analysed meeting just finds the names | ||
| they entered gone, with nothing saying why or that re-entering | ||
| them is all it takes. */} | ||
| {staleNotice && ( | ||
| <p | ||
| className="text-[11.5px]" | ||
| style={{ color: 'var(--fg-2)', margin: 0 }} | ||
| data-testid="speaker-stale-assignments" | ||
| > | ||
| {staleNotice} | ||
| </p> | ||
| )} | ||
| {minimumSpeakers > totalClusters && ( | ||
| <p | ||
| className="text-[11.5px]" | ||
|
|
@@ -405,8 +486,15 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan | |
| clickable while a confirm was still in progress. */} | ||
| {visibleRows.map((row) => { | ||
| const key = rowKey(row); | ||
| const anyConfirmPending = confirmSpeaker.isPending || markCluster.isPending; | ||
| // setReviewState belongs in this gate even though it writes only a | ||
| // marker: it is a read-modify-write of the same sidecar a confirm | ||
| // rewrites, and that pair is exactly the overlap the backend can | ||
| // narrow but not close (see _freshest_channel). Serialising the | ||
| // clicks is the half of it the UI can actually guarantee. | ||
| const anyConfirmPending = | ||
| confirmSpeaker.isPending || markCluster.isPending || setReviewState.isPending; | ||
| const isMarked = row.suggestion.contains_multiple_speakers; | ||
| const isKept = isKeptGeneric(row.suggestion); | ||
| const samples = row.suggestion.samples ?? []; | ||
| const isExpanded = expanded.has(key); | ||
| // Expanding is only worth offering when there is more than the | ||
|
|
@@ -423,7 +511,7 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan | |
| <div className="flex min-w-0 flex-col gap-0.5"> | ||
| <span | ||
| className={`text-[13.5px] ${row.suggestion.confirmed_by_user || isMarked ? 'font-medium' : ''}`} | ||
| style={{ color: isMarked ? 'var(--fg-2)' : 'var(--fg-1)' }} | ||
| style={{ color: isMarked || isKept ? 'var(--fg-2)' : 'var(--fg-1)' }} | ||
| > | ||
| {suggestionLabel(row.suggestion)} | ||
| </span> | ||
|
|
@@ -434,6 +522,14 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan | |
| <span className="text-[11.5px]" style={{ color: 'var(--fg-2)' }}> | ||
| Left out of naming and voice recognition. | ||
| </span> | ||
| ) : isKept ? ( | ||
| <span | ||
| className="text-[11.5px]" | ||
| style={{ color: 'var(--fg-2)' }} | ||
| data-testid={`speaker-kept-generic-${key}`} | ||
| > | ||
| Kept generic — you decided not to name this speaker. | ||
| </span> | ||
| ) : ( | ||
| row.suggestion.sample_text && ( | ||
| <span | ||
|
|
@@ -500,7 +596,7 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan | |
| become available, and a "Change" picker would be an | ||
| invitation to do the one thing this marking exists to | ||
| prevent. The undo below is what stays reachable. */} | ||
| {!isMarked && ( | ||
| {showsNamingActions(row.suggestion) && ( | ||
| <> | ||
| {/* Hidden once confirmed_by_user is set -- re-approving an | ||
| already-confirmed cluster is a no-op that changes | ||
|
|
@@ -638,17 +734,34 @@ export function SpeakerReviewPanel({ summaryFile, isDiarised }: SpeakerReviewPan | |
| {isMarked ? <Undo2 className="size-[13px]" /> : <Users className="size-[13px]" />} | ||
| {isMarked ? 'One person' : null} | ||
| </Button> | ||
| <Button | ||
| size="sm" | ||
| variant="ghost" | ||
| aria-label="Keep generic label" | ||
| title="Keep generic label" | ||
| disabled={anyConfirmPending} | ||
| onClick={() => setDismissed((prev) => new Set(prev).add(key))} | ||
| data-testid={`speaker-keep-generic-${key}`} | ||
| > | ||
| <X className="size-[13px]" /> | ||
| </Button> | ||
| {/* The row stays put and reads as parked, rather than | ||
| vanishing. Hiding it was fine while the decision lived | ||
| for one session; now that it is written down, a hidden | ||
| row would put its undo somewhere nobody can reach -- | ||
| and a reviewer coming back tomorrow would have no way to | ||
| tell "I decided to leave this" from "this never | ||
| appeared". */} | ||
| {showsKeepGenericButton(row.suggestion) && ( | ||
| <Button | ||
| size="sm" | ||
| variant={isKept ? 'outline' : 'ghost'} | ||
| aria-label={isKept ? 'Reopen this speaker for naming' : 'Keep generic label'} | ||
| title={isKept ? 'Reopen this speaker for naming' : 'Keep generic label'} | ||
| disabled={anyConfirmPending} | ||
| onClick={() => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The "Keep generic"/"Reopen" mutation has no error feedback, unlike Approve/Change/New person and the multi-speaker toggle on the same row. When the backend refuses the write (for example the cluster no longer exists in the channel, or the sidecar is missing), the rejection is swallowed silently and the click looks like it did nothing, while every other action on the row surfaces a red "Couldn't ..." message. Consider handling the mutation's onError the same way confirm and setMultiSpeaker do, writing to the feedback map (and teaching its message mapping to describe the failed keep-generic/reopen operation), so a failed marking is never mistaken for a successful one. Prompt for AI agents |
||
| setReviewState.mutate({ | ||
| meetingStem, | ||
| channel: row.channel, | ||
| diarizationSpeakerId: row.diarizationSpeakerId, | ||
| generic: !isKept, | ||
| }) | ||
| } | ||
| data-testid={`speaker-keep-generic-${key}`} | ||
| > | ||
| {isKept ? <Undo2 className="size-[13px]" /> : <X className="size-[13px]" />} | ||
| {isKept ? 'Reopen' : null} | ||
| </Button> | ||
| )} | ||
| </div> | ||
| </div> | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: A merged row can remain partially "kept generic" in T1 after marking it mixed because only one raw id is cleared. Clearing
review_statefor the selected id and itsmerged_fromids would mirror real behavior and avoid mock drift.Prompt for AI agents