fix(DataUtils): improve isPercent validation to exclude invalid formats#7178
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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 insrcfolder files, then runnpm run omnidocto 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
📒 Files selected for processing (2)
src/util/DataUtils.tstest/util/DataUtils.spec.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; |
There was a problem hiding this comment.
🧩 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 tsxRepository: 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.tsRepository: 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 -B2Repository: 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.tsRepository: 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.tsRepository: 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.
There was a problem hiding this comment.
Omnidoc now runs on every build automatically. Which instructions say it's manual?
There was a problem hiding this comment.
🧩 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 Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 1.18kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-treeshaking-polarAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
The
isPercentutility function was incorrectly identifying empty strings""and single percent signs"%"as valid percentages. This resulted inNaNvalues during chart calculations ingetPercentValue.\n\nThis PR updatesisPercentto require a length of at least 2 and adds regression tests.\n\nVerified withnpm run test -- test/util/DataUtils.spec.ts.Summary by CodeRabbit