Conversation
WalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cartesian/Area.tsx (1)
463-499: Guard against non‑numericstrokeWidthwhen computing animation clip rects
VerticalRectandHorizontalRectnow size the animation clip rectangles using:width={maxX + (strokeWidth ? parseInt(`${strokeWidth}`, 10) : 1)} // and height={Math.floor(maxY + (strokeWidth ? parseInt(`${strokeWidth}`, 10) : 1))}If a caller ever passes a non‑numeric
strokeWidthstring (e.g.'inherit','thin'),parseIntwill returnNaN, and you’ll end up withwidth/heightasNaNeven thoughmaxX/maxYare valid numbers. Previously this always added1, so the new behavior is slightly more fragile.To keep the intent (“pad by stroke width when numeric, otherwise fall back to 1”) you could normalize like:
- width={maxX + (strokeWidth ? parseInt(`${strokeWidth}`, 10) : 1)} + const numericStrokeWidth = strokeWidth == null ? NaN : Number.parseFloat(String(strokeWidth)); + const extra = Number.isFinite(numericStrokeWidth) ? numericStrokeWidth : 1; + width={maxX + extra}and similarly for the vertical height path.
This preserves existing behavior for default and typical numeric stroke widths, while avoiding
NaNdimensions for odd inputs.Also applies to: 502-539, 541-559
🧹 Nitpick comments (4)
omnidoc/readApiDoc.ts (1)
68-86: API doc comment lookup looks good; consider a safer locale fallback
getCommentOfcorrectly mirrors how API docs storedescand cleanly returnsundefinedwhen a component or prop isn’t documented. One minor robustness tweak you could consider: whendescis an object anddesc['en-US']is missing, optionally fall back to another available locale (e.g., the first key) instead of returningundefined. This would make the helper more resilient to partial localization without changing the primary “en‑US first” behavior.omnidoc/readStorybookDoc.ts (1)
134-163: Storybook comment retrieval is correct; shared lookup logic could be DRYed up
getCommentOfcorrectly reuses the same component/argTypes discovery pattern asgetDefaultValueOfand returnsargType.descriptionwhen present, otherwiseundefined. That’s exactly what the omnidoc tests expect.There is some duplicated code between
getDefaultValueOfandgetCommentOf(finding the story module, validatingargTypes, extracting theStorybookArg). If you touch this again, consider extracting a small helper likegetStoryArgType(component, prop): StorybookArg | undefinedand have both methods delegate to it.storybook/stories/API/cartesian/Area.stories.tsx (1)
12-61: Area Storybook argTypes consolidation looks consistent with the new Area APIDefining a dedicated
AreaArgTypesand driving bothargTypesandAPI.argsfrom it is a good move:
- The merge order (
AreaSpecificProps→LineStyle→ Area‑specificstroke/dot→AnimationProps→legendType→GeneralProps→data→ResponsiveProps→baseLine) is sane and lets you override shared Line props (e.g.,dot) with Area‑specific defaults.- Using
getStoryArgsFromArgsTypesObject(AreaArgTypes)to seedAPI.argskeeps Storybook defaults aligned with argTypes, while the explicit overrides (e.g.,stroke,dot,activeDot,tooltipType) tailor the demo.The only thing to watch over time is that the duplicated
dot/strokedescriptions here stay in sync with any shared definitions inStyles.ts, but that’s a documentation maintenance concern rather than a correctness issue.Also applies to: 99-113
omnidoc/readProject.ts (1)
371-432: Project comment lookup works; optional improvements for multi-declaration origin bias and error handling clarityThe new
getCommentOf+getJSDocFromDeclarationpipeline is nicely structured, and the ts-morph APIs you're using (Node.isJSDocable,getJsDocs(),getComment()) are confirmed to work correctly for interface properties and type aliases.
- You reuse
getPropMetaand preferorigin === 'recharts', which matches how default values are prioritized.- You first check
@descriptiontags and then fall back to the main JSDoc body, which is reasonable for mixed TSDoc/JSDoc usage.getJSDocFromDeclarationhandles both string and node-array comments defensively.Two optional refinements worth considering:
Multiple declarations (DOM + Recharts)
For props coming from an intersection of Recharts and DOM types,property.getDeclarations()can include both sources. Right now you take the first declaration with a non‑undefined comment, which may pick the DOM JSDoc even whentargetProp.origin === 'recharts'. To keep the "prefer Recharts docs" behavior consistent, you could:const declarations = property.getDeclarations(); const preferred = declarations.find(decl => this.getDeclarationOrigin(decl) === targetProp.origin) ?? declarations[0]; return this.getJSDocFromDeclaration(preferred);(And only fall back to scanning all declarations if
preferredhas no JSDoc.)Error handling
Thetry { ... } catch { return undefined; }makes the method very forgiving, but it will also swallow unexpected ts-morph errors. If this becomes hard to debug, you might want to narrow thetryblock to just the parts that can legitimately throw for unknown components/props, or at least log in non‑test environments.The implementation is sound as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
omnidoc/DocReader.ts(1 hunks)omnidoc/omnidoc.spec.ts(1 hunks)omnidoc/readApiDoc.spec.ts(1 hunks)omnidoc/readApiDoc.ts(1 hunks)omnidoc/readProject.spec.ts(1 hunks)omnidoc/readProject.ts(1 hunks)omnidoc/readStorybookDoc.spec.ts(1 hunks)omnidoc/readStorybookDoc.ts(1 hunks)src/cartesian/Area.tsx(2 hunks)storybook/stories/API/cartesian/Area.stories.tsx(3 hunks)test/cartesian/Area.spec.tsx(7 hunks)test/component/Legend.spec.tsx(1 hunks)test/component/Tooltip/Tooltip.content.spec.tsx(3 hunks)test/component/Tooltip/itemSorter.spec.tsx(17 hunks)www/src/docs/api/Area.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-23T13:30:10.388Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.388Z
Learning: The `TooltipIndex` type from recharts is defined in `src/state/tooltipSlice.ts` but is not currently exported from the public API entry points. It should not be imported from `recharts/types/state/tooltipSlice` as this is an internal implementation path. An ESLint rule is needed to prevent regressions.
Applied to files:
test/component/Tooltip/itemSorter.spec.tsxtest/component/Tooltip/Tooltip.content.spec.tsxstorybook/stories/API/cartesian/Area.stories.tsx
🧬 Code graph analysis (3)
omnidoc/readStorybookDoc.ts (1)
storybook/StorybookArgs.ts (1)
StorybookArg(13-48)
storybook/stories/API/cartesian/Area.stories.tsx (6)
storybook/StorybookArgs.ts (1)
StorybookArgs(59-61)storybook/stories/API/props/Styles.ts (1)
LineStyle(62-136)storybook/stories/API/props/AnimationProps.ts (1)
AnimationProps(43-48)storybook/stories/API/props/Tooltip.ts (1)
ResponsiveProps(30-33)src/cartesian/Area.tsx (1)
Area(1084-1084)storybook/stories/API/props/utils.ts (1)
getStoryArgsFromArgsTypesObject(5-16)
src/cartesian/Area.tsx (6)
src/util/types.ts (8)
ActiveDotType(978-1002)AnimationDuration(605-605)AnimationTiming(603-603)NullableCoordinate(93-96)DataKey(60-60)DotType(1022-1049)LegendType(69-80)TooltipType(81-81)src/state/chartDataSlice.ts (1)
ChartData(12-12)src/component/LabelList.tsx (1)
ImplicitLabelListType(85-85)src/index.ts (1)
LegendType(111-111)src/component/DefaultTooltipContent.tsx (1)
TooltipType(16-16)src/shape/Curve.tsx (1)
CurveType(59-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, Test, Pack
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
test/component/Tooltip/Tooltip.content.spec.tsx (1)
122-122: LGTM! Test expectations updated for new strokeWidth default.The tooltip payload expectations have been correctly updated to include
strokeWidth: 1, aligning with the new default value introduced in this PR.Also applies to: 141-141, 160-160
test/component/Legend.spec.tsx (1)
1878-1878: LGTM! Legend payload test updated correctly.The legend payload expectation now includes
strokeWidth: 1, consistent with the Area component's new default.test/cartesian/Area.spec.tsx (2)
252-252: LGTM! Event handler test expectations updated.The onClick, onMouseOver, onMouseOut, and onTouchMove event handler tests have been correctly updated to expect
strokeWidth: 1in the Area props.Also applies to: 292-292, 315-315, 352-352
798-798: LGTM! State integration test expectations updated.The legend and tooltip payload expectations in the state integration tests now correctly include
strokeWidth: 1.Also applies to: 856-856, 878-878
omnidoc/DocReader.ts (1)
36-41: LGTM! Clean interface extension.The new
getCommentOfmethod provides a clear API for retrieving prop documentation across different sources. The return type appropriately usesundefinedto handle cases where comments are not available.omnidoc/readProject.spec.ts (1)
869-891: LGTM! Comprehensive test coverage for getCommentOf.The test suite covers all essential scenarios:
- Successful comment retrieval with content validation
- Graceful handling of unknown component
- Graceful handling of unknown prop
- Proper behavior when Recharts props shadow DOM props
The test for prop shadowing (Area.stroke) is particularly valuable given Recharts' pattern of extending DOM elements.
www/src/docs/api/Area.ts (1)
134-134: LGTM! Documentation default value corrected.Changing the
strokeWidthdefault from string'1'to number1better aligns with the actual default value in the codebase and matches the numeric type more naturally (though both are valid per the type definition).omnidoc/readStorybookDoc.spec.ts (1)
200-211: LGTM! Consistent test coverage for StorybookDocReader.The tests for
getCommentOffollow the same pattern as the API and Project readers, ensuring consistent behavior across all documentation sources.omnidoc/readApiDoc.spec.ts (1)
87-106: LGTM! Complete test coverage for ApiDocReader.getCommentOf.The tests validate:
- Comment retrieval with content verification via inline snapshot
- Proper undefined return for unknown components
- Proper undefined return for unknown props
The inline snapshot for the
ifOverflowcomment provides useful regression protection.test/component/Tooltip/itemSorter.spec.tsx (1)
80-585: Tooltip itemSorter tests correctly updated for Area strokeWidth defaultsThe added
strokeWidth: 1expectations for Area tooltip payloads/configs are consistent withdefaultAreaProps.strokeWidth = 1and the waySetAreaTooltipEntrySettingsnow forwardsstrokeWidth. Non‑Area series still expectstrokeWidth: undefined, which matches their behavior. No issues from the test changes themselves.Also applies to: 2280-2632
omnidoc/omnidoc.spec.ts (1)
239-505: Comment consistency tests are well‑scoped and match the intended semanticsThe new “comment/documentation consistency” suite is structured sensibly:
normalizeText/areCommentsSimilargive you a predictable comparison (case/whitespace insensitive, but still strict on content).- The three exclusion lists let you progressively tighten consistency without breaking the suite on known offenders.
- Each test only flags mismatches when at least one side has a non‑empty comment, which is exactly what the descriptions promise.
This should give good signal on future doc drift across API docs, Storybook, and project JSDoc without adding brittle coupling.
src/cartesian/Area.tsx (1)
122-275: Area public props, defaults, and tooltip integration are consistentThe expanded
AreaPropsJSDoc, the newstrokeWidth?: string | numberwith@default 1, anddefaultAreaProps.strokeWidth = 1all line up:
AreaFnresolves defaults viaresolveDefaultProps, soprops.strokeWidthis always defined as either the user value or1.SetAreaTooltipEntrySettingsnow includesstrokeWidthin the tooltip configuration, which matches the updated tests and omnidoc expectations.- The doc comments for animation, legendType, connectNulls, dot, id, etc. reflect the actual runtime semantics in
AreaImplandAreaWithAnimation.From a runtime perspective, these changes look coherent and shouldn’t introduce regressions.
Also applies to: 813-832, 1042-1084
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6684 +/- ##
=======================================
Coverage 94.02% 94.02%
=======================================
Files 499 499
Lines 42547 42548 +1
Branches 4873 4873
=======================================
+ Hits 40004 40005 +1
Misses 2538 2538
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 50 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
Description
Adds new capability to omnidoc: checking that prop comments/descriptions are the same.
Also includes fixes for Area for a preview. Other components are excluded for now, I will work on them one by one.
Related Issue
#6069
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.