🐛 [RUM-17016] register bridge-config responder at instrument time#170
🐛 [RUM-17016] register bridge-config responder at instrument time#170bcaudan wants to merge 4 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
Stale comment
PR Review — Score: 4.7 / 5
This is a well-targeted fix for the deferred-init hang: registering the
datadog:bridge-configresponder at instrument time, backed by a process-global holder, directly addresses the root cause while preserving safe fallback behavior. TheSymbol.forpattern matches existing instrumentation idempotency,monitor()wraps the IPC callback correctly, and the unit plus e2e coverage give good confidence. Documentation clearly explains the lifecycle and the pre-init reload caveat. I would approve this PR.Why 4.7: Correct architectural split between instrument-time registration and init-time config publication; thorough tests (holder sharing, responder behavior, deferred-init e2e); docs updated in README and ARCHITECTURE.md; fallback values are conservative.
Why not 5: Minor robustness gap around returning a live config reference from the holder, and the new e2e scenario asserts host propagation but not
defaultPrivacyLevel(unit tests cover the holder, so risk is low).
Findings
- [Minor] Live config reference —
getBridgeConfig()returns the holder's object directly; accidental mutation could affect subsequent IPC responses.- [Nit] E2e privacy assertion — deferred-init scenario verifies
allowedWebViewHostsbefore/after init but notdefaultPrivacyLevel.
Architectural flow
LoadingsequenceDiagram participant App participant Instrument participant Holder as Process-global holder participant IPC as ipcMain responder participant Preload participant Init as init() / RendererPipeline App->>Instrument: import instrument (before electron) Instrument->>IPC: register datadog:bridge-config listener Instrument->>Holder: seed fallback (mask, []) Note over App,Preload: Window opens before init() Preload->>IPC: sendSync(datadog:bridge-config) IPC->>Holder: getBridgeConfig() Holder-->>IPC: fallback config IPC-->>Preload: immediate response (no hang) App->>Init: init(config) after consent gate Init->>Holder: setBridgeConfig(real config) Note over App,Preload: New window after init Preload->>IPC: sendSync(datadog:bridge-config) IPC->>Holder: getBridgeConfig() Holder-->>IPC: real config IPC-->>Preload: configured valuesBefore:
RendererPipelineregistered thedatadog:bridge-configresponder only duringinit(). Wheninit()was deferred (e.g. user-consent gating), the preload's synchronoussendSyncfound no listener and blocked the renderer for ~30s.After: The responder is registered unconditionally at instrument time and reads from a process-global holder seeded with safe fallbacks.
init()publishes the real configuration viasetBridgeConfig, so windows opened before init load promptly with fallback values and windows opened after init receive the configured privacy level and webview hosts.Sent by Cursor Automation: electron-sdk reviews
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
b066e98 to
5a5bbec
Compare
5a5bbec to
e089d13
Compare
|
@codex review |
There was a problem hiding this comment.
PR Review — Score: 4.9 / 5
This is a well-targeted fix for the deferred-init hang. Registering the datadog:bridge-config responder at instrument time, backed by a process-global holder, directly addresses the root cause while preserving safe fallback behavior. Follow-up commits since the prior review addressed both earlier findings (getBridgeConfig() now returns a defensive copy; the e2e scenario asserts defaultPrivacyLevel before and after init). The Symbol.for pattern matches existing instrumentation idempotency, monitor() wraps the IPC callback correctly, and the unit plus e2e coverage give strong confidence. Documentation clearly explains the lifecycle and the pre-init reload caveat. I would approve this PR.
Why 4.9: Correct architectural split between instrument-time registration and init-time config publication; prior review feedback fully addressed; thorough tests (holder sharing, copy semantics, responder behavior, deferred-init e2e with bounded load timeout); docs updated in README and ARCHITECTURE.md; fallback values are conservative.
Why not 5: No blocking or meaningful residual issues — the remaining gap is purely stylistic (e.g. setBridgeConfig stores the config reference while getBridgeConfig clones on read; harmless given buildConfiguration produces a stable object and IPC always goes through the copy path).
Architectural flow
sequenceDiagram
participant App
participant Instrument
participant Holder as Process-global holder
participant IPC as ipcMain responder
participant Preload
participant Init as init() / RendererPipeline
App->>Instrument: import instrument (before electron)
Instrument->>IPC: register datadog:bridge-config listener
Instrument->>Holder: seed fallback (mask, [])
Note over App,Preload: Window opens before init()
Preload->>IPC: sendSync(datadog:bridge-config)
IPC->>Holder: getBridgeConfig() (returns copy)
Holder-->>IPC: fallback config
IPC-->>Preload: immediate response (no hang)
App->>Init: init(config) after consent gate
Init->>Holder: setBridgeConfig(real config)
Note over App,Preload: New window after init
Preload->>IPC: sendSync(datadog:bridge-config)
IPC->>Holder: getBridgeConfig() (returns copy)
Holder-->>IPC: real config
IPC-->>Preload: configured values
Before: RendererPipeline registered the datadog:bridge-config responder only during init(). When init() was deferred (e.g. user-consent gating), the preload's synchronous sendSync found no listener and blocked the renderer for ~30s.
After: The responder is registered unconditionally at instrument time and reads from a process-global holder seeded with safe fallbacks. init() publishes the real configuration via setBridgeConfig, so windows opened before init load promptly with fallback values and windows opened after init receive the configured privacy level and webview hosts.
Sent by Cursor Automation: electron-sdk reviews
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |


Motivation
Importing
@datadog/electron-sdk/instrumentbeforeelectronbut deferringinit()(a common pattern whengating on user consent) made the first window/frame load hang for ~30s. The injected preload does a
synchronous
ipcRenderer.sendSync('datadog:bridge-config')at frame commit, but the responder was onlyregistered by
init(). With no responder, Electron blocked the renderer until the sendSync timeout.Fixes #150 (RUM-17016).
Changes
datadog:bridge-configresponder unconditionally at instrument time, so the preload'ssynchronous request always resolves immediately, whether or not
init()has run.(
maskprivacy, no extra webview hosts). It must be a process global because theinstrumentandinit()entry points ship in separate bundles.init()(viaRendererPipeline) now updates the holder with the real config instead of owning theresponder. A window loaded before
init()reads the fallback; one loaded after reads the real config.ARCHITECTURE.mdand the pre-init reload caveat in the README.Test instructions
require('@datadog/electron-sdk/instrument')beforeelectronand do NOT callinit().BrowserWindowand load any content.sendSync ... without listenerswarning.init(...), open a second window, and confirm it picks up the configuredallowedWebViewHosts/defaultPrivacyLevel.Checklist