diff --git a/src/MediaManager.html b/src/MediaManager.html index 5ccd934..e415077 100644 --- a/src/MediaManager.html +++ b/src/MediaManager.html @@ -1206,8 +1206,8 @@ */ var controller = { init() { - // Start polling for SKU selection - setInterval(() => this.checkSku(), 2000); + // Initialize by checking SKU once + // Since this is a modal, the selection cannot change during the session. this.checkSku(); }, @@ -1222,10 +1222,19 @@ this.loadMedia(); } else if (!sku && !state.sku) { if (document.getElementById('error-ui').style.display !== 'flex') { - this.loadMedia(); + this.loadMedia(); // Likely to trigger error UI } } }) + .withFailureHandler(e => { + console.warn("SKU check failed", e); + // If it fails once at startup, we probably should alert or retry once, + // but for now let's just leave it. If it fails, the UI might hang on "Loading..." + // potentially better to trigger error UI? + if (document.getElementById('loading-ui').style.display !== 'none') { + alert("Failed to load product info: " + e.message); + } + }) .getSelectedProductInfo(); }, diff --git a/src/mediaHandlers.test.ts b/src/mediaHandlers.test.ts index d479bdd..1d4243c 100644 --- a/src/mediaHandlers.test.ts +++ b/src/mediaHandlers.test.ts @@ -449,6 +449,29 @@ describe("mediaHandlers", () => { }) test("getSelectedProductInfo should return sku and title from sheet", () => { + // Mock SpreadsheetApp behavior specifically for the optimized implementation + // The implementation calls: + // 1. sheet.getRange(1, 1, 1, lastCol).getValues()[0] (headers) + // 2. sheet.getRange(row, 1, 1, lastCol).getValues()[0] (values) + + const mockRange = { + getValues: jest.fn() + }; + + const mockSheet = { + getName: jest.fn().mockReturnValue("product_inventory"), + getLastColumn: jest.fn().mockReturnValue(3), + getActiveRange: jest.fn().mockReturnValue({ getRow: () => 5 }), + getRange: jest.fn().mockReturnValue(mockRange) + }; + + (global.SpreadsheetApp.getActiveSheet as jest.Mock).mockReturnValue(mockSheet); + + // First call: Headers + mockRange.getValues.mockReturnValueOnce([["sku", "title", "thumbnail"]]); + // Second call: Row Values + mockRange.getValues.mockReturnValueOnce([["TEST-SKU", "Test Product Title", "thumb.jpg"]]); + const info = getSelectedProductInfo() expect(info).toEqual({ sku: "TEST-SKU", title: "Test Product Title" }) }) diff --git a/src/mediaHandlers.ts b/src/mediaHandlers.ts index 42db775..9217bec 100644 --- a/src/mediaHandlers.ts +++ b/src/mediaHandlers.ts @@ -17,14 +17,34 @@ export function showMediaManager() { export function getSelectedProductInfo(): { sku: string, title: string } | null { const ss = new GASSpreadsheetService() + + // Optimization: Direct usage to avoid multiple service calls overhead + // Use SpreadsheetApp only once if possible to get active context const sheet = SpreadsheetApp.getActiveSheet() if (sheet.getName() !== "product_inventory") return null const row = sheet.getActiveRange().getRow() if (row <= 1) return null // Header - const sku = ss.getCellValueByColumnName("product_inventory", row, "sku") - const title = ss.getCellValueByColumnName("product_inventory", row, "title") + // Optimization: Get the whole row values in one go + // We need to know which index is SKU and Title. + // Getting headers once is cheaper than searching by name twice if we cache or just linear scan once. + // Actually, getCellValueByColumnName does: getSheet -> getHeaders (read) -> getRowData (read). + // Doing it twice = 6 operations. + // Let's do it manually efficiently: + + const headers = sheet.getRange(1, 1, 1, sheet.getLastColumn()).getValues()[0] as string[]; + const skuIdx = headers.indexOf("sku"); + const titleIdx = headers.indexOf("title"); + + if (skuIdx === -1) return null; // No SKU column + + // Read the specific row + // getRange(row, 1, 1, lastCol) + const rowValues = sheet.getRange(row, 1, 1, sheet.getLastColumn()).getValues()[0]; + + const sku = rowValues[skuIdx]; + const title = titleIdx !== -1 ? rowValues[titleIdx] : ""; return sku ? { sku: String(sku), title: String(title || "") } : null }