fix: stop a storage-blocked browser blanking the app (#1137)

[MOHITKOURAV01]

Description

TenantContext already knew that localStorage can throw — it has three guarded helpers
built for exactly that (readStoredTenant, persistTenant, forgetTenant), each with a
try/catch and a comment explaining why. Two other reads in the same file skipped them
and called localStorage.getItem bare.

Reading localStorage is not merely "returns null when empty". The property access itself
throws SecurityError: The operation is insecure. when the browser is set to block site
data — Firefox's Block cookies and site data, Safari private browsing with storage
disabled, or the widget embedded in a partitioned iframe. The read at line 230 ran inside a
useEffect with no try, so the throw escaped into React's commit phase and unmounted
TenantProvider, which wraps the app. Blank page.

The repo's own #843 regression test has been failing on main because of it:

FAIL  src/tests/contexts/TenantContext.test.jsx > TenantContext — unavailable storage (Issue #843)
      > renders with the default when reading storage throws
SecurityError: The operation is insecure.
 ❯ src/context/TenantContext.jsx:230:35

---

Related Issue

Closes #1137

---

Type of Change

• Bug Fix
• New Feature
• Documentation
• UI/UX Improvement
• Refactoring
• Performance Improvement

---

Changes Made

src/context/TenantContext.jsx

• readStoredTenantId() — a guarded raw read, with readStoredTenant() rebuilt on top
  of it. Every read in the module now goes through one place.

  It deliberately does not validate against KNOWN_TENANTS. That static list is for the
  offline picker; the API returns workspaces this build has never heard of, and a stored one
  of those still has to be restorable. Validation belongs at the call sites that want it —
  readStoredTenant() applies it, fetchTenants doesn't. There's a test for the API-only
  case so this doesn't get "tidied up" later.

• Deleted the mount effect's restore block. It could never have run:

  useEffect(() => {
    const activeId = localStorage.getItem(STORAGE_KEY);
    fetchTenants().then(() => {
      if (activeId && tenants.length > 0) {   // `tenants` is the initial []

  Empty dependency array, so tenants inside the callback is the [] captured at mount.
  Resolving a promise doesn't rebind a captured variable — it schedules a state update for
  the next render. tenants.length > 0 was never true and the body was dead code.

  It was also redundant: fetchTenants already restores the saved workspace itself, from
  the data it just received rather than from stale state. So the effect contributed one
  unguarded storage read, one dead branch, and nothing else. The effect now just calls
  fetchTenants().

• currentTenant read through a ref inside fetchTenants. Taking it as a dependency
  gave fetchTenants a new identity on every workspace selection — and it's part of the
  context value, so each change invalidated the useMemo and re-rendered every consumer of
  useTenant(). The callback is stable now, which also means the mount effect can list its
  dependency honestly instead of lying with [].

• A non-array tenant response is treated as an empty list. setTenants({...}) followed
  by tenants.find(...) elsewhere is a TypeError waiting on a shape change.

src/tests/contexts/TenantContext.storage.test.jsx (new, 13 tests)

Storage-blocked mount, a getItem that only starts throwing after the initialiser (this is
the one that exercises the old line 230 specifically), workspace selection with no readable
storage, refused writes, restoring a stored workspace, falling back when the stored id is
gone, restoring an API-only workspace, not clobbering a manual selection, fetch-once-on-
mount, fetchTenants identity stability, a rejected fetch, a non-array response, and
settings updates.

---

Testing

• Tested locally
• No console errors
• Existing functionality works as expected

$ npx vitest run src/tests/contexts/
 ✓ src/tests/contexts/TenantContext.test.jsx (25 tests)
 ✓ src/tests/contexts/TenantContext.storage.test.jsx (13 tests)
 Test Files  2 passed (2)
      Tests  38 passed (38)

Against the code before this change, 6 of the new tests fail plus the existing #843 one:

 × TenantContext — unavailable storage (Issue #843) > renders with the default when reading storage throws
 × storage that refuses to be read > mounts on the default workspace instead of crashing
 × storage that refuses to be read > survives a throwing read inside the mount effect, not only the initialiser
 × storage that refuses to be read > still selects a workspace from the API when storage cannot be read
 × restoring the stored workspace > keeps `fetchTenants` stable across a workspace change
 × a tenant API that misbehaves > treats a non-array response as an empty list rather than throwing
      Tests  6 failed | 32 passed (38)

The existing TenantContext.test.jsx is unmodified — all 25 of its tests pass, including
the one that was failing.

Note on CI

Lint, Build and Playwright are red on main and on every open PR (the 3 parse errors of
#1129). Nothing here touches those files.

[vercel]

@MOHITKOURAV01 is attempting to deploy a commit to the Aditya Mahajan's projects Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions]

Thank You for Your Contribution! 🎉

Hi @MOHITKOURAV01,

Thank you for opening this Pull Request and contributing to our project. We truly appreciate your efforts.

Please make sure that:

• Your code follows the project's guidelines.
• You have linked the appropriate issue (if applicable).
• Screenshots are added for UI/UX changes.
• Your PR is ready for review.

The maintainer @Aditya8369 will review your PR shortly!

Happy Contributing! 🚀

[MOHITKOURAV01]

Overlap with #1054 — checked, and here is the resolution

Both this PR and #1054 change src/context/TenantContext.jsx. Each is mergeable into main
on its own, but whichever lands second will conflict, so I've done the merge locally and
resolved it.

#1054 independently added a guarded raw reader for the API-driven paths — the same fix as
half of this PR, under a different name (readRawStoredTenant vs readStoredTenantId).
I've renamed to #1054's name and taken its comment verbatim in
e6a5ada, so git's three-way merge now sees both branches making the
identical change there rather than two competing ones. That takes the conflict from four
hunks down to two.

The two that remain are the real differences this PR is for. Take this branch's side on
both:

1. fetchTenants — keep #1054's explanatory comment, take this branch's list /
currentTenantRef body:

      // If we have API tenants and no current tenant is set, use the first one.
      // Read through readRawStoredTenant() rather than touching localStorage
      // directly: a browser with site data blocked throws on the bare call, and
      // this one runs inside an async callback where nothing catches it (#843).
      if (list.length > 0 && !currentTenantRef.current) {
        const savedTenantId = readRawStoredTenant();

2. The mount effect — take this branch's side outright. The block on #1054's side is the
dead code this PR removes (it reads tenants from the mount closure, which is always the
initial [], so its body can never run):

  useEffect(() => {
    fetchTenants();
  }, [fetchTenants]);

I ran the merged result: 54 tests pass — 25 from the existing TenantContext.test.jsx,
13 from this PR's TenantContext.storage.test.jsx, and #1054's
tenantService.client.test.js and historicalDataService.tenantScoping.test.js. eslint
is clean on the merged file.

If you'd rather avoid the conflict entirely, merging #1054 first and pinging me works too —
I'll rebase this one onto it and push the resolution above.