fix(billing): Guard localStorage access in navBillingStatus and trialStarter#112970
Open
fix(billing): Guard localStorage access in navBillingStatus and trialStarter#112970
Conversation
…navBillingStatus Replace 4 raw localStorage.getItem/setItem calls with localStorageWrapper from sentry/utils/localStorage. On devices where window.localStorage is null (e.g. Android WebView without DOM Storage enabled), raw access throws TypeError. localStorageWrapper safely falls back to a noopStorage that returns null for reads and no-ops for writes. Add unit and integration tests verifying the component renders, auto-opens, and allows dismiss interaction when localStorage is unavailable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
the
Scope: Frontend
Contributor
Sentry Snapshot Testing
|
Consolidate 3 redundant unit tests into 1 focused test that covers render, auto-open, and setItem gracefully. Add afterEach with jest.restoreAllMocks() to prevent mockReturnValue leaking across describe blocks (clearMocks only resets call history, not implementations). Move createStorage utility tests to a dedicated createStorage.spec.tsx file — they test shared utility behavior, not component behavior. Remove implementation-detail test that only verified call arguments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…trialStarter Replace raw localStorage.removeItem call with localStorageWrapper.removeItem. On devices where window.localStorage is null (e.g. Android WebView without DOM Storage enabled), raw access throws TypeError. localStorageWrapper safely falls back to noopStorage. Add test verifying trial start succeeds when localStorage is unavailable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The jest.mock spread of localStorageWrapper missed prototype methods like removeItem because native Storage objects don't have own-property methods. The "Start Trial" button in billing status tests triggers trialStarter which calls localStorageWrapper.removeItem(), causing TypeError in CI. Explicitly mock all Storage methods as pass-throughs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dashed
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes JAVASCRIPT-38TV
The Bug
On devices where
window.localStorageisnull— notably Android WebViewwithout DOM Storage enabled (e.g. Sony BRAVIA smart TVs) — two gsApp
components crash with:
Root cause chain:
WebSettings.setDomStorageEnabled(true)to enable itwindow.localStorageevaluates tonull(not undefined — literally null)navBillingStatus.tsxandtrialStarter.tsxcalllocalStorage.getItem()/.setItem()/.removeItem()directly on this null valueThe crash in
navBillingStatus.tsxis caught by anErrorBoundarywithcustomComponent={null}, so the billing status icon silently disappears.User impact is minimal but the fix is trivial.
The Fix
Sentry already has a safe localStorage wrapper:
createStorage()tests actual read/write capability at module load. IflocalStorageis null or throws, it returns anoopStorageobject thatreturns
nullfor reads and no-ops for writes. Both components justweren't using it.
Changes:
navBillingStatus.tsx— replace 4 rawlocalStorage.getItem/setItemcalls withlocalStorageWrappertrialStarter.tsx— replace 1 rawlocalStorage.removeItemcall withlocalStorageWrapperTests
navBillingStatus.spec.tsx— 2 tests: component renders/auto-opens and allows dismiss when localStorage is unavailabletrialStarter.spec.tsx— 1 test: trial starts successfully when localStorage is unavailablecreateStorage.spec.tsx(new file) — 3 tests: noopStorage fallback when storage is null, when setItem throws, and happy path