Repository navigation
fix(review): #1258 review findings, round 3 - #1272
Merged
Merged
Conversation
… conflict never showed On the copy-editor path, open()'s `stale` refusal named the source's current version even when the source had moved alone past the edit's sourceBaseUpdatedAt. The client keeps that as conflictSourceUpdatedAt, so "keep mine" passed both locks and overwrote a source change the user never saw. The refusal now names the source's current version only when the copy was written with it (the source has not moved past the copy's updatedAt), and otherwise the edit's own sourceBaseUpdatedAt, so the replay gets `stale_source` and the user chooses again. Thread: #1258 (comment) Class sweep: no other place names a source version that moved alone. lockVersions() already names sourceBase; `stale_source` names the source move that is its own conflict; `incomplete` and the write context do not feed a conflict replay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…opy's 409 names The user-sharing spec promised both copy-editor 409s carry "the source's version". A `stale` answer now names the source's current version only when the copy was written with it, and otherwise the edit's own sourceBaseUpdatedAt. The Requirement says so, a new Scenario covers the source that moved alone, and the docblocks of staleRow(), StaleWriteException and ShareSaveController::staleBody that repeated the old promise now match. Thread: #1258 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n cost one more choice The "Keeping mine after the copy moved" scenario promised the replay saves when neither version moved again. When a copy-only write moved the copy past the source, the stale 409 now names the edit's source base, so "keep mine" can first meet a `stale_source` refusal. That is safe; the scenario now states the extra choice. Thread: #1258 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ase it checks testAStaleCopyNamesTheSourceVersionItFound now runs through the source-base branch: only the copy moved and the source stayed at the edit's base. It is kept, apart from the source-moved-alone case, under the name testAStaleCopyWhoseSourceStayedNamesTheSourceBase. Thread: #1258 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nly when its version equals the copy's movedOn() compares versions for equality, so a stale copy's 409 names the source's current version only when the source's updatedAt equals the copy's; a copy written after its source gets the edit's base. The user-sharing Requirement said "has not moved past"; it and the docblocks of staleRow() and StaleWriteException::getSourceUpdatedAt() now say the versions are equal. Thread: #1258 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes for the round-3 review of #1258 (#1258 (review)). Merge into
feature/1254-share-save, then re-request the review; nothing here is merged by the reviewer.Findings → commits
ebdf3758open()names the source's current version on astale409 only when it equals the copy's version, and the edit's ownsourceBaseUpdatedAtotherwise; twoShareSaveServiceTestcases, one per branch794089b6user-sharingspec, plus thestaleBody/staleRow/StaleWriteExceptiondocblocks, say which source version a stale copy's 409 names; new Scenario "A stale copy does not name a source that moved alone"862ae320offline-edit-queuespec: a copy written after its source can cost one more choice ("keep mine" first getsstale_source), which is safe and documented, not changed2d2e8964testAStaleCopyNamesTheSourceVersionItFoundrenamed totestAStaleCopyWhoseSourceStayedNamesTheSourceBase, the case it checks78fad4a2movedOn()'s!==Left to the author (threads stay open)
Checks run on this branch
phpunit -c phpunit-unit.xml --filter 'ShareSaveServiceTest|ShareSaveControllerTest|ShareServiceTest'(dev container, PHP 8.4) → OK, 130 tests, 352 assertionstestAStaleCopyDoesNotNameASourceVersionThatMovedAlonebefore the fix → red (-'2026-10-01T10:00:00+00:00'/+'2026-10-01T10:00:09+00:00')npx vitest run tests/store/offlineQueue.spec.js tests/vitest/offline-snapshot.spec.js(node 24) → 32 passed, including the "keep mine" casephpcs --standard=phpcs.xmlon the touched lib/ files → 0 errors; phpmd and phpstan onShareSaveService.php→ cleangit grep -n "moved past"→ empty🤖 Generated with Claude Code