fix/PRO-3405/exchange-max-value-blocker#328
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EnterAmount Component
participant Redux Store
participant OfferFetcher
User->>EnterAmount Component: Inputs token amount
EnterAmount Component->>Redux Store: Dispatch setIsAboveLimit (true/false)
EnterAmount Component->>Redux Store: Update input value
EnterAmount Component->>EnterAmount Component: Check isAboveLimit
alt isAboveLimit is false
EnterAmount Component->>OfferFetcher: Fetch best swap offer
else isAboveLimit is true
EnterAmount Component--xOfferFetcher: Prevent offer fetch
EnterAmount Component->>User: Show above-limit error
end
Possibly related PRs
Suggested reviewers
Poem
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
npm error Exit handler never called! ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx (1)
201-206: Simplify the conditional logic for better maintainability.The current implementation uses two separate if statements that could be consolidated for better readability and maintainability.
Consider refactoring to a single conditional:
- if (tokenBalance && Number(value) > tokenBalance - (deploymentCost ?? 0)) { - dispatch(setIsAboveLimit(true)); - } - if (tokenBalance && Number(value) <= tokenBalance - (deploymentCost ?? 0)) { - dispatch(setIsAboveLimit(false)); - } + if (tokenBalance) { + const isAboveBalanceLimit = Number(value) > tokenBalance - (deploymentCost ?? 0); + dispatch(setIsAboveLimit(isAboveBalanceLimit)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/pillarx-app/components/MediaGridCollection/tests/__snapshots__/DisplayCollectionImage.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx(6 hunks)src/apps/the-exchange/reducer/theExchangeSlice.ts(4 hunks)src/utils/number.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx (3)
src/apps/the-exchange/hooks/useReducerHooks.tsx (1)
useAppSelector(6-6)src/utils/number.tsx (1)
formatExponential(77-111)src/apps/the-exchange/reducer/theExchangeSlice.ts (1)
setIsAboveLimit(115-117)
🪛 Biome (1.9.4)
src/utils/number.tsx
[error] 1-1: Do not shadow the global "parseInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (5)
src/apps/the-exchange/reducer/theExchangeSlice.ts (1)
27-27: LGTM! Clean Redux state management implementation.The
isAboveLimitflag is properly integrated into the Redux slice with correct typing, initialization, and action creators. This follows Redux Toolkit best practices perfectly.Also applies to: 54-54, 115-117, 139-139
src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx (3)
9-9: Good integration of the isAboveLimit state.The Redux state integration is clean and follows the established patterns in the component.
Also applies to: 77-79
163-165: Excellent deployment cost integration in balance validation.The logic correctly accounts for deployment costs when validating token limits, and the enhanced error message using
formatExponentialprovides clear feedback to users about their spendable balance.
180-180: Smart optimization to prevent unnecessary API calls.Preventing offer fetching when
isAboveLimitis true is an excellent optimization that avoids wasted API calls. The cleanup to setisOfferLoading(false)is also good practice.Also applies to: 186-186
src/utils/number.tsx (1)
77-111:Details
✅ Verification successful
Verify the formatExponential function with edge cases.
The function logic appears correct for converting exponential notation to decimal strings, but given its complexity, it should be thoroughly tested with various edge cases.
Please test this function with edge cases to ensure correctness:
🏁 Script executed:
#!/bin/bash # Test formatExponential function with various edge cases cat > test_format_exponential.js << 'EOF' // Import the function (assuming it can be imported in Node.js context) const formatExponential = (num) => { const str = num.toString(); if (!str.includes('e') && !str.includes('E')) { return str; } const [mantissa, exponent] = str.toLowerCase().split('e'); const exp = parseInt(exponent, 10); const [intPart, fracPart = ''] = mantissa.split('.'); const decimalPlaces = exp > 0 ? exp : -exp; if (exp > 0) { const newIntPart = intPart + (fracPart + '0'.repeat(decimalPlaces)).slice(0, decimalPlaces); const newFracPart = (fracPart + '0'.repeat(decimalPlaces)).slice(decimalPlaces); const final = newFracPart ? `${newIntPart}.${newFracPart}` : newIntPart; return final; } const newIntPart = '0'.repeat(decimalPlaces - intPart.length) + intPart; const final = `0.${newIntPart}${fracPart}`; return final; }; // Test cases const testCases = [ 1.23e-4, // Small positive number 1.23e4, // Large positive number 1.23e-10, // Very small number 1.23e10, // Very large number 1e-5, // No fractional part in mantissa 1.2345e-3, // Multiple decimal places 0, // Zero 123, // Regular number (no exponential) -1.23e-4 // Negative number ]; testCases.forEach(num => { console.log(`Input: ${num}, Output: "${formatExponential(num)}"`); }); EOF node test_format_exponential.jsLength of output: 347
formatExponential passes all tested edge cases
All tried inputs—including very small/large exponents (positive & negative), zero, and non-exponential numbers—produce the correct decimal strings. No further changes needed.
| @@ -1,4 +1,4 @@ | |||
| import { parseInt as parseIntLodash } from 'lodash'; | |||
| import { parseInt, parseInt as parseIntLodash } from 'lodash'; | |||
There was a problem hiding this comment.
Fix the shadowed global parseInt import.
The static analysis tool correctly identifies that importing parseInt shadows the global parseInt property, which can lead to confusion about variable origins.
Apply this fix to remove the shadowed import:
-import { parseInt, parseInt as parseIntLodash } from 'lodash';
+import { parseInt as parseIntLodash } from 'lodash';Then update line 88 to use the native parseInt:
- const exp = parseInt(exponent);
+ const exp = parseInt(exponent, 10);📝 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.
| import { parseInt, parseInt as parseIntLodash } from 'lodash'; | |
| // In src/utils/number.tsx | |
| // --- Update the import to avoid shadowing the global parseInt | |
| -import { parseInt, parseInt as parseIntLodash } from 'lodash'; | |
| +import { parseInt as parseIntLodash } from 'lodash'; | |
| // … other code … | |
| // --- At line 88, use the native parseInt with an explicit radix | |
| - const exp = parseInt(exponent); | |
| + const exp = parseInt(exponent, 10); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Do not shadow the global "parseInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In src/utils/number.tsx at line 1, the import statement imports `parseInt` from
lodash, which shadows the global `parseInt` function and causes confusion.
Remove the import of `parseInt` from lodash entirely to avoid shadowing. Then,
at line 88, update the code to use the native global `parseInt` function instead
of the lodash version.
Deploying x with
|
| Latest commit: |
adf40fc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://504fae3f.x-e62.pages.dev |
| Branch Preview URL: | https://fix-pro-3405-exchange-max-va.x-e62.pages.dev |
3 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit