Amp 149016 global is enabled locked#1562
Amp 149016 global is enabled locked#1562daniel-graham-amplitude wants to merge 14 commits intomainfrom
Conversation
… AMP-149016-is-enabled-diagnostics
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| testValue, | ||
| }); | ||
| } | ||
| console.log('isEnabled -- exiting with result:', value === testValue); |
There was a problem hiding this comment.
Debug console.log statements left in production code
High Severity
Two console.log debug statements were left in production code. cookie.ts logs 'isEnabled -- exiting with result:' on every cookie-enabled check, and config.ts logs 'TLD -- exiting with domain:' during top-level domain detection. These will produce noisy output in every end-user's browser console during SDK initialization.
Additional Locations (1)
| } | ||
|
|
||
| return ''; | ||
| }); |
There was a problem hiding this comment.
Missing diagnostics event that test expects to find
Medium Severity
The new test at line 468 expects diagnosticsClient.recordEvent to be called with 'cookies.tld.failure' when TLD detection fails, but neither getTopLevelDomain nor legacyGetTopLevelDomain ever records this event. The return '' at the end of both functions silently discards the failure without any diagnostics reporting, causing the test to fail.
Additional Locations (1)
|
|
||
| const legacyGetTopLevelDomain = async (url?: string, diagnosticsClient?: IDiagnosticsClient) => { | ||
| if ( | ||
| !(await new CookieStorage<number>(undefined, { diagnosticsClient }).isEnabled()) || |
There was a problem hiding this comment.
Redundant isEnabled check on legacy fallback path
Low Severity
legacyGetTopLevelDomain re-checks isEnabled() (line 518) even though getTopLevelDomain already verified it (line 476) before calling the legacy function. On the legacy path (no navigator.locks), each isEnabled() call creates a test cookie with a unique key, does a set/get/remove cycle — so this doubles the cookie operations during initialization unnecessarily.
1 participant
Summary
Checklist
Note
Medium Risk
Touches core cookie enablement checks and top-level-domain detection used during browser initialization; behavior now depends on
navigator.locksavailability with fallback paths and new diagnostics logging, so regressions could affect cookie-based identity storage.Overview
Prevents concurrent cookie enablement/TLD checks from racing by wrapping
CookieStorage.isEnabled()andgetTopLevelDomain()domain-probing innavigator.locks(with legacy fallback when locks aren’t available).Adds diagnostics reporting for cookie enablement failures/value mismatches and propagates an optional
diagnosticsClientintogetTopLevelDomain()from browser config initialization.Expands automated coverage with a new Playwright E2E regression test and updates unit tests/mocks (including
legacyIsEnabled) plus a strongertest-server/cookies/is-enabled.htmlstress test.Written by Cursor Bugbot for commit 3e642db. This will update automatically on new commits. Configure here.