Sentry pointed at the crash. The bug was one frame upstream.

This is a submission for DEV's Summer Bug Smash: Clear the Lineup powered by Sentry.

Project Overview

Formbricks is an open-source survey and experience
management platform — the self-hostable alternative to Qualtrics. It is also one of the repos
this challenge lists as already running Sentry.

The part I worked in is its JavaScript SDK: packages/js-core, which bootstraps the SDK and
keeps its config in localStorage, and packages/surveys, which talks to the API and renders
the survey widget. That code does not run on formbricks.com. It runs embedded in other
people's websites, which changes what a crash costs — nobody on the Formbricks team is
watching the page it breaks.

Bug Fix or Performance Improvement

The issue

formbricks#6581 has been open since
22 September 2025 and still is, with nobody assigned. The body cites Sentry issue
FORMBRICKS-CLOUD-3VE:

TypeError: Cannot read properties of null (reading 'data')

That error is a nuisance to act on, because it names the property that got read, not the
thing that was null, and not the code that made it null. You get a frame in whatever
component happened to touch the value — and the actual defect is somewhere upstream, in
whatever handed it over.

Formbricks' JS SDK is embedded in other people's sites. A crash there isn't a broken page
in the Formbricks app; it's a survey widget that silently dies inside a customer's product.
That's what made the issue worth taking.

Finding where the null was born

The reported frame reads .data off something, so the question isn't "what crashed" — it's
"who was allowed to hand that function a null in the first place?" Working backwards through
the callers ends at makeRequest in packages/surveys/src/lib/utils.ts.

Its signature is a promise:

const makeRequest = async <T>(...): Promise<Result<T, ApiErrorResponse>> => {

Result<T> is the codebase's discriminated union — { ok: true, data: T } or
{ ok: false, error: E }. Every caller branches on .ok and then trusts .data.

But the API can answer 200 OK with {"data": null}, and makeRequest passed that
straight through as ok(null). A success carrying nothing. The type says T; the value is
null; TypeScript never sees it because the response was cast on the way in.

Then response-queue.ts does what every caller does — and note that it does it correctly:

if (!response.ok) {
  return err(response.error);
}

this.surveyState.updateResponseId(response.data.id);   // ← the frame Sentry reports

That caller isn't sloppy. It checks .ok. It propagates the error. It earns the right to
read .data on the next line, because the type it was handed says that's safe. Then it
crashes anyway.

The crash is one frame away from the bug. That distance is why the issue is still open
nearly eleven months later. Nothing in the reported stack points at makeRequest, and
nothing in the crashing function is wrong.

Two more places had the same shape:

• ApiClient.uploadFile() destructured data from the storage API's signing response with no null check.
• Two call sites in packages/js-core called JSON.parse(localStorage.getItem(...)) unguarded.

That last one turned out to be the more interesting half.

The localStorage half: a contract violated from the inside

Config.loadFromLocalStorage() in packages/js-core:

public loadFromLocalStorage(): Result<TConfig> {
  if (typeof window !== "undefined") {
    const savedConfig = localStorage.getItem(JS_LOCAL_STORAGE_KEY);
    if (savedConfig) {
      // TODO: validate config
      // This is a hack to get around the fact that we don't have a proper
      // way to validate the config yet.
      const parsedConfig = JSON.parse(savedConfig) as TConfig;   // ← throws
      return ok(parsedConfig);
    }
  }

  return err(new Error("No or invalid config in local storage"));
}

Two things in there were already telling on the bug before I touched it.

There's a TODO saying the config isn't validated — so the gap was known. And the error
path's message already says "No or invalid config", which means someone wrote a branch
for exactly this case.

But JSON.parse throws, so a corrupt value never reaches that err four lines below. It
leaves through the exception instead, past the function's own contract. The handling was
written; the input just never arrived at it.

And the constructor calls it like the contract is real:

private constructor() {
  const savedConfig = this.loadFromLocalStorage();   // no try/catch
  if (savedConfig.ok) { this.config = savedConfig.data; }
}

There's no try/catch because there shouldn't need to be one — the function returns a
Result.

So where does the throw actually land? Not on the host page, it turns out.
CommandQueue.run() wraps every command in wrapThrowsAsync and, on failure, does this:

console.error("🧱 Formbricks - Global error: ", result.error);

Setup never completes. isSetup is never set. And every SDK command after it hits the
guard at the top of the queue loop:

if (!setupResult.ok) {
  console.warn(`🧱 Formbricks - Setup not complete.`);
  continue;
}

That's the whole failure. One corrupt localStorage entry, and the SDK spends the rest of
the page lifetime politely declining to do anything. The host site works fine. Nothing is
visibly broken. The survey just never appears, and the only trace is two console lines
nobody is reading on a customer's production site.

A crash that takes the page down gets reported in an hour. This one can run for months.

localStorage is not a place where you get to assume well-formed input. You can get a
truncated write from a tab that closed mid-setItem, a quota failure, a hand-edit in
devtools, or a payload written by a previous version of the SDK. The same unguarded parse
also sat in migrateLocalStorage(), which runs first during setup — so a corrupt entry
aborted setup before anything else could run.

Code

All three commits are on my fork's main,
rebased onto upstream ace2c9b — 289 insertions across 9 files:

commit
de95090	fix(surveys) — null guards in makeRequest / uploadFile, plus safeJsonParse
2171a15	fix(js-core) — route both localStorage parse sites through it
a730481	test(surveys) — cover the uploadFile guard

Files touched: js-core/src/lib/common/{config,setup,utils}.ts and
surveys/src/lib/{utils,api-client}.ts, plus their four test files.

The before/after Sentry reproduction is a separate repo:
bugsmash-sentry-repro.

Why there's no upstream PR

Formbricks' CONTRIBUTING.md:

we don't have the capacity to properly facilitate community contributions … only in rare
exceptions

The challenge asks entrants to read CONTRIBUTING.md and not add to maintainer burnout, and
its FAQ allows merging on a fork when upstream can't take the PR. So it is merged on the fork.

Opening a PR would have felt more like a submission. It would also have put a 289-line review
across 9 files on a team that has said in writing they can't take it.

My Improvements

The fix

A single small helper, and routing every unguarded parse through it:

export const safeJsonParse = <T>(value: unknown, fallback: T): T => {
  if (typeof value !== "string" || !value.trim()) return fallback;
  try {
    return JSON.parse(value) as T;
  } catch {
    return fallback;
  }
};

Then:

• makeRequest returns err() instead of ok(null) when the payload is empty.
• uploadFile throws a named error before the destructure.
• Both js-core parse sites fall back to the err path each function already declared.

js-core has no workspace dependencies, so it can't import the copy in
@formbricks/surveys — it got its own. Duplication I'd normally push back on, but a
package with zero deps is a deliberate property here, not an oversight.

Auditing my own fix

The fix was written, tested, green. Then I went back and read it as if someone else had
sent it to me. Two things were wrong, and neither one shows up as a failing test.

1. The helper was dead code

safeJsonParse was exported. It was unit-tested — nine tests, all passing. It was cited
in the commit message as the fix for a JSON.parse crash class.

Nothing called it.

The tests passed because they tested the helper directly. Coverage went up. The diff looked
right. And the actual crash sites were untouched, because I'd fixed the category of bug
and then not connected it to the two places that had it.

That's the failure mode I'd flag hardest in someone else's PR, and it went straight past me
in my own.

2. The guard nobody tested

uploadFile got its null check. api-client.test.ts had eight existing uploadFile tests
and they all passed.

They also all passed against the pre-fix source. I checked:

Tests  17 passed (17)

Seventeen green tests, and not one of them would have noticed the guard being deleted,
inverted, or never written. The fix was real and the coverage number said nothing about it.

Both follow-ups came from auditing the fix, not the codebase. The check that would have
caught either one takes about a minute:

Grep your own fix for call sites and test coverage before you claim it.
If the new helper appears only in its own definition and its own test file, it's dead.
If the guarded line still behaves identically with the guard removed, it's untested.

Proving the tests are real

A regression test that has never failed is a guess. So for each one I reverted the fix and
re-ran:

× loadFromStorage() returns err instead of throwing on malformed JSON
  → SyntaxError: Expected property name or '}' in JSON at position 1
× loadFromStorage() returns err instead of throwing on a truncated write
  → SyntaxError: Unterminated string in JSON at position 26
× throws a named error when the signing response carries a null data payload
  → TypeError: Cannot destructure property 'signedUrl' of 'data' as it is null.

Same error class as the Sentry issue. Restored the fix, all green:

• js-core — 304 passing, 20/20 files
• surveys — 626 passing

Two of the control tests correctly pass both ways, which is the point of having them.

A detour worth mentioning

pnpm install is unusable on this repo from WSL — on /mnt/c it hangs with zero output
for ten minutes and never finishes. I couldn't run the suite the normal way at all.

What worked: copy the package source to the Linux filesystem, write a minimal
vitest.config.ts, install with plain npm. js-core has no workspace deps so it's
clean; surveys needs types, survey-ui, and i18n-utils copied alongside it in the
same packages/ layout, because several tests import ../../../types/x relatively.

One trap cost me a while: pin the same vitest the repo pins. My harness was on vitest 3
against a repo pinning 4.1.6, and four tests in an untouched file failed with
No script element for surveys.umd.cjs was appended to document.head. They failed
identically against pre-fix source — nothing to do with my change — and vanished on 4.1.6.
I nearly reported a passing suite as "304 passing except four known failures" on the
strength of a version mismatch in my own tooling.

Best Use of Sentry

Since the issue came from Sentry, "fixed" should be demonstrable in Sentry — not just in a
diff. So I built a reproduction that runs the real package source at both commits
against a live DSN:
bugsmash-sentry-repro.

src-before/ is git archive of upstream ace2c9b. src-after/ is the fork with the
fix. No hand-edited source — the repo's NOTICE.md gives the commands to reproduce both
trees yourself, so you don't have to take my word for what's in them.

Each scenario runs in its own process and the exception is left uncaught on purpose.
The entire reason this reached Sentry is that it escaped every handler in its path — so
catching it and calling captureException by hand would produce an event with a different
shape than the reported one.

Every event is tagged variant:before / variant:after.

scenario	before	after
config-truncated	crashed — SyntaxError: Unterminated string in JSON, uncaught, out of the constructor	handled — err("No or invalid config in local storage")
config-hand-edited	crashed — SyntaxError: Expected property name or '}'	handled — err("No or invalid config in local storage")
make-request-null	crashed — ok(null) propagates → TypeError: Cannot read properties of null	handled — err("API returned an empty data payload")
upload-null-data	crashed — TypeError: Cannot destructure property 'signedUrl' of 'data' as it is null.	crashed — Error: Upload API returned an empty data payload

Totals: 5 crashed, 3 handled.

That last row is the one I'd point a Sentry engineer at. It still throws. An empty
signing response genuinely is a failure and uploadFile should still fail. The crash rate
doesn't move.

What moves is what lands in the dashboard. Before, Sentry gets an error naming a local
variable in a destructuring pattern — you have to go read the source to learn which network
call failed. After, it gets an error that says the upload API returned an empty payload.

Same event count. One of them you can triage from the issue list.

In the dashboard those five crashes arrive as three issues, not four, and the reason is
worth a moment. Sentry groups by stack trace, not by message — and both localStorage
scenarios throw from the same JSON.parse on the same line of loadFromLocalStorage. A
truncated write and a hand-edited value are different inputs with different error text, and
they land in one issue together.

Which is exactly what makes the production version hard to read. You don't see "corrupt
localStorage, four flavours." You see one issue with a rising event count and whichever
message happened to arrive last — and the fix has to cover causes the issue title never
mentions.

The variant:after side inverts that, and this is the result I did not expect going in.
Four rows, but only one carries Sentry's red Unhandled badge — the uploadFile error
that should still fail. The other three are the handled paths reporting themselves, each as
its own entry:

Error: Upload API returned an empty data payload          Unhandled
[make-request-null]  null API payload handled without an unnamed crash
[config-truncated]   corrupt localStorage handled without throwing
[config-hand-edited] corrupt localStorage handled without throwing

The two localStorage causes that were fused into a single issue before are now separate
and individually named. Grouping by stack trace collapsed them precisely because they
crashed at the same line; once neither one crashes, each is visible on its own terms.

So the fix does two things to the telemetry, not one. It renames the crash that remains, and
it un-merges the two failures that were hiding behind one row.

variant:before — three issues, every one of them Unhandled. The SyntaxError row carries
four events because both localStorage scenarios group into it.

variant:after — four rows, one Unhandled. The two localStorage causes that shared an
issue before are now separate and individually named.

The pair that makes the point. Same function, same line, both still Unhandled. One names a
variable in a destructuring pattern; the other names the failure.

One thing that bit me while building this, and is worth knowing if you write a harness like
it: you cannot read the outcome from the exit status once a DSN is set. With Sentry
disabled the process dies through Node's default handler and exits non-zero. With a DSN, the
SDK installs its own uncaughtException handler, flushes the event, and exits 0 — so a
crashed run and a clean run become indistinguishable by status, and my first pass cheerfully
labelled all eight runs "handled" while printing their stack traces directly above the label.
The harness reads a sentinel the script prints on its success path instead.

Best Use of Google AI

I ran the whole investigation past Gemini afterwards, in one conversation, in the order I'd
actually hit things. The useful result wasn't a win or a failure — it was that the model's
accuracy tracked the shape of the question far more than the difficulty of the problem.

Cold triage: a good list, in the wrong order

Given only the Sentry error and the Result<T> signature — no mention of makeRequest — it
produced four candidate causes. The right one was fourth:

"A backend API might return {"ok": true, "data": null} … the type definition is out of
sync with backend reality."

Above it sat three plausible-sounding wrong trees: a 204 empty body, ad-blockers
monkey-patching fetch on the host page, and prototype pollution. All real phenomena for an
embedded SDK. None of them this bug.

Its first recommended fix is the interesting part:

if (result.ok && result.data != null) {
  // Safe to access result.data.id
}

That patches the crash frame — every call site — rather than the helper that manufactured the
bad Result. It's the exact trap this post is about, and the model walked into it. Its
second suggestion (validate inside the helper before returning ok) was the right layer, so
it offered both without ranking them.

Fair summary: it generated a candidate list containing the answer. Picking the answer out of
the list still needed someone who knew the codebase.

The localStorage method: strong, then confidently wrong

Shown loadFromLocalStorage and its constructor, it got everything local: the uncaught
SyntaxError, the unguarded constructor, the unvalidated cast. It named the real-world
causes unprompted — "interrupted write, browser crash, or user tampering" — which are the
scenarios in my repro. It even flagged JSON.parse("null"), one of the three regression tests.

Its proposed fix is essentially the shipped one.

Then it asserted this:

"it can break the host website's JavaScript execution, preventing their own application
logic, checkout flows, or analytics from running. Host developers will quickly remove your
SDK to protect their site."

Which is false, and I'd already checked why: CommandQueue.run() catches it and logs. The
host page is fine.

It had two functions and no queue, and extrapolated a blast radius the surrounding code
contradicts. Note the direction of the error — it reached for the dramatic failure. The
real one is quieter and worse: no broken site, just a survey that never appears, for months.

The review that mattered

Then I showed it the full first commit and asked one open question: "Is the fix complete?"

It said no — correct verdict. Its reason: uploadFile throws a raw Error instead of
returning a Result, a "discrepancy in error handling strategy." It suggested returning
err({...}) instead.

Except uploadFile is declared ): Promise<string>. It never returned a Result, and it
already throws in nine other places. Throwing is its convention; the new guard is the most
consistent thing that could have gone there. The suggested fix wouldn't compile.

And it named neither actual defect.

So I asked two closed questions instead:

Is safeJsonParse actually called anywhere in the shipped production code path by this
commit? Is there a test that would fail if the if (!data) guard were deleted?

No. safeJsonParse … is not called anywhere else … zero production modules invoke it.
(It appears to have been written preemptively … but was never actually wired into
loadFromLocalStorage or other parsing logic in this PR.)

No. … deleting its if (!data) guard would allow all tests to pass successfully,
leaving that part of the code entirely unprotected and untested.

Both correct, immediately, no hedging. It even named loadFromLocalStorage — the exact call
site the follow-up commit wired up — and described the falsification check I'd run by hand.

Same diff. Same conversation. Same model. The only thing that changed was the question.

What I'd actually take from it

It was never missing information. It was answering what I asked.

"Is this complete?" is an open question, and an open question gets answered from the most
salient thing in view — in a diff, that's the code that changed. So it reviewed the patch, and
when the patch looked fine, it reached for a style objection to satisfy the question.
"Is this called?" and "is this covered?" are closed, and both were answered instantly from
identical input, because they point at what the patch left out.

The rule I'm keeping: ask a review model closed questions.

And the limit, stated honestly: the follow-up worked because I already knew what to ask.
Nothing here found the dead code for me. That still came from reading my own diff and
wondering who called the thing.

What I'd take away from this

The bug itself is small: four unguarded reads across two packages. What kept it alive for
eleven months is distance. The crash lands one frame from the defect, the SDK's own error
handler turns the fallout into two console lines nobody reads, and Sentry groups two distinct
causes into one row. Every layer between the defect and the person who could fix it removes a
little more information.

The fix closes that distance in both directions — the code fails where it's wrong, and the
report says what went wrong.

The part I'll carry forward is smaller and less flattering. Three separate systems told me
this fix was done: a green test suite, a clean diff, and a review model. All three were
answering a narrower question than the one I thought I'd asked. The suite proved the helper
worked, not that anything used it. The diff showed correct code, not reachable code. The
model reviewed the patch, because that's what I gave it.

None of them were wrong. They were just each answering "is this code good?" when the
question I needed was "is this code doing anything?" — and that one has to be asked out
loud, by name, of every piece.

Fork: naufalfx805-source/formbricks ·
Repro: bugsmash-sentry-repro ·
Issue: formbricks#6581 ·
Sentry: FORMBRICKS-CLOUD-3VE

My Smash Stories entry — a race condition in a CLI's credential store — lands on the same lesson from the other side.