Skip to content

MilestonesPage divides by zero on an unfunded budget and never clamps an over-100% distributed percentage #75

Description

@chonilius

Overview

src/app/milestones/page.tsx computes each milestone's funding progress with a bare division and no guard against a zero (or exceeded) budget:

{milestones.map((m) => {
  const pct = m.distributed / m.budget;
  return (
    <div key={m.id} ...>
      ...
      <div className="mt-4 h-2 w-full overflow-hidden rounded-full bg-slate-100 dark:bg-slate-800">
        <div
          className="h-full bg-indigo-600"
          style={{ width: `${Math.min(pct * 100, 100)}%` }}
        />
      </div>
      <div className="mt-3 flex items-center justify-between text-sm ...">
        <span>
          {formatCurrency(m.distributed, m.asset)} of{" "}
          {formatCurrency(m.budget, m.asset)}
        </span>
        <span>{formatPercent(pct)}</span>
      </div>
      ...

Two distinct bugs here, both reachable with real backend data given Milestone.budget/distributed are just coerceNonNegative(raw.budget)-derived numbers (see src/lib/adapters.ts:94-106) with no floor above zero and no relationship enforced between the two fields:

  1. Divide-by-zero. A freshly-created milestone with budget: 0 (a legitimate state — a maintainer creates the milestone shell before any sponsor has funded anything, or a milestone's issues were all removed and its budget reset) produces pct = 0 / 0 = NaN in JavaScript. Math.min(NaN * 100, 100) is NaN, so style={{ width: "NaN%" }} is passed to the DOM directly (this line doesn't go through formatPercent) — invalid CSS that browsers silently drop, leaving the progress bar's width at whatever it happened to inherit/default to (typically collapsing to 0 width from the h-2 container with no explicit width, but this is undefined behavior riding on browser CSS-parsing leniency, not a deliberate 0% state). The adjacent text readout, formatPercent(pct), does guard against non-finite values (if (!Number.isFinite(value)) return "0%", see src/lib/utils.ts:40-43), so for budget: 0, distributed: 0 it actually renders "0%" rather than the literal string "NaN%" — which sounds safer but is its own problem, covered next.
  2. The "safe" budget: 0, distributed > 0 case is actually the worse bug. 5 / 0 === Infinity in JavaScript (not NaN — only 0 / 0 is NaN), and Number.isFinite(Infinity) is false, so formatPercent catches this too and also returns "0%". That means a milestone that has actually received real distributed funds against a zero-recorded budget displays "0% — $X of $0 USDC", reading as "nothing has happened yet" when the opposite is true. This is worse than an obviously-broken "NaN%" would be, precisely because it looks plausible instead of visibly wrong.
  3. Unclamped over-100% display. If distributed ever exceeds budget (a real possibility given rounding/timing between multiple sponsors funding concurrently, or a data-entry/backend correction), the progress bar is correctly clamped via Math.min(pct * 100, 100) — but the text readout, formatPercent(pct), has no clamp at all and would show something like "127%" right next to a progress bar that's visually maxed out at 100%, an inconsistent and confusing display of the exact same underlying number.

Requirements

  • Guard the pct calculation against budget === 0 (and, defensively, against budget < 0 if that's ever possible) — render an explicit, honest state (e.g. "Not yet funded" / a 0%-width bar with no percentage text, rather than "NaN%") instead of propagating NaN into a CSS custom property and a text node.
  • Clamp formatPercent(pct)'s displayed value to be consistent with the bar's own Math.min(..., 100) clamp — either clamp inside formatPercent generally (check other call sites first to make sure a >100% clamp is universally correct for all of formatPercent's callers, not just this one) or clamp pct itself before passing it to both the bar width and the text.
  • Consider whether an over-100%-distributed state is itself a signal worth surfacing distinctly (e.g. a small badge/tooltip noting the milestone is over-distributed relative to its budget) rather than merely capping the visual representation — this is a real data-integrity signal for a maintainer/sponsor to know about, not just a rendering nuisance.
  • Apply the same audit to MaintenancePool-related percentage displays if any exist with the same shape (checked: pools.map(...) in the same file does not currently compute a percentage, only raw balance/deposit figures, so no equivalent fix is needed there today — but confirm this holds after any future changes).

Acceptance Criteria

  • A milestone with budget: 0 renders a progress bar and percentage text with no NaN anywhere in the DOM, in a state that's honestly distinguishable from "0% of a real, funded budget."
  • A milestone with distributed > budget renders a percentage text that agrees with the bar's clamped-at-100% visual state (both capped consistently, or both showing the true over-100% figure with a clear "over-distributed" treatment — pick one and apply it to both, don't leave them disagreeing).
  • A normal milestone (0 < distributed <= budget) renders identically to today — no visual regression for the common case.
  • A test covers all three cases: budget: 0, distributed > budget, and the normal case.

Additional Notes

Precise references:

  • src/app/milestones/page.tsx:30 (const pct = m.distributed / m.budget;), :38-43 (bar width, correctly clamped), :44-49 (text readout, formatPercent(pct), not clamped).
  • src/lib/utils.ts:40-43formatPercent: if (!Number.isFinite(value)) return "0%"; return \${Math.round(value * 100)}%`;. This is what makes the budget: 0, distributed > 0` case (bug Handle Freighter network-mismatch and wallet-state edge cases before signing #2 above) the one most worth getting right in the fix — it's the case that currently produces a plausible-looking but wrong "0%" rather than an obviously-broken "NaN%".
  • src/lib/adapters.ts:94-106 (adaptMilestone) — confirms budget/distributed are independently coerceNonNegative'd from raw backend strings with no cross-field validation (nothing enforces distributed <= budget or budget > 0 at the adapter layer).

Edge cases: negative budget/distributed are already floored to the adapter's fallback (0) by coerceNonNegative, so true negative values shouldn't reach this component — but re-verify assumptions hold if adaptMilestone is ever changed.

Test/reproduction plan: render the milestones list with three fixture milestones — {budget: 0, distributed: 0}, {budget: 0, distributed: 50}, {budget: 100, distributed: 150} — and assert: no "NaN" substring appears anywhere in the rendered output for any of them; the {budget: 0, distributed: 50} case does not render "0%" (the misleading-Infinity case flagged above); and the {budget: 100, distributed: 150} case's bar width and percentage text agree with each other under whatever clamping policy is chosen.

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26Third CampaignCampaign: Third CampaignbugSomething isn't workingvery hardVery difficult task, expert-level effort required

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions