Add live duplicate check to the new-entry dialog#2411
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
the
💻 FW Lite
📝 WalkthroughWalkthroughAdds a duplicate-entry detection feature to the entry editor: a new duplicate-check utility module classifies candidate entries by match kind, a ChangesDuplicate Check Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![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 (3)
frontend/viewer/tests/new-entry-duplicates.test.ts (3)
21-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelectors depend on internal inline
styleattributes.
lexemeInput/glossInputlocate fields via[style*="grid-area: lexemeForm"]and[style*="grid-area: gloss"]. These are implementation details of the component's CSS grid layout, not stable testing contracts — any layout refactor (e.g. switching to a class-based grid or renaming grid areas) will silently break these locators.♻️ Suggested approach
Add
data-testid(or rely on accessible labels) on the lexeme/gloss form fields in the dialog component, then select viadialog.getByTestId('lexeme-form-field')etc. This decouples tests from layout implementation.🤖 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/tests/new-entry-duplicates.test.ts` around lines 21 - 27, The current test helpers lexemeInput and glossInput are coupled to internal inline style selectors, which makes the Playwright locators brittle. Update the dialog component to expose stable selectors such as data-testid or accessible labels for the lexeme and gloss fields, then change these helpers to use those stable hooks instead of querying [style*="grid-area: ..."] so the new-entry-duplicates test no longer depends on CSS layout details.
103-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest hardcodes fixture-specific counts ("3", "10+") tightly coupled to demo dataset.
This test's assertions (
toHaveCount(3), checking for the literal text'10+') depend on the exact number of demo entries whose headword starts with'ba'. Any change todemo-entry-data.ts(adding/removing entries starting with "ba") will silently break this test without an obvious link to the root cause.Consider deriving the expected counts from the demo dataset itself (e.g., counting matching entries programmatically) or adding an inline comment enumerating which demo entries are expected to match, to make the coupling more discoverable when it breaks.
🤖 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/tests/new-entry-duplicates.test.ts` around lines 103 - 119, The duplicate-list test is hardcoded to fixture-specific counts, so update the assertions in new-entry-duplicates.test.ts to avoid depending on a fixed "3" or literal "10+" tied to the demo dataset. Use the existing test helpers around openNewEntryDialog, lexemeInput, duplicateRows, and duplicatesSummary to derive the expected number of matches programmatically from the demo entries or document the exact expected "ba" matches inline so changes in demo-entry-data.ts are easier to track.
82-82: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNon-null assertion on
entryIdcould produce a confusing failure if absent.
new URL(page.url()).searchParams.get('entryId')!would silently coercenullto satisfy the type checker if the param is missing, leading to an unclear downstream assertion failure viaentryHasGlossValuerather than a clear "entryId missing" error.🧪 Suggested tweak
- const entryId = new URL(page.url()).searchParams.get('entryId')!; + const entryId = new URL(page.url()).searchParams.get('entryId'); + expect(entryId).toBeTruthy(); await expect(async () => { - expect(await browsePage.api.entryHasGlossValue(entryId, newGloss)).toBe(true); + expect(await browsePage.api.entryHasGlossValue(entryId!, newGloss)).toBe(true); }).toPass({timeout: 5000});🤖 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/tests/new-entry-duplicates.test.ts` at line 82, The `entryId` lookup in `new-entry-duplicates.test.ts` is using a non-null assertion that can hide a missing query param and cause a misleading failure later. Update the test around `page.url()` and `searchParams.get('entryId')` to explicitly check for a missing value and fail immediately with a clear message before calling `entryHasGlossValue`, so the failure points directly to the absent `entryId`.
🤖 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/DuplicateCheck.svelte`:
- Around line 113-128: The pending save in addSenseToEntry can overwrite a newer
user navigation because openEntry(target) always runs after createSense
resolves. Update the DuplicateCheck.svelte flow so a “go to entry” click during
an in-flight add-sense either waits/blocks, or the post-save navigation is
skipped if the user has already navigated elsewhere; use the existing
addingSense state and the entry-opening logic around addSenseToEntry/openEntry
to keep the final navigation aligned with the user’s last click.
---
Nitpick comments:
In `@frontend/viewer/tests/new-entry-duplicates.test.ts`:
- Around line 21-27: The current test helpers lexemeInput and glossInput are
coupled to internal inline style selectors, which makes the Playwright locators
brittle. Update the dialog component to expose stable selectors such as
data-testid or accessible labels for the lexeme and gloss fields, then change
these helpers to use those stable hooks instead of querying [style*="grid-area:
..."] so the new-entry-duplicates test no longer depends on CSS layout details.
- Around line 103-119: The duplicate-list test is hardcoded to fixture-specific
counts, so update the assertions in new-entry-duplicates.test.ts to avoid
depending on a fixed "3" or literal "10+" tied to the demo dataset. Use the
existing test helpers around openNewEntryDialog, lexemeInput, duplicateRows, and
duplicatesSummary to derive the expected number of matches programmatically from
the demo entries or document the exact expected "ba" matches inline so changes
in demo-entry-data.ts are easier to track.
- Line 82: The `entryId` lookup in `new-entry-duplicates.test.ts` is using a
non-null assertion that can hide a missing query param and cause a misleading
failure later. Update the test around `page.url()` and
`searchParams.get('entryId')` to explicitly check for a missing value and fail
immediately with a clear message before calling `entryHasGlossValue`, so the
failure points directly to the absent `entryId`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 74e64c61-0469-474a-9d9b-58ef682e3de4
📒 Files selected for processing (13)
frontend/viewer/src/lib/entry-editor/DuplicateCheck.sveltefrontend/viewer/src/lib/entry-editor/NewEntryDialog.sveltefrontend/viewer/src/lib/entry-editor/duplicate-check.test.tsfrontend/viewer/src/lib/entry-editor/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/new-entry-duplicates.test.ts
| async function addSenseToEntry(target: IEntry): Promise<void> { | ||
| if (!sense || addingSense) return; | ||
| addingSense = true; | ||
| try { | ||
| const senseSnapshot = {...$state.snapshot(sense), entryId: target.id}; | ||
| await saveHandler.handleSave(() => lexboxApi.createSense(target.id, senseSnapshot)); | ||
| } finally { | ||
| addingSense = false; | ||
| } | ||
| AppNotification.display( | ||
| pt($t`Sense added to "${writingSystemService.headword(target)}"`, | ||
| $t`Meaning added to "${writingSystemService.headword(target)}"`, | ||
| viewService.currentView), | ||
| {type: 'success', timeout: 'short'}); | ||
| openEntry(target); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Race: clicking "go to entry" on another row while an add-sense save is pending can trigger a surprise second navigation.
addingSense only disables the add-sense icon buttons; the "go to entry" buttons stay clickable. If a user clicks a different match's "go to entry" while addSenseToEntry is still awaiting createSense, the dialog closes and navigation happens immediately — but once the pending save resolves, addSenseToEntry still calls openEntry(target) again, silently re-navigating the user to a different entry than the one they just chose.
🛠️ Suggested fix
<button
type="button"
class="grow min-w-0 flex items-center gap-2 rounded bg-background/80 hover:bg-accent px-2.5 py-2 text-start"
title={goToLabel}
aria-label={headword ? `${goToLabel}: ${headword}` : goToLabel}
+ disabled={addingSense}
onkeydown={trapEnter}
onclick={() => openEntry(match.entry)}>Also applies to: 176-193
🤖 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/DuplicateCheck.svelte` around lines 113
- 128, The pending save in addSenseToEntry can overwrite a newer user navigation
because openEntry(target) always runs after createSense resolves. Update the
DuplicateCheck.svelte flow so a “go to entry” click during an in-flight
add-sense either waits/blocks, or the post-save navigation is skipped if the
user has already navigated elsewhere; use the existing addingSense state and the
entry-opening logic around addSenseToEntry/openEntry to keep the final
navigation aligned with the user’s last click.
|
I like it! |
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>
2 participants
Searches existing entries as you type in the new-entry dialog and surfaces likely duplicates before you save.
duplicate-check.ts— debounced multi-field search (headword/citation/gloss) via the existing FTSsearchEntries; results classified as same word / similar word / similar meaning. Classification approximates the backend match semantics (invariant case fold; diacritics significant only when the typed text has them; morph tokens stripped likeEntrySearchService.StripMorphTokens); collation equivalences (ss/eszett, ligatures) are not replicated — a backend folding change should re-check this fileDuplicateCheck.svelte— amber collapsible strip inNewEntryDialog.svelte; auto-expands on an exact headword match; quiet green "Looks like a new word" line when nothing matchessearchEntriescapped at 10 rows — typically 1-3 queries per pause; in-process on desktop, SignalR round-trips on server-hosted sessionsNote: the
.poregen also picks up stale activity-view#:refs from an earlier refactor (benign, identical across locales).Screenshots
Test plan
Considered and rejected
🤖 Generated with Claude Code