fix: the theme preview says what it is doing, and says when it fails
Some checks failed
Forgejo Android APK / Root app tests (push) Successful in 46s
Forgejo Docker Build / Root app tests (push) Successful in 57s
Forgejo Android APK / Build signed APK (push) Successful in 2m4s
Forgejo Docker Build / Build Docker image (push) Successful in 19s
Forgejo Docker Build / Deploy to the host (push) Failing after 1s
Some checks failed
Forgejo Android APK / Root app tests (push) Successful in 46s
Forgejo Docker Build / Root app tests (push) Successful in 57s
Forgejo Android APK / Build signed APK (push) Successful in 2m4s
Forgejo Docker Build / Build Docker image (push) Successful in 19s
Forgejo Docker Build / Deploy to the host (push) Failing after 1s
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU
This commit is contained in:
parent
a505244b97
commit
8602c7bd14
2 changed files with 89 additions and 8 deletions
|
|
@ -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.
|
||||
|
|
|
|||
60
test/theme-preview-visible.test.js
Normal file
60
test/theme-preview-visible.test.js
Normal file
|
|
@ -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');
|
||||
});
|
||||
Loading…
Reference in a new issue