From 9d91ff8df98ade1817ac92c195e67480991cc02c Mon Sep 17 00:00:00 2001 From: windingwind <33902321+windingwind@users.noreply.github.com> Date: Mon, 13 Oct 2025 19:18:51 +0200 Subject: [PATCH] Improve attachments section test (#5576) Cancel pending discard to avoid race condition Cancel pending render/discard after tests --- .../content/zotero/elements/attachmentBox.js | 78 ++++++++++++------- .../zotero/elements/attachmentPreview.js | 16 ++++ .../content/zotero/elements/attachmentsBox.js | 48 ++++++++++-- test/tests/itemPaneTest.js | 32 ++++++-- 4 files changed, 133 insertions(+), 41 deletions(-) diff --git a/chrome/content/zotero/elements/attachmentBox.js b/chrome/content/zotero/elements/attachmentBox.js index a24770de6c..ecc166fc84 100644 --- a/chrome/content/zotero/elements/attachmentBox.js +++ b/chrome/content/zotero/elements/attachmentBox.js @@ -93,6 +93,10 @@ _previewDiscarded = false; + _discardTimeoutID = null; + + _pendingDiscardPreviewRenderId = null; + constructor() { super(); @@ -293,7 +297,11 @@ } destroy() { - this.discard(); + this._cancelPendingDiscard(); + if (this._preview) { + this._preview._clearPendingTasks(); + this._preview.discard?.(); + } this._preview?.remove(); delete this._preview; @@ -325,8 +333,10 @@ } if (event != 'modify' || !this.item?.id || !ids.includes(this.item.id)) return; - // Wait for the render finish and then refresh - this._waitForRender(this._forceRenderAll.bind(this)); + Promise.all([ + this.updateInfo(), + this.updatePreview() + ]); } async asyncRender() { @@ -338,6 +348,7 @@ this.previewElem.render(); } this._lastPreviewRenderId = `${Date.now()}-${Math.random()}`; + this._cancelPendingDiscard(); return; } @@ -353,19 +364,37 @@ this._asyncRendering = false; this._lastPreviewRenderId = `${Date.now()}-${Math.random()}`; + this._cancelPendingDiscard(); } discard() { if (!this._preview) return; - let lastPreviewRenderId = this._lastPreviewRenderId; - setTimeout(() => { - if (!this._asyncRendering && this._lastPreviewRenderId === lastPreviewRenderId) { + + this._cancelPendingDiscard(); + + this._pendingDiscardPreviewRenderId = this._lastPreviewRenderId; + + this._discardTimeoutID = setTimeout(() => { + this._discardTimeoutID = null; + + if (!this._asyncRendering + && this._pendingDiscardPreviewRenderId === this._lastPreviewRenderId) { this._preview?.discard(); this._previewDiscarded = true; } + + this._pendingDiscardPreviewRenderId = null; }, this._discardPreviewTimeout); } + _cancelPendingDiscard() { + if (this._discardTimeoutID) { + clearTimeout(this._discardTimeoutID); + this._discardTimeoutID = null; + } + this._pendingDiscardPreviewRenderId = null; + } + onViewClick(event) { ZoteroPane_Local.viewAttachment(this.item.id, event, !this.editable); } @@ -375,6 +404,7 @@ } async updateInfo() { + if (!this.initialized) return; // Cancel editing filename when refreshing this._isEditingFilename = false; @@ -530,10 +560,19 @@ } async updatePreview() { - if (this.usePreview) { - this.previewElem.item = this.item; - await this.previewElem.render(); + if (!this.initialized) return; + if (!this.usePreview + // Skip only when the section is manually collapsed (when there's attachment), + // This is necessary to ensure the rendering of the first added attachment + // because the section is force-collapsed if no attachment. + || !this._section?.open || !this.item) { + return; } + this.previewElem.item = this.item; + await this.previewElem.render(); + this._lastPreviewRenderId = `${Date.now()}-${Math.random()}`; + + this._cancelPendingDiscard(); } updateItemIndexedState() { @@ -733,27 +772,6 @@ return this.querySelector(`#${id}`); } - async _waitForRender(callback) { - let resolve, reject; - Promise.race([new Promise(((res, rej) => { - resolve = res; - reject = rej; - })), Zotero.Promise.delay(3000)]).then(() => callback()); - let i = 0; - let finished = false; - // Wait for render to finish - while (i < 100) { - if (!this._asyncRendering) { - finished = true; - break; - } - await Zotero.Promise.delay(10); - i++; - } - if (finished) resolve(); - else reject(new Error("AttachmentBox#_waitForRender timeout")); - } - _initPreview() { this._preview = document.createXULElement('attachment-preview'); this._preview.setAttribute('tabindex', '0'); diff --git a/chrome/content/zotero/elements/attachmentPreview.js b/chrome/content/zotero/elements/attachmentPreview.js index dac28ad9f0..7f6062dcb8 100644 --- a/chrome/content/zotero/elements/attachmentPreview.js +++ b/chrome/content/zotero/elements/attachmentPreview.js @@ -301,6 +301,22 @@ await this._processTask(); } + /** + * Clear all pending tasks and reset processing states. + */ + _clearPendingTasks() { + try { + this._lastTask = null; + this._isProcessingTask = false; + this._isRendering = false; + this._isDiscarding = false; + } + catch (e) { + // Ignore errors during cleanup to prevent cascading failures + this._debug(`Error during task cleanup: ${e.message}`); + } + } + /** * Process the most recent task * @returns {Promise} diff --git a/chrome/content/zotero/elements/attachmentsBox.js b/chrome/content/zotero/elements/attachmentsBox.js index 038cd1dd88..9796c0ddd4 100644 --- a/chrome/content/zotero/elements/attachmentsBox.js +++ b/chrome/content/zotero/elements/attachmentsBox.js @@ -54,6 +54,10 @@ _previewDiscarded = false; + _discardTimeoutID = null; + + _pendingDiscardPreviewRenderId = null; + get item() { return this._item; } @@ -134,7 +138,11 @@ } destroy() { - this.discard(); + this._cancelPendingDiscard(); + if (this._preview) { + this._preview._clearPendingTasks(); + this._preview.discard?.(); + } this._preview?.remove(); delete this._preview; @@ -204,9 +212,17 @@ if (this._isAlreadyRendered("async")) { if (this._previewDiscarded) { this._previewDiscarded = false; - this.previewElem.render(); + // Only re-render preview if conditions allow it + // This prevents creating preview DOM when box is invisible + if (this.initialized && this._renderStage === "final") { + let attachment = await this._getPreviewAttachment(); + if (attachment && this.usePreview && (!this._attachmentIDs.length || this._section?.open)) { + this.previewElem.render(); + } + } } this._lastPreviewRenderId = `${Date.now()}-${Math.random()}`; + this._cancelPendingDiscard(); return; } this._renderStage = "final"; @@ -223,15 +239,32 @@ discard() { if (!this._preview) return; - let lastPreviewRenderId = this._lastPreviewRenderId; - setTimeout(() => { - if (!this._asyncRendering && this._lastPreviewRenderId === lastPreviewRenderId) { + + this._cancelPendingDiscard(); + + this._pendingDiscardPreviewRenderId = this._lastPreviewRenderId; + + this._discardTimeoutID = setTimeout(() => { + this._discardTimeoutID = null; + + if (!this._asyncRendering + && this._pendingDiscardPreviewRenderId === this._lastPreviewRenderId) { this._preview?.discard(); this._previewDiscarded = true; } + + this._pendingDiscardPreviewRenderId = null; }, this._discardPreviewTimeout); } - + + _cancelPendingDiscard() { + if (this._discardTimeoutID) { + clearTimeout(this._discardTimeoutID); + this._discardTimeoutID = null; + } + this._pendingDiscardPreviewRenderId = null; + } + updateCount() { if (!this._item?.isRegularItem()) { return; @@ -260,6 +293,7 @@ let attachment = await this._getPreviewAttachment(); this.toggleAttribute('data-use-preview', !!attachment && Zotero.Prefs.get('showAttachmentPreview')); if (!attachment) { + this._cancelPendingDiscard(); return; } if (!this.usePreview @@ -272,6 +306,8 @@ this.previewElem.item = attachment; await this.previewElem.render(); this._lastPreviewRenderId = `${Date.now()}-${Math.random()}`; + + this._cancelPendingDiscard(); } async _getPreviewAttachment() { diff --git a/test/tests/itemPaneTest.js b/test/tests/itemPaneTest.js index 2c8263ec0d..158b50390a 100644 --- a/test/tests/itemPaneTest.js +++ b/test/tests/itemPaneTest.js @@ -5,7 +5,7 @@ describe("Item pane", function () { let res = await waitForCallback( () => box._asyncRenderItemID && !box._asyncRendering && (!itemID || box._asyncRenderItemID == itemID), - 100, 3); + 100, 10); if (res instanceof Error) { throw res; } @@ -19,7 +19,7 @@ describe("Item pane", function () { let res = await waitForCallback( () => preview._reader?.itemID == itemID && !preview._isProcessingTask && !preview._lastTask - , 100, 3); + , 100, 10); if (res instanceof Error) { throw res; } @@ -736,6 +736,17 @@ describe("Item pane", function () { }); afterEach(function () { + // Ensure all previews are properly discarded and cleaned up + let itemDetails = ZoteroPane.itemPane._itemDetails; + let attachmentsBox = itemDetails.getPane(paneID); + + // Force cleanup of any pending operations and queued tasks + if (attachmentsBox._preview) { + attachmentsBox._cancelPendingDiscard?.(); + attachmentsBox._preview._clearPendingTasks(); + attachmentsBox._preview.discard?.(); + } + Zotero_Tabs.select("zotero-pane"); Zotero_Tabs.closeAll(); }); @@ -1305,7 +1316,7 @@ describe("Item pane", function () { const discardTimeout = 50; - // Temporarily set discard timeout to 100ms for testing + // Temporarily set discard timeout for testing let currentDiscardTimeout = attachmentsBox._discardPreviewTimeout; attachmentsBox._discardPreviewTimeout = discardTimeout; @@ -1321,8 +1332,8 @@ describe("Item pane", function () { // Scroll the attachments pane out of view await waitForScrollToPane(itemDetails, 'info'); - // Wait a bit for the preview to be discarded - await Zotero.Promise.delay(discardTimeout + 100); + // Wait for the intersection observer to trigger discard and the discard process to complete + await waitForCallback(() => !attachmentsBox._preview._isReaderInitialized); assert.isFalse(attachmentsBox._preview._isReaderInitialized); @@ -1539,6 +1550,17 @@ describe("Item pane", function () { }); afterEach(function () { + // Ensure all previews are properly discarded and cleaned up + let itemDetails = ZoteroPane.itemPane._itemDetails; + let attachmentBox = itemDetails.getPane(paneID); + + // Force cleanup of any pending operations and queued tasks + if (attachmentBox._preview) { + attachmentBox._cancelPendingDiscard?.(); + attachmentBox._preview._clearPendingTasks(); + attachmentBox._preview.discard?.(); + } + Zotero_Tabs.select("zotero-pane"); Zotero_Tabs.closeAll(); });