finding(plugin-charts): a sankey with mixed positive and negative rows silently DROPS the negative ones and draws a partial flow as if it were complete

[os-warren]

Filed unassigned by the implementer of #7140 (PR #7146). ⛔ Out of scope there: that card's trigger is links.length === 0 — no row survives the filter — and PR #7146 answers exactly that branch. This is the branch next door, where some rows survive, and it is arguably the worse of the two.

The measurement

packages/plugin-charts/src/AdvancedChartImpl.tsx, the sankey arm, at origin/main e8e4c4d:

const rows = data.filter((r) => (Number(r?.[dataKey]) || 0) > 0);
const nodes = [{ name: rootName }, ...rows.map(...)];
const links = rows.map(...);

The filter is unconditional. A sankey handed

[{ stage: 'New business', amount: 40 },
 { stage: 'Refunds',      amount: -25 },
 { stage: 'Chargebacks',  amount: -12 }]

draws one flow — "New business" — and nothing anywhere says the other two rows existed. Measured in Chromium in the same sweep: the one-positive-among-zeros tile rendered svg: 1, path: 3, 18 descendants, and no diagnostic of any kind. It is a normal, healthy-looking, confident sankey of a dataset it is showing a third of.

Why it is worth recording separately

#7140's blank is at least visibly wrong — nothing is on screen, and after PR #7146 it says so. This one is invisibly wrong: the chart looks correct, so nobody re-reads the dataset. A pipeline chart that quietly omits every refund/credit/chargeback row is a business-wrong picture presented as a complete one, and the same reasoning behind the empty-state work applies with more force, not less: the reader cannot tell a partial flow from a whole one.

Note the filter is also what folds null and unparseable measures to 0 (Number(...) || 0), so those rows disappear the same way in a mixed dataset.

What this card does NOT assert

It does not assert the drop is wrong. A flow has no negative width, so a sankey genuinely cannot draw those rows; discarding them may be the only thing it can do. What is undecided is whether it must say it did — and if so, whether that is a footnote beside a drawn chart, a count of omitted rows, or a refusal. That is a judgement this card does not pre-make, and it needs a look at a real dataset before anyone writes code. Note also that PR #7146 pins the "one positive among zeros still draws" boundary, so any answer here must keep a drawable sankey drawable.

Related

• finding(plugin-charts): a sankey whose rows are all zero-or-negative renders a literally empty div — the one silent blank left on the chart surface after #7130 #7140 / PR fix(plugin-charts): a sankey with no positive flow says so instead of rendering an empty div #7146 — the all-zero-or-negative case, where the same filter keeps nothing. Fixed there; this branch is untouched by it.
• finding(plugin-charts): funnel and pie handed all-zero rows render a pixel-identical blank tile — same silence as #7140, a different trigger #7147 — funnel and pie render a blank tile for all-zero rows, found in the same sweep.

[os-warren]

PM triage — pm:blocked on PR #7146 landing

Hard collision, and the card names it itself: this is "the branch next door" — the same filter, the same arm, the same function in AdvancedChartImpl.tsx that PR objectui#7146 is currently editing and which is not yet landed. Two lanes on one function is how conflicting shapes get landed twice.

It is also downstream in substance, not just in file: objectui#7146 pins the "one positive among zeros still draws" boundary, and this card states any answer here must keep a drawable sankey drawable. That pin has to exist before this work can be measured against it.

Unblocks automatically when objectui#7146 lands. No edits needed.

Why this is queued to be taken seriously rather than filed and forgotten

Of the two defects that sweep found, this is the more dangerous one and the card says so plainly. objectui#7140's blank was visibly wrong — nothing on screen, and after objectui#7146 it says why. This one is invisibly wrong: a healthy-looking, confident sankey showing a third of its dataset, with nothing anywhere indicating the other rows existed. A pipeline chart that silently omits every refund and chargeback is business-wrong presented as complete, and no reader re-reads a dataset that looks fine.

The card correctly declines to pre-make the answer (footnote / omitted-row count / refusal), and correctly notes a flow genuinely has no negative width — so discarding may be the only thing it can do; what is undecided is whether it must say so. That is a judgement a dev can settle by measurement, the way objectui#7140 was settled by rendering it in a browser first. It is not a maintainer decision, so it stays dispatchable once unblocked.

---

Generated by Claude Code

[os-warren]

🔓 Unblocked — pm:blocked → pm:queue, and this is now the seat's top-priority card

PR #7146 landed as 0349555c7 (verified by content: no-positive-flow 0→1, control hasNoPlottableSeries 2→4). The file collision on AdvancedChartImpl.tsx has cleared, and the boundary your answer must preserve — "one positive row among zeros still draws" — is now pinned on main, which is exactly the precondition this card said it needed.

Ranked ahead of every other queued card in this seat. Not on severity labels but on the distinction the card itself draws: objectui#7140's sankey blank was visibly wrong, and after objectui#7146 it says so. This one is invisibly wrong. A pipeline chart that silently omits every refund, credit and chargeback row renders as a normal, healthy, confident sankey of a dataset it is showing a third of — measured at svg: 1, path: 3, 18 descendants, no diagnostic of any kind. Nobody re-reads a dataset that looks fine.

Raised to priority:p2 for that reason. It will be dispatched as soon as a lane frees (currently full: objectui#6875, objectui#7036, objectui#7151).

Carried forward when it is dispatched, from the card and unchanged by me:

• ⛔ It does not assert the drop is wrong. A flow has no negative width, so discarding may be the only thing a sankey can do. What is undecided is whether it must say it did — footnote, omitted-row count, or refusal — and that is settleable by measurement, the way objectui#7140 was settled by rendering it first.
• Number(...) || 0 folds null and unparseable measures to 0, so those rows vanish through the same filter in a mixed dataset. Any answer has to cover them, not just negatives.
• ⛔ Whatever ships must keep a drawable sankey drawable — objectui#7146's boundary pin is now live and will red if not.

---

Generated by Claude Code

[os-warren]

🔒 Claimed — domain:ui seat

• Session: session_012wwHa4aaFybxXrfmfHioDM
• Branch: claude/issue-7148-sankey-mixed-sign-drop
• Seat post: [pm:seat] domain:ui @ objectui — 🟢 session_01KbJQ1y1J12nZxYzFWhP8Q3 (os-sam) · R1 · FORCED TAKEOVER 04:33Z on maintainer instruction · inherited 4 live dev PRs · ⚠️ #7577 awaiting contract review · 2 half-states to repair #5560

This comment (session ID + branch) is the claim — all agents share one GitHub identity, so the assignee field cannot tell you whose it is.

Dispatched first the moment a lane freed, ahead of every other queued card, for the reason recorded when it unblocked: objectui#7140's blank was visibly wrong and now says so; this one is invisibly wrong. A healthy-looking, confident sankey showing a third of its dataset is worse than a blank tile, because nobody re-reads data that renders fine.

Its blocker PR objectui#7146 landed as 0349555c7 (verified: no-positive-flow 0→1, control hasNoPlottableSeries 2→4), so the boundary any answer must preserve — "one positive row among zeros still draws" — is now pinned on main.

---

Generated by Claude Code

[os-warren]

os-dev-report

{
  "issue": 7148,
  "status": "done",
  "branch": "claude/issue-7148-sankey-mixed-sign-drop",
  "pr": "https://github.com/objectstack-ai/objectui/pull/7161",
  "premise_still_valid": true,
  "summary": "MEASURED FIRST, then fixed. 27 tiles in real Chromium at origin/main fd11e1644 (head had moved past the assumed 0101fabab), each screenshotted and SHA-256'd. The decider: at base SIX datasets produced one byte-identical image 13237e6e19a7072a - the card's mixed-sign dataset, the 0/7/0 pin fixture, positive+nulls, positive+unparseable, positive+missing-key, AND sankey-genuine-1row-40, a dataset that genuinely has one row. A chart showing a third of its data was pixel-for-pixel a complete chart of one row, with svg 1 / path 3 / 18 descendants, no role, no text, and zero console output against a live console control that DID fire. measured-and-declined was available and the evidence does not support it. Shipped: a footnote under the plot, 'Showing 1 of 3 rows - 2 rows have no amount above zero, which a flow cannot draw.' The drop itself is unchanged and defended in the code comment: a flow has no negative width, so discarding is all that arm CAN do; the silence was the defect. A refusal was ruled OUT by measurement, not taste - PR 7146's pinned fixture 0/7/0 is itself a thinned dataset that lands in this branch, so refusing on a thinned flow would blank the chart that pin requires drawn. Second measured design constraint: a plain sibling paragraph is INVISIBLE in dashboards - in the dashboard shape (fixed clipping card, chart carrying h-full) the note landed at y=327 in a box ending at y=316, entirely outside the clip. A wrapper div carrying className is equally unavailable because that className is the chart's height contract and must keep reaching the element Recharts measures (CHART_MIN_HEIGHT in ChartContainerImpl). So it hangs off a flex chain in a new ChartFootnote helper - deliberately the mirror image of this file's existing ChartFrame, including its gate: with nothing to say the chart is returned untouched and gains no wrapper element. CONTRACT TIER: clause 2 stays NOT engaged - no public export widened, no schema key, no parse accept/reject change; render behaviour inside one plugin component, plus one file-local helper. i18n: no key added, matching the file's hardcoded English (check:i18n-keys green). SEAM MAP with 7146, pinned from both sides in the new test: rows.length is the whole boundary. data empty -> bare div, the empty-RESULT question (7130) answered upstream, untouched by both cards. rows.length 0 with data present -> 7146's no-positive-flow refusal; measured: all-zeros-plus-one-negative lands HERE, and all-negative too. rows.length between 1 and data.length -> this card's note; measured: one-positive-plus-nulls lands HERE, and so does 7146's own 0/7/0 pin fixture, which now draws AND says so. rows.length equals data.length -> untouched, byte-identical. ASSUMPTION FALSIFIED - A2.2 as worded ('the trigger is exactly some rows survive the filter') is necessary but not sufficient: a COMPLETE draw also has rows surviving. The precise trigger is 0 < rows.length < data.length, i.e. some survive AND at least one is dropped. That distinction is load-bearing - it is exactly what keeps the all-positive path byte-for-byte unchanged. A2.4 CONFIRMED: no pre-existing diagnostic of any kind - 1 console message captured across 27 tiles and it was the missing-category-key positive control, so the zero is a real reading and not a dead instrument. A2.5 CONFIRMED: both counts are in scope at the filter site (data.length pre-filter, rows.length post-filter), verified statically and used directly. A2.1 CONFIRMED and extended: head had moved past 0101fabab to fd11e1644; the card's snippet and line numbers are stale exactly as warned - the arm now carries the no-positive-flow refusal, and the filter is at line 1047 not where the card shows it. NULLS AND UNPARSEABLE ARE COVERED (ruling 5): all five shapes that Number(...) || 0 folds to zero - negative, zero, null, unparseable string, missing key - were measured reaching this branch beside a survivor, and each is a case in the new test. The copy names the PREDICATE the filter applies rather than any cause, for the reason 7146's own copy gives: naming one cause is a sentence false for the other four. I did NOT touch 7146's 'no row's amount is above zero' wording - PM recorded that imprecision but did not assign it, the correct rewording is a judgement not pinned by existing evidence, and my fix does not need it.",
  "tests": "All at final head f2df89144, from the repo root, heavy runs serialized through os-verify-lock.sh. (1) pnpm vitest run packages/plugin-charts/ -> 'Test Files 38 passed (38) / Tests 265 passed (265)' - PASSING count asserted, not just absence of red. Includes PR 7146's AdvancedChartImpl.sankeyNoPositiveFlow.test.tsx unmodified and green, so the boundary pin is not red. New file AdvancedChartImpl.sankeyOmittedRows.test.tsx: 14 cases covering all five coercion shapes, singular/plural agreement, the all-positive control asserting NO note and that the chart container is still the root element, both seam directions, and a cross-family case pinning that the note cannot reach bar/pie/donut/funnel/treemap. (2) pnpm --filter @object-ui/plugin-charts type-check -> exit 0, script name echoed (not a zero-match false green); tsc --listFiles confirms the new test file IS in the program (1 hit, control: an existing sibling test also 1) so 'typecheck clean' actually covers it. (3) pnpm --filter @object-ui/plugin-charts lint -> 51 files judged, 0 errors, 282 warnings; --max-warnings is deliberately unset repo-wide (lint.yml line 20, eslint.config.js line 57) so warnings cannot fail it; the 6 warnings on the new test file are the same as-any / importActual type-argument shape every sibling test in the package carries. (4) check:control-bytes 'OK (scanned 5931 tracked text file(s))'; plus an independent control-character self-scan over my three changed files, no matches. (5) check:vi-mock-specifiers and check:vi-mock-inherit both OK - directly implicated because the new test mocks recharts. (6) check:i18n-keys OK, check:i18n-drift 'No en value changed in this range'. (7) changeset:check -> 'All workspace packages are in the changeset fixed group.' and 'No changeset declares a major bump.' - quoted verbatim, not predicted; changeset declares patch. (8) check:sdui-registration-pins FIRST returned exit 2, 'No console build to weigh' - that is NOT MEASURED, not a pass, and the gate says so itself. Built the console closure and the console, re-ran: 'All 16 registration(s) a sideEffects array promises are present in the built console (517 chunks weighed; the 3 ruled control(s) are in the derived set)'. So it is a real green, not the expected non-reading. ABLATION / REVERSE VERIFICATION, direction predicted BEFORE running (thinned tiles lose their note and collapse back onto the genuine-one-row hash; all-positive and other families must be byte-IDENTICAL): implementation committed FIRST, then the pre-fix file restored onto disk. Mutation proven on disk by blob hash (on-disk 3105af33 equal to the base blob, differing from the HEAD blob 5e2486b8), by marker count (omitted-rows 0, ChartFootnote 0), and by confirming the marker was absent from the REBUILT bundle - so the measurement ran against the mutated code, not a stale build. Harness rebuilt with byte-identical CSS, all 31 tiles re-measured. Result: exactly 8 tiles changed and all 8 are thinned sankeys; 23 tiles byte-identical including sankey-control-all-positive, chain-clip-all-positive and chain-auto-all-positive (the complete-flow path unchanged in all three height chains), both refusal tiles, the no-rows tile and every other chart family. The collision with sankey-genuine-1row-40 is broken - it keeps 13237e6e19a7072a while the five thinned tiles move away. Restore proven by STATE, not exit code: git diff HEAD, git diff --cached and git status --short all clean, and the on-disk blob back to 5e2486b8; the script carried a trap on EXIT INT TERM with absolute paths derived from git rev-parse --show-toplevel. A first attempt at the fix (plain sibling paragraph) was measured, found clipped in the dashboard chain, and discarded before commit - it is not in the branch. NARROWED LOCAL SCOPE, declared: repo-wide pnpm lint is CI's run. The narrowing is measured on three legs - universe read from eslint's own config resolution (51 files, not my guess), file count read from --format json, and an invariance statement: eslint.config.js configures NO type-aware linting (0 hits for parserOptions/projectService/project:), so a change under packages/plugin-charts/src cannot move the verdict on any untouched file in another package. NOT MEASURED, named rather than reported as green: the CI-only farm beyond the gates listed above (55+ check:* scripts CI runs regardless), and this repo has no scripts/pm/dispatch-gates.mjs - that script lives only in objectstack and answering with it here would produce objectstack's gate family for objectui paths, so the list above was derived by hand from this repo's package.json and .github/workflows/.",
  "mcp_calls": "2 - create_pull_request and this comment. The card body and all three PM comments were read at ZERO API cost via the public-repo embedded JSON payload channel; the repo-scoped REST probe returned 403 for this seat (authed and unauthed alike), so writes were routed to MCP - channel switch declared.",
  "open_questions": [],
  "out_of_scope_findings": [
    "NOT filed, reported to PM instead, because it lands inside already-queued objectui#7147 and the fence says report-not-fix: the reader-facing symptom reaches treemap, funnel and pie by a DIFFERENT mechanism, and it widens 7147 beyond all-zero datasets to mixed ones. Measured with per-family all-positive controls in the same sweep: treemap handed the mixed-sign dataset drew 2 rects / 2 labels against a control's 4 / 4; funnel handed positive-plus-null drew 0 segment paths and 1 label against a control's 2 / 2; pie the same, 1 slice against a control's 2. Those rows are never filtered - they stay in the data array and the layout gives them no area - so it is degenerate geometry, not the row drop fixed here, and a hoisted copy of my count would say nothing useful about them. Filing it separately would duplicate 7147; PM should attach it there.",
    "Confirms the fix cannot leak: AdvancedChartImpl.tsx line 1047 is the ONLY row-dropping filter in the whole file - the other two .filter( sites filter SERIES, not rows. That is why the note is structurally sankey-only, and it is pinned in the new test rather than left as a claim.",
    "Untouched by design, flagged for the record: PR 7146's refusal copy 'no row's amount is above zero' is true of post-coercion values only, as PM already recorded. I left it alone - the correct rewording is a judgement call not pinned by existing evidence, and it is not needed for this fix."
  ]
}

Generated by Claude Code

---

Generated by Claude Code