OUT-4093 | Stop reading localStorage in the assignee cache - #1434
Conversation
Accessing window.localStorage throws SecurityError when Chrome blocks third-party storage for the embedded iframe, and migrateAssignees read it unguarded. The migration shipped in OUT-2348 eleven months ago and only moved a cache; anything unmigrated by now just re-fetches over the network. Fixes TASKS-9X Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
hasStorageAccess() reports access to unpartitioned *cookies*; the cache lives in IndexedDB, so it was the wrong signal. MDN documents it returning false in browsers that don't block third-party access by default, which sent us into requestStorageAccess() — automatically denied without a user gesture, and there is none during AssigneeCacheGetter's mount effect. Net effect: the gate disabled the cache for the users it was meant to protect, and threw on Chrome 111, which predates the API. The surrounding try/catch already degrades to a network fetch when storage is genuinely denied, which is what MDN recommends instead of the gate. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThe PR removes the obsolete localStorage-to-localforage migration and the Storage Access API gate from the assignee cache, allowing storage failures to degrade safely to network fetching.
Confidence Score: 5/5The PR appears safe to merge, with assignee cache failures continuing to fall back to the existing network-fetch path. The removed migration only warmed an optimization cache, current cache callers use compatible data types, and failed localforage reads or writes remain contained without preventing network population of assignees. Important Files Changed
Reviews (1): Last reviewed commit: "OUT-4093 | Drop the Storage Access API g..." | Re-trigger Greptile |
SandipBajracharya
left a comment
There was a problem hiding this comment.
LGTM. Just verifying: if we return empty array then the app fetches from the assignees from backend right?
Fixes the Sentry
SecurityError: Failed to read the 'localStorage' property from 'Window'(OUT-4093, TASKS-9X) by deleting the code that readlocalStoragerather than guarding it. Net -25 lines inforageStorage.ts.migrateAssignees()touchedwindow.localStorage, which throws on property access when Chrome blocks third-party storage in our iframe. It shipped in OUT-2348 eleven months ago tagged "remove this after a while" and only moved a cache, so unmigrated users just re-fetch over the network.What to look at:
forageStorage.ts— thehasStorageAccess()/requestStorageAccess()gate is gone. It reports access to unpartitioned cookies, but the cache is IndexedDB; MDN documents it returningfalsewhere third-party access is actually fine, which fed into arequestStorageAccess()that is auto-denied without a user gesture (there is none inAssigneeCacheGetter's mount effect). It disabled the cache for the users it was meant to protect. The existing try/catch already degrades to a network fetch.forageStorage.test.ts— stubsglobalThis.windowbecausejest-environment-jsdomisn't installed. ThetoHaveBeenCalledWithassertions are load-bearing: without them the tests pass vacuously via thetypeof window === 'undefined'early return.Verified:
yarn tscclean;grep -rn localStorage src/empty, so no path can still throw this; 3 new tests pass. The 10 failures inwithErrorHandler/authenticatereproduce on a cleanmainand are unrelated.Not verified: the cache actually populating in a real Chrome iframe with third-party cookies blocked. That upside rests on documentation, not observation — the crash fix does not.
Supersedes #1407, which guarded the migration instead of removing it.