fix(logpanel): preserve warnings/errors when cap prunes the log (#4900)
* fix(logpanel): preserve warnings/errors when cap prunes the log When the Research Logs panel hits its 500-entry DOM cap, the previous implementation dropped entries blindly from the head of the DOM (oldest-first). On long research runs that meant old warnings and errors were the first to be flushed, even when the cap was blown by a flood of routine info entries — exactly the opposite of what a user navigating to a stale error message would want. The prune now walks the cap-excess slots in priority order: 1. info entries (least diagnostic) 2. milestone entries 3. warning entries 4. error entries (most diagnostic — last to be dropped) Implementation: a new pruneToCap(container, cap) helper is shared by both prune paths (batch load in loadLogsForResearch and live insert in addLogEntryToPanel). It re-queries the children NodeList each loop so the removal doesn't iterate stale entries. The helper is exposed as window.logPanel._pruneToCap for unit tests, which exercise the ordering invariants across all four categories. Tests: 5 new tests under 'pruneToCap — per-category ordered prune' cover the priority order, the 'info-flood' scenario where 1000 info entries must not displace older warnings/errors, and the all-warnings- or-errors fallback path. All 744 JS tests pass. * fix(changelog): add trailing newline to end of changelog fragment * fix(logpanel): harden the prune helper and integrate it with per-category counters Follow-up addressing the AI code review on #4900 and the merge conflicts produced by #4898 and #4901: - pruneToCap now filters on the .ldr-console-log-entry class so a transient .ldr-empty-log-message, .ldr-loading-spinner, or .ldr-error-message that briefly co-exists with log entries cannot be miscounted or removed during pruning. - Priority order is hoisted into a frozen PRUNE_REMOVABLE_ORDER constant instead of being recreated on every loop iteration. - Return type simplified from Array<{type: string}> to string[]; the addLogEntryToPanel live-insert path now consumes that return value directly to decrement the per-category counts tracked by #4898, replacing the duplicated indexed loop that walked the entries a second time. - Docstring documents the chronological-gap trade-off (early errors survive alongside newer info) so future readers understand why surviving entries are not strictly the newest N. - Tests updated to the string[] shape, plus a new regression test that seeds a placeholder child and verifies it survives pruning intact alongside targeted entries. Rebased onto upstream/main to resolve conflicts with #4898's count state init and #4901's content-dedup-bypass path. * test(logpanel): drop redundant under-cap test; clarify all-info prune test name Follow-up to the AI review on #4900: - The 'returns an empty array and does nothing when the cap already accommodates every log entry' test was a near-duplicate of the existing 'is a no-op when already under the cap' test in the same describe block. Removed. - The 'prunes the oldest entries when count exceeds MAX_LOG_ENTRIES' test is the all-info case (which is observationally identical to the old head-drop behavior since info is dropped first in the new priority order). Renamed and annotated so future readers understand that per-category prune ordering is exercised by the pruneToCap describe block lower in the file, not by this test. PR description typo ('Follow-up: #4900 (skip dedup for non-info)' should reference #4901) was fixed via gh pr edit. --------- Co-authored-by: GitHub Copilot <copilot@github.com>
This commit is contained in:
S
@@ -0,0 +1 @@
|
||||
When the Research Logs panel hits its 500-entry DOM cap, the oldest entries are flushed first — which previously meant old warnings and errors got dropped even when the cap was blown by a flood of routine info entries. The prune now walks the cap-excess slots in priority order (info first, then milestones, then warnings, then errors), so the panel keeps its most diagnostic entries even on long research runs.
|
||||
@@ -477,6 +477,73 @@
|
||||
return await response.json();
|
||||
}
|
||||
|
||||
/**
|
||||
* Trim log entries from the container down to `cap`, preferring to drop
|
||||
* the least-actionable categories first.
|
||||
*
|
||||
* The plain "remove from head" prune loses the panel's most diagnostic
|
||||
* entries — old warnings and errors get flushed even when the cap was
|
||||
* blown by a flood of routine info entries. Walk the DOM and, for each
|
||||
* excess slot, drop the oldest entry whose type is in the currently-
|
||||
* removable category (info, then milestone). Once those are exhausted,
|
||||
* fall back to dropping warnings, then errors, in age order.
|
||||
*
|
||||
* Trade-off: ordered pruning means surviving entries are no longer
|
||||
* strictly the newest N — an early error can sit among hundreds of
|
||||
* newer info entries. That is intentional: warnings/errors are the
|
||||
* most diagnostic categories and should outlive routine info spam
|
||||
* during a long research run.
|
||||
*
|
||||
* Only `.ldr-console-log-entry` children are pruned; transient
|
||||
* placeholders such as `.ldr-empty-log-message`,
|
||||
* `.ldr-loading-spinner`, and `.ldr-error-message` (which can
|
||||
* briefly co-exist with log entries during async loads) are left
|
||||
* alone.
|
||||
*
|
||||
* @param {Element} container - The log container element.
|
||||
* @param {number} cap - The maximum allowed entry count after pruning.
|
||||
* @returns {string[]} The categories of removed entries, in removal
|
||||
* order. Each element corresponds to one DOM removal. Callers can
|
||||
* consume the return value to keep an external per-category counter
|
||||
* in sync without re-querying the DOM.
|
||||
*/
|
||||
const PRUNE_REMOVABLE_ORDER = Object.freeze(['info', 'milestone', 'warning', 'error']);
|
||||
function pruneToCap(container, cap) {
|
||||
const removed = [];
|
||||
while (true) {
|
||||
// Re-query every iteration: each remove() mutates the live
|
||||
// NodeList, so a cached handle would go stale.
|
||||
const entries = container.querySelectorAll('.ldr-console-log-entry');
|
||||
if (entries.length <= cap) break;
|
||||
let dropped = null;
|
||||
for (const targetType of PRUNE_REMOVABLE_ORDER) {
|
||||
for (const entry of entries) {
|
||||
const type = (entry.dataset.logType || 'info').toLowerCase();
|
||||
if (type === targetType) {
|
||||
dropped = entry;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (dropped) break;
|
||||
}
|
||||
// Defensive fallback — should be unreachable because every log
|
||||
// entry is created via createLogEntryElement which sets
|
||||
// dataset.logType, but if some non-log child slipped through
|
||||
// somehow, drop the oldest of those to guarantee forward
|
||||
// progress.
|
||||
if (!dropped) {
|
||||
const fallback = entries[0];
|
||||
if (!fallback) break;
|
||||
removed.push((fallback.dataset.logType || 'info').toLowerCase());
|
||||
fallback.remove();
|
||||
continue;
|
||||
}
|
||||
removed.push(dropped.dataset.logType.toLowerCase());
|
||||
dropped.remove();
|
||||
}
|
||||
return removed;
|
||||
}
|
||||
|
||||
/**
|
||||
* Load logs for a specific research
|
||||
* @param {string} researchId - The research ID to load logs for
|
||||
@@ -700,10 +767,10 @@
|
||||
}
|
||||
logContent.appendChild(fragment);
|
||||
|
||||
// Prune oldest entries (at DOM start, since oldest-first ordering)
|
||||
while (logContent.children.length > MAX_LOG_ENTRIES) {
|
||||
logContent.firstElementChild.remove();
|
||||
}
|
||||
// Prune to the cap, preferring to drop info/milestone entries over
|
||||
// warnings/errors so the panel keeps its diagnostic entries
|
||||
// even after a long research run flushes the head.
|
||||
pruneToCap(logContent, MAX_LOG_ENTRIES);
|
||||
|
||||
// Reset and recompute per-category counts from the DOM
|
||||
// after the batch insert + prune. Counting off the DOM
|
||||
@@ -1035,23 +1102,22 @@
|
||||
}
|
||||
|
||||
// Prune oldest entries if over limit to prevent unbounded DOM growth.
|
||||
// DOM order is oldest -> newest, so the oldest entries sit at the
|
||||
// head of the NodeList.
|
||||
const entries = consoleLogContainer.querySelectorAll('.ldr-console-log-entry');
|
||||
if (entries.length > MAX_LOG_ENTRIES) {
|
||||
const toRemove = entries.length - MAX_LOG_ENTRIES;
|
||||
for (let i = 0; i < toRemove; i++) {
|
||||
// Read the log type before removing so we can decrement
|
||||
// the matching per-category count for the per-filter
|
||||
// badges. Unknown types default to 'info' to match the
|
||||
// createLogEntryElement fallback below.
|
||||
const prunedType = (entries[i].dataset.logType || 'info').toLowerCase();
|
||||
entries[i].remove();
|
||||
// Prefer to drop the least-actionable categories first (info, then
|
||||
// milestones, then warnings, then errors) so a long research run
|
||||
// doesn't flush the panel's diagnostic tail. Mirrors the batch-load
|
||||
// prune above.
|
||||
const removed = pruneToCap(consoleLogContainer, MAX_LOG_ENTRIES);
|
||||
if (removed.length > 0) {
|
||||
// Keep the per-category counter for the filter badges in sync
|
||||
// with what was actually removed. pruneToCap returns the
|
||||
// categories in removal order, so we can decrement directly
|
||||
// without re-querying the DOM for each entry's type.
|
||||
for (const prunedType of removed) {
|
||||
if (window._logPanelState.counts[prunedType] !== undefined) {
|
||||
window._logPanelState.counts[prunedType]--;
|
||||
}
|
||||
}
|
||||
updateLogCounter(-toRemove);
|
||||
updateLogCounter(-removed.length);
|
||||
updateFilterCounters();
|
||||
}
|
||||
|
||||
@@ -1222,7 +1288,10 @@
|
||||
initialize: initializeLogPanel,
|
||||
addLog: addConsoleLog,
|
||||
filterLogs: filterLogsByType,
|
||||
loadLogs: loadLogsForResearch
|
||||
loadLogs: loadLogsForResearch,
|
||||
// Exposed for unit tests so the per-category prune ordering can be
|
||||
// exercised in isolation from the rest of the panel pipeline.
|
||||
_pruneToCap: pruneToCap
|
||||
};
|
||||
|
||||
// Self-invoke to initialize when DOM content is loaded
|
||||
|
||||
@@ -142,11 +142,14 @@ function setupPanelDom({ page = 'progress', researchId } = {}) {
|
||||
logPanel.initialize(rid);
|
||||
}
|
||||
|
||||
function makeLiveEntry(message) {
|
||||
// Mimic what addLogEntryToPanel produces in the DOM.
|
||||
function makeLiveEntry(message, type = 'info') {
|
||||
// Mimic what addLogEntryToPanel produces in the DOM. The optional
|
||||
// `type` argument lets the per-category prune tests seed entries
|
||||
// of a specific category into the container directly.
|
||||
const entry = document.createElement('div');
|
||||
entry.className = 'ldr-console-log-entry';
|
||||
entry.dataset.logId = `live-${message}`;
|
||||
entry.dataset.logType = type;
|
||||
const span = document.createElement('span');
|
||||
span.className = 'ldr-log-message';
|
||||
span.textContent = message;
|
||||
@@ -431,7 +434,14 @@ describe('addLog / loadLogs — ordering invariants', () => {
|
||||
// it while the timer bug was still the root cause. The test must fill
|
||||
// to the real MAX_LOG_ENTRIES cap to exercise the prune, so the work
|
||||
// can't be reduced without weakening the assertion.
|
||||
it('prunes the oldest entries when count exceeds MAX_LOG_ENTRIES', { timeout: 20000 }, () => {
|
||||
// Note: this is the all-info case. With all-info inserts the new
|
||||
// per-category prune ordering is observationally identical to the
|
||||
// old head-drop behavior (info is dropped first, so the oldest info
|
||||
// entry is what comes off the head). Per-category prune ordering --
|
||||
// the part the priority order actually changes -- is exercised by
|
||||
// the `pruneToCap -- per-category ordered prune` describe block
|
||||
// lower in this file.
|
||||
it('drops the oldest info entry when the cap is exceeded by info-only inserts', { timeout: 20000 }, () => {
|
||||
const container = document.getElementById('console-log-container');
|
||||
|
||||
// One insert over the cap. The live-insert prune in
|
||||
@@ -1003,3 +1013,151 @@ describe('per-category counters', () => {
|
||||
expect(getFilterCount('all')).toBe(4);
|
||||
});
|
||||
});
|
||||
|
||||
describe('pruneToCap — per-category ordered prune', () => {
|
||||
function seedEntry(container, message, type, index) {
|
||||
const entry = makeLiveEntry(message, type);
|
||||
// logpanel.js sorts by dataset.logTimeMs for chronological ordering.
|
||||
// We use a synthetic timestamp so insertion order matches DOM order.
|
||||
entry.dataset.logTimeMs = String(index);
|
||||
container.appendChild(entry);
|
||||
return entry;
|
||||
}
|
||||
|
||||
it('drops info entries before any other category', () => {
|
||||
const container = document.getElementById('console-log-container');
|
||||
// 3 info + 1 warning + 1 error; cap=2.
|
||||
seedEntry(container, 'info-0', 'info', 0);
|
||||
seedEntry(container, 'info-1', 'info', 1);
|
||||
seedEntry(container, 'info-2', 'info', 2);
|
||||
seedEntry(container, 'warn-0', 'warning', 3);
|
||||
seedEntry(container, 'err-0', 'error', 4);
|
||||
|
||||
const removed = logPanel._pruneToCap(container, 2);
|
||||
|
||||
// 3 removals needed (5 -> 2). All should be info entries.
|
||||
expect(removed.length).toBe(3);
|
||||
for (const r of removed) expect(r).toBe('info');
|
||||
expect(container.children.length).toBe(2);
|
||||
// The warning and error must survive.
|
||||
const survivingTypes = Array.from(container.children).map(
|
||||
(c) => c.dataset.logType
|
||||
);
|
||||
expect(survivingTypes).toEqual(['warning', 'error']);
|
||||
});
|
||||
|
||||
it('drops milestone entries after info is exhausted', () => {
|
||||
const container = document.getElementById('console-log-container');
|
||||
seedEntry(container, 'info-0', 'info', 0);
|
||||
seedEntry(container, 'info-1', 'info', 1);
|
||||
seedEntry(container, 'milestone-0', 'milestone', 2);
|
||||
seedEntry(container, 'warn-0', 'warning', 3);
|
||||
seedEntry(container, 'err-0', 'error', 4);
|
||||
|
||||
// cap=2 -> need 3 removals. 2 info first, then the milestone.
|
||||
const removed = logPanel._pruneToCap(container, 2);
|
||||
|
||||
expect(removed).toEqual(['info', 'info', 'milestone']);
|
||||
expect(container.children.length).toBe(2);
|
||||
const surviving = Array.from(container.children).map(
|
||||
(c) => c.dataset.logType
|
||||
);
|
||||
expect(surviving).toEqual(['warning', 'error']);
|
||||
});
|
||||
|
||||
it('preserves old warnings and errors even when the cap is blown by a flood of info', () => {
|
||||
const container = document.getElementById('console-log-container');
|
||||
// 1 early error, 1 early warning, then 1000 info entries.
|
||||
seedEntry(container, 'early-error', 'error', 0);
|
||||
seedEntry(container, 'early-warning', 'warning', 1);
|
||||
for (let i = 0; i < 1000; i++) {
|
||||
seedEntry(container, `info-${i}`, 'info', 2 + i);
|
||||
}
|
||||
|
||||
// cap=200. We need to drop 1002 - 200 = 802 entries. The two
|
||||
// diagnostic entries are the oldest, but the ordered prune must
|
||||
// protect them: all 802 drops must be info entries.
|
||||
const removed = logPanel._pruneToCap(container, 200);
|
||||
|
||||
expect(removed.length).toBe(802);
|
||||
expect(removed.every((r) => r === 'info')).toBe(true);
|
||||
expect(container.children.length).toBe(200);
|
||||
const survivingMessages = Array.from(
|
||||
container.querySelectorAll('.ldr-log-message')
|
||||
).map((el) => el.textContent);
|
||||
expect(survivingMessages).toContain('early-error');
|
||||
expect(survivingMessages).toContain('early-warning');
|
||||
});
|
||||
|
||||
it('falls back to dropping warnings then errors when no info/milestone are left', () => {
|
||||
const container = document.getElementById('console-log-container');
|
||||
seedEntry(container, 'warn-0', 'warning', 0);
|
||||
seedEntry(container, 'warn-1', 'warning', 1);
|
||||
seedEntry(container, 'err-0', 'error', 2);
|
||||
|
||||
// cap=1 -> need 2 removals. No info/milestone, so fall back to
|
||||
// dropping the oldest warning first, then the next oldest
|
||||
// warning, before touching the error. Errors are the most
|
||||
// diagnostic category, so they're the last to be dropped.
|
||||
const removed = logPanel._pruneToCap(container, 1);
|
||||
|
||||
expect(removed).toEqual(['warning', 'warning']);
|
||||
expect(container.children.length).toBe(1);
|
||||
expect(container.firstElementChild.dataset.logType).toBe('error');
|
||||
expect(
|
||||
container.firstElementChild.querySelector('.ldr-log-message')
|
||||
.textContent
|
||||
).toBe('err-0');
|
||||
});
|
||||
|
||||
it('is a no-op when already under the cap', () => {
|
||||
const container = document.getElementById('console-log-container');
|
||||
seedEntry(container, 'info-0', 'info', 0);
|
||||
seedEntry(container, 'err-0', 'error', 1);
|
||||
|
||||
const removed = logPanel._pruneToCap(container, 10);
|
||||
|
||||
expect(removed).toEqual([]);
|
||||
expect(container.children.length).toBe(2);
|
||||
});
|
||||
|
||||
|
||||
it('ignores placeholder children (.ldr-empty-log-message / spinner / error) when pruning', () => {
|
||||
const container = document.getElementById('console-log-container');
|
||||
// Simulate the loadLogsForResearch moment where a transient
|
||||
// spinner is still in the container alongside incoming entries.
|
||||
const spinner = document.createElement('div');
|
||||
spinner.className = 'ldr-loading-spinner';
|
||||
spinner.textContent = 'Loading...';
|
||||
container.appendChild(spinner);
|
||||
|
||||
// Seed one of every category so each priority bucket is exercised.
|
||||
seedEntry(container, 'info-A', 'info', 0);
|
||||
seedEntry(container, 'milestone-A', 'milestone', 1);
|
||||
seedEntry(container, 'warn-A', 'warning', 2);
|
||||
seedEntry(container, 'err-A', 'error', 3);
|
||||
|
||||
// Cap of 2 means 2 of the 4 log entries must be dropped.
|
||||
// The helper must walk the priority order (info, milestone,
|
||||
// warning, error) AND must NOT count or remove the placeholder.
|
||||
// If it had counted the spinner, only one entry would be
|
||||
// dropped and the test would fail on .removed.length.
|
||||
const removed = logPanel._pruneToCap(container, 2);
|
||||
|
||||
expect(removed).toEqual(['info', 'milestone']);
|
||||
// Placeholder is left in place; entry nodes are kept too.
|
||||
expect(container.querySelector('.ldr-loading-spinner')).toBe(spinner);
|
||||
const survivingLogEntries =
|
||||
container.querySelectorAll('.ldr-console-log-entry');
|
||||
expect(survivingLogEntries.length).toBe(2);
|
||||
expect(survivingLogEntries[0].dataset.logType).toBe('warning');
|
||||
expect(survivingLogEntries[1].dataset.logType).toBe('error');
|
||||
// Total DOM children = surviving 2 entries + 1 placeholder.
|
||||
expect(container.children.length).toBe(3);
|
||||
expect(container.children[0]).toBe(spinner);
|
||||
});
|
||||
|
||||
// The "no-op when already under the cap" case is covered by
|
||||
// `it('is a no-op when already under the cap', ...)` above; no
|
||||
// need to duplicate it here.
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user