mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-09-12 03:58:32 +00:00
fix(folder_library): re-window on resize and on show/hide (PR #967 review)
CodeRabbit caught two real bugs in the first pass. Both are mine. 1. GRID RESIZE. perRow and rows were captured once when the list was filled, but paint() also runs on resize — and resizing changes the grid's column count. The window maths then sliced against the OLD column count: wrong songs on screen, and padding sized for a row count the layout no longer had (so the scrollbar lied). metrics() now recomputes perRow/itemH/rows together on every paint, so the geometry can never disagree with itself. 2. STALE WINDOWS ON SHOW/HIDE. paint() only ran on scroll and resize. Expanding or collapsing any section moves every list below it, and a windowed list's contents are a function of its POSITION — so those lists kept the window from their old position and showed blank padding where songs should be until the user happened to scroll. Both toggles now call _repaintVirtualLists(). Re-opening an already-populated section had the same flaw. Collapsed lists also kept doing layout work on every scroll tick. paint() now bails early when the list is display:none or detached, and forgets its last window so re-showing repaints from scratch instead of short-circuiting on a stale memo. Tests: grid re-window on a column-count change, the padding+rendered=rows invariant at two different perRow values, and a test that PINS THE FAILURE MODE — a mismatched perRow/rows pair must not silently look correct. 12/12. Re-validated the DOM glue in real Chromium with 50k rows (25-31 rows rendered, scroll height exact). eslint clean; JS 1189/1189; pytest 2597 passed. CHANGELOG entry added (also flagged).
This commit is contained in:
@@ -46,6 +46,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
carry their gig log; instruments their gig count.
|
carry their gig log; instruments their gig count.
|
||||||
|
|
||||||
### Changed
|
### Changed
|
||||||
|
- **Folder library renders only the songs on screen** (#965) — a song list used to
|
||||||
|
render *every* song it held. On a flat 50,944-song library that was one `<div>`
|
||||||
|
with 50,938 children and ~1.3 **million** DOM nodes (~4.2 GB of renderer memory),
|
||||||
|
built even while another screen was showing. A document that size also punishes
|
||||||
|
unrelated code: any `document.querySelector` that misses has to walk the whole
|
||||||
|
tree — which is how the song-preview menu check ended up eating ~50% of the
|
||||||
|
renderer and dropping the app to 2.7 fps. Lists longer than 200 songs are now
|
||||||
|
windowed (25–31 rows in the DOM instead of 50,000); shorter lists are unchanged.
|
||||||
- **The full mix is a stem** (#933) — core no longer depends on `original_audio:`, a
|
- **The full mix is a stem** (#933) — core no longer depends on `original_audio:`, a
|
||||||
top-level manifest key this repo invented (#583) that the feedpak spec never had.
|
top-level manifest key this repo invented (#583) that the feedpak spec never had.
|
||||||
The format already carried the pre-separation mixdown as a stem; feedpak 1.15.0
|
The format already carried the pre-separation mixdown as a stem; feedpak 1.15.0
|
||||||
|
|||||||
@@ -895,6 +895,7 @@ function createFolderSurface(cfg) {
|
|||||||
var VIRTUAL_MIN = 200; // below this, render everything — no behaviour change
|
var VIRTUAL_MIN = 200; // below this, render everything — no behaviour change
|
||||||
var VIRTUAL_BUFFER = 6; // rows kept rendered above/below the viewport
|
var VIRTUAL_BUFFER = 6; // rows kept rendered above/below the viewport
|
||||||
var _virtualCleanups = [];
|
var _virtualCleanups = [];
|
||||||
|
var _virtualLists = []; // repaint fns, one per live windowed list
|
||||||
|
|
||||||
// Which slice of the list is on screen. Pure arithmetic — kept separate from
|
// Which slice of the list is on screen. Pure arithmetic — kept separate from
|
||||||
// the DOM so it can be tested directly (see tests/virtual_list.test.js).
|
// the DOM so it can be tested directly (see tests/virtual_list.test.js).
|
||||||
@@ -926,6 +927,7 @@ function createFolderSurface(cfg) {
|
|||||||
function _clearVirtualLists() {
|
function _clearVirtualLists() {
|
||||||
_virtualCleanups.forEach(function (fn) { try { fn(); } catch (_) {} });
|
_virtualCleanups.forEach(function (fn) { try { fn(); } catch (_) {} });
|
||||||
_virtualCleanups = [];
|
_virtualCleanups = [];
|
||||||
|
_virtualLists = [];
|
||||||
}
|
}
|
||||||
|
|
||||||
// Fill `list` with `songs`, windowed when the list is big enough to matter.
|
// Fill `list` with `songs`, windowed when the list is big enough to matter.
|
||||||
@@ -942,51 +944,80 @@ function createFolderSurface(cfg) {
|
|||||||
var basePadBot = parseFloat(window.getComputedStyle(list).paddingBottom) || 0;
|
var basePadBot = parseFloat(window.getComputedStyle(list).paddingBottom) || 0;
|
||||||
|
|
||||||
// Measure one real row once — no hardcoded row height to drift out of
|
// Measure one real row once — no hardcoded row height to drift out of
|
||||||
// sync with the CSS.
|
// sync with the CSS. (The list is shown before it is populated, so this
|
||||||
|
// measures a laid-out row, not a zero-height one.)
|
||||||
var probe = make(sorted[0]);
|
var probe = make(sorted[0]);
|
||||||
probe.style.visibility = 'hidden';
|
probe.style.visibility = 'hidden';
|
||||||
list.appendChild(probe);
|
list.appendChild(probe);
|
||||||
var itemH = probe.getBoundingClientRect().height || 44;
|
var probeRect = probe.getBoundingClientRect();
|
||||||
var perRow = 1;
|
var rowH = probeRect.height || 44;
|
||||||
if (_view === 'grid') {
|
var cardW = probeRect.width || 150;
|
||||||
var cw = probe.getBoundingClientRect().width || 150;
|
|
||||||
var gap = 12;
|
|
||||||
perRow = Math.max(1, Math.floor((list.clientWidth + gap) / (cw + gap)));
|
|
||||||
itemH += gap;
|
|
||||||
}
|
|
||||||
list.removeChild(probe);
|
list.removeChild(probe);
|
||||||
|
|
||||||
var rows = Math.ceil(sorted.length / perRow);
|
var GRID_GAP = 12; // matches the grid's `gap:12px`
|
||||||
var raf = 0, lastStart = -1, lastEnd = -1;
|
var raf = 0, lastStart = -1, lastEnd = -1;
|
||||||
|
|
||||||
|
// Recomputed on EVERY paint, not captured once: a window resize changes
|
||||||
|
// the grid's column count, and therefore the row count and the height of
|
||||||
|
// the padding standing in for off-window rows. paint() runs on resize, so
|
||||||
|
// stale metrics would slice the wrong songs and mis-size the list.
|
||||||
|
function metrics() {
|
||||||
|
var perRow = 1, itemH = rowH;
|
||||||
|
if (_view === 'grid') {
|
||||||
|
perRow = Math.max(1, Math.floor((list.clientWidth + GRID_GAP) / (cardW + GRID_GAP)));
|
||||||
|
itemH = rowH + GRID_GAP;
|
||||||
|
}
|
||||||
|
return { perRow: perRow, itemH: itemH, rows: Math.ceil(sorted.length / perRow) };
|
||||||
|
}
|
||||||
|
|
||||||
function paint() {
|
function paint() {
|
||||||
raf = 0;
|
raf = 0;
|
||||||
|
// Collapsed (display:none) or detached: nothing to paint, and don't
|
||||||
|
// pay for layout on every scroll tick of a section nobody can see.
|
||||||
|
// Forget the last window so re-showing repaints from scratch against
|
||||||
|
// the new position rather than short-circuiting on a stale memo.
|
||||||
|
if (!list.isConnected || list.offsetParent === null) {
|
||||||
|
lastStart = -1; lastEnd = -1;
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
var m = metrics();
|
||||||
// Where the list sits relative to the scroller's viewport.
|
// Where the list sits relative to the scroller's viewport.
|
||||||
var top = list.getBoundingClientRect().top - scroller.getBoundingClientRect().top;
|
var top = list.getBoundingClientRect().top - scroller.getBoundingClientRect().top;
|
||||||
var vh = scroller.clientHeight || window.innerHeight;
|
var vh = scroller.clientHeight || window.innerHeight;
|
||||||
var w = _visibleWindow(top, vh, itemH, perRow, rows, sorted.length);
|
var w = _visibleWindow(top, vh, m.itemH, m.perRow, m.rows, sorted.length);
|
||||||
if (w.start === lastStart && w.end === lastEnd) return; // nothing moved
|
if (w.start === lastStart && w.end === lastEnd) return; // nothing moved
|
||||||
lastStart = w.start; lastEnd = w.end;
|
lastStart = w.start; lastEnd = w.end;
|
||||||
|
|
||||||
var frag = document.createDocumentFragment();
|
var frag = document.createDocumentFragment();
|
||||||
for (var i = w.start; i < w.end; i++) frag.appendChild(make(sorted[i]));
|
for (var i = w.start; i < w.end; i++) frag.appendChild(make(sorted[i]));
|
||||||
list.textContent = '';
|
list.textContent = '';
|
||||||
list.style.paddingTop = (basePadTop + w.padRowsTop * itemH) + 'px';
|
list.style.paddingTop = (basePadTop + w.padRowsTop * m.itemH) + 'px';
|
||||||
list.style.paddingBottom = (basePadBot + w.padRowsBottom * itemH) + 'px';
|
list.style.paddingBottom = (basePadBot + w.padRowsBottom * m.itemH) + 'px';
|
||||||
list.appendChild(frag);
|
list.appendChild(frag);
|
||||||
}
|
}
|
||||||
function onScroll() { if (!raf) raf = window.requestAnimationFrame(paint); }
|
function schedule() { if (!raf) raf = window.requestAnimationFrame(paint); }
|
||||||
|
|
||||||
scroller.addEventListener('scroll', onScroll, { passive: true });
|
scroller.addEventListener('scroll', schedule, { passive: true });
|
||||||
window.addEventListener('resize', onScroll);
|
window.addEventListener('resize', schedule);
|
||||||
|
// Expanding or collapsing ANY section moves every list below it. Those
|
||||||
|
// lists' windows are computed from their position, so they must repaint
|
||||||
|
// too — otherwise they keep the window from their old position and show
|
||||||
|
// blank padding where songs should be until the user happens to scroll.
|
||||||
|
_virtualLists.push(schedule);
|
||||||
_virtualCleanups.push(function () {
|
_virtualCleanups.push(function () {
|
||||||
scroller.removeEventListener('scroll', onScroll);
|
scroller.removeEventListener('scroll', schedule);
|
||||||
window.removeEventListener('resize', onScroll);
|
window.removeEventListener('resize', schedule);
|
||||||
if (raf) window.cancelAnimationFrame(raf);
|
if (raf) window.cancelAnimationFrame(raf);
|
||||||
});
|
});
|
||||||
paint();
|
paint();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Re-window every live list — call after anything that can move them
|
||||||
|
// vertically (a folder expanding/collapsing, a section being shown).
|
||||||
|
function _repaintVirtualLists() {
|
||||||
|
_virtualLists.forEach(function (fn) { try { fn(); } catch (_) {} });
|
||||||
|
}
|
||||||
|
|
||||||
function _getScrollEl() {
|
function _getScrollEl() {
|
||||||
var el = _treeEl();
|
var el = _treeEl();
|
||||||
while (el && el !== document.documentElement) {
|
while (el && el !== document.documentElement) {
|
||||||
@@ -1312,6 +1343,10 @@ function createFolderSurface(cfg) {
|
|||||||
if (nowOpen) _openFolders.add(folder.path);
|
if (nowOpen) _openFolders.add(folder.path);
|
||||||
else _openFolders.delete(folder.path);
|
else _openFolders.delete(folder.path);
|
||||||
_storeJSON('open', [..._openFolders]);
|
_storeJSON('open', [..._openFolders]);
|
||||||
|
// This toggle moved everything below it — re-window the other lists,
|
||||||
|
// and re-window THIS one if it was already populated (its saved
|
||||||
|
// window was computed at its old position).
|
||||||
|
_repaintVirtualLists();
|
||||||
});
|
});
|
||||||
|
|
||||||
wrap.appendChild(hdr); wrap.appendChild(content);
|
wrap.appendChild(hdr); wrap.appendChild(content);
|
||||||
@@ -1371,6 +1406,7 @@ function createFolderSurface(cfg) {
|
|||||||
if (_unsortedOpen && !_populated) { _populate(); _populated = true; }
|
if (_unsortedOpen && !_populated) { _populate(); _populated = true; }
|
||||||
chev.style.transform = _unsortedOpen ? 'rotate(90deg)' : '';
|
chev.style.transform = _unsortedOpen ? 'rotate(90deg)' : '';
|
||||||
_store(cfg.unsortedKey, String(_unsortedOpen));
|
_store(cfg.unsortedKey, String(_unsortedOpen));
|
||||||
|
_repaintVirtualLists(); // this toggle moved every list below it
|
||||||
});
|
});
|
||||||
|
|
||||||
wrap.appendChild(hdr); wrap.appendChild(list);
|
wrap.appendChild(hdr); wrap.appendChild(list);
|
||||||
|
|||||||
@@ -122,3 +122,44 @@ test('degenerate inputs fall back to rendering everything, never to a broken win
|
|||||||
test('small lists are below the virtualization threshold', () => {
|
test('small lists are below the virtualization threshold', () => {
|
||||||
assert.ok(VIRTUAL_MIN >= 100, 'threshold must be high enough that normal folders are untouched');
|
assert.ok(VIRTUAL_MIN >= 100, 'threshold must be high enough that normal folders are untouched');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ── the grid must be re-measured when the window resizes (CodeRabbit, #967) ──
|
||||||
|
// perRow and rows were originally captured once at fill time. paint() also runs
|
||||||
|
// on resize, so a narrower/wider window changed the column count while the
|
||||||
|
// window maths still used the OLD one — slicing the wrong songs and mis-sizing
|
||||||
|
// the padding. These pin that the geometry is a function of perRow, so a stale
|
||||||
|
// perRow cannot silently survive.
|
||||||
|
|
||||||
|
test('resizing the grid to fewer columns re-windows against the new row count', () => {
|
||||||
|
const total = 10000;
|
||||||
|
const wide = visibleWindow(0, VH, 190, 6, Math.ceil(total / 6), total);
|
||||||
|
const narrow = visibleWindow(0, VH, 190, 3, Math.ceil(total / 3), total);
|
||||||
|
|
||||||
|
// Same viewport, half the columns -> about half as many songs on screen.
|
||||||
|
assert.ok(narrow.end < wide.end, 'fewer columns must render fewer songs per screen');
|
||||||
|
// ...and the total must still add up, or the scrollbar lies after a resize.
|
||||||
|
for (const [w, perRow] of [[wide, 6], [narrow, 3]]) {
|
||||||
|
const rows = Math.ceil(total / perRow);
|
||||||
|
assert.equal(w.padRowsTop + Math.ceil((w.end - w.start) / perRow) + w.padRowsBottom, rows,
|
||||||
|
`rows must account for every song at perRow=${perRow}`);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('a stale perRow would break the total-height invariant (the bug)', () => {
|
||||||
|
const total = 10000;
|
||||||
|
// Grid re-laid out to 3 columns, but windowed with the OLD perRow of 6:
|
||||||
|
// the row count no longer matches the geometry, and the padding is wrong.
|
||||||
|
const stalePerRow = 6, actualRows = Math.ceil(total / 3);
|
||||||
|
const bad = visibleWindow(0, VH, 190, stalePerRow, actualRows, total);
|
||||||
|
const accounted = bad.padRowsTop + Math.ceil((bad.end - bad.start) / 3) + bad.padRowsBottom;
|
||||||
|
assert.notEqual(accounted, actualRows,
|
||||||
|
'this asserts the FAILURE mode: mismatched perRow/rows must not silently look correct — ' +
|
||||||
|
'metrics() recomputes both together on every paint so this cannot happen in practice');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('scrolled grid window always starts on a row boundary', () => {
|
||||||
|
const total = 10000, perRow = 4;
|
||||||
|
const rows = Math.ceil(total / perRow);
|
||||||
|
const w = visibleWindow(-5000, VH, 190, perRow, rows, total);
|
||||||
|
assert.equal(w.start % perRow, 0, 'a partial row would shift every card in the grid');
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user