Closed zed-wong closed 6 months ago
vercel[bot]
commented
6 months ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| mr-market | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Apr 4, 2024 0:34am |
railway-app[bot]
commented
6 months ago This PR is being deployed to Railway 🚅
railway-app[bot]
commented
6 months ago This PR is being deployed to Railway 🚅
Mr.Market: ◻️ REMOVED
github-actions[bot]
commented
6 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/2d7be3d482542bbdeb49fd3f4187a57ef000c57b)
github-actions[bot]
commented
6 months ago | ⏱️ Estimated effort to review [1-5] | 4, due to the extensive changes across multiple files, including UI components, utility functions, and backend services. The PR touches on various aspects such as UI enhancements, localization updates, and backend service refactoring, which requires a thorough review to ensure consistency and correctness across the application. |
| 🧪 Relevant tests | No |
| 🔍 Possible issues | Possible Bug: The removal of the "favorites" and "mainstream" tabs without corresponding backend or state management updates could lead to unexpected behavior or errors if those categories are still referenced elsewhere in the code. |
| Performance Concern: The addition of utility functions like `handleCoingeckoTokenName` directly in the UI component rendering path could lead to performance issues, especially if these functions are called frequently in large lists or tables. | |
| 🔒 Security concerns | No |
| relevant file | interface/src/lib/helpers/utils.ts |
| suggestion | Consider memoizing the `handleCoingeckoTokenName` function to avoid recalculating the token name for the same input multiple times, which can improve performance in components that render large lists of tokens. [important] |
| relevant line | export const handleCoingeckoTokenName = (name: string) => { |
| relevant file | interface/src/lib/components/market/token/singlePair.svelte |
| suggestion | Ensure that `formatDecimals` function handles cases where the number is very large or very small, to avoid displaying numbers in scientific notation in the UI. [important] |
| relevant line | {`${formatDecimals(pair.last, 8)} ${handleCoingeckoTokenName(pair.target)}`} |
| relevant file | interface/src/routes/(bottomNav)/(market)/market/spot/+page.svelte |
| suggestion | Refactor the filtering logic to a utility function to improve code readability and maintainability. This function can take the `defaultsPairs` and `activeSpotTab` as inputs and return the filtered pairs. [medium] |
| relevant line | $: pairs = $activeSpotTab === 0 ? defaultsPairs : defaultsPairs.filter(item => item.exchange === exchangeMap[$activeSpotTab]); |
| relevant file | interface/src/lib/components/market/elements/secondTabs.svelte |
| suggestion | Add a comment explaining the purpose of the `onDestroy` hook in this context, especially why it's necessary to reset `activeCoinTab` and `activeSpotTab` to 0. This will help future maintainers understand the intention behind this logic. [medium] |
| relevant line | onDestroy(()=>{ |
github-actions[bot]
commented
6 months ago | Category | Suggestions |
| Maintainability |
Remove commented-out code related to the "favorites" tab.___ **Consider removing the commented-out code related to the "favorites" tab inCoinsTypeTabs unless there's a clear intention to re-enable it in the near future. Keeping commented-out code can lead to confusion and clutter in the codebase.** [interface/src/lib/helpers/constants.ts [299-302]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-1d9500711f0f58654b9e0e95aa0e7fdc798a0b74f1c2310b09e52123e52d6bf2R299-R302) ```diff -// { -// name: "favorites", -// id: "favorites", -// }, +// Code related to "favorites" tab removed. ``` |
Refactor the filtering logic for
___
**Refactor the filtering logic for | |
Maintain consistency in quotation marks usage within class attributes.___ **Ensure consistent use of quotation marks for class names to maintain code consistency andreadability.** [interface/src/lib/components/market/token/singlePair.svelte [14]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-e71c158df546b173f765b294e2772760c5fc40ed9d1b8acddbee52575dc6bd6dR14-R14) ```diff -
+
```
| |
| Enhancement |
Use ellipsis character for truncating token names.___ **For thehandleCoingeckoTokenName function, consider using ellipsis (…) instead of three dots ( ...) for a more semantically correct representation when truncating token names.**
[interface/src/lib/helpers/utils.ts [267]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-cca6fb1729226e759cd4dba0b09a9c83f7f109709c77fd68c77feaf0ba512380R267-R267)
```diff
-return `${name.slice(0,6)}...`
+return `${name.slice(0,6)}…`
```
|
Validate
___
**Consider validating the | |
Improve code readability and performance by destructuring the
___
**Consider destructuring | |
Simplify the assignment of
___
**Use optional chaining to simplify the expression for setting the `state` variable.**
[interface/src/lib/components/spot/history/orderDetail.svelte [8]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-f23b2cd24acc1ec41f0fb4400075be6ed4ca2249c023bd60333e00cc92b420acR8-R8)
```diff
-$: state = o ? o.state : 'ORDER_CREATED';
+$: state = o?.state || 'ORDER_CREATED';
```
| |
| Possible issue |
Verify
___
**Since the |
Review
___
**Ensure that resetting the | |
| Best practice |
Improve code organization by using a named function for page reload.___ **Use a reactive statement to handle the reloading of the page instead of an inline functionin the on:click event.**
[interface/src/routes/(bottomNav)/(market)/market/token/+page.svelte [42]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-19c0a8d872890887d2cdc3e2396e8d574271959ac474c6ba1f65fc9ae81792e4R42-R42)
```diff
- |
github-actions[bot]
commented
6 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/1f8e01771788f5d64ecdbddb67f8958696ee83bf)
github-actions[bot]
commented
6 months ago Persistent review updated to latest commit https://github.com/Hu-Fi/Mr.Market/commit/1f8e01771788f5d64ecdbddb67f8958696ee83bf
github-actions[bot]
commented
6 months ago | Category | Suggestions |
| Maintainability |
Remove commented-out code if it's no longer needed.___ **Consider removing the commented-out code related to the "favorites" tab if it's no longerneeded. Keeping commented-out code can lead to confusion and clutter in the codebase.** [interface/src/lib/helpers/constants.ts [299-302]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-1d9500711f0f58654b9e0e95aa0e7fdc798a0b74f1c2310b09e52123e52d6bf2R299-R302) ```diff -// { -// name: "favorites", -// id: "favorites", -// }, +// Code related to "favorites" tab removed. ``` |
Add a comment explaining the default value of
___
**Since | |
Improve readability by using descriptive class names.___ **Consider using a more descriptive class name instead of an empty string for betterreadability and maintainability.** [interface/src/lib/components/market/token/singlePair.svelte [15]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-e71c158df546b173f765b294e2772760c5fc40ed9d1b8acddbee52575dc6bd6dR15-R15) ```diff -
+
```
| |
| Best practice |
Use more descriptive variable names.___ **To improve readability and maintainability, consider using a more descriptive variablename instead of name for the function parameter. For example, tokenName would make it clearer that the function expects the name of a token.** [interface/src/lib/helpers/utils.ts [265]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-cca6fb1729226e759cd4dba0b09a9c83f7f109709c77fd68c77feaf0ba512380R265-R265) ```diff -export const handleCoingeckoTokenName = (name: string) => { +export const handleCoingeckoTokenName = (tokenName: string) => { ``` |
Log errors before throwing them for better debugging.___ **To enhance error handling, consider logging the error before throwing it. This can helpwith debugging and provide more context about the error's origin.** [server/src/modules/coingecko/coingecko.service.ts [67-69]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-fc8069e1fb6be7ac5ca3b7e95e0de665542438306d2ea1ac330338bb91117b42R67-R69) ```diff +console.error(`Error fetching /coins/market for ${vs_currency}: ${error}`); throw new Error( `markets/${vs_currency} Failed to GET /coins/market: ${error.message}`, ); ``` | |
Use constants for tab indices to improve code readability.___ **For better maintainability and to avoid magic numbers, consider defining constants for thetab indices (0 for 'token', 1 for 'spot') used in the active variable assignment. This will make the code more readable and easier to understand.** [interface/src/lib/components/market/elements/secondTabs.svelte [19-20]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-028779b1de0a34a553b8c4d06b2f5416e3852961a5d0caa87066cd3b43cad9f2R19-R20) ```diff -$: active = $page.url.pathname.includes(MARKET_BAR_ITEMS[0].key) ? 0 : - $page.url.pathname.includes(MARKET_BAR_ITEMS[1].key) ? 1 : 0 +const TOKEN_TAB_INDEX = 0; +const SPOT_TAB_INDEX = 1; +$: active = $page.url.pathname.includes(MARKET_BAR_ITEMS[TOKEN_TAB_INDEX].key) ? TOKEN_TAB_INDEX : + $page.url.pathname.includes(MARKET_BAR_ITEMS[SPOT_TAB_INDEX].key) ? SPOT_TAB_INDEX : TOKEN_TAB_INDEX; ``` | |
Use optional chaining for safer property access.___ **To handle potential null or undefined values more gracefully, consider using optionalchaining ( ?.) when accessing properties of o.**
[interface/src/lib/components/spot/history/orderDetail.svelte [8]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-f23b2cd24acc1ec41f0fb4400075be6ed4ca2249c023bd60333e00cc92b420acR8-R8)
```diff
-$: state = o ? o.state : 'ORDER_CREATED';
+$: state = o?.state ?? 'ORDER_CREATED';
```
| |
Use an enum for exchange identifiers for better type safety.___ **To ensure type safety and maintainability, consider defining an enum for exchangeidentifiers instead of using a plain object.** [interface/src/routes/(bottomNav)/(market)/market/spot/+page.svelte [12-18]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-7ba2839ad827bde33f4463d7a9f5a0e8cf605545b6a4dacdde0d65eb52a7cd05R12-R18) ```diff -const exchangeMap: { [key: number]: string} = { - 1: "okx", - 2: "bitfinex", - 3: "mexc", - 4: "gate", - 5: "lbank" -}; +enum Exchange { + OKX = 1, + Bitfinex = 2, + MEXC = 3, + Gate = 4, + LBank = 5 +} ``` | |
| Performance |
Optimize performance by simplifying the condition for sorting tokens.___ **For better performance, avoid using$marketData || $marketDataState === 'loading' in the ternary operation for sortedTokens as it causes unnecessary computations. Instead, directly check $marketDataState.**
[interface/src/routes/(bottomNav)/(market)/market/token/+page.svelte [12]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-19c0a8d872890887d2cdc3e2396e8d574271959ac474c6ba1f65fc9ae81792e4R12-R12)
```diff
-$: sortedTokens = sortCoins($selectedField, !$marketData || $marketDataState === 'loading' ? tokens : $marketData, $asc)
+$: sortedTokens = $marketDataState === 'loading' ? tokens : sortCoins($selectedField, $marketData, $asc)
```
|
| Enhancement |
Disable the "Retry" button after click to improve user experience.___ **To improve user experience, consider disabling the "Retry" button after it's clicked toprevent multiple reloads and indicate that an action is being taken.** [interface/src/routes/(bottomNav)/(market)/market/token/+page.svelte [42]](https://github.com/Hu-Fi/Mr.Market/pull/130/files#diff-19c0a8d872890887d2cdc3e2396e8d574271959ac474c6ba1f65fc9ae81792e4R42-R42) ```diff - |
User description
Fix #119 filter in market page
Type
bug_fix, enhancement
Description
Changes walkthrough
11 files
constants.ts
Update Coin Type Tabs and Rename Inscriptioninterface/src/lib/helpers/constants.ts
CoinsTypeTabs.CoinsTypeTabs.utils.ts
Add Utility Function for Token Name Formattinginterface/src/lib/helpers/utils.ts
handleCoingeckoTokenNamefunction to shorten token namescontaining '0X'.
market.ts
Refactor Active Tab Store for Marketinterface/src/lib/stores/market.ts
Writabletype and used it foractiveSpotTab.activeSecondTabtoactiveSpotTab.coingecko.controller.ts
Refactor Coingecko Controller for Market Categoryserver/src/modules/coingecko/coingecko.controller.ts
getCoinMarketsByCategorymethod for better readability.coingecko.service.ts
Refactor Coingecko Service for Market Data Fetchingserver/src/modules/coingecko/coingecko.service.ts
coinsMarketsmethod for better readability and errorhandling.
en.json
Update English Localization for Inscriptionsinterface/src/i18n/en.json - Renamed "inscription" to "inscriptions".
zh.json
Update Chinese Localization for Inscriptionsinterface/src/i18n/zh.json - Renamed "inscription" to "inscriptions" in Chinese localization.
secondTabs.svelte
Refactor Market Second Tabs Componentinterface/src/lib/components/market/elements/secondTabs.svelte
activeCoinTabandactiveSpotTab.singlePair.svelte
Improve Token Pair Display Formattinginterface/src/lib/components/market/token/singlePair.svelte
handleCoingeckoTokenNamefor token name formatting.formatDecimalsfor formatting pair's last price.+page.svelte
Simplify Spot Market Pairs Filtering Logicinterface/src/routes/(bottomNav)/(market)/market/spot/+page.svelte
+page.svelte
Improve Error Handling and Loading UI in Token Market Pageinterface/src/routes/(bottomNav)/(market)/market/token/+page.svelte
TokenFilterLoaderwithFilterLoader.1 files
home.ts
Change Default Active Coin Tabinterface/src/lib/stores/home.ts - Changed default value of `activeCoinTab` from 1 to 0.
1 files
coingecko.service.spec.ts
Refactor Coingecko Service Test for Market Dataserver/src/modules/coingecko/coingecko.service.spec.ts - Refactored test for `coinsMarkets` method for better readability.
2 files
tabs.svelte
Reset Active Coin Tab on Destroy in Home Market Tabsinterface/src/lib/components/home/markets/tabs.svelte - Added `onDestroy` lifecycle to reset `activeCoinTab`.
orderDetail.svelte
Add Default State Handling in Order Detailinterface/src/lib/components/spot/history/orderDetail.svelte - Added default state for order detail component.
1 files
tokenFilterLoader.svelte
Remove Unused Token Filter Loader Skeletoninterface/src/lib/components/skeleton/market/tokenFilterLoader.svelte - Removed unused skeleton loader component.