From 7d63742485ac3b29608aeed55fb03f3799ba5a8d Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Wed, 15 Jul 2026 00:13:42 +0200 Subject: [PATCH 1/5] test(sheet): cover saving before folder navigation --- test/e2e/tests/sheet.spec.js | 10 ++++++-- .../blocks/edit/da-title/da-title.test.js | 19 +++++++++++++++ test/unit/blocks/sheet/index.test.js | 21 +++++++++++++++++ test/unit/blocks/sheet/utils-utils.test.js | 23 ++++++++++++++++++- 4 files changed, 70 insertions(+), 3 deletions(-) diff --git a/test/e2e/tests/sheet.spec.js b/test/e2e/tests/sheet.spec.js index 212efbf9..87d12cd4 100644 --- a/test/e2e/tests/sheet.spec.js +++ b/test/e2e/tests/sheet.spec.js @@ -1,7 +1,7 @@ import { test, expect } from '@playwright/test'; import { getTestSheetURL, waitForSave } from '../utils/page.js'; -test('New sheet', async ({ page }, workerInfo) => { +test('Last cell edit persists when immediately navigating to the parent folder', async ({ page }, workerInfo) => { test.setTimeout(30000); const url = getTestSheetURL('sheet1', workerInfo); @@ -22,7 +22,13 @@ test('New sheet', async ({ page }, workerInfo) => { await page.locator('[data-x="0"][data-y="1"]').dblclick(); await page.locator('td input').fill(enteredText); - await page.waitForTimeout(3000); + // Leave while the cell editor is still active and before the autosave debounce expires. + await page.locator('.da-title-name-label').click(); + + await page.goto(url); + await expect(page.locator('da-sheet-tabs')).toBeVisible(); + await expect(page.locator('[data-x="0"][data-y="1"]')).toHaveText(enteredText); + await page.close(); }); diff --git a/test/unit/blocks/edit/da-title/da-title.test.js b/test/unit/blocks/edit/da-title/da-title.test.js index 7f799abd..7a41b14b 100644 --- a/test/unit/blocks/edit/da-title/da-title.test.js +++ b/test/unit/blocks/edit/da-title/da-title.test.js @@ -89,6 +89,25 @@ describe('DaTitle', () => { expect(h1.textContent).to.equal('page'); }); + it('waits for beforeNavigate before following the parent link', async () => { + el = await fixture(); + let release; + let called = false; + el.beforeNavigate = () => new Promise((resolve) => { + called = true; + release = resolve; + }); + + const link = el.shadowRoot.querySelector('.da-title-name-label'); + const event = new MouseEvent('click', { bubbles: true, cancelable: true }); + link.dispatchEvent(event); + + expect(event.defaultPrevented).to.be.true; + expect(called).to.be.true; + release(false); + await nextFrame(); + }); + it('renders actions wrapper', async () => { el = await fixture(); const actionsWrapper = el.shadowRoot.querySelector('.da-title-actions'); diff --git a/test/unit/blocks/sheet/index.test.js b/test/unit/blocks/sheet/index.test.js index 5b3db8dd..66f8a1c5 100644 --- a/test/unit/blocks/sheet/index.test.js +++ b/test/unit/blocks/sheet/index.test.js @@ -21,6 +21,27 @@ function mockJspreadsheetTabs(container, data) { container.jexcel = data.map((d) => ({ name: d.sheetName, options: {} })); } +describe('commitActiveEditors', () => { + it('commits only worksheets with an active editor', () => { + const activeCell = document.createElement('td'); + const calls = []; + const sheets = [ + { + edition: activeCell, + closeEditor: (...args) => calls.push(args), + }, + { + edition: null, + closeEditor: (...args) => calls.push(args), + }, + ]; + + sh.commitActiveEditors(sheets); + + expect(calls).to.deep.equal([[activeCell, true]]); + }); +}); + describe('init - double restore', () => { let el; let daTitle; diff --git a/test/unit/blocks/sheet/utils-utils.test.js b/test/unit/blocks/sheet/utils-utils.test.js index 0f39b59d..49781e2e 100644 --- a/test/unit/blocks/sheet/utils-utils.test.js +++ b/test/unit/blocks/sheet/utils-utils.test.js @@ -3,7 +3,12 @@ import { expect } from '@esm-bundle/chai'; const { setNx } = await import('../../../../scripts/utils.js'); setNx('/test/fixtures/nx', { hostname: 'example.com' }); -const { saveSheets, handleSave, staleCheck } = await import('../../../../blocks/sheet/utils/utils.js'); +const { + saveSheets, + handleSave, + flushPendingSave, + staleCheck, +} = await import('../../../../blocks/sheet/utils/utils.js'); // The new api.js makes an hlx6 upgrade probe before each source op. The probe // must respond without an x-api-upgrade-available header so the source URL @@ -133,5 +138,21 @@ describe('sheet/utils utils', () => { await new Promise((r) => { setTimeout(r, 1200); }); expect(captured).to.contain('/source/o/r/test'); }); + + it('Flushes a pending save immediately to its original path', async () => { + window.location.hash = '#/o/r/original'; + let captured; + window.fetch = wrap((url) => { + if (typeof url === 'string' && url.includes('/source/')) captured = url; + return Promise.resolve(new Response('', { status: 200 })); + }); + + handleSave([buildSheet('a', [['x']])], 'edit'); + window.location.hash = '#/o/r/other'; + const result = await flushPendingSave(); + + expect(result).to.be.true; + expect(captured).to.contain('/source/o/r/original'); + }); }); }); From c04fc37d3f34f453fa7de16fc15769523875f10c Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Wed, 15 Jul 2026 00:24:14 +0200 Subject: [PATCH 2/5] fix(sheet): flush edits before folder navigation --- blocks/edit/da-title/da-title.js | 9 +++++++++ blocks/sheet/sheet.js | 14 ++++++++++++-- blocks/sheet/utils/index.js | 6 ++++++ blocks/sheet/utils/utils.js | 32 ++++++++++++++++++++++++++------ 4 files changed, 53 insertions(+), 8 deletions(-) diff --git a/blocks/edit/da-title/da-title.js b/blocks/edit/da-title/da-title.js index 9fabeb43..18a66cae 100644 --- a/blocks/edit/da-title/da-title.js +++ b/blocks/edit/da-title/da-title.js @@ -215,6 +215,14 @@ export default class DaTitle extends LitElement { } } + async handleNavigate(e) { + const modified = e.button !== 0 || e.metaKey || e.ctrlKey || e.shiftKey || e.altKey; + if (!this.beforeNavigate || modified) return; + e.preventDefault(); + const { href } = e.currentTarget; + if (await this.beforeNavigate() !== false) window.location.href = href; + } + async handleAction(action) { this._status = null; this._isSending = true; @@ -417,6 +425,7 @@ export default class DaTitle extends LitElement {
${this.details.parentName}

${this.details.name}

diff --git a/blocks/sheet/sheet.js b/blocks/sheet/sheet.js index 49bae136..61a0252a 100644 --- a/blocks/sheet/sheet.js +++ b/blocks/sheet/sheet.js @@ -2,8 +2,13 @@ import { LitElement, html, nothing } from 'da-lit'; import getPathDetails from '../shared/pathDetails.js'; 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 { commitActiveEditors, getData } from './utils/index.js'; +import { + flushPendingSave, + staleCheck, + showDaDialog, + restoreVersion, +} from './utils/utils.js'; import { convertSheets } from '../edit/utils/helpers.js'; const { default: getStyle } = await import(`${getNx()}/utils/styles.js`); @@ -176,6 +181,11 @@ export default async function init(el) { daSheet.className = 'da-sheet'; wrapper.append(daSheet); + daTitle.beforeNavigate = () => { + commitActiveEditors(daSheet.jexcel); + return flushPendingSave().catch(() => false); + }; + // Sheet & Version Wrapper const versionWrapper = document.createElement('div'); versionWrapper.classList.add('da-version-wrapper'); diff --git a/blocks/sheet/utils/index.js b/blocks/sheet/utils/index.js index d39f3da3..a0491f78 100644 --- a/blocks/sheet/utils/index.js +++ b/blocks/sheet/utils/index.js @@ -10,6 +10,12 @@ const SHEET_TEMPLATE = { minDimensions: [20, 20], sheetName: 'data' }; let permissions; let canWrite; +export function commitActiveEditors(sheets) { + sheets?.forEach((sheet) => { + if (sheet.edition) sheet.closeEditor(sheet.edition, true); + }); +} + function resetSheets(el) { document.querySelector('da-sheet-tabs')?.remove(); if (!el.jexcel) return; diff --git a/blocks/sheet/utils/utils.js b/blocks/sheet/utils/utils.js index f9f2bc89..03ca461a 100644 --- a/blocks/sheet/utils/utils.js +++ b/blocks/sheet/utils/utils.js @@ -1,4 +1,4 @@ -import { convertSheets, debounce, saveToDa } from '../../edit/utils/helpers.js'; +import { convertSheets, saveToDa } from '../../edit/utils/helpers.js'; import { getNx2Api } from '../../../scripts/utils.js'; const DEBOUNCE_TIME = 1000; @@ -77,7 +77,7 @@ class StaleCheck { export const staleCheck = new StaleCheck(); -export const saveSheets = async (sheets) => { +export const saveSheets = async (sheets, pathname = window.location.hash.replace('#', '')) => { const convertedJson = convertSheets(sheets); document.querySelector('da-sheet-panes').data = convertedJson; @@ -85,8 +85,6 @@ export const saveSheets = async (sheets) => { // 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 @@ -97,7 +95,24 @@ export const saveSheets = async (sheets) => { return true; }; -const debouncedSaveSheets = debounce(saveSheets, DEBOUNCE_TIME); +let pendingSave; +let saveTimeout; +let savePromise = Promise.resolve(true); + +export function flushPendingSave() { + clearTimeout(saveTimeout); + saveTimeout = undefined; + + if (pendingSave) { + const { sheets, pathname } = pendingSave; + pendingSave = undefined; + savePromise = savePromise + .catch(() => false) + .then(() => saveSheets(sheets, pathname)); + } + + return savePromise; +} export async function restoreVersion(daTitle, daSheet, versionData) { const initSheet = (await import('./index.js')).default; @@ -110,7 +125,12 @@ export function handleSave(jexcel, view) { staleCheck.markEdited(); if (view === 'config') return; if (staleCheck.isBlocked) return; - debouncedSaveSheets(jexcel); + clearTimeout(saveTimeout); + pendingSave = { + sheets: jexcel, + pathname: window.location.hash.replace('#', ''), + }; + saveTimeout = setTimeout(flushPendingSave, DEBOUNCE_TIME); } export async function showDaDialog({ title, body, confirmLabel }) { From 6d66753b133281d2d0f270a0175390a08d291dda Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Wed, 15 Jul 2026 01:55:59 +0200 Subject: [PATCH 3/5] test(sheet): model jspreadsheet edition as [cell, ...] array commitActiveEditors' test modelled edition as the cell element, but jspreadsheet sets it to [cell, innerHTML, colIndex, rowIndex] while a cell editor is open. closeEditor expects the cell (edition[0]). The wrong model let the code pass the whole array to closeEditor without the test noticing, which is the webkit save-before-navigation failure. --- test/unit/blocks/sheet/index.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/blocks/sheet/index.test.js b/test/unit/blocks/sheet/index.test.js index 66f8a1c5..89ffa439 100644 --- a/test/unit/blocks/sheet/index.test.js +++ b/test/unit/blocks/sheet/index.test.js @@ -23,11 +23,13 @@ function mockJspreadsheetTabs(container, data) { describe('commitActiveEditors', () => { it('commits only worksheets with an active editor', () => { + // jspreadsheet sets `edition` to [cell, innerHTML, colIndex, rowIndex] while a + // cell editor is open; closeEditor expects the cell element (edition[0]). const activeCell = document.createElement('td'); const calls = []; const sheets = [ { - edition: activeCell, + edition: [activeCell, 'Key', 0, 1], closeEditor: (...args) => calls.push(args), }, { From ebaf67b9c44b530a1663b6805c988b898b7b4736 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Wed, 15 Jul 2026 01:57:26 +0200 Subject: [PATCH 4/5] fix(sheet): pass editor cell to closeEditor so edits save before folder nav commitActiveEditors passed the whole edition array to closeEditor, which expects the cell element (edition[0]) and calls edition.getAttribute. Passing the array threw, so beforeNavigate never reached flushPendingSave and the last edit was lost. It only surfaced on webkit: chromium and firefox blur-close the open cell editor before the click handler runs, so commitActiveEditors sees no active edition and the blur-triggered save flushes. Webkit leaves the editor open, so the bad call ran and threw. Passing edition[0] commits the open editor, which fires onafterchanges and lets the pending save flush. --- blocks/sheet/utils/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blocks/sheet/utils/index.js b/blocks/sheet/utils/index.js index a0491f78..a99907a1 100644 --- a/blocks/sheet/utils/index.js +++ b/blocks/sheet/utils/index.js @@ -12,7 +12,7 @@ let canWrite; export function commitActiveEditors(sheets) { sheets?.forEach((sheet) => { - if (sheet.edition) sheet.closeEditor(sheet.edition, true); + if (sheet.edition) sheet.closeEditor(sheet.edition[0], true); }); } From 6d084bdf2cf989edbf153b6551f24cda584ef3a4 Mon Sep 17 00:00:00 2001 From: Ben Peter Date: Wed, 15 Jul 2026 10:43:17 +0200 Subject: [PATCH 5/5] test(sheet): wait for leave-navigation before reloading The parent-folder link commits the open editor and flushes the save, then navigates to the parent folder. The test reloaded the sheet immediately after the click, racing that async save+navigation: webkit lost the save, and once the save was awaited, firefox landed on the parent folder instead of the sheet. Waiting for the app to leave the sheet view guarantees the save landed (beforeNavigate awaits it) before reopening the sheet to assert. Verified locally against the sheetfix preview on chromium, firefox and webkit, for both the da-sites/da-status and da-testautomation/da-e2e-tests org/site configs (the two CI suites). --- test/e2e/tests/sheet.spec.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/e2e/tests/sheet.spec.js b/test/e2e/tests/sheet.spec.js index 87d12cd4..67c016e4 100644 --- a/test/e2e/tests/sheet.spec.js +++ b/test/e2e/tests/sheet.spec.js @@ -23,7 +23,11 @@ test('Last cell edit persists when immediately navigating to the parent folder', await page.locator('td input').fill(enteredText); // Leave while the cell editor is still active and before the autosave debounce expires. + // The parent link commits the open editor and flushes the save, then navigates to the + // parent folder. Waiting for that navigation guarantees the save landed (beforeNavigate + // awaits it) and avoids racing it with the reload below. await page.locator('.da-title-name-label').click(); + await page.waitForURL((u) => !String(u).includes('/sheet#')); await page.goto(url); await expect(page.locator('da-sheet-tabs')).toBeVisible();