SCAL-336321 Order the pre-render show cycle, and always state its runtime filters - #659
Merged
Merged
Conversation
…ests reconcileRuntimeParams triggers UpdateRuntimeFilters after UpdateEmbedParams, so UpdateEmbedParams is no longer the last processTrigger call. Two showPreRender tests asserted it with toHaveBeenLastCalledWith and failed; they now assert the same payload with toHaveBeenCalledWith. Make the fallback branch null-safe. getPreRenderObj() reads an untyped property off the pre-render wrapper node, so it can return an object with no viewConfig; reading viewConfig.runtimeFilters off it threw a TypeError that the surrounding catch turned into a logger.error, which jest-setup escalates to a fatal error and surfaced on an unrelated liveboard.spec test. Add coverage for the three reconcile paths — filters from the new config, clearing the filters left by the previous config, and no filters at all — plus the missing-viewConfig case. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…ts params land `LiveboardEmbed.beforePrerenderVisible()` queued two container-ready callbacks: the base one that posts `UpdateEmbedParams`, then its own that posts `HostEvent.Navigate`. The first suspends on `await getUpdateEmbedParamsObject()` (which awaits `getAppInitData()`), so the second ran to completion first and the real order on the wire was `Navigate`, `UpdateEmbedParams`, `UpdateRuntimeFilters`. The container therefore started loading the new liveboard while still holding the previous config's `runtimeFilterParams`, and the params that arrived mid-load were dropped — the reported symptom of a filter sticking across a shared pre-render and `runtimeFilters: []` failing to reset it. `beforePrerenderVisible()` now publishes `preRenderParamsApplied`, which resolves once the params (and the `reconcileRuntimeParams` follow-up) have been posted and the container has had 200ms to apply them; the liveboard navigation awaits it. It resolves on the failure path too, so navigation is delayed but never blocked, and both callbacks are gated on the same container-ready signal, so neither can strand the other. Ordering is pinned by a test verified to fail without the change: it observed `["Navigate", "updateEmbedParams", "UpdateRuntimeFilters"]`. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses race conditions in pre-rendered embeds by delaying navigation until the UpdateEmbedParams payload has settled and by reconciling runtime filters on shared pre-renders. The review feedback highlights two key improvements: safely guarding the .map() call on prevRuntimeFilters with Array.isArray to prevent runtime TypeErrors, and explicitly asserting the presence of UpdateEmbedParams in test assertions to avoid false positives from indexOf returning -1.
commit: |
…iew config flag reconcileRuntimeParams() fires an extra HostEvent.UpdateRuntimeFilters on every show-cycle of a shared pre-render, and re-sends the previous config's filters with empty values when the incoming config declares none. That is the right behaviour for an app that shares one preRenderId across embeds with different runtime filters, but it is a behaviour change for everyone else. Put it behind BaseViewConfig.reconcileRuntimeFiltersOnPreRender, defaulting to off, so the reconcile is opt-in per embed. The ordering half of this PR — holding Navigate until the show-cycle's UpdateEmbedParams has been posted and settled — is unconditional and unchanged. Tests: the four reconcile cases now opt in explicitly, and a new case pins the default, asserting no UpdateRuntimeFilters goes out when the flag is absent even though the previous config left filters on the pre-render. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…le parameters too The wrapper's embed back-pointer was written only by the instance that created the pre-render, so it never moved. getPreRenderObj() therefore meant "the creator", not "what is showing", and from the second hand-over on the reconcile compared against a config that had stopped being current a hop earlier: A -> B -> C had C clearing A's filters instead of B's. showPreRender() now ends with takeOverPreRender(), which repoints the wrapper at the instance being shown. beforePrerenderVisible() captures the predecessor synchronously, before any await, because the reconcile runs in an async callback by which time the wrapper already says `this`. connectPreRendered() adopts the state that belongs to the shared iframe, beside inheritPreRenderContainer(). That hand-over exposed a latent trap in isEmbedContainerLoaded: the flag is set per instance by whichever ones are subscribed when the container announces itself, and hidePreRender() unsubscribes. An instance hidden before the container came up would report false forever and, once it owned the wrapper, hand that false to everyone after it — stranding UpdateEmbedParams, Navigate and getCurrentContext permanently. The flag now lives on the wrapper node, where it belongs: it describes the iframe, not an instance. Also: - reconcileRuntimeParams() split into filters and parameters. Parameters get the re-apply half only: a parameter always carries a value, so there is no equivalent of `values: []` and inventing one would be wrong for numeric and boolean parameters. The December 2025 container change broke parameters in exactly the same way as filters; it simply went unreported. - The flag is renamed reconcileRuntimeFiltersOnPreRender -> reconcileRuntimeParamsOnPreRender now that it gates both. It has never shipped, so this is not a breaking change. - LiveboardEmbed writes currentLiveboardState on itself rather than on the instance it took over from, so "current" is true of the owner again. - Documented that the reconcile is what the LB1 -> LB1 case depends on; when the liveboard changes, the ordered Navigate reloads the route anyway. Tests: A -> B -> C chain; hand-over of the pointer; show -> hide -> container loads -> third instance shows (verified to fail without the wrapper-node flag); parameters re-applied in order; no UpdateParameters when the config declares none. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The flag belongs with the other pre-render options rather than on BaseViewConfig: preRenderConfig.reconcileRuntimeParams, read through getPreRenderConfig(). Also strips the explanatory comment blocks added with this change down to one or two lines where the reason is not obvious from the code.
…-facing checkEmbedContainerLoaded already resolves the container state on connect, so adoptPreRenderState copied a flag nobody was waiting on, and the comment justifying it claimed a stranding that cannot happen. The public doc said what the SDK does internally — UpdateEmbedParams is not public API, so a customer cannot act on it. It now says what the flag is for and when to set it.
The reconcile no longer posts its own HostEvent.UpdateRuntimeFilters (or UpdateParameters). The show-cycle's UpdateEmbedParams payload already carries this config's runtimeFilters and runtimeParameters, so the only thing it cannot express on its own is the reset: a config that declares no filters, where the columns to clear belong to the previous config. Those are now folded into the same payload with empty `values` rather than sent after it. One post instead of two or three, and the ordering question between them disappears — Navigate still waits on the same settle window. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…erParams The payload carries runtime filters in two fields, and the reconcile was writing the one the container ignores. `runtimeFilters` is the raw array, present only because the payload spreads the view config; `runtimeFilterParams` is the serialized `col1=…&op1=…&val1=…` string the container reads. getDefaultAppInitData() builds that string from viewConfig.runtimeFilters, so it is already right when a config declares filters — and null in exactly the case that leaks, a config that declares none, since the columns to reset belong to the predecessor and nothing in this view config names them. The observed payload was `runtimeFilterParams: null` beside a populated `runtimeFilters` array. Both fields are now written from the same reconciled list. Serializing through getFilterQuery leaves a dangling separator for a filter with no values (`col1=Color&op1=IN&`), so empty segments are dropped here rather than in the shared builder every URL render goes through. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…eEmbedParams
Drops the reconcileRuntimeParams flag and the predecessor tracking behind it.
The show-cycle payload now always carries runtimeFilterParams — this config's
filters serialized, or `&` when it declares none.
`&` rather than null or '' because the container only reads the field when it
is truthy:
if (embedConfigFromEvent.runtimeFilterParams) { ...updateRuntimeFilterParams(parsed) }
so both falsy forms read as "no news" and leave the previous embed's filters
standing — the filter that sticks. A lone separator passes that gate and
`new URLSearchParams('&')` yields no pairs, so the container's state becomes {}
and it falls through to embedParams, which the same event has just rebuilt from
this view config.
Stating the filters outright means never having to work out what the previous
embed's were, so getPredecessorViewConfig() and the predecessor capture go. It
also closes `runtimeFilters: []`, which serialized to null before and now reads
as empty like any other config with no filters.
takeOverPreRender() stays: it carries the isEmbedContainerLoaded hand-over fix
and currentLiveboardState ownership, which are separate bugs.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
runtimeParameterParams has the identical shape to runtimeFilterParams: the container reads it only when truthy, and falls through to embedParams when the parsed object is empty. So a config declaring no parameters left the previous embed's standing, exactly as its filters did. The payload now always states them too — serialized, or `&` when there are none. Earlier revisions left this alone because a parameter always carries a value and there is no empty form to invent; with the marker there is nothing to invent, since it says "none at all" rather than "this one, emptied". No segment trimming here, unlike the filters: a parameter always has both halves of its pair, and `paramVal1=` is an empty-string value rather than an absence. Also a TODO on the pre-render block: it wants its own file, the way full height got one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Showing a pre-render that is already on the requested liveboard sent a Navigate the container treats as a no-op, so the liveboard component was never unmounted and kept the state it had accumulated — moved filter chips, selections, tab. That show now goes lb -> home -> lb. Home unmounts the liveboard, so the Navigate that follows rebuilds it from the UpdateEmbedParams posted just before. The comparison is the whole route, not the liveboard id: the path is built from the viz, tab and personalized view too, so a same-liveboard show that moves to another tab is a real navigation and needs no help. What the container is showing is read from the predecessor's currentLiveboardState, captured synchronously because showPreRender() hands the wrapper over after this runs. The home hop is awaited — two Navigates in the same tick are a router's invitation to coalesce — and a failure warns and carries on, since stale state beats a liveboard that never comes back. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Taking the pre-render over from itself is a hide/show of one embed, not a hand-over, so the home round trip must not fire. The state the liveboard is holding there is that embed's own — the filter chips this user moved on this liveboard — and clearing it would throw away their session for a visibility toggle. The predecessor is now compared against `this` before its route is read, so a self-takeover leaves the route check with nothing to match and takes the ordinary path. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…r sends The home hop awaited its trigger. The container does not acknowledge Navigate, so that promise did not resolve on the route change — it resolved when TRIGGER_TIMEOUT fired, parking the re-shown liveboard on the home page for a full 30 seconds. Observed on a cluster: Navigate home at 18:55:56, the liveboard back at 18:56:26. It now posts home and waits HOME_UNMOUNT_SETTLE_MS (200ms) instead, the same reasoning and the same order of magnitude as the params settle window. The wait is still needed — both routes in one tick invites the router to coalesce them, leaving the liveboard mounted — but it is now bounded by us rather than by a timeout. The trigger keeps a catch so its eventual rejection does not surface as an unhandled promise. Regression test uses a promise that never settles, which is what a real home hop looks like; it fails if the await comes back. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
… down Same behaviour, much less of it. The two serialize helpers and their JSDoc fold into the payload builder, clearLiveboardStateViaHome folds into its one caller, the empty-segment trim goes (URLSearchParams ignores a trailing separator), and three overlapping tests go. Comments are two lines each, saying why rather than restating the code. Widened the home-hop test margin to 900ms: it clears two 200ms settle windows and the old 600ms flaked once on a full run. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
adityamittal3107
approved these changes
Sep 15, 2026
utsavkapoor
approved these changes
Sep 15, 2026
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
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.
Problem
On a shared pre-render, a
runtimeFiltersvalue from one liveboard sticks when the host switches to another liveboard, andruntimeFilters: []does not reset it. The container fix attempted in blink-v2 ([SCAL-333947], PR #68332) did not close it and has been reverted.There are three independent causes. The host events go out in the wrong order, the payload never says "no filters" in a way the container reads, and a same-route show never unmounts the liveboard that is holding the stale state.
Cause 1 — the host events go out in the wrong order
LiveboardEmbed.beforePrerenderVisible()queues two container-ready callbacks:super.beforePrerenderVisible()→ postsHostEvent.UpdateEmbedParamsHostEvent.NavigateBoth are invoked in registration order, but the first one suspends on
await this.getUpdateEmbedParamsObject()(which awaitsgetAppInitData()). It yields at thatawait, callback 2 runs to completion, and the real order on the wire is:That is observed output, not a reading of the code — it is what the new test records when the fix is removed.
So the container starts loading the new liveboard while still holding the previous config's
runtimeFilterParams, and the params that arrive mid-load are dropped.Fix
TsEmbed.beforePrerenderVisible()now publishespreRenderParamsApplied, a promise that resolves onceUpdateEmbedParamshas been posted and the container has hadUPDATE_EMBED_PARAMS_SETTLE_MS(200ms) to apply it.LiveboardEmbedawaits that before triggeringNavigate.The settle window is deliberate rather than an ack-wait: the container applies the payload through React state, so the post being delivered is not the same as the new params being in effect, and awaiting
trigger()would risk stalling navigation on the 30s trigger timeout.Two properties worth checking in review:
beforePrerenderVisible(), outsideexecuteAfterEmbedContainerLoaded, so the navigation callback has something to await whether the container is already loaded or not;finally, so a params failure delays navigation but never blocks it. Both callbacks are gated on the same container-ready signal, so neither can strand the other.Cause 2 — nothing in the payload says "no filters"
The payload carries runtime filters in two fields:
runtimeFiltersruntimeFilterParamscol1=…&op1=…&val1=…string — what the container readsgetDefaultAppInitData()builds the serialized form fromviewConfig.runtimeFilters, and only whenexcludeRuntimeFiltersfromURLis set, because a URL render puts the filters in the iframe's query string instead. A show cycle has no URL to put them in. So a config that declares no filters sent this:{ "runtimeFilterParams": null, // ← the field the container reads "runtimeFilters": undefined }and the container's handler is:
nullis falsy, so the branch never runs and the container keeps the previous embed'sruntimeFilterParamsstate.getRuntimeParamsthen prefers that state overembedParams:so the stale value outranks the freshly-rebuilt params on every show. That is the filter that sticks.
Fix
The show-cycle payload now always states both — this config's serialized, or
'&'when it declares none:runtimeParameterParamshas the identical shape — same truthy gate atembed.container.tsx:192, same!_.isEmpty(embedParameterParams) ? embedParameterParams : embedParamsfall-through, andgetRuntimeParameterInput({})breaks on a missingparam1exactly as the filter reader breaks on a missingcol1. So a config declaring no parameters left the previous embed's standing, just as its filters did. The December 2025 container change broke both; only the filters were reported.One difference: no segment trimming for parameters. A parameter always has both halves of its pair, and
paramVal1=is an empty-string value rather than an absence — there is a test pinning that it does not collapse to the empty marker.The same is true of
runtimeParameterParams,updateEmbedParameterParamsandgetRuntimeParameterInput.Why
'&'and notnullor''. Both falsy forms read as "no news" at that gate and leave the previous filters standing. A lone separator is truthy, so the branch fires — andnew URLSearchParams('&')yields no pairs (the urlencoded parser skips empty sequences), soparsedFiltersis{}andupdateRuntimeFilterParams({})replaces the state with an empty object._.isEmpty({})is then true, so the container falls through toembedParams— whichupdateEmbedParams(processedParams)has just replaced, from this same event, with this config's params. A config with no filters contributes nocol1,getRuntimeFilterInputbreaks on the first iteration, and the result is[].Both
updateRuntimeFilterParamsandupdateEmbedParamsare plainuseStatesetters (embed.container.tsx:66,77), so each is a replace, not a merge — that is what makes the fall-through safe.What this removes
Stating the filters outright means the SDK never has to work out what the previous embed's were. So the earlier revisions of this branch are gone:
preRenderConfig.reconcileRuntimeParamsflag — deleted, not defaulted on. There is nothing left to opt into: no extra host events, no extra traffic, one field in a payload that was already being sent.getPredecessorViewConfig()and the predecessor capture — deleted. The clear-by-empty-values approach they existed for needed to name the predecessor's columns, and correctness then depended on the predecessor chain being right across every A → B → C hand-over.HostEvent.UpdateRuntimeFilters/UpdateParametersposts — deleted. They re-sent values the payload already carried, and added a second ordering question next toNavigate.Net −119 lines against the previous head of this branch.
It also closes
runtimeFilters: [], flagged as an open question on the earlier revision: it serialized tonullbefore, and now reads as empty like any other config with no filters.takeOverPreRender()stays — see below. It is carrying two bugs of its own.Pre-render ownership
preRenderWrapper[__tsEmbed]was written only inhandleInsertionIntoDOM(), i.e. by the instance that creates the pre-render, and never moved, sogetPreRenderObj()meant "the creator", not "what is showing".showPreRender()now ends withtakeOverPreRender(), which repoints the wrapper at the instance being shown.connectPreRendered()adopts the state that belongs to the shared iframe, besideinheritPreRenderContainer().That hand-over exposed a latent trap worth reviewing carefully.
isEmbedContainerLoadedwas set per instance, by whichever ones were subscribed when the container announced itself — andhidePreRender()unsubscribes. An instance hidden before the container came up reportsfalseforever, and once it owned the wrapper it handed thatfalseto everyone after it:UpdateEmbedParamsnever fires,preRenderParamsAppliednever resolves,Navigatenever goes out,getCurrentContext()hangs. Permanently, silently. The flag now lives on the wrapper node, where it belongs — it describes the iframe, not an instance. The regression test for it was verified to fail without that change.LiveboardEmbedalso now writescurrentLiveboardStateon itself rather than on the instance it took over from, so "current" is true of the owner again.Cause 3 — a same-route show never unmounts the liveboard
Showing the same liveboard again through the shared pre-render (LB1 → LB1) sent a
Navigateto the route the container was already on, which its router treats as a no-op. The liveboard component was never unmounted, so it kept everything it had accumulated — filter chips the user had moved, selections, the active tab — andUpdateEmbedParamswas left as the only thing carrying the new values into a component that had already made up its mind.Fix
That show now goes lb → home → lb. Home unmounts the liveboard; the
Navigatethat follows rebuilds it from theUpdateEmbedParamsposted just before.Three things worth review:
currentLiveboardState, captured synchronously inbeforePrerenderVisible()—showPreRender()callstakeOverPreRender()afterwards, so by the time the async callback runsgetPreRenderObj()names this instance instead. It diverts only on a positive match: an unknown predecessor takes the ordinary path, since a plainNavigateto a route the container is not on does the unmounting by itself.thisbefore its route is read.Navigate, so awaiting that trigger does not resolve on the route change — it resolves whenTRIGGER_TIMEOUTfires. An earlier revision of this branch did await it, and parked the re-shown liveboard on the home page for a full 30 seconds:Navigate homeat 18:55:56, the liveboard back at 18:56:26, measured on a cluster. It now posts home and waitsHOME_UNMOUNT_SETTLE_MS(200ms), the same reasoning and magnitude as the params window. The wait is still needed — both routes in one tick invites the router to coalesce them, leaving the liveboard mounted — but it is bounded by us rather than by a timeout. The trigger keeps acatchso its eventual rejection is not an unhandled promise, and a failure warns and carries on: stale state beats a liveboard that never comes back.Verification
await this.preRenderParamsAppliedremoved,should trigger Navigate only after UpdateEmbedParams has settledfails with the received array quoted above. It is not a test that passes either way.serializeRuntimeFiltersto returnnullfor an empty list fails 4 tests and nothing else — the three "should mark the params empty…" cases and the hand-along chain. Dropping theruntimeParameterParamsline fails its own case.tsc --noEmitclean; eslint 0 errors and no new warnings (20 on the touched files, same as baseline).col1=Color&op1=IN&val1=red&val1=bluefor a declared filter,param1=Region%20Param¶mVal1=Westfor a parameter,'&'for none of either — and pin that noUpdateRuntimeFilters/UpdateParametersfollows the payload.AuthInittest needed 1305ms rather than 1005ms — 1000ms container-ready fallback plus the 200ms window.Notes for the reviewer
'&'is the reviewable decision. It works because the container's gate is a truthiness check, and it is not a shape blink documents. If someone tidies it to'', the fix silently reverts with no blink-side test to catch it — hence the named constant and the comment at both the constant and the call site. The non-fragile version is a one-character container change,!= nullinstead of truthy, which would let this be''; worth filing against blink so this can be simplified later.embed.container.tsx:190-207,runtime-filters.util.ts:267-295,embed-util.ts:718), not executed. The step most worth confirming by hand isnew URLSearchParams('&')→ no pairs →{}.'home', the same stringAppEmbedsends forPage.Home. Worth a look from someone who knows whether a Liveboard-mode container renders anything visible at that route during the hop — a flicker would be the cost of clearing the state.runtime-filters.util.ts:390endsembedFilter.length > 0 ? embedFilter : getRuntimeFilter(), so an empty result there reads filters off the iframe URL.V1EmbeddefaultsexcludeRuntimeFiltersfromURL: true, so the URL is normally bare and this lands on[]— but a customer who sets that flagfalsecould see the creating embed's filters come back throughPinboardAttributeFilterContainer.TsEmbed— wrapper elements, ownership keys, lifecycle, positioning, and now the params contract — and two of the bugs fixed here came from state being owned by the wrong thing. Filed as SCAL-338011, with aTODOat thepreRenderWrapperdeclaration. Refactor only, best done after this merges.LiveboardEmbednavigates its pre-render on show;AppEmbeddoes not overridebeforePrerenderVisible, so it is unaffected.Refs SCAL-336321, SCAL-333947.