Skip to content

fix(DataUtils): improve isPercent validation to exclude invalid formats#7178

Merged
PavelVanecek merged 1 commit into
recharts:mainfrom
vamsi2246:fix/is-percent-validation
Mar 29, 2026
Merged

fix(DataUtils): improve isPercent validation to exclude invalid formats#7178
PavelVanecek merged 1 commit into
recharts:mainfrom
vamsi2246:fix/is-percent-validation

Conversation

@vamsi2246

@vamsi2246 vamsi2246 commented Mar 28, 2026

Copy link
Copy Markdown
Contributor

The isPercent utility function was incorrectly identifying empty strings "" and single percent signs "%" as valid percentages. This resulted in NaN values during chart calculations in getPercentValue.\n\nThis PR updates isPercent to require a length of at least 2 and adds regression tests.\n\nVerified with npm run test -- test/util/DataUtils.spec.ts.

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced percentage string validation to correctly reject empty values and standalone percent symbols that were previously accepted.

@coderabbitai

coderabbitai Bot commented Mar 28, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The isPercent validation function now requires at least one character before the percent sign (value.length > 1), rejecting strings like '%' alone. A JSDoc comment documenting the validation rules was added, and test cases were updated to reflect this stricter behavior.

Changes

Cohort / File(s) Summary
Validation logic tightening
src/util/DataUtils.ts, test/util/DataUtils.spec.ts
Tightened isPercent to reject strings with only a percent sign by requiring value.length > 1. Added JSDoc documentation and updated test cases for '' and '%' inputs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: improving isPercent validation to exclude invalid formats, which is the core objective of the PR.
Description check ✅ Passed The description addresses the problem, solution, and testing verification, but lacks several template sections including Related Issue, Motivation/Context details, and Types of changes checklist.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/util/DataUtils.ts (1)

21-27: Remember to update generated API docs after JSDoc changes.

Please run the omnidoc generation step and include resulting doc updates if any files change.

As per coding guidelines src/**/*.{ts,tsx}: “Modify JSDoc comments and TypeScript definitions in src folder files, then run npm run omnidoc to update generated API documentation files”.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/util/DataUtils.ts` around lines 21 - 27, You updated the JSDoc for the
percent-checking function in DataUtils.ts but didn't regenerate API docs; run
the doc-generation step (npm run omnidoc) after modifying JSDoc and add any
changed generated files to the PR so the API docs stay in sync — specifically
ensure the updated comment for the percent-checking function in DataUtils.ts is
reflected in the generated docs and commit those generated doc file changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/util/DataUtils.ts`:
- Line 29: The JSDoc for isPercent in DataUtils.ts was updated but the generated
API docs were not regenerated; run the docs generator (npm run omnidoc) to
update the generated documentation after modifying JSDoc for isPercent, commit
the changed output artefacts, and ensure the regenerated files (from omnidoc)
are included in the PR so the public API docs match the source comments.

---

Nitpick comments:
In `@src/util/DataUtils.ts`:
- Around line 21-27: You updated the JSDoc for the percent-checking function in
DataUtils.ts but didn't regenerate API docs; run the doc-generation step (npm
run omnidoc) after modifying JSDoc and add any changed generated files to the PR
so the API docs stay in sync — specifically ensure the updated comment for the
percent-checking function in DataUtils.ts is reflected in the generated docs and
commit those generated doc file changes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 473fd8d4-d8d0-483d-8b81-a8852f4b9b9a

📥 Commits

Reviewing files that changed from the base of the PR and between 39d0f27 and 6c1140c.

📒 Files selected for processing (2)
  • src/util/DataUtils.ts
  • test/util/DataUtils.spec.ts

Comment thread src/util/DataUtils.ts
*/
export const isPercent = (value: string | number | undefined): value is Percent =>
typeof value === 'string' && value.indexOf('%') === value.length - 1;
typeof value === 'string' && value.length > 1 && value.indexOf('%') === value.length - 1;

@coderabbitai coderabbitai Bot Mar 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 1) Locate ternary patterns where isPercent controls Number(...) fallback
ast-grep --pattern 'isPercent($VAL) ? $A : Number($VAL)'

# 2) Inspect all isPercent call sites with context
rg -n -C3 '\bisPercent\s*\(' src --type ts --type tsx

# 3) Check for regression tests around bare "%" in related suites
rg -n -C2 "'%'" test --type ts --type tsx

Repository: recharts/recharts

Length of output: 417


🏁 Script executed:

#!/bin/bash
# Read the isPercent function and its context
echo "=== isPercent function in DataUtils.ts ==="
sed -n '21,35p' src/util/DataUtils.ts

echo -e "\n=== Call sites in responsiveContainerUtils.ts ==="
sed -n '35,50p' src/component/responsiveContainerUtils.ts

echo -e "\n=== Check for tests covering isPercent with bare '%' ==="
find test -name "*.ts" -o -name "*.tsx" | head -5
rg "isPercent|bare|%'" test 2>/dev/null | head -20

echo -e "\n=== Search for NaN handling in responsiveContainerUtils ==="
sed -n '1,100p' src/component/responsiveContainerUtils.ts | grep -A5 -B5 "NaN\|isNaN"

Repository: recharts/recharts

Length of output: 3244


🏁 Script executed:

#!/bin/bash
# Check omnidoc related files and workflow
echo "=== Checking for omnidoc in package.json ==="
rg "omnidoc" . --type json 2>/dev/null | head -5

echo -e "\n=== Look for generated documentation ==="
find . -name "*omnidoc*" -o -name "*API*" 2>/dev/null | grep -v node_modules | head -10

echo -e "\n=== Check DataUtils exports and JSDoc ==="
head -50 src/util/DataUtils.ts

Repository: recharts/recharts

Length of output: 2235


🏁 Script executed:

#!/bin/bash
# Check if omnidoc was run / if documentation files were updated
echo "=== Check if omnidoc test passes ==="
find . -path ./node_modules -prune -o -name "omnidoc.spec.ts" -type f -print

echo -e "\n=== Look for responsiveContainerUtils test file ==="
find test -name "*responsiveContainer*" -o -name "*ResponsiveContainer*"

echo -e "\n=== Check git status for omnidoc artifacts ==="
git log --oneline -5 2>/dev/null | head -3
git diff --name-only 2>/dev/null | grep -i omnidoc || echo "No omnidoc files in diff"

echo -e "\n=== Search for test coverage of responsiveContainerUtils with edge cases ==="
rg -n "calculatedWidth|calculatedHeight|NaN" test --type ts --type tsx -A2 -B2

Repository: recharts/recharts

Length of output: 544


🏁 Script executed:

#!/bin/bash
# Final check: does the PR show omnidoc changes or are they expected on build?
echo "=== Check API documentation changes ==="
git diff HEAD -- omnidoc/*.ts 2>/dev/null | head -50
git diff HEAD -- www/ 2>/dev/null | head -50
echo "No diff shown - checking if docs directory exists"

echo -e "\n=== Verify isPercent is exported and would be in omnidoc ==="
rg "export.*isPercent|isPercent.*export" src/util/DataUtils.ts

Repository: recharts/recharts

Length of output: 295


🏁 Script executed:

#!/bin/bash
# Check test coverage in responsiveContainerUtils.spec.ts
echo "=== responsiveContainerUtils.spec.ts full content ==="
cat test/component/responsiveContainerUtils.spec.ts

echo -e "\n=== Check for edge case tests ==="
rg -n "bare|'%'|NaN|edge|invalid" test/component/responsiveContainerUtils.spec.ts

Repository: recharts/recharts

Length of output: 6009


Run npm run omnidoc to update generated API documentation.

The JSDoc changes to isPercent are accurate, and the updated validation (requiring length > 1 and trailing %) is properly tested in test/util/DataUtils.spec.ts, including the case where bare '%' returns false. Downstream code in responsiveContainerUtils.ts handles the NaN from Number('%') gracefully through falsy checks (if (calculatedWidth)).

However, per coding guidelines, JSDoc modifications in the src folder require running npm run omnidoc to update generated API documentation. This step is missing from the PR.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/util/DataUtils.ts` at line 29, The JSDoc for isPercent in DataUtils.ts
was updated but the generated API docs were not regenerated; run the docs
generator (npm run omnidoc) to update the generated documentation after
modifying JSDoc for isPercent, commit the changed output artefacts, and ensure
the regenerated files (from omnidoc) are included in the PR so the public API
docs match the source comments.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omnidoc now runs on every build automatically. Which instructions say it's manual?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

Good question — let me check the current build scripts and CI configuration to verify.


🏁 Script executed:

#!/bin/bash
echo "=== package.json scripts related to omnidoc/build ==="
rg -n "omnidoc|build" package.json | head -30

echo -e "\n=== CI/CD workflow files ==="
find .github/workflows -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -10

echo -e "\n=== Check if omnidoc is part of a prebuild or build script ==="
cat package.json | python3 -c "import json,sys; d=json.load(sys.stdin); [print(k,':',v) for k,v in d.get('scripts',{}).items()]"

echo -e "\n=== Check workflow files for omnidoc ==="
grep -rn "omnidoc" .github/ 2>/dev/null | head -20

Length of output: 4325


@PavelVanecek, you're correct — my comment was wrong. The package.json has "prebuild": "npm run omnidoc", so omnidoc runs automatically before every npm run build. CI also validates this via the test-omnidoc job in the workflow. No manual step is needed, and the review comment should be disregarded.

Apologies for the noise — this was based on an outdated learning I had stored. I'll update it now.


✏️ Learnings added
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 7178
File: src/util/DataUtils.ts:29-29
Timestamp: 2026-03-29T05:24:31.762Z
Learning: In recharts/recharts, omnidoc runs automatically via the `prebuild` npm hook (`"prebuild": "npm run omnidoc"` in package.json), so it executes before every `npm run build`. There is no need to manually run `npm run omnidoc` after modifying JSDoc comments in `src` files — the CI `omnidoc` job in `.github/workflows/ci.yml` validates the generated docs automatically.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


⛔ Learnings removed
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2026-02-26T22:44:25.602Z
Learning: Applies to src/**/*.{ts,tsx} : Modify JSDoc comments and TypeScript definitions in `src` folder files, then run `npm run omnidoc` to update generated API documentation files

🧠 Learnings used
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2026-02-26T22:44:25.602Z
Learning: Applies to src/**/*.{ts,tsx} : Modify JSDoc comments and TypeScript definitions in `src` folder files, then run `npm run omnidoc` to update generated API documentation files

Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-01-31T13:38:06.811Z
Learning: Applies to src/**/*.{ts,tsx} : Update TypeScript interfaces and JSDoc comments in source code to automatically update API documentation

@codecov

codecov Bot commented Mar 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.68%. Comparing base (ad7e3f3) to head (6c1140c).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7178   +/-   ##
=======================================
  Coverage   89.68%   89.68%           
=======================================
  Files         537      537           
  Lines       40674    40677    +3     
  Branches     5547     5550    +3     
=======================================
+ Hits        36477    36480    +3     
  Misses       4189     4189           
  Partials        8        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codecov

codecov Bot commented Mar 28, 2026

Copy link
Copy Markdown

Bundle Report

Changes will increase total bundle size by 1.18kB (0.02%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
recharts/bundle-cjs 1.3MB 293 bytes (0.02%) ⬆️
recharts/bundle-es6 1.13MB 293 bytes (0.03%) ⬆️
recharts/bundle-umd 547.78kB 12 bytes (0.0%) ⬆️
recharts/bundle-treeshaking-cartesian 643.93kB 293 bytes (0.05%) ⬆️
recharts/bundle-treeshaking-polar 448.36kB 293 bytes (0.07%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: recharts/bundle-es6

Assets Changed:

Asset Name Size Change Total Size Change (%)
util/DataUtils.js 293 bytes 5.44kB 5.7% ⚠️
view changes for bundle: recharts/bundle-treeshaking-polar

Assets Changed:

Asset Name Size Change Total Size Change (%)
bundle.js 293 bytes 448.36kB 0.07%
view changes for bundle: recharts/bundle-umd

Assets Changed:

Asset Name Size Change Total Size Change (%)
Recharts.js 12 bytes 547.78kB 0.0%
view changes for bundle: recharts/bundle-treeshaking-cartesian

Assets Changed:

Asset Name Size Change Total Size Change (%)
bundle.js 293 bytes 643.93kB 0.05%
view changes for bundle: recharts/bundle-cjs

Assets Changed:

Asset Name Size Change Total Size Change (%)
util/DataUtils.js 293 bytes 6.3kB 4.88%

@PavelVanecek
PavelVanecek merged commit fc96239 into recharts:main Mar 29, 2026
52 checks passed
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.

2 participants