Mattupham/fe 725 porfolio v3 variants detected popup #3871
Conversation
…as asset variants
…tId, autoclose, and close button, update local storage logic
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
| ...toastOptions, | ||
| toastId: ToastType.ALLOYED_ASSETS, // prevents duplicate toasts by using an explicit toast id | ||
| autoClose: false, // prevents Alloyed Assets toast from closing automatically - https://fkhadra.github.io/react-toastify/autoClose/ | ||
| closeButton: false, |
There was a problem hiding this comment.
some custom toast options to match figma designs + behavior
WalkthroughThe pull request introduces enhancements to asset management functionality across multiple files. It adds new functions for checking asset variants, a custom React hook for managing user notifications regarding these assets, and updates the toast notification system to include a new toast type. The changes affect core logic, UI components, and testing files, reflecting a comprehensive update to asset variant handling and user interaction. 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: 6
🧹 Outside diff range and nitpick comments (13)
packages/web/components/alert/types.ts (1)
15-19: LGTM! Enum updates improve type safety and add new functionality.The changes to the
ToastTypeenum are well-implemented:
- Explicit string assignments for existing members enhance code clarity and reduce the risk of typos.
- The addition of
ALLOYED_ASSETSaligns with the PR objective of detecting portfolio variants.Consider using a more descriptive name for
ALLOYED_ASSETSif appropriate, such asVARIANT_ASSETS_DETECTEDorPORTFOLIO_VARIANTS_FOUND, to make its purpose clearer to other developers.packages/web/components/alert/page-navigation-toast.tsx (1)
7-24: LGTM with suggestions: Toast configuration is comprehensive.The toast configuration and content are well-structured and provide a good user experience. The use of custom styling classes allows for consistent theming. However, consider the following improvements:
- Move hardcoded values to constants for better maintainability:
const TOAST_AUTO_CLOSE_MS = 3000; const TOAST_BG_CLASS = 'bg-osmoverse-700'; // In the toast configuration autoClose: TOAST_AUTO_CLOSE_MS, className: TOAST_BG_CLASS,
- Consider extracting the JSX content into a separate component for better readability and reusability:
const PageNavigationToastContent: React.FC<{ pageName: string }> = ({ pageName }) => ( <div className="flex items-center gap-3"> <Icon id="arrow-right" width={24} height={24} /> <div> <h6 className="text-lg font-semibold">New Page</h6> <p className="text-sm text-osmoverse-200">You are now on: {pageName}</p> </div> </div> ); // In the toast function toast(<PageNavigationToastContent pageName={pageName} />, { ... });These changes would enhance the maintainability and readability of the code.
packages/trpc/src/portfolio.ts (1)
41-53: LGTM: New methodgetHasAssetVariantsadded correctly.The new
getHasAssetVariantsmethod has been implemented correctly, following the established patterns in the router. It properly usespublicProcedure, validates input with a zod schema, and utilizes the context to accessassetLists.Consider adding a brief comment above the method to explain its purpose and what "asset variants" refers to in this context. This would improve code readability and maintainability.
packages/server/src/queries/complex/portfolio/__tests__/allocation.spec.ts (2)
192-346: LGTM: Asset constants are well-defined, but consider reducing redundancy.The new asset constants (ASSET_OSMO, ASSET_SAIL, and ASSET_ETH_AXL) are well-structured and provide comprehensive information. However, there's some redundancy in the data, particularly in the
logoURIsandpriceobjects.Consider creating helper functions or using object composition to reduce redundancy and improve maintainability. For example:
const createLogoURIs = (png: string, svg?: string) => ({ png, ...(svg && { svg }) }); const createPrice = (poolId: string, denom: string) => ({ poolId, denom }); const ASSET_OSMO: Asset = { // ... other properties logoURIs: createLogoURIs( "https://raw.githubusercontent.com/cosmos/chain-registry/master/osmosis/images/osmo.png", "https://raw.githubusercontent.com/cosmos/chain-registry/master/osmosis/images/osmo.svg" ), price: createPrice("1464", "ibc/498A0751C798A0D9A389AA3691123DADA57DAA4FE165D5C75894505B876BA6E4"), // ... remaining properties };This approach would make the constants more concise and easier to maintain.
348-398: LGTM: Comprehensive test suite for asset variants, with room for improvement.The new test suite for checking asset variants is well-structured and covers the main scenarios: presence of variants, absence of variants, and no assets. This provides good coverage for the
checkHasAssetVariantsfunction.Consider the following improvements to enhance the test suite:
- Use a beforeEach block to set up common test data, reducing duplication:
describe("Has Asset Variants", () => { let baseAssets: Asset[]; beforeEach(() => { baseAssets = [ASSET_OSMO, ASSET_SAIL]; }); // ... test cases });
Add edge cases, such as:
- An asset with an empty variantGroupKey
- Multiple assets with the same variantGroupKey
- Assets with variantGroupKey that don't match any user assets
Use test.each for parameterized testing of similar scenarios:
test.each([ { desc: "with variants", userAssets: [...], expectedResult: true }, { desc: "without variants", userAssets: [...], expectedResult: false }, { desc: "no assets", userAssets: [], expectedResult: false }, ])("should return $expectedResult $desc", ({ userAssets, expectedResult }) => { const result = checkHasAssetVariants(userAssets, baseAssets); expect(result).toBe(expectedResult); });These improvements would make the tests more robust, easier to maintain, and cover more scenarios.
packages/web/components/layouts/main.tsx (1)
Line range hint
1-95: Summary: New hook added, but usage unclearThe
useHasAssetVariantshook has been added to theMainLayoutcomponent, suggesting new functionality related to asset variants. However, the hook's return value is not being used in the current implementation. Consider the following next steps:
- Implement the logic to use the hook's return value in the component.
- Add comments explaining the purpose of the hook if its effects are not visible in this file.
- If the implementation is incomplete, consider adding a TODO comment or creating a follow-up task.
To get a better understanding of the
useHasAssetVariantshook and its intended usage, you may want to review its implementation:#!/bin/bash # Find and display the implementation of useHasAssetVariants rg --type typescript -A 20 'export function useHasAssetVariants' packages/web/hookspackages/server/src/queries/complex/portfolio/allocation.ts (2)
177-191: Consider adding unit tests forcheckHasAssetVariantsfunction.Adding unit tests will help ensure that
checkHasAssetVariantsbehaves correctly with various inputs, including edge cases where assets may or may not have variants.Would you like assistance in creating unit tests for this function?
193-217: Consider adding unit tests forgetHasAssetVariantsfunction.Unit tests will help verify that
getHasAssetVariantscorrectly interacts withqueryAllocationand handles different scenarios, including error cases.Would you like assistance in creating unit tests or setting up mocks for
queryAllocation?packages/web/components/alert/toast.tsx (3)
244-259: Reminder: Implement the 'onConvert' FunctionalityThe
onConvertfunction currently contains placeholder comments indicating that the modal linkage and conversion logic will be implemented in another PR. Ensure that this functionality is properly implemented to provide the intended user experience when the "Convert" button is clicked.Let me know if you need assistance in implementing the modal interaction or if you'd like me to open a GitHub issue to track this task.
235-239: Code Simplification: Streamline State Update in 'onDismiss'The state update in the
onDismissfunction can be simplified. SincesetDoNotShowAgainis being set based on theisCheckedstate, you can directly passisCheckedtosetDoNotShowAgainwithout the conditional statement.Simplify the code as follows:
const onDismiss = () => { - if (isChecked) { - setDoNotShowAgain(true); - } else { - setDoNotShowAgain(false); - } + setDoNotShowAgain(isChecked); closeToast(); };
227-230: Code Style Suggestion: Destructure Full Array from useLocalStorageWhile currently only the setter function from
useLocalStorageis used, consider destructuring both the value and setter for consistency and potential future use. This also improves code readability.Adjust the destructuring as follows:
- const [, setDoNotShowAgain] = useLocalStorage( + const [doNotShowAgain, setDoNotShowAgain] = useLocalStorage( AlloyedAssetsToastDoNotShowKey, false );packages/web/hooks/use-has-asset-variants.tsx (2)
31-31: Simplify the address parameter by removing the default empty stringSince
enabledistrueonly whenwallet?.addressis defined, defaulting to an empty string is unnecessary. Passingwallet.addressdirectly clarifies that an address is expected here.Apply this change:
{ - address: wallet?.address ?? "", + address: wallet.address, },
12-54: Rename the hook to reflect its side-effect behaviorThe hook
useHasAssetVariantsprimarily triggers a toast notification and doesn't return any value. Renaming it to something likeuseDisplayAssetVariantToastwould more accurately describe its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (18)
packages/web/localizations/de.jsonis excluded by!**/*.jsonpackages/web/localizations/en.jsonis excluded by!**/*.jsonpackages/web/localizations/es.jsonis excluded by!**/*.jsonpackages/web/localizations/fa.jsonis excluded by!**/*.jsonpackages/web/localizations/fr.jsonis excluded by!**/*.jsonpackages/web/localizations/gu.jsonis excluded by!**/*.jsonpackages/web/localizations/hi.jsonis excluded by!**/*.jsonpackages/web/localizations/ja.jsonis excluded by!**/*.jsonpackages/web/localizations/ko.jsonis excluded by!**/*.jsonpackages/web/localizations/pl.jsonis excluded by!**/*.jsonpackages/web/localizations/pt-br.jsonis excluded by!**/*.jsonpackages/web/localizations/ro.jsonis excluded by!**/*.jsonpackages/web/localizations/ru.jsonis excluded by!**/*.jsonpackages/web/localizations/tr.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-cn.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-hk.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-tw.jsonis excluded by!**/*.jsonpackages/web/public/icons/sprite.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (9)
- packages/server/src/queries/complex/portfolio/tests/allocation.spec.ts (2 hunks)
- packages/server/src/queries/complex/portfolio/allocation.ts (2 hunks)
- packages/trpc/src/portfolio.ts (2 hunks)
- packages/web/components/alert/page-navigation-toast.tsx (1 hunks)
- packages/web/components/alert/toast.tsx (3 hunks)
- packages/web/components/alert/types.ts (1 hunks)
- packages/web/components/layouts/main.tsx (2 hunks)
- packages/web/hooks/index.ts (1 hunks)
- packages/web/hooks/use-has-asset-variants.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (11)
packages/web/components/alert/page-navigation-toast.tsx (2)
1-4: LGTM: Imports are appropriate and well-structured.The imports are correctly set up for the functionality being implemented. The use of the
~alias in the import path forIconsuggests a custom path configuration, which is a good practice for maintaining consistent imports across the project.
6-6: LGTM: Function declaration is clear and well-structured.The
showPageNavigationToastfunction is appropriately named, exported, and typed. It takes apageNameparameter of typestring, which aligns well with its purpose of displaying a navigation toast.packages/trpc/src/portfolio.ts (1)
2-2: LGTM: Import statement updated correctly.The import statement has been properly updated to include
getHasAssetVariantsfrom the "@osmosis-labs/server" module. This change is consistent with the new method added to the router.packages/server/src/queries/complex/portfolio/__tests__/allocation.spec.ts (2)
1-1: LGTM: New imports are appropriate for the added functionality.The new imports for the
Assettype andcheckHasAssetVariantsfunction are correctly added and necessary for the new tests and functionality introduced in this file.Also applies to: 6-6
Line range hint
1-398: Overall, the changes are well-implemented with room for optimization.The additions to this file, including new imports, asset constants, and a test suite for checking asset variants, are generally well-structured and provide good coverage for the new functionality. The test cases cover the main scenarios, which is commendable.
However, there are opportunities for improvement:
- Reducing redundancy in the asset constants
- Enhancing the test suite with more robust setup and additional edge cases
- Utilizing more advanced testing patterns for better maintainability
These optimizations would further improve the code quality and test coverage. Despite these suggestions, the current implementation is functional and provides a solid foundation for the new feature.
packages/web/components/layouts/main.tsx (3)
11-16: LGTM: New hook import added correctlyThe
useHasAssetVariantshook has been properly imported along with the existing hooks. This addition suggests new functionality related to asset variants in the component.
Line range hint
45-95: Verify completeness of useHasAssetVariants implementationThe
useHasAssetVariantshook has been added, but there are no changes in the component's render logic to reflect its usage. Please confirm if the implementation is complete or if there are pending changes to utilize the hook's functionality in the UI.To check for any usage of the hook's return value in the render logic, you can run:
#!/bin/bash # Search for any usage of variables or functions that might come from useHasAssetVariants in the JSX rg --type typescript -A 10 'useHasAssetVariants' packages/web/components/layouts/main.tsx | rg 'return'
42-43: Verify the usage of useHasAssetVariants hookThe
useHasAssetVariantshook has been added, but its return value is not being used. Please confirm if this is intentional or if there's additional logic to be implemented using the hook's return value.To check for any usage of the hook's return value, you can run:
packages/server/src/queries/complex/portfolio/allocation.ts (1)
177-191:⚠️ Potential issueEnsure
variantGroupKeyis defined to prevent potential runtime errors.In the comparison
matchingAsset.coinMinimalDenom !== matchingAsset.variantGroupKey, ifvariantGroupKeyisundefined, the comparison may not behave as expected. Please verify that all assets havevariantGroupKeydefined or add a null check to handle undefined values.Run the following script to check for assets without
variantGroupKey:packages/web/components/alert/toast.tsx (1)
72-84: Confirmation: Proper Implementation of 'ALLOYED_ASSETS' Toast TypeThe addition of the
ToastType.ALLOYED_ASSETScase in thedisplayToastfunction appears correct. The custom toast is configured to prevent duplicates and to stay open until manually closed, which aligns with the intended user experience.packages/web/hooks/use-has-asset-variants.tsx (1)
36-39: Verify that the toast is displayed only once per sessionEnsure that the toast notification doesn't appear multiple times unnecessarily. Since the toast is triggered on successful queries, additional checks might be needed to prevent repeated displays.
Consider verifying the toast behavior across different app states.
| }; | ||
|
|
||
| const onConvert = () => { | ||
| // TODO: link to modal in other PR |
There was a problem hiding this comment.
will slot this in the other PR for asset conversion
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/web/components/alert/toast.tsx (2)
72-84: LGTM: New toast type handled correctlyThe implementation for
ToastType.ALLOYED_ASSETSis well-structured and prevents duplicate toasts. TheautoCloseandcloseButtonsettings are appropriate for the custom behavior of this toast type.Consider extracting the toast options into a separate constant for better readability:
const ALLOYED_ASSETS_TOAST_OPTIONS = { toastId: ToastType.ALLOYED_ASSETS, autoClose: false, closeButton: false, }; // Then in the switch case: showToast( ({ closeToast }) => ( <AlloyedAssetsToast {...alert} closeToast={closeToast} /> ), { ...toastOptions, ...ALLOYED_ASSETS_TOAST_OPTIONS, } );This change would improve code organization and make it easier to maintain these options in the future.
224-259: LGTM: Well-structured AlloyedAssetsToast componentThe
AlloyedAssetsToastcomponent is well-implemented, with proper state management usinguseLocalStorageanduseState. The logic for handling the "do not show again" functionality is correct.Regarding the TODO comment:
The
onConvertfunction is currently a placeholder. Would you like assistance in creating a GitHub issue to track the implementation of the modal and the associated logic mentioned in the TODO comment?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/server/src/queries/complex/portfolio/tests/allocation.spec.ts (2 hunks)
- packages/web/components/alert/toast.tsx (3 hunks)
- packages/web/hooks/index.ts (1 hunks)
- packages/web/hooks/use-has-asset-variants.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/hooks/index.ts
- packages/web/hooks/use-has-asset-variants.tsx
🧰 Additional context used
🔇 Additional comments (6)
packages/web/components/alert/toast.tsx (3)
2-2: LGTM: New imports added for enhanced functionalityThe new imports (
useState,useLocalStorage,Button, andCheckbox) are appropriate for the implementation of theAlloyedAssetsToastcomponent and its associated functionality.Also applies to: 9-9, 13-14
221-222: LGTM: Exported constant for local storage keyThe addition of
AlloyedAssetsToastDoNotShowKeyas an exported constant is a good practice. It centralizes the management of the local storage key and allows for easy reuse across the application if needed.
Line range hint
1-331: Overall assessment: Well-implemented Alloyed Assets toast featureThe changes to
packages/web/components/alert/toast.tsxeffectively implement the new Alloyed Assets toast functionality. The code is well-structured, maintains consistency with existing patterns, and properly handles state management and user interactions. A few minor suggestions have been made for improvement, including:
- Extracting toast options into a separate constant for better readability.
- Addressing the TODO comment for the
onConvertfunction.- Correcting a potential typo in a class name.
These small adjustments will further enhance the quality and maintainability of the code. Great work overall!
packages/server/src/queries/complex/portfolio/__tests__/allocation.spec.ts (3)
1-1: LGTM: New imports and asset constants are well-defined and necessary.The new imports for the
Assettype andcheckHasAssetVariantsfunction are appropriate for the added functionality. The asset constants (ASSET_OSMO, ASSET_SAIL, ASSET_ETH_AXL) are well-structured and provide comprehensive information about each asset, which is crucial for the new tests.Also applies to: 6-6, 192-346
348-397: LGTM: Comprehensive test suite for asset variant checking.The new test suite "Has Asset Variants" is well-structured and covers the main scenarios for the
checkHasAssetVariantsfunction:
- When there are asset variants
- When there are no asset variants
- When the user has no assets
The tests make good use of the previously defined asset constants and have clear assertions that match the expected behavior.
Line range hint
1-398: Overall, the changes align well with the PR objectives.The additions to this test file enhance the coverage for the allocation module, particularly in the area of asset variant detection. This aligns well with the PR objective of introducing variant detection logic. The new tests are well-structured, comprehensive, and consistent with the existing code style.
Some key points:
- New asset constants provide detailed information for testing.
- The
checkHasAssetVariantsfunction is thoroughly tested with various scenarios.- The changes maintain the file's original structure while adding valuable new tests.
These enhancements will help ensure the reliability of the variant detection feature in the portfolio.
…et from Map, rename checkbox state for remind me later
| !doNotShowAgain && | ||
| Boolean(wallet?.isWalletConnected && wallet?.address); | ||
|
|
||
| api.local.portfolio.getHasAssetVariants.useQuery( |
There was a problem hiding this comment.
From looking at implementation of useHasAssetVariants
What if we just run that function inside getAllocation and return the result as a flag in the response since we operate on that same data that we query elsewhere anyways? Then useHasAssetVariants here would call getAllocation looking for that hasAssetVariants: boolean flag instead when deciding to show the toast.
Pros:
- Can remove a redundant tRPC procedure call (this one,
getHasAssetVariants) inportfolioRouter - Prevent redundant API call to SQS for
passthrough/portfolio-assets, reducing load on SQS services and avoiding an unnecessary query from the browser - If we check for convertible assets on every page anyways, we can reuse that allocation query data when they navigate to the portfolio page, increasing navigation performance
Cons:
- May be hard to know that flag is there. In other words, the purpose of
getAllocationhas expanded a bit to involve alloyed assets logic
Overall, I think it's worth it, as we can delete a bunch of code. Which means less chance for error, and less to test.
There was a problem hiding this comment.
It's not necessarily redundant, I reused the underlyingqueryAllocation, but separated the procedures out since the processing / logic is different. It doesn't add that much code and we do the processing differently on the portfolio page per item
This setup shows a clear distinction with what data is being processed
There was a problem hiding this comment.
I'm seeing redundant calls to portfolio-assets from different initiator call stacks.
I think it could easily be avoided, and would save on bandwidth to SQS and improve browser performance:
| const enabled = | ||
| alloyedAssets && | ||
| !doNotShowAgain && | ||
| Boolean(wallet?.isWalletConnected && wallet?.address); |
There was a problem hiding this comment.
It seems to enable we should also check for wallet.address !== "" as it seems the query is going through with an empty string "" before the wallet is loaded:
There was a problem hiding this comment.
I updated this with const { isLoading: isWalletLoading } = useWalletSelect(); and the check for the empty wallet address
| !doNotShowAgain && | ||
| Boolean(wallet?.isWalletConnected && wallet?.address); | ||
|
|
||
| api.local.portfolio.getHasAssetVariants.useQuery( |
There was a problem hiding this comment.
I'm seeing redundant calls to portfolio-assets from different initiator call stacks.
I think it could easily be avoided, and would save on bandwidth to SQS and improve browser performance:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/web/components/alert/toast.tsx (1)
244-259: TODO: ImplementonConvertfunctionalityThe
onConvertfunction currently contains only comments outlining the expected behavior. Ensure that this functionality is implemented in the upcoming PR mentioned in the TODO comment.Would you like assistance in creating a GitHub issue to track the implementation of the
onConvertfunction?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/server/src/queries/complex/portfolio/allocation.ts (2 hunks)
- packages/web/components/alert/toast.tsx (3 hunks)
- packages/web/hooks/use-has-asset-variants.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/hooks/use-has-asset-variants.tsx
🧰 Additional context used
🔇 Additional comments (9)
packages/server/src/queries/complex/portfolio/allocation.ts (3)
3-3: LGTM: Import statement added correctly.The new import for
AssetandAssetListfrom "@osmosis-labs/types" is necessary for the newly added functions and is placed appropriately with other imports.
177-193: LGTM: Efficient implementation ofcheckHasAssetVariants.The function is well-implemented:
- It uses a Map for O(1) lookups, improving performance as suggested in a past review.
- The logic correctly checks if any user coin has variants.
- The implementation is concise and easy to understand.
Great job on incorporating the previous feedback!
176-219: Overall implementation aligns well with PR objectives.The new functions
checkHasAssetVariantsandgetHasAssetVariantsare well-integrated into the existing codebase and effectively implement the variant detection logic as outlined in the PR objectives. The code is consistent with the existing style and enhances the functionality without disrupting existing features.Great job on implementing this feature! Just ensure to address the error handling suggestion for
queryAllocationto further improve the robustness of the implementation.packages/web/components/alert/toast.tsx (6)
2-2: LGTM: Import statements updated correctlyThe new import statements for
useState,useLocalStorage,Button, andCheckboxare correctly added and necessary for the implementation of the newAlloyedAssetsToastcomponent.Also applies to: 9-9, 13-14
72-84: LGTM: New ALLOYED_ASSETS toast type handled correctlyThe
displayToastfunction has been updated to handle the newToastType.ALLOYED_ASSETScase appropriately. The implementation uses a custom render function to pass down thecloseToastfunctionality and sets specific options to prevent duplicate toasts and auto-closing. This approach is consistent with the handling of other toast types and suitable for the Alloyed Assets notification.
221-222: LGTM: Clear and well-named constant for local storage keyThe
AlloyedAssetsToastDoNotShowKeyconstant is appropriately named and exported. It provides a clear, reusable key for managing the visibility state of the Alloyed Assets toast in local storage.
305-315: LGTM: Checkbox accessibility improvedThe checkbox and its label are now properly associated within a
labelelement, which improves accessibility for screen readers and keyboard navigation. This is a good practice and addresses the previous accessibility concern.
321-321:⚠️ Potential issueFix typo in className
There appears to be a typo in the
classNameproperty. The class!text-wosmongton-200is likely incorrect. Please verify and update it to the correct value, possibly!text-osmoverse-200to match the naming convention used elsewhere.Apply this diff to correct the class name:
- className="!border !border-osmoverse-alpha-700 !text-wosmongton-200" + className="!border !border-osmoverse-alpha-700 !text-osmoverse-200"Likely invalid or redundant comment.
234-242: Verify the logic for settingdoNotShowAgainThe current implementation of
onDismisssetsdoNotShowAgaintofalsewhen the user checks "remind me later", and totruewhen they don't. This seems counterintuitive. Please verify if this is the intended behavior or if it should be reversed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/trpc/src/portfolio.ts (1)
41-57: LGTM: New methodgetHasAssetVariantsimplemented correctly.The new method is well-structured and consistent with other router methods. It includes proper input validation using zod and handles the empty address case efficiently. The use of context to pass
assetListsis correct.Consider adding an explicit return type to the query for improved type safety:
.query(async ({ input: { address }, ctx }): Promise<{ hasAssetVariants: boolean }> => { // ... existing implementation ... })This change would make the return type more explicit and consistent with TypeScript best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/trpc/src/portfolio.ts (2 hunks)
- packages/web/hooks/use-has-asset-variants.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/hooks/use-has-asset-variants.tsx
🧰 Additional context used
🔇 Additional comments (2)
packages/trpc/src/portfolio.ts (2)
2-2: LGTM: Import statement updated correctly.The import statement has been properly updated to include the
getHasAssetVariantsfunction from "@osmosis-labs/server". This change aligns with the new method added to the router.
Line range hint
1-57: Summary: Changes align well with PR objectives.The modifications to this file successfully implement the variant detection logic as outlined in the PR objectives. The new
getHasAssetVariantsmethod is well-integrated into the existingportfolioRouter, following consistent patterns and best practices.Key points:
- The import statement is correctly updated to include the new function.
- The new method is properly implemented with input validation and error handling.
- The changes maintain the existing code structure and style.
These changes contribute to the overall goal of prompting users to convert their alloyed asset variants if their portfolio includes such assets.
* prevent redundant query * prevent query before mount
| const userBalanceDenoms = | ||
| categories["user-balances"]?.account_coins_result?.map( | ||
| (result) => result.coin.denom | ||
| ) ?? []; | ||
|
|
||
| // check for alloyed asset variants |
There was a problem hiding this comment.
Giga Nit: variants include canonical assets such as USDC, not just alloys
There was a problem hiding this comment.
ha updated that comment for clarity
There was a problem hiding this comment.
I noticed nothing happens when I click convert:
https://github.com/user-attachments/assets/f2445a3a-34c5-47f7-9f26-1ad786f20d23
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/server/src/queries/complex/portfolio/allocation.ts (3)
24-25: LGTM: New property added to interface.The addition of the
hasVariantsproperty to theGetAllocationResponseinterface is appropriate and aligns with the PR objectives.Consider updating the comment to be more specific:
/** Indicates whether there are asset variants (including alloys and canonical assets) that can be converted to canonical form. */This change would make the comment more informative and align it with the implementation details.
191-207: LGTM: Efficient implementation of asset variant checking.The
checkHasAssetVariantsfunction is well-implemented, using a Map for efficient asset lookups. This aligns with a previous review suggestion and provides good performance.For improved readability, consider extracting the variant check logic into a separate function:
const isVariant = (asset: Asset | undefined) => asset && asset.coinMinimalDenom !== asset.variantGroupKey; return userCoinMinimalDenoms.some((coinMinimalDenom) => isVariant(assetMap.get(coinMinimalDenom)) );This change would make the main function body more concise and the logic more explicit.
Line range hint
145-189: Add error handling forqueryAllocation.While the changes effectively implement asset variant detection, the previously suggested error handling for
queryAllocationis still missing. This could lead to unhandled promise rejections if the query fails.Consider implementing error handling as suggested in a previous review:
export async function getAllocation({ address, assetLists, allocationLimit = 5, }: { address: string; assetLists: AssetList[]; allocationLimit?: number; }): Promise<GetAllocationResponse> { let data; try { data = await queryAllocation({ address, }); } catch (error) { console.error('Error querying allocation:', error); // Return a default or error response return { all: [], assets: [], available: [], totalCap: new PricePretty(DEFAULT_VS_CURRENCY, new Dec(0)), hasVariants: false, }; } // Rest of the function remains the same }This change will improve the robustness of the function by gracefully handling potential errors from
queryAllocation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/server/src/queries/complex/portfolio/allocation.ts (3 hunks)
- packages/web/hooks/use-has-asset-variants.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/hooks/use-has-asset-variants.tsx
🧰 Additional context used
🔇 Additional comments (1)
packages/server/src/queries/complex/portfolio/allocation.ts (1)
3-3: LGTM: Import statement updated correctly.The addition of the
Assettype to the import statement is appropriate and necessary for the new functionality introduced in this file.
| const userBalanceDenoms = | ||
| categories["user-balances"]?.account_coins_result?.map( | ||
| (result) => result.coin.denom | ||
| ) ?? []; | ||
|
|
||
| // check for asset variants, alloys and canonical assets such as USDC | ||
| const hasVariants = checkHasAssetVariants( | ||
| userBalanceDenoms, | ||
| assetLists.flatMap((list) => list.assets) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Asset variant detection logic added correctly.
The changes in the getAllocation function effectively implement the asset variant detection logic. The extraction of user balance denoms and the call to checkHasAssetVariants are implemented correctly.
Consider memoizing the result of assetLists.flatMap((list) => list.assets) if assetLists is not expected to change frequently. This could improve performance, especially for large asset lists:
const memoizedAssetListAssets = useMemo(() => assetLists.flatMap((list) => list.assets), [assetLists]);
// Then use memoizedAssetListAssets in the checkHasAssetVariants callThis optimization would be particularly beneficial if getAllocation is called multiple times with the same assetLists.
Also applies to: 187-187
What is the purpose of the change:
Linear Task
https://linear.app/osmosis/issue/FE-725/porfolio-v3-variants-detected-popup
Brief Changelog
Testing and Verifying
add jest test cases for
screenshot
example on any page
variants.detected.logic.mov
local storage persistence example
variants.detected.logic.local.storage.mov