Feature/client rtx migration - #3
Draft
tuj wants to merge 89 commits into
Draft
Conversation
…it tests to vitests
tuj
marked this pull request as draft
June 2, 2026 13:32
Two release/3.0.0 fixes landed in files this branch had moved, so git could not follow the renames and they arrived as delete/modify conflicts. Both are re-implemented on top of RTK Query: - Issue os2display#517: getAllResultsFromPath paginated by comparing the collected count against hydra:totalItems, which turned one bad response into ~435k requests over 21 hours. core/api-query.js queryAllPages already followed hydra:next; it now also stops on an empty page, caps at MAX_PAGES 100 to match the admin's get-all-pages helper, and rejects instead of returning partial results so a truncated playlist cannot reach a screen as if complete. Playlist preview moves from query() to queryAllPages(), lifting its one-page cap. - Retrieving-bind-key spinner: kept, wired to the context callbacks and reauthenticateRef rather than the removed document event bus. Tests carried in by the release were ported to the new architecture: api-helper-pagination.test.js folded into api-query.test.js (keeping the os2display#517 regression case), app-bind-key.test.jsx folded into app.test.jsx, and playlist-form-tenants-pagination.test.jsx re-pointed at admin/redux. Also applies task coding-standards:assets:apply over 26 files that were already failing the prettier check before this merge.
API Specification - Non-breaking changesAPI Changelog 1 vs. 1API ChangesGET /v2/media/{id}
|
Follow-up to the release/3.0.0 merge, from re-review of the conflict resolution. queryAllPages rejected on a failed page but still returned the rows it had collected when it hit MAX_PAGES, and the cap is exactly the pathological case os2display#517 describes. It now rejects there too, so the docblock's promise that a caller cannot mistake a truncated result for a complete one actually holds. That reject then reached getRegions and getSlidesForRegions, where Promise.allSettled logged the failure and moved on, leaving slidesData unset. enrichSlides fell back to an empty list, so one bad page mid-pagination blanked the region. Upstream did not do this: getAllResultsFromPath returned {}, Object.keys(undefined) threw in getScreen, and the whole cycle was abandoned with the previous content left on screen. Both helpers now give the cycle up explicitly, so a screen keeps what it has and retries on the next pull.
regionRemoved() clears one region's interval and is driven by Region unmounts. ContentService.stop() replaces onRegionRemoved with a no-op before App clears the screen, so those unmounts arrived nowhere: the abandoned service kept one setInterval per region running and kept calling updateRegionSlides on the live React setters. On the reauth path that happened once per failed refresh, so intervals accumulated for as long as the device ran — and these screens run for months without a reload. ScheduleService gains stopAll(), called from ContentService.stop() before the callbacks are detached. updateRegion() also bails when stopped, for two reasons: a pull already in flight at teardown still lands there, and the interval is registered inside loadConfig().then(), where the captured intervals map is not the one stopAll() swaps in — so only the flag keeps a late registration off the live map.
pull() races getScreen against a timeout, but rejecting the race does not cancel the getScreen behind it. The orphan kept running and could still call onContent with stale data — and worse, write previousScreenChecksums and previousSlideChecksums over the values a newer cycle had already recorded. The next cycle would then compare against those older checksums, conclude nothing had changed, and stop refetching: content frozen on the screen with no error anywhere. Each pull now claims a generation. getScreen takes the generation it belongs to and bails at the points where it already checked stopped, including immediately before the checksum writes — nothing awaits between that gate and onContent, so passing it means the cycle is still current. stop() bumps the generation too, because start() clears the stopped flag and would otherwise let an abandoned cycle come back to life.
The media.yaml operationId change (getv2MediaById to get-v2-media-by-id) renamed the generated hook to useGetV2MediaByIdQuery, but enhanced-api.ts still destructured the old lowercase name. That export has been undefined ever since. Nothing imports it today, which is why it went unnoticed, but the next caller would have got undefined is not a function.
release/3.0.0 deleted this component in 968e0ac; the branch still carried it. Nothing imports it, and it reads translations under a campaign-icon keyPrefix that has no entries in da/common.json, so it could never have rendered its labels anyway. The overridden-by-campaign strings it referenced live under screen-list and are equally unused on release/3.0.0, so they are left alone rather than cleaned up from this branch.
query() falls back to the RTK Query cache whenever a request fails, which is what keeps a screen rendering through a network outage. For screens, layouts, templates and media that is exactly right. Feeds are the exception: enrichSlide fetches them with forceRefetch precisely because the content must be fresh, but the fallback then served arbitrarily old data with no staleness signal, so a wall display could show feed content weeks out of date and looking current. query() takes an optional maxAge that gates only the cache fallback. It compares against fulfilledTimeStamp, which is the last successful fetch: verified against @reduxjs/toolkit 2.8.2 that writePendingCacheEntry sets only status, requestId, originalArgs and startedTimeStamp, and the queryThunk.rejected reducer sets only status and error, so neither clears data or fulfilledTimeStamp and it survives any number of failed refetches. A missing timestamp cannot be shown to be fresh, so it counts as too old. enrichSlide reads feedMaxAge from client config (default 24 hours) and now marks a slide invalid when its feed cannot be resolved, rather than rendering it with no data. region.jsx and touch-region.jsx already filter invalid slides, and if that empties a region ScheduleService.checkForEmptyContent shows the tenant fallback image. A blank branded screen is the right outcome for a stale feed; a template rendering an empty data set is not, because it is indistinguishable from a genuine empty result.
keepUnusedDataFor: 2592000 was meant to mirror JWT_SCREEN_REFRESH_TOKEN_TTL at 30 days, but RTK clamps it to THIRTY_TWO_BIT_MAX_TIMER_SECONDS (2147483647 / 1e3 - 1), so the effective retention was about 24.85 days and the comment was quietly wrong. Infinity is explicitly supported — handleUnsubscribe returns before setting any timer — and says what is meant. Comment records why the window is long: query() unsubscribes as soon as it resolves, so entries must outlive the pull interval for relationsChecksum-unchanged relations to be served from cache instead of refetched every cycle, and the cache is what carries a screen through an outage. Staleness is bounded per resource via query()'s maxAge instead.
The client bundled all 86 generated operations while calling 16. That shipped every admin endpoint, create/update/delete included, to a device standing unattended in a public space, and registered all of them on the RTK Query slice. openapi-config.js now passes filterEndpoints. The names are matched against the generated endpoint keys, so the list is the same set the client already used: verified by cross-referencing every one of the 86 names against assets/client, and the regenerated file contains exactly those 16. generated-api.ts drops from 2705 lines to 433; the admin client is unaffected and regenerates byte-identical. Adding a client call now means adding its name to the list first, or the endpoint will not exist on clientApi.
46d77aa straightened out what looked like a typo in screen.jsx: gridTemplateColumns was assigned gridTemplateRows and vice versa. The swap is load-bearing. createGrid() builds its outer array from its columns argument, and each quoted string it emits is a CSS row whose names are CSS columns, so the areas it returns are transposed relative to the argument names: createGrid(3, 2) yields 'a b' 'c d' 'e f', three CSS rows of two CSS columns. Assigning the track lists straight through leaves grid-template-areas declaring configColumns rows while grid-template-rows sizes configRows tracks. They agree only on square grids, and six of the nine layouts in assets/shared/screen-layouts are asymmetric — six-areas and touch-template (4x44), two-boxes-vertical and -reversed (1x5), three-boxes-horizontal (3x1), two-boxes (2x1). The same change was proposed upstream as os2display#389 and closed unmerged for this reason; it has its own branch here in #6, where fixing createGrid or the layout JSON convention belongs. The screen test pinned the regression and could not detect it, because createGrid is mocked there; it now asserts the compensated values.
Covers what this branch adds on top of release/3.0.0 only: the os2display#517 pagination fix and the bind-key spinner came in with the merge and are already documented under 3.0.0-rc8 and 3.0.0-rc1. Bugs introduced and fixed within the branch are left out, as are the tests.
enrichSlide already read config?.feedMaxAge ?? defaults.feedMaxAgeDefault, but nothing produced it: ClientConfigController never emitted feedMaxAge and no env var backed it, so the config branch was unreachable and only the 24 hour default could ever apply. Adds CLIENT_FEED_MAX_AGE, wired through config/services.yaml and emitted by ClientConfigController alongside the other client timings. The default is 86400000 ms, identical to defaults.feedMaxAgeDefault, so behaviour is unchanged until an operator tunes it. Milliseconds to match the surrounding CLIENT_* intervals.
The feed staleness bullet described a fixed 24 hour limit, which was accurate when only defaults.feedMaxAgeDefault could apply. CLIENT_FEED_MAX_AGE now backs it, so the entry names the env var and its default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Replaces the screen client's hand-rolled fetch layer and
documentCustomEvent bus with RTKQuery and a React context. 127 files, +7,922 / −1,783 against
release/3.0.0(d588a380).Upstream issue: os2display#404
What moved where
client/data-sync/api-helper.jsclient/core/api-query.jsquery()/queryAllPages()over RTK Queryclient/data-sync/pull-strategy.jsclient/service/pull-strategy.jsgetScreensplit intofetchLayoutAndRegions,buildCampaignLayout,enrichSlidesclient/data-sync/data-sync.jsclient/service/data-sync.jsclient/logger/logger.js,client/util/{app-storage,client-config-loader,local-storage-keys}.jsclient/core/assets/shared/redux/assets/admin/redux/client/redux/New:
client/client-state-context.jsx(replaces the event bus) andclient/redux/(
empty-api,enhanced-api,generated-api,base-query,store,reauthenticate-ref).Services no longer dispatch
documentevents. They receive acallbacksref at construction andcall it directly; components read state through
useClientState().Behaviour changes worth knowing
queryAllPagesrejects rather than returning apartial collection, and
getRegions/getSlidesForRegionsabandon the cycle instead ofleaving a region empty. A screen keeps its current content and retries on the next pull.
CLIENT_FEED_MAX_AGE(new, default 24h)is not served from cache; the slide is marked invalid and filtered out. If that empties a
region, the tenant fallback image shows. A template rendering an empty feed is
indistinguishable from a genuine empty result, which is why the slide goes instead.
433 lines), so a device in a public space no longer ships the admin's write operations.
Adding a client call means adding its name to
filterEndpointsinclient/redux/openapi-config.jsfirst.keepUnusedDataFor: Infinity— the previous2592000was silently clamped to ~24.85 days byRTK. The cache is what carries a screen through an outage; staleness is bounded per resource
via
query()'smaxAgeinstead.Fixes to shipped behaviour
ScheduleServiceleaked one interval per region on teardown —ContentService.stop()detachedthe callbacks before the Region unmounts that would have cleared them, so the reauth path
accumulated timers for as long as the device ran.
getScreenkept running and could overwrite a newer cycle'srelationsChecksumvalues, convincing the client nothing had changed and stopping content updates. Cycles now
carry a generation.
useGetv2MediaByIdQueryin the admin'senhanced-api.tshad been undefined since themedia.yamloperationId rename.Carried over from release/3.0.0
The merge (
5e2ba951) brought two fixes that lived in files this branch had moved, so theyarrived as delete/modify conflicts and were re-implemented on RTK Query:
hydra:totalItemsinstead ofhydra:next, once turning a singleresponse into ~435k requests over 21 hours.
queryAllPagesfollowshydra:next, stops on anempty page and caps at 100.
Also reverted
46d77aac, which "fixed" thegridTemplateColumns/gridTemplateRowsswap inscreen.jsx. The swap is load-bearing:createGrid()returns areas transposed relative to itsargument names, so removing it misrenders the six of nine stock layouts that aren't square. Same
change as os2display#389, closed unmerged for this reason; it belongs in #6.
How to verify
task test:unit— 331 unit tests, 25 new files.Then, on a real screen:
localStorage.setItem("apiToken", "invalid")and wait forthe next pull. Expect: 401 →
reauthenticateRef→ refresh fails → storage cleared, screenblanks, spinner returns, bind key appears. Watch that scheduling intervals stop — before
869792cfthey kept firing.synthetic single-region layout to take over, then revert cleanly when the campaign expires
(
previousHadActiveCampaignforces a layout refetch on the way back).region and in playlist preview. A screen in more than one screen group should pick up
campaigns from every group. Request count must stay bounded.
Known gaps
Authenticationtag inclient/redux/enhanced-api.tsisn't provided by any clientendpoint, so those
invalidatesTagsare inert. Harmless, not cleaned up here.checkLoginnow goes throughbase-query, so/v2/authentication/screenreceives anAuthorization: Bearerheader from localStorage where the old fetch sent cookies only. Worthconfirming the server ignores a stale token there.