diff --git a/CHANGELOG.md b/CHANGELOG.md index 459c95b9..790a5cc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +- Fixed a region that changes type — between the default and touch-button renderers under the same + region id — going blank until the next pull delivered different content, because the outgoing + component's cleanup dropped the region's scheduled slides before the incoming one asked for them. +- Fixed absent slides counting as content, which suppressed the fallback image and left the screen + black on a region that held nothing else. +- Fixed the screen client caching an error response as its configuration, which left `apiEndpoint` + undefined for the whole config interval and so failed every request made in it. +- Removed the screen client's template requests. Templates are bundled into the client, and the only + thing it took from `/v2/templates/{id}` was the id already carried by the slide, so the request + cost one round trip per template per pull and emptied a region whenever it was throttled (#507). +- Fixed a slide whose template the client cannot resolve taking down its whole region: the error was + thrown before the slide's own error boundary mounted, so the region's boundary caught it instead + and, having no way to reset, showed the error fallback until the client was reloaded. - Fixed the screen client treating a collection cut short by the 100-page backstop as a complete one, which cached the truncated data behind the server's checksum until an editor changed the content (#507). The collection is now given up, the same as when a page request fails. @@ -34,6 +47,28 @@ All notable changes to this project will be documented in this file. `NGINX_RATE_LIMIT_BURST` and answers `429` instead of `503`. - Made the screen client retry throttled and temporarily failing API requests with backoff, and bounded how many requests one pull may have in flight (#507). +- Fixed the screen client caching a partially failed pull as if it were complete, which left a + region, layout or media item stale until someone edited the content in the admin (#507). +- Made the screen client wait for a pull to finish before starting the next one, so a slow pull can + no longer have a second one stacked on top of it (#507). +- Added a timeout to the client config request, so a request that never answers can no longer stall + the screen client's polling (#507). +- Fixed the screen client showing stale playlist content until the layout happened to change, where a + pull that served the layout from cache handed the regions an unchanged object, so they never asked + for the slides the pull had just fetched (#507). +- Removed a tenant config request and a colour-scheme rebuild that the screen client repeated on + every pull (#507). +- Fixed a failed layout request blanking the screen client, where every region was unmounted and + playback restarted from the first slide on the next successful pull (#507). The last known good + layout is kept instead, unless it belongs to a campaign or to a layout the screen has since been + moved away from. +- Fixed the screen client suppressing the fallback image while showing nothing, where slides the + regions had dropped were still counted as content, so a screen with no renderable slides went + black instead (#507). +- Fixed a failed slides request emptying a playlist whose region had loaded fine, which lost that + playlist's content for a whole pull (#507). The slides the previous pull loaded are kept instead. +- Fixed the screen client pairing slides and playlists with the previous pull by position rather + than by id, so reordering a playlist in the admin could pair a slide with another slide's media. ## [3.0.0-rc8] - 2026-08-24 diff --git a/README.md b/README.md index 8f7de2c7..99e12f20 100644 --- a/README.md +++ b/README.md @@ -753,7 +753,7 @@ compose `environment:` with defaults in `infrastructure/nginx/Dockerfile`: | `NGINX_RATE_LIMIT_BURST` | Requests allowed to exceed the rate before rejection | `500` | Size these for the screen client, not for a browser. One pull from a screen on a multi-region layout is -a burst of one request per region, playlist, slide, template, media and feed — easily a few hundred +a burst of one request per region, playlist, slide, media and feed — easily a few hundred requests within a couple of seconds. Setting the limit too low makes regions and images randomly fail to render. diff --git a/assets/client/app.jsx b/assets/client/app.jsx index cc9d6bde..3cf9497a 100644 --- a/assets/client/app.jsx +++ b/assets/client/app.jsx @@ -276,12 +276,17 @@ function App({ preview, previewId }) { }; }, []); + // Keyed on the id, not the screen object: the screen is emitted on every pull, + // and loadTenantConfig is an api request - one per pull is the fan-out #507 is + // about. + const screenId = screen?.["@id"]; + useEffect(() => { - if (screen && screen["@id"]) { - releaseService.setScreenIdInUrl(screen["@id"]); + if (screenId) { + releaseService.setScreenIdInUrl(screenId); tenantService.loadTenantConfig(); } - }, [screen]); + }, [screenId]); return (
diff --git a/assets/client/components/region.jsx b/assets/client/components/region.jsx index 021a2720..d58f3888 100644 --- a/assets/client/components/region.jsx +++ b/assets/client/components/region.jsx @@ -3,6 +3,7 @@ import { createGridArea } from "../../shared/grid-generator/grid-generator"; import { TransitionGroup, CSSTransition } from "react-transition-group"; import ErrorBoundary from "./error-boundary.jsx"; import idFromPath from "../util/id-from-path"; +import isRenderableSlide from "../util/is-renderable-slide"; import logger from "../logger/logger"; import Slide, { MIN_SLIDE_DWELL_MS } from "./slide.jsx"; import nextRunId from "../../shared/slide-utils/next-run-id.js"; @@ -105,8 +106,9 @@ function Region({ region }) { function regionContentListener(event) { const receivedSlides = [...event.detail.slides]; - // Filter out invalid slides. - setNewSlides(receivedSlides.filter((slide) => !slide.invalid)); + // Filter out invalid slides. Shared with ScheduleService.checkForEmptyContent + // so the two cannot disagree about what counts as content. + setNewSlides(receivedSlides.filter(isRenderableSlide)); } // Setup event listener for region content. @@ -137,7 +139,10 @@ function Region({ region }) { }; }, []); - // Notify that region is ready. + // Notify that region is ready. Mount only: content is pushed by ScheduleService + // from here on, and asking again whenever the region prop changes identity was + // both redundant and unreliable - a pull that served the layout from cache + // hands back the same object, so the effect never ran (#507). useEffect(() => { const event = new CustomEvent("regionReady", { detail: { @@ -145,7 +150,7 @@ function Region({ region }) { }, }); document.dispatchEvent(event); - }, [region]); + }, []); // Start the progress if no slide is currently playing. useEffect(() => { diff --git a/assets/client/components/screen.jsx b/assets/client/components/screen.jsx index 2560f6d1..6f018d2f 100644 --- a/assets/client/components/screen.jsx +++ b/assets/client/components/screen.jsx @@ -68,8 +68,13 @@ function Screen({ screen }) { }); }; + // Keyed on the flag, not the screen object: the screen is emitted on every pull, + // and re-running this strips color-scheme-* off the html element before an async + // config load puts it back - a theme flash whenever that load is not cached. + const enableColorSchemeChange = screen?.enableColorSchemeChange; + useEffect(() => { - if (screen?.enableColorSchemeChange) { + if (enableColorSchemeChange) { logger.info("Enabling color scheme change."); refreshColorScheme(); // Refresh color scheme every 5 minutes. @@ -90,7 +95,7 @@ function Screen({ screen }) { "color-scheme-dark", ); }; - }, [screen]); + }, [enableColorSchemeChange]); return (
diff --git a/assets/client/components/slide.jsx b/assets/client/components/slide.jsx index 573aa897..50be6402 100644 --- a/assets/client/components/slide.jsx +++ b/assets/client/components/slide.jsx @@ -13,6 +13,27 @@ import "./slide.scss"; // the next advance -- keep region.scss's opacity transition in step. export const MIN_SLIDE_DWELL_MS = 1000; +/** + * Render the slide's template. + * + * A component rather than a call in Slide's own render: renderSlide throws when + * the slide names a template this build does not bundle, and an argument is + * evaluated before ErrorBoundary mounts. The throw escaped the boundary meant + * to contain it and hit the region's instead, which has no handler and never + * resets - so one unrenderable slide replaced its whole region with the error + * fallback until the client was reloaded. Thrown from inside the boundary's + * subtree it costs that one slide, which is then moved on from. + * + * @param {object} props - Props. + * @param {object} props.slide - The slide data. + * @param {number} props.run - Run id. Changes each time the slide should run. + * @param {Function} props.slideDone - The function to call when the slide is done running. + * @returns {object} - The rendered template. + */ +function SlideTemplate({ slide, run, slideDone }) { + return renderSlide(slide, run, slideDone); +} + /** * Slide component. * @@ -137,7 +158,11 @@ function Slide({ slide, id, run, slideDone, slideError, forwardRef }) { data-execution-id={slide.executionId} > - {renderSlide(slide, run, slideDoneAfterMinimumDwell)} +
); diff --git a/assets/client/components/touch-region.jsx b/assets/client/components/touch-region.jsx index 085fa8e7..06d7ba20 100644 --- a/assets/client/components/touch-region.jsx +++ b/assets/client/components/touch-region.jsx @@ -97,7 +97,7 @@ function TouchRegion({ region }) { }; }, []); - // Notify that region is ready. + // Notify that region is ready. Mount only, see Region. useEffect(() => { const event = new CustomEvent("regionReady", { detail: { @@ -105,7 +105,7 @@ function TouchRegion({ region }) { }, }); document.dispatchEvent(event); - }, [region]); + }, []); // Make sure current slide is set. useEffect(() => { diff --git a/assets/client/data-sync/api-helper.js b/assets/client/data-sync/api-helper.js index 3d4d6ebf..a17fe7c1 100644 --- a/assets/client/data-sync/api-helper.js +++ b/assets/client/data-sync/api-helper.js @@ -1,5 +1,8 @@ import logger from "../logger/logger"; import appStorage from "../util/app-storage"; +import fetchWithTimeout, { + REQUEST_TIMEOUT, +} from "../util/fetch-with-timeout.js"; // Statuses that mean "try again later" rather than "this failed". // 429 is the rate limit response from the reverse proxy, 502/503/504 are @@ -20,9 +23,11 @@ const RETRY_BASE_DELAY = 500; // for an hour inside a poll that runs every few minutes. const MAX_RETRY_DELAY = 30000; -// Give up on a single request after this long. A socket that never answers -// neither fails nor retries, which is the one case backoff cannot help. -const REQUEST_TIMEOUT = 15000; +// Spread applied on top of a Retry-After the server sent. Every client rejected +// in the same second is handed the same value, so honouring it verbatim would +// re-synchronise the burst the header exists to spread. Added rather than +// subtracted: RFC 9110 makes Retry-After a minimum, not a target. +const RETRY_AFTER_JITTER = 250; // Backstop so a misbehaving collection cannot page indefinitely. Matches the // limit used by the admin's get-all-pages helper. @@ -68,7 +73,9 @@ class ApiHelper { ); if (!Number.isNaN(retryAfter) && retryAfter > 0) { - return Math.min(retryAfter * 1000, MAX_RETRY_DELAY); + const asked = Math.min(retryAfter * 1000, MAX_RETRY_DELAY); + + return asked + Math.floor(Math.random() * RETRY_AFTER_JITTER); } // Full jitter: anywhere in [0, capped]. Spreads a burst that was rejected @@ -150,17 +157,12 @@ class ApiHelper { logger.log("info", `Fetching: ${this.endpoint + path}`); - // A request that never answers would otherwise sit in Promise.allSettled - // forever, holding a worker and stalling the pull. - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), REQUEST_TIMEOUT); let response; try { - response = await fetch(this.endpoint + path, { - headers, - signal: controller.signal, - }); + // A request that never answers would otherwise sit in the fan-out + // forever, holding a worker and stalling the pull. + response = await fetchWithTimeout(this.endpoint + path, { headers }); } catch (err) { const timedOut = err.name === "AbortError"; @@ -172,8 +174,6 @@ class ApiHelper { // A transport error and a timeout are both worth another attempt. return { data: null, retry: true, status: null, response: null }; - } finally { - clearTimeout(timeout); } if (response.ok === false) { diff --git a/assets/client/data-sync/pull-strategy.js b/assets/client/data-sync/pull-strategy.js index 2fe28bc1..50daf7f0 100644 --- a/assets/client/data-sync/pull-strategy.js +++ b/assets/client/data-sync/pull-strategy.js @@ -4,14 +4,23 @@ import ApiHelper from "./api-helper"; import { cloneDeep } from "lodash"; import ClientConfigLoader from "../util/client-config-loader.js"; import { settleWithConcurrency } from "../util/concurrency.js"; +import templateDataFromSlide from "../util/template-data-from-slide.js"; // Maximum requests in flight during a pull. A multi-region layout fans out one -// request per region, playlist, slide, template, media item and feed; sending -// all of them at once is what empties the reverse proxy's rate-limit bucket and -// leaves regions blank (#507). Keeping the burst well under the configured -// burst size means the retry layer rarely has to do anything. +// request per region, playlist, slide, media item and feed; sending all of them +// at once is what empties the reverse proxy's rate-limit bucket and leaves +// regions blank (#507). Keeping the burst well under the configured burst size +// means the retry layer rarely has to do anything. const MAX_CONCURRENT_REQUESTS = 6; +// Stored in place of a checksum the pull is not entitled to keep, so the group +// is refetched next time. A fresh symbol is unequal to every string the server +// can send *and* to every other symbol, which null is not: the API answers null +// for a group whose relation map is empty, and null === null would take the +// cached branch - the freeze this sentinel exists to prevent. Only in memory, +// so it never has to survive serialisation. +const CHECKSUM_UNAVAILABLE = () => Symbol("checksum-unavailable"); + /** * PullStrategy. * @@ -20,12 +29,30 @@ const MAX_CONCURRENT_REQUESTS = 6; class PullStrategy { lastestScreenData; + // Screen checksums to compare the next pull against. Kept apart from + // lastestScreenData because a pull that fell back to cached data must not be + // credited with the server's fresh checksum for the part it failed to fetch, + // and the screen object must not carry that correction: relationsChecksum is + // handed to the client as the server sent it, not as what this pull happens to + // be entitled to compare against. + lastestScreenChecksums; + // Helper for all api calls. apiHelper; // Fetch-interval in ms. interval; + // Set by stop(), so a pull already in flight does not schedule another one. + stopped = false; + + // Handle of the pending pull, and the generation it belongs to. Restarting + // bumps the generation, so a pull left over from an earlier start() cannot + // schedule alongside the current chain. + activeTimeout; + + chainId = 0; + // Path to screen that should be loaded data for. entryPoint = ""; @@ -50,9 +77,10 @@ class PullStrategy { * Gets all campaigns, both from screen and groups. * * @param {object} screen The screen object to extract campaigns from. + * @param {object} report Collects which relation groups came back degraded. * @returns {Promise} Array of campaigns (playlists). */ - async getCampaignsData(screen) { + async getCampaignsData(screen, report = {}) { const screenGroupCampaigns = []; try { @@ -77,10 +105,19 @@ class PullStrategy { (result.value.results ?? []).forEach(({ campaign }) => { screenGroupCampaigns.push(campaign); }); + + return; } + + report.campaigns = true; }); + } else { + // getAllResultsFromPath answers a bare {} when a page failed, so a + // missing results key is a failure rather than an empty collection. + report.campaigns = true; } } catch (err) { + report.campaigns = true; logger.error(err); } @@ -91,25 +128,31 @@ class PullStrategy { const screenCampaignsResponse = await this.apiHelper.getAllResultsFromPath(screen.campaigns); - screenCampaigns = (screenCampaignsResponse.results ?? []).map( - ({ campaign }) => campaign, - ); + if ( + Object.prototype.hasOwnProperty.call(screenCampaignsResponse, "results") + ) { + screenCampaigns = screenCampaignsResponse.results.map( + ({ campaign }) => campaign, + ); + } else { + report.campaigns = true; + } } catch (err) { + report.campaigns = true; logger.error(err); } - return new Promise((resolve) => { - resolve([...screenCampaigns, ...screenGroupCampaigns]); - }); + return [...screenCampaigns, ...screenGroupCampaigns]; } /** * Get slides for regions. * * @param {Array} regions Paths to regions. + * @param {object} report Collects which relation groups came back degraded. * @returns {Promise} Regions data. */ - async getRegions(regions) { + async getRegions(regions, report = {}) { const reg = /\/v2\/screens\/.*\/regions\/(?.*)\/playlists/; // Pair each region id with its request up front. Reading the id back out of @@ -144,6 +187,8 @@ class PullStrategy { return; } + report.regions = true; + // Keep the last known good playlists for this region rather than an empty // list. On signage, content that is one pull out of date beats a black // region, and a rejected request says nothing about what should be shown. @@ -170,13 +215,40 @@ class PullStrategy { return regionData; } + /** + * The playlist this region held on the previous pull. + * + * Matched on @id rather than on the position in the region: playlists get + * added, removed and reordered between pulls, and handing a playlist another + * playlist's slides is worse than handing it none - the same reasoning + * getRegions applies when it pairs region ids with their requests up front. + * + * @param {string} regionId The region the playlist belongs to. + * @param {string} playlistId The playlist's @id. + * @returns {object|undefined} The playlist, if the previous pull had it. + */ + previousPlaylist(regionId, playlistId) { + if (playlistId === undefined) { + return undefined; + } + + const previousRegion = this.lastestScreenData?.regionData?.[regionId]; + + if (!Array.isArray(previousRegion)) { + return undefined; + } + + return previousRegion.find((playlist) => playlist["@id"] === playlistId); + } + /** * Get slides for the given regions. * * @param {object} regions Regions to fetch slides for. + * @param {object} report Collects which relation groups came back degraded. * @returns {Promise} Promise with slides for the given regions. */ - async getSlidesForRegions(regions) { + async getSlidesForRegions(regions, report = {}) { const regionData = cloneDeep(regions); const entries = []; @@ -201,28 +273,91 @@ class PullStrategy { results.forEach((result, index) => { const { regionKey, playlistKey } = entries[index]; + const playlist = regionData[regionKey][playlistKey]; if ( result.status !== "fulfilled" || result.value?.results === undefined ) { - // Leave slidesData alone: cloneDeep kept whatever the previous pull - // attached, which is better than an empty playlist. + report.regions = true; + + // Keep the last known good slides for this playlist rather than none, + // the same trade getRegions and the layout branch make. cloneDeep only + // carried slidesData over when the playlists themselves came from the + // previous pull - the getRegions fallback or the cache branch. In the + // normal path they are fresh API objects that never had it, so without + // this the playlist empties for a whole pull. + const previous = this.previousPlaylist(regionKey, playlist["@id"]); + + if (previous?.slidesData !== undefined) { + logger.warn( + `Could not load slides for playlist ${playlistKey} in region ${regionKey}. Keeping the previously loaded slides.`, + ); + + // Cloned so newScreen does not share the array with lastestScreenData: + // the relations loop below writes back into it in place. + playlist.slidesData = cloneDeep(previous.slidesData); + + return; + } + logger.warn( - `Could not load slides for playlist ${playlistKey} in region ${regionKey}.`, + `Could not load slides for playlist ${playlistKey} in region ${regionKey} and have no earlier slides for it.`, ); return; } - regionData[regionKey][playlistKey].slidesData = result.value.results.map( - (playlistSlide) => playlistSlide.slide, - ); + // Dropped rather than carried as a hole in the array. A PlaylistSlide + // whose slide relation is absent maps to undefined, and the relations + // loop in getScreen writes templateData onto every entry - which throws + // on undefined and takes the entire pull with it, so one broken row in + // one playlist froze the whole screen on its last content, every pull. + playlist.slidesData = result.value.results + .map((playlistSlide) => playlistSlide.slide) + .filter((slide) => slide != null); }); return regionData; } + /** + * Screen checksums the next pull should compare itself against. + * + * A pull that fell back to cached data for a relation group is not entitled to + * the server's checksum for it. Storing it anyway is what froze screens: the + * next pull compares equal, takes the cache branch, and serves the degraded + * data until somebody edits the content in Admin (#507). Clearing the + * checksum makes the group differ, so the next pull fetches it again. + * + * @param {object|null} checksums Checksums as the server sent them. + * @param {object} report Relation groups this pull failed to load. + * @returns {object|null} Checksums to compare the next pull against. + */ + static checksumsToStore(checksums, report) { + if (checksums === null) { + return null; + } + + const stored = { ...checksums }; + + if (report.campaigns) { + // One getCampaignsData call covers both, so neither can be trusted. + stored.campaigns = CHECKSUM_UNAVAILABLE(); + stored.inScreenGroups = CHECKSUM_UNAVAILABLE(); + } + + if (report.layout) { + stored.layout = CHECKSUM_UNAVAILABLE(); + } + + if (report.regions) { + stored.regions = CHECKSUM_UNAVAILABLE(); + } + + return stored; + } + /** * Fetch screen. * @@ -254,18 +389,27 @@ class PullStrategy { newScreen.hasActiveCampaign = false; - const newScreenChecksums = newScreen?.relationsChecksum ?? []; - const oldScreenChecksums = - this.lastestScreenData?.relationsChecksum ?? null; + // Which relation groups this pull failed to load. A group listed here keeps + // whatever the previous pull had, so its checksum must not be stored - see + // checksumsToStore. + const report = {}; + + // Null rather than [] when the server sends no checksums at all: an empty + // object compares equal to the next empty object on every key, which would + // freeze the screen on cached data after the first pull. The API really can + // send nothing here - the DTO getter answers null for an empty map. + const newScreenChecksums = newScreen?.relationsChecksum ?? null; + const oldScreenChecksums = this.lastestScreenChecksums ?? null; if ( relationChecksumEnabled === false || + newScreenChecksums === null || oldScreenChecksums === null || oldScreenChecksums?.campaigns !== newScreenChecksums?.campaigns || oldScreenChecksums?.inScreenGroups !== newScreenChecksums?.inScreenGroups ) { logger.info(`Fetching campaigns.`); - newScreen.campaignsData = await this.getCampaignsData(newScreen); + newScreen.campaignsData = await this.getCampaignsData(newScreen, report); } else { logger.info(`Campaigns data loaded from cache.`); newScreen.campaignsData = this.lastestScreenData.campaignsData; @@ -307,6 +451,7 @@ class PullStrategy { ]; newScreen.regionData = await this.getSlidesForRegions( newScreen.regionData, + report, ); } else { logger.info(`Has no active campaign.`); @@ -314,12 +459,45 @@ class PullStrategy { // Get layout: Defines layout and regions. if ( relationChecksumEnabled === false || + newScreenChecksums === null || this.lastestScreenData?.hasActiveCampaign || oldScreenChecksums === null || oldScreenChecksums?.layout !== newScreenChecksums?.layout ) { logger.info(`Fetching layout.`); newScreen.layoutData = await this.apiHelper.getPath(newScreen.layout); + + if (newScreen.layoutData === null) { + report.layout = true; + + // Keep the last known good layout rather than none, the same trade + // getRegions makes. Screen builds its regions from layoutData.regions, + // so a null unmounts every one of them: regionRemoved drops the + // scheduling state, and the next good pull restarts playback from the + // first slide. + // + // Only the layout this screen actually wants, though. A previous pull + // in campaign mode holds the synthetic full-screen layout, and a pull + // from before the screen was moved to another layout holds regions + // that no longer match the regionData fetched below. + const previous = this.lastestScreenData; + const reusable = + previous?.layoutData != null && + previous.hasActiveCampaign !== true && + previous.layout === newScreen.layout; + + if (reusable) { + logger.warn( + `Could not load layout (${newScreen.layout}). Keeping the previously loaded layout.`, + ); + + newScreen.layoutData = previous.layoutData; + } else { + logger.warn( + `Could not load layout (${newScreen.layout}) and have no earlier layout for it.`, + ); + } + } } else { // Get layout: Defines layout and regions. logger.info(`Layout loaded from cache.`); @@ -329,13 +507,14 @@ class PullStrategy { // Fetch regions playlists: Yields playlists of slides for the regions if ( relationChecksumEnabled === false || + newScreenChecksums === null || this.lastestScreenData?.hasActiveCampaign || oldScreenChecksums === null || oldScreenChecksums?.regions !== newScreenChecksums?.regions ) { logger.info(`Fetching regions and slides for regions.`); - const regions = await this.getRegions(newScreen.regions); - newScreen.regionData = await this.getSlidesForRegions(regions); + const regions = await this.getRegions(newScreen.regions, report); + newScreen.regionData = await this.getSlidesForRegions(regions, report); } else { logger.info(`Regions and slides for regions loaded from cache.`); newScreen.regionData = this.lastestScreenData.regionData; @@ -343,7 +522,6 @@ class PullStrategy { } // Cached data. - const fetchedTemplates = {}; const fetchedMedia = {}; // Iterate all slides and load required relations. @@ -358,71 +536,65 @@ class PullStrategy { // this guard the whole pull rejects and every region goes blank. const dataEntrySlidesData = dataEntryPlaylist.slidesData ?? {}; + // Looked up once per playlist rather than once per slide. + const previousPlaylist = this.previousPlaylist( + regionKey, + dataEntryPlaylist["@id"], + ); + for (const slideKey of Object.keys(dataEntrySlidesData)) { const slide = cloneDeep(dataEntrySlidesData[slideKey]); - let previousSlide = null; - - // Find the slide in previous data for comparing relationsChecksum values. - if ( - this.lastestScreenData?.regionData[regionKey] && - this.lastestScreenData.regionData[regionKey][playlistKey] && - this.lastestScreenData.regionData[regionKey][playlistKey] - .slidesData[slideKey] - ) { - previousSlide = cloneDeep( - this.lastestScreenData.regionData[regionKey][playlistKey] - .slidesData[slideKey], - ); - } else { - previousSlide = {}; - } - - const newSlideChecksums = slide.relationsChecksum ?? []; + // Find the slide in previous data for comparing relationsChecksum + // values, and for the media cache below. Matched on @id, not on + // position: an editor reordering a playlist would otherwise pair a + // slide with a different slide's relations, and the cached branch + // would hand it that slide's media. + const previousSlideSource = previousPlaylist?.slidesData?.find( + (candidate) => candidate["@id"] === slide["@id"], + ); + + const previousSlide = previousSlideSource + ? cloneDeep(previousSlideSource) + : {}; + + // Null rather than [] for the same reason as the screen checksums + // above: two empty maps compare equal on every key. + const newSlideChecksums = slide.relationsChecksum ?? null; const oldSlideChecksums = previousSlide?.relationsChecksum ?? null; - // Fetch template if it has changed. - if ( - relationChecksumEnabled === false || - oldSlideChecksums === null || - newSlideChecksums.templateInfo !== oldSlideChecksums.templateInfo - ) { - const templatePath = slide.templateInfo["@id"]; - - // Load template into slide.templateData. - if ( - Object.prototype.hasOwnProperty.call( - fetchedTemplates, - templatePath, - ) - ) { - slide.templateData = fetchedTemplates[templatePath]; - } else { - logger.info(`Fetching template data.`); - const templateData = await this.apiHelper.getPath(templatePath); - slide.templateData = templateData; - - if (templateData !== null) { - fetchedTemplates[templatePath] = templateData; - } - } - } else { - logger.info(`Template data loaded from cache.`); - slide.templateData = previousSlide.templateData; - } + // Read the template off the slide rather than requesting it. Nothing + // in the API's template resource is used for rendering - see + // templateDataFromSlide. + slide.templateData = templateDataFromSlide(slide); // A slide cannot work without templateData. Mark as invalid. if (slide.templateData === null) { logger.warn( - `Template (${slide.templateInfo["@id"]}) not loaded, slideId: ${slide["@id"]}`, + `Template (${slide.templateInfo?.["@id"]}) has no id, slideId: ${slide["@id"]}`, ); slide.invalid = true; + } else if (slide.invalid === true) { + // Carried over from an earlier pull, either through the cached + // regions branch or from lastestScreenData. Region filters invalid + // slides out, so reading a usable template achieves nothing unless + // the flag is cleared with it. + delete slide.invalid; } - // Fetch media if it has changed. + // Fetch media if it has changed, or if any item in the cached set + // failed to load last time. Without the second condition a failed + // fetch is cached as null behind an unchanged checksum, and every + // later pull reuses the null - the item stays missing until an editor + // touches the slide (#507). The checksum cannot carry that signal: + // when the regions branch is served from cache, slide and + // previousSlide are clones of the same cached object, so their + // checksums always agree. if ( relationChecksumEnabled === false || + newSlideChecksums === null || oldSlideChecksums === null || + Object.values(previousSlide.mediaData ?? {}).includes(null) || newSlideChecksums.media !== oldSlideChecksums.media ) { const nextMediaData = {}; @@ -442,6 +614,12 @@ class PullStrategy { } slide.mediaData = nextMediaData; + + if (Object.values(nextMediaData).includes(null)) { + logger.warn( + `Media not loaded for slideId: ${slide["@id"]}. Retrying on the next pull.`, + ); + } } else { logger.info(`Media data loaded from cache.`); slide.mediaData = previousSlide.mediaData; @@ -460,6 +638,10 @@ class PullStrategy { /* eslint-enable no-restricted-syntax,no-await-in-loop */ this.lastestScreenData = newScreen; + this.lastestScreenChecksums = PullStrategy.checksumsToStore( + newScreenChecksums, + report, + ); // Deliver result to rendering const event = new CustomEvent("content", { @@ -478,16 +660,6 @@ class PullStrategy { return this.apiHelper.getAllResultsFromPath(path, keys); } - async getTemplateData(slide) { - return new Promise((resolve) => { - const templatePath = slide.templateInfo["@id"]; - - this.apiHelper.getPath(templatePath).then((data) => { - resolve(data); - }); - }); - } - async getFeedData(slide) { return new Promise((resolve) => { if (!slide?.feed?.feedUrl) { @@ -512,32 +684,55 @@ class PullStrategy { * Start the data synchronization. */ start() { - // Pull now. - this.getScreen(this.entryPoint) - .catch((err) => { - // A failed first pull must not stop the poll interval from being - // scheduled, or the screen stays dead until it is reloaded. - logger.error(err); - }) - .finally(() => { - // Make sure nothing is running. - this.stop(); - - // Start interval for pull periodically. - this.activeInterval = setInterval( - () => this.getScreen(this.entryPoint), + // Make sure nothing is running. + this.stop(); + + this.stopped = false; + this.chainId += 1; + + // Pull now, then keep rescheduling. + this.pull(this.chainId); + } + + /** + * Run one pull, then schedule the next one. + * + * @param {number} chainId The generation this chain belongs to. + */ + async pull(chainId) { + try { + await this.getScreen(this.entryPoint); + } catch (err) { + // A failed pull must not stop the poll from being scheduled, or the + // screen stays dead until it is reloaded. + logger.error(err); + } finally { + // In finally rather than after the catch: a throw from logger.error + // would otherwise end the chain silently. The generation check keeps a + // restart from leaving two chains scheduling against each other. + if (this.stopped === false && chainId === this.chainId) { + // Scheduled only once the previous pull settled. setInterval would + // start a second pull on top of a slow one, doubling the fan-out + // exactly when the backend is already struggling (#507). + this.activeTimeout = setTimeout( + () => this.pull(chainId), this.interval, ); - }); + } + } } /** * Stop the data synchronization. */ stop() { - if (this.activeInterval !== undefined) { - clearInterval(this.activeInterval); - delete this.activeInterval; + // A pull already in flight cannot be cancelled, so the flag is what keeps + // it from scheduling a successor once it finishes. + this.stopped = true; + + if (this.activeTimeout !== undefined) { + clearTimeout(this.activeTimeout); + delete this.activeTimeout; } } } diff --git a/assets/client/service/content-service.js b/assets/client/service/content-service.js index 19a97927..b84d85ad 100644 --- a/assets/client/service/content-service.js +++ b/assets/client/service/content-service.js @@ -1,5 +1,3 @@ -import sha256 from "crypto-js/sha256"; -import Base64 from "crypto-js/enc-base64"; import PullStrategy from "../data-sync/pull-strategy"; import { screenForPlaylistPreview, @@ -9,6 +7,7 @@ import logger from "../logger/logger"; import DataSync from "../data-sync/data-sync"; import ScheduleService from "./schedule-service"; import ClientConfigLoader from "../util/client-config-loader.js"; +import templateDataFromSlide from "../util/template-data-from-slide.js"; /** * ContentService. @@ -22,8 +21,6 @@ class ContentService { scheduleService; - screenHash; - /** * Constructor. */ @@ -110,33 +107,28 @@ class ContentService { contentHandler(event) { logger.info("Event received: content"); - const data = event.detail; - this.currentScreen = data.screen; + this.currentScreen = event.detail.screen; - const screenData = { ...this.currentScreen }; + const { regionData } = this.currentScreen; - // Remove regionData to only emit screen when it has changed. - for (let i = 0; i < screenData.regions.length; i += 1) { - delete screenData.regionData; + // Push before emitting. A pull that served the layout from cache hands back + // the very same region objects, so a region cannot be relied on to notice + // the new content itself - that is what left playlists stale (#507). + // Pushing first also means the cache is populated before a region mounted by + // the emit below asks for it in regionReady. + // eslint-disable-next-line no-restricted-syntax + for (const regionKey of Object.keys(regionData ?? {})) { + this.scheduleService.updateRegion(regionKey, regionData[regionKey]); } - const newHash = Base64.stringify(sha256(JSON.stringify(screenData))); - - // TODO: Handle issue where region data is not present for a given region. Remove given region content. - - if (newHash !== this.screenHash) { - logger.info("Screen has changed. Emitting screen."); - this.screenHash = newHash; - ContentService.emitScreen(screenData); - } else { - logger.info("Screen has not changed. Not emitting screen."); + // Regions are fed through ScheduleService, so the screen goes out without + // them. Emitted unconditionally: re-rendering is cheap because Screen keys + // its regions by id, so React reconciles them in place and playback state + // survives. + const screenData = { ...this.currentScreen }; + delete screenData.regionData; - // eslint-disable-next-line guard-for-in,no-restricted-syntax - for (const regionKey in data.screen.regionData) { - const region = this.currentScreen.regionData[regionKey]; - this.scheduleService.updateRegion(regionKey, region); - } - } + ContentService.emitScreen(screenData); } /** @@ -151,12 +143,30 @@ class ContentService { logger.info(`Event received: regionReady for ${regionId}`); - if (this.currentScreen) { - this.scheduleService.updateRegion( - regionId, - this.currentScreen.regionData[regionId], - ); + // A region that changes type - default <-> touch-buttons under the same + // region id - is a different component, so Screen unmounts one and mounts + // the other in a single commit. React runs the old component's cleanup + // before the new one's effects, so regionRemoved has already dropped the + // cached slides and cleared the scheduling interval by the time regionReady + // asks for them. The region would then show nothing until the next pull + // happened to change its content - up to the pull interval of blank screen. + // + // Rebuild from the screen the last pull delivered instead. updateRegion + // caches the slides, re-registers the interval and sends the slides on, + // which is everything regionReady would have done. + if (!this.scheduleService.hasRegion(regionId)) { + const regionData = this.currentScreen?.regionData?.[regionId]; + + if (regionData !== undefined) { + logger.info(`Restoring content for remounted region ${regionId}.`); + + this.scheduleService.updateRegion(regionId, regionData); + + return; + } } + + this.scheduleService.regionReady(regionId); } /** @@ -271,7 +281,7 @@ class ContentService { static async attachReferencesToSlide(strategy, slide) { /* eslint-disable no-param-reassign */ - slide.templateData = await strategy.getTemplateData(slide); + slide.templateData = templateDataFromSlide(slide); slide.feedData = await strategy.getFeedData(slide); slide.mediaData = {}; diff --git a/assets/client/service/schedule-service.js b/assets/client/service/schedule-service.js index 23b56757..9e488af3 100644 --- a/assets/client/service/schedule-service.js +++ b/assets/client/service/schedule-service.js @@ -5,6 +5,7 @@ import isPublished from "../util/isPublished"; import logger from "../logger/logger"; import ClientConfigLoader from "../util/client-config-loader.js"; import ScheduleUtils from "../util/schedule"; +import isRenderableSlide from "../util/is-renderable-slide"; import { cloneDeep } from "lodash"; /** @@ -22,6 +23,7 @@ class ScheduleService { constructor() { this.updateRegion = this.updateRegion.bind(this); + this.regionReady = this.regionReady.bind(this); this.checkForEmptyContent = this.checkForEmptyContent.bind(this); this.sendSlides = this.sendSlides.bind(this); } @@ -29,11 +31,15 @@ class ScheduleService { checkForEmptyContent() { logger.info("Checking for empty content."); - // Check for empty content. + // Check for empty content. Counted with the test Region applies when it + // renders, not just "has slides": a region whose slides all failed their + // template fetch holds slides that will never be shown, and counting those + // as content suppressed the fallback image and left the screen black. const values = Object.values(this.regions); const contentEmpty = - values.filter((value) => value?.slides.length > 0).length === 0; + values.filter((value) => value?.slides?.some(isRenderableSlide)) + .length === 0; if (contentEmpty !== this.contentEmpty) { this.contentEmpty = contentEmpty; @@ -46,6 +52,37 @@ class ScheduleService { } } + /** + * A region has mounted and is listening. Send it what is cached. + * + * updateRegion's hash gate exists to avoid re-sending content a region is + * already showing. A region that has only just mounted is showing nothing and + * missed whatever was dispatched before it registered its listener, so it gets + * the current slides regardless of the hash. + * + * @param {string} regionId - The region id. + */ + regionReady(regionId) { + const cached = this.regions[regionId]; + + if (!cached) { + logger.info(`ScheduleService: no content cached for region ${regionId}.`); + return; + } + + this.sendSlides(regionId, cached.slides); + } + + /** + * Whether slides are cached for a region. + * + * @param {string} regionId - The region id. + * @returns {boolean} True if the region has cached content. + */ + hasRegion(regionId) { + return Object.prototype.hasOwnProperty.call(this.regions, regionId); + } + /** * Remove scheduling interval for region if region is removed. * @@ -61,6 +98,13 @@ class ScheduleService { // Remove cached version of region data. delete this.regions[regionId]; + + // Losing a region changes whether anything is left to show. Without this, + // unmounting the last region that had content leaves contentEmpty stuck at + // false, so the fallback image never comes back and the screen is black - + // which is exactly what a failed layout request does (it renders no + // regions at all). + this.checkForEmptyContent(); } /** @@ -80,8 +124,19 @@ class ScheduleService { // Extract slides from playlists. const slides = ScheduleService.findScheduledSlides(region, regionId); - // Calculate a hash of the region to test if it has changed. - const hash = Base64.stringify(sha256(JSON.stringify({ region, slides }))); + // Calculate a hash of the scheduled slides to test if they have changed. + // + // This gate stays, unlike the one ContentService used to have: that one only + // decided whether React re-rendered, this one decides whether new arrays are + // pushed into region state. It is also the only thing that notices a feed + // changed - feedData is refetched on every pull and carries no checksum, so + // nothing else can see it. That is why the whole slide payload is hashed and + // not just ids. + // + // The region itself is deliberately not part of the input: findScheduledSlides + // derives the slides from it, so anything that changes what is rendered shows + // up in slides anyway, and hashing both doubles the work for every pull. + const hash = Base64.stringify(sha256(JSON.stringify(slides))); const newContent = hash !== this?.regions[regionId]?.hash; // Update region. @@ -130,15 +185,16 @@ class ScheduleService { // Extract slides from playlists. const slides = ScheduleService.findScheduledSlides(region.region, regionId); - // Calculate a hash of the region to test if it has changed. - const hash = Base64.stringify( - sha256(JSON.stringify({ region: region.region, slides })), - ); + // Calculate a hash of the scheduled slides to test if they have changed. + const hash = Base64.stringify(sha256(JSON.stringify(slides))); const newContent = hash !== this?.regions[regionId]?.hash; - // Update region. + // Update region. The slides have to be stored under the same key updateRegion + // uses - regionReady replays them to a region that has just mounted, and a + // stale entry here would hand it the content from before the last schedule + // change. this.regions[regionId].hash = hash; - this.regions[regionId].slide = slides; + this.regions[regionId].slides = slides; if (newContent) { // Send slides to region. diff --git a/assets/client/util/client-config-loader.js b/assets/client/util/client-config-loader.js index f940bb77..4abe0a98 100644 --- a/assets/client/util/client-config-loader.js +++ b/assets/client/util/client-config-loader.js @@ -1,5 +1,6 @@ // Only fetch new config if more than 15 minutes have passed. import appStorage from "./app-storage.js"; +import fetchWithTimeout from "./fetch-with-timeout.js"; const configFetchIntervalDefault = 15 * 60 * 1000; @@ -11,71 +12,107 @@ let latestFetchTimestamp = 0; let activePromise = null; +/** + * Config used when it cannot be loaded at all. + * + * Built fresh per call so a caller that edits what it got back cannot corrupt + * the fallback for everyone after it, and deliberately not assigned to + * configData, so a later call tries the real config again. + * + * @returns {object} The default config. + */ +function buildDefaultConfig() { + return { + apiEndpoint: "/api", + dataStrategy: { + type: "pull", + config: { + interval: 30000, + }, + }, + loginCheckTimeout: 20000, + configFetchInterval: 900000, + refreshTokenTimeout: 15000, + releaseTimestampIntervalTimeout: 600000, + colorScheme: { + type: "library", + lat: 56.0, + lng: 10.0, + }, + schedulingInterval: 60000, + debug: false, + }; +} + const ClientConfigLoader = { async loadConfig() { if (activePromise) { return activePromise; } - activePromise = new Promise((resolve) => { - const nowTimestamp = new Date().getTime(); + const nowTimestamp = new Date().getTime(); - if ( - latestFetchTimestamp + - (configData?.configFetchInterval ?? configFetchIntervalDefault) >= + // `configData !== null` guards the cold start: with no config loaded yet, + // the interval comparison alone can be satisfied by a small clock value and + // hand back null as though it were config. + if ( + configData !== null && + latestFetchTimestamp + + (configData?.configFetchInterval ?? configFetchIntervalDefault) >= nowTimestamp - ) { - resolve(configData); - } else { - fetch(`/config/client`) - .then((response) => response.json()) - .then((data) => { - latestFetchTimestamp = nowTimestamp; - configData = data; - - // Make api endpoint available through localstorage. - appStorage.setApiUrl(configData.apiEndpoint); - - resolve(configData); - }) - .catch(() => { - if (configData !== null) { - resolve(configData); - } else { - // eslint-disable-next-line no-console - console.error("Could not load config. Will use default config."); - - // Default config. - resolve({ - apiEndpoint: "/api", - dataStrategy: { - type: "pull", - config: { - interval: 30000, - }, - }, - loginCheckTimeout: 20000, - configFetchInterval: 900000, - refreshTokenTimeout: 15000, - releaseTimestampIntervalTimeout: 600000, - colorScheme: { - type: "library", - lat: 56.0, - lng: 10.0, - }, - schedulingInterval: 60000, - debug: false, - }); - } - }) - .finally(() => { - activePromise = null; - }); - } + ) { + return configData; + } + + // Cleared on every path. The cached branch used to leave it set, which + // pinned every later call to this one promise - so a single request that + // never answered could never be retried, and every screen pull awaiting the + // config would stall behind it (#507). + activePromise = ClientConfigLoader.fetchConfig(nowTimestamp).finally(() => { + activePromise = null; }); return activePromise; }, + + /** + * Fetch the config, falling back to the last known good one. + * + * @param {number} nowTimestamp Time the request was started. + * @returns {Promise} The config. + */ + async fetchConfig(nowTimestamp) { + try { + const response = await fetchWithTimeout(`/config/client`); + + // An error body is not config. fetch only rejects on transport failure, + // so without this a 4xx/5xx payload that happens to parse as JSON is + // stored as the config and served for the whole config interval - with + // apiEndpoint undefined, which takes every later request with it. + if (!response.ok) { + throw new Error(`Config request answered ${response.status}.`); + } + + const data = await response.json(); + + latestFetchTimestamp = nowTimestamp; + configData = data; + + // Make api endpoint available through localstorage. + appStorage.setApiUrl(configData.apiEndpoint); + + return configData; + } catch { + if (configData !== null) { + return configData; + } + + // eslint-disable-next-line no-console + console.error("Could not load config. Will use default config."); + + return buildDefaultConfig(); + } + }, }; Object.freeze(ClientConfigLoader); diff --git a/assets/client/util/fetch-with-timeout.js b/assets/client/util/fetch-with-timeout.js new file mode 100644 index 00000000..18ae5ee5 --- /dev/null +++ b/assets/client/util/fetch-with-timeout.js @@ -0,0 +1,33 @@ +// Give up on a single request after this long. A socket that never answers +// neither fails nor succeeds, so it cannot be waited on: it would hold a worker +// in the pull fan-out, or - for the client config - leave a pull awaiting a +// promise that never settles and end the poll chain (#507). +export const REQUEST_TIMEOUT = 15000; + +/** + * fetch() that gives up rather than waiting forever. + * + * Rejects with the usual AbortError once the ceiling passes, so callers can tell + * a timeout from a transport error by `err.name` and decide whether to retry. + * + * @param {string} resource The resource to fetch. + * @param {object} options Options passed on to fetch, minus `signal`. + * @param {number} timeout Milliseconds to wait before aborting. + * @returns {Promise} The response, if one arrives in time. + */ +export default async function fetchWithTimeout( + resource, + options = {}, + timeout = REQUEST_TIMEOUT, +) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeout); + + try { + return await fetch(resource, { ...options, signal: controller.signal }); + } finally { + // Also on the success path: an uncleared timer would keep the event loop + // busy and, in tests, accumulate across every request. + clearTimeout(timer); + } +} diff --git a/assets/client/util/is-renderable-slide.js b/assets/client/util/is-renderable-slide.js new file mode 100644 index 00000000..a09e4399 --- /dev/null +++ b/assets/client/util/is-renderable-slide.js @@ -0,0 +1,20 @@ +/** + * Whether a slide can actually be put on screen. + * + * A slide the pull could not load a template for is marked invalid and dropped + * by Region. ScheduleService has to apply the same test when it decides whether + * a region has content: counting slides the region will drop is what let a + * screen whose templates all failed report itself as non-empty, so the fallback + * image stayed hidden and the screen went black instead. + * + * An absent slide is not renderable either. `slide?.invalid !== true` answered + * true for null and undefined, so a region holding nothing but absent slides + * reported itself as having content - the same black screen this test exists to + * prevent, reached from the other side. + * + * @param {object} slide The slide to test. + * @returns {boolean} True if the slide should be rendered. + */ +export default function isRenderableSlide(slide) { + return slide != null && slide.invalid !== true; +} diff --git a/assets/client/util/template-data-from-slide.js b/assets/client/util/template-data-from-slide.js new file mode 100644 index 00000000..081a6788 --- /dev/null +++ b/assets/client/util/template-data-from-slide.js @@ -0,0 +1,21 @@ +import idFromPath from "./id-from-path"; + +/** + * The template data a slide renders with. + * + * There is nothing to fetch here. Every template's render code and config is + * bundled into this build (see the glob import in shared/slide-utils/templates.js), + * and the only thing rendering wants from templateData is the ULID it looks the + * bundled module up by - which is the last segment of the IRI the slide already + * carries. Requesting /v2/templates/{ulid} to read it back cost a request per + * template per pull and, when that request was throttled, a region that emptied + * itself once its playlist wrapped (#507). + * + * @param {object} slide The slide. + * @returns {object|null} The template data, or null if the slide names no template. + */ +export default function templateDataFromSlide(slide) { + const id = idFromPath(slide?.templateInfo?.["@id"]); + + return id ? { id } : null; +} diff --git a/assets/tests/client/api-helper-retry.test.js b/assets/tests/client/api-helper-retry.test.js index cd2f8a60..cb082aa1 100644 --- a/assets/tests/client/api-helper-retry.test.js +++ b/assets/tests/client/api-helper-retry.test.js @@ -250,3 +250,61 @@ describe("ApiHelper.getPath retries throttled requests", () => { expect(fetchMock).not.toHaveBeenCalled(); }); }); + +describe("ApiHelper.retryDelay spreads a Retry-After", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + /** + * A response carrying a Retry-After header. + * + * @param {string} value The header value. + * @returns {object} A response-alike. + */ + function withRetryAfter(value) { + return { headers: new Headers({ "Retry-After": value }) }; + } + + it("waits at least as long as the server asked", () => { + // RFC 9110 makes Retry-After a minimum, so the jitter is added rather than + // subtracted - a client must never come back early. + vi.spyOn(Math, "random").mockReturnValue(0); + + expect(ApiHelper.retryDelay(withRetryAfter("5"), 0)).toBeGreaterThanOrEqual( + 5000, + ); + }); + + it("does not hand every client the same wait", () => { + // Everyone rejected in the same second gets the same header value, so + // honouring it verbatim re-synchronises the burst it exists to spread. + vi.spyOn(Math, "random").mockReturnValue(0.999999); + + const delay = ApiHelper.retryDelay(withRetryAfter("5"), 0); + + expect(delay).toBeGreaterThan(5000); + expect(delay).toBeLessThan(5500); + }); + + it("keeps the jitter inside the clamp for an absurd Retry-After", () => { + vi.spyOn(Math, "random").mockReturnValue(0.999999); + + const delay = ApiHelper.retryDelay(withRetryAfter("3600"), 0); + + expect(delay).toBeGreaterThan(30000); + expect(delay).toBeLessThan(30500); + }); + + it("falls back to backoff for an HTTP-date Retry-After", () => { + // parseInt turns a date into NaN; the backoff is the acceptable degradation. + vi.spyOn(Math, "random").mockReturnValue(0.5); + + const delay = ApiHelper.retryDelay( + withRetryAfter("Wed, 21 Oct 2026 07:28:00 GMT"), + 0, + ); + + expect(delay).toBeLessThan(500); + }); +}); diff --git a/assets/tests/client/client-config-loader.test.js b/assets/tests/client/client-config-loader.test.js new file mode 100644 index 00000000..7f42c115 --- /dev/null +++ b/assets/tests/client/client-config-loader.test.js @@ -0,0 +1,196 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +import { REQUEST_TIMEOUT } from "../../client/util/fetch-with-timeout"; + +const loaderPath = "../../client/util/client-config-loader.js"; + +/** + * A fetch that only ever settles by being aborted. + * + * @returns {Function} A fetch stub. + */ +function neverAnswers() { + return vi.fn( + (resource, options) => + new Promise((resolve, reject) => { + options.signal.addEventListener("abort", () => { + const err = new Error("The operation was aborted."); + err.name = "AbortError"; + + reject(err); + }); + }), + ); +} + +describe("ClientConfigLoader", () => { + let fetchMock; + + beforeEach(() => { + // The loader keeps its cache in module scope, so each case needs a fresh + // copy of the module. + vi.resetModules(); + fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + localStorage.clear(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.useRealTimers(); + }); + + it("fetches the config on the first call", async () => { + fetchMock.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ relationsChecksumEnabled: true }), + }); + + const { default: ClientConfigLoader } = await import(loaderPath); + + const config = await ClientConfigLoader.loadConfig(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock.mock.calls[0][0]).toBe("/config/client"); + expect(config.relationsChecksumEnabled).toBe(true); + }); + + it("caches the response across repeated calls", async () => { + fetchMock.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ relationsChecksumEnabled: true }), + }); + + const { default: ClientConfigLoader } = await import(loaderPath); + + await ClientConfigLoader.loadConfig(); + await ClientConfigLoader.loadConfig(); + await ClientConfigLoader.loadConfig(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("falls back to defaults when the request fails", async () => { + fetchMock.mockRejectedValue(new Error("network down")); + + const { default: ClientConfigLoader } = await import(loaderPath); + + const config = await ClientConfigLoader.loadConfig(); + + expect(config.apiEndpoint).toBe("/api"); + expect(config.schedulingInterval).toBe(60000); + }); + + it("does not hand back null before any config has loaded", async () => { + // The cache guard used to be satisfied by the interval comparison alone, so + // a small clock value could return the not-yet-loaded null as config. + vi.useFakeTimers(); + vi.setSystemTime(0); + + fetchMock.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ relationsChecksumEnabled: true }), + }); + + const { default: ClientConfigLoader } = await import(loaderPath); + + await expect(ClientConfigLoader.loadConfig()).resolves.not.toBeNull(); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("recovers after a request that never answers", async () => { + // The in-flight promise used to be cleared only on the fetch path's + // finally, so a stalled request pinned every later call to the same pending + // promise - and every screen pull awaiting the config stalled behind it + // (#507). + vi.useFakeTimers(); + vi.stubGlobal("fetch", neverAnswers()); + + const { default: ClientConfigLoader } = await import(loaderPath); + + const first = ClientConfigLoader.loadConfig(); + + await vi.advanceTimersByTimeAsync(REQUEST_TIMEOUT + 1); + + // It settles rather than hanging, on the default config. + await expect(first).resolves.toMatchObject({ apiEndpoint: "/api" }); + + // And a later call tries again instead of returning the stalled promise. + fetchMock = vi.fn().mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ relationsChecksumEnabled: true }), + }); + vi.stubGlobal("fetch", fetchMock); + + await expect(ClientConfigLoader.loadConfig()).resolves.toMatchObject({ + relationsChecksumEnabled: true, + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("falls back to the default config on an error response", async () => { + // fetch only rejects on transport failure, so a 500 whose body happens to + // parse as JSON was stored as the config and served for the whole config + // interval - with apiEndpoint undefined, taking every request with it. + fetchMock.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ error: "Internal Server Error" }), + }); + + const { default: ClientConfigLoader } = await import(loaderPath); + + await expect(ClientConfigLoader.loadConfig()).resolves.toMatchObject({ + apiEndpoint: "/api", + }); + }); + + it("does not cache an error response", async () => { + // Nothing was stored, so the next call goes back to the network rather than + // waiting out the config interval on a fallback. + fetchMock.mockResolvedValueOnce({ + ok: false, + status: 503, + json: () => Promise.resolve({}), + }); + + const { default: ClientConfigLoader } = await import(loaderPath); + + await ClientConfigLoader.loadConfig(); + + fetchMock.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ relationsChecksumEnabled: true }), + }); + + await expect(ClientConfigLoader.loadConfig()).resolves.toMatchObject({ + relationsChecksumEnabled: true, + }); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("keeps the last known good config when a later request errors", async () => { + fetchMock.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ apiEndpoint: "/real-api" }), + }); + + const { default: ClientConfigLoader } = await import(loaderPath); + + await ClientConfigLoader.loadConfig(); + + // Past the config interval, so the next call refetches. + vi.useFakeTimers(); + vi.setSystemTime(Date.now() + 16 * 60 * 1000); + + fetchMock.mockResolvedValueOnce({ + ok: false, + status: 502, + json: () => Promise.resolve({}), + }); + + await expect(ClientConfigLoader.loadConfig()).resolves.toMatchObject({ + apiEndpoint: "/real-api", + }); + }); +}); diff --git a/assets/tests/client/content-service.test.js b/assets/tests/client/content-service.test.js new file mode 100644 index 00000000..e2f838b8 --- /dev/null +++ b/assets/tests/client/content-service.test.js @@ -0,0 +1,246 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("../../client/logger/logger", () => ({ + default: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), log: vi.fn() }, +})); + +vi.mock("../../client/util/client-config-loader.js", () => ({ + default: { + loadConfig: () => Promise.resolve({ schedulingInterval: 60000 }), + }, +})); + +// Neither is exercised here, but both reach for the network at import time. +vi.mock("../../client/data-sync/pull-strategy", () => ({ + default: class PullStrategy {}, +})); + +vi.mock("../../client/data-sync/data-sync", () => ({ + default: class DataSync {}, +})); + +import ContentService from "../../client/service/content-service"; + +const REGION_ID = "01JB1D9E3ZMTFBT7CYHFHGX5KA"; + +/** + * Build a screen as PullStrategy delivers it. + * + * @param {object} options Options. + * @param {Array} options.slideTitles Slides in the region's single playlist. + * @param {object} options.layoutData Layout, passed by reference to mimic a + * pull that served the layout from cache. + * @param {string} options.regionsChecksum The screen's regions checksum. + * @returns {object} The screen. + */ +function buildScreen({ slideTitles, layoutData, regionsChecksum }) { + return { + "@id": "/v2/screens/01JB1D9E3ZMTFBT7CYHFHGX5KC", + title: "Screen", + relationsChecksum: { layout: "unchanged", regions: regionsChecksum }, + layoutData, + regionData: { + [REGION_ID]: [ + { + "@id": "/v2/playlists/01JB1D9E3ZMTFBT7CYHFHGX5KB", + title: "Playlist", + schedules: [], + slidesData: slideTitles.map((title) => ({ + "@id": `/v2/slides/${title}`, + title, + })), + }, + ], + }, + }; +} + +/** + * Dispatch a content event. + * + * @param {object} screen The screen. + */ +function dispatchContent(screen) { + document.dispatchEvent(new CustomEvent("content", { detail: { screen } })); +} + +describe("ContentService", () => { + let contentService; + let screens; + let regionSends; + let screenHandler; + let regionHandler; + + beforeEach(() => { + vi.useFakeTimers(); + + screens = []; + regionSends = []; + + screenHandler = (event) => screens.push(event.detail.screen); + regionHandler = (event) => regionSends.push(event.detail.slides); + + document.addEventListener("screen", screenHandler); + document.addEventListener(`regionContent-${REGION_ID}`, regionHandler); + + contentService = new ContentService(); + contentService.start(); + }); + + afterEach(() => { + contentService.stop(); + document.removeEventListener("screen", screenHandler); + document.removeEventListener(`regionContent-${REGION_ID}`, regionHandler); + vi.useRealTimers(); + }); + + it("delivers new region content when the layout came from cache", () => { + // A pull that reuses the cached layout hands back the very same layoutData + // object, so the region components see an unchanged prop and never ask for + // content. Delivery has to come from here instead (#507). + const layoutData = { + grid: { rows: 1, columns: 1 }, + regions: [{ "@id": `/v2/layouts/regions/${REGION_ID}`, gridArea: ["a"] }], + }; + + dispatchContent( + buildScreen({ + slideTitles: ["a"], + layoutData, + regionsChecksum: "before", + }), + ); + dispatchContent( + buildScreen({ + slideTitles: ["a", "b"], + layoutData, + regionsChecksum: "after", + }), + ); + + expect(regionSends).toHaveLength(2); + expect(regionSends[1].map((slide) => slide.title)).toEqual(["a", "b"]); + }); + + it("emits the screen on every content event", () => { + const layoutData = { grid: { rows: 1, columns: 1 }, regions: [] }; + const screen = buildScreen({ + slideTitles: ["a"], + layoutData, + regionsChecksum: "same", + }); + + dispatchContent(screen); + dispatchContent(screen); + + expect(screens).toHaveLength(2); + }); + + it("emits the screen without regionData and leaves relationsChecksum alone", () => { + const layoutData = { grid: { rows: 1, columns: 1 }, regions: [] }; + + dispatchContent( + buildScreen({ + slideTitles: ["a"], + layoutData, + regionsChecksum: "abc", + }), + ); + + expect(screens[0]).not.toHaveProperty("regionData"); + expect(screens[0].relationsChecksum).toEqual({ + layout: "unchanged", + regions: "abc", + }); + }); + + it("hands a region its content when it reports ready after the push", () => { + const layoutData = { grid: { rows: 1, columns: 1 }, regions: [] }; + + document.removeEventListener(`regionContent-${REGION_ID}`, regionHandler); + dispatchContent( + buildScreen({ + slideTitles: ["a"], + layoutData, + regionsChecksum: "abc", + }), + ); + document.addEventListener(`regionContent-${REGION_ID}`, regionHandler); + + document.dispatchEvent( + new CustomEvent("regionReady", { detail: { id: REGION_ID } }), + ); + + expect(regionSends).toHaveLength(1); + expect(regionSends[0].map((slide) => slide.title)).toEqual(["a"]); + }); + + it("restores content for a region that unmounted and remounted", async () => { + // Changing a region's type swaps the component behind an unchanged region + // id, so Screen unmounts one and mounts the other in a single commit and + // React runs the outgoing cleanup before the incoming effects. regionReady + // therefore arrives with the cache regionRemoved has just dropped, and the + // region showed nothing until a later pull happened to change its content. + const layoutData = { grid: { rows: 1, columns: 1 }, regions: [] }; + + dispatchContent( + buildScreen({ + slideTitles: ["a"], + layoutData, + regionsChecksum: "abc", + }), + ); + + expect(regionSends).toHaveLength(1); + + document.dispatchEvent( + new CustomEvent("regionRemoved", { detail: { id: REGION_ID } }), + ); + document.dispatchEvent( + new CustomEvent("regionReady", { detail: { id: REGION_ID } }), + ); + + expect(regionSends).toHaveLength(2); + expect(regionSends[1].map((slide) => slide.title)).toEqual(["a"]); + + // regionRemoved cleared the scheduling interval too, so it has to come back + // with the content or the region never picks up a schedule change again. + await vi.waitFor(() => + expect(contentService.scheduleService.intervals[REGION_ID]).toBeDefined(), + ); + }); + + it("sends nothing for a region the current screen does not have", () => { + const layoutData = { grid: { rows: 1, columns: 1 }, regions: [] }; + + dispatchContent( + buildScreen({ + slideTitles: ["a"], + layoutData, + regionsChecksum: "abc", + }), + ); + + document.dispatchEvent( + new CustomEvent("regionRemoved", { detail: { id: "unknown-region" } }), + ); + document.dispatchEvent( + new CustomEvent("regionReady", { detail: { id: "unknown-region" } }), + ); + + expect(regionSends).toHaveLength(1); + }); + + it("survives a screen with no regionData", () => { + const layoutData = { grid: { rows: 1, columns: 1 }, regions: [] }; + const screen = buildScreen({ + slideTitles: [], + layoutData, + regionsChecksum: "abc", + }); + delete screen.regionData; + + expect(() => dispatchContent(screen)).not.toThrow(); + expect(screens).toHaveLength(1); + }); +}); diff --git a/assets/tests/client/fetch-with-timeout.test.js b/assets/tests/client/fetch-with-timeout.test.js new file mode 100644 index 00000000..8f3536a6 --- /dev/null +++ b/assets/tests/client/fetch-with-timeout.test.js @@ -0,0 +1,87 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +import fetchWithTimeout, { + REQUEST_TIMEOUT, +} from "../../client/util/fetch-with-timeout"; + +/** + * A fetch that only ever settles by being aborted. + * + * @returns {Function} A fetch stub. + */ +function neverAnswers() { + return vi.fn( + (resource, options) => + new Promise((resolve, reject) => { + options.signal.addEventListener("abort", () => { + const err = new Error("The operation was aborted."); + err.name = "AbortError"; + + reject(err); + }); + }), + ); +} + +describe("fetchWithTimeout", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it("aborts a request that never answers", async () => { + // A socket that never answers neither fails nor succeeds. Awaiting one is + // what let a single stalled request stall a whole screen pull (#507). + vi.stubGlobal("fetch", neverAnswers()); + + const promise = fetchWithTimeout("/config/client"); + const assertion = expect(promise).rejects.toThrowError( + /operation was aborted/, + ); + + await vi.advanceTimersByTimeAsync(REQUEST_TIMEOUT + 1); + + await assertion; + }); + + it("leaves a request that answers in time alone", async () => { + const response = { ok: true, status: 200 }; + + vi.stubGlobal( + "fetch", + vi.fn(() => Promise.resolve(response)), + ); + + await expect(fetchWithTimeout("/config/client")).resolves.toBe(response); + }); + + it("clears its timer on the success path", async () => { + // An uncleared timer would survive every successful request. + vi.stubGlobal( + "fetch", + vi.fn(() => Promise.resolve({ ok: true })), + ); + + await fetchWithTimeout("/config/client"); + + expect(vi.getTimerCount()).toBe(0); + }); + + it("passes options through and adds a signal", async () => { + const fetchMock = vi.fn(() => Promise.resolve({ ok: true })); + + vi.stubGlobal("fetch", fetchMock); + + await fetchWithTimeout("/v2/screens", { method: "POST" }); + + const [resource, options] = fetchMock.mock.calls[0]; + + expect(resource).toBe("/v2/screens"); + expect(options.method).toBe("POST"); + expect(options.signal).toBeInstanceOf(AbortSignal); + }); +}); diff --git a/assets/tests/client/is-renderable-slide.test.js b/assets/tests/client/is-renderable-slide.test.js new file mode 100644 index 00000000..7ecbfa99 --- /dev/null +++ b/assets/tests/client/is-renderable-slide.test.js @@ -0,0 +1,34 @@ +import { describe, it, expect } from "vitest"; + +import isRenderableSlide from "../../client/util/is-renderable-slide"; + +describe("isRenderableSlide", () => { + it("accepts a slide with no invalid flag", () => { + expect(isRenderableSlide({ "@id": "/v2/slides/a" })).toBe(true); + }); + + it("rejects a slide marked invalid", () => { + expect(isRenderableSlide({ "@id": "/v2/slides/a", invalid: true })).toBe( + false, + ); + }); + + it("accepts a slide whose invalid flag was cleared", () => { + // PullStrategy deletes the flag rather than setting it false, but a false + // must not be read as invalid either. + expect(isRenderableSlide({ "@id": "/v2/slides/a", invalid: false })).toBe( + true, + ); + }); + + it("rejects an absent slide", () => { + // The optional chain this used to be written with answered true for both: + // a region holding nothing but absent slides therefore counted as having + // content, which suppressed the fallback image and left the screen black - + // the same fault the invalid flag exists to prevent, reached from the other + // side. ScheduleService.checkForEmptyContent and Region share this test, so + // the hole showed up in both at once. + expect(isRenderableSlide(null)).toBe(false); + expect(isRenderableSlide(undefined)).toBe(false); + }); +}); diff --git a/assets/tests/client/pull-strategy-cache-recovery.test.js b/assets/tests/client/pull-strategy-cache-recovery.test.js new file mode 100644 index 00000000..6e1bb177 --- /dev/null +++ b/assets/tests/client/pull-strategy-cache-recovery.test.js @@ -0,0 +1,558 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const { mockGetPath, mockGetAllResultsFromPath, mockLoadConfig, mockLogger } = + vi.hoisted(() => ({ + mockGetPath: vi.fn(), + mockGetAllResultsFromPath: vi.fn(), + mockLoadConfig: vi.fn(), + mockLogger: { log: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + })); + +vi.mock("../../client/logger/logger", () => ({ default: mockLogger })); + +vi.mock("../../client/util/client-config-loader.js", () => ({ + default: { loadConfig: mockLoadConfig }, +})); + +vi.mock("../../client/data-sync/api-helper", () => ({ + default: vi.fn().mockImplementation(function () { + this.getPath = mockGetPath; + this.getAllResultsFromPath = mockGetAllResultsFromPath; + }), +})); + +import PullStrategy from "../../client/data-sync/pull-strategy"; + +const SCREEN = "01ARZ3NDEKTSV4RRFFQ69G5FAV"; +const REGION = "01BRZ3NDEKTSV4RRFFQ69G5FAV"; +const PLAYLIST = "01CRZ3NDEKTSV4RRFFQ69G5FAV"; +const SLIDE = "01DRZ3NDEKTSV4RRFFQ69G5FAV"; +const TEMPLATE = "01FRZ3NDEKTSV4RRFFQ69G5FAV"; + +const screenPath = `/v2/screens/${SCREEN}`; +const layoutPath = `/v2/layouts/01ERZ3NDEKTSV4RRFFQ69G5FAV`; +const groupsPath = `${screenPath}/screen-groups`; +const campaignsPath = `${screenPath}/campaigns`; +const regionPath = `${screenPath}/regions/${REGION}/playlists`; +const slidesPath = `/v2/playlists/${PLAYLIST}/slides`; +const templatePath = `/v2/templates/${TEMPLATE}`; +const mediaPath = `/v2/media/01GRZ3NDEKTSV4RRFFQ69G5FAV`; + +/** + * Build a screen as the API would return it. + * + * @param {object|null} relationsChecksum Checksums to advertise. + * @returns {object} The screen. + */ +function buildScreen(relationsChecksum) { + const screen = { + "@id": screenPath, + layout: layoutPath, + regions: [regionPath], + campaigns: campaignsPath, + inScreenGroups: groupsPath, + }; + + if (relationsChecksum !== null) { + screen.relationsChecksum = relationsChecksum; + } + + return screen; +} + +const checksums = { + campaigns: "campaigns-1", + layout: "layout-1", + regions: "regions-1", + inScreenGroups: "groups-1", +}; + +/** + * Build a slide, optionally referencing a media item. + * + * @param {Array} media Media ids the slide uses. + * @returns {object} The slide. + */ +function buildSlide(media = []) { + return { + "@id": `/v2/slides/${SLIDE}`, + relationsChecksum: { templateInfo: "template-1", media: "media-1" }, + templateInfo: { "@id": templatePath }, + media, + }; +} + +/** + * A successful paginated collection response. + * + * @param {string} path The path that was requested. + * @param {Array} results The rows. + * @returns {object} The response. + */ +function collection(path, results) { + return { path, results, keys: {} }; +} + +describe("PullStrategy recovers from a degraded pull", () => { + beforeEach(() => { + mockGetPath.mockReset(); + mockGetAllResultsFromPath.mockReset(); + mockLogger.warn.mockReset(); + mockLoadConfig.mockReset(); + + // The client config omits relationsChecksumEnabled when it cannot be + // loaded, and `undefined !== false` leaves caching on, so the tests that + // matter here are the ones with it explicitly enabled. + mockLoadConfig.mockResolvedValue({ relationsChecksumEnabled: true }); + }); + + /** + * Wire up the api helper mocks. + * + * @param {object} options Per-path overrides. + */ + function wireApi({ + screen = buildScreen(checksums), + slide = buildSlide(), + regionResponses = null, + mediaResponses = null, + slidesResponses = null, + } = {}) { + const regionQueue = regionResponses ? [...regionResponses] : null; + const mediaQueue = mediaResponses ? [...mediaResponses] : null; + const slidesQueue = slidesResponses ? [...slidesResponses] : null; + + mockGetPath.mockImplementation((path) => { + if (path === screenPath) { + return Promise.resolve(screen); + } + + if (path === layoutPath) { + return Promise.resolve({ grid: { rows: 1, columns: 1 }, regions: [] }); + } + + if (path === mediaPath) { + return Promise.resolve( + mediaQueue && mediaQueue.length > 0 + ? mediaQueue.shift() + : { assets: {} }, + ); + } + + return Promise.resolve(null); + }); + + mockGetAllResultsFromPath.mockImplementation((path) => { + if (path === groupsPath || path === campaignsPath) { + return Promise.resolve(collection(path, [])); + } + + if (path === regionPath) { + if (regionQueue && regionQueue.length > 0) { + return Promise.resolve(regionQueue.shift()); + } + + return Promise.resolve( + collection(path, [ + { + playlist: { + "@id": `/v2/playlists/${PLAYLIST}`, + slides: slidesPath, + }, + }, + ]), + ); + } + + if (path === slidesPath) { + if (slidesQueue && slidesQueue.length > 0) { + return Promise.resolve(slidesQueue.shift()); + } + + return Promise.resolve(collection(path, [{ slide }])); + } + + return Promise.resolve({}); + }); + } + + /** + * Number of times a path was requested. + * + * @param {object} mock The mock to inspect. + * @param {string} path The path to count. + * @returns {number} Call count. + */ + function callsFor(mock, path) { + return mock.mock.calls.filter(([called]) => called === path).length; + } + + it("refetches a region whose playlists failed, even though the checksum is unchanged", async () => { + // The whole point of the fix: a pull that fell back to cached data must not + // be credited with the server's checksum, or the next pull compares equal, + // takes the cache branch and the region stays stale until an editor happens + // to change the content (#507). + wireApi({ regionResponses: [{}] }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + expect(callsFor(mockGetAllResultsFromPath, regionPath)).toBe(2); + expect(mockLogger.warn).toHaveBeenCalled(); + }); + + it("still serves regions from cache when the pull was clean", async () => { + // The counterpart to the test above: the fix must not simply disable + // checksum caching. + wireApi(); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + expect(callsFor(mockGetAllResultsFromPath, regionPath)).toBe(1); + }); + + it("refetches the layout after a failed layout request", async () => { + wireApi(); + + mockGetPath.mockImplementation((path) => { + if (path === screenPath) { + return Promise.resolve(buildScreen(checksums)); + } + + if (path === layoutPath) { + return Promise.resolve(null); + } + + return Promise.resolve({ resources: {} }); + }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + expect(callsFor(mockGetPath, layoutPath)).toBe(2); + }); + + it("never caches a screen that advertises no checksums", async () => { + // The API really can send nothing here: the DTO getter answers null for an + // empty map. Two empty maps compare equal on every key, so defaulting to {} + // would freeze the screen after the first pull. + wireApi({ screen: buildScreen(null) }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + expect(callsFor(mockGetAllResultsFromPath, regionPath)).toBe(2); + }); + + it("reads the template off the slide instead of requesting it", async () => { + // Templates are bundled into the client build, and the only thing rendering + // wants from templateData is the id it looks the bundled module up by. That + // id is already in the slide's template IRI, so the request the pull used to + // make bought nothing and cost a region whenever it was throttled. + wireApi(); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + expect(callsFor(mockGetPath, templatePath)).toBe(0); + + const slide = + strategy.lastestScreenData.regionData[REGION][0].slidesData[0]; + + expect(slide.templateData).toEqual({ id: TEMPLATE }); + expect(slide.invalid).toBeUndefined(); + }); + + it("marks the slide invalid when its template IRI carries no id", async () => { + // Region drops invalid slides. Nothing can be rendered without an id to + // resolve the template module by, so this is the one case left where a + // slide has to be held back. + const slide = buildSlide(); + slide.templateInfo = { "@id": "/v2/templates/" }; + + wireApi({ slide }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + + const pulled = + strategy.lastestScreenData.regionData[REGION][0].slidesData[0]; + + expect(pulled.templateData).toBeNull(); + expect(pulled.invalid).toBe(true); + expect(mockLogger.warn).toHaveBeenCalled(); + }); + + it("keeps the previously loaded slides when a playlist's slides request fails", async () => { + // getRegions hands back playlists straight off the API, which have never + // carried slidesData, so cloneDeep has nothing to preserve here. Without a + // lookup the playlist empties for a whole pull even though the region + // itself loaded fine. + mockLoadConfig.mockResolvedValue({ relationsChecksumEnabled: false }); + + wireApi({ + slidesResponses: [ + collection(slidesPath, [{ slide: buildSlide() }]), + // getAllResultsFromPath answers a bare {} when a page failed. + {}, + ], + }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + const { slidesData } = strategy.lastestScreenData.regionData[REGION][0]; + + expect(slidesData).toHaveLength(1); + expect(slidesData[0]["@id"]).toBe(`/v2/slides/${SLIDE}`); + }); + + it("does not hand a new playlist the slides of the one it replaced", async () => { + // The reason the lookup matches on @id: a playlist that merely sits at the + // same position is a different playlist, and showing its predecessor's + // slides is worse than showing none. + mockLoadConfig.mockResolvedValue({ relationsChecksumEnabled: false }); + + const replacementSlides = "/v2/playlists/replacement/slides"; + + wireApi({ + regionResponses: [ + collection(regionPath, [ + { + playlist: { + "@id": `/v2/playlists/${PLAYLIST}`, + slides: slidesPath, + }, + }, + ]), + collection(regionPath, [ + { + playlist: { + "@id": "/v2/playlists/replacement", + slides: replacementSlides, + }, + }, + ]), + ], + }); + + const previousGetAll = mockGetAllResultsFromPath.getMockImplementation(); + + mockGetAllResultsFromPath.mockImplementation((path) => { + if (path === replacementSlides) { + return Promise.resolve({}); + } + + return previousGetAll(path); + }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + const playlist = strategy.lastestScreenData.regionData[REGION][0]; + + expect(playlist["@id"]).toBe("/v2/playlists/replacement"); + expect(playlist.slidesData).toBeUndefined(); + }); + + it("clears an invalid flag carried over from an earlier pull", async () => { + // Region drops invalid slides, so a flag that outlives the reason for it + // keeps the slide off screen forever. The pull now reads the template off + // every slide it sees, including the ones it takes from the cache. + wireApi(); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + + strategy.lastestScreenData.regionData[REGION][0].slidesData[0].invalid = + true; + + await strategy.getScreen(screenPath); + + const slide = + strategy.lastestScreenData.regionData[REGION][0].slidesData[0]; + + expect(slide.templateData).toEqual({ id: TEMPLATE }); + expect(slide.invalid).toBeUndefined(); + }); + + it("refetches media that failed, rather than reusing the cached null", async () => { + wireApi({ slide: buildSlide([mediaPath]), mediaResponses: [null] }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + expect(callsFor(mockGetPath, mediaPath)).toBe(2); + + const slide = + strategy.lastestScreenData.regionData[REGION][0].slidesData[0]; + + expect(slide.mediaData[mediaPath]).toEqual({ assets: {} }); + }); + + /** + * Serve a queue of responses per path, falling back to the last entry. + * + * @param {object} queues Screens and layouts, in pull order. + */ + function wireLayoutQueue({ screens, layouts }) { + const screenQueue = [...screens]; + const layoutQueue = [...layouts]; + + mockGetPath.mockImplementation((path) => { + if (path === screenPath) { + return Promise.resolve( + screenQueue.length > 1 ? screenQueue.shift() : screenQueue[0], + ); + } + + if (path.startsWith("/v2/layouts/")) { + return Promise.resolve( + layoutQueue.length > 1 ? layoutQueue.shift() : layoutQueue[0], + ); + } + + return Promise.resolve({ resources: {} }); + }); + } + + const layout = { + "@id": layoutPath, + grid: { rows: 1, columns: 1 }, + regions: [{ "@id": `/v2/layouts/regions/${REGION}`, gridArea: ["a"] }], + }; + + it("keeps the previously loaded layout when a later request for it fails", async () => { + // Screen builds its regions from layoutData.regions, so a null unmounts + // every one of them: the scheduling state is dropped and the next good pull + // restarts playback from the first slide. Content one pull out of date beats + // a black screen, the same trade getRegions makes. + wireApi(); + wireLayoutQueue({ + screens: [ + buildScreen(checksums), + buildScreen({ ...checksums, layout: "layout-2" }), + ], + layouts: [layout, null], + }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + expect(callsFor(mockGetPath, layoutPath)).toBe(2); + expect(strategy.lastestScreenData.layoutData).toEqual(layout); + }); + + it("does not fall back to the synthetic layout of a campaign pull", async () => { + // Campaign mode swaps in a full-screen layout with one hardcoded region. + // Reusing it once the campaign is over would leave the client rendering a + // region no playlist is scheduled for. + wireApi(); + wireLayoutQueue({ + screens: [ + buildScreen(checksums), + // Ending the campaign moves its checksum too, otherwise the second pull + // serves campaignsData from cache and never leaves campaign mode. + buildScreen({ + ...checksums, + layout: "layout-2", + campaigns: "campaigns-2", + }), + ], + // Campaign mode builds its layout rather than fetching one, so the only + // layout request in this test is the failing one on the second pull. + layouts: [null], + }); + + const campaigns = [ + collection(campaignsPath, [ + { + campaign: { + "@id": `/v2/playlists/${PLAYLIST}`, + published: { from: null, to: null }, + slides: slidesPath, + }, + }, + ]), + collection(campaignsPath, []), + ]; + + const previousGetAll = mockGetAllResultsFromPath.getMockImplementation(); + + mockGetAllResultsFromPath.mockImplementation((path) => { + if (path === campaignsPath) { + return Promise.resolve( + campaigns.length > 1 ? campaigns.shift() : campaigns[0], + ); + } + + return previousGetAll(path); + }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + expect(strategy.lastestScreenData.hasActiveCampaign).toBe(true); + + await strategy.getScreen(screenPath); + + expect(strategy.lastestScreenData.hasActiveCampaign).toBe(false); + expect(strategy.lastestScreenData.layoutData).toBeNull(); + }); + + it("does not fall back to the layout the screen has been moved away from", async () => { + const otherLayoutPath = "/v2/layouts/01HRZ3NDEKTSV4RRFFQ69G5FAV"; + + wireApi(); + wireLayoutQueue({ + screens: [ + buildScreen(checksums), + { + ...buildScreen({ ...checksums, layout: "layout-2" }), + layout: otherLayoutPath, + }, + ], + layouts: [layout, null], + }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + // The old layout's regions do not match the regionData this pull fetched. + expect(strategy.lastestScreenData.layoutData).toBeNull(); + }); + + it("keeps media cached when nothing failed", async () => { + wireApi({ slide: buildSlide([mediaPath]) }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: screenPath }); + + await strategy.getScreen(screenPath); + await strategy.getScreen(screenPath); + + expect(callsFor(mockGetPath, mediaPath)).toBe(1); + }); +}); diff --git a/assets/tests/client/pull-strategy-region-failure.test.js b/assets/tests/client/pull-strategy-region-failure.test.js index b84912a3..f2d00296 100644 --- a/assets/tests/client/pull-strategy-region-failure.test.js +++ b/assets/tests/client/pull-strategy-region-failure.test.js @@ -123,3 +123,39 @@ describe("PullStrategy.getRegions with a failed region request", () => { expect(Object.keys(regionData)).toEqual([REGION_A]); }); }); + +describe("PullStrategy.getSlidesForRegions", () => { + const slidesPath = "/v2/playlists/1/slides"; + + beforeEach(() => { + mockGetAllResultsFromPath.mockReset(); + mockLogger.warn.mockReset(); + }); + + it("drops a playlist row whose slide relation is absent", async () => { + // A row with no slide maps to undefined, and getScreen's relations loop + // writes templateData onto every entry. Assigning to undefined throws, and + // nothing between there and pull()'s catch handles it, so one broken row + // aborted the whole pull - the screen kept its last content on every pull + // after, which is the freeze this whole series of fixes is about (#507). + mockGetAllResultsFromPath.mockResolvedValue({ + path: slidesPath, + results: [ + { slide: { "@id": "/v2/slides/a", title: "a" } }, + { slide: null }, + {}, + { slide: { "@id": "/v2/slides/b", title: "b" } }, + ], + keys: {}, + }); + + const strategy = new PullStrategy({ endpoint: "", entryPoint: "" }); + const regionData = await strategy.getSlidesForRegions({ + [REGION_A]: [{ "@id": "/v2/playlists/1", slides: slidesPath }], + }); + + expect( + regionData[REGION_A][0].slidesData.map((slide) => slide.title), + ).toEqual(["a", "b"]); + }); +}); diff --git a/assets/tests/client/pull-strategy-schedule.test.js b/assets/tests/client/pull-strategy-schedule.test.js new file mode 100644 index 00000000..032ef7e4 --- /dev/null +++ b/assets/tests/client/pull-strategy-schedule.test.js @@ -0,0 +1,165 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +const { mockLogger } = vi.hoisted(() => ({ + mockLogger: { log: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); + +vi.mock("../../client/logger/logger", () => ({ default: mockLogger })); + +vi.mock("../../client/data-sync/api-helper", () => ({ + default: vi.fn().mockImplementation(function () { + this.getPath = vi.fn(); + this.getAllResultsFromPath = vi.fn(); + }), +})); + +import PullStrategy from "../../client/data-sync/pull-strategy"; + +const INTERVAL = 1000; +const screenPath = "/v2/screens/01ARZ3NDEKTSV4RRFFQ69G5FAV"; + +/** + * A PullStrategy whose pulls are held open until released. + * + * @returns {object} The strategy and a release function. + */ +function strategyWithHeldPulls() { + const strategy = new PullStrategy({ + endpoint: "", + entryPoint: screenPath, + interval: INTERVAL, + }); + + const pending = []; + + // getScreen is an own property from the constructor bind, so replacing it + // here is what pull() will call. + strategy.getScreen = vi.fn( + () => + new Promise((resolve) => { + pending.push(resolve); + }), + ); + + return { + strategy, + async releaseAll() { + while (pending.length > 0) { + pending.shift()(); + // Let the finally block that schedules the next pull run. + // eslint-disable-next-line no-await-in-loop + await Promise.resolve(); + } + }, + }; +} + +describe("PullStrategy scheduling", () => { + beforeEach(() => { + vi.useFakeTimers(); + mockLogger.error.mockReset(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("does not start a second pull while one is still running", async () => { + // setInterval fired regardless of whether the previous pull had finished, + // so a slow pull got a second one stacked on top of it - doubling the + // fan-out exactly when the backend was already struggling (#507). + const { strategy, releaseAll } = strategyWithHeldPulls(); + + strategy.start(); + + expect(strategy.getScreen).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(INTERVAL * 5); + + expect(strategy.getScreen).toHaveBeenCalledTimes(1); + + await releaseAll(); + await vi.advanceTimersByTimeAsync(INTERVAL); + + expect(strategy.getScreen).toHaveBeenCalledTimes(2); + + strategy.stop(); + }); + + it("keeps polling after a pull rejects", async () => { + const strategy = new PullStrategy({ + endpoint: "", + entryPoint: screenPath, + interval: INTERVAL, + }); + + strategy.getScreen = vi + .fn() + .mockRejectedValueOnce(new Error("boom")) + .mockResolvedValue(undefined); + + strategy.start(); + + await vi.advanceTimersByTimeAsync(INTERVAL); + + expect(strategy.getScreen).toHaveBeenCalledTimes(2); + expect(mockLogger.error).toHaveBeenCalled(); + + strategy.stop(); + }); + + it("schedules nothing more when stopped during the first pull", async () => { + // stop() used to be ignored here: start()'s finally scheduled the interval + // anyway, so a DataSync that ContentService had already discarded kept + // polling and dispatching content events. + const { strategy, releaseAll } = strategyWithHeldPulls(); + + strategy.start(); + strategy.stop(); + + await releaseAll(); + await vi.advanceTimersByTimeAsync(INTERVAL * 5); + + expect(strategy.getScreen).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(0); + }); + + it("leaves only one chain running when restarted mid-pull", async () => { + const { strategy, releaseAll } = strategyWithHeldPulls(); + + strategy.start(); + strategy.start(); + + // Two pulls are in flight - the abandoned one and the new chain's - but + // only the current generation may schedule a successor. + expect(strategy.getScreen).toHaveBeenCalledTimes(2); + + await releaseAll(); + await vi.advanceTimersByTimeAsync(INTERVAL); + + expect(strategy.getScreen).toHaveBeenCalledTimes(3); + + strategy.stop(); + }); + + it("stops cleanly between pulls", async () => { + const strategy = new PullStrategy({ + endpoint: "", + entryPoint: screenPath, + interval: INTERVAL, + }); + + strategy.getScreen = vi.fn().mockResolvedValue(undefined); + + strategy.start(); + + await vi.advanceTimersByTimeAsync(INTERVAL); + expect(strategy.getScreen).toHaveBeenCalledTimes(2); + + strategy.stop(); + + await vi.advanceTimersByTimeAsync(INTERVAL * 5); + + expect(strategy.getScreen).toHaveBeenCalledTimes(2); + }); +}); diff --git a/assets/tests/client/schedule-service.test.js b/assets/tests/client/schedule-service.test.js new file mode 100644 index 00000000..d18eb30c --- /dev/null +++ b/assets/tests/client/schedule-service.test.js @@ -0,0 +1,247 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("../../client/logger/logger", () => ({ + default: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), log: vi.fn() }, +})); + +vi.mock("../../client/util/client-config-loader.js", () => ({ + default: { + loadConfig: () => Promise.resolve({ schedulingInterval: 60000 }), + }, +})); + +import ScheduleService from "../../client/service/schedule-service"; + +const REGION_ID = "01JB1D9E3ZMTFBT7CYHFHGX5KA"; + +/** + * Build a minimal slide. + * + * @param {string} id Slide id. + * @param {object} extra Extra slide properties. + * @returns {object} The slide. + */ +function buildSlide(id, extra = {}) { + return { + "@id": `/v2/slides/${id}`, + title: id, + ...extra, + }; +} + +/** + * Build a region: one playlist holding the given slides. + * + * @param {Array} slidesData The slides. + * @param {object} extra Extra playlist properties. + * @returns {Array} The region. + */ +function buildRegion(slidesData, extra = {}) { + return [ + { + "@id": "/v2/playlists/01JB1D9E3ZMTFBT7CYHFHGX5KB", + title: "Playlist", + schedules: [], + slidesData, + ...extra, + }, + ]; +} + +/** + * Record every regionContent event for a region. + * + * @param {string} regionId The region id. + * @returns {object} The recorded sends and a teardown. + */ +function recordSends(regionId) { + const sends = []; + const handler = (event) => sends.push(event.detail.slides); + + document.addEventListener(`regionContent-${regionId}`, handler); + + return { + sends, + stop: () => + document.removeEventListener(`regionContent-${regionId}`, handler), + }; +} + +/** + * Record the contentEmpty/contentNotEmpty events, in order. + * + * @returns {object} The recorded events and a teardown. + */ +function recordEmptyState() { + const events = []; + const onEmpty = () => events.push("contentEmpty"); + const onNotEmpty = () => events.push("contentNotEmpty"); + + document.addEventListener("contentEmpty", onEmpty); + document.addEventListener("contentNotEmpty", onNotEmpty); + + return { + events, + stop: () => { + document.removeEventListener("contentEmpty", onEmpty); + document.removeEventListener("contentNotEmpty", onNotEmpty); + }, + }; +} + +describe("ScheduleService", () => { + let service; + let recorder; + + beforeEach(() => { + vi.useFakeTimers(); + service = new ScheduleService(); + recorder = recordSends(REGION_ID); + }); + + afterEach(() => { + recorder.stop(); + vi.useRealTimers(); + }); + + it("sends slides the first time a region is updated", () => { + service.updateRegion(REGION_ID, buildRegion([buildSlide("a")])); + + expect(recorder.sends).toHaveLength(1); + expect(recorder.sends[0].map((slide) => slide.title)).toEqual(["a"]); + }); + + it("does not re-send identical content", () => { + service.updateRegion(REGION_ID, buildRegion([buildSlide("a")])); + service.updateRegion(REGION_ID, buildRegion([buildSlide("a")])); + + expect(recorder.sends).toHaveLength(1); + }); + + it("re-sends when only feedData changed", () => { + service.updateRegion( + REGION_ID, + buildRegion([buildSlide("a", { feedData: { entries: ["one"] } })]), + ); + service.updateRegion( + REGION_ID, + buildRegion([buildSlide("a", { feedData: { entries: ["two"] } })]), + ); + + // Feeds are refetched on every pull and carry no checksum, so this hash is + // the only thing that can notice they changed. + expect(recorder.sends).toHaveLength(2); + expect(recorder.sends[1][0].feedData).toEqual({ entries: ["two"] }); + }); + + it("does not re-send when only a playlist field that is not rendered changed", () => { + service.updateRegion( + REGION_ID, + buildRegion([buildSlide("a")], { title: "Before" }), + ); + service.updateRegion( + REGION_ID, + buildRegion([buildSlide("a")], { title: "After" }), + ); + + // The region is no longer part of the hash input - only the slides it + // produces are. + expect(recorder.sends).toHaveLength(1); + }); + + it("replays the cached slides on regionReady, despite an unchanged hash", () => { + service.updateRegion(REGION_ID, buildRegion([buildSlide("a")])); + expect(recorder.sends).toHaveLength(1); + + service.regionReady(REGION_ID); + + expect(recorder.sends).toHaveLength(2); + expect(recorder.sends[1].map((slide) => slide.title)).toEqual(["a"]); + }); + + it("delivers to a region that mounts after its content arrived", () => { + // The push happens before React has mounted the region, so that dispatch is + // lost. Without the replay the region would stay blank: updateRegion's hash + // gate reports no change on every later pull. + recorder.stop(); + service.updateRegion(REGION_ID, buildRegion([buildSlide("a")])); + + recorder = recordSends(REGION_ID); + service.regionReady(REGION_ID); + + expect(recorder.sends).toHaveLength(1); + expect(recorder.sends[0].map((slide) => slide.title)).toEqual(["a"]); + }); + + it("sends nothing on regionReady when no content is cached", () => { + service.regionReady(REGION_ID); + + expect(recorder.sends).toHaveLength(0); + }); + + it("keeps the cached slides in step with checkScheduling", () => { + const region = buildRegion([buildSlide("a")]); + service.updateRegion(REGION_ID, region); + + // Same object ScheduleService holds, so checkScheduling recomputes from it. + region[0].slidesData.push(buildSlide("b")); + service.checkScheduling(REGION_ID); + + expect(recorder.sends).toHaveLength(2); + + service.regionReady(REGION_ID); + + // The replay has to hand out what checkScheduling last worked out, not the + // set from before the schedule moved. + expect(recorder.sends[2].map((slide) => slide.title)).toEqual(["a", "b"]); + }); + + it("reports empty content when every slide in the region is invalid", () => { + // Region drops invalid slides, so holding them is not the same as having + // something to show. Counting them kept contentEmpty at false, which + // suppressed the fallback image and left the screen black. + const state = recordEmptyState(); + + service.updateRegion(REGION_ID, buildRegion([buildSlide("a")])); + expect(state.events).toEqual(["contentNotEmpty"]); + + service.updateRegion( + REGION_ID, + buildRegion([buildSlide("a", { invalid: true })]), + ); + + expect(state.events).toEqual(["contentNotEmpty", "contentEmpty"]); + + state.stop(); + }); + + it("reports empty content when the last region with content is removed", () => { + // A failed layout request renders no regions at all, so every region + // unmounts. Without re-checking here the fallback image never comes back. + const state = recordEmptyState(); + + service.updateRegion(REGION_ID, buildRegion([buildSlide("a")])); + expect(state.events).toEqual(["contentNotEmpty"]); + + service.regionRemoved(REGION_ID); + + expect(state.events).toEqual(["contentNotEmpty", "contentEmpty"]); + + state.stop(); + }); + + it("drops the cached region and its interval on regionRemoved", async () => { + service.updateRegion(REGION_ID, buildRegion([buildSlide("a")])); + + // The interval is registered behind an await on the config. + await vi.waitFor(() => expect(service.intervals[REGION_ID]).toBeDefined()); + + service.regionRemoved(REGION_ID); + + expect(service.intervals[REGION_ID]).toBeUndefined(); + expect(service.regions[REGION_ID]).toBeUndefined(); + + service.regionReady(REGION_ID); + expect(recorder.sends).toHaveLength(1); + }); +}); diff --git a/assets/tests/client/slide-template-error.test.jsx b/assets/tests/client/slide-template-error.test.jsx new file mode 100644 index 00000000..c080f973 --- /dev/null +++ b/assets/tests/client/slide-template-error.test.jsx @@ -0,0 +1,140 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { render, act, cleanup } from "@testing-library/react"; + +const { slideDoneCallbacks } = vi.hoisted(() => ({ + slideDoneCallbacks: new Map(), +})); + +vi.mock("../../client/logger/logger", () => ({ + default: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); + +// What the real templates.js does for an id with no module in this build - see +// templates-lookup.test.js. Mocked so the case can be exercised without pulling +// every template into the test. +vi.mock("../../shared/slide-utils/templates.js", () => ({ + renderSlide: (slide, run, slideDone) => { + if (slide?.templateData?.id === MISSING) { + throw new Error(`Cannot find module '${MISSING}'`); + } + + slideDoneCallbacks.set(slide.executionId, slideDone); + + return
; + }, + getConfig: () => ({}), +})); + +vi.mock("../../client/components/slide.scss", () => ({})); +vi.mock("../../client/components/region.scss", () => ({})); +vi.mock("../../client/components/error-boundary.scss", () => ({})); + +import Slide from "../../client/components/slide.jsx"; +import Region from "../../client/components/region.jsx"; + +const MISSING = "01JZZZZZZZZZZZZZZZZZZZZZZZ"; +const PRESENT = "01FP2SNGFN0BZQH03KCBXHKYHG"; +const REGION_ID = "01JB1D9E3ZMTFBT7CYHFHGX5KA"; + +/** + * Build a minimal slide. + * + * @param {string} executionId - The execution id. + * @param {string} template - The template id the slide names. + * @returns {object} The slide. + */ +function createSlide(executionId, template) { + return { + executionId, + templateData: { id: template }, + mediaData: {}, + content: {}, + }; +} + +describe("a slide naming a template this build does not have", () => { + beforeEach(() => { + slideDoneCallbacks.clear(); + }); + + afterEach(() => { + cleanup(); + }); + + it("shows the fallback and moves on rather than wedging the region", () => { + // A slide that throws and never reports back would hold the region on the + // error fallback forever, so the boundary has to end the slide's turn. + vi.useFakeTimers(); + + try { + const slideError = vi.fn(); + + const { container } = render( + , + ); + + expect(container.querySelector(".error-boundary")).not.toBeNull(); + expect(slideError).not.toHaveBeenCalled(); + + act(() => { + vi.advanceTimersByTime(5000); + }); + + expect(slideError).toHaveBeenCalledTimes(1); + expect(slideError.mock.calls[0][0].executionId).toBe("unresolvable"); + } finally { + vi.useRealTimers(); + } + }); + + it("costs its own slide, not every slide in the region", () => { + // renderSlide used to be called as an argument in Slide's render, so it + // threw before Slide's own boundary mounted and hit the region's instead - + // which has no handler and never resets, so one unrenderable slide replaced + // the whole region with the fallback until the client was reloaded. + vi.useFakeTimers(); + + try { + const bad = createSlide("BADSLIDE", MISSING); + const good = createSlide("GOODSLIDE", PRESENT); + + const { container } = render( + , + ); + + act(() => { + document.dispatchEvent( + new CustomEvent(`regionContent-${REGION_ID}`, { + detail: { slides: [bad, good] }, + }), + ); + }); + + // Only the bad slide shows the fallback - the region is intact. + expect( + container.querySelector("#BADSLIDE .error-boundary"), + ).not.toBeNull(); + expect(container.querySelector(".region > .error-boundary")).toBeNull(); + + // And the region moves on, so the good slide still gets its turn. + act(() => { + vi.advanceTimersByTime(5000); + }); + + expect( + container.querySelector('[data-testid="template-GOODSLIDE"]'), + ).not.toBeNull(); + } finally { + vi.useRealTimers(); + } + }); +}); diff --git a/assets/tests/client/template-data-from-slide.test.js b/assets/tests/client/template-data-from-slide.test.js new file mode 100644 index 00000000..3dbbe3a6 --- /dev/null +++ b/assets/tests/client/template-data-from-slide.test.js @@ -0,0 +1,41 @@ +import { describe, it, expect } from "vitest"; +import templateDataFromSlide from "../../client/util/template-data-from-slide"; + +const TEMPLATE = "01FP2SNGFN0BZQH03KCBXHKYHG"; + +describe("templateDataFromSlide", () => { + it("reads the template id off the slide's template IRI", () => { + expect( + templateDataFromSlide({ + templateInfo: { "@id": `/v2/templates/${TEMPLATE}` }, + }), + ).toEqual({ id: TEMPLATE }); + }); + + it("ignores the options the IRI is delivered alongside", () => { + expect( + templateDataFromSlide({ + templateInfo: { + "@id": `/v2/templates/${TEMPLATE}`, + options: { fade: false }, + }, + }), + ).toEqual({ id: TEMPLATE }); + }); + + it("returns null when the IRI carries no id", () => { + expect( + templateDataFromSlide({ templateInfo: { "@id": "/v2/templates/" } }), + ).toBeNull(); + }); + + it("returns null when the slide names no template", () => { + expect(templateDataFromSlide({ templateInfo: {} })).toBeNull(); + expect(templateDataFromSlide({})).toBeNull(); + }); + + it("returns null rather than throwing on a missing slide", () => { + expect(templateDataFromSlide(undefined)).toBeNull(); + expect(templateDataFromSlide(null)).toBeNull(); + }); +}); diff --git a/assets/tests/shared/templates-lookup.test.js b/assets/tests/shared/templates-lookup.test.js new file mode 100644 index 00000000..ea934a4e --- /dev/null +++ b/assets/tests/shared/templates-lookup.test.js @@ -0,0 +1,24 @@ +import { describe, it, expect } from "vitest"; +import { renderSlide } from "../../shared/slide-utils/templates.js"; + +// The client reads a template id off the slide and looks the bundled module up +// by it - it no longer asks the API for anything. An id with no module in this +// build is therefore the one remaining way a template can be missing, and it +// has to fail loudly rather than render a blank slide: Slide's ErrorBoundary +// turns the throw into the fallback image and moves the region on. +describe("renderSlide template lookup", () => { + it("throws for an id no bundled template has", () => { + expect(() => + renderSlide( + { templateData: { id: "01JZZZZZZZZZZZZZZZZZZZZZZZ" } }, + "run", + () => {}, + ), + ).toThrow(/Cannot find module/); + }); + + it("renders nothing when the slide names no template", () => { + expect(renderSlide({}, "run", () => {})).toBe(""); + expect(renderSlide({ templateData: null }, "run", () => {})).toBe(""); + }); +});