Skip to content

Conversation

@myieye
Copy link
Collaborator

@myieye myieye commented Oct 23, 2025

Resolves: #2060

Am fast-tracking this to unblock a workshop.
See the critical Harmony changes in sillsdev/harmony#54

EDIT: this mostly worked, but another bug was found that was patched directly in develop in cac4c65

@github-actions github-actions bot added 💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related 📦 Lexbox issues related to any server side code, fw-headless included labels Oct 23, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 23, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This PR modifies merge-conflict handling in CRDT-based change tracking by unconditionally duplicating all changes with new entity IDs, unconditionally replacing translations by ID to handle conflicts, and adds test cases validating that deleted entries, senses, and example sentences can be properly recreated.

Changes

Cohort / File(s) Summary
Change handling and duplication logic
backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs, backend/FwLite/LcmCrdt/Changes/ExampleSentences/AddTranslationChange.cs
Replaced conditional CreateChange detection with unconditional duplicate addition logic; removed IsCreateChange helper; changed translation application to unconditionally replace by ID via RemoveAll before adding, addressing merge-conflict scenarios
Recreation test cases
backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs, backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs, backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs
Added three new async test methods (RecreateDeletedExampleSentence, RecreateDeletedSense, RecreateDeletedEntry) validating deletion and recreation workflows with initial data preservation
Submodule update
backend/harmony
Updated subproject commit reference hash

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The changes combine core logic modifications affecting duplicate/conflict handling with straightforward test additions. The logic adjustments require careful review to validate merge-conflict semantics, while tests follow consistent patterns. Mixed complexity across heterogeneous file types warrants moderate review depth.

Possibly related PRs

  • PR #2000: Modifies example-sentence translation model and change types (AddTranslationChange, related update/remove handlers) with overlapping translation application logic.
  • PR #1796: Introduced the IsCreateChange helper and conditional duplicate logic that this PR now reverses by removing the helper and applying unconditional duplication.

Suggested reviewers

  • hahn-kev
  • rmunn

Poem

🐰 When entries deleted come back to life,
Merge conflicts cease to bring us strife,
We duplicate with guids anew,
And test what broken dreams can do,
Resurrection blooms in tests so true!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Allow recreating deleted entities instead of throwing" accurately summarizes the primary change in the PR. The changes show modifications to support entity recreation (AddTranslationChange.cs now unconditionally replaces translations, test files add new test methods for RecreateDeletedEntry/Sense/ExampleSentence), and UseChangesTests.cs now handles SupportsNewEntity() for recreating deleted entities. The title is concise, clear, and specific enough for a developer scanning history to understand the main objective.
Linked Issues Check ✅ Passed The changes directly address the objectives in issue #2060. The PR modifies AddTranslationChange.cs to unconditionally remove and replace translations with a comment noting merge-conflict scenarios, and UseChangesTests.cs now unconditionally adds duplicates and handles SupportsNewEntity() for entity recreation. Three new test methods (RecreateDeletedEntry, RecreateDeletedSense, RecreateDeletedExampleSentence) specifically test the ability to delete and recreate entities, directly implementing the expected behavior of restoring deleted entities instead of throwing exceptions during merge conflicts.
Out of Scope Changes Check ✅ Passed All code changes in the PR are directly aligned with the objective of enabling recreation of deleted entities. The modifications to UseChangesTests.cs and AddTranslationChange.cs implement the core logic for handling recreated entities in merge-conflict scenarios, while the new test methods verify this functionality. The submodule update is a standard maintenance change. No out-of-scope modifications were detected.
Description Check ✅ Passed The pull request description is minimal but directly related to the changeset. It references issue #2060, which is the core issue the PR is designed to resolve: allowing deleted entities to be recreated instead of throwing exceptions during merge conflicts. The changeset includes modifications to handle merge-conflict scenarios (such as the unconditional translation replacement in AddTranslationChange), new tests specifically for recreating deleted entities (RecreateDeletedExampleSentence, RecreateDeletedSense, RecreateDeletedEntry), and updates to the Harmony submodule with critical changes. The description ties these changes together by indicating they resolve the referenced issue and provides context about fast-tracking to unblock a workshop.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

C# Unit Tests

130 tests  ±0   130 ✅ ±0   19s ⏱️ -1s
 20 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit d374922. ± Comparison against base commit 19cc689.

@argos-ci
Copy link

argos-ci bot commented Oct 23, 2025

The latest updates on your projects. Learn more about Argos notifications ↗︎

Awaiting the start of a new Argos build…

@github-actions
Copy link

UI unit Tests

  1 files  ±0   45 suites  ±0   28s ⏱️ -2s
111 tests ±0  111 ✅ ±0  0 💤 ±0  0 ❌ ±0 
160 runs  ±0  160 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit d374922. ± Comparison against base commit 19cc689.

@myieye myieye merged commit cb8e1e4 into develop Oct 23, 2025
6 of 12 checks passed
@myieye myieye deleted the 2060-merge-conflict-in-fieldworks-undoes-delete-entry branch October 23, 2025 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related 📦 Lexbox issues related to any server side code, fw-headless included

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge conflict in FieldWorks undoes delete entry

2 participants