From 671a256a6ac7f86a9cc468d2949dd343da163746 Mon Sep 17 00:00:00 2001 From: Mazeyar Moeini Feizabadi Date: Tue, 10 Nov 2026 21:34:58 +0100 Subject: [PATCH] fix(news): follow-up review of the fixes Deleted World News API news is not restored by a refresh that fails; a refund goes only to the day the read was counted in, and not to a request that timed out; a cockpit brief from another place is dropped, not kept for a language change; a feed list started over lets scrolling ask again. Co-Authored-By: Claude Fable 5.2 --- server/providers/news/keyed.js | 14 ++++- server/providers/news/keyed.test.mjs | 7 ++- server/providers/news/upstream.js | 8 +-- src/ui/cockpitBriefing.js | 6 +- src/ui/cockpitBriefing.test.mjs | 90 ++++++++++++++++++++++------ src/ui/newsPanel.js | 7 ++- 6 files changed, 102 insertions(+), 30 deletions(-) diff --git a/server/providers/news/keyed.js b/server/providers/news/keyed.js index 8d9eab539..7bd5ca464 100644 --- a/server/providers/news/keyed.js +++ b/server/providers/news/keyed.js @@ -370,7 +370,13 @@ export function createKeyedNews({ (value) => ({ key, value, readAt: now() }), (error) => { logFailure(section, what, key, error); - return { ...prior, failedAt: now(), error }; + // What is held now: news deleted meanwhile stays deleted. + const held = store.get(id); + return { + ...(held?.key === key ? held : { key }), + failedAt: now(), + error, + }; }, ) .then((entry) => { @@ -720,6 +726,7 @@ export function createKeyedNews({ throw new UpstreamError("World News API's reads for today spent", { quota: true, }); + const { day } = budget; try { const reports = ( await fetchWorldNews({ @@ -735,8 +742,9 @@ export function createKeyedNews({ } catch (error) { // Out of points: nothing more is asked today. if (error.status === 402) budget.spent = true; - // Never reached (offline, timed out): no read of the day's. - if (error.unreached && budget.reads > 0) budget.reads -= 1; + // Never reached (offline): no read of the day it was counted in. + if (error.unreached && budget.day === day && budget.reads > 0) + budget.reads -= 1; throw error; } }, diff --git a/server/providers/news/keyed.test.mjs b/server/providers/news/keyed.test.mjs index 810c801ab..7b836f4a8 100644 --- a/server/providers/news/keyed.test.mjs +++ b/server/providers/news/keyed.test.mjs @@ -1229,8 +1229,11 @@ test('a World News API read that never reached it is not counted against the day // Back online: the day's reads are all still there. answers.worldnews = WORLD_NEWS; assert.equal((await paris()).status, 200); - // An answer, even a failing one, is a read: two of them spend the day. - answers.worldnews = 500; + // An answer, even a failing one, is a read, and so is a request that + // timed out, which it may have read: two of them spend the day. + answers.worldnews = () => { + throw new DOMException('timed out', 'TimeoutError'); + }; const berlin = () => ask('/feed', 'hotspot=all&at=52.52,13.4'); assert.deepEqual(await berlin(), UPSTREAM_ERROR); clock.now += 2 * MINUTE; diff --git a/server/providers/news/upstream.js b/server/providers/news/upstream.js index 4c58815a5..f82b4611f 100644 --- a/server/providers/news/upstream.js +++ b/server/providers/news/upstream.js @@ -115,11 +115,11 @@ async function getText( : AbortSignal.timeout(TIMEOUT_MS), }); } catch (cause) { + // A request that timed out may have been read; one refused was not. + const timedOut = cause?.name === 'TimeoutError'; throw new UpstreamError( - cause?.name === 'TimeoutError' - ? `${what} timed out` - : `${what} unreachable`, - { unreached: true }, + timedOut ? `${what} timed out` : `${what} unreachable`, + { unreached: !timedOut }, ); } if (!response.ok) { diff --git a/src/ui/cockpitBriefing.js b/src/ui/cockpitBriefing.js index ad95f94c1..20c95a3df 100644 --- a/src/ui/cockpitBriefing.js +++ b/src/ui/cockpitBriefing.js @@ -225,7 +225,11 @@ export function maybeRefreshRegionalBrief(info) { if (previous) { this.regionalBrief = cachedNews(previous, previous); this.renderRegionalBrief(this.regionalBrief, info); - } else this.renderRegionalBriefStatus('unavailable', info); + } else { + // Another place's brief: not to come back with a language change. + this.regionalBrief = null; + this.renderRegionalBriefStatus('unavailable', info); + } } }) .finally(() => { diff --git a/src/ui/cockpitBriefing.test.mjs b/src/ui/cockpitBriefing.test.mjs index 85adbc3d4..a97059aaa 100644 --- a/src/ui/cockpitBriefing.test.mjs +++ b/src/ui/cockpitBriefing.test.mjs @@ -219,7 +219,12 @@ test('the news footer follows the news state across page changes, not a static R showBriefPage.call(owner, 1); renderRegionalBrief.call( owner, - { newsStatus: 'ready', newsSource: 'wire', newsWindowHours: 24, articles: [story('a')] }, + { + newsStatus: 'ready', + newsSource: 'wire', + newsWindowHours: 24, + articles: [story('a')], + }, INFO, ); await nextTick(); @@ -231,7 +236,12 @@ test('the news footer follows the news state across page changes, not a static R showBriefPage.call(owner, 0); renderRegionalBrief.call( owner, - { newsStatus: 'ready', newsSource: 'wire', newsWindowHours: 12, articles: [story('a')] }, + { + newsStatus: 'ready', + newsSource: 'wire', + newsWindowHours: 12, + articles: [story('a')], + }, INFO, ); await nextTick(); @@ -252,13 +262,20 @@ test('the news banner is hidden by the stylesheet, not only by the attribute', a fileURLToPath(new URL('./styles/cockpit.css', import.meta.url)), 'utf8', ); - assert.match(css, /\.cockpit-brief-status\[hidden\]\s*\{\s*display:\s*none;?\s*\}/); + assert.match( + css, + /\.cockpit-brief-status\[hidden\]\s*\{\s*display:\s*none;?\s*\}/, + ); const { $, owner } = fixture(t); const style = document.head.appendChild(document.createElement('style')); t.after(() => style.remove()); style.textContent = css; showBriefPage.call(owner, 1); - renderRegionalBrief.call(owner, { newsStatus: 'ready', articles: [story('a')] }, INFO); + renderRegionalBrief.call( + owner, + { newsStatus: 'ready', articles: [story('a')] }, + INFO, + ); await nextTick(); const banner = $('#cockpit-news-status'); assert.equal(banner.hidden, true); @@ -268,13 +285,16 @@ test('the news banner is hidden by the stylesheet, not only by the attribute', a assert.equal(getComputedStyle(banner).display, 'grid'); }); -const hoursAgo = (hours) => new Date(Date.now() - hours * 3_600_000).toISOString(); +const hoursAgo = (hours) => + new Date(Date.now() - hours * 3_600_000).toISOString(); test('ready, cached, empty and unavailable each label the news differently', async (t) => { const { $, text, owner } = fixture(t); showBriefPage.call(owner, 1); const rows = () => - [...document.querySelectorAll('#cockpit-news-list strong')].map((row) => row.textContent); + [...document.querySelectorAll('#cockpit-news-list strong')].map( + (row) => row.textContent, + ); const banner = () => ({ hidden: $('#cockpit-news-status').hidden, state: $('#cockpit-news-status').dataset.state, @@ -304,11 +324,19 @@ test('ready, cached, empty and unavailable each label the news differently', asy text: 'CACHED · REFRESH FAILED', }); assert.deepEqual(rows(), ['Story a', 'Story b']); - assert.equal(text('#cockpit-brief-source'), 'GOOGLE NEWS RSS · CACHED 30M AGO'); + assert.equal( + text('#cockpit-brief-source'), + 'GOOGLE NEWS RSS · CACHED 30M AGO', + ); renderRegionalBrief.call( owner, - { ...ready, newsStatus: 'empty', newsSource: 'Google News RSS', articles: [] }, + { + ...ready, + newsStatus: 'empty', + newsSource: 'Google News RSS', + articles: [], + }, INFO, ); await nextTick(); @@ -354,7 +382,9 @@ test('articles outside the window are dropped even when a server sends them', as ); await nextTick(); assert.deepEqual( - [...document.querySelectorAll('#cockpit-news-list strong')].map((row) => row.textContent), + [...document.querySelectorAll('#cockpit-news-list strong')].map( + (row) => row.textContent, + ), ['Story two', 'Story one'], ); assert.equal($('#cockpit-news-status').hidden, true); @@ -421,34 +451,51 @@ test('a failed refresh keeps the earlier articles as cached, and clears only whe assert.equal($('#cockpit-news-status').hidden, false); assert.equal(text('#cockpit-news-status'), 'CACHED · REFRESH FAILED'); assert.equal(rows(), 2); - assert.equal(text('#cockpit-brief-source'), 'GOOGLE NEWS RSS · CACHED 1H AGO'); + assert.equal( + text('#cockpit-brief-source'), + 'GOOGLE NEWS RSS · CACHED 1H AGO', + ); showBriefPage.call(owner, 2); showBriefPage.call(owner, 1); await nextTick(); - assert.equal(text('#cockpit-brief-source'), 'GOOGLE NEWS RSS · CACHED 1H AGO'); + assert.equal( + text('#cockpit-brief-source'), + 'GOOGLE NEWS RSS · CACHED 1H AGO', + ); // So does a brief whose news read failed while weather answered. await refresh(() => - Promise.resolve({ status: 'partial', newsStatus: 'unavailable', articles: [] }), + Promise.resolve({ + status: 'partial', + newsStatus: 'unavailable', + articles: [], + }), ); assert.equal(text('#cockpit-news-status'), 'CACHED · REFRESH FAILED'); assert.equal(rows(), 2); // A successful refresh clears the cached label; a true empty clears rows. await refresh(() => - Promise.resolve({ status: 'ready', newsStatus: 'empty', newsSource: 'Google News RSS', articles: [] }), + Promise.resolve({ + status: 'ready', + newsStatus: 'empty', + newsSource: 'Google News RSS', + articles: [], + }), ); assert.equal(text('#cockpit-news-status'), 'NO NEWS IN THE LAST 24 H'); assert.equal(rows(), 0); assert.equal(text('#cockpit-brief-source'), 'GOOGLE NEWS RSS · LAST 24 H'); // Another aircraft never inherits the last one's rows. - await refresh(() => Promise.resolve({ newsStatus: 'ready', articles: [story('c')] })); - assert.equal(rows(), 1); - await refresh( - () => Promise.reject(new Error('503')), - { ...INFO, icao24: 'def456' }, + await refresh(() => + Promise.resolve({ newsStatus: 'ready', articles: [story('c')] }), ); + assert.equal(rows(), 1); + await refresh(() => Promise.reject(new Error('503')), { + ...INFO, + icao24: 'def456', + }); assert.equal(text('#cockpit-news-status'), 'REGIONAL NEWS UNAVAILABLE'); assert.equal(rows(), 0); }); @@ -499,7 +546,10 @@ test('the card and its toggle render the collapse the controller presents', asyn await nextTick(); assert.equal(card.dataset.collapsed, 'true'); assert.equal(toggle.getAttribute('aria-expanded'), 'false'); - assert.equal(toggle.getAttribute('aria-label'), 'Expand cockpit briefing panel'); + assert.equal( + toggle.getAttribute('aria-label'), + 'Expand cockpit briefing panel', + ); assert.equal(toggle.title, 'Expand briefing panel'); assert.equal(toggle.textContent.trim(), 'right_panel_open'); assert.equal(owner.signalUserCollapsed, true); @@ -562,6 +612,8 @@ test("a refresh that fails after a move never shows the last location's brief as await refresh(() => Promise.reject(new TypeError('Failed to fetch')), 45); assert.equal($('#cockpit-news-status').dataset.state, 'unavailable'); assert.equal(rows(), 0); + // Nor is it held, for a language change to show again. + assert.equal(owner.regionalBrief, null); // Nor does a brief there whose news read failed borrow the last place's rows. await refresh(here, 40); diff --git a/src/ui/newsPanel.js b/src/ui/newsPanel.js index a63f77739..ca48d0e95 100644 --- a/src/ui/newsPanel.js +++ b/src/ui/newsPanel.js @@ -67,7 +67,12 @@ export function createNewsPanel({ container, setLayerParams = () => {} } = {}) { const filters = JSON.stringify(next?.filters ?? null); // A new list (other filters or hotspot), or a read that failed, may stop // at the same length as the last request: let the next scroll ask. - if (filters !== shownFilters || (next?.error && !view?.error)) + // A list started over is shorter than the one the last request saw. + if ( + filters !== shownFilters || + (next?.error && !view?.error) || + (next?.items?.length ?? 0) < requestedAt + ) requestedAt = -1; shownFilters = filters; view = next || null;