Skip to content

fix(#2313): guard the display formatters that could render literal NaN - #2529

Merged
dcccrypto merged 1 commit into
playgroundfrom
fix/numeric-display-hygiene
Sep 2, 2026
Merged

fix(#2313): guard the display formatters that could render literal NaN#2529
dcccrypto merged 1 commit into
playgroundfrom
fix/numeric-display-hygiene

Conversation

@dcccrypto

@dcccrypto dcccrypto commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Closes #2313.

(NaN).toFixed(2) is the string "NaN", and every magnitude comparison against NaN is false — so NaN falls through each n >= 1e6 branch and lands on the final toFixed, producing $NaN in the DOM with no error raised anywhere.

Scope correction

The issue says "zero Number.isFinite() guards anywhere in the Earn display pipeline". That is no longer true, and the real exposure is narrower. I checked each claim rather than taking the count:

site status
lib/formatters.ts formatCompact already guarded
OiCapMeter utilPct guarded at source — if (maxOI <= 0) return 0
LpPositionDashboard userSharePct guarded at source — lpSupply > 0n ? … : 0
VaultDepositRail / VaultRow fees safe (?? 10, plain field)

Three real gaps, all fixed

  1. Watchlist.tsx carried a private copy of formatCompact with no guard at all.
  2. MarketInfoBar.tsx carried a second private copy that guarded null but not NaN — the two copies had already diverged in exactly the way duplication invites.
  3. DepositWithdrawPanel.tsx computed a share preview inline as Number(previewShares) / Number(lpSupply + previewShares) — which is 0/0 = NaN for the first deposit into an empty pool. That is the one a user can actually hit, and it renders NaN% on the deposit screen.

Both private copies are deleted in favour of a shared, guarded formatCompactUsd; the inline division goes through a new guarded formatPercent.

Consolidating is the durable half. Two copies of a formatter will drift — and here they already had.

The shared version keeps the stricter of the two behaviours (null-guarded, like MarketInfoBar's), not the laxer one.

Verification

  • Negative control: removing the two guards fails 7 of the 12 new tests
  • five more pin that real values still format across every magnitude branch, so this cannot be satisfied by returning a placeholder for everything
  • launch suite 3123 passed / 16 skipped / 0 failed

🤖 Generated with Claude Code

https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D

Summary by CodeRabbit

  • New Features

    • Added consistent formatting for compact USD values and percentages.
    • Invalid or unavailable numeric values now display as an em dash instead of NaN or infinity.
    • Deposit share previews now use standardized percentage formatting.
  • Bug Fixes

    • Improved volume, open interest, and share-preview displays for invalid or missing values.
    • Standardized numeric formatting across watchlists and market information.

The mechanism is quiet: `(NaN).toFixed(2)` is the STRING "NaN", and every
magnitude comparison against NaN is false — so NaN falls through each
`n >= 1e6` branch and lands on the final toFixed, producing "$NaN" in the DOM
with no error raised anywhere.

SCOPE CORRECTION. The issue says "zero Number.isFinite guards anywhere in the
Earn display pipeline". That is no longer true, and the real exposure is
narrower. I checked each claim:

  lib/formatters.ts formatCompact  — ALREADY guarded
  OiCapMeter utilPct               — guarded at source (`if (maxOI <= 0) return 0`)
  LpPositionDashboard userSharePct — guarded at source (`lpSupply > 0n ? ... : 0`)
  VaultDepositRail / VaultRow fees — safe (`?? 10` / plain field)

Three real gaps remained, and they are fixed here:

1. Watchlist.tsx carried a PRIVATE copy of formatCompact with no guard at all.
2. MarketInfoBar.tsx carried a second private copy that guarded `null` but not
   NaN — so the two copies had already diverged in exactly the way duplication
   invites.
3. DepositWithdrawPanel.tsx computed a share preview inline as
   `Number(previewShares) / Number(lpSupply + previewShares)`, which is 0/0 —
   NaN — for the first deposit into an empty pool. That is the one a user could
   actually hit, and it renders "NaN%" on the deposit screen.

Both private copies are deleted and replaced by a shared, guarded
`formatCompactUsd`; the inline division goes through a new guarded
`formatPercent`. Consolidating is the durable half — two copies of a formatter
will drift again, and here they already had.

The shared version keeps the STRICTER of the two behaviours (null-guarded, like
MarketInfoBar's) rather than the laxer one.

Negative control: removing the two guards fails 7 of the 12 new tests. Five more
pin that real values still format across every magnitude branch, so this cannot
be satisfied by returning a placeholder for everything.

Launch suite: 3123 passed / 16 skipped / 0 failed.

Refs: #2313

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
@vercel

vercel Bot commented Sep 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
percolator-launch Ready Ready Preview Sep 2, 2026 6:18pm UTC
percolator-mainnet Ready Ready Preview Sep 2, 2026 6:18pm UTC
percolator-playground Ready Ready Preview Sep 2, 2026 6:18pm UTC

Request Review

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: f955bf8b-11e7-4b4b-8bcb-69d8240e1315

📥 Commits

Reviewing files that changed from the base of the PR and between 91dfff1 and 31deeb3.

📒 Files selected for processing (5)
  • app/__tests__/lib/formatters-nan-guards.test.ts
  • app/components/dashboard/Watchlist.tsx
  • app/components/earn/DepositWithdrawPanel.tsx
  • app/components/trade/MarketInfoBar.tsx
  • app/lib/formatters.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds shared guarded formatters for USD and percentage values. Watchlist, MarketInfoBar, and DepositWithdrawPanel use them. Tests cover non-finite inputs, nullish values, magnitude branches, precision options, and zero-over-zero calculations.

Changes

Formatter guard consolidation

Layer / File(s) Summary
Add guarded shared formatters
app/lib/formatters.ts
Adds formatCompactUsd and formatPercent. Both return for invalid numeric inputs.
Adopt shared display formatters
app/components/dashboard/Watchlist.tsx, app/components/trade/MarketInfoBar.tsx, app/components/earn/DepositWithdrawPanel.tsx
Replaces local or inline formatting with the shared formatters.
Validate formatter edge cases
app/__tests__/lib/formatters-nan-guards.test.ts
Tests non-finite values, nullish USD inputs, real-value formatting branches, precision options, and the 0 / 0 share-preview case.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 31dee

The change prevents invalid numeric values from rendering as NaN and consolidates duplicated display formatting without altering security, data, deployment, or runtime behavior. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The changes address several NaN display gaps in Watchlist, MarketInfoBar, and DepositWithdrawPanel [#2313]. However, the linked issue also requires replacing all four vulnerable formatter copies and g… Update all affected Earn components and formatter copies required by issue #2313, or provide evidence that each unchanged location already has equivalent finite-value guards. Ensure .toFixed() calls and AnimatedNumber value props cannot…
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: guarding display formatters against literal NaN output.
Out of Scope Changes check ✅ Passed The formatter consolidation, component updates, and regression tests directly support the NaN-guarding objectives in issue #2313. No unrelated changes are shown.
Full details: Linked Issues check

Explanation

The changes address several NaN display gaps in Watchlist, MarketInfoBar, and DepositWithdrawPanel [#2313]. However, the linked issue also requires replacing all four vulnerable formatter copies and guarding the listed Earn display boundaries, which this changeset does not demonstrate.

Resolution

Update all affected Earn components and formatter copies required by issue #2313, or provide evidence that each unchanged location already has equivalent finite-value guards. Ensure .toFixed() calls and AnimatedNumber value props cannot receive non-finite values.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/numeric-display-hygiene

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant