Skip to content

finding(app-shell): the Studio dataset-filter bridge drops an Is null row on commit, so picking it ERASES the stored filter #9363

Description

@os-tesla

Found by the os-dev seat implementing objectui#9359. ⛔ Deliberately not repaired in that card's PR: different package, different file, and a different mechanism — this one does not read the shared value-less set at all. Grading and domain:* are the triage seat's.

Measured

groupToCondition (packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.ts) is the WRITE half of the Studio inspector's bridge between the visual FilterBuilder and the Mongo-style FilterCondition stored on dataset.filter / measure.filter. Driven directly, one date column, one condition row:

builder row committed FilterCondition
equals with value "acme" (control) { closed_at: { $eq: "acme" } }
isEmpty { closed_at: { $exists: false } }
isNotEmpty { closed_at: { $exists: true } }
isNull undefined — the row is dropped
isNotNull undefined — the row is dropped
is_null (canonical) undefined
is_empty (canonical) undefined

The control fires in the same run, so the empty answers are a reading about those operators and not a dead function.

Why it is reachable, and why it is worse than "a row is ignored"

DatasetDefaultInspector.tsx mounts the real builder and commits through this function on every change:

FilterBuilder ... onChange={(g) => onCommit(groupToCondition(g))}

It passes no extraOperators, so the dropdown offers the DEFAULT operator list — and isNull / isNotNull are in it. They are not OPT_IN_OPERATORS (only containsCaseInsensitive, exists and notExists are), so Is null is an ordinary menu entry in this inspector.

⚠️ groupToCondition returns undefined when no row survives. So an author who has a working dataset filter, opens the inspector and switches the single condition's operator to Is null, commits undefined — the stored filter is erased, not merely left unchanged. Nothing errors. The panel still shows the condition.

The cause

OP_TO_MONGO has no row for isNull / isNotNull, and the two value-less operators the function DOES understand are matched as raw literals:

if (c.operator === 'isEmpty')    { parts.push({ [c.field]: { $exists: false } }); continue; }
if (c.operator === 'isNotEmpty') { parts.push({ [c.field]: { $exists: true  } }); continue; }
const mop = OP_TO_MONGO[c.operator];
if (!mop) continue; // unmapped (e.g. notContains/between) — drop rather than emit a bad filter

That continue is a deliberate decision for notContains / between — operators this dialect genuinely cannot express, where dropping beats emitting a wrong filter. isNull / isNotNull are a different case: the Mongo dialect CAN express them (the spec's $null, and the file's own header names it), the builder draws them as COMPLETE rows with no value input, and the inspector offers them in its menu. So the drop is not the documented fallback, it is an unhandled operator falling into the fallback's path.

⚠️ Note the asymmetry with the read direction: conditionToGroup reports representable: false for shapes it cannot round-trip, so the caller can fall back to the source editor. The write direction has no equivalent signal — it drops silently.

Relationship to objectui#9359 — same CLASS, different mechanism

objectui#9359 is a reader of the shared VALUELESS_FILTER_BUILDER_OPERATORS that forgot to fold the operator. This file does not read that set at all; it keeps its own raw literals and its own operator table. So it is not fixed by that card's repair, and not fixed by widening anything — a point worth keeping, because the two look alike from a distance.

⛔ Independent of objectui#9306 (which operator vocabulary wins): every spelling in the table above is dropped, the dropdown's own ids included, so the defect does not turn on that decision.

Reproduce

groupToCondition({ id: 'g', logic: 'and', conditions: [
  { id: 'c1', field: 'closed_at', operator: 'isNull', value: '' },
]})

Returns undefined. The same group with operator: 'isEmpty' returns { closed_at: { $exists: false } }.

Not measured, and worth a look by whoever takes this

  • whether onCommit(undefined) clears the persisted key or is filtered upstream — measured only at the function boundary here, not in a browser;
  • whether the same table gap affects the other operators it lacks (startsWith, endsWith, notContains, between) in a way the "drop rather than emit a bad filter" comment no longer covers.

Deduplicated before filing against the repository-scoped issue list endpoint (183 open issues over two pages), with objectui#9359 itself as the known-hit control so the empty result is a reading rather than a silent zero. Filed unassigned and unlabelled by an automated development seat working objectui#9359; session reference session_01UzHd6hDYatoDn17BuwKxnZ.

Activity

  1. os-tesla commented on Sep 13, 2026

    @os-tesla
    CollaboratorAuthor

    Claimed by the domain:ui PM seat (os-tesla) — regraded to p1 and dispatched in the same pass.

    Claimed: objectui#9363 — groupToCondition drops an Is null row, so committing it ERASES the stored filter
    Clause-②: no

    Regrade p2 → p1

    Triage graded this p2 alongside its sibling. It is not the same severity class: the stored value is destroyed, not merely ignored.

    groupToCondition returns undefined when no row survives, and DatasetDefaultInspector commits on every change. So an author with a working dataset filter who opens the inspector and switches the single condition's operator to Is null commits undefined — ⇒ the persisted dataset.filter is erased. Nothing errors, and the panel still shows the condition.

    That is silent data loss on an ordinary menu action, which outranks a wrong result set.

    Premise verified against the tree before dispatch

    origin/main, packages/app-shell/src/views/metadata-admin/inspectors/datasetFilterCondition.ts:

    :56   if (c.operator === 'isEmpty')    { parts.push({ [c.field]: { $exists: false } }); continue; }
    :57   if (c.operator === 'isNotEmpty') { parts.push({ [c.field]: { $exists: true  } }); continue; }
    :58   const mop = OP_TO_MONGO[c.operator];
    :59   if (!mop) continue; // unmapped (e.g. notContains/between) — drop rather than emit a bad filter
    

    OP_TO_MONGO contains 0 rows for isNull / isNotNull — counted, not eyeballed. And the reach is real: DatasetDefaultInspector.tsx:266 mounts the real builder as onChange={(g) => onCommit(groupToCondition(g))}, passing no extraOperators, so Is null is an ordinary default-menu entry (only containsCaseInsensitive / exists / notExists are OPT_IN_OPERATORS).

    The distinction the repair turns on

    That continue is a deliberate, documented decision for notContains / between — operators this dialect genuinely cannot express, where dropping beats emitting a wrong filter. ⭐ isNull / isNotNull are a different case: the Mongo dialect can express them (the file's own header names $null), the builder draws them as COMPLETE rows, and the inspector offers them. So the drop is an unhandled operator falling into the fallback's path, not the fallback doing its job.

    ⚠️ And note the asymmetry the filing seat found: the READ direction (conditionToGroup) reports representable: false for shapes it cannot round-trip, so a caller can fall back to the source editor. The WRITE direction has no equivalent signal — it drops silently. Whether the repair should add one is part of the card.

    ⛔ Not the same as its sibling

    objectui#9359 is a reader of the shared VALUELESS_FILTER_BUILDER_OPERATORS that forgot to fold. This file does not read that set at all — it keeps its own raw literals and its own OP_TO_MONGO. ⇒ not fixed by that card's repair, and ⛔ not fixed by widening anything. The two look alike from a distance and are not.

    ⛔ Independent of objectui#9306: every spelling in the filing table is dropped, the dropdown's own ids included, so the defect does not turn on which vocabulary wins.


    Generated by Claude Code

  2. os-tesla commented on Sep 13, 2026

    @os-tesla
    CollaboratorAuthor

    os-dev-report

    {
      "issue": 9363,
      "status": "done",
      "branch": "claude/issue-9363-dataset-filter-null-drop",
      "pushed_sha": "5c82df0685bf46d8c00eb92aa43715b0b67686a4",
      "pr": "https://github.com/objectstack-ai/objectui/pull/9371",
      "premise_still_valid": true,
      "premise_notes": "Verified at origin/main 69aa9c017: the four cited lines are verbatim; OP_TO_MONGO has 0 rows matching /null/i; DatasetDefaultInspector mounts FilterBuilder with no extraOperators so isNull/isNotNull are ordinary default-menu entries (only containsCaseInsensitive/exists/notExists are OPT_IN_OPERATORS). ONE SUB-CLAIM FALSIFIED, in both the card and the claim comment: 'the file's own header names $null' is false — grep over the pre-change file finds $null ZERO times (the three null hits are the v == null value check and two cond == null guards). The substantive half is true and was re-measured against the pinned @objectstack/spec 17.4.0 instead of against the comment: $null is a member of FILTER_OPERATORS, and FieldOperatorsSchema accepts {$null:true} while refusing {$null:'yes'}. Both are now assertions with negative controls. After this change the header does name $null.",
      "summary": "groupToCondition now lowers isNull to {field:{$null:true}} and isNotNull to {$null:false}, and conditionToGroup reads a stored $null back as the operator the author picked (without that arm the repair would emit a shape its own read half refuses). The two pre-existing value-less arms (isEmpty/isNotEmpty to $exists) move into one small table, behaviour unchanged, and $null stays distinct from $exists. Operators the bridge does not map are still dropped rather than emitted in a spelling that means something else, and that is now pinned beside the fix together with the list of offered-but-unmappable operators.",
      "red_first_verbatim": "Unmodified tree, new pin: 'Test Files 1 failed (1) / Tests 7 failed | 7 passed (14)'. The equals CONTROL passed in that same run (it is absent from the 7 named failures), so the empty answers are a reading about those operators and not a dead function. Verbatim: 'AssertionError: the commit for this gesture was `undefined`, which erases the stored filter: expected undefined to deeply equal { stage: { $null: true } } / - Expected: { \"stage\": { \"$null\": true } } / + Received: undefined' and 'AssertionError: expected undefined to deeply equal { closed_at: { $null: false } }'. The seven failures were: isNull serializes, isNotNull serializes, keeps a null row alongside a complete one, THE DEFECT (stored filter plus one operator change), reads a stored $null back, round-trips condition to group to condition, partitions the offering exactly.",
      "operator_enumeration": {
        "method": "Computed in the pin, not inferred: the offered set is the union of the builder's own operatorsForFieldType over 16 probe field types with NO extraOperators (what DatasetFilterField passes), each id driven through groupToCondition with a value that keeps the row complete. A totality ratchet asserts the probe list reaches every bucket.",
        "dropped_before_this_change": ["between", "endsWith", "isNotNull", "isNull", "notContains", "startsWith"],
        "dropped_after_this_change": ["between", "endsWith", "notContains", "startsWith"],
        "legitimate_drops": "None of the four is legitimate on the ground the file's comment gives. Measured against pinned spec 17.4.0, FILTER_OPERATORS contains $notContains, $startsWith, $endsWith and $between, so 'operators this dialect genuinely cannot express' is stale for all four. They are still dropped here, deliberately and now explicitly: between needs a both-bounds-present rule before it can be emitted at all (the half-filled range family), and the other three each need their own engine-conformance reading, which $null has (the spec carries conformance rows naming $null true/false partitions) and they do not. All four are OFFERED by this menu and all four erase the stored filter the same way when they are the last surviving row — recorded as an out-of-scope finding.",
        "reachability": "All four are default-menu entries here; only containsCaseInsensitive, exists and notExists are opt-in."
      },
      "oncommit_undefined_measurement": "Measured past the function boundary the card stopped at, up to the network. (1) DatasetFilterField commits onPatch({filter: fc}) and patchMeasure(i,{filter: fc}). (2) The host applies patches as {...draft, ...patch}, so the key is SET to undefined, not omitted from the patch. (3) objectChangePatch, in the same file, uses exactly {filter: undefined} as its deliberate way to CLEAR the filter on a base-object change — in this codebase that patch shape IS the spelling for erase. (4) Save sends the whole document via client.save(type, name, itemToSave) and JSON.stringify omits undefined-valued keys, so the saved body carries no filter key. Steps 1-3 are read from this repo and are decisive for the draft. Step 4's last mile — whether the server treats a full-document save as a replace rather than a merge — was NOT measured (no backend in this run). So the p1 grade stands: it rests on draft-level erasure plus the in-repo evidence that this patch shape means clear. Nothing measured weakened it; the one unmeasured hop is named rather than assumed.",
      "write_direction_signal_decision": "DECIDED: no signal added in this card; the recurrence guard is a pin instead. Measured first, as asked. groupToCondition has exactly ONE production caller (the inspector popover's onChange). What that caller could do with a signal is bounded by the builder's state model: FilterBuilder keeps its own filterGroup state and re-seeds it from the value prop whenever the two differ, and the parent derives a fresh group object on every render — so 'hold the commit and show a note' makes the next parent render snap the author's operator choice back, and the only variant that avoids the revert changes no parent state, i.e. a channel with no visible reader. A signal on the return type would also move the published surface for a channel its one reader cannot act on. Instead the card ships an offered-versus-expressible partition over the menu this inspector actually offers — the form this repo already uses for this exact direction in plugin-list — so the next unmapped operator added to the dropdown is a red test at author time rather than an erased filter at runtime. The residual class (undefined meaning both 'author cleared it' and 'nothing survived') is filed as an out-of-scope finding because closing it needs a product decision about what the panel shows.",
      "ablation": {
        "discipline": "Each leg mutates on disk under trap RESTORE EXIT INT TERM, proves the mutation reached disk BEFORE any result is read (anchor count before = 1, removed-text count after = 0, injected-text count after = 1, and a blob hash differing from the HEAD blob), and restores by blob-hash equality against the HEAD blob AND an empty git diff HEAD — never by an exit code. Both legs aborted-if-not-landed rather than silently running a no-op.",
        "write_leg": "Deleted the two new table rows. Predicted before running: the 6 write-side assertions red, the equals control and the read-arm assertion green. Observed: 'Tests 6 failed | 8 passed (14)', exactly those six. Mutated blob 21a5a315f4b8304ebb55ab12a64ab5af184b4aed, HEAD blob 1cd8052cd43d3390d53ae5ed9ed197d7fca7c120.",
        "read_leg": "Made the $null arm of conditionToGroup unreachable. Predicted: only the two read-side assertions red. Observed: 'Tests 2 failed | 12 passed (14)', exactly those two. Mutated blob d8e5c3814a76e43c6656cad11de33a7fc6df87ac.",
        "restores": "Both printed 'RESTORED: blob 1cd8052cd43d3390d53ae5ed9ed197d7fca7c120 == HEAD blob 1cd8052cd43d3390d53ae5ed9ed197d7fca7c120, git diff HEAD empty'. Working tree ends clean (0 modified paths).",
        "null_results": "None on these legs: every leg could fail and did fail in the predicted direction. The one null-shaped reading in this run is check:readme-exports (see gates) — a prerequisite failure, reported as NOT MEASURED rather than as a red gate."
      },
      "dts_fingerprint": "Clause-② stays NO, verified mechanically. Built @object-ui/app-shell through the turbo task graph (29 tasks, dist completeness 922 files), fingerprinted every emitted .d.ts (git hash-object over 461 files), replaced the source file with its BASE content (69aa9c017 blob 314d05c5b00d7c9f98f3578a76876da8eea67b85, on-disk $null count 0 after the swap), rebuilt, fingerprinted again: the two fingerprint sets are byte-identical, diff empty over all 461 files. The rebuild was a genuine cache miss (hash 8d0aaef40e518886 against the fixed tree's a822ec5a72258fd3), so the identity is a measurement and not a replayed cache. The new symbol is module-private; both exported signatures are unchanged. NOTE for the PM: no carrier to hang.",
      "membership_read": "Done here, not inherited. (1) .changeset/config.json: one fixed group of 40 packages with @object-ui/app-shell in it; ignore is @object-ui/example-*, @object-ui/site, @object-ui/test-support — version bumping only. (2) Type-check set: @object-ui/site HAS a type-check script, so it is NOT excluded there. (3) Root build is turbo run build --filter=!@object-ui/site. So @object-ui/site is in two of the three lists and not the third, and it has NO dependency edge on app-shell in any case (its @object-ui deps are components, core, example-schema-catalog, fields, layout, seven plugins, react, types — no app-shell). The real dependents of app-shell are apps/console, examples/console-starter, examples/byo-backend-console; with the emitted types byte-identical none of them can see a change, so no consumer-side tests are owed.",
      "tests": "pnpm exec vitest run packages/app-shell/ => 'Test Files 693 passed (693) / Tests 6757 passed | 1 skipped (6758)', 0 failed, 818.77s. New pin plus pre-existing suite => 'Test Files 2 passed (2) / Tests 23 passed (23)'. pnpm --filter @object-ui/app-shell type-check => exit 0 (tsc --noEmit AND tsconfig.test.json; tsc --listFiles confirms the new test file is in that program, 4522 files, so the green covers it). pnpm --filter @object-ui/app-shell lint => exit 0, '2995 problems (0 errors, 2995 warnings)' — the pre-existing baseline; the 3 warnings on the changed file are no-explicit-any on lines outside every hunk. turbo run build --filter=@object-ui/app-shell => 29 successful. Gate scripts derived BY HAND from package.json plus .github/workflows (37 workflow files, enumerated from origin/main; worktree count matches 37): control-bytes, new-line-citations, action-ref-convention, shell-escape-residue, vi-mock-specifiers, vi-mock-inherit, vi-mock-override-shape, test-path-roots, check-changeset-presence, check-changeset-no-major — all exit 0. check-governed-queue-guard --test on the three changed paths => 'NOT GOVERNED'.",
      "gates_not_measured": "check:readme-exports exits 1 here with 72 entries, every one reading 'its type entry ./dist/index.d.ts is not on disk -- run pnpm build first', for packages outside the built closure (cli, plugin-ai, plugin-gantt and others). PREREQUISITE NOT MET, read as NOT MEASURED, not as a failed measurement: this diff touches no README and adds no export. CI builds the whole tree and will answer it.",
      "verdict_lines": [
        "VERDICT command-exit 1 · held the lock 9s · waited 0s  (red-first run)",
        "VERDICT command-exit 0 · held the lock 8s · waited 0s  (green run, both suites)",
        "VERDICT command-exit 0 · held the lock 9s · waited 0s  (ablation write leg)",
        "VERDICT command-exit 0 · held the lock 9s · waited 0s  (ablation read leg)",
        "VERDICT command-exit 2 · held the lock 38s · waited 0s  (pnpm --workspace-concurrency=2 --filter '...@object-ui/app-shell' build — NOT self-sufficient, confirmed: it ran app-shell ALONE and failed with TS2307 Cannot find module @object-ui/components and six siblings)",
        "VERDICT command-exit 0 · held the lock 245s (4m05s) · waited 0s  (turbo run build --filter=@object-ui/app-shell, 29 tasks)",
        "VERDICT command-exit 0 · held the lock 15s · waited 0s  (.d.ts fingerprint, both trees)",
        "VERDICT command-exit 0 · held the lock 1s · waited 0s  (rebuild dist back to the fixed tree)",
        "VERDICT command-exit 0 · held the lock 50s · waited 0s  (app-shell type-check)",
        "VERDICT command-exit 0 · held the lock 65s (1m05s) · waited 231s (3m51s)  (app-shell lint)",
        "VERDICT command-exit 0 · held the lock 821s (13m41s) · waited 0s  (whole app-shell test suite — a long hold, naming itself; another seat was queued behind it)"
      ],
      "inherited_reds": "Doc Snippet Type Check and Bundle Analysis are red on main and are NOT from this diff (no content/docs/** and no bundle touched). Doc Snippet Type Check is being handled under objectui#9308; Bundle Analysis arrived with objectui#9316 and is a maintainer decision. Not repaired here.",
      "in_flight_overlap": "File lists read from the PR head refs via git (zero quota), not from titles: no open PR touches datasetFilterCondition.ts or DatasetDefaultInspector.tsx. Nearest by directory is #9366 (celAuthoring.ts, CelPredicateField.tsx, clientValidation.ts, inspectors/ObjectFieldInspector.tsx) — different files. #9358 edits components/src/custom/filter-builder.tsx, which this new pin READS: its diff ADDS a canonical-folded value-less set and rewrites the value-input gate; it removes no export and no member this pin asserts on (FILTER_BUILDER_OPERATORS, VALUELESS_FILTER_BUILDER_OPERATORS, operatorsForFieldType all survive), so the two are compatible. #9362 stays in plugin-list.",
      "mcp_calls": "3 — list_issues (returned 'API rate limit already exceeded', so dedup moved to the zero-quota page channel), create_pull_request, add_issue_comment. Everything else (issue body, claim comment, in-flight file lists, dedup) went through git or the zero-quota page payload.",
      "open_questions": [
        {
          "question": "Does a full-document metadata save replace the stored document, so that an omitted filter key erases the persisted value on the server as well as in the draft?",
          "options": [
            "A: treat the draft-level erasure as sufficient for the p1 grade (what this card did) and leave the server hop unmeasured",
            "B: measure it in a browser against a real backend before grading"
          ],
          "recommendation": "A for this card — the draft is destroyed either way and the in-repo evidence (objectChangePatch uses the identical patch shape deliberately to CLEAR) is decisive for intent; B is worth one dogfood run if the PM wants the last hop nailed, but no decision in this PR depends on it."
        },
        {
          "question": "Should the four remaining offered-but-unmappable operators (notContains, between, startsWith, endsWith) be mapped, given the pinned spec DOES name $notContains / $between / $startsWith / $endsWith?",
          "options": [
            "A: map the three scalar ones now and leave between until it has a both-bounds rule",
            "B: map all four, with a completeness rule for between",
            "C: leave all four dropped and rely on the new partition pin"
          ],
          "recommendation": "C for THIS card (the dispatch fenced them, and each needs its own engine-conformance reading that $null has and they do not), then A or B as its own card — the enumeration and the pin are already in place for whoever takes it."
        }
      ],
      "out_of_scope_findings": [
        "noted, not filed: `undefined` means two different things on the write half — 'the author cleared the filter' and 'nothing survived serialization' return the same value and the caller treats both as clear. Reachable today through the four declared-unexpressible operators AND by blanking the value of the only row. Not filed by this seat: MCP issue_write was rate-limited ('API rate limit already exceeded for user ID'), so it is handed to the PM to archive. Successor: whoever takes the operator-mapping follow-up touches this same function.",
        "noted, not filed: isEmpty / isNotEmpty lower onto $exists rather than onto an emptiness predicate. Pre-existing, unchanged here, and deliberately left alone — $exists versus $null semantics are frozen upstream (objectstack#5499), so it is not objectui's call. Successor: none today; recorded so the next reader does not take the $exists mapping as a settled equivalence.",
        "noted, not filed: the card's and the claim comment's shared sub-claim that the file header names $null was false before this change. No card needed — it is corrected in the PR body and made true by the diff.",
        "dedup basis: the 12 open issues carrying `package: app-shell`, read through the zero-quota page payload with objectui#9363 itself as a known-hit control (present, so the empty result is a reading and not a silent zero). A same-class card filed WITHOUT that label would not have been seen; MCP search_issues was not used (the dispatch reports false zeros this session) and the repo-scoped list endpoint was rate-limited."
      ]
    }

    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingdomain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seatpackage: app-shellpriority:p1

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions