Add live duplicate check to the new-entry dialog - #2433
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Spec-cited review against EntrySearchService/SqlHelpers found the client fold diverged: host-locale lowercasing, unconditional accent-stripping (backend keeps diacritics significant when the query has them), and no morph-token handling (typed "-aji" vs suffix entry "aji" missed exact). Also ranks each search by the writing system it was typed in, fixes the mount-time "Checking" flash, singularizes the one-match banner, and closes a pre-existing double-Enter double-create in the dialog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- match rows expand to reveal Add sense / Go to entry; DictionaryEntry gains an inline mode for the compact collapsed rows - classification: attribute same-word matches to the field that hit (Same headword vs Same lexeme form), rank similar words closest-in-length first, drop cross-field coincidences instead of a vague 'related' kind - classify in a $derived so the lazy morph-types resource warms at mount and re-classifies without re-searching - dedupe queries per field kind: the same text typed as lexeme and gloss keeps its gloss query (same-meaning matches were lost) - share the banner message/tint/Enter-trap between the strip and the extracted DuplicateSummaryPill - scope backend-parity doc claims to the CRDT FTS path; drop unverified FLEx attribution - adapt the Playwright suite to the new interaction model and cover the jump pill (7 tests); pin length-delta boundary, gloss containment direction, and closest-form ranking in unit tests (33; 8/8 targeted mutants killed) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…icates # Conflicts: # frontend/viewer/src/locales/en.po # frontend/viewer/src/locales/es.po # frontend/viewer/src/locales/fr.po # frontend/viewer/src/locales/id.po # frontend/viewer/src/locales/ko.po # frontend/viewer/src/locales/ms.po # frontend/viewer/src/locales/sw.po # frontend/viewer/src/locales/vi.po
…etection-review-ny053c
…fill test gaps - Move new-entry-duplicates.test.ts to tests/ui/ and port it to the DemoProjectPage page object introduced by the E2E restructure (#1866) - Cap the jump pill at 32rem so long headword previews don't stretch it across the whole dialog - New tests: trapEnter unit test, Enter-inside-strip must not create the entry, pill dismiss persists, add-sense must not also create an entry Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R49zrCE7hPgm9QJ5bfugaV
A failed searchEntries call previously vanished into the unread resource.error — no toast, no status line, the widget just went blank. Show an inline status line instead of a toast: the search re-fires per typing pause, and a toast per failure would bury the dialog. Show-more now uses $plural so translators get per-language plural forms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R49zrCE7hPgm9QJ5bfugaV
This reverts commit d267bd1.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesDuplicate entry detection
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
…etection-review-ny053c
…ntry-detection-sync-1bcb1f # Conflicts: # frontend/viewer/src/locales/en.po # frontend/viewer/src/locales/es.po # frontend/viewer/src/locales/fr.po # frontend/viewer/src/locales/id.po # frontend/viewer/src/locales/ko.po # frontend/viewer/src/locales/ms.po # frontend/viewer/src/locales/sw.po # frontend/viewer/src/locales/vi.po
Always send the backend search with diacritics stripped (it only matches accent-insensitively for a diacritic-free query), then classify an exact diacritic match as same-word and an accent-only difference as similar-word. Classification now runs against the displayed headword from the writing-system service, dropping the client-side morph-token stripping. Extract the duplicate pill/widget wiring into DuplicateCheckSection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The entries for "Meaning added to {0}" and "This entry may already exist" were extracted without a msgstr line in all seven non-English locales, leaving the catalogs malformed. Add the empty msgstr so they parse as normal untranslated entries.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
expand() sets userToggled=true, which recomputed the `expanded` $derived back to (hasExactWordMatch && !userToggled) = false — so clicking the jump pill closed the strip it was meant to open. Restore `expanded` to $state with an effect for the exact-match auto-open. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0940483 to
e11f04a
Compare
The empty-input test in the normalizeForExactCompare block called normalizeForLooseCompare, so exact-compare's handling of a missing value was never covered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/viewer/src/lib/entry-editor/duplicate-check/duplicate-check.ts (1)
82-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant trim/coalesce.
normalizeForExactComparealready trims and always returns a string, so.trim() ?? ''on line 83 is dead. Minor cleanup.♻️ Proposed tidy
- const decomposed = normalizeForExactCompare(value).trim() ?? ''; - return decomposed.toLowerCase() + return normalizeForExactCompare(value).toLowerCase() // remove diacritics .replace(/\p{Mn}/gu, '');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/viewer/src/lib/entry-editor/duplicate-check/duplicate-check.ts` around lines 82 - 91, Remove the redundant `.trim() ?? ''` from `normalizeForLooseCompare`; use the string returned by `normalizeForExactCompare(value)` directly before lowercasing and removing diacritics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/viewer/src/lib/entry-editor/duplicate-check/duplicate-check.ts`:
- Around line 127-133: Update the docstring for classifyDuplicateCheckResults to
remove the unsupported claim that similar words are ordered closest-in-length
first. Document only the ordering guarantees implemented by the kindRank sort
and preserved candidate search-relevance order.
---
Nitpick comments:
In `@frontend/viewer/src/lib/entry-editor/duplicate-check/duplicate-check.ts`:
- Around line 82-91: Remove the redundant `.trim() ?? ''` from
`normalizeForLooseCompare`; use the string returned by
`normalizeForExactCompare(value)` directly before lowercasing and removing
diacritics.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7b7390db-8a03-4492-a355-b942ad36a7c3
📒 Files selected for processing (18)
frontend/viewer/src/lib/activity/ActivityItemChangePreview.sveltefrontend/viewer/src/lib/components/dictionary/DictionaryEntry.sveltefrontend/viewer/src/lib/entry-editor/EntryOrSenseItemList.sveltefrontend/viewer/src/lib/entry-editor/NewEntryDialog.sveltefrontend/viewer/src/lib/entry-editor/duplicate-check/DuplicateCheck.sveltefrontend/viewer/src/lib/entry-editor/duplicate-check/DuplicateCheckSection.sveltefrontend/viewer/src/lib/entry-editor/duplicate-check/DuplicateSummaryPill.sveltefrontend/viewer/src/lib/entry-editor/duplicate-check/duplicate-check.test.tsfrontend/viewer/src/lib/entry-editor/duplicate-check/duplicate-check.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.pofrontend/viewer/tests/ui/new-entry-duplicates.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
Probably an edge case so feel free to ignore it, but I did find this a little janky Video.mp4 |
|
I couldn't help myself Video.mp4 |
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
I left some feedback, I'm happy to merge it, but there's a few things that could be improved.
I also made some minor tweaks myself. Mostly around the pill, I added a threshold for triggering the intersection check, this means that when only 50% of the check area is showing, then the pill will show or hide, previously it would only trigger on 100%, however it is a bit weird now if the duplicate check is open, then the pill shows up too early possibly. I'll leave it and you can check it out and revert it if you don't like it.
| analysis: string[]; | ||
| } | ||
|
|
||
| export function duplicateResultContainerClass(hasExactWordMatch: boolean): string { |
There was a problem hiding this comment.
this seems odd to be here instead of in the svelte file
There was a problem hiding this comment.
[Claude, autonomous]
Moved into DuplicateCheck.svelte's module <script>, since it's only used by this component.
| // the cap is min(shorter length, MAX_SIMILAR_LENGTH_DELTA=3): a short query grows by at most its | ||
| // own length, a long one by at most 3. | ||
| // (Typing "uz" must not surface every word containing it.) | ||
| const shortAtCap = makeEntry({lexemeForm: {seh: 'uzem'}}); // 'uz' + 2 = delta 2, kept |
There was a problem hiding this comment.
I'm not sure I understand this enough, the comment says 'uz' + 2 = delta 2... what does that mean? this is saying it won't match words more than 2 chars longer than the search term, if it's 3 longer than it doesn't count as a match. In that case though we should probably change the name or something, since the max similar is actually 2, not 3. From what I can tell the code is working correctly but it is confusing. It might help to pull out a utility and test that with very simple inputs (eg isSimilar('abc', 'abcdef')
There was a problem hiding this comment.
[Claude, autonomous]
Pulled the logic into an exported isSimilarWord(a, b) and deleted the confusing length-delta test. It's now tested directly with trivial inputs — isSimilarWord('abc', 'abcdef'), the short-word case ('a'/'an'/'and'), etc.
On the name: MAX_SIMILAR_LENGTH_DELTA (3) is the upper bound for long words; short words get a smaller allowance equal to their own length, so 'uz' effectively caps at a delta of 2. That's now documented on the function and pinned by tests rather than a comment.
|
|
||
| it('matches the bare lexeme of a suffix as a lexeme hit, not a headword hit', () => { | ||
| // typing 'aji' (no token) against suffix entry '-aji': the lexeme matched, the headword didn't | ||
| const suffixEntry = makeEntry({lexemeForm: {seh: 'aji'}, morphType: MorphTypeKind.Suffix}); |
There was a problem hiding this comment.
would it not match citation form '-aji' like in the previous test?
There was a problem hiding this comment.
It would, but that'd just be a "similar" match. This test is documenting the fact that we report/find an exact match on lexeme-form even though the text is not an exact match on headword.
| // citation form — the citation hit must win the field attribution | ||
| const entry = makeEntry({lexemeForm: {seh: 'fuz'}, citationForm: {seh: 'fuza'}}); | ||
| const result = classifyDuplicateCheckResults([entry], vernQueries('fuz', 'fuza'), writingSystems); | ||
| expect(result[0]).toEqual({entry, kind: 'same-word', field: 'headword'}); |
There was a problem hiding this comment.
from this test it sounds like field should just be citation form then.
There was a problem hiding this comment.
What about calculated headwords that use the lexeme-form?
- headword and lexeme-form cover everything.
- citation-form and lexeme-form do not.
| export type SameWordField = 'headword' | 'lexeme'; | ||
|
|
||
| /** Minimal slice of WritingSystemsService, so unit-testable. */ | ||
| export interface DuplicateCheckWritingSystems { |
There was a problem hiding this comment.
can we change this so that it's based on the real class? Eg Pick<WritingSystemService, 'vernacularNoAudio' | 'analysisNoAudio' | 'headword'>? One Goto will work in the editor, and 2 if there's a change to a signature then it will break inside this file, and not where the functions exported from this class are called.
There was a problem hiding this comment.
[Claude, autonomous]
Done, it's now Pick<WritingSystemService, 'vernacularNoAudio' | 'analysisNoAudio' | 'headword'>, so goto-definition lands on the real service and a signature change breaks here.
| it('drops a candidate whose gloss equals a typed vernacular value (cross-field coincidence)', () => { | ||
| // typing lexeme 'nyumba' must not surface an entry merely because its gloss is 'nyumba' | ||
| const entry = withGloss('cabana', 'nyumba'); | ||
| expect(classifyDuplicateCheckResults([entry], vernQueries('nyumba'), writingSystems)).toEqual([]); |
There was a problem hiding this comment.
this test would probably be more clear if you defined 2 entries, used one for the search and one to generate the query via getDuplicateCheckQueries. In fact I put this comment on the wrong line at first because this test is so similar to the previous one
There was a problem hiding this comment.
[Claude, autonomous]
Reworked exactly that way: the test now builds a typed entry, runs it through getDuplicateCheckQueries, then checks a separate candidate entry, so it no longer mirrors the previous test.
| } | ||
| </script> | ||
|
|
||
| <div class="min-h-9 flex flex-col justify-center w-full" aria-live="polite"> |
There was a problem hiding this comment.
I assume we're always rendering this to avoid layout thrashing?
There was a problem hiding this comment.
[Claude, autonomous]
Yes. The outer min-h-9 shell is always rendered so the strip toggling in/out doesn't shift the form. Added a comment saying so.
| {pt($t`Could not check for similar entries`, $t`Could not check for similar words`, viewService.currentView)} | ||
| {:else} | ||
| <Icon icon="i-mdi-check-circle-outline" class="size-4 text-green-600 dark:text-green-500" /> | ||
| {pt($t`No similar entries found`, $t`Looks like a new word`, viewService.currentView)} |
There was a problem hiding this comment.
these texts are very different between lite and FLEx was that intentional?
There was a problem hiding this comment.
[Claude, autonomous]
Mostly drift, not intentional. Aligned the lite "success" copy to No similar words found to match the FLEx No similar entries found. The error/loading copy stays view-specific (words vs entries) on purpose via pt(...).
| <div class="grow min-w-0 text-sm {isExpanded ? '' : 'line-clamp-1'}"> | ||
| <DictionaryEntry entry={match.entry} inline={!isExpanded} hideExamples={!isExpanded} /> | ||
| </div> | ||
| {#if badge} |
There was a problem hiding this comment.
I think this is always true
There was a problem hiding this comment.
[Claude, autonomous]
You're right, kindLabel returns a string for every kind, so it was always truthy. Removed the {#if badge} and the badge now renders unconditionally.
- Type DuplicateCheckWritingSystems as a Pick of the real WritingSystemService
- Move duplicateResultContainerClass into DuplicateCheck.svelte's module script
- Drop auto-expand on exact match (form jumped while typing) and slide the box in/out
- Remove always-true {#if badge}; align lite copy to "No similar words found"
- Test isSimilarWord directly with trivial inputs; clarify suffix/lexeme-attribution
test names; realistic gloss-overlap data; cross-field test via getDuplicateCheckQueries
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PWYjmnKqQz7EURdNYU2YvA
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWYjmnKqQz7EURdNYU2YvA
3 participants
Resolves #1752.
Searches existing entries as you type in the new-entry dialog and surfaces likely duplicates before you save.
Screenshots
A sticky pill is used to immediately communicate that the currently entered word or sense is a potential duplicate.
Similar words "pill"
Same form/headword "pill"
Similar words list
Collapsed:
Three kinds of matches: Same word, similar word and similar meaning: (Should there be "Same meaning"? Meh)
Note "Same word" ("Same lexeme form" in classic view), because it's a lexeme form exact match even though it's not a headword exact match:
First 3 are shown. More can be added to the DOM on demand:
Diacritic mismatch remotes an exact match to a similar match:
With the added diacritic it's an exact match:
Morph-tokens are NOT stripped before querying for potential duplicates. That might be an improvement (feels very similar to stripping diacritics, which seems like an obviously good idea).