Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| branches: [main, 'feat/**'] | ||
| paths-ignore: | ||
| - '**.md' | ||
| # TODO: re-enable this when we set up the example tests. |
There was a problem hiding this comment.
By this I mean when I put in the contract tests since I imagine both tests will use some headless emulator
| import { createClient } from '@launchdarkly/electron-client-sdk'; | ||
| import type { LDContextStrict, LDOptions } from '@launchdarkly/electron-client-sdk'; | ||
|
|
||
| app.disableHardwareAcceleration(); |
There was a problem hiding this comment.
Is this to help with running headless? We probably want a comment that explains why we are doing this, and not to copy it.
There was a problem hiding this comment.
ah yes - this was my attempts to try to enable the example app testing
|
|
||
| const flagKey = process.env.LAUNCHDARKLY_FLAG_KEY || 'sample-feature'; | ||
|
|
||
| const launchDarklyUser: LDContextStrict = { |
There was a problem hiding this comment.
Can this be LDContext instead?
| launchDarklyOptions, | ||
| ); | ||
| await launchDarklyMainProcessClient.start(); | ||
|
|
There was a problem hiding this comment.
Should we check and log the start result?
| // Quit when all windows are closed, except on macOS. There, it's common | ||
| // for applications and their menu bar to stay active until the user quits | ||
| // explicitly with Cmd + Q. | ||
| app.on('window-all-closed', () => { |
There was a problem hiding this comment.
Ah, the feature that causes mac users to leave all apps they even use open.
There was a problem hiding this comment.
lol, this is actually part of the electron-forge scaffolding. My thought processes is that people like things that look familiar :) but could remove if you have strong feelings about this.
There was a problem hiding this comment.
It doesn't hurt anything because we aren't shipping a real app. (In a previous real app I did work on it also made this scaffolding, and I think we eventually made the app close when it didn't have remaining windows open.
joker23
force-pushed
the
skz/electron-example-app
branch
from
5c173f3 to
f9c7408
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| // LAUNCHDARKLY_MOBILE_KEY from the environment is inlined into the renderer bundle at build time. | ||
| const launchDarklyClientId = process.env.LAUNCHDARKLY_MOBILE_KEY ?? 'example-client-id'; | ||
|
|
||
| const launchDarklyFlagKey = process.env.LAUNCHDARKLY_FLAG_KEY ?? 'sample-feature'; |
There was a problem hiding this comment.
Inconsistent nullish operators for environment variable defaults
Low Severity
main.ts reads env vars with || while vite.renderer.config.ts uses ??. Since || treats empty string as falsy but ?? does not, an empty LAUNCHDARKLY_FLAG_KEY env var would result in 'sample-feature' in the main process but '' in the renderer — causing the two processes to evaluate different flag keys. Using the same operator in both places would keep them in sync.
Additional Locations (1)
| launchDarklyMainProcessClient.on('error', (error: Error) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error('error event received', error); | ||
| }); |
There was a problem hiding this comment.
Error event handler receives context instead of error
Medium Severity
The SDK emits error events with (context, error) — context first, then the actual error. But this handler declares only (error: Error), so the parameter named error actually receives the LDContext object. The real Error (second argument) is silently discarded. The console.error call will log the context object instead of the error, making debugging misleading. Since this is an example app that users will copy, the incorrect handler signature will propagate.
2 participants
Note
Medium Risk
Adds a new Electron Forge app (with Electron/Vite dependencies) and wires it into the monorepo, which may impact install/build times and workspace tooling. CI changes are currently comment-only but introduce scaffolding for future scheduled/example runs.
Overview
Adds a new
packages/sdk/electron/exampleworkspace (@internal/electron-example) scaffolded with Electron Forge + Vite/TypeScript to demonstrate the Electron SDK, including main/renderer/preload wiring via@launchdarkly/electron-client-sdkbridge and build-time injection ofLAUNCHDARKLY_MOBILE_KEY/LAUNCHDARKLY_FLAG_KEY.Registers the example in the root
package.jsonworkspaces and updates the Electron GitHub Actions workflow with commented-out scheduling and placeholder (disabled) example test job setup.Written by Cursor Bugbot for commit d96f2d3. This will update automatically on new commits. Configure here.