fix(sheet): serialize reads behind in-flight saves to prevent drift m…

blocks/edit/da-title/da-title.js

@@ -226,31 +226,39 @@ export default class DaTitle extends LitElement {
 
     // Bail before writing if the remote drifted under us — protects against
     // last-write-wins. Drift triggers the stale-content dialog via onStale.
+    // The POST itself runs inside staleCheck.runSave so it shares the same
+    // serialisation gate as the debounced saveSheets flow — otherwise a
+    // Preview/Publish click could race a background autosave on the same file.
     if (view === 'sheet' || view === 'config') {
       const { staleCheck } = await import('../../sheet/utils/utils.js');
       if (await staleCheck.checkForDrift()) {
         this._isSending = false;
         return;
       }
-    }
 
-    // Only save to DA if it is a sheet or config
-    if (view === 'sheet') {
-      const sheetPath = fullpath.replace('.json', '');
-      const dasSave = await saveToDa(sheetPath, this.sheet);
-      if (!dasSave.ok) return;
-    }
-    if (view === 'config') {
-      const daConfigResp = await saveDaConfig(fullpath, this.sheet);
-      if (!daConfigResp.ok) {
-        // eslint-disable-next-line no-console
-        console.log('Saving configuration failed because:', daConfigResp.status, await daConfigResp.text());
-        return;
-      }
-    }
-    if (view === 'sheet' || view === 'config') {
-      // Tell anything listening save was successful
-      this.handleSuccess('save');
+      const savedOk = await staleCheck.runSave(async () => {
+        let resp;
+        if (view === 'sheet') {
+          const sheetPath = fullpath.replace('.json', '');
+          resp = await saveToDa(sheetPath, this.sheet);
+        } else {
+          resp = await saveDaConfig(fullpath, this.sheet);
+        }
+        if (!resp.ok) {
+          if (view === 'config') {
+            // eslint-disable-next-line no-console
+            console.log('Saving configuration failed because:', resp.status, await resp.text());
+          }
+          return false;
+        }
+        // markSynced inside runSave so _lastEtag is updated before pendingSave
+        // resolves — a concurrent read that unblocks on pendingSave otherwise
+        // sees a stale baseline etag.
+        staleCheck.markSynced(resp.headers.get('etag'));
+        this.handleSuccess('save');
+        return true;
+      });
+      if (!savedOk) return;
     }
 
     // AEM Actions

blocks/sheet/sheet.js

@@ -4,7 +4,6 @@ import { getNx } from '../../scripts/utils.js';
 import '../edit/da-title/da-title.js';
 import { getData } from './utils/index.js';
 import { staleCheck, showDaDialog, restoreVersion } from './utils/utils.js';
-import { convertSheets } from '../edit/utils/helpers.js';
 
 const { default: getStyle } = await import(`${getNx()}/utils/styles.js`);
 
@@ -122,7 +121,7 @@ async function reloadSheet(daTitle, daSheet) {
 async function setSheet(details, daTitle, daSheet) {
   // Drop any open stale-content dialog so its Cancel can't act on the new path's staleCheck.
   document.body.querySelectorAll(':scope > da-dialog').forEach((d) => d.remove());
-  // Full reset before the load — getData calls markSynced which sets _lastJsonString.
+  // Full reset before the load — getData calls markSynced which sets _lastEtag.
   // start() below only wires up the interval without resetting state.
   staleCheck.stop();
 
@@ -186,11 +185,6 @@ export default async function init(el) {
 
   el.append(daTitle, versionWrapper);
 
-  daTitle.addEventListener('success', (e) => {
-    if (e.detail.action !== 'save') return;
-    staleCheck.markSynced(convertSheets(daSheet.jexcel));
-  });
-
   window.addEventListener('hashchange', async () => {
     details = getPathDetails();
     setSheet(details, daTitle, daSheet);

blocks/sheet/utils/index.js

@@ -94,6 +94,9 @@ export async function getData(input) {
   const { config, source, versions } = await getNx2Api();
   const { org, site, path, view, versionId } = input;
 
+  // Version snapshots are immutable; only live-file reads need to wait for our own writes.
+  if (!versionId) await staleCheck.awaitPendingSave();
+
   let resp;
   let isVersion = false;
   if (versionId) {
@@ -120,7 +123,7 @@ export async function getData(input) {
   const json = await resp.json();
 
   if (!isVersion) {
-    staleCheck.markSynced(json);
+    staleCheck.markSynced(resp.headers.get('etag'));
     const sheetPanes = document.querySelector('da-sheet-panes');
     if (sheetPanes) sheetPanes.data = json;
   }

blocks/sheet/utils/utils.js

@@ -8,10 +8,12 @@ class StaleCheck {
   constructor() {
     this._intervalId = null;
     this._doc = null;
-    this._lastJsonString = null;
+    this._lastEtag = null;
     this._hasLocalEdits = false;
     this._saveBlocked = false;
     this._onStale = null;
+    // Set while a POST is in flight so reads (drift + reload) queue behind it.
+    this._pendingSave = null;
   }
 
   // `details` is the pathDetails object ({ org, site, path, view }).
@@ -26,14 +28,16 @@ class StaleCheck {
     if (this._intervalId) clearInterval(this._intervalId);
     this._intervalId = null;
     this._doc = null;
-    this._lastJsonString = null;
+    this._lastEtag = null;
     this._hasLocalEdits = false;
     this._saveBlocked = false;
     this._onStale = null;
+    // Detach so a new file's load isn't gated on the previous file's save.
+    this._pendingSave = null;
   }
 
-  markSynced(json) {
-    this._lastJsonString = JSON.stringify(json);
+  markSynced(etag) {
+    this._lastEtag = etag;
     this._hasLocalEdits = false;
     // Clear the post-Cancel block: a fresh sync (load or save) is the recovery path.
     this._saveBlocked = false;
@@ -51,10 +55,32 @@ class StaleCheck {
     return this._saveBlocked;
   }
 
+  // Loops so a save that started mid-wait is also awaited.
+  async awaitPendingSave() {
+    while (this._pendingSave) {
+      // eslint-disable-next-line no-await-in-loop
+      await this._pendingSave;
+    }
+  }
+
+  async runSave(saveOp) {
+    await this.awaitPendingSave();
+    let resolve;
+    const p = new Promise((r) => { resolve = r; });
+    this._pendingSave = p;
+    try {
+      return await saveOp();
+    } finally {
+      resolve();
+      if (this._pendingSave === p) this._pendingSave = null;
+    }
+  }
+
   // Returns true if drift was detected (caller should not write).
   // Skips while blocked so a stale dialog doesn't keep re-firing on subsequent edits/polls.
   async checkForDrift() {
     if (this._saveBlocked) return true;
+    await this.awaitPendingSave();
     try {
       const { config, source } = await getNx2Api();
       const { org, site, path, view } = this._doc;
@@ -63,9 +89,10 @@ class StaleCheck {
         ? await config.get({ org, site })
         : await source.get({ org, site, path });
       if (!resp.ok) return false;
+      // No baseline (never synced) or missing header: nothing to compare against.
+      const etag = resp.headers.get('etag');
+      if (!etag || !this._lastEtag || etag === this._lastEtag) return false;
       const json = await resp.json();
-      const text = JSON.stringify(json);
-      if (text === this._lastJsonString) return false;
       this._onStale({ json, dirty: this._hasLocalEdits });
       return true;
     } catch {
@@ -78,23 +105,24 @@ class StaleCheck {
 export const staleCheck = new StaleCheck();
 
 export const saveSheets = async (sheets) => {
-  const convertedJson = convertSheets(sheets);
-  document.querySelector('da-sheet-panes').data = convertedJson;
+  document.querySelector('da-sheet-panes').data = convertSheets(sheets);
 
   // Bail before writing if the remote moved out from under us — protects against
   // last-write-wins between concurrent editors. Drift triggers the onStale flow.
   if (await staleCheck.checkForDrift()) return false;
 
   const { hash } = window.location;
   const pathname = hash.replace('#', '');
-  const dasSave = await saveToDa(pathname, sheets);
-  if (!dasSave.ok) {
-    // eslint-disable-next-line no-console
-    console.error('Error saving sheet', dasSave);
-    return false;
-  }
-  staleCheck.markSynced(convertedJson);
-  return true;
+  return staleCheck.runSave(async () => {
+    const dasSave = await saveToDa(pathname, sheets);
+    if (!dasSave.ok) {
+      // eslint-disable-next-line no-console
+      console.error('Error saving sheet', dasSave);
+      return false;
+    }
+    staleCheck.markSynced(dasSave.headers.get('etag'));
+    return true;
+  });
 };
 
 const debouncedSaveSheets = debounce(saveSheets, DEBOUNCE_TIME);

test/unit/blocks/sheet/utils-utils.test.js

@@ -67,44 +67,54 @@ describe('sheet/utils utils', () => {
     });
   });
 
-  describe('staleCheck after restore', () => {
+  describe('etag drift detection', () => {
     const DETAILS = { org: 'org', site: 'repo', path: '/sheet', view: 'sheet' };
     const serverJson = { ':type': 'sheet', ':sheetname': 'data', data: [{ key: 'a' }] };
 
-    beforeEach(() => {
-      staleCheck.start({ details: DETAILS, onStale: () => {} });
-      staleCheck.markSynced(serverJson);
-    });
-
     afterEach(() => {
       staleCheck.stop();
     });
 
-    it('markSynced with jspreadsheet-format array causes saveSheets to falsely bail', async () => {
-      // Simulates the old (buggy) restore handler calling markSynced with wrong-format data
-      const jspsheetData = [{ sheetName: 'data', data: [['key'], ['a']], columns: [] }];
-      staleCheck.markSynced(jspsheetData);
+    it('bails saveSheets when server etag differs from the recorded baseline', async () => {
+      let onStaleFired = false;
+      staleCheck.start({ details: DETAILS, onStale: () => { onStaleFired = true; } });
+      staleCheck.markSynced('"baseline"');
 
       window.location.hash = '#/org/repo/sheet';
-      window.fetch = async () => new Response(JSON.stringify(serverJson), { status: 200 });
+      window.fetch = wrap(async (url, opts) => {
+        if (opts?.method === 'POST' || opts?.method === 'PUT') {
+          return new Response('', { status: 200, headers: { ETag: '"remote"' } });
+        }
+        return new Response(JSON.stringify(serverJson), {
+          status: 200,
+          headers: { ETag: '"remote"', 'Content-Type': 'application/json' },
+        });
+      });
 
       const sheets = [buildSheet('data', [['key'], ['a']])];
       const result = await saveSheets(sheets);
-      expect(result).to.be.false; // drift falsely detected — save bailed
+      expect(result).to.be.false;
+      expect(onStaleFired).to.be.true;
     });
 
-    it('Preserves correct baseline so saveSheets proceeds when server is unchanged', async () => {
-      // Restore handler does NOT call markSynced with wrong-format data — baseline stays intact.
+    it('proceeds when server etag matches the recorded baseline', async () => {
+      staleCheck.start({ details: DETAILS, onStale: () => {} });
+      staleCheck.markSynced('"baseline"');
 
       window.location.hash = '#/org/repo/sheet';
-      window.fetch = async (url, opts) => {
-        if (opts?.method === 'PUT') return new Response('', { status: 200 });
-        return new Response(JSON.stringify(serverJson), { status: 200 });
-      };
+      window.fetch = wrap(async (url, opts) => {
+        if (opts?.method === 'POST' || opts?.method === 'PUT') {
+          return new Response('', { status: 200, headers: { ETag: '"next"' } });
+        }
+        return new Response(JSON.stringify(serverJson), {
+          status: 200,
+          headers: { ETag: '"baseline"', 'Content-Type': 'application/json' },
+        });
+      });
 
       const sheets = [buildSheet('data', [['key'], ['a']])];
       const result = await saveSheets(sheets);
-      expect(result).to.be.true; // no false drift — save went through
+      expect(result).to.be.true;
     });
   });