You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Summary: Clean extraction of speaker name selection logic from SpeakerRowView into a testable SpeakerNameSelectionPolicy enum, plus enabling NSComboBox inline autocomplete. The policy is stateless (caseless enum with static functions), well-tested, and the UI integration is minimal — one flag flip (completes = true) and delegate method wiring. Net -81 lines from the view, +129 in a focused policy file with +60 in tests.
Blockers: None.
Should fix:
Subtle behavior change in duplicate detection: The old makeIdentityLabels grouped duplicates by $0.displayName.lowercased(). The new version uses normalizedSearchText(displayName($0)), which adds .diacriticInsensitive folding. This means "José García" and "Jose Garcia" would now be treated as duplicates and get disambiguation labels (with call count + UUID prefix), where previously they'd be treated as distinct. This is arguably better behavior, but it's a silent change worth being aware of — confirm this is intentional.
"You" sentinel not in lookup: The "You" label is prepended to knownPeopleLabels (line 445) but never added to knownPeopleByLabel. If a user manually types "You" (without the batch "Keep as You" toggle), knownPeopleOption(matching: "You") returns nil, so buildUpdate() emits a .named action with newName: "You" rather than a .collapsedToMe. Verify the downstream SpeakerNamingCoordinator handles a .named update with newName: "You" gracefully — if it does, this is fine; if it expects .collapsedToMe for that case, it could create an orphan profile.
Nits:
Every call site and test case repeats the same id: { $0.id }, displayName: { $0.displayName }, callCount: { $0.callCount } closure triple. A test helper or a lightweight protocol conformance could cut the boilerplate, but the generic approach is reasonable for keeping the policy type-agnostic.
normalizedSearchText calls .folding(options: .caseInsensitive) then .lowercased(). The .lowercased() is mostly redundant after case-insensitive folding, though it serves as a safe normalization guarantee across locales — fine to keep.
visibleKnownPeopleLabels() is recomputed on each NSComboBoxDataSource callback (numberOfItems, objectValueForItemAt, indexOfItemWithStringValue), so the same sort runs 3+ times per keystroke. At N=2-20 speakers this is negligible, but a per-edit-cycle cache would be a cleanliness win if the list ever grows.
What looks good:
The completedLabel logic correctly requires exactly one unambiguous prefix match before completing — no surprising auto-completions when multiple names share a prefix.
Stateless enum with pure static functions is the right design here: no threading concerns, trivially testable, and the @MainActor isolation stays where it belongs (in SpeakerRowView).
Good test coverage: unique prefix match, ambiguous prefix refusal, and duplicate-name disambiguation all exercised.
The match ranking (exact → prefix → word-start → label-prefix → substring → label-substring) with call-count tiebreaking is well-thought-out and produces intuitive sort order.
The extraction is faithful — the logic in SpeakerNameSelectionPolicy matches the old inline implementations with no silent behavioral drift (aside from the diacritic folding note above).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
User feedback: typing an existing speaker name should auto-complete/select it instead of requiring a dropdown click.