From 8602c7bd144c7740af71f46b05704f91475f1e93 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 21:56:22 +0200 Subject: [PATCH] fix: the theme preview says what it is doing, and says when it fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The box was hidden until an image loaded and hidden again on error, so a preview that failed and a preview that did not exist looked identical — an empty gap under the dropdown. That is what it looked like from the outside, and it cost a diagnosis: the endpoint renders a correct 934x525 PNG in 0.66s when called exactly as the browser calls it, cookie and all, and nothing on the page could have told anyone that. The first view of a theme is a pptx render, a Gotenberg round trip and a pdftoppm — a second or so before anything appears, and then cached on disk. A second of silence reads as a feature that is not there, so it now says it is drawing, and on failure says the look itself is unaffected, which is true: the preview is a picture of the export, not the thing that produces it. img.src is assigned after both handlers. A cached preview can complete before a handler attached below the assignment exists, which would leave the box stuck on "Drawing...". describeTheme already ends by calling it, so the call sitting beside it fired a second identical request on every load. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU --- public/js/myResources.js | 37 ++++++++++++++---- test/theme-preview-visible.test.js | 60 ++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 test/theme-preview-visible.test.js diff --git a/public/js/myResources.js b/public/js/myResources.js index 30ba0d0b..904bf234 100644 --- a/public/js/myResources.js +++ b/public/js/myResources.js @@ -167,7 +167,6 @@ } describeTheme(themes); themeSelect.onchange = function () { describeTheme(themes); }; - showThemePreview(); } if (themeRow) themeRow.hidden = themes.length < 2; @@ -213,18 +212,40 @@ // read as a leak. // A rendered slide in the chosen palette. The renderer draws it, so it cannot // promise a look the export does not deliver. + // + // The first view of a theme costs a pptx render, a Gotenberg round trip and a + // pdftoppm — a second or so — and after that it is served from a disk cache. + // So it says it is working, rather than leaving an empty gap that reads as a + // feature that is not there. + // + // A failure says so too. Hiding the box on error, which is what this did, + // makes a broken preview and an absent one look identical, from both sides of + // the screen: nothing to see, and nothing to report. function showThemePreview() { var select = document.getElementById('mr-theme'); var box = document.getElementById('mr-theme-preview'); - if (!select || !box) return; + if (!select || !select.value || !box) return; + var url = '/api/my-resources/theme-preview/' + encodeURIComponent(select.value); + + var note = box.querySelector('p') || document.createElement('p'); + note.style.cssText = 'margin:0;font-size:12px;color:var(--g500);'; + note.textContent = 'Drawing a sample slide...'; + if (!note.parentNode) box.appendChild(note); + var img = box.querySelector('img') || document.createElement('img'); - img.alt = 'A slide rendered in the ' + (select.options[select.selectedIndex] || {}).text + ' theme'; - img.loading = 'lazy'; - img.style.cssText = 'max-width:min(100%,340px);border:1px solid var(--g200);border-radius:8px;display:block;'; - img.onerror = function () { box.hidden = true; }; - img.onload = function () { box.hidden = false; }; - img.src = '/api/my-resources/theme-preview/' + encodeURIComponent(select.value); + img.alt = 'A slide rendered in the ' + ((select.options[select.selectedIndex] || {}).text || 'chosen') + ' theme'; + img.style.cssText = 'max-width:min(100%,340px);border:1px solid var(--g200);border-radius:8px;display:none;'; + img.onload = function () { img.style.display = 'block'; note.hidden = true; }; + img.onerror = function () { + img.style.display = 'none'; + note.hidden = false; + note.textContent = 'No preview just now — the look itself is unaffected.'; + }; + // Assigned last: in a cache hit, onload can fire before the handlers below + // the assignment would have been attached. + img.src = url; if (!img.parentNode) box.appendChild(img); + box.hidden = false; } // Says what the chosen theme is for, which is the part a name cannot carry. diff --git a/test/theme-preview-visible.test.js b/test/theme-preview-visible.test.js new file mode 100644 index 00000000..aa6d7997 --- /dev/null +++ b/test/theme-preview-visible.test.js @@ -0,0 +1,60 @@ +// The theme preview box was hidden on error and hidden until an image loaded, +// so a preview that failed and a preview that did not exist looked exactly the +// same — an empty gap. That cost a diagnosis: the endpoint was working the +// whole time and there was no way to tell from the page. +// +// The first view of a theme is a pptx render, a Gotenberg round trip and a +// pdftoppm, so it is a second or so before anything appears. Silence for that +// second reads as a missing feature. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const read = f => fs.readFileSync(path.join(__dirname, '..', f), 'utf8'); +const ui = read('public/js/myResources.js'); +const fn = ui.slice(ui.indexOf('function showThemePreview'), ui.indexOf('function describeTheme')); + +test('a failed preview says so instead of vanishing', () => { + assert.doesNotMatch(fn, /onerror = function \(\) \{ box\.hidden = true; \}/); + assert.match(fn, /img\.onerror = function \(\)[\s\S]{0,200}note\.textContent = 'No preview just now/); +}); + +test('it says it is working while the render happens', () => { + assert.match(fn, /note\.textContent = 'Drawing a sample slide\.\.\.'/); + assert.match(fn, /box\.hidden = false/); +}); + +test('the note is replaced by the image, not left stacked above it', () => { + assert.match(fn, /img\.onload = function \(\) \{ img\.style\.display = 'block'; note\.hidden = true; \}/); +}); + +test('src is assigned after the handlers, so a cache hit is not missed', () => { + // A cached preview can complete before a handler attached after the + // assignment would exist, leaving the box stuck on "Drawing...". + const onload = fn.indexOf('img.onload'); + const onerror = fn.indexOf('img.onerror'); + const src = fn.indexOf('img.src ='); + assert.ok(onload > -1 && onerror > -1 && src > -1); + assert.ok(src > onload && src > onerror, 'img.src must be set after both handlers'); +}); + +test('no request for a theme that has not been chosen', () => { + assert.match(fn, /if \(!select \|\| !select\.value \|\| !box\) return;/); +}); + +test('the preview is requested once per change, not twice', () => { + // describeTheme ends by calling it; a second call beside it fired an + // identical request on every load. + assert.equal((ui.match(/\n\s*showThemePreview\(\);/g) || []).length, 1); +}); + +test('the route it calls exists and is the one that renders', () => { + const route = read('src/routes/myResources.js'); + assert.match(fn, /'\/api\/my-resources\/theme-preview\/' \+ encodeURIComponent\(select\.value\)/); + assert.match(route, /router\.get\('\/my-resources\/theme-preview\/:id'/); + // It must be registered before the '/my-resources/:id' catch-all, or that + // would answer instead. + assert.ok(route.indexOf("'/my-resources/theme-preview/:id'") < route.indexOf("router.get('/my-resources/:id'"), + 'theme-preview must be registered before the :id route'); +});