Improve attachments section test (#5576)

Cancel pending discard to avoid race condition
Cancel pending render/discard after tests

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');

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<void>}

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() {

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();
 		});