✨ enable partial view updates by default for CDN users without a proxy - #5000
✨ enable partial view updates by default for CDN users without a proxy#5000mormubis wants to merge 11 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 4d8fb3c | Docs | View more details | Give us feedback! |
Bundles Sizes Evolution
|
Reverts the getSdkSetup accessor and its browser-core export. rum-core now declares __BUILD_ENV__SDK_SETUP__ locally, like the eight existing __BUILD_ENV__SDK_VERSION__ consumers. Drops the four npm-branch tests, which are unreachable under Karma because webpack.base.ts pins setup: 'cdn'. Also fixes prettier on the beta_enable_view_updates line.
Both declares of __BUILD_ENV__SDK_SETUP__ now read 'npm' | 'cdn', matching the setup argument of getBuildEnvDefines. With the previous 'string' type a typo like === 'CDN' would compile and silently disable the feature.
| trackLongTasks: { type: 'boolean', default: true, strict: false }, | ||
| trackViewsManually: { type: 'boolean', default: false, strict: false }, | ||
| betaEnableViewUpdates: { type: 'boolean', default: false }, | ||
| betaEnableViewUpdates: { type: 'boolean' }, |
There was a problem hiding this comment.
I removed the default. With default: false you can't tell if the user passed false or didn't pass anything, both end up as false after validation. So the default is resolved in validateAndBuildRumConfiguration instead.
The intersection already resolves it: (boolean | undefined) & boolean is boolean, and the required member wins over the optional one. Only allowedTracingUrls needs the Omit, because its schema type and resolved type are incompatible.
sdk_setup is merged in via combine() and cast to TelemetryEvent['telemetry'], so the type is never checked there. The narrow only pays off in rum-core where the value is compared against 'cdn'. Leaving telemetry.ts untouched avoids a declare that can go stale if a third setup is ever added.
…global The narrow only guarded a typo in === 'cdn', which the tests already catch. string matches the telemetry declare and the eight __BUILD_ENV__SDK_VERSION__ ones.
Both call sites now just call the helper, matching getRemoteConfigurationId which keeps its own ?? internally. Before this, the two call sites were the only 'field ?? helper()' pattern in the file.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92afd29757
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * TODO next major: remove the option. | ||
| */ | ||
| function isViewUpdatesEnabled(explicit: boolean | undefined, proxy: InitConfiguration['proxy']): boolean { | ||
| return explicit ?? (__BUILD_ENV__SDK_SETUP__ === 'cdn' && !proxy) |
There was a problem hiding this comment.
issue: For CDN sessions that experience a visibility/page-state transition after the initial full view has flushed, this default activates a corrupt diff path: processPageStates emits _dd.page_states newest-first, while computeAssembledViewDiff treats that array as append-only and takes current.slice(last.length). For example, a base [active] followed by [hidden, active] sends [active] instead of the new hidden entry, so the backend loses the transition until a full checkpoint; keep the default off until this array is diffed according to its ordering or sent with replace semantics.
Useful? React with 👍 / 👎.
| * TODO next major: remove the option. | ||
| */ | ||
| function isViewUpdatesEnabled(explicit: boolean | undefined, proxy: InitConfiguration['proxy']): boolean { | ||
| return explicit ?? (__BUILD_ENV__SDK_SETUP__ === 'cdn' && !proxy) |
There was a problem hiding this comment.
issue: The Salesforce bundle is built through the same webpack path marked as setup: 'cdn', but the supported installation flow downloads it and registers or commits it as a Salesforce static resource. With no proxy, this condition therefore opts those pinned deployments into view updates even though publishing a corrected CDN bundle cannot roll them back—the exact safety distinction used to keep npm users disabled. Treat the Salesforce build as pinned for this rollout or override its default explicitly.
Useful? React with 👍 / 👎.
The bridge replaces the batch transport, and view_update events are only created by createBatchDispatcher, so a WebView can never send a partial update. Reporting the option as enabled there inflated the rollout telemetry. The explicit value is ignored too, since it cannot take effect either.
1 participant
Motivation
Partial view updates have been behind
betaEnableViewUpdatessince #4833. This is the Sept 7 step of the Road to GA: enable it by default where we know it's safe.CDN users always run the latest bundle, so we can roll back by shipping a new version. npm users pin a version, so they opt in explicitly.
proxyusers stay off too, a proxy may not forward theview_updateevent type yet.Changes
betaEnableViewUpdatesdefaults totrueon CDN builds withoutproxy, andfalseeverywhere else. An explicit value from the user always wins.The schema
defaultonly takes static values, so I removed it from the field and resolve it invalidateAndBuildRumConfigurationinstead, next to the other post-processed options. That's the part worth a look: withdefault: falseyou can't tell "passedfalse" from "passed nothing", both come out asfalse. Without the default an unset option isundefined, soisViewUpdatesEnabledcan fill it in and an explicitfalsesurvives.Telemetry now reports the resolved value instead of the raw one. Otherwise every defaulted user shows up as
undefinedand we can't follow the rollout.CDN vs npm comes from
__BUILD_ENV__SDK_SETUP__, declared locally inconfiguration.tslike the other build-env consumers do. Unit tests and the e2e bundles are both webpack builds, so both run ascdn. Nothing covers the npm branch automatically.Test instructions
yarn devand open the sandbox. It setsproxy: '/proxy', so the RUM configuration telemetry should reportbeta_enable_view_updates: false.proxyinsandbox/index.htmland reload. It should now betrue. Requests fail auth without the proxy, that's expected, the configuration event still goes out.yarn test:unit --spec packages/browser-rum-core/src/domain/configuration/configuration.spec.tsfor the proxy/no-proxy and explicit-value cases.Checklist
proxy, so they cover the disabled path.