Conversation
github-actions
Bot
added
the
💻 FW Lite
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds support for moving senses between entries without losing them during synchronization. It introduces a new ChangesMove-Sense Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (1)
705-707:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the entry path before deleting a sense.
This method still ignores
entryId, so a delete sent against the old entry route will remove a sense even after it has been moved. That reopens the moved-sense data-loss case this PR is trying to close on the CRDT backend.Suggested fix
public async Task DeleteSense(Guid entryId, Guid senseId) { + await using var repo = await repoFactory.CreateRepoAsync(); + var sense = await repo.GetSense(senseId) ?? throw NotFoundException.ForType<Sense>(senseId); + VerifySenseBelongsToEntry(entryId, sense); await AddChange(new DeleteChange<Sense>(senseId)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs` around lines 705 - 707, DeleteSense currently ignores the entryId and unconditionally enqueues DeleteChange<Sense>, which allows deleting a sense from its old entry after it was moved; update DeleteSense to first validate that the senseId belongs to the provided entryId before calling AddChange. Specifically, inside DeleteSense use the existing lookup method for entries (e.g., GetEntry, FindEntryById, or equivalent in this class) to load the entry for entryId, confirm entry.Senses (or the entry's sense collection) contains senseId (or that the sense's parent matches entryId), and only then call AddChange(new DeleteChange<Sense>(senseId)); otherwise return/throw an appropriate error to reject the operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`:
- Around line 1439-1442: The ownership check in VerifySenseBelongsToEntry uses a
non-existent ILexSense.Member EntryId; update the thrown message to use the
available properties: replace sense.Id with sense.Guid (the sense's GUID) and
replace sense.EntryId with sense.Entry.Guid (the entry's GUID) so the
NotFoundException includes the correct actual entry Id and sense Id using the
ILexSense members.
---
Outside diff comments:
In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs`:
- Around line 705-707: DeleteSense currently ignores the entryId and
unconditionally enqueues DeleteChange<Sense>, which allows deleting a sense from
its old entry after it was moved; update DeleteSense to first validate that the
senseId belongs to the provided entryId before calling AddChange. Specifically,
inside DeleteSense use the existing lookup method for entries (e.g., GetEntry,
FindEntryById, or equivalent in this class) to load the entry for entryId,
confirm entry.Senses (or the entry's sense collection) contains senseId (or that
the sense's parent matches entryId), and only then call AddChange(new
DeleteChange<Sense>(senseId)); otherwise return/throw an appropriate error to
reject the operation.
🪄 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: 025a0b68-44ac-424a-94eb-7472afc5f800
📒 Files selected for processing (13)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.csbackend/FwLite/FwLiteWeb/Routes/MiniLcmRoutes.csbackend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txtbackend/FwLite/LcmCrdt/Changes/MoveSenseToEntryChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/Data/MiniLcmRepository.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/MiniLcm.Tests/SenseTestsBase.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs
myieye
force-pushed
the
fix-syncing-moved-senses
branch
from
70565f0 to
a5fcb79
Compare
github-actions
Bot
added
the
📦 Lexbox
C# Unit Tests165 tests 165 ✅ 19s ⏱️ Results for commit 41f85de. ♻️ This comment has been updated with latest results. |
myieye
force-pushed
the
fix-syncing-moved-senses
branch
2 times, most recently
from
1d8cdbd to
75edb4c
Compare
myieye
force-pushed
the
fix-syncing-moved-senses
branch
from
75edb4c to
41f85de
Compare
rmunn
left a comment
rmunn
left a comment
There was a problem hiding this comment.
Only just started the review so I haven't looked at the rest of the code, but I found something that needs changing in the first file. I'll go and review the rest now, but I wanted to give you this feedback ASAP so that you can work on it even if I don't manage to review the rest of the PR today.
| public Task<Sense?> GetSense(Guid entryId, Guid id) | ||
| { | ||
| SenseRepository.TryGetObject(id, out var lcmSense); | ||
| if (lcmSense is not null) VerifySenseBelongsToEntry(entryId, lcmSense); |
There was a problem hiding this comment.
There's a PR recently merged from @imnasnainaec where this API method gets called with Guid.Empty for the entry ID. We may want to check for an empty entry GUID and forward to the GetSense(Guid id) call if the entry is unknown. The code that currently calls GetSense(Guid.Empty, senseId) can be updated to just call GetSense(senseId) once this PR is in, but in the overlap period we will want to allow GetSense(Guid.Empty, senseId) not to throw an error.
|
I ran the tests multiple times. Sometimes they passed, sometimes they failed. When they failed, it was always the same: "System.InvalidOperationException : Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct." There's a race condition in the test, which sometimes passes if the two MockFwProjectLoader instances are being set up at different times. But if they run at the same time then this can trigger. I was either seeing all tests passing, or about 45 tests failing all together with the same error. (Not surprising since the CanSyncComponentWhenSenseMovesToDifferentEntry test is a data-driven test that runs itself 24 times). |
2 participants
Resolves #2251