diff --git a/blocks/edit/da-title/da-title.js b/blocks/edit/da-title/da-title.js index 9fabeb43..79052b3f 100644 --- a/blocks/edit/da-title/da-title.js +++ b/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 diff --git a/blocks/sheet/sheet.js b/blocks/sheet/sheet.js index 7ca441de..36a08f21 100644 --- a/blocks/sheet/sheet.js +++ b/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 { loadStyle } = await import(`${getNx()}/utils/utils.js`); @@ -125,7 +124,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(); @@ -189,11 +188,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); diff --git a/blocks/sheet/utils/index.js b/blocks/sheet/utils/index.js index e4e62287..2e40ee09 100644 --- a/blocks/sheet/utils/index.js +++ b/blocks/sheet/utils/index.js @@ -108,15 +108,18 @@ 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) { isVersion = true; resp = await versions.get({ org, site, path, versionId }); } else if (view === 'config') { - resp = await config.get({ org, site }); + resp = await config.get({ org, site, cachebust: true }); } else { - resp = await source.get({ org, site, path }); + resp = await source.get({ org, site, path, cachebust: true }); } // Set permissions even if the file is a 404 @@ -134,7 +137,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; } diff --git a/blocks/sheet/utils/utils.js b/blocks/sheet/utils/utils.js index f9f2bc89..07859f61 100644 --- a/blocks/sheet/utils/utils.js +++ b/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,21 +55,43 @@ 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; const resp = view === 'config' - ? await config.get({ org, site }) - : await source.get({ org, site, path }); + ? await config.get({ org, site, cachebust: true }) + : await source.get({ org, site, path, cachebust: true }); if (!resp.ok) return false; + 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,8 +104,7 @@ 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. @@ -87,14 +112,16 @@ export const saveSheets = async (sheets) => { 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); diff --git a/test/unit/blocks/sheet/utils-utils.test.js b/test/unit/blocks/sheet/utils-utils.test.js index 0f39b59d..b7d58f69 100644 --- a/test/unit/blocks/sheet/utils-utils.test.js +++ b/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; }); });