Simplify no-op commit

[myieye]

For some reason I didn't originally think of the idea of simply creating 0 change objects, which works and is much cleaner than my weird hack.

[coderabbitai]

📝 Walkthrough

Walkthrough

A test file was refactored to replace a private tuple-returning helper method with a new nested FakeCommit class featuring a static NoOp factory. Unnecessary imports were removed, snapshot filtering logic was eliminated, and test flows were adjusted to use the new factory pattern.

Changes

Cohort / File(s)	Summary
Test refactoring backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs	Replaced private helper returning tuple with nested FakeCommit class and static NoOp factory; removed snapshot filtering by no-op entity id; eliminated LcmCrdt.Changes and SIL.Harmony.Changes imports; updated test flow to use new NoOp factory.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Verify the FakeCommit class implementation is correctly nested and the NoOp factory produces expected Commit objects
• Confirm removal of snapshot filtering aligns with the new no-op handling mechanism
• Check that test flows using the updated NoOp factory maintain the same test coverage and assertions

Poem

🐰 A helper transformed with grace,
From tuples to a factory place,
The FakeCommit hops into view,
No-op patterns shiny and new! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Simplify no-op commit' clearly and concisely summarizes the main change: refactoring the no-op commit mechanism to be simpler.
Description check	✅ Passed	The description explains the rationale for the change by contrasting the new cleaner approach (creating 0 change objects) with the previous 'weird hack', which is directly related to the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch simplify-no-op-commit

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[argos-ci]

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

Build	Status	Details	Updated (UTC)
default (Inspect)	✅ No changes detected	-	Nov 10, 2025, 2:35 PM

[github-actions]

UI unit Tests

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

Results for commit 9567197. ± Comparison against base commit 89aa91c.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs (1)

163-180: LGTM! Clean factory pattern implementation.

The FakeCommit class with its NoOp factory method is a much cleaner solution than the previous approach. Returning Commit instead of FakeCommit provides good encapsulation.

Minor observation: Lines 177-178 may be redundant if the base constructor already sets HybridDateTime and the parent hash. However, this could be intentional defensive programming or necessary due to base class behavior.

If the base constructor already sets these properties correctly, consider this cleanup:

 [SetsRequiredMembers]
 public FakeCommit(Guid id, HybridDateTime hybridDateTime) : base(id, "", NullParentHash, hybridDateTime)
 {
-    HybridDateTime = hybridDateTime;
-    SetParentHash(NullParentHash);
 }

Only apply if the base constructor behavior is verified to handle these correctly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 89aa91c and 9567197.

📒 Files selected for processing (1)

• backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs (2 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: imnasnainaec
Repo: sillsdev/languageforge-lexbox PR: 1867
File: platform.bible-extension/src/main.ts:239-246
Timestamp: 2025-07-31T19:10:41.178Z
Learning: In the sillsdev/languageforge-lexbox repository, user imnasnainaec prefers to defer code improvements when there are related TODO comments indicating planned refactoring work, choosing to bundle related changes together rather than making incremental improvements that would need to be modified again during the larger refactoring.

📚 Learning: 2025-04-17T02:52:44.986Z

Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.

Applied to files:

• backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs

📚 Learning: 2025-11-07T08:54:01.604Z

Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 2093
File: backend/FwLite/LcmCrdt/HistoryService.cs:173-200
Timestamp: 2025-11-07T08:54:01.604Z
Learning: In LcmCrdt, the `DataModel.GetLatest<T>(Guid id)` method returns the latest snapshot of an entity regardless of whether it has been deleted or not. It only returns null (and thus throws NotFoundException when null-coalesced) when the entity truly never existed or the data is corrupt.

Applied to files:

• backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs

⏰ 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). (4)

• GitHub Check: Analyze (csharp)
• GitHub Check: frontend
• GitHub Check: frontend-component-unit-tests
• GitHub Check: Build FW Lite and run tests

🔇 Additional comments (2)

backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs (2)

117-118: LGTM! Much cleaner approach.

The new FakeCommit.NoOp factory method is significantly cleaner than the previous tuple-based workaround. Using DateTimeOffset.MinValue appropriately isolates this test commit from actual data timestamps.

---

123-129: LGTM! Simplified query aligns with the new approach.

Removing the snapshot filtering is correct since the new NoOp commit creates zero ChangeEntities, eliminating the need to exclude a fake entity. This is cleaner and more maintainable.