fix(analytics-core): coerce non-string apiKey in remote config localStorage key#1764
fix(analytics-core): coerce non-string apiKey in remote config localStorage key#1764Mercy811 wants to merge 1 commit into
Conversation
…torage key When customers pass a non-string apiKey to amplitude.init (undefined, null, a number, etc), the RemoteConfigLocalStorage constructor crashes with "e.substring is not a function". Surfaces as an unhandled promise rejection because init is async. ~399 events/7d on the dashboard. Coerce to empty string when apiKey isn't a string so the key prefix is stable and init can continue; the downstream fetch will fail on its own with a clearer signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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: ff735bca55
ℹ️ 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".
|
|
||
| constructor(apiKey: string, logger: ILogger) { | ||
| this.key = `AMP_remote_config_${apiKey.substring(0, 10)}`; | ||
| const apiKeyString = typeof apiKey === 'string' ? apiKey : ''; |
There was a problem hiding this comment.
For browser init calls that pass a non-string API key such as null or a number, this fallback prevents RemoteConfigLocalStorage from throwing, but the same _init path then constructs DiagnosticsClient, whose DiagnosticsStorage constructor still executes apiKey.substring(0, 10). In IndexedDB-capable browsers, the init promise can therefore still reject with the same TypeError after the remote-config subscription completes, so the customer-visible failure this change targets is not actually fixed for those inputs.
Useful? React with 👍 / 👎.
1 participant
Summary
When a caller passes a non-string
apiKeytoamplitude.init(undefined, null, a number, etc),RemoteConfigLocalStorage's constructor callsapiKey.substring(0, 10)and crashes withTypeError: e.substring is not a function. Because the constructor runs inside an async_init, the failure surfaces as an unhandled promise rejection in the customer's app, then aborts the rest of SDK init.Volume: 399 events / 7d on the SDK Diagnostics – Browser SDK dashboard.
Example logs
What the customer sees
TypeError: e.substring is not a functionin their browser console and any RUM/error-monitoring they runamplitude.init(...)rejects; subsequentamplitude.track(...)calls never get a real client and quietly do nothing (or queue forever)Fix
Coerce non-string
apiKeyto an empty string when building the localStorage key. The downstream remote-config fetch will still fail (because the apiKey is genuinely invalid), but in a structured way — logged as a 4xx withisLastFetchInvalidApiKey = true— instead of throwing a confusing TypeError that aborts the rest of init.packages/analytics-core/src/remote-config/remote-config-localstorage.ts:9— guardapiKey.substring(...)against non-string input.packages/analytics-core/test/remote-config/remote-config-localstorage.test.ts— regression coverage forundefined,null, number, and object inputs.Test plan
pnpm --filter @amplitude/analytics-core test— 797 pass, 100% coverage maintainedpnpm --filter @amplitude/analytics-core lint— clean (pre-existing warnings only)remote-config-localstorage.ts:9events🤖 Generated with Claude Code