DEV Community: Jiwon Yoon

Clear the Lineup — doesNotEqual was always true for single-select survey answers in Formbricks

Jiwon Yoon — Sat, 18 Jul 2026 12:35:33 +0000

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

Project Overview

Formbricks is an open-source survey and experience-management platform. Its packages/surveys package ships the client-side survey runner, and inside it, packages/surveys/src/lib/logic.ts is the module that decides, for every question and every "skip logic" / branching rule a survey author configures, whether a given condition (equals, doesNotEqual, contains, isEmpty, and friends) is true for a respondent's answer. This is pure, unglamorous logic — but it's load-bearing: it's what routes respondents through the correct sequence of questions.

I picked up issue #8527 for the "Clear the Lineup" track, which reports that doesNotEqual conditions weren't behaving correctly for single-choice questions.

Bug Fix or Performance Improvement

The doesNotEqual operator is supposed to be the exact logical negation of equals. For most answer shapes it was. But for MultipleChoiceSingle answers, the survey runtime sometimes represents the selected value as a single-element array rather than a bare string (this happens via getLeftOperandValue, e.g. when a choice answer is merged with an "other" option path). To handle that shape, both equals and doesNotEqual had a special-case clause that unwraps a one-element array and compares its only entry to the right-hand value.

equals's fallback:

return (
  (Array.isArray(leftValue) &&
    leftValue.length === 1 &&
    typeof rightValue === "string" &&
    leftValue.includes(rightValue)) ||
  leftValue === rightValue
);

doesNotEqual's fallback (before the fix):

return (
  (Array.isArray(leftValue) &&
    leftValue.length === 1 &&
    typeof rightValue === "string" &&
    !leftValue.includes(rightValue)) ||
  leftValue !== rightValue
);

At a glance this looks like a faithful "negate every sub-expression" mirror of equals. It isn't. The array-includes clause was correctly inverted (!leftValue.includes(rightValue)), but the second disjunct, leftValue !== rightValue, is not the negation of the whole equals expression — it's the negation of only the plain-comparison branch.

Here's the JavaScript gotcha that makes this a real bug and not just a stylistic quibble: when leftValue is an array like ["Option 1"] and rightValue is the string "Option 1" (or a choice id like "opt1"), leftValue !== rightValue is always true — an array and a string are never ===, regardless of contents, so the !== comparison can never be false for this input shape. Because the whole fallback is an OR of two clauses, one clause that's unconditionally true makes the entire expression unconditionally true. The array-includes clause's correct, careful negation gets swallowed by the second clause and never has a chance to matter. In other words: doesNotEqual was a tautology whenever the left value was a single-element array — it returned true no matter what the respondent actually selected.

The practical impact: any survey using skip logic or branching with "does not equal" against a single-choice question would silently misroute every respondent, since the condition was always satisfied. This is the kind of bug that's invisible in casual manual testing — you only notice it if you specifically test the branch that's supposed to be excluded, which is exactly the path people tend not to click through.

Code

PR: https://github.com/jiwonyoon-dev/formbricks/pull/1 (merged)

A transparency note on where this PR lives: the upstream formbricks/formbricks repository currently restricts opening pull requests to existing collaborators, so I couldn't submit this fix upstream directly. The fix is a merged PR on my fork instead — the challenge rules explicitly accept fork merges — and the branch is ready to upstream the moment the maintainers open contributions or pick it up from issue #8527.

The fix replaces the piecewise-negated fallback with a literal De Morgan negation of the entire equals expression, guaranteeing equals and doesNotEqual can never agree on the same inputs:

-        return (
+        return !(
           (Array.isArray(leftValue) &&
             leftValue.length === 1 &&
             typeof rightValue === "string" &&
-            !leftValue.includes(rightValue)) ||
-          leftValue !== rightValue
+            leftValue.includes(rightValue)) ||
+          leftValue === rightValue
         );

And the regression test added to packages/surveys/src/lib/logic.test.ts, in the "Edge Cases" describe block right after the existing "evaluates multiple choice conditions for equals/doesNotEqual" test, reusing the same mockSurvey / mockVariablesData fixtures already in scope:

test("evaluates doesNotEqual for single-selection answers stored as an array (#8527)", () => {
  // Regression test: doesNotEqual must be the exact negation of equals. Previously, when the
  // left value was a single-element array (e.g. a MultipleChoiceSingle answer merged with
  // "other" options) matching a static right value, doesNotEqual incorrectly returned true.
  ...
  // The single selected answer matches the compared option, so doesNotEqual must be false.
  expect(
    evaluateLogic(
      singleSelectSurvey,
      { singleSelectQ: ["Option 1"] },
      mockVariablesData,
      doesNotEqualMatchingCondition,
      "default"
    )
  ).toBe(false);

  // The single selected answer does not match the compared option, so doesNotEqual must be true.
  expect(
    evaluateLogic(
      singleSelectSurvey,
      { singleSelectQ: ["Option 1"] },
      mockVariablesData,
      doesNotEqualNonMatchingCondition,
      "default"
    )
  ).toBe(true);

  // Plain string comparisons must remain unaffected by the fix.
  expect(
    evaluateLogic(mockSurvey, mockData, mockVariablesData, doesNotEqualStringCondition, "default")
  ).toBe(false);
  expect(
    evaluateLogic(mockSurvey, mockData, mockVariablesData, doesNotEqualStringMismatchCondition, "default")
  ).toBe(true);
});

The commit follows the conventional format already used across the repo's last 15 commits on main: fix(surveys): make doesNotEqual the exact negation of equals for single-select answers (#8527).

My Improvements

I'll be upfront: I worked through this with an AI coding agent (Claude) doing the hands-on triage, reproduction, and patch drafting, while I reviewed and directed each step. That's the honest shape of how this fix came together, and I think it's a fair way to work a bug like this — the agent is good at mechanically tracing through boolean algebra and running the test suite over and over; I was there to sanity-check the reasoning and make the call on scope.

Before the fix, we added the regression test above to the unmodified logic.ts and ran it:

npx vitest run src/lib/logic.test.ts

AssertionError: expected true to be false // Object.is equality
- Expected: false
+ Received: true
  at src/lib/logic.test.ts:1436:9

That's the exact tautology from the triage, caught red-handed: doesNotEqual returned true for a matching single-select answer.

After the fix, the same focused run passed 42/42, including all three assertions in the new regression test (single-select array matching → false, single-select array not matching → true, plain-string match/mismatch → unaffected). Running the full package suite, npx vitest run in packages/surveys, gave 26 test files / 651 tests, all passing — no regressions elsewhere. npx eslint src/lib/logic.ts src/lib/logic.test.ts --ext .ts produced zero output (clean).

I also verified the change algebraically, not just empirically: when the array-single-element-and-string condition is false, both the old and new fallback reduce to the same thing (leftValue !== rightValue vs. the negation of leftValue === rightValue — identical). The only input shape whose behavior changes is the tautology case itself, which is exactly the invariant we wanted: equals and doesNotEqual can never both be true, or both be false, for the same inputs.

Scope decisions and honest limitations. I deliberately left the PictureSelection/Date special-case branches inside doesNotEqual untouched, even though they don't have a symmetric branch on the equals side — that asymmetry predates this issue and fixing it isn't part of a minimal, reviewable patch for #8527. I also ran tsc --noEmit for awareness and saw only pre-existing, unrelated module-resolution errors from @formbricks/survey-ui and @formbricks/i18n-utils — artifacts of only packages/surveys being pnpm-installed with --filter rather than a full monorepo install. Neither touched file appears in that error list, but I want to note the gap honestly rather than pretend the build was 100% clean end-to-end; I didn't attempt a full monorepo install to clear it since it's environmental noise, not something the code change caused. No e2e/integration tests exist for this pure-function logic module in this package — unit tests in the 1500+-line logic.test.ts are the established convention here, so that's what I extended.

Lesson worth keeping. The generalizable gotcha is: someArray !== someString is always true in JavaScript, because they're different types and can never be triple-equal. Any fallback shaped like (specialCase) || leftValue !== rightValue is a landmine the moment leftValue can be an array — the "obviously true when types differ" clause silently overrides whatever careful special-casing you did in the first disjunct. When you're negating a multi-clause boolean expression, negate the whole expression (!(...)), don't negate each clause independently and OR them back together — those are not equivalent once more than one clause is in play. That's the one-line rule that would have prevented this bug from ever shipping.

Clear the Lineup — Astro's `popover` Attribute Had No Way to Say False

Jiwon Yoon — Sat, 18 Jul 2026 12:26:10 +0000

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

Project Overview

Astro is the content-first web framework much of the JS ecosystem reaches for — docs sites, blogs, increasingly full apps — built around server-rendering .astro templates to plain HTML with an island-based hydration model. Buried in that renderer is a small, unglamorous function called addAttribute(), whose entire job is deciding what string, if any, gets printed for a given prop value and attribute name. Nobody thinks about it — until it starts emitting invalid HTML on every page using one specific attribute on a custom element.

That's issue #17398: "For Custom HTML Tags Astro Compile Rewrites popover Attribute to Invalid Value."

Bug Fix or Performance Improvement

The problem: here's a piece of web-platform trivia I didn't know until this bug bit me: the Popover API has no valid "false". popover isn't a plain boolean HTML attribute like disabled or checked — it's an enumerated attribute with exactly three legal states: absent, "auto", or "manual". There is no string spelling of "off." Write popover="false" into the DOM and the browser sees an attribute present with an invalid value, which per spec falls back to a valid popover state. So popover="false" doesn't disable a popover — it enables one.

That's exactly what Astro was doing on custom elements. Write <el-popover popover class="..."> in a .astro file, and the compiled output was <el-popover popover="true" class="...">. The issue's reporter had a working StackBlitz repro on Astro 7.0.9 with Tailwind Plus Elements, showing this silently breaking light-dismiss (click-outside-to-close) behavior — no console warning, no type error, nothing pointing at the actual cause. It's the kind of bug you only find by staring at rendered HTML and knowing the spec well enough to notice something's off.

The hunt

I'll be upfront about how this went: I worked through it with Claude, using Claude Code as an agentic pair rather than reading 500+ files solo. The initial pass had already narrowed things to handleBooleanAttribute() in packages/astro/src/runtime/server/render/util.ts and named two suspect PRs. A named suspect isn't a confirmed root cause, though, so the real work was reading the current source myself and checking the story against it line by line.

It held up, and it's a genuinely nice case of two individually-correct PRs colliding:

PR #13037 taught addAttribute() that popover is special: it's boolean-shaped (true/false in JS) but also accepts the strings "auto"/"manual", so it can't just live in the simple htmlBooleanAttributes regex like hidden does. It routes popover through the shared handleBooleanAttribute() helper instead.
PR #13909, later and unrelated, taught that same shared helper a different rule: for custom elements (any tag name containing a hyphen), always stringify boolean attributes into key="true"/key="false", fixing real bugs where Lit components and things like autoplay need an actual attribute value rather than presence/absence, matching how custom-element property reflection works.

Neither PR was wrong on its own. They just both modified the same chokepoint function for opposite reasons, and popover is the one attribute that's boolean-shaped in JS but has no valid "off" string in HTML — so it fell straight through the crack. The function was checking "is this a custom element?" before it ever asked "is this specifically popover?" — so popover inherited stringification semantics meant for a completely different problem (Lit prop reflection).

Code

PR: https://github.com/withastro/astro/pull/17422

The fix, in packages/astro/src/runtime/server/render/util.ts:

 function handleBooleanAttribute(
    key: string,
    value: boolean | string,
    shouldEscape: boolean,
    tagName?: string,
 ): string {
+   // The Popover API only accepts "auto", "manual", or the attribute being absent.
+   // There's no valid string value for "off", so it must always be rendered as a
+   // boolean attribute, even on custom elements.
+   if (key === 'popover') {
+       return markHTMLString(value ? ` ${key}` : '');
+   }
    // For custom elements, always render as string attributes
    if (tagName && isCustomElement(tagName)) {
        return markHTMLString(` ${key}="${toAttributeString(value, shouldEscape)}"`);

Six lines. A guard that checks popover first and always returns it as a bare-or-omitted boolean attribute, regardless of tag name, before the custom-element stringify branch ever runs. download and hidden — the other two attributes that flow through this same helper — are deliberately left untouched, since their custom-element stringification is correct and unrelated to this bug.

I added a matching regression test to the existing suite, packages/astro/test/units/app/astro-attrs.test.ts:

     <custom-el id="popover-custom-el-true"${addAttribute(true, 'popover', true, 'custom-el')} />
     <custom-el id="popover-custom-el-false"${addAttribute(false, 'popover', true, 'custom-el')} />
     <custom-el id="popover-custom-el-auto"${addAttribute('auto', 'popover', true, 'custom-el')} />

with expectations:

            'popover-custom-el-true': { attribute: 'popover', value: '' },
            'popover-custom-el-false': { attribute: 'popover', value: undefined },
            'popover-custom-el-auto': { attribute: 'popover', value: 'auto' },

plus a changeset documenting the patch for the release notes. Three files, 18 lines, all additions.

My Improvements

Confirming the bug before touching code. Before writing a fix, I reproduced the failure using the test file's own existing convention — calling addAttribute() directly rather than compiling a full .astro file, since every other popover case in that file already does it that way. Adding the custom-element case and running the suite gave a clean, unambiguous failure:

AssertionError [ERR_ASSERTION]: Expected popover to be  for #popover-custom-el-true
'true' !== ''
actual: 'true', expected: '', operator: 'strictEqual'

That's the bug, byte for byte: the renderer emitting popover="true" where it should emit bare popover.

Writing a guard, not a special case pile-up. The tempting quick fix is an if (tagName?.includes('-')) check tucked into addAttribute() itself. I didn't want popover logic living in two places, so the guard went directly inside handleBooleanAttribute(), ahead of the custom-element branch. Every caller of that function now gets the correct behavior for free, and the diff reads as "popover is exempt from custom-element stringification" rather than a scattered patch.

After the fix, the same test command passed all three new cases — popover-custom-el-true → bare popover, popover-custom-el-false → attribute omitted, popover-custom-el-auto → popover="auto" preserved — and every plain-element popover case kept passing unchanged, since the new branch is byte-identical output to the old fallback on that path. I widened the net past that one file: the class-list/style, directives, html-primitives, slots, hydration, and URL-attribute-XSS suites (124/124), the custom-element escaping suite (18/18), and the JSX custom-elements rendering suite (5/5). 150 passing tests total, directly touching the changed function and its neighbors, zero failures.

I'll be equally honest about what I didn't run: the full 235-file unit suite. Astro is a 517-package monorepo, and a full pnpm install was blocked by an unrelated npm registry trust-downgrade flag on a Netlify integration's dependency — nothing to do with this bug. Scoping the install with pnpm install --filter ...astro got me a working build, but left packages like @astrojs/markdown-remark unbuilt, and since the test runner concatenates all matched files into one process, a single missing import aborts the whole run before anything executes. So I ran the focused, targeted slice instead of claiming a full green suite I didn't actually get.

The toolchain had its own side quest: the repo's pnpm-lock.yaml is a multi-document YAML file — a self-pin block for pnpm@11.10.0 concatenated ahead of the real lockfile — that only pnpm 11.x parses correctly. My globally-installed pnpm 10.x quietly treated it as malformed and fell back to full re-resolution from the registry, which is what surfaced the unrelated trust-downgrade error in the first place. A "broken lockfile" warning was masking a different supply-chain gate one layer down. I also learned that setting NODE_OPTIONS mid-process to add --experimental-strip-types does nothing: Node reads that variable once at boot, and Astro's own test runner needs it passed to the initial node invocation directly, not toggled at runtime.

The broader lesson I'm carrying past this specific bug: not every HTML attribute that looks boolean is boolean. disabled, hidden, and checked are true booleans — present or absent, nothing in between. popover looks like one in JS (true/false), but it's actually an enumerated attribute with a closed set of legal string values and no spelling for "off." Any framework's attribute-serialization layer that treats "this prop is boolean-typed" as equivalent to "render it as a bare HTML boolean attribute" is exactly where enumerated attributes like popover — or tristate ones like aria-checked — will quietly fall through the crack. Two correct PRs, one shared chokepoint function, and the one attribute that doesn't fit either PR's mental model cleanly: that's the whole bug.

Clear the Lineup — Chasing a Stack Overflow in Typst's `#eval` Down to One Wrong Word

Jiwon Yoon — Sat, 18 Jul 2026 12:23:29 +0000

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

Project Overview

Typst is the Rust-based markup typesetting system that's been quietly eating LaTeX's lunch for the last couple of years — you write plain-text markup, Typst compiles it to PDF (or HTML) with a real incremental compiler underneath instead of a multi-pass macro soup. Part of what makes it pleasant is that it exposes its own evaluator to itself: a document can call #eval("some typst code") and get back whatever that code produces, at runtime, inside the document being compiled. It's a small feature, but it opens up a surprisingly deep rabbit hole, which is exactly where this bug was hiding.

Bug Fix or Performance Improvement

The problem: typst/typst#8632 reports that Typst crashes instead of erroring when #eval is used to recursively re-import the file it's running in. The repro is one line. Save this as overflow.typ:

#eval("import \"overflow.typ\"")

Then:

$ ./target/debug/typst compile overflow.typ overflow.pdf
thread 'main' (750833) has overflowed its stack
fatal runtime error: stack overflow, aborting

No PDF, no diagnostic, no exit code you can act on — just a native stack overflow and a SIGABRT. Typst has had cyclic-import detection since forever; a normal top-level import "overflow.typ" inside itself gets caught cleanly with an error: cyclic import message. Something about routing the import through #eval specifically was bypassing that check.

The hunt

I'll be upfront about how this investigation actually went: I worked through it with Claude (Anthropic's coding agent) rather than solo. The triage pass it did before I sat down with the code had already narrowed the search to one function — eval_string in crates/typst-eval/src/lib.rs — and named the suspect line. That's a big head start, but a named suspect isn't a confirmed root cause, so the actual work was reading that function next to its sibling, the file-level eval() a few lines above it, and checking the claim against the source myself rather than taking the plan on faith.

And it held up. Side by side:

File-level eval() builds its engine with route: Route::extend(route).with_id(id), and import_file bails with a cyclic-import error whenever route.contains(id) is true for the file it's about to open.
eval_string — the routine that actually backs the #eval() builtin — builds a deliberately "detached" engine (fresh Scope, EmptyIntrospector, Context::none()), and for its route it just did route: Route::default().

That's the entire bug, in one word: default() instead of extend().

Root cause deep-dive

Route is a linked structure — each frame in a call chain points back at its caller, and Typst uses it for two independent guards: route.contains(id) for cyclic-import detection, and Route::check_call_depth (an ~80-deep recursion cap — the same guard that trips on deeply nested show rules or recursive calls elsewhere in Typst). Both guards work by walking that outer chain.

Route::default() has no outer chain and no id. So from the guards' point of view, every single #eval() call looks like the very first evaluation ever performed — depth zero, no ancestors — no matter how many stack frames of real recursion are sitting underneath it. Trace what actually happens with overflow.typ:

Evaluating overflow.typ builds route R0, tagged with id overflow.typ.
The #eval("import \"overflow.typ\"") call invokes eval_string, which throws away R0 and builds R_fresh = Route::default().
Inside that fresh, historyless engine, it evaluates import "overflow.typ". import_file checks R_fresh.contains(overflow.typ) — false, because R_fresh remembers nothing.
That import re-enters eval() → eval_string() → another fresh, disconnected route → another import → ... forever.

Nothing ever trips, because nothing is ever checked against real history. The only thing left to stop it is Rust's stack guard page, and that failure mode is a process abort, not a SourceResult error — which is exactly what made this bug both easy to trigger and unpleasant to hit.

The tell, once you're looking for it: eval_closure, the sibling routine used for calling ordinary closures, already threads route: Tracked<Route> through and does Route::extend(route). The correct pattern already existed in the same file. eval_string was just the one routine that didn't follow it.

Code

PR: https://github.com/typst/typst/pull/8661

The fix threads the caller's real Route through eval_string instead of manufacturing a disconnected one. Because Typst splits its compiler across crates and links these "routines" via function-pointer tables rather than trait objects (to dodge circular crate dependencies), adding one parameter meant touching every signature and every call site — 7 files, 18 insertions, 3 deletions total.

The core fix, in crates/typst-eval/src/lib.rs:

     introspector: Tracked<dyn Introspector + '_>,
+    route: Tracked<Route>,
     context: Tracked<Context>,
     string: &str,
     spans: SpanMode,

-        route: Route::default(),
+        route: Route::extend(route),

The trait signature in crates/typst-library/src/routines.rs gains the matching parameter, and the real call site — the #eval() builtin in crates/typst-library/src/foundations/mod.rs — now passes the engine's actual route instead of nothing:

         EmptyIntrospector.track(),
+        engine.route.track(),
         Context::none().track(),
         &text,
         SpanMode::Uniform(span),

Three other call sites had no engine/route in scope at all, because they're genuine top-level entry points: the CSL bibliography math renderer, and the typst eval / typst query CLI subcommands. Those got Route::default().track() passed explicitly, which preserves their exact prior behavior instead of silently changing it — keeping the diff minimal and honest about what actually needed to change versus what was just plumbing.

One deliberate detail: eval_string does not call .with_id(...) on its extended route, unlike file-level eval(). That's correct, not an oversight — eval_string evaluates a bare string, which has no FileId of its own to attach. Cycle detection still works fine because the calling file's id is already baked into the outer chain being threaded in.

My Improvements

Before/after, same binary, same repro:

# BEFORE (unmodified main)
$ ./target/debug/typst compile overflow.typ overflow.pdf
thread 'main' (750833) has overflowed its stack
fatal runtime error: stack overflow, aborting

# AFTER (fix applied)
$ ./target/debug/typst compile overflow.typ overflow.pdf
error: cyclic import
  ┌─ overflow.typ:1:6
  │
1 │ #eval("import \"overflow.typ\"")
  │       ^^^^^^^^^^^^^^^^^^^^^^^^^
EXIT CODE: 1

Non-cyclic #eval usage — #eval("1 + 1"), cross-file #eval("import \"other.typ\"; other.x"), typst eval "1+1", typst query — all still work unchanged.

A crash bug is annoying to test-first: a stack overflow SIGABRTs the whole test process, so you can't just run the unfixed code under cargo test and expect a failing assertion — you'd just crash the test runner. So verification happened at two layers. First, an actual CLI build of the vulnerable binary against a kill-guard to capture the real "before" crash text above. Then a snapshot test, added to tests/suite/foundations/eval.typ, mirroring the existing file-level import-cyclic test:

--- eval-string-cyclic-import eval ---
// Cyclic import of this very file through `eval`.
// Error: 7-30 cyclic import
#eval("import \"./eval.typ\"")

Test results: the new test passes on its own (1/1), and nothing nearby regressed — eval (24 passed), import (99 passed), recursion (9 passed), cite/bibliography (66 passed), and the full integration suite comes back clean at 3720 passed, 0 failed, 0 skipped. Unit tests across typst-eval, typst-library, and typst-cli add another 34 passed. cargo fmt --check and cargo clippy are clean on every touched file.

Two things I want to flag honestly. First, the AI-assisted part: the triage that named eval_string and Route::default() came out of an earlier pass with Claude, and I worked the actual fix and verification with it too rather than purely solo — I want to be upfront about that rather than presenting it as a from-scratch human discovery. What I didn't outsource was believing it; I read the sibling functions myself, traced the recursion by hand, and built both binaries to see the crash and the fix with my own eyes before trusting either.

Second, a real lesson that'll outlast this specific bug: this is a codebase where several near-identical "routine" functions exist side by side (eval_string, eval_closure, and others), and one of them was quietly missing a parameter that all its siblings had. That asymmetry is a strong, checkable signal — worth grepping for the next time a similar "only one code path forgets to do the bookkeeping" bug shows up, in this codebase or any other with a family of nearly-parallel functions. It's also a nice reminder that "detached" or "sandboxed" execution contexts (which eval_string is deliberately trying to be) need real care about which invariants they're allowed to detach from — isolating the introspector and style context was intentional and fine; silently disconnecting the recursion guard was not.