PT-4029: feat(comments): resolveConflict (accept/reject) write backend for verseText merge conflicts

[rolfheij-sil]

Adds the write half of merge-conflict resolution (PT-4029, epic PT-4024): a resolveConflict(threadId, 'accept' | 'reject') mutation on the comments data provider that applies a verseText conflict's resolution and marks the note resolved, so the conflict card (PT-4030) has a backend to call.

Stacked on #2482 (pt-4028-conflict-note-fields) — reuses its CreateVerseTextConflictComment test fixture; the base is intentionally that branch, mirroring how #2484 stacks on #2482.

What it does

• ResolveConflict(threadId, resolution) on ParatextProjectDataProvider, registered as the resolveConflict comment wire method alongside the existing comment mutations.
• Reuses PT9's CommentEditHelper.SaveEdits(owner: null, …) orchestration: accept resolves the note and leaves the auto-merged (winning) verse text untouched; reject writes the losing side's USFM into the verse (temporary chapter-edit grant → splice → restore), then resolves.
• Permission gate: only a project administrator, or the user the conflict is assigned to, may resolve (on top of the base VerifyUserCanResolveThread check).
• Guards that all throw before any event fires: invalid resolution string, missing thread, non-verseText conflict, already-resolved thread (prevents a reject-after-accept from rewriting a resolved verse), and a canceled SaveEdits.
• Refresh: fires a comment data-update for both paths; reject also fires a Scripture-text data-update, because the raw PutText inside SaveEdits bypasses the Set* methods that normally notify open editors.
• TS: resolveConflict(threadId: string, resolution: 'accept' | 'reject'): Promise<void> declared on ILegacyCommentProjectDataProvider.

Notes for reviewers

• The already-resolved guard goes slightly beyond the design doc (which was silent on re-resolution); it blocks a state transition PT9's UI structurally never allows. Flagging in case that behavior should differ.
• PT-4030 consumer note: canUserResolveThread is a broader gate than this method's admin-or-assignee check — the conflict UI should not use it to enable Accept/Reject.
• The reject write is best-effort inside PT9's SaveEdits (ReplaceAcceptedText no-ops with a trace if the verse marker is missing) — inherited PT9 behavior, documented in the method remarks.

Tests

11 new NUnit tests in ParatextProjectDataProviderCommentTests: accept (strict full-verse no-write assertion) / reject (loser text written) / guards (non-conflict, invalid resolution, reject-after-accept, creator-resolve safety pin) / permissions (non-admin denied, non-admin assignee reject succeeds and writes, most-recent reassignment wins) / data-update events (accept → comment update only; reject → comment + scripture updates). Full comment-test class 88/88 green; npm run typecheck clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JU63vyD4mRXpEGX5KKdyuB

---

This change is 

---

Automated review summary

Code Review Summary

Branch: pt-4029-resolve-conflict

Base: origin/main

Date: 2026-07-08

Review model: Claude Opus 4.8

Files changed: 9

Overview

PT-4029 adds a resolveConflict (accept / reject / merge) write backend for verseText merge
conflicts in the legacy comment/conflict system. Resolving applies the chosen resolution to the
underlying ParatextData via PT9's CommentEditHelper.SaveEdits; it is gated by permissions
(admin / assignee / team) and, for reject/merge, a staleness guard, and it is serialized under
_resolveConflictLock so the already-resolved check is atomic with the write (closing a
check-then-act race). It also adds a PT9 merge-diff preview (MergedText → mergedText JSON), a
getConflictResolutionOptions capability that reports which actions are available, and the
resolveConflict / getConflictResolutionOptions methods on the legacy comment PDP interface plus
the supporting platform-bible-utils types.

The concurrency design, JSON serialization, and cross-language contract are sound and exceptionally
well-tested (an 803-line test addition covering all three resolutions, the 8-task concurrency race,
the full permission matrix, and the staleness paths). Review surfaced one critical data-loss bug
in the merge path and one important permission gap, both now fixed and verified with C# tests;
plus a maintainability fix (a hand-duplicated type) and two nits.

API Changes

• lib/platform-bible-utils: Added mergedText?: string to LegacyComment (read-only display
  field; regenerated in dist/index.d.ts).
• lib/platform-bible-utils: (added during review) Added ConflictResolutionOptions = 'none' | 'accept' | 'acceptOrReject' | 'acceptRejectOrMerge' as the single source of truth for
  the conflict-resolution capability union (regenerated in dist/index.d.ts).
• extensions/src/legacy-comment-manager: Added resolveConflict(threadId, resolution) and
  getConflictResolutionOptions(threadId) to ILegacyCommentProjectDataProvider; added
  ConflictResolutionOptions (now re-exported from platform-bible-utils rather than declared
  locally); added mergedText to the omit lists of NewLegacyComment / LegacyCommentReply;
  documented a new @throws on addCommentToThread (throws when setting 'Resolved' on a merge
  conflict — use resolveConflict).

All API changes are additive; no exports removed or narrowed.

Findings

Critical — Must address before merge

• resolveConflict(id, "merge") on an overlapping-edit conflict erased the verse. The write
  path gated merge only on staleness, never on merge availability. For overlapping edits
  CommentEditHelper.GetMergedUsfm returns null, and PT9's MergeAcceptedText splices that null
  into the chapter USFM (C# concatenates null as ""), deleting the entire verse (\v marker and
  all) and writing it back via PutText; SaveEdits still returned true, so the thread was marked
  Resolved and success events fired — silent scripture loss. (fixed during review: ResolveConflict
  now throws when resolution == "merge" && GetMergedUsfm(thread) == null, mirroring the invariant
  GetConflictResolutionOptions already computes. New test ResolveConflict_MergeOnOverlappingConflict_ThrowsAndLeavesVerseIntact;
  revert-verified — without the gate the test fails with "no exception thrown", i.e. the verse is
  erased.) (c-sharp/Projects/ParatextProjectDataProvider.cs)
• addCommentToThread(status: 'Resolved') on a verseText conflict thread now throws (it
  previously resolved it); the compensating UI that routes conflicts to resolveConflict lives on
  the separate pt-4030 branch. (Author: skip — the throw only affects the conflict-resolution
  path, which pt-4030 moves entirely to resolveConflict; normal-thread resolves are unaffected.
  Accepted as a landing-order item: do not release PT-4029: feat(comments): resolveConflict (accept/reject) write backend for verseText merge conflicts #2495 to end-users ahead of pt-4030.)

Important — Should address before merge

• Permission gate dropped PT9's CanEdit(book, chapter) requirement for non-admin resolvers.
  PT9's GetResolutionOptions allows a non-admin only when assignee and
  Permissions.CanEdit(book, chapter); the PR checked admin-or-assignee but not chapter-edit rights,
  and SaveEdits's EnsureCanEditChapter temporarily grants the edit — so a non-admin assignee (or
  any team member, for team-assigned conflicts) could write a verse in a chapter they cannot edit.
  (fixed during review: VerifyUserCanResolveConflict now also requires
  scrText.Permissions.CanEdit(vref.BookNum, vref.ChapterNum) for non-admins. The existing "allowed"
  non-admin fixture was given chapter-edit rights (so those tests now mean "assignee WITH edit
  rights"), and two new tests cover the denied-without-edit case; revert-verified.)
  (c-sharp/Projects/ParatextProjectDataProvider.cs)
• ConflictResolutionOptions was hand-duplicated with a dangling reference. The sync comment
  pointed at platform-bible-react's conflict-note-card.types.ts, which does not exist in this
  branch or on main (only on pt-4030), and pt-4030 carries a third copy — the union was headed
  for triplication with reciprocal "keep textually identical" comments. (fixed during review: hoisted
  the union into lib/platform-bible-utils/src/comments.types.ts (both consumers already import from
  there), re-exported from legacy-comment-manager.d.ts, dist regenerated. Follow-up on pt-4030:
  drop its local copy and import from platform-bible-utils too.)

Minor — Consider

• IsConflictVerseStale regularizes AND .Trim()s both sides, more lenient than PT9 (which
  regularizes only the recorded side and never trims) — a whitespace-only post-merge edit is not
  classified stale and could be clobbered by a later reject/merge. (Author's deliberate design:
  there is a MatTwoWinnerWhitespaceUsfm fixture asserting this is intended; documented divergence.)
• MergedText getter (and its sibling conflict getters) are not exception-guarded during
  serialization — a malformed conflict comment reaching XmlDocument.LoadXml would propagate out of
  PlatformCommentConverter.Write and fail the entire getCommentThreads serialization, not just
  that field. Held off during review: RejectedText/AcceptedText/RejectedResultText share the
  same unguarded LoadXml, so a guard on MergedText alone is inconsistent, and a silent null-catch
  is its own smell. Recommended follow-up: per-comment resilience in PlatformCommentConverter.Write
  (skip + log a comment that fails to serialize) so one malformed comment can't drop the whole thread
  list. (c-sharp/JsonUtils/PlatformCommentWrapper.cs)
• VerifyUserCanResolveConflict / ResolveConflict / GetConflictResolutionOptions each call
  FindThread(threadId) redundantly (up to 3× per call). (Left — in-memory lookup, low value;
  having the verify helper return the thread would churn the diff for negligible benefit.)
• IsThreadAssignedToCurrentUser was misnamed (it takes a userName parameter and has no
  notion of "current"). (fixed during review: renamed to IsThreadAssignedToUser.)
• Exception-message punctuation inconsistency — the new InvalidDataException("...does not exist") throws lacked the trailing period used by the surrounding exception messages. (fixed
  during review: added the period to the three new throws. The exception type — InvalidDataException,
  documented in the TSDoc and used consistently across the new code — was intentionally left as-is.)
• The resolveConflict action union ('accept'|'reject'|'merge') and the
  getConflictResolutionOptions capability union read similarly but mean different things. (Left —
  correctly distinct concepts; the TSDoc already cross-links them.)

Template Propagation

Shared Regions Modified

None. No #region shared with markers in any of the 9 changed files.

Extension Config Changes

None. The only extensions/ file is a type-declaration file; no package.json/tsconfig/webpack/
.eslintrc*/CI changes.

Positive Observations

• The _resolveConflictLock correctly makes the already-resolved guard atomic with the SaveEdits
  write, closing the check-then-act race — and it's genuinely tested with 8 racing tasks asserting
  exactly-once resolution and final verse content.
• Strong reuse of PT9 infrastructure (SaveEdits, MergedText's RenderConflictSideHtml) rather
  than reimplementation; ResolveConflict and GetConflictResolutionOptions share one
  VerifyUserCanResolveConflict (enforcement vs. capability).
• The IsResolved/mergedText cross-process contract (TS ↔ JSON ↔ C#) matches exactly; dist
  regeneration mirrors src.
• Exemplary test coverage: all three resolutions, the concurrency race, the full permission matrix,
  the staleness paths (including the Done-status and unreadable-verse edges), and both mergeable
  (independent) and non-mergeable (overlapping) conflict fixtures.

Interview Notes

• Purpose (author-confirmed via PR title): a resolveConflict accept/reject/merge write backend
  for verseText merge conflicts, with server-side permission and staleness enforcement.
• C1 (merge data loss): author asked for the fix; applied + tested (incl. revert test).
• C2 (addCommentToThread throw): author chose to skip, on the basis that pt-4030 moves conflict
  resolution to resolveConflict; recorded as an accepted landing-order item.
• I1 (chapter-edit gate): the author's existing test deliberately asserted the relaxed behavior
  (a non-admin assignee without chapter-edit rights could resolve via EnsureCanEditChapter's temporary
  grant); the author confirmed they want to match PT9 strictly, so that test was re-scoped to
  "assignee WITH edit rights" and a denial test added.
• I2 (union hoist): author confirmed hoisting to platform-bible-utils as the single source of
  truth; a pt-4030 follow-up is needed to drop that branch's local copy.
• No areas were deferred to AI or left unexplained; the author engaged directly on every Critical/Important.

In-Review Quality Check

Changes were made during review (C1, I1, I2, M4, M5), touching 6 files (2 C#, 4 TS):

• C# tests: full c-sharp-tests suite — 1366 passed, 6 skipped, 0 failed; the
  conflict/resolution subset (112 tests) green. C1 and I1 were revert-tested (each fix's tests fail
  when the fix is disabled).
• C# format: csharpier-formatted.
• TS: platform-bible-utils typecheck clean; dist/index.d.ts regenerated via the library build
  (dts-bundle-generator); prettier clean.
• Typecheck caveat: the extension-level tsc could not run cleanly inside the isolated review
  worktree — its node_modules symlink resolves platform-bible-utils / legacy-comment-manager to
  the other (pt-4030) checkout, which lacks the new ConflictResolutionOptions export, producing a
  spurious TS2694 / TS6200. This is a cross-checkout artifact, not a code defect. Run
  npm run typecheck in a normal checkout (or rely on CI) as the final gate for the I2 import wiring.

Suggested Review Focus

• The C1 fix: confirm refusing merge (vs. silently degrading to accept/reject) is the desired
  behavior when the two sides overlap.
• I1: confirm the intent to require standing CanEdit(book, chapter) for non-admin resolvers
  (this tightens behavior vs. what the PR originally shipped and its original test asserted).
• M2 follow-up: decide whether to add per-comment resilience to PlatformCommentConverter.Write
  so a single malformed conflict can't fail the whole getCommentThreads response.
• Landing order: PT-4029: feat(comments): resolveConflict (accept/reject) write backend for verseText merge conflicts #2495 should not reach end-users ahead of pt-4030 (see C2), and pt-4030 needs
  a small follow-up to import ConflictResolutionOptions from platform-bible-utils (see I2).
• Full npm run typecheck in a standard checkout (see the typecheck caveat).

AI-assisted review — session

[lyonsil]

AI-assisted code review (Claude Opus 4.8, max-effort recall pass) of the resolveConflict / getConflictResolutionOptions write backend. 14 inline findings below, most-severe first: 1 confirmed regression (non-verseText conflicts unresolvable), a one-sided lock, and a MergedText serialization throw that can drop the whole thread list — plus correctness edges, reuse/simplification, and one borderline convention nit. Grounded against the PT9 CommentEditHelper / CommentThread sources and the PR's own test fixtures. Non-blocking (COMMENT event).

[lyonsil]

Regarding the review items:

1. The C1 fix: confirm refusing merge (vs. silently degrading to accept/reject) is the desired behavior when the two sides overlap.

• I think you were talking through this with Sebastian. I'm not as familiar with all the various ways this can go, but I trust that if you've aligned with him, we're good.

2. I1: confirm the intent to require standing CanEdit(book, chapter) for non-admin resolvers (this tightens behavior vs. what the PR originally shipped and its original test asserted).

• This aligns to P9 behavior and looks good.

3. M2 follow-up: decide whether to add per-comment resilience to PlatformCommentConverter.Write so a single malformed conflict can't fail the whole getCommentThreads response.

• This was flagged in the review comments from /code-review. Seems like a general code quality change to make if it's not a lot of trouble.

4. Landing order: PT-4029: feat(comments): resolveConflict (accept/reject) write backend for verseText merge conflicts #2495 should not reach end-users ahead of pt-4030 (see C2), and pt-4030 needs a small follow-up to import ConflictResolutionOptions from platform-bible-utils (see I2).

• You're managing this.

5. Full npm run typecheck in a standard checkout (see the typecheck caveat).

• CI handles this.

[lyonsil]

@lyonsil reviewed 11 files and all commit messages, made 1 comment, and resolved 14 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved.