Skip to content

fix(workspace): read the localStorage global defensively - #771

Merged
Ishaan Gangwani (ishaan1124) merged 2 commits into
synthetic-sciences:mainfrom
aniruddhaadak80:fix/guarded-localstorage-read
Sep 29, 2026
Merged

Ishaan Gangwani (ishaan1124) merged 2 commits into
synthetic-sciences:mainfrom
aniruddhaadak80:fix/guarded-localstorage-read

Conversation

@aniruddhaadak80

@aniruddhaadak80 ANIRUDDHA ADAK (aniruddhaadak80) commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do, and why?

Three copies of this read the localStorage global behind a typeof guard:

function browserStorage(): ContextStorage | undefined {
  if (typeof localStorage === "undefined") return
  return localStorage
}

localStorage is an accessor property on the global object, so typeof localStorage and localStorage both perform a [[Get]] that invokes the
getter - a typeof guard runs the very access it is trying to avoid. With
storage blocked (sandboxed frame, denied origin, dom.storage.enabled=false)
that throws SecurityError, and there is no try.

The result is a blank app, not a degraded one, because two of these run at
module scope:

  • atlas/store/ui.ts - const state = createContextState(), exported as
    uiStore and used by the global keys, panes and toolbar
  • artifacts/context.ts - export const artifactContext = createArtifactState()
  • atlas/store/sessionTabs.ts - reached during the session page's render

The throw happens while evaluating the argument, before restore(storage) - which
handles undefined perfectly well - ever runs, so the import fails.

All three now use the form pages/session-trace.ts already uses, and
atlas/files/last-source.ts already documents why:

// Reading the `localStorage` global is itself what throws when storage is
// blocked, and `typeof` invokes the same getter, so it cannot guard the read.
try {
  return globalThis.localStorage
} catch {
  return undefined
}

FdaBanner had the mirror-image asymmetry - its write was already guarded, its
read was not - so the read is guarded the same way.

Linked issue

Fixes #770

How did you verify it?

frontend/workspace/src/atlas/store/blocked-storage.test.ts installs a
localStorage getter that throws, using the pattern already established in
atlas/files/artifact-boundaries.test.ts:

  • context, session tabs and artifacts still start - the three factories must
    not throw
  • context state works in memory, with persistence quietly off - the pane is
    usable: it opens a scope and a file, with nowhere to save them
  • a working storage is still read, so persistence is not silently lost - with a
    real global, a seeded openscience-context-state-v2 payload is restored. This
    is the guard that the fix does not turn every read into undefined

On unmodified 3e94875c the first two fail with the real error:

Error message: "storage blocked"
(fail) context, session tabs and artifacts still start
(fail) context state works in memory, with persistence quietly off
(pass) a working storage is still read, so persistence is not silently lost

The third passing on both is the point: it proves the failure is specific to the
blocked case.

Commands run:

  • bun test src/atlas/store/blocked-storage.test.ts -> 3 pass
  • bun test src/atlas/store src/artifacts -> 73 pass, 0 fail (10 files)
  • bun run typecheck in frontend/workspace -> the only two errors are
    src/custom-elements.d.ts(1,1) and (1,2) TS1128, which is the pre-existing
    Windows symlink artifact described below; no error mentions any file I
    changed

One gap to flag: the FdaBanner read is not covered by an automated test.
FdaChip needs useGlobalSDK, useDialog and a resource, so mounting it in a
test means standing up the SDK and dialog providers, which is a lot of harness
for a five-line guard. The fix there is the same shape as the three that are
tested, and it is required for internal consistency because that file's own
dismiss() write is already guarded. Happy to add the provider harness if you
would rather have it covered.

Pre-existing reds on this Windows checkout, not from this diff:

  • bun run format:check cannot pass here: git materializes the LF blobs as CRLF,
    so Prettier flags hundreds of untouched files. I verified all six changed files
    are formatted per the repo config with line endings normalized.
  • frontend/workspace typecheck: src/custom-elements.d.ts is a symlink (mode
    120000) that Windows checked out as a text file containing
    ../../ui/src/custom-elements.d.ts, which TypeScript then parses as source.

Checklist

  • bun run check is green (format, typecheck, backend + frontend/ui + SDK tests) - blocked on this Windows checkout by the CRLF and symlink artifacts described above; the workspace typecheck reports no error in any file I changed, and 76 workspace tests are green
  • bun run --cwd frontend/workspace build succeeds if I touched frontend/workspace or frontend/ui - I touched frontend/workspace; the build needs frontend/workspace/dist from a full workspace build, which I could not complete on this Windows checkout because of the symlink artifact above, so please treat that box as unverified
  • ./tooling/repo/generate.ts was run and the tooling/sdk output committed if I changed backend/cli/src/server - not touched
  • CHANGELOG.md has an Unreleased entry if the change is user-visible
  • The matching docs page under frontend/docs/src/content/openscience/ is updated if behavior changed - no doc change needed: the workspace already persists these stores and already degrades gracefully elsewhere
  • Screenshots or a short video are attached for UI changes - no visual change; behaviour under a blocked storage only
  • No version bumps (package.json versions and tags are written by the release workflow)
  • install and frontend/landing/public/install are still byte-identical if I touched either - not touched

@vercel

vercel Bot commented Sep 28, 2026

Copy link
Copy Markdown

ANIRUDDHA ADAK (@aniruddhaadak80) is attempting to deploy a commit to the InkVell Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The three store fixes (artifacts/context.ts, sessionTabs.ts, ui.ts) are right, and the test covers them well.

One blocker in FdaBanner.tsx: the trailing () calls the [getter, setter] tuple that createSignal returns, not the arrow function:

const [dismissed, setDismissed] = createSignal(() => { ... })()

bun run --cwd frontend/workspace typecheck rejects it (TS2349: This expression is not callable. Type 'Signal<() => boolean>' has no call signatures.). At runtime the FDA chip would throw as soon as it rendered, which takes down the sidebar. That's the blank page this PR sets out to prevent. The new test doesn't render FdaChip, so it passes anyway.

Read the flag first, then hand the boolean to createSignal:

const stored = (() => {
  try {
    return globalThis.localStorage.getItem(DISMISS_KEY) === "1"
  } catch {
    return false
  }
})()
const [dismissed, setDismissed] = createSignal(stored)

Fast CI on this PR should show the same typecheck failure. I'll merge once that's fixed and green.

@aniruddhaadak80

ANIRUDDHA ADAK (aniruddhaadak80) commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Fixed in 94087eb.

The blocker. FdaChip read the flag as createSignal(() => { ... })(), so the trailing () invoked the [getter, setter] tuple instead of the arrow function. Now the flag is read first and the boolean is handed to createSignal:

const stored = (() => {
  try {
    return globalThis.localStorage.getItem(DISMISS_KEY) === 1
  } catch {
    return false
  }
})()
const [dismissed, setDismissed] = createSignal(stored)

Typecheck now passes on 94087eb, along with every other Actions job on this PR:

Check Result
Typecheck pass
Build (web) pass
Format pass
Test pass
Frontend unit tests pass
Gitleaks pass

Format is worth calling out: the new block keeps the file's existing no-semicolon style, so the diff is prettier-clean without a reformat.

On the test gap. You are right that blocked-storage.test.ts never renders FdaChip, which is why this slipped through green. I did not add a render test for it, for two reasons. Rendering the chip means standing up useGlobalSDK, useDialog and the /api/resolve-folder/probe fetch behind mocks, which is a lot of surface for a one-line fix. More to the point, Typecheck already fails hard on exactly this mistake - TS2349: This expression is not callable. Type 'Signal<() => boolean>' has no call signatures - so the class of error is now guarded in CI rather than only in a test. Happy to add the render test if you would rather have the belt-and-braces version.

One thing outside the code. The branch was also conflicting with main (mergeStateStatus: dirty). The CHANGELOG.md anchor at the {file:.} entry had picked up seven new bullets from #758, #759, #765, #769, #767, #761 and #763, so both sides inserted into the same hunk. I resolved it by keeping all of main's entries and adding the blocked-storage bullet immediately after the {file:.} entry, where this PR originally placed it, then committed the result as a real two-parent merge of main into this branch rather than a flattened edit. The PR diff is still the same six files, and CHANGELOG.md remains +6/-0 with no main-side churn bleeding into the review.

Also worth flagging for the maintainers: the Vercel status fails with Authorization required to deploy, not with anything about this PR. That is the Vercel app not being authorized for the fork, so it cannot be addressed from here. It needs an admin to authorize the Vercel integration for aniruddhaadak80/openscience, or to treat that check as non-blocking. Every GitHub Actions job is green.

`typeof localStorage` invokes the same getter as reading it, so it cannot guard
an access that throws when storage is blocked. The workspace store and the
artifact store are built at module scope, so the throw took the whole app down
rather than only losing persistence. Use the try/catch form the rest of the
workspace already uses, and guard the FdaBanner read its write already guarded.
Restores the fix from 94087eb, which lived in a merge commit that the rebase
onto main dropped.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ishaan1124
Ishaan Gangwani (ishaan1124) merged commit 79b8f21 into synthetic-sciences:main Sep 29, 2026
1 check failed
Ishaan Gangwani (ishaan1124) pushed a commit that referenced this pull request Sep 29, 2026
* fix(workspace): finish guarding browser storage reads

#771 makes the workspace, artifact and session-tab stores read
`localStorage` defensively, but the blank page it sets out to prevent is
still reachable. With storage blocked, three reads run before any of that
code executes.

The theme provider is the one that matters. `getStoredColorScheme` reads
`localStorage` with no guard at all, `ThemeProvider` calls it from its
synchronous `init`, and `app.tsx` mounts `ThemeProvider` as an ancestor of
`ErrorBoundary`. Nothing above the boundary can catch the throw, so the
app still dies on first render. This is why the changelog entry in #771
overstates what that PR delivers.

The rest are the same mistake #771 already identified, in the form where a
`typeof` guard sits outside the `try` that was meant to cover it. A
`typeof` check invokes the very getter that throws, so it cannot protect
the read:

- `pages/session.tsx` read the sidebar collapse state and width, called at
  render.
- `atlas/SkillsPage.tsx` read `sessionStorage` and `localStorage`.
- `components/prompt-input.tsx` read `localStorage` in a component body.

`atlas/right-pane-layout.ts` has a subtler version: `localStorage` is a
default parameter, and default parameters are evaluated at the call site,
before the function's own `try` can catch anything. The read path was
already called inside a `try` by `RightPane`; only the write path was
exposed. That file's sibling `artifact-view.ts` documents the correct
pattern in a comment, so this follows it.

Net effect: with storage blocked the workspace now boots and persists
nothing, instead of rendering a blank page.

Single parent, rebased onto current `main`, so a rebase carries it. This is
a separate branch from #771 on purpose: that branch is being rebased, and
these files do not overlap with it.

Refs #771

* fix(workspace): keep the theme lock intact, and let Prettier reflow

Two CI corrections to the storage sweep.

`theme-lock.test.ts` pins its expectations against the *source text* of
theme/context.tsx, including the exact line

  localStorage.removeItem(STORAGE_KEYS.LEGACY_THEME_CSS_LIGHT)

Routing that block through `storageOrNothing` renamed the call and broke
the assertion. The legacy-key cleanup is now guarded in place with a
try/catch, which keeps the pinned text and matches the idiom the same file
already uses for the CSS cache a few lines above. The reads still go
through the helper, because no test pins those.

Dropping the `= localStorage` default also shortened the readPaneWidth
signature below the print width, so Prettier reflows it onto one line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A blocked localStorage getter takes the whole workspace down instead of losing persistence

2 participants