[WordRepository, WordService] Refine DeleteFrontier usage #4100
Conversation
📝 WalkthroughWalkthroughRepository and service APIs changed: frontier deletions now optionally filter by audio file and return the deleted Word; bulk frontier deletion renamed; WordService renamed Delete -> DeleteAudio and Update now returns a nullable Id. Signatures updated across controller, repository, interfaces, mocks, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as AudioController
participant Service as WordService
participant Repo as WordRepository
participant DB as MongoDB
Client->>Service: DeleteAudio(projectId, userId, wordId, fileName)
Service->>Repo: DeleteFrontier(projectId, wordId, fileName)
Repo->>DB: FindOneAndDelete(filter by projectId+wordId + audio.FileName)
DB-->>Repo: deleted Word or null
Repo-->>Service: Word? (deleted frontier word)
Service->>Service: remove audio entries, update history
Service-->>Client: return Word? (Ok or NotFound)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (7)
✏️ Tip: You can disable this entire section by setting 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4100 +/- ##
===========================================
+ Coverage 74.79% 86.46% +11.66%
===========================================
Files 295 54 -241
Lines 10946 4750 -6196
Branches 1372 583 -789
===========================================
- Hits 8187 4107 -4080
+ Misses 2363 500 -1863
+ Partials 396 143 -253
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
imnasnainaec
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors the DeleteFrontier method in WordRepository and updates WordService to leverage the improved API, making operations more atomic and returning more useful values instead of simple success booleans.
Changes:
- Enhanced
DeleteFrontierto accept an optionalaudioFileNameparameter and return the deleted word instead of a boolean - Renamed the multi-word overload from
DeleteFrontiertoDeleteFrontierWordsto avoid signature conflicts - Updated
WordService.Updateto return the ID of the updated word instead of a boolean, and refactored to use the returned word fromDeleteFrontier - Renamed
WordService.Delete(projectId, userId, wordId, fileName)toDeleteAudioand removed the unusedDelete(projectId, userId, wordId)overload
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Backend/Repositories/WordRepository.cs | Added GetProjectWordWithAudioFilter helper, updated DeleteFrontier to return deleted word and accept optional audio parameter, renamed multi-word overload to DeleteFrontierWords |
| Backend/Interfaces/IWordRepository.cs | Updated interface signatures to match repository implementation |
| Backend/Services/WordService.cs | Removed unused Delete overload, renamed remaining Delete to DeleteAudio, updated Update to return word ID, refactored methods to use returned word from DeleteFrontier |
| Backend/Interfaces/IWordService.cs | Updated interface signatures to match service implementation |
| Backend/Controllers/AudioController.cs | Updated call from Delete to DeleteAudio |
| Backend/Services/MergeService.cs | Updated call from DeleteFrontier to DeleteFrontierWords |
| Backend.Tests/Services/WordServiceTests.cs | Updated test method names and assertions, modernized collection initializer syntax |
| Backend.Tests/Mocks/WordRepositoryMock.cs | Updated mock implementation to match new repository signatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Backend.Tests/Mocks/WordRepositoryMock.cs`:
- Around line 130-139: The mock DeleteFrontier currently treats empty-string
audioFileName differently and uses RemoveAll which deletes all matches; change
it to mirror repository FindOneAndDeleteAsync by treating audioFileName == null
or empty the same (i.e., ignore audio filter when
string.IsNullOrEmpty(audioFileName)) when locating the word with _frontier.Find,
and then remove only that single found instance (e.g., call
_frontier.Remove(word) or remove by index) instead of RemoveAll; ensure the
method still returns the deleted Word or null via Task.FromResult.
In `@Backend/Services/WordService.cs`:
- Around line 135-137: The Update method currently discards the result of
Create(userId, word) and returns word.Id which was cleared by PrepEditedData
(PrepEditedData sets word.Id = ""), so Update always returns empty string; fix
by using the value returned from Create (or have Create return the created
object's Id) and return that Id from Update instead of returning the original
word.Id, or alternatively stop clearing Id in PrepEditedData when creating new
records; update the Update method (and/or Create/PrepEditedData) so the returned
Id comes from the created entity rather than the mutated input object.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Backend.Tests/Mocks/WordRepositoryMock.csBackend.Tests/Services/WordServiceTests.csBackend/Controllers/AudioController.csBackend/Interfaces/IWordRepository.csBackend/Interfaces/IWordService.csBackend/Repositories/WordRepository.csBackend/Services/MergeService.csBackend/Services/WordService.cs
🧰 Additional context used
🧬 Code graph analysis (4)
Backend/Controllers/AudioController.cs (1)
src/types/word.ts (1)
newWord(88-103)
Backend.Tests/Mocks/WordRepositoryMock.cs (1)
Backend/Models/Word.cs (3)
Word(10-177)Word(87-107)Word(110-132)
Backend/Interfaces/IWordRepository.cs (1)
Backend/Models/Word.cs (3)
Word(10-177)Word(87-107)Word(110-132)
Backend.Tests/Services/WordServiceTests.cs (3)
Backend/Services/WordService.cs (1)
Word(21-30)Backend.Tests/Util.cs (2)
Word(48-61)Util(11-140)Backend/Models/Word.cs (3)
Word(10-177)Word(87-107)Word(110-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: docker_build
🔇 Additional comments (16)
Backend/Interfaces/IWordService.cs (1)
11-12: Signature updates align with the new service API.
Return types are explicit and match the refactor goals.Backend/Interfaces/IWordRepository.cs (1)
24-25: Repository delete signatures look consistent.
Optional audio filter plus renamed bulk delete align with the refactor.Backend.Tests/Mocks/WordRepositoryMock.cs (1)
142-147: Bulk delete rename is consistent.
Return type and behavior remain clear.Backend/Repositories/WordRepository.cs (3)
43-50: Audio filter helper is clear and scoped.
LGTM.
262-271: Atomic delete with optional audio filter looks correct.
Good use of FindOneAndDeleteAsync for an all-in-one operation.
276-281: Bulk delete rename aligns with the interface.
LGTM.Backend.Tests/Services/WordServiceTests.cs (5)
29-29: Test data setup updates look consistent.
EditedBy/Audio/list initializations align with the new shapes.Also applies to: 37-37, 44-44, 54-54, 65-65, 74-74
55-57: DeleteAudio test expectations align with the new contract.
Null-return cases and the updated call site are covered.Also applies to: 66-66, 75-75
83-85: Update tests reflect nullable id returns.
Covers both null and non-null outcomes.Also applies to: 95-95
126-126: RestoreFrontierWords scenarios remain well covered.
Inputs reflect the updated collection usage.Also applies to: 135-136, 145-145
185-185: Sense/gloss setup adjustments are consistent.
No issues noted.Also applies to: 206-208, 226-226
Backend/Services/WordService.cs (3)
11-13: LGTM! Primary constructor with field assignment is correct.The primary constructor syntax (C# 12) combined with explicit field initialization is a valid pattern that provides both the convenience of primary constructors and explicit field documentation.
59-74: LGTM! Atomic audio deletion logic is correct.The method correctly:
- Atomically fetches and removes the word from frontier with the audio filter
- Removes the matching audio entries
- Creates a new word version with updated history
78-93: LGTM! Atomic deletion pattern is correct.The method correctly uses
DeleteFrontierto atomically fetch and remove the word from the frontier before adding it back with the deleted status.Backend/Services/MergeService.cs (1)
120-124: LGTM! Correct migration to batch deletion API.The change to
DeleteFrontierWordsaligns with the new repository API for batch deletions, and thechildIdslist construction is appropriate.Backend/Controllers/AudioController.cs (1)
180-185: LGTM! Correctly updated to use the renamedDeleteAudiomethod.The error handling remains appropriate—returning
NotFoundwhen the word or audio file combination doesn't exist in the frontier.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 8 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec).
2 participants
In
WordRepo, updateDeleteFrontiermethodaudioFileNameparamaudioFileNameparam to be optionalIn
WordServiceDeletemethod: remove the unused overload; rename the used overload toDeleteAudioDeleteAudio,DeleteFrontierWord,Updatemethods, make use ofDeleteFrontier's returned wordUpdatemethod, return the id of the updated word, rather than just a success boolThis change is
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.