[Refactor] Split combineConfiguredScale into its own function#6930
Merged
Conversation
Contributor
WalkthroughThis PR refactors the scale handling system by introducing a two-step approach: creating a CustomScaleDefinition configuration via combineConfiguredScale, then materializing it into a RechartsScale via a simplified rechartsScaleFactory. New scale combiners are added, selectors are updated to use this pattern, and test files are adapted accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/state/selectors/combiners/combineRealScaleType.ts`:
- Around line 6-38: The function combineRealScaleType currently builds a
prefixed key (`scale${upperFirst(scale)}`) and returns it, which doesn't match
the D3ScaleType union; instead, validate existence using the prefixed key but
return the original unprefixed scale string (or fall back to 'point').
Concretely: inside combineRealScaleType where you compute `const name =
\`scale\${upperFirst(scale)}\``, call the validator against `name` (keep using
d3Scales and upperFirst) but if it passes return `scale as D3ScaleType`; if it
fails return `'point'`. Also update or replace isSupportedScaleName so it
returns a boolean (not a `name is D3ScaleType` type guard) since it checks
prefixed keys in d3Scales, or add a new boolean helper used only for the
prefixed-key check. Ensure all branches return values assignable to D3ScaleType
| undefined.
Comment on lines
+6
to
+38
| function isSupportedScaleName(name: string): name is D3ScaleType { | ||
| return name in d3Scales; | ||
| } | ||
|
|
||
| export const combineRealScaleType = ( | ||
| axisConfig: BaseCartesianAxis | undefined, | ||
| hasBar: boolean, | ||
| chartType: string, | ||
| ): D3ScaleType | undefined => { | ||
| if (axisConfig == null) { | ||
| return undefined; | ||
| } | ||
| const { scale, type } = axisConfig; | ||
| if (scale === 'auto') { | ||
| if ( | ||
| type === 'category' && | ||
| chartType && | ||
| (chartType.indexOf('LineChart') >= 0 || | ||
| chartType.indexOf('AreaChart') >= 0 || | ||
| (chartType.indexOf('ComposedChart') >= 0 && !hasBar)) | ||
| ) { | ||
| return 'point'; | ||
| } | ||
| if (type === 'category') { | ||
| return 'band'; | ||
| } | ||
|
|
||
| return 'linear'; | ||
| } | ||
| if (typeof scale === 'string') { | ||
| const name = `scale${upperFirst(scale)}`; | ||
|
|
||
| return isSupportedScaleName(name) ? name : 'point'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "combineRealScaleType.ts" | head -20Repository: recharts/recharts
Length of output: 116
🏁 Script executed:
find . -type f \( -name "*.ts" -o -name "*.tsx" \) | grep -i scale | head -20Repository: recharts/recharts
Length of output: 1011
🏁 Script executed:
fd -t f "combineRealScaleType" -o fd -t f "d3Scale" | head -20Repository: recharts/recharts
Length of output: 184
🏁 Script executed:
cat -n ./src/state/selectors/combiners/combineRealScaleType.tsRepository: recharts/recharts
Length of output: 1435
🏁 Script executed:
# Find D3ScaleType definition
rg "type D3ScaleType|interface D3ScaleType" -A 5Repository: recharts/recharts
Length of output: 251
🏁 Script executed:
# Find d3Scales definition
rg "const d3Scales|const.*d3Scales" -A 10Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Search for where d3Scales is used and what it contains
rg "victory-vendor/d3-scale" -A 2Repository: recharts/recharts
Length of output: 3351
🏁 Script executed:
# Get complete D3ScaleType definition
rg "type D3ScaleType" -A 20Repository: recharts/recharts
Length of output: 827
🏁 Script executed:
# Check how combineRealScaleType return value is used downstream
rg "combineRealScaleType" -B 2 -A 5Repository: recharts/recharts
Length of output: 6208
🏁 Script executed:
# Let me check the actual keys in d3Scales to confirm the review is correct
python3 << 'EOF'
# Check what the actual keys would be in d3Scales
d3_scales = {
'scaleLinear': None,
'scaleBand': None,
'scalePoint': None,
'scaleLog': None,
'scalePow': None,
'scaleSqrt': None,
'scaleSymlog': None,
'scaleTime': None,
'scaleUtc': None,
'scaleIdentity': None,
'scaleOrdinal': None,
'scaleQuantile': None,
'scaleQuantize': None,
'scaleSequential': None,
'scaleThreshold': None,
}
d3_scale_type = {
'linear', 'pow', 'sqrt', 'log', 'symlog', 'identity', 'time',
'band', 'point', 'ordinal', 'quantile', 'quantize', 'utc', 'sequential', 'threshold'
}
# Check if scaleLinear is in d3_scale_type
test_value = 'scaleLinear'
print(f"Is '{test_value}' in D3ScaleType? {test_value in d3_scale_type}")
# Verify the mapping
print("\nVerifying scale name mappings:")
for scale_name in d3_scale_type:
prefixed = f'scale{scale_name[0].upper()}{scale_name[1:]}'
is_in_d3 = prefixed in d3_scales
print(f" '{scale_name}' -> '{prefixed}' (in d3Scales: {is_in_d3})")
EOFRepository: recharts/recharts
Length of output: 147
🏁 Script executed:
# Check test file to see how combineRealScaleType is tested
cat ./test/state/selectors/selectRealScaleType.spec.tsxRepository: recharts/recharts
Length of output: 1061
🏁 Script executed:
# Check combineConfiguredScale to see how it uses the scaleType
cat -n ./src/state/selectors/combiners/combineConfiguredScale.ts | head -100Repository: recharts/recharts
Length of output: 4208
🏁 Script executed:
# Search for where the return value of combineRealScaleType is actually used
rg "selectRealScaleType|combineRealScaleType" -B 3 -A 10 | head -80Repository: recharts/recharts
Length of output: 6915
🏁 Script executed:
# Check if there are any direct comparisons or type checks on the return value
rg "realScaleType.*==|realScaleType.*===|realScaleType.*in " -A 2 -B 2Repository: recharts/recharts
Length of output: 2235
Return unprefixed scale types; current logic emits scale* keys that don't match D3ScaleType
combineRealScaleType returns scale${upperFirst(scale)} (e.g., scaleBand) which is not part of the D3ScaleType union ('band' | 'linear' | ...). This causes type inconsistency: the function signature claims it returns D3ScaleType | undefined, but the actual value is a d3-scale key that doesn't match the type. Downstream code already has workarounds checking for both prefixed and unprefixed values, with @ts-expect-error comments acknowledging the mismatch. The type guard is also unsound—it narrows to D3ScaleType but actually validates against d3Scales keys. Validate via the prefixed key but return the original unprefixed scale string.
🔧 Proposed fix
-function isSupportedScaleName(name: string): name is D3ScaleType {
- return name in d3Scales;
-}
+function isSupportedScaleType(scale: string): scale is D3ScaleType {
+ const name = `scale${upperFirst(scale)}`;
+ return name in d3Scales;
+}
...
if (typeof scale === 'string') {
- const name = `scale${upperFirst(scale)}`;
-
- return isSupportedScaleName(name) ? name : 'point';
+ return isSupportedScaleType(scale) ? scale : 'point';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function isSupportedScaleName(name: string): name is D3ScaleType { | |
| return name in d3Scales; | |
| } | |
| export const combineRealScaleType = ( | |
| axisConfig: BaseCartesianAxis | undefined, | |
| hasBar: boolean, | |
| chartType: string, | |
| ): D3ScaleType | undefined => { | |
| if (axisConfig == null) { | |
| return undefined; | |
| } | |
| const { scale, type } = axisConfig; | |
| if (scale === 'auto') { | |
| if ( | |
| type === 'category' && | |
| chartType && | |
| (chartType.indexOf('LineChart') >= 0 || | |
| chartType.indexOf('AreaChart') >= 0 || | |
| (chartType.indexOf('ComposedChart') >= 0 && !hasBar)) | |
| ) { | |
| return 'point'; | |
| } | |
| if (type === 'category') { | |
| return 'band'; | |
| } | |
| return 'linear'; | |
| } | |
| if (typeof scale === 'string') { | |
| const name = `scale${upperFirst(scale)}`; | |
| return isSupportedScaleName(name) ? name : 'point'; | |
| function isSupportedScaleType(scale: string): scale is D3ScaleType { | |
| const name = `scale${upperFirst(scale)}`; | |
| return name in d3Scales; | |
| } | |
| export const combineRealScaleType = ( | |
| axisConfig: BaseCartesianAxis | undefined, | |
| hasBar: boolean, | |
| chartType: string, | |
| ): D3ScaleType | undefined => { | |
| if (axisConfig == null) { | |
| return undefined; | |
| } | |
| const { scale, type } = axisConfig; | |
| if (scale === 'auto') { | |
| if ( | |
| type === 'category' && | |
| chartType && | |
| (chartType.indexOf('LineChart') >= 0 || | |
| chartType.indexOf('AreaChart') >= 0 || | |
| (chartType.indexOf('ComposedChart') >= 0 && !hasBar)) | |
| ) { | |
| return 'point'; | |
| } | |
| if (type === 'category') { | |
| return 'band'; | |
| } | |
| return 'linear'; | |
| } | |
| if (typeof scale === 'string') { | |
| return isSupportedScaleType(scale) ? scale : 'point'; | |
| } |
🤖 Prompt for AI Agents
In `@src/state/selectors/combiners/combineRealScaleType.ts` around lines 6 - 38,
The function combineRealScaleType currently builds a prefixed key
(`scale${upperFirst(scale)}`) and returns it, which doesn't match the
D3ScaleType union; instead, validate existence using the prefixed key but return
the original unprefixed scale string (or fall back to 'point'). Concretely:
inside combineRealScaleType where you compute `const name =
\`scale\${upperFirst(scale)}\``, call the validator against `name` (keep using
d3Scales and upperFirst) but if it passes return `scale as D3ScaleType`; if it
fails return `'point'`. Also update or replace isSupportedScaleName so it
returns a boolean (not a `name is D3ScaleType` type guard) since it checks
prefixed keys in d3Scales, or add a new boolean helper used only for the
prefixed-key check. Ensure all branches return values assignable to D3ScaleType
| undefined.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6930 +/- ##
=======================================
Coverage 94.26% 94.26%
=======================================
Files 569 571 +2
Lines 55746 55767 +21
Branches 5185 5187 +2
=======================================
+ Hits 52550 52571 +21
Misses 3187 3187
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Bundle ReportChanges will increase total bundle size by 2.51kB (0.09%) ⬆️. 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-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
ckifer
approved these changes
Jan 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
I am going to reuse this combiner in another selector later and this turned out to be a fairly big change that I can isolate so let's merge this refactor first.
Related Issue
#1678
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.