DEV Community: Naufal Hafizh

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

Naufal Hafizh — Sun, 16 Aug 2026 09:50:49 +0000

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.

nice

Naufal Hafizh — Sun, 16 Aug 2026 01:48:27 +0000

Summer Bug Smash: Smash Stories 🐛🛹

Naufal Hafizh

Aug 15

I shipped a lock. Then I had to prove it could fail.

#bugsmash #testing #node #devchallenge

5 min read

I shipped a lock. Then I had to prove it could fail.

Naufal Hafizh — Sat, 15 Aug 2026 10:56:08 +0000

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

## The bug that doesn't report itself

testsprite-cli stores API keys in a credentials file, one entry per profile. Writing a
profile looked like this:

  mutateCredentialsFile(path, file => {
    file[profile] = { ...file[profile], ...entry };
    return file;
  });

Read the whole file, mutate in memory, write the whole thing back via renameSync.

The rename is atomic, and that's what makes this interesting — it's not a corruption bug.
The file is never left half-written. Someone thought about atomicity and got it right.

What the rename doesn't decide is whose snapshot wins. Two writers, both read the same
starting state, both add their own profile, second renameSync lands last. The first
profile is gone.

Not corrupted. Not errored. Gone.

  $ testsprite setup --profile staging &
  $ testsprite setup --profile prod &
  $ # both exit 0
  $ # one of them didn't happen

Nothing raises. Nothing exits non-zero. You find out later, when a command that needs
staging reports AUTH_REQUIRED, and you assume you typo'd something and set it up again.

That's the part that stuck with me. The failure mode isn't "it broke," it's "it quietly
didn't happen, and the error surfaces somewhere else, later, attached to a different
command." A user hitting this has no path back to the cause.

And it isn't exotic usage. Two shells. A CI job overlapping a local setup. An agent running
commands in parallel — which, for a CLI whose whole job is being driven by an agent, is
approximately the normal case.

## The fix, which was the easy part

An advisory lock file held across the entire read-modify-write cycle, not just the write:

  writeFileSync(lockPath, token, { flag: 'wx' });

wx is O_EXCL — it succeeds for exactly one process and throws EEXIST for everyone
else. Losers spin until a 5-second timeout.

Two details that aren't optional:

Stale lock reclaim. A process killed mid-write leaves the lock file behind and every
future write hangs. So a lock past a threshold gets reclaimed.

Re-checking ownership after the wait. Reclaim introduces its own race: while you were
waiting, someone may have decided your lock was stale and taken it. So immediately before
the write, assertHeld() re-reads the lock file and confirms the token is still yours.

That merged as #272. Tests passed.
Done.

## Except the tests couldn't have failed

Here's what I noticed afterward, and it's the actual story.

The suite had tests for the credentials module. They passed. They also would have passed
with the entire lock deleted — because every branch of that lock is cross-process by
construction, and the tests all ran in one process.

Think about what you'd need to reach:

The EEXIST path needs a second process already holding the lock.
The wait loop needs contention that lasts longer than one process's critical section.
Stale reclaim needs a lock file left by a process that died.
assertHeld() failing needs a third party to steal ownership mid-flight.

Not one of those is reachable from a single process. An in-process mutex wouldn't even have
fixed the original bug, and an in-process test can't reach the fix that did.

So the lock was real code protecting a real race, sitting behind a green suite that was
structurally incapable of noticing if it broke. Delete the lock, tests still pass, bug
comes back silently.

That's a worse position than having no test, because the green check reads as a claim.

## Making a race lose on purpose

#280 is tests only — no src/
changes, nothing ships to users.

It spawns eight real child processes — spawn(process.execPath, [CHILD_PATH]), not
worker threads, not a mocked fs — each writing a distinct profile to one shared
credentials file.

And spawning them is not enough, which is the trap.

Eight Node processes come up staggered by tens of milliseconds. The critical section is
shorter than that. Left alone they queue politely, one after another, never contend, and
the test passes without ever touching the lock — the same failure it was written to rule
out, one level up.

The fix is a starting gate:

Each child writes a <profile>.ready marker, then spin-waits for a shared start file.
The parent blocks until all eight ready markers exist.
The parent writes start, releasing all eight into the critical section together.

The children sleep between polls with Atomics.wait on a throwaway SharedArrayBuffer
rather than setTimeout — a genuine blocking sleep, with no event-loop scheduling sitting
between the release and the write.

Now they collide on purpose. All eight profiles have to be present at the end, and no lock
file may remain.

## The other thing a concurrency test has to prove

A concurrency test that quietly stops testing anything is worse than no test — which is the
same hazard as the staggered-start one, wearing a different hat. Eight children exiting
early for an unrelated reason produces a perfectly green run that asserts nothing.

So the child's contract is pinned by exit code:

| Case | Exit | Asserted |
|---|---|---|
| Missing required env vars | 1 | names all four required vars |
| Start marker never appears | 2 | Timed out waiting for start marker: <path> |
| writeProfile rejects (bad] profile name) | 3 | stderr contains Invalid request. |

Without these, one typo'd env var name makes all eight children bail before they write
anything, and the test reports success.

Every case builds its own mkdtempSync root and removes it in finally; nothing touches
the real credentials path. Timeouts are bounded — 10s per child, 20s for the concurrent
case, 15s for the timeout case — because an unbounded concurrency test in CI is its own
kind of outage.

## What I took from it

The two PRs are the same length of work and only one of them was hard. Writing the lock was
mechanical once the race was clear. Building a situation where the lock visibly matters
took a starting gate, a blocking sleep primitive, and a set of exit codes whose only job is
to stop the test from lying.

The pattern I keep running into, in this repo and elsewhere:

A fix isn't verified until you've watched the test fail without it.

For a race, that's not a figure of speech — it's the entire engineering problem, because
the default outcome of a concurrency test is that it passes for the wrong reason.

Both PRs are merged: #272 (the
lock, from issue #77) and
#280 (the cross-process test).

I also wrote up a null-propagation crash class in Formbricks
for Clear the Lineup, which ended in the same place from the other direction: a fix that
looked complete, and a green suite that wasn't checking the thing I'd claimed.

DEV Community: Naufal Hafizh

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

Project Overview

Bug Fix or Performance Improvement

The issue

Finding where the null was born

The localStorage half: a contract violated from the inside

Code

Why there's no upstream PR

My Improvements

The fix

Auditing my own fix

1. The helper was dead code

2. The guard nobody tested

Proving the tests are real

A detour worth mentioning

Best Use of Sentry

Best Use of Google AI

Cold triage: a good list, in the wrong order

The localStorage method: strong, then confidently wrong

The review that mattered

What I'd actually take from it

What I'd take away from this

nice

I shipped a lock. Then I had to prove it could fail.

I shipped a lock. Then I had to prove it could fail.

The `localStorage` half: a contract violated from the inside

The `localStorage` method: strong, then confidently wrong