Mattupham/fe 1085 show small allocations based on dust filter state #3909
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (6)
packages/web/components/complex/portfolio/portfolio-dust.ts (1)
3-4: Please document the threshold value and its implications.While the constants are well-structured, the
DUST_THRESHOLDvalue of 0.02 (2%) would benefit from documentation explaining:
- Why this specific threshold was chosen
- What constitutes "dust" in this context
- The implications of filtering out positions below this threshold
Consider adding a JSDoc comment above the constants:
+/** + * Key used for storing the portfolio dust visibility preference + */ export const PORTFOLIO_HIDE_DUST_KEY = "portfolio-hide-dust"; +/** + * Threshold (2%) below which portfolio positions are considered "dust". + * Positions with value < 2% of the total portfolio value will be hidden when dust filtering is enabled. + */ export const DUST_THRESHOLD = new Dec(0.02);packages/web/components/complex/portfolio/portfolio-page.tsx (2)
71-74: Consider adding type safety to hideDust state.While the implementation is correct, consider adding explicit type annotation to prevent potential type coercion issues.
- const [hideDust, setHideDust] = useLocalStorage( + const [hideDust, setHideDust] = useLocalStorage<boolean>( PORTFOLIO_HIDE_DUST_KEY, true );
147-149: Consider architectural improvements for dust filtering.The current implementation passes the dust filtering state to multiple components individually. Consider these improvements:
- Create a DustFilterContext if this state needs to be shared across many components
- Create a custom hook (e.g.,
useDustFilter) to encapsulate the filtering logic and state management- Remove Boolean() coercion by ensuring type safety at the state level
Example implementation:
// dustFilterContext.ts import { createContext, useContext, ReactNode } from 'react'; import { useLocalStorage } from 'react-use'; interface DustFilterContextType { hideDust: boolean; setHideDust: (value: boolean) => void; } const DustFilterContext = createContext<DustFilterContextType | undefined>(undefined); export const DustFilterProvider = ({ children }: { children: ReactNode }) => { const [hideDust, setHideDust] = useLocalStorage<boolean>( PORTFOLIO_HIDE_DUST_KEY, true ); return ( <DustFilterContext.Provider value={{ hideDust, setHideDust }}> {children} </DustFilterContext.Provider> ); }; export const useDustFilter = () => { const context = useContext(DustFilterContext); if (context === undefined) { throw new Error('useDustFilter must be used within a DustFilterProvider'); } return context; };Also applies to: 178-181
packages/web/components/table/portfolio-asset-balances.tsx (3)
64-68: LGTM! Consider makingtableTopPaddingmore type-safe.The component renaming and props structure look good. The
hideDuststate management has been properly lifted up.Consider using a more specific type for
tableTopPadding:- tableTopPadding?: number; + tableTopPadding?: `${number}px` | number;
Line range hint
375-397: Consider improving the dust filter toggle placement.While the implementation is solid, the button placement at the bottom of the table might be less discoverable, especially for users with many assets. Consider moving it above the table, possibly next to the search box, for better visibility and user experience.
- {assetsData.length > 0 && ( - <div className="flex items-center justify-end gap-4 py-2 px-4"> + <div className="flex items-center justify-between gap-4 my-3"> + <SearchBox + className="!w-[33.25rem] xl:!w-96" + currentValue={searchQuery?.query ?? ""} + onInput={onSearchInput} + placeholder={t("portfolio.searchBalances")} + debounce={500} + /> + {assetsData.length > 0 && ( <Button onClick={() => setHideDust(!hideDust)} className="gap-2 !border !border-osmoverse-700 !py-2 !px-4 !text-wosmongton-200" variant="outline" size="lg-full" > {hideDust ? t("portfolio.showHidden", { hiddenDustCount: hiddenDustCount.toString(), }) : t("portfolio.hideSmallBalances")} <Icon id="chevron-down" className={classNames("h-4 w-4 transition-transform", { "rotate-180": !hideDust, })} /> </Button> + )} + </div> - )} - <SearchBox - className="my-3 !w-[33.25rem] xl:!w-96" - currentValue={searchQuery?.query ?? ""} - onInput={onSearchInput} - placeholder={t("portfolio.searchBalances")} - debounce={500} - />
Line range hint
1-646: Consider splitting the component into smaller modules.While the implementation is solid, the file is quite long (646 lines). Consider splitting it into smaller, more focused components:
- Move table cells (
BalanceCell,PriceCell,AssetActionsCell) to separate files- Extract
AssetActionsDropdownto its own component file- Create a separate file for action-related utilities (
getActionOptions,handleSelectAction)This would improve maintainability and make the code more modular.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/web/components/complex/portfolio/allocation.tsx (3 hunks)
- packages/web/components/complex/portfolio/portfolio-dust.ts (1 hunks)
- packages/web/components/complex/portfolio/portfolio-page.tsx (7 hunks)
- packages/web/components/table/portfolio-asset-balances.tsx (2 hunks)
🔇 Additional comments (7)
packages/web/components/complex/portfolio/portfolio-dust.ts (1)
1-1: LGTM! Good choice using Dec for financial calculations.Using
Decfrom@keplr-wallet/unitis appropriate for handling decimal values in financial calculations, as it helps avoid floating-point precision issues.packages/web/components/complex/portfolio/allocation.tsx (4)
1-1: LGTM: Import changes are appropriate.The addition of
PricePrettyandDUST_THRESHOLDimports supports the new dust filtering functionality.Also applies to: 8-8
59-64: LGTM: Well-designed helper function.The
shouldShowItemInSelectedListfunction is pure, well-named, and encapsulates the dust filtering logic effectively.
118-141: LGTM: Robust allocation bar implementation.The changes effectively handle:
- Dust filtering using the helper function
- Edge case of negligible percentages (< 0.01)
- Proper width calculations and color assignments
8-8: Verify the DUST_THRESHOLD value.The code imports DUST_THRESHOLD but its value (0.02) should be verified:
- Is this value appropriate for all supported currencies?
- Should it be configurable based on the user's currency?
✅ Verification successful
Let me generate additional verification scripts to understand the dust threshold implementation better.
Based on the gathered information, I can now provide a final response:
The DUST_THRESHOLD value of 0.02 USD is appropriate and well-implemented.
The dust threshold implementation is consistent with industry practices:
- The value (0.02 USD) is reasonable as it's small enough to not impact significant holdings while effectively filtering out negligible amounts
- It's currency-agnostic as it's applied to USD-converted values, making it work uniformly across all supported assets
- The implementation uses the
Decclass from@keplr-wallet/unitensuring precise decimal handlingThe threshold is used consistently across the codebase for:
- Portfolio asset balances filtering
- Allocation calculations
- Portfolio value aggregations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of DUST_THRESHOLD to understand the context rg "DUST_THRESHOLD" -B 2 -A 2 # Look for any currency-specific configurations rg -g "*.ts" -g "*.tsx" "currency.*config" -B 2 -A 2Length of output: 7655
Script:
#!/bin/bash # Check the portfolio-dust.ts file to understand the implementation cat packages/web/components/complex/portfolio/portfolio-dust.ts # Look for Dec implementation to understand the decimal handling rg "class Dec" -A 10 # Search for any currency-specific minimum amounts or thresholds rg -g "*.ts" -g "*.tsx" "minimum.*amount|min.*amount" -B 2 -A 2Length of output: 103467
packages/web/components/complex/portfolio/portfolio-page.tsx (1)
5-5: LGTM: Import changes are well-structured.The imports are appropriate for implementing the dust filtering functionality, with good separation of concerns and proper naming conventions.
Also applies to: 14-14, 29-29
packages/web/components/table/portfolio-asset-balances.tsx (1)
Line range hint
151-167: LGTM! Efficient implementation of dust filtering.The filtering logic is well-implemented with proper performance optimization using
useMemo. The combination of dust filtering and watchlist sorting is handled cleanly.
mattupham
deleted the
mattupham/fe-1085-show-small-allocations-based-on-dust-filter-state
branch
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
packages/web/components/complex/portfolio/portfolio-dust.ts (1)
3-4: Add documentation and consider making the threshold configurable.A few suggestions to improve the constants:
- Add JSDoc comments explaining the purpose and significance of the 0.02 threshold value.
- Consider making
DUST_THRESHOLDconfigurable based on market conditions or user preferences.- Document the expected usage pattern of
PORTFOLIO_HIDE_DUST_KEY.Example documentation:
+/** Key used to persist dust visibility preference in storage */ export const PORTFOLIO_HIDE_DUST_KEY = "portfolio-hide-dust"; +/** + * Threshold value (in fiat) below which an asset is considered "dust". + * Assets below this value can be hidden based on user preference. + * @todo Consider making this configurable or environment-specific + */ export const DUST_THRESHOLD = new Dec(0.02);packages/web/components/complex/portfolio/allocation.tsx (2)
59-64: Add JSDoc documentation to the helper function.Consider adding JSDoc documentation to improve maintainability and provide better IDE support.
+/** + * Determines if an item should be shown in the selected list based on dust settings + * @param hideDust - Whether dust filtering is enabled + * @param fiatValue - The fiat value to check against dust threshold + * @returns boolean indicating if the item should be shown + */ const shouldShowItemInSelectedList = ( hideDust: boolean, fiatValue: PricePretty ) => { return !hideDust || !getIsDust(fiatValue); };
126-132: Extract negligible percentage logic to a constant.Consider extracting the negligible percentage threshold and width calculation to named constants for better maintainability.
+const NEGLIGIBLE_PERCENTAGE = new Dec(0.01); +const MIN_BAR_WIDTH = "0.1%"; + - const isNegligiblePercent = percentage - .toDec() - .lt(new Dec(0.01)); + const isNegligiblePercent = percentage + .toDec() + .lt(NEGLIGIBLE_PERCENTAGE); - const width = isNegligiblePercent - ? "0.1%" - : percentage.toString(); + const width = isNegligiblePercent + ? MIN_BAR_WIDTH + : percentage.toString();packages/web/components/complex/portfolio/portfolio-page.tsx (1)
147-149: Consider implementing a context for dust filter state.The implementation looks good, but as this feature grows, consider moving the dust filter state to a React Context if it needs to be shared across more components. This would avoid prop drilling and make the state management more maintainable.
Current implementation is clean and follows React best practices.
Also applies to: 178-181
packages/web/components/table/portfolio-asset-balances.tsx (1)
Line range hint
367-392: Add aria-label to improve button accessibility.The dust toggle button implementation is clean, but could benefit from an aria-label to improve accessibility for screen readers.
<Button onClick={() => setHideDust(!hideDust)} className="gap-2 !border !border-osmoverse-700 !py-2 !px-4 !text-wosmongton-200" variant="outline" size="lg-full" + aria-label={hideDust + ? t("portfolio.showHidden", { hiddenDustCount: hiddenDustCount.toString() }) + : t("portfolio.hideSmallBalances")} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/web/components/complex/portfolio/allocation.tsx (3 hunks)
- packages/web/components/complex/portfolio/portfolio-dust.ts (1 hunks)
- packages/web/components/complex/portfolio/portfolio-page.tsx (6 hunks)
- packages/web/components/table/portfolio-asset-balances.tsx (3 hunks)
🔇 Additional comments (6)
packages/web/components/complex/portfolio/portfolio-dust.ts (2)
1-1: LGTM!Imports are correctly specified and all imported types are used in the code.
1-7: Verify consistent usage of dust filtering across components.The implementation looks good and aligns with PR objectives. Let's verify its usage pattern across the codebase.
✅ Verification successful
Based on the results, I can now generate the final response:
Dust filtering implementation is consistently used across components
The verification shows proper usage of dust-related functionality:
getIsDustfunction is used consistently in portfolio components (portfolio-asset-balances.tsx,allocation.tsx)PORTFOLIO_HIDE_DUST_KEYis correctly used for persistent storage- The
0.02threshold appears only once in the dust implementation file, with no other hardcoded dust thresholds found in portfolio-related code🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check consistent usage of dust-related functionality echo "Checking usage of PORTFOLIO_HIDE_DUST_KEY..." rg "PORTFOLIO_HIDE_DUST_KEY" -A 2 echo -e "\nChecking usage of getIsDust function..." rg "getIsDust" -A 2 echo -e "\nChecking for any hardcoded dust thresholds that should use DUST_THRESHOLD..." rg "0\.02" --type tsLength of output: 97189
packages/web/components/complex/portfolio/portfolio-page.tsx (2)
5-5: LGTM: Import changes are well-organized.The new imports are properly organized and align with the dust filtering feature implementation.
Also applies to: 14-14, 29-29
69-72: Verify the persistence of the dust filter state.The implementation of dust filtering state using
useLocalStoragelooks good. The default value oftruemakes sense for hiding small allocations by default.Let's verify the constant definition:
✅ Verification successful
Constant is properly defined and imported
The
PORTFOLIO_HIDE_DUST_KEYconstant is correctly defined inportfolio-dust.tsand imported inportfolio-page.tsx. The implementation ensures consistent usage of the storage key across the codebase with no duplicates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the PORTFOLIO_HIDE_DUST_KEY constant definition # and ensure it's not duplicated elsewhere in the codebase rg "PORTFOLIO_HIDE_DUST_KEY" --type tsLength of output: 371
packages/web/components/table/portfolio-asset-balances.tsx (2)
64-68: LGTM: Well-structured component definition with clear props.The component has been appropriately renamed to
PortfolioAssetBalancesTableto better reflect its purpose, and the new dust-related props are properly typed.
24-24: Verify consistent dust threshold usage across the application.The implementation imports
getIsDustfrom a shared location, which is good for consistency. Let's verify that this is the only dust threshold implementation in the codebase.✅ Verification successful
Dust threshold implementation is consistent across the codebase
The verification confirms that:
- There is a single source of truth for dust threshold in
portfolio-dust.tswithDUST_THRESHOLD = new Dec(0.02)- The
getIsDustfunction is properly exported and reused across components- All dust-related functionality consistently imports and uses this implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other potential dust threshold implementations rg -g '!*.test.*' -g '!*.spec.*' "DUST|dust.*threshold|threshold.*dust" --type ts --type tsxLength of output: 122
Script:
#!/bin/bash # Search for dust-related implementations with proper file type patterns rg -t typescript -t javascriptreact -t typescriptreact "DUST|dust.*threshold|threshold.*dust" # Also search for the specific getIsDust function rg -t typescript -t javascriptreact -t typescriptreact "getIsDust" # Use ast-grep to find any dust-related function implementations ast-grep --pattern 'function $_($$ARGS) { $$$ dust $$$ }'Length of output: 298
Script:
#!/bin/bash # Search for dust-related implementations using ripgrep without file type filters rg "DUST|dust.*threshold|threshold.*dust" --type-add 'ts:*.{ts,tsx}' -t ts # Look for getIsDust function rg "getIsDust" --type-add 'ts:*.{ts,tsx}' -t ts # Find the actual implementation of getIsDust function fd -e ts -e tsx | xargs cat | grep -A 10 "export.*getIsDust"Length of output: 1993
| export const getIsDust = (value: PricePretty) => | ||
| value.toDec().lt(DUST_THRESHOLD); |
There was a problem hiding this comment.
Add input validation and error handling.
The function should be more robust by handling edge cases:
Consider this safer implementation:
-export const getIsDust = (value: PricePretty) =>
- value.toDec().lt(DUST_THRESHOLD);
+/**
+ * Determines if a given price value is considered "dust" by comparing it against DUST_THRESHOLD
+ * @param value The price to check
+ * @returns true if the value is below the dust threshold
+ * @throws if value is null/undefined
+ */
+export const getIsDust = (value: PricePretty): boolean => {
+ if (!value) {
+ throw new Error("Cannot check dust status of null/undefined value");
+ }
+ try {
+ return value.toDec().lt(DUST_THRESHOLD);
+ } catch (error) {
+ console.error("Error converting price to decimal:", error);
+ return false; // Conservative approach: don't hide values if conversion fails
+ }
+};📝 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.
| export const getIsDust = (value: PricePretty) => | |
| value.toDec().lt(DUST_THRESHOLD); | |
| /** | |
| * Determines if a given price value is considered "dust" by comparing it against DUST_THRESHOLD | |
| * @param value The price to check | |
| * @returns true if the value is below the dust threshold | |
| * @throws if value is null/undefined | |
| */ | |
| export const getIsDust = (value: PricePretty): boolean => { | |
| if (!value) { | |
| throw new Error("Cannot check dust status of null/undefined value"); | |
| } | |
| try { | |
| return value.toDec().lt(DUST_THRESHOLD); | |
| } catch (error) { | |
| console.error("Error converting price to decimal:", error); | |
| return false; // Conservative approach: don't hide values if conversion fails | |
| } | |
| }; |
| const isDust = getIsDust(fiatValue); | ||
|
|
||
| if (hideDust && isDust) return null; | ||
|
|
There was a problem hiding this comment.
Use consistent dust filtering approach.
The dust filtering logic is implemented differently in two places. The bar chart uses shouldShowItemInSelectedList while the detailed list uses direct checks. Consider using the helper function consistently.
- const isDust = getIsDust(fiatValue);
-
- if (hideDust && isDust) return null;
+ if (!shouldShowItemInSelectedList(hideDust, fiatValue)) return null;Committable suggestion was skipped due to low confidence.
| {selectedList | ||
| .filter(({ fiatValue }) => | ||
| shouldShowItemInSelectedList(hideDust, fiatValue) | ||
| ) | ||
| .map(({ key, percentage }, index) => { | ||
| const colorClass = | ||
| COLORS[selectedOption][index % COLORS[selectedOption].length]; | ||
|
|
||
| const isNegligiblePercent = percentage | ||
| .toDec() | ||
| .lt(new Dec(0.01)); | ||
|
|
||
| const width = isNegligiblePercent | ||
| ? "0.1%" | ||
| : percentage.toString(); | ||
|
|
||
| return ( | ||
| <div | ||
| key={key} | ||
| className={classNames("h-full rounded-[4px]", colorClass)} | ||
| style={{ width }} | ||
| /> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize performance with useMemo.
The filtered and mapped array for the colored bars could be memoized to prevent unnecessary recalculations on re-renders.
+ const coloredBars = useMemo(
+ () =>
+ selectedList
+ .filter(({ fiatValue }) =>
+ shouldShowItemInSelectedList(hideDust, fiatValue)
+ )
+ .map(({ key, percentage }, index) => {
+ const colorClass =
+ COLORS[selectedOption][index % COLORS[selectedOption].length];
+
+ const isNegligiblePercent = percentage
+ .toDec()
+ .lt(new Dec(0.01));
+
+ const width = isNegligiblePercent
+ ? "0.1%"
+ : percentage.toString();
+
+ return (
+ <div
+ key={key}
+ className={classNames("h-full rounded-[4px]", colorClass)}
+ style={{ width }}
+ />
+ );
+ }),
+ [selectedList, hideDust, selectedOption]
+ );Then use it in the render:
- {selectedList
- .filter(({ fiatValue }) =>
- shouldShowItemInSelectedList(hideDust, fiatValue)
- )
- .map(({ key, percentage }, index) => {
- const colorClass =
- COLORS[selectedOption][index % COLORS[selectedOption].length];
-
- const isNegligiblePercent = percentage
- .toDec()
- .lt(new Dec(0.01));
-
- const width = isNegligiblePercent
- ? "0.1%"
- : percentage.toString();
-
- return (
- <div
- key={key}
- className={classNames("h-full rounded-[4px]", colorClass)}
- style={{ width }}
- />
- );
- })}
+ {coloredBars}📝 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.
| {selectedList | |
| .filter(({ fiatValue }) => | |
| shouldShowItemInSelectedList(hideDust, fiatValue) | |
| ) | |
| .map(({ key, percentage }, index) => { | |
| const colorClass = | |
| COLORS[selectedOption][index % COLORS[selectedOption].length]; | |
| const isNegligiblePercent = percentage | |
| .toDec() | |
| .lt(new Dec(0.01)); | |
| const width = isNegligiblePercent | |
| ? "0.1%" | |
| : percentage.toString(); | |
| return ( | |
| <div | |
| key={key} | |
| className={classNames("h-full rounded-[4px]", colorClass)} | |
| style={{ width }} | |
| /> | |
| ); | |
| })} | |
| const coloredBars = useMemo( | |
| () => | |
| selectedList | |
| .filter(({ fiatValue }) => | |
| shouldShowItemInSelectedList(hideDust, fiatValue) | |
| ) | |
| .map(({ key, percentage }, index) => { | |
| const colorClass = | |
| COLORS[selectedOption][index % COLORS[selectedOption].length]; | |
| const isNegligiblePercent = percentage | |
| .toDec() | |
| .lt(new Dec(0.01)); | |
| const width = isNegligiblePercent | |
| ? "0.1%" | |
| : percentage.toString(); | |
| return ( | |
| <div | |
| key={key} | |
| className={classNames("h-full rounded-[4px]", colorClass)} | |
| style={{ width }} | |
| /> | |
| ); | |
| }), | |
| [selectedList, hideDust, selectedOption] | |
| ); | |
| {coloredBars} |
| if (!asset.usdValue) return null; | ||
| const isDust = getIsDust(asset.usdValue); |
There was a problem hiding this comment.
Consider separating zero value handling from dust filtering.
The current implementation filters out assets with no USD value regardless of the hideDust setting. Consider separating these concerns to allow users to see zero-value assets when dust is not hidden.
- if (!asset.usdValue) return null;
- const isDust = getIsDust(asset.usdValue);
- if (hideDust && isDust) return null;
+ const isDust = asset.usdValue ? getIsDust(asset.usdValue) : false;
+ if (hideDust && (isDust || !asset.usdValue)) return null;Committable suggestion was skipped due to low confidence.
What is the purpose of the change:
When hide dust for portfolio page is set to true, we should also filter out assets / items in the allocation section that are below the threshold
Linear Task
https://linear.app/osmosis/issue/FE-1085/show-small-allocations-based-on-dust-filter-state
Brief Changelog
Testing and Verifying
hide.dust.for.allocation.mov