Omnidoc: Fix default props docs for Bar, Brush, ReferenceLine, Text, Treemap#6640
Omnidoc: Fix default props docs for Bar, Brush, ReferenceLine, Text, Treemap#6640
Conversation
WalkthroughThe changes introduce an 'auto' mode for animation activation props that auto-detects SSR state (defaulting to disabled during SSR), export default prop configurations for Bar/Brush/Treemap components, add new public props (angle to Text, leaveTimeOut/alwaysShowText to Brush), extend prop type definitions across animation and component modules, update Storybook story argTypes and prop metadata with defaultValue annotations, and remove component filtering from omnidoc tests to cover all public components. Changes
Sequence DiagramsequenceDiagram
participant Component
participant Animation Layer
participant Global
Note over Component,Global: When isActive prop is 'auto'
Component->>Animation Layer: pass isActiveProp='auto'
Animation Layer->>Global: query Global.isSsr
Global-->>Animation Layer: SSR state (true/false)
Animation Layer->>Animation Layer: compute isActive = !Global.isSsr
Animation Layer->>Animation Layer: proceed with animation if isActive=true
Note over Component,Global: When isActive prop is boolean
Component->>Animation Layer: pass isActiveProp=boolean value
Animation Layer->>Animation Layer: use isActiveProp directly as isActive
Animation Layer->>Animation Layer: proceed based on provided boolean
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
www/src/docs/api/Brush.ts (3)
41-49: Contradiction:isOptional: falsewith a default value.The
heightproperty is marked asisOptional: false(line 44) but has adefaultVal: 40(line 43). If a property has a default value, it should typically be optional since users don't need to explicitly provide it.Consider changing to:
- isOptional: false, + isOptional: true,
51-59: Contradiction:isOptional: falsewith a default value.The
travellerWidthproperty is marked asisOptional: false(line 54) but has adefaultVal: 5(line 53). Properties with default values should be marked as optional.Consider changing to:
- isOptional: false, + isOptional: true,
1-111: Missing documentation for new properties mentioned in AI summary.According to the AI summary, two new properties were added to BrushProps:
leaveTimeOut?: numberandalwaysShowText?: boolean. However, these properties are not documented in this API documentation file.If these properties were added to the Brush component, they should also be added to the documentation array. Consider adding entries like:
{ name: 'leaveTimeOut', type: 'Number', defaultVal: /* appropriate default */, isOptional: true, desc: { 'en-US': /* description */, 'zh-CN': /* description */, }, }, { name: 'alwaysShowText', type: 'Boolean', defaultVal: /* appropriate default */, isOptional: true, desc: { 'en-US': /* description */, 'zh-CN': /* description */, }, }
🧹 Nitpick comments (1)
www/src/docs/api/TextAPI.ts (1)
16-25: Improved type consistency forangle, butwidthremains inconsistent.The change from string
'null'to number0correctly aligns the default value with the'Number'type declaration.However, the
widthproperty on line 29 still uses the string'null'as itsdefaultVal. For consistency, consider either:
- Changing
width'sdefaultValtonull(without quotes) if null is the actual default, or- Using a numeric default if applicable
Apply this diff to align
widthwith the same pattern:- defaultVal: 'null', + defaultVal: null,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
omnidoc/componentsAndDefaultPropsMap.ts(1 hunks)omnidoc/omnidoc.spec.ts(2 hunks)src/animation/CSSTransitionAnimate.tsx(3 hunks)src/animation/JavascriptAnimate.tsx(2 hunks)src/cartesian/Bar.tsx(3 hunks)src/cartesian/Brush.tsx(3 hunks)src/chart/Treemap.tsx(2 hunks)src/component/Text.tsx(2 hunks)storybook/stories/API/cartesian/Bar.stories.tsx(7 hunks)storybook/stories/API/cartesian/ReferenceLine.stories.tsx(1 hunks)storybook/stories/API/chart/Treemap.stories.tsx(4 hunks)storybook/stories/API/component/Text.stories.tsx(1 hunks)storybook/stories/API/props/RectangleProps.ts(1 hunks)storybook/stories/API/props/ReferenceComponentShared.ts(1 hunks)storybook/stories/API/props/TextProps.ts(2 hunks)www/src/docs/api/Bar.ts(8 hunks)www/src/docs/api/Brush.ts(3 hunks)www/src/docs/api/ReferenceLine.ts(2 hunks)www/src/docs/api/TextAPI.ts(2 hunks)www/src/docs/api/Treemap.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/cartesian/Bar.tsx (6)
src/util/ChartUtils.ts (1)
StackId(443-443)src/util/types.ts (5)
DataKey(59-59)TooltipType(80-80)LegendType(68-79)ActiveShape(1038-1043)AnimationDuration(604-604)src/util/BarUtils.tsx (1)
MinPointSize(62-62)src/zIndex/ZIndexLayer.tsx (1)
ZIndexable(14-23)src/animation/easing.ts (1)
EasingInput(144-144)src/component/LabelList.tsx (1)
ImplicitLabelListType(85-85)
omnidoc/componentsAndDefaultPropsMap.ts (3)
src/cartesian/Bar.tsx (1)
defaultBarProps(702-716)src/cartesian/Brush.tsx (1)
defaultBrushProps(999-1008)src/chart/Treemap.tsx (1)
defaultTreeMapProps(414-424)
src/animation/CSSTransitionAnimate.tsx (1)
src/animation/AnimationManager.ts (2)
ReactSmoothStyle(4-4)AnimationManager(21-26)
storybook/stories/API/component/Text.stories.tsx (1)
storybook/stories/API/props/TextProps.ts (1)
TextProps(3-61)
⏰ 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 (34)
www/src/docs/api/TextAPI.ts (1)
6-15: Improved type consistency, but verify theisOptionalflag.The change from string
'false'to booleanfalsecorrectly aligns the default value with the declared type. However,isOptional: falseappears inconsistent—if a prop has a default value, it's typically optional for the consumer.Please verify whether
scaleToFitis truly required or ifisOptionalshould betrue.www/src/docs/api/Brush.ts (1)
61-70: LGTM! Correctly configured optional property with default.The
gapproperty correctly hasisOptional: truewithdefaultVal: 1, which is the expected pattern for properties that have default values.storybook/stories/API/props/ReferenceComponentShared.ts (1)
18-18: LGTM! Default value documentation adds clarity for axis IDs.Both
defaultValue: 0additions forxAxisIdandyAxisIdare accurate. Recharts YAxis defaults yAxisId to 0, and the implementation confirms the same for xAxisId at line 133 insrc/cartesian/XAxis.tsx.src/component/Text.tsx (1)
88-91: LGTM! Default value properly documented.The
@defaultValueannotation correctly documents the angle prop's default of 0, which matches the implementation intextDefaultPropsat line 355.storybook/stories/API/props/RectangleProps.ts (1)
20-24: LGTM! Documentation updated for 'auto' mode.The defaultValue correctly documents the new 'auto' mode for
isUpdateAnimationActive, which aligns with the PR-wide pattern of SSR-aware animation activation.src/animation/JavascriptAnimate.tsx (3)
9-9: LGTM! Global import added for SSR detection.The import is used correctly at line 54 to implement SSR-aware auto mode for animations.
17-17: LGTM! Type extended to support 'auto' mode.The
isActiveprop now acceptsboolean | 'auto', enabling SSR-aware animation activation.
43-54: LGTM! SSR-aware 'auto' mode implemented correctly.When
isActiveis 'auto', animations are automatically disabled during SSR (Global.isSsr === true) and enabled on the client. This provides a sensible default that avoids hydration mismatches.storybook/stories/API/props/TextProps.ts (2)
4-14: LGTM! Default values documented correctly.The
defaultValueadditions forlineHeight('1em') andbreakAll(false) accurately reflect the values intextDefaultPropsfrom src/component/Text.tsx.
29-35: LGTM! Angle default documented.The
defaultValue: 0correctly documents the angle prop's default, matching the implementation in Text.tsx line 355.storybook/stories/API/component/Text.stories.tsx (1)
6-9: LGTM! Cleaner argTypes reference.Removing the object spread and directly referencing
TextPropsis more concise and maintainable.src/chart/Treemap.tsx (2)
390-392: LGTM! Animation props extended to support 'auto' mode.The type changes enable SSR-aware animation activation, aligning with the PR-wide pattern.
414-424: Code changes verified as correct.The verification confirms that
CSSTransitionAnimateproperly accepts and handles 'auto' mode. The component'sisActiveprop is typed asboolean | 'auto'(line 15 of CSSTransitionAnimate.tsx), and the implementation explicitly handles 'auto' by converting it to a boolean based on SSR status (line 48). ThedefaultTreeMapPropscorrectly assigns'auto'to bothisAnimationActiveandisUpdateAnimationActive, which match the type definitions in the Props interface. The types are compatible and properly constrained withas const satisfies Partial<Props>.omnidoc/componentsAndDefaultPropsMap.ts (2)
5-7: LGTM! Imports added for new default props.The imports correctly reference the exported default props from Bar, Brush, and Treemap components.
13-20: LGTM! Component metadata map extended.The entries for Bar, Brush, and Treemap properly link their default props, enabling omnidoc to generate accurate documentation.
storybook/stories/API/cartesian/ReferenceLine.stories.tsx (1)
56-61: No issues found – default value is accurate.Verification confirms the
positionprop default value in the ReferenceLine component implementation (line 237 in src/cartesian/ReferenceLine.tsx) is'middle', which matches the Storybook documentation.omnidoc/omnidoc.spec.ts (1)
137-173: LGTM! Expanded test coverage for all public components.Removing the component filters ensures that all public API and Storybook components are now validated for default prop consistency. This aligns well with the PR objective of documenting default props for previously excluded components (ReferenceLine, Text, Treemap).
www/src/docs/api/Treemap.ts (3)
55-63: LGTM! 'auto' mode enables SSR-aware animation.The new
boolean | "auto"type and'auto'default allow the component to automatically detect SSR environments and disable animations accordingly. This is a good enhancement for SSR compatibility.
47-47: Verified: aspectRatio default value matches implementation.The documentation correctly reflects the implementation. The formula
0.5 * (1 + Math.sqrt(5))at line 415 ofsrc/chart/Treemap.tsxcomputes to the golden ratio (≈1.618033988749895), which matches the documented value.
85-93: The review comment is based on incorrect assumptions about this change.This commit does not change the animation easing behavior. The implementation default in
src/chart/Treemap.tsxwas already'linear'and remains unchanged. The diff shows only a documentation correction inwww/src/docs/api/Treemap.ts, where the incorrect default value"'ease'"is being fixed to the actual implementation default of'linear'. Existing users are unaffected—this is a documentation fix to align with the already-implemented behavior.Likely an incorrect or invalid review comment.
storybook/stories/API/chart/Treemap.stories.tsx (1)
37-88: LGTM! Storybook argTypes now document default values.The changes properly document default values for:
nameKey: 'name'aspectRatio: 1.618033988749895 (golden ratio)animationEasing: 'linear'dataKey: 'value'This improves the developer experience in Storybook by showing accurate defaults.
src/cartesian/Brush.tsx (2)
47-78: LGTM! New props properly documented with JSDoc defaults.The addition of
leaveTimeOutandalwaysShowTextprops with proper@defaultValueannotations helps with:
- Type checking and IDE support
- Documentation generation
- Runtime default resolution
Both props are properly integrated into the component logic (lines 640 and 911).
999-1008: LGTM! Exporting defaultBrushProps improves API consistency.Exporting
defaultBrushPropsas a public constant:
- Allows external code to reference canonical defaults
- Improves consistency with other components (Bar, Treemap)
- Uses
as const satisfies Partial<Props>for type safetyThe defaults are comprehensive and include the newly added props.
www/src/docs/api/Bar.ts (4)
16-16: LGTM! Numeric axis ID defaults are now documented.Documenting
xAxisIdandyAxisIddefaults as0(numeric) improves clarity and aligns with typical usage patterns.Also applies to: 26-26
36-46: LGTM! Additional prop defaults are now documented.Documenting defaults for:
legendType: 'rect'label: falseminPointSize: 0background: falseactiveBar: falseThis improves the API documentation completeness.
Also applies to: 83-83, 102-102, 138-138
208-216: LGTM! Bar component adopts 'auto' SSR-aware animation mode.The
isAnimationActivedefault of'auto'enables automatic SSR detection, consistent with other components in this PR (Treemap, CSSTransitionAnimate).
228-236: Confirm whether 400ms is the intended Bar-specific default.Bar's 400ms animationDuration default differs significantly from other components (e.g., Treemap, Radar both use 1500ms). While the 400ms value is consistently documented and implemented in the code, please confirm this is intentional Bar-specific behavior rather than an oversight.
storybook/stories/API/cartesian/Bar.stories.tsx (2)
363-369: Nice catch! Typo fixed from "mergered" to "merged".
369-448: LGTM! Storybook argTypes now properly document Bar defaults.The additions document default values for:
background: falselegendType: 'rect'activeBar: falselabel: falseminPointSize: 0animationDuration: 400This aligns with the API documentation and improves the Storybook developer experience.
www/src/docs/api/ReferenceLine.ts (1)
7-7: LGTM! Axis ID defaults corrected to numeric type.Verification confirms the ReferenceLine implementation correctly uses numeric defaults (
xAxisId: 0, yAxisId: 0at lines 294–295 insrc/cartesian/ReferenceLine.tsx). Type consistency is improved with numeric values, aligning with the Bar component's documentation.src/animation/CSSTransitionAnimate.tsx (1)
8-8: Verification confirms implementation is correct.The
Global.isSsrinitialization properly detects SSR environments:
parseIsSsrByDefault()checks for browser APIs (window,window.document, etc.) and returnstrueif any check fails, indicating an SSR environment- In
CSSTransitionAnimate.tsxat line 53, the 'auto' mode correctly uses!Global.isSsr: disabling animations in SSR (whereGlobal.isSsr === true) and enabling them on the client (whereGlobal.isSsr === false)- The implementation maintains backward compatibility by falling back to the provided boolean value when
isActivePropis not 'auto'All locations mentioned (8, 16, 43, 53-54) are correct and the SSR-aware implementation is sound.
src/cartesian/Bar.tsx (3)
206-206: LGTM - Type consistency maintained.The
isAnimationActivetype is correctly updated to match the publicBarPropsinterface change, maintaining type consistency between public and internal types.
702-716: Good practice: Exporting default props for better visibility.Exporting
defaultBarPropsimproves documentation and allows external consumers to reference the canonical defaults. The use ofas const satisfies Partial<Props>ensures type safety while preserving literal types.The additions of
background,label, andzIndexdefaults complete the prop documentation and provide clear default behavior.
114-175: zIndex default value verified—documentation is accurate.Confirmed that
DefaultZIndexes.barequals 300, matching the JSDoc@defaultValue 300on lines 172-175. No documentation mismatch exists.
| /** | ||
| * @defaultValue auto | ||
| */ | ||
| isAnimationActive?: boolean | 'auto'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify 'auto' mode is properly handled in animation system.
The type change from boolean to boolean | 'auto' for isAnimationActive introduces a new mode that auto-detects SSR state. Ensure that:
- The
JavascriptAnimatecomponent (line 590) properly handles the'auto'value - The SSR detection logic is implemented and tested
- This change is documented in the changelog or migration guide as it affects the public API
Run the following script to verify JavascriptAnimate handles the 'auto' value:
🏁 Script executed:
#!/bin/bash
# Description: Verify JavascriptAnimate component handles 'auto' value for isActive prop
# Find the JavascriptAnimate implementation
ast-grep --pattern $'export $_ JavascriptAnimate($_) {
$$$
}'
# Also search for type definitions of the isActive prop
rg -nP 'isActive.*\?.*:.*' --type=ts -g '**/JavascriptAnimate.ts*' -C 3Length of output: 1236
I'll continue verification to ensure the SSR detection logic is properly implemented and tested.
Now I'll search the codebase directly for Global definition, tests, and changelog entries.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find Global definition and isSsr implementation
rg -nP 'export.*Global|const.*Global|type.*Global' --type=ts --type=tsx -C 3
# Search for where isSsr is used/defined
rg -nP 'isSsr' --type=ts --type=tsx -C 2 | head -50
# Find test files for JavascriptAnimate
fd -e 'spec\|test' JavascriptAnimate
Length of output: 55
---
🏁 Script executed:
```shell
#!/bin/bash
# Find Global definition and isSsr implementation (fix file types)
rg -nP 'export.*Global|const.*Global|type.*Global' -C 3
# Search for where isSsr is used/defined
rg -nP 'isSsr' -C 2 | head -60
# Find test files for JavascriptAnimate
fd 'JavascriptAnimate.*\.(test|spec)\.(ts|tsx|js)$'
# Also search for CHANGELOG
fd 'CHANGE.*' -type f | head -20
Length of output: 7616
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the JavascriptAnimate test files for 'auto' mode coverage
cat test/animation/JavascriptAnimate.progress.spec.tsx | grep -i 'auto\|isActive'
# Also check the timing spec
cat test/animation/JavascriptAnimate.timing.spec.tsx | grep -i 'auto\|isActive'
# Search for CHANGELOG entries
find . -iname '*changelog*' -o -iname '*changelog*' -o -iname '*release*' | grep -E '\.(md|txt)$' | head -5
# Also look for documentation about this change
rg -i 'boolean.*auto|auto.*boolean' --type=md -C 2
Length of output: 768
---
🏁 Script executed:
```shell
#!/bin/bash
# Read full test files to check for 'auto' mode tests
echo "=== JavascriptAnimate.progress.spec.tsx ==="
wc -l test/animation/JavascriptAnimate.progress.spec.tsx
rg "'auto'" test/animation/JavascriptAnimate.progress.spec.tsx
echo "=== JavascriptAnimate.timing.spec.tsx ==="
wc -l test/animation/JavascriptAnimate.timing.spec.tsx
rg "'auto'" test/animation/JavascriptAnimate.timing.spec.tsx
# Check CHANGELOG for relevant entries
echo "=== CHANGELOG entries about isAnimationActive or auto mode ==="
rg -i 'isAnimationActive|animation.*auto|auto.*animation' CHANGELOG.md -C 2 | head -40
# Verify Bar.tsx at the specified lines
echo "=== Bar.tsx lines 150-165 ==="
sed -n '150,165p' src/cartesian/Bar.tsx
Length of output: 1256
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any tests covering 'auto' mode across all animation tests
echo "=== Searching for 'auto' mode in all test files ==="
rg "'auto'" test/animation/ -C 2
# Check for migration guide or docs
echo "=== Looking for migration or upgrade guides ==="
fd -i 'migrat|upgrade|guide' . -type f | grep -E '\.(md|rst|txt)$'
# Verify how isAnimationActive is used in Bar component
echo "=== Bar.tsx usage of isAnimationActive ==="
rg 'isAnimationActive' src/cartesian/Bar.tsx -C 2
Length of output: 1244
Add test coverage and documentation for 'auto' mode.
Verification confirms the implementation is correct: JavascriptAnimate and CSSTransitionAnimate properly handle the 'auto' value by detecting SSR state via Global.isSsr. However:
- Missing test coverage: No tests found for the
'auto'mode across animation test files—only boolean modes are tested - Missing CHANGELOG entry: No documentation of this new feature in CHANGELOG.md
Add tests covering the 'auto' mode behavior in JavascriptAnimate (verify it resolves to false when Global.isSsr is true, and true otherwise), and document this feature in CHANGELOG.md.
🤖 Prompt for AI Agents
In src/cartesian/Bar.tsx around lines 156-159, the new isAnimationActive type
supports 'auto' but there are no tests or docs; add unit tests for
JavascriptAnimate to cover the 'auto' mode by toggling Global.isSsr: set
Global.isSsr = true and assert the animation resolves/returns false, set
Global.isSsr = false and assert it resolves/returns true (place tests alongside
existing animation tests, e.g., tests/animate/JavascriptAnimate.test.tsx, and
restore Global.isSsr after each test), and update CHANGELOG.md with a short
entry under the current/unreleased section describing that isAnimationActive
accepts 'auto' which detects SSR via Global.isSsr and enables/disables
animations accordingly.
There was a problem hiding this comment.
We don't actually support SSR yet, the chart doesn't render at all in SSR so there is not much to test. I will add that later once we add SSR support.
There was a problem hiding this comment.
@PavelVanecek, understood! That makes perfect sense—there's no point in testing SSR-specific behavior when the chart doesn't render in SSR environments yet. The 'auto' mode is preparatory work for when SSR support is added, and testing it then will be more meaningful and practical. Thanks for clarifying the context!
✏️ Learnings added
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6640
File: src/cartesian/Bar.tsx:156-159
Timestamp: 2025-11-16T09:14:24.891Z
Learning: In recharts, SSR (Server-Side Rendering) is not yet supported—charts don't render at all in SSR environments. The `isAnimationActive: 'auto'` mode is preparatory work for future SSR support, so testing of this mode should be deferred until SSR support is actually implemented.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6640 +/- ##
==========================================
- Coverage 94.17% 94.17% -0.01%
==========================================
Files 494 494
Lines 41666 41658 -8
Branches 4823 4824 +1
==========================================
- Hits 39240 39232 -8
Misses 2421 2421
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 673 bytes (0.03%) ⬆️. 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
Bar, Brush, ReferenceLine, Text, Treemap
#6069
Summary by CodeRabbit
New Features
'auto') for smoother animations across environments.Tests
Documentation