fix(sheet): serialize reads behind in-flight saves to prevent drift m…#1113
Draft
hannessolo wants to merge 3 commits into
Draft
fix(sheet): serialize reads behind in-flight saves to prevent drift m…#1113hannessolo wants to merge 3 commits into
hannessolo wants to merge 3 commits into
Conversation
…isfires Debounced saves could overlap: a new saveSheets would run its checkForDrift GET while the previous saveSheets' POST was still on the wire, so the server correctly returned pre-write state that then failed the body-diff against the locally-updated _lastJsonString. That produced spurious "Content changed" dialogs and, via the non-dirty onStale branch, silent sheet resets. Add a _pendingSave promise to StaleCheck. saveSheets now wraps its POST + markSynced in staleCheck.runSave(...) so at most one save is in flight at a time. checkForDrift and getData (source/config paths) await this before issuing their GET, so no read ever races an in-flight write on the same document. Version reads skip the wait since version keys are immutable. Deadlock-free: POSTs only fire from saveSheets after its own checkForDrift completes, so a POST is always preceded by a read; the read serialisation therefore also bounds POST concurrency to one. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hannessolo
marked this pull request as draft
July 15, 2026 15:43
…s POST body saveSheets converted the sheet once at the top (used for markSynced) and saveToDa converted the live jexcel again to build the POST body. Because the two calls straddle awaits on checkForDrift and runSave, the user keeps typing in between, and the POST ends up persisting a newer snapshot than markSynced records. The next drift-check GET reads back the POSTed (newer) bytes, compares against the recorded (older) _lastJsonString, and fires a spurious onStale — dialog on dirty, silent reloadSheet on non-dirty. Thread the pre-converted snapshot through saveToDa to saveJson so the POST body and _lastJsonString come from the same jexcel state. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Replace _lastJsonString / full-body JSON comparison in StaleCheck with _lastEtag / ETag comparison. Track the ETag returned by POST responses and the initial GET, and compare against the drift-check GET's ETag. Eliminates the double-convertSheets snapshot race entirely: there is no longer any recorded JSON string that needs to match the POST body, so saveToDa no longer needs the jsonToSave passthrough parameter and the sheet.js 'success' listener no longer needs to re-run convertSheets on the live jexcel to update the baseline. da-title.handleAction reads the ETag directly off the POST/config response and calls markSynced inside runSave. Also more robust: ETag comparison is not fooled by JSON key-ordering, whitespace, or server-side field additions the way body-string comparison was. da-admin returns ETag on both PUT and GET responses and exposes it via Access-Control-Expose-Headers, so the browser can read it cross-origin. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hannessolo
marked this pull request as ready for review
July 15, 2026 17:16
hannessolo
requested review from
chrischrischris,
kptdobe and
usman-khalid
and removed request for
kptdobe
July 15, 2026 17:16
hannessolo
marked this pull request as draft
July 16, 2026 07:32
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
1 participant
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.
Draft - still buggy
https://sheetseq--da-live--adobe.aem.live
This PR fixes a bug where rapidly typing in the sheet editor either loses updates or causes the "out of sync" popup to appear.
Approach:
_pendingSavepromise so at most one POST is in flightcheckForDriftand live-filegetDataonawaitPendingSaveso no read races a same-tab write - since reads are much faster the polling could hit this caseda-title.handleAction) insiderunSaveso it can't race a background autosave