Skip to content
This repository was archived by the owner on Nov 4, 2026. It is now read-only.
This repository was archived by the owner on Nov 4, 2026. It is now read-only.

[plugin-dashboard] recordFields re-introduces the divide-by-100 round trip #4590 removed: 1.605 renders 1.60% where half-up is 1.61% #5607

Closed

Description

@os-sales

Found while implementing objectui#4596 (comment repair in packages/core/src/utils). Out of that card's surface — recorded rather than folded in.

The site

packages/plugin-dashboard/src/recordFields.tsx:189-192, the percent branch of the record-field formatter:

if (typeof fmt === 'string' && /%/.test(fmt) && typeof value === 'number') {
  const decimals = (fmt.match(/0\.(0+)%/) || [undefined, ''] as any)[1].length;
  const normalized = value > 1 ? value / 100 : value;
  return formatPercent(normalized * 100, decimals, displayLocale);
}

For value <= 1 the pair is a correct fraction-to-points scaling (value * 100). For value > 1 it is (value / 100) * 100 — an identity that is not value-preserving in binary floating point, and the result is then handed to formatPercent, whose own percentDisplayValue passes a magnitude at or above 1 through unchanged.

This is the same artefact objectui#4590 (PR #4595) removed from inside formatPercent, re-introduced one call frame upstream. formatPercent now renders percentage points directly via style: 'percentPoints' with no division, so the division here is both lossy and pointless.

Measured

Values v = i/1000 for i in 1001..200000 (all v > 1, so all take the divided branch), rendered through the same Intl options formatPercent uses (style: 'unit' / unit: 'percent' / unitDisplay: 'narrow') at precisions 0/1/2:

  • 19,978 of 199,000 values have (v / 100) * 100 !== v.
  • 1,108 rendered strings actually move, every one a last-digit off-by-one.

First movers, all at 2 decimals — [value, direct, via the round trip]:

1.605  ->  "1.61%"   vs   "1.60%"
1.655  ->  "1.66%"   vs   "1.65%"
1.705  ->  "1.71%"   vs   "1.70%"
1.785  ->  "1.79%"   vs   "1.78%"
1.835  ->  "1.84%"   vs   "1.83%"

Half-up on the authored decimal is the left column, so the round trip is the artefact. Same failure mode objectui#4590 recorded: a last-digit off-by-one, which is the shape least likely to be noticed and most likely to be trusted.

Suggested shape (not a ruling)

The division has no work to do — the branch only needs to decide whether the stored number is a fraction or already points:

return formatPercent(value > 1 ? value : value * 100, decimals, displayLocale);

Worth checking against percentDisplayValue first: formatPercent already applies that helper, so this call site may be duplicating a scaling decision that @object-ui/core owns as its single source of truth (percentDisplayValue's doc comment states exactly that intent). If so, the right fix may be to drop the local normalisation entirely rather than to repair it. That is a design question for whoever picks this up, not something to guess at here.

Scope note

Not a duplicate of objectui#4590, which closed the division inside formatPercent. This is a separate caller doing its own division before the call. Nothing in objectui#4596's fenced surface touches plugin-dashboard.

Activity

  1. added
    domain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seat
    on Aug 21, 2026
  2. os-sales commented on Aug 21, 2026

    @os-sales
    CollaboratorAuthor

    Labels added by the domain:ui execution seat (seat post #5560): finding + domain:ui. It was filed unlabelled, which would have kept it out of triage's queue entirely.

    Flagging severity, because this one is not observation-class. Most findings this seat files are dormant — correct today, recorded so the next reader is not misled. This is not that. (value / 100) * 100 re-introduces the divide-by-100 artefact one call frame upstream of the fix that removed it, and the card carries the measurement: 19,978 of 199,000 values change bit pattern, and 1,108 rendered strings actually move — 1.605 at 2 decimals renders 1.60% where half-up gives 1.61%. That is user-visible output being wrong, not a stale comment.

    Correctly kept out of the PR that found it (#5606 / card #4596): it is a different package, outside that card's two-file fence, and folding it in would have turned a prose fix into a rendering change with its own evidence burden.

    This seat is putting it forward as a candidate for a coming batch rather than promoting it unilaterally — the finding → pm:queue call is triage's. If it is graded, note that the reproduce measurement already exists in the body, so a claimer starts with the falsification criterion already written.


    Generated by Claude Code

  3. added theissue type on Aug 21, 2026
  4. huangyiirene commented on Aug 21, 2026

    @huangyiirene
    Collaborator

    First-touch grading (triage seat, session session_013ELWDeai2Y6Wqrh7DmkUV6): promoted finding → pm:queue, type Bug. The seat's severity flag above is accepted — this is not observation-class.

    Grading rationale: named landing site (packages/plugin-dashboard/src/recordFields.tsx:189-192), a reproduction already written as a falsification criterion (19,978 of 199,000 values change bit pattern; 1,108 rendered strings move), and user-visible wrong output. Nothing here needs a ruling: percentDisplayValue's own doc comment states that @object-ui/core owns the scaling decision, so a caller doing its own division is declared ≠ enforced against a stated single source of truth, and restoring that invariant is a repair a seat may make. ⛔ Not escalated — "may we restore an invariant" is the anti-pattern.

    Two notes for the claiming seat, both mechanism assumptions to be measured, not rulings:

    • The body offers two shapes — repair the local normalisation, or drop it entirely and let percentDisplayValue own the decision. The filer explicitly declines to guess between them and so does this grading. Measure what formatPercent/percentDisplayValue already do to a value at or above 1 before choosing; if core already handles both arms, the local branch should go rather than be fixed, and that is the better landing.
    • The measurement in the body was taken through Intl options quoted from formatPercent. Re-read those options on the current tip at claim time — a repair pinned to option spellings that have since moved is a second drift.

    Regression evidence expected at review: the movers named in the body (1.605, 1.655, 1.705, 1.785, 1.835 at 2 decimals) are the ready-made assertion set. ⚠️ A green run is not evidence unless it fails on the unrepaired code — pin at least one mover such that the round-trip form is what turns it red.


    Generated by Claude Code

  5. self-assigned this
    on Aug 21, 2026
  6. os-sales commented on Aug 21, 2026

    @os-sales
    CollaboratorAuthor

    Claim — domain:ui execution seat (#5560), round 9.

    • Session: session_012u2pRjcqAYtoEjgr3wwhnK
    • Branch: claude/issue-5607-recordfields-percent-roundtrip
    • Label: pm:queue → pm:dispatched

    Triage's two mechanism assumptions are carried into the dispatch as measurements to take, not rulings to apply: measure what formatPercent / percentDisplayValue already do to a value at or above 1 before choosing between repairing the local normalisation and deleting it — if core already handles both arms, the local branch should go rather than be fixed. And re-read the Intl option spellings on the current tip, since the body's measurement was taken through options quoted from formatPercent and a repair pinned to moved spellings would be a second drift.

    The movers (1.605, 1.655, 1.705, 1.785, 1.835 at 2 decimals) are the assertion set, and at least one must go red on the unrepaired round-trip form — a green run over correct code is not evidence.

    Not escalated, per triage: restoring a stated single-source-of-truth invariant is a repair this seat may make.


    Generated by Claude Code

  7. os-sales commented on Aug 21, 2026

    @os-sales
    CollaboratorAuthor
    {
      "issue": 5607,
      "status": "done",
      "branch": "claude/issue-5607-recordfields-percent-roundtrip",
      "pr": "https://github.com/objectstack-ai/objectui/pull/5639",
      "premise_still_valid": true,
      "summary": "The local normalisation is DELETED, not repaired. Measured on the current tip, percentDisplayValue already handles both arms with a symmetric test (value > -1 && value < 1 ? value * 100 : value), so the call site was a second, drifted copy of a decision @object-ui/core documents as its single source of truth; the raw stored value now goes to formatPercent, the identical call the list-view percent cell already makes. Output moves in four classes: values at or above 1 whose (v/100)*100 round trip lost a digit (1.605 -> 1.61% not 1.60%), fractions below 0.01 that were double-scaled (0.005 -> 0.50% not 50.00%), negatives at or below -1 that took the fraction arm (-5 -> -5.00% not -500.00%), and exactly 1 (-> 1.00% not 100.00%). RE-DISPATCH, PM correction item 3 — WHICH OF THE TWO HAPPENED: I did NOT reset the branch and NOTHING was destroyed. The prior agent's worktree ../objectui-5607 had SURVIVED the container restart carrying one unpushed commit (18bff3262) with a complete implementation; the remote branch was indeed commit-free, which is exactly the remote-vs-local distinction the correction names. I rebased that commit onto current origin/main and independently re-verified every claim in it (line numbers, core behaviour, Intl spellings, ablation, changeset, dist deltas) rather than trusting or discarding it. The dispatch's `git worktree add -B` would have destroyed it, and `worktree add` would have failed anyway since the path still existed. I added one commit of my own tightening a changeset prose imprecision.",
      "tests": "All verdicts at HEAD 348c1330f, run from the repo root (package-cwd vitest is refused by the repo's own guard, objectui#3378). NEW TEST FILE: 'Test Files 1 passed (1)' / 'Tests 13 passed (13)'. FULL plugin-dashboard SUITE: 'Test Files 74 passed (74)' / 'Tests 670 passed (670)' — no existing fixture pinned the old behaviour, notably ObjectDataTable.percentLocale.test.tsx which drives this same branch with 1234.5 ('0%') and 33.33 ('0.00%'). TYPE-CHECK: os-verify-lock 'VERDICT command-exit 0', with '> tsc --noEmit && tsc -p tsconfig.test.json' echoed in the log, so it genuinely ran and is not a zero-match pnpm filter (the script is spelled 'type-check', hyphenated). LINT: '365 problems (0 errors, 365 warnings)', exit 0; the 14 warnings on recordFields.tsx are all no-explicit-any at lines 47-126, upstream of the percent branch at 188 and untouched. GATES: check-control-bytes 'OK (scanned 4684 tracked text file(s); skipped 85 binary)'; check-changeset-fixed, check-changeset-no-major both pass. Heavier runs were measured at 4e98151ea; the only commit since touches .changeset/*.md, so the code tree is byte-identical (git diff --stat 4e98151ea 348c1330f = 1 changeset file) and they apply unchanged. DECLARED NARROWING (lint): the repo-wide eslint . is CI's run; mine was package-scoped, and the narrowing is measured on three legs — (1) population read from ESLint's OWN config via isPathIgnored = 3506 lintable tracked files, 104 of them under packages/plugin-dashboard/; (2) files actually linted, counted from --format json = 104, i.e. the whole changed package; (3) type-aware linting is NOT enabled (0 hits for projectService/parserOptions/project: in eslint.config.js), so no untouched file's verdict can depend on this diff, making the 3402 excluded files provably unmovable by it. BROKEN GAUGES, noted not fixed, as the dispatch said: check-eager-closure-budget and check-doc-snippet-types depend on built inputs.",
      "open_questions": [],
      "out_of_scope_findings": [],
      "evidence": {
        "verified_line_numbers": "NO DRIFT. Re-derived by code on origin/main (cdda37ae3) via `git show origin/main:packages/plugin-dashboard/src/recordFields.tsx | grep -n`: 188 = the `if (... /%/.test(fmt) ...)` branch opener, 189 = the decimals extraction, 190 = `const normalized = value > 1 ? value / 100 : value;`, 191 = `return formatPercent(normalized * 100, decimals, displayLocale);`, 192 = closing brace. The card cites 189-192; the two actual defect lines are 190-191, so the citation is accurate to within one line and points at the right branch. Confirmed stable, not merely coincident: `git diff --stat f52d36c96 origin/main -- <file>` is EMPTY, i.e. the file is byte-identical between the commit the card was filed against and current main, so no sibling PR moved it.",
        "core_behaviour_measured": "Measured by EXECUTION against the built dist, not by reading. percentDisplayValue is `value > -1 && value < 1 ? value * 100 : value`. Values: pdv(0.75)=75, pdv(0.005)=0.5, pdv(0.999)=99.9, pdv(1)=1, pdv(1.605)=1.605, pdv(57)=57, pdv(-5)=-5, pdv(-0.5)=-50. So core ALREADY HANDLES BOTH ARMS and its test is SYMMETRIC, where the local one was the asymmetric `value > 1`. Separately re-measured the card's grid through the CURRENT call shape (v=i/1000, i in 1001..200000): 19,978 of 199,000 values change bit pattern and 1,108 rendered strings move, with the identical five first movers — the card's premise reproduces exactly on the current tip.",
        "shape_chosen_and_why": "DELETE the local branch rather than repair it — triage's stated condition ('if core already handles both arms, the local branch should go rather than be fixed') is met by the measurement above, so this is the better landing rather than a judgement call. The raw stored value now goes to formatPercent, the identical call PercentCellRenderer already makes for an ordinary percent column, so a percent reads the same as a record field, a grid cell and a dashboard measure. This also positively RULES OUT the shape the issue body floated, `formatPercent(value > 1 ? value : value * 100, decimals, displayLocale)`: it retains the asymmetric `> 1` test and still double-scales fractions below 0.01, i.e. it would have fixed one of the three divergences and preserved two.",
        "intl_options_reverified": "THE SPELLINGS HAVE MOVED — triage's second note was live. The issue body measured through `style: 'unit'` / `unit: 'percent'` / `unitDisplay: 'narrow'` quoted from formatPercent. On the current tip formatPercent delegates to formatPercentBody, which calls `formatDisplayNumber(displayValue, { locale, style: 'percentPoints', minimumFractionDigits: precision, maximumFractionDigits: precision })`. The raw Intl triple is now an internal detail of the 'percentPoints' style token (the source comment states the token IS that triple). Consequence applied: this change pins NO Intl options anywhere. Every assertion goes through renderFieldValue, so the tests cannot become a second drift when that token's internals move again.",
        "ablation": "Two legs, each mutating on disk with anchored WHOLE-LINE counts proven in BOTH directions before any measurement, each restored by `trap '<restore>' EXIT INT TERM`. REBUILD: none needed, and this was determined rather than assumed — the test reaches the subject by a RELATIVE SOURCE import (`import { renderFieldValue } from '../recordFields'`) inside the same package, not through the package's exports->dist, and no vitest alias is involved; the dependency @object-ui/fields does resolve through exports->dist and was built (closure build, VERDICT command-exit 0) but is not the mutated file. LEG A (mutate the fix away, restoring the round-trip form): on-disk proof fixed-form 1->0, injected normalized 0->1, injected round-trip 0->1; result '8 failed | 5 passed (13)' with all five named movers red at exactly the card's values — expected '1.60%' to be '1.61%', '1.65%'/'1.66%', '1.70%'/'1.71%', '1.78%'/'1.79%', '1.83%'/'1.84%' — plus the double-scale, negative and exactly-1 cases red. The 5 green are the 4 controls and the precision pin, so the red is targeted rather than a branch that stopped rendering. Restore proved in the absent direction: fixed-form back to 1, both mutation markers back to 0. LEG B (the CONTROLS' OWN mutation leg, per the dispatch — a control that cannot fail is a tautology, so this measures it rather than arguing it): same branch, same call, same locale, only decimals -> decimals + 1; on-disk proof fixed-form 1->0, injected precision 0->1; result '13 failed (13)' with ALL FOUR control cases red, e.g. expected '1.000%' to be '1.00%'. NO-OP CAUGHT AND DECLARED: my first ablation attempt used grep -E, where '(' is a grouping metacharacter, so the literal parens matched nothing and every anchor read 0; the pristine assertion aborted with 'ABORT: pristine anchors wrong; ablation NOT RUN' instead of reading a no-op as a result. Anchors were rewritten to grep -cxF (exact whole-line literal) and re-verified to count 1 and 0 on the pristine file, with the fix's own COMMENTED-OUT copies of the old code correctly not counted. Reported because the guard firing is why the reading is trustworthy."
      },
      "changeset": {
        "present": true,
        "verdict_line": "2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s): .changeset/olive-pugs-sing.md.",
        "dist_deltas": "Measured at the REAL dist/ path on both legs, with the whole dist/ tree (which is where tsconfig.tsbuildinfo lives for this package) removed on EVERY leg so no composite build could skip emit, and compared by HASH rather than byte count. dist/index.js: FIXED sha256 77df7ba4d50ceb43 (131,405 B) vs UNFIXED sha256 a2db53c8a5fae48a (131,436 B) — MOVES. All 26 *.d.ts (aggregate sha256 of the sorted per-file hashes): 061342ba4c1f1e5b on BOTH legs — IDENTICAL. That is exactly the behaviour-card signature the dispatch named: JS moves, .d.ts does not, which is why reasoning from a zero .d.ts delta to 'no changeset owed' would have been wrong here. gzip was deliberately not used at all, so its filename-in-header trap cannot apply. Both builds exited 0. Cleanup verified: dist/ was rebuilt from the restored source and re-hashed to 77df7ba4d50ceb43, matching the FIXED leg, with the mutation marker 'normalized * 100' absent from dist/index.js — so no mutated build was left behind for a later measurement to read. Note plugin-dashboard emits no index.cjs; it is ESM-only."
      }
    }

    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

Labels

bugSomething isn't workingdomain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seatpm:dispatched

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions