Conversation
WalkthroughThis pull request refactors type annotations, removes utility functions and memoization wrappers, adds defensive null-checking in cache eviction, and tightens TypeScript compiler strictness settings across the codebase. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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
🧹 Nitpick comments (1)
src/util/LRUCache.ts (1)
9-11: Add validation for maxSize to prevent edge cases.While the null check added in lines 27-29 is a good defensive measure, it would be better to validate that
maxSize > 0in the constructor. IfmaxSizeis 0 or negative, the eviction logic could behave unexpectedly (the condition on line 25 would be true even for an empty cache).Apply this diff to add validation:
constructor(maxSize: number) { + if (maxSize <= 0) { + throw new Error('maxSize must be greater than 0'); + } this.maxSize = maxSize; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/state/touchEventsMiddleware.ts(1 hunks)src/state/zIndexSlice.ts(1 hunks)src/util/LRUCache.ts(1 hunks)src/util/YAxisUtils.tsx(1 hunks)src/util/scale/getNiceTickValues.ts(5 hunks)src/util/scale/util/arithmetic.ts(1 hunks)src/util/scale/util/utils.ts(0 hunks)src/util/useElementOffset.ts(1 hunks)test/util/scale/arithmetic.spec.ts(1 hunks)test/util/scale/utils.spec.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/util/scale/util/utils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/state/touchEventsMiddleware.ts (1)
src/state/store.ts (1)
RechartsRootState(94-94)
src/util/scale/getNiceTickValues.ts (3)
src/index.ts (1)
getNiceTickValues(117-117)src/util/scale/index.ts (2)
getNiceTickValues(1-1)getTickValuesFixedDomain(1-1)src/util/types.ts (1)
NumberDomain(686-686)
⏰ 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). (1)
- GitHub Check: Build, Test, Pack
🔇 Additional comments (12)
src/util/useElementOffset.ts (1)
53-53: LGTM! Type consistency improved.The parameter type widening from
HTMLDivElement | nulltoHTMLElement | nullaligns the implementation with the exportedSetElementOffsettype signature (line 40). Since the function only usesgetBoundingClientRect()which is available on allHTMLElementinstances, this change is safe and makes the hook more flexible.src/util/LRUCache.ts (1)
27-29: LGTM! Defensive null check improves type safety.The null check before deleting
firstKeyis appropriate for strict TypeScript compliance. WhilefirstKeyshould never be null/undefined in normal operation (since we're in a branch wherecache.size >= maxSize), this check:
- Satisfies strict TypeScript typing where
Map.keys().next().valueis typed asK | undefined- Provides defensive protection against edge cases
src/util/YAxisUtils.tsx (1)
29-29: LGTM! Type annotation aligns with function parameter.The explicit type annotation ensures the callback parameter type matches the
ticksparameter type, which is required for strict TypeScript settings.src/state/zIndexSlice.ts (1)
35-35: LGTM! Explicit type annotations for strict mode.The explicit type annotations on the reduce callback parameters and return type are necessary for stricter TypeScript settings and improve code clarity.
src/state/touchEventsMiddleware.ts (1)
14-14: LGTM! Type parameter ensures correct state typing.Adding the
RechartsRootStatetype parameter tocreateListenerMiddlewareensures the middleware has properly typed state access, which aligns with theListenerEffectAPI<RechartsRootState, AppDispatch>usage on line 20.tsconfig.json (1)
14-23: LGTM! Progressive enablement of strict TypeScript options.The incremental approach to enabling strict TypeScript settings is appropriate for a large codebase. The TODO comments on lines 16 and 22 clearly document the remaining work items referenced in issue #6645.
The enabled options (
strictBindCallApply,strictBuiltinIteratorReturn,strictPropertyInitialization,noImplicitAny,noImplicitThis,useUnknownInCatchVariables) represent solid progress toward full strictness.test/util/scale/utils.spec.ts (1)
2-2: LGTM! Consistent with memoize removal.The removal of the memoize import and its associated test suite aligns with the broader refactoring to remove memoization from the codebase.
test/util/scale/arithmetic.spec.ts (1)
3-3: LGTM! Test cleanup aligns with implementation changes.The removal of tests for interpolation utilities is consistent with their removal from the arithmetic module.
src/util/scale/getNiceTickValues.ts (3)
7-7: LGTM! Consistent with memoize removal.The removal of the memoize import aligns with the broader refactoring effort.
175-199: All call sites are properly memoized through parent selectors.The only production call to
getNiceTickValuesoccurs atsrc/state/selectors/axisSelectors.ts:1203within thecombineNiceTicksfunction, which is used as a result selector in acreateSelectormemoized selector (line 1224). The memoization is applied at the selector level via reselect'screateSelector, making the function-level memoization redundant. No other production call sites were found outside memoized contexts.
210-237: Remove memoization concern — getTickValuesFixedDomain is already protected by selector memoization.The function is called exclusively within
selectNiceTicks(src/state/selectors/axisSelectors.ts:1218), which usescreateSelectorfrom reselect. This automatically memoizes the result based on input selector dependencies (selectAxisDomain,selectAxisSettings,selectRealScaleType), so the removal of internal memoization poses no performance risk. The selector pattern provides the necessary caching layer.src/util/scale/util/arithmetic.ts (1)
54-54: Removal of interpolation utilities verified.Search confirms that interpolateNumber, uninterpolateNumber, and uninterpolateTruncation have no remaining references in the codebase. Safe to remove from exports.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6646 +/- ##
==========================================
- Coverage 94.17% 94.17% -0.01%
==========================================
Files 494 494
Lines 41666 41638 -28
Branches 4823 4813 -10
==========================================
- Hits 39240 39212 -28
Misses 2421 2421
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will decrease total bundle size by 4.71kB (-0.18%) ⬇️. 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-cjsAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
Description
There's more strict settings to turn on so let's kick it off.
One change to call out is the getNiceTickValues. It was wrapped in a custom memoize implementation that throws away types.
strictFunctionTypessettings flags that as a problem (and it should). I tried to replace it with memoize from es-toolkit but turns out that es-toolkit only supports memoizing functions with a single argument. We have 3 arguments. Because we only ever use this function inside an already memoized selector, I figured we might just as well throw away the extra memo layer instead of reimplementing memoize we don't need.Other changes are removing unused code, and fixing types. There are around 400 errors total to go through.
Related Issue
#6645
Summary by CodeRabbit
Bug Fixes
Chores