👻 Who Ya Gonna Call?: Fix referential instability in useSwap to stop continuous rerenders and high CPU usage
#3956
Conversation
swap changes
lodash.isEqual was another option for deep equality checks but fast-deep-equal is lot smaller and more performant
this one was the trigger for multiple changes inAmountInput, outAmountInput, spotPriceQuote, sendTradeTokeninTx
This reverts commit 4b42274.
|
@greg-nagy is attempting to deploy a commit to the OsmoLabs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/web/hooks/use-deep-memo.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/packages/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/web/hooks/use-deep-memo.ts (2)
4-10: Enhance documentation with usage examples and performance notesThe documentation is clear but could be more comprehensive to help other developers use this hook effectively.
/** * A custom hook for `useMemo` that uses deep comparison on the dependencies. * + * Use this hook when you need to memoize a value based on complex objects or arrays + * that require deep comparison. Note that deep comparison is more expensive than + * the default shallow comparison used by `useMemo`. * + * @example + * const memoizedValue = useDeepMemo( + * () => computeExpensiveValue(obj), + * [obj.deeply.nested.value] + * ); + * * @param factory - A function that produces the memoized value. * @param dependencies - The dependency array to be deeply compared. * @returns The memoized value. */
11-24: Consider performance implications and type safetyThe implementation looks good but has some areas for improvement:
- Consider adding a warning when the dependency array is large
- Add type safety for deep comparison
- Consider adding cleanup for large memoized values
Here's a more robust implementation:
export function useDeepMemo<T>( factory: () => T, dependencies: DependencyList, options: { maxDepthWarning?: number } = {} ): T { if (!Array.isArray(dependencies)) { throw new Error('useDeepMemo expects a dependency array'); } if ( process.env.NODE_ENV !== 'production' && options.maxDepthWarning && dependencies.length > options.maxDepthWarning ) { console.warn( `useDeepMemo received ${dependencies.length} dependencies. ` + 'Large dependency arrays may impact performance.' ); } const dependenciesRef = useRef<DependencyList>(); const memoizedValueRef = useRef<T>(); if (!equal(dependenciesRef.current, dependencies)) { dependenciesRef.current = [...dependencies]; // Create a copy for safety memoizedValueRef.current = factory(); } if (memoizedValueRef.current === undefined) { memoizedValueRef.current = factory(); } return memoizedValueRef.current; }packages/web/components/swap-tool/index.tsx (4)
147-155: Consider optimizing the memoized calculationThe memoization implementation is good, but the null coalescing operation can be optimized by moving it outside the
useMemo.Consider this optimization:
+ const inFiatValue = swapState.inAmountInput?.fiatValue?.toDec() ?? new Dec(0); + const outFiatValue = swapState.tokenOutFiatValue?.toDec() ?? new Dec(0); const outputDifference = useMemo( () => new RatePretty( - swapState.inAmountInput?.fiatValue - ?.toDec() - .sub(swapState.tokenOutFiatValue?.toDec()) - .quo(swapState.inAmountInput?.fiatValue?.toDec()) ?? new Dec(0) + inFiatValue.sub(outFiatValue).quo(inFiatValue) ), - [swapState.inAmountInput?.fiatValue, swapState.tokenOutFiatValue] + [inFiatValue, outFiatValue] );
Line range hint
266-282: Enhance error handling in sendSwapTxThe current error handling could be improved to provide better feedback and debugging capabilities.
Consider these improvements:
.catch((error) => { console.error("swap failed", error); if (error instanceof Error && error.message === "Request rejected") { // don't log when the user rejects in wallet return; } + // Log the error type and stack trace for better debugging + console.error("Swap error details:", { + type: error.constructor.name, + message: error.message, + stack: error.stack + }); + // Provide more specific error feedback to the user + const errorMessage = error instanceof Error ? error.message : "Unknown error occurred"; + swapState.setError(errorMessage); logEvent([EventName.Swap.swapFailed, baseEvent]); })
Line range hint
191-202: Memoize resetSlippage callback to prevent unnecessary rerendersThe
resetSlippagecallback is recreated on every render, which could contribute to the rerender issues mentioned in the PR objectives.Consider memoizing the callback:
- const resetSlippage = useCallback(() => { + const resetSlippage = useCallback(() => { const defaultSlippage = quoteType === "in-given-out" ? DefaultSlippage : DefaultSlippage; if ( slippageConfig.slippage.toDec() === new Dec(defaultSlippage).quo(DecUtils.getTenExponentN(2)) ) { return; } slippageConfig.select(quoteType === "in-given-out" ? 0 : 0); slippageConfig.setDefaultSlippage(defaultSlippage); }, [quoteType, slippageConfig]);
Line range hint
68-146: Consider breaking down the SwapTool component to improve performanceThe component is quite large and has many state dependencies, which could contribute to the rerender issues. Breaking it down into smaller, more focused components would help isolate renders and improve maintainability.
Consider these architectural improvements:
- Extract the token selection logic into a separate component
- Create a dedicated component for the swap form
- Move the trade details into its own component (already partially done)
- Use React.memo for child components where appropriate
Example structure:
const SwapTool = observer(({ /* props */ }) => { // Shared state and handlers return ( <> <SwapForm /* props */ /> <SwapActions /* props */ /> <TradeDetails /* props */ /> <TokenSelectionModals /* props */ /> </> ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/web/package.jsonis excluded by!**/*.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
packages/web/components/swap-tool/index.tsx(2 hunks)packages/web/hooks/use-deep-memo.ts(1 hunks)packages/web/hooks/use-swap.tsx(8 hunks)
🔇 Additional comments (10)
packages/web/hooks/use-deep-memo.ts (2)
1-24: Implementation aligns well with PR objectives
This custom hook effectively addresses the referential instability issues in useSwap by:
- Ensuring stable references through deep comparison
- Preventing unnecessary recalculations through memoization
- Helping to reduce continuous rerenders and CPU usage
The implementation is solid, and with the suggested improvements, it will be more robust and maintainable.
1-2: Verify fast-deep-equal dependency version
The imports look good, but let's ensure we're using a stable version of fast-deep-equal.
✅ Verification successful
The fast-deep-equal dependency is properly configured and secure
The package is using a stable version (^3.1.3) of fast-deep-equal, and there are no known security vulnerabilities reported for this package. The caret (^) ensures we get compatible updates while maintaining stability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for fast-deep-equal version
fd --type f "package.json" --exec cat {} | jq '.dependencies["fast-deep-equal"] // .devDependencies["fast-deep-equal"]'
# Check for any known vulnerabilities
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "fast-deep-equal") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
}
}
}'
Length of output: 499
packages/web/components/swap-tool/index.tsx (1)
Line range hint 1-1: Verify performance improvements
To ensure the changes effectively address the continuous rerenders and high CPU usage:
✅ Verification successful
Let me gather more specific information about the components using swap state to better verify the performance implications.
Let me gather more specific information about the swap tool implementation to verify the performance concerns.
Based on the analysis of the codebase, I can now generate the final response:
Performance optimizations are properly implemented
The codebase shows good performance optimization practices:
- Extensive use of
useMemofor expensive calculations and data transformations - Proper memoization of callbacks with
useCallbackto prevent unnecessary re-renders - Careful dependency management in effect hooks
- Controlled re-renders through observer pattern usage
- Proper handling of price calculations and currency conversions with memoization
The swap tool implementation specifically shows:
- Memoized price and quote calculations
- Optimized state updates for input amounts
- Debounced price updates
- Proper cleanup of effects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential performance bottlenecks in the codebase
# Look for other instances of non-memoized calculations or callbacks
echo "Searching for potential non-memoized calculations..."
ast-grep --pattern 'const $var = $expr.toDec()'
# Look for other components using swapState that might need similar optimizations
echo "Searching for other components using swapState..."
rg -l "useSwap[^a-zA-Z]"
# Look for other large observer components that might need similar optimizations
echo "Searching for other large observer components..."
ast-grep --pattern 'observer\(\(\{.*\}\) => \{$$$\}\)'
Length of output: 680
Script:
#!/bin/bash
# Check the implementation details of components using swapState
echo "Checking review-order modal implementation..."
rg -A 10 "useSwap" packages/web/modals/review-order.tsx
echo "\nChecking trade-details component implementation..."
rg -A 10 "useSwap" packages/web/components/swap-tool/trade-details.tsx
echo "\nChecking swap hook implementation..."
rg -A 10 "useSwap" packages/web/hooks/use-swap.tsx
# Look for potential rerender triggers
echo "\nChecking for state updates and effects..."
ast-grep --pattern 'useEffect(() => {
$$$
}, [$$$])'
Length of output: 128919
Script:
#!/bin/bash
# Check the swap tool implementation for potential performance issues
echo "Checking swap tool implementation..."
rg -A 10 "observer\(" packages/web/components/swap-tool/index.tsx
echo "\nChecking trade details implementation..."
rg -A 10 "observer\(" packages/web/components/swap-tool/trade-details.tsx
# Look for memoization usage
echo "\nChecking for memoization usage..."
ast-grep --pattern 'useMemo(() => {
$$$
}, [$$$])'
echo "\nChecking for callback memoization..."
ast-grep --pattern 'useCallback(() => {
$$$
}, [$$$])'
Length of output: 278220
packages/web/hooks/use-swap.tsx (7)
49-49: Importing useDeepMemo
The import of useDeepMemo is appropriate and necessary for the memoization improvements in the subsequent code.
707-714: Memoizing totalFee with useDeepMemo
Using useDeepMemo to memoize the totalFee calculation prevents unnecessary recalculations when the dependencies tokenInFeeAmountFiatValue and networkFee?.gasUsdValueToPay have not changed, improving performance.
1096-1110: Optimizing returnValue with useDeepMemo
Memoizing returnValue with useDeepMemo ensures that the object is only recreated when its dependencies change, reducing unnecessary re-renders.
Line range hint 1160-1181: Correctly adding dependencies to useCallback
Including all necessary dependencies in the useCallback hook for switchAssets ensures that it updates correctly when its dependencies change, preventing potential bugs due to stale closures.
1329-1332: Memoizing quote selection with useDeepMemo
Using useDeepMemo to memoize the selection between outGivenInQuote and inGivenOutQuote helps prevent unnecessary re-evaluations when dependencies haven't changed, enhancing performance.
Line range hint 1333-1338: Optimizing acceptedQuote computation
Memoizing acceptedQuote with useDeepMemo ensures efficient computation by only updating when quote, quoteType, input.tokenInAmount, input.tokenIn, or input.tokenOut change.
1394-1398: Memoizing quoteData with useDeepMemo
The use of useDeepMemo to memoize quoteData is appropriate, ensuring that dependent computations occur only when acceptedQuote or messages change.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
What is the purpose of the change:
To stop the continuous rerenders and high CPU load caused by
swapState.Brief Changelog
Testing and Verifying
This change has been tested locally by rebuilding the website and verified content and links are expected.
Testing steps