Skip to content

fix(electron): ensure valid values cross bridge#1256

Draft
joker23 wants to merge 2 commits intomainfrom
skz/sdk-1944/electron-json-variation
Draft

fix(electron): ensure valid values cross bridge#1256
joker23 wants to merge 2 commits intomainfrom
skz/sdk-1944/electron-json-variation

Conversation

@joker23
Copy link
Copy Markdown
Contributor

@joker23 joker23 commented Apr 8, 2026

This PR will ensure that the values we pass through the electron bridge are serializable by electron. This is important because the bridge code will throw if none serializable values are passed in.


Note

Medium Risk
Changes renderer→main IPC behavior for variation/track/JSON calls by catching sendSync serialization failures and returning undefined, which could alter downstream expectations if callers relied on throws or guaranteed return values. Adds a new log IPC channel, so miswiring could hide errors if logging is not configured.

Overview
Prevents renderer-side ipcRenderer.sendSync calls from throwing when passed non-IPC-serializable values by routing jsonVariation, jsonVariationDetail, variation, variationDetail, and track through a new safeSendSync wrapper that drops the value and returns undefined.

Adds a new synchronous IPC channel log so the renderer can emit a warn message to the main process; ElectronClient dispatches this to the configured logger when the severity matches a valid LDLogger method. Tests were updated to cover log dispatch, invalid severity handling, and the new “sendSync throws → warn + undefined” behavior.

Reviewed by Cursor Bugbot for commit fea3339. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

@launchdarkly/js-sdk-common size report
This is the brotli compressed size of the ESM build.
Compressed size: 25661 bytes
Compressed size limit: 29000
Uncompressed size: 126143 bytes

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

@launchdarkly/browser size report
This is the brotli compressed size of the ESM build.
Compressed size: 179348 bytes
Compressed size limit: 200000
Uncompressed size: 830127 bytes

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

@launchdarkly/js-client-sdk size report
This is the brotli compressed size of the ESM build.
Compressed size: 31736 bytes
Compressed size limit: 34000
Uncompressed size: 112937 bytes

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

@launchdarkly/js-client-sdk-common size report
This is the brotli compressed size of the ESM build.
Compressed size: 37181 bytes
Compressed size limit: 38000
Uncompressed size: 204409 bytes

@joker23 joker23 force-pushed the skz/sdk-1944/electron-json-variation branch from 0880a72 to 72dbb13 Compare April 8, 2026 21:11
@joker23
Copy link
Copy Markdown
Contributor Author

joker23 commented Apr 8, 2026

@cursor review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a useful common tool?

@joker23
Copy link
Copy Markdown
Contributor Author

joker23 commented Apr 8, 2026

@cursor review

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 2464c85. Configure here.

@joker23 joker23 force-pushed the skz/sdk-1944/electron-json-variation branch 2 times, most recently from db7a834 to a5237e3 Compare April 9, 2026 23:49
@joker23 joker23 force-pushed the skz/sdk-1944/electron-json-variation branch from a5237e3 to fea3339 Compare April 10, 2026 00:01
@joker23
Copy link
Copy Markdown
Contributor Author

joker23 commented Apr 10, 2026

@cursor review

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit fea3339. Configure here.

if (typeof logFn === 'function') {
logFn.call(this.logger, message);
}
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log handler permits calling arbitrary methods on logger

Medium Severity

The log IPC handler accepts any string level from the renderer and looks up this.logger[level], calling it if typeof is 'function'. This allows invoking not just the four intended log methods (error, warn, info, debug), but any function-typed property on the logger—including inherited Object.prototype methods like constructor, toString, and hasOwnProperty, as well as internal methods of concrete logger classes (e.g., BasicLogger's static get). A whitelist check against the valid log levels would be more correct and match the test expectation that "invalid severity levels" are ignored.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fea3339. Configure here.

);
return undefined as T;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning undefined for detail methods causes downstream TypeError

Medium Severity

safeSendSync returns undefined as T on error. For variationDetail and jsonVariationDetail, the declared return type is LDEvaluationDetail / LDEvaluationDetailTyped, which have required properties value and reason. Callers (e.g., ElectronRendererClient.variationDetail) pass this through directly, so any consumer accessing .value or .reason will get a TypeError — replacing one crash with a less informative one downstream.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fea3339. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant