General fixes #3981
General fixes #3981
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThis pull request consists of extensive formatting and style adjustments across multiple files in the codebase. The changes primarily involve standardizing string quotation marks from double quotes to single quotes, removing unnecessary semicolons in property declarations and method implementations, and modifying the structure of various components and test cases for consistency. No alterations to the underlying logic, functionality, or control flow have been made; the focus is strictly on improving code readability and maintaining stylistic uniformity. 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/one-click-trading/use-one-click-trading-swap-review.tsOops! Something went wrong! :( ESLint: 8.50.0 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 0
🧹 Outside diff range and nitpick comments (4)
packages/web/components/one-click-trading/one-click-remaining-time.tsx (1)
58-63: LGTM! Consider adding a comment explaining the delimiter choice.The addition of the
delimitedByparameter improves the readability of time display when using short units. This change aligns well with fixing the misleading countdown phrasing mentioned in the PR objectives.Consider adding a brief comment explaining why space delimiter is used for short units:
{displayHumanizedTime({ humanizedTime, t, + // Use space delimiter for short units (e.g., "1 h 30 m") for better readability delimitedBy: useShortTimeUnits ? " " : undefined, })}{" "} {t("remaining")}packages/web/stores/__tests__/transfer-history.spec.tsx (1)
91-93: Consider adding more test cases for time transitionsWhile the test verifies the "Unknown time remaining" state, consider adding test cases for:
- Edge case when time transitions from known to unknown
- Different time ranges (minutes to seconds)
it("updates the time remaining text", () => { const estimatedArrivalUnix = dayjs().add(5, "minutes").unix(); render( <PendingTransferCaption isWithdraw={true} amount="10 OSMO" chainPrettyName="Osmosis" estimatedArrivalUnix={estimatedArrivalUnix} /> ); jest.advanceTimersByTime(300000); // Advance time by 5 minutes expect(screen.getByText(/Unknown time remaining/)).toBeInTheDocument(); + + // Test transition from known to unknown time + const transitionEstimatedArrival = dayjs().add(2, "seconds").unix(); + render( + <PendingTransferCaption + isWithdraw={true} + amount="10 OSMO" + chainPrettyName="Osmosis" + estimatedArrivalUnix={transitionEstimatedArrival} + /> + ); + + expect(screen.getByText(/2 seconds remaining/)).toBeInTheDocument(); + jest.advanceTimersByTime(1500); + expect(screen.getByText(/Unknown time remaining/)).toBeInTheDocument(); });packages/web/stores/transfer-history.tsx (1)
431-432: LGTM: Improved time remaining display logicThe change from 5 seconds to 1 second threshold provides more accurate feedback to users and aligns with the PR objective of fixing misleading countdown phrasing.
Consider adding a constant for the threshold value to make it more maintainable:
+const TIME_REMAINING_THRESHOLD_SECONDS = 1; + export const PendingTransferCaption: FunctionComponent<{ // ... }> = ({ isWithdraw, amount, chainPrettyName, estimatedArrivalUnix }) => { // ... const updateTime = () => { // ... if (progressRef.current) { progressRef.current.textContent = - date.diff(dayjs(), "seconds") < 1 + date.diff(dayjs(), "seconds") < TIME_REMAINING_THRESHOLD_SECONDS ? t("unknownTimeRemaining") : `${t("estimated")} ${displayHumanizedTime({packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts (1)
250-258: Consider simplifying the loading state logic.While the loading state logic is now more comprehensive and fixes the unclickable button issue, the nested ternary expression could be simplified for better readability.
Consider refactoring the loading state calculation like this:
- const isLoading = shouldSend1CTTx - ? isLoadingOneClickMessages || - (shouldFetchExistingSessionAuthenticator && - !sessionAuthenticatorError && - enabledFetchingSessionAuthenticator - ? isLoadingSessionAuthenticator - : false) || - isLoadingInfo - : false; + const isLoadingSessionAuth = + shouldFetchExistingSessionAuthenticator && + !sessionAuthenticatorError && + enabledFetchingSessionAuthenticator && + isLoadingSessionAuthenticator; + + const isLoading = shouldSend1CTTx + ? isLoadingOneClickMessages || isLoadingSessionAuth || isLoadingInfo + : false;Also applies to: 263-263
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (17)
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!**/*.json
📒 Files selected for processing (25)
packages/e2e/pages/pool-page.ts(1 hunks)packages/e2e/pages/pools-page.ts(1 hunks)packages/e2e/pages/portfolio-page.ts(1 hunks)packages/e2e/pages/swap-page.ts(1 hunks)packages/e2e/pages/trade-page.ts(1 hunks)packages/e2e/pages/transactions-page.ts(1 hunks)packages/e2e/tests/claim.spec.ts(1 hunks)packages/e2e/tests/monitoring.limit.wallet.spec.ts(1 hunks)packages/e2e/tests/monitoring.market.wallet.spec.ts(1 hunks)packages/e2e/tests/monitoring.swap.wallet.spec.ts(1 hunks)packages/e2e/tests/pools.spec.ts(1 hunks)packages/e2e/tests/portfolio.wallet.spec.ts(1 hunks)packages/e2e/tests/select.spec.ts(1 hunks)packages/e2e/tests/swap.wallet.spec.ts(1 hunks)packages/e2e/tests/trade.wallet.spec.ts(1 hunks)packages/e2e/tests/transactions.wallet.spec.ts(1 hunks)packages/web/components/complex/portfolio/assets-overview.tsx(1 hunks)packages/web/components/nomic/nomic-pending-transfers.tsx(6 hunks)packages/web/components/one-click-trading/one-click-remaining-time.tsx(1 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-session.ts(1 hunks)packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts(2 hunks)packages/web/modals/profile.tsx(1 hunks)packages/web/server/api/routers/bridge-transfer.ts(1 hunks)packages/web/stores/__tests__/transfer-history.spec.tsx(2 hunks)packages/web/stores/transfer-history.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (17)
- packages/e2e/tests/monitoring.swap.wallet.spec.ts
- packages/e2e/tests/portfolio.wallet.spec.ts
- packages/e2e/tests/monitoring.market.wallet.spec.ts
- packages/e2e/tests/claim.spec.ts
- packages/web/modals/profile.tsx
- packages/e2e/tests/monitoring.limit.wallet.spec.ts
- packages/e2e/tests/select.spec.ts
- packages/e2e/tests/pools.spec.ts
- packages/e2e/tests/transactions.wallet.spec.ts
- packages/e2e/tests/trade.wallet.spec.ts
- packages/e2e/pages/pool-page.ts
- packages/e2e/tests/swap.wallet.spec.ts
- packages/e2e/pages/portfolio-page.ts
- packages/e2e/pages/transactions-page.ts
- packages/e2e/pages/pools-page.ts
- packages/e2e/pages/swap-page.ts
- packages/e2e/pages/trade-page.ts
🧰 Additional context used
📓 Learnings (1)
packages/web/hooks/one-click-trading/use-one-click-trading-session.ts (1)
Learnt from: greg-nagy
PR: osmosis-labs/osmosis-frontend#3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-session.ts:89-92
Timestamp: 2024-11-22T11:02:18.859Z
Learning: In the `useOneClickTradingSession` hook within `packages/web/hooks/one-click-trading/use-one-click-trading-session.ts`, storing the expired flag in the store is not desirable as it introduces unnecessary persistence without real usage. The `isExpired` status should be calculated rather than stored.
🔇 Additional comments (11)
packages/web/components/one-click-trading/one-click-remaining-time.tsx (1)
58-63: Verify consistent time display formatting across the application.
Let's ensure this time display formatting change is consistent with other uses of displayHumanizedTime in the codebase.
✅ Verification successful
Time display formatting is consistent across the application
The verification shows that the displayHumanizedTime function in packages/web/utils/date.ts is designed to handle an optional delimitedBy parameter, and all usages across the codebase are consistent with this implementation:
- Most usages pass only
humanizedTimeandtparameters, defaulting to the " and " delimiter - The one-click-trading component specifically uses the optional
delimitedByparameter for short format - All implementations correctly use the function signature and expected parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of displayHumanizedTime to verify consistency
rg "displayHumanizedTime" --type ts --type tsx -B 2 -A 2
Length of output: 84
Script:
#!/bin/bash
# Search for other uses of displayHumanizedTime using ripgrep without file type restrictions
rg "displayHumanizedTime" -B 2 -A 2
Length of output: 5518
Script:
#!/bin/bash
# Find the implementation of displayHumanizedTime function using ast-grep
ast-grep --pattern 'function displayHumanizedTime($_) {
$$$
}'
Length of output: 779
Script:
#!/bin/bash
# Find the file containing displayHumanizedTime implementation
fd "displayHumanizedTime"
Length of output: 25
packages/web/stores/__tests__/transfer-history.spec.tsx (1)
24-26: LGTM: Translation key addition aligns with UI improvements
The addition of the unknownTimeRemaining translation key supports the improved handling of transfer progress display.
packages/web/hooks/one-click-trading/use-one-click-trading-session.ts (1)
106-107: LGTM: Improved null handling for isEnabled flag
The addition of the null coalescing operator ?? provides better handling of undefined values, making the code more robust.
packages/web/components/nomic/nomic-pending-transfers.tsx (3)
104-110: LGTM: Clean state management setup
The use of useShallow with useNomicTransactionsStore is a good practice for optimizing re-renders.
141-145: Performance improvement with memoized filtering
Good use of useMemo to prevent unnecessary re-filtering of transactions. The filtering by userOsmoAddress ensures that only relevant transactions are displayed.
178-231: Verify transaction display logic
The transaction display logic looks correct, but let's verify the calculations and display of fees.
✅ Verification successful
Based on the search results, I can now generate the final response:
Fee calculation logic is correctly implemented
The fee calculations in the pending transfers display are properly implemented and consistent with the rest of the codebase:
- The network and provider fee subtractions are handled consistently with the bridge transfer data model (
packages/bridge/src/interface.ts) - The fee calculations follow the same pattern across different components that handle transfers
- Both fiat value and amount calculations properly account for network and provider fees using the
sub()operation - The display format matches the localization patterns found in the translation files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent fee calculation patterns across the codebase
# Look for similar fee calculations
ast-grep --pattern 'fiatValue?.sub($_, $_)'
# Check for other instances of network and provider fee handling
rg -A 2 'networkFee|providerFee' --type typescript
Length of output: 133
Script:
#!/bin/bash
# Search for fee-related calculations in TypeScript/JavaScript files
rg -t js -t jsx -t ts -t tsx "networkFee|providerFee" -A 2
# Look for similar fee calculations with optional chaining
rg -t js -t jsx -t ts -t tsx "fiatValue\?\.sub\(" -A 2
# Find other instances of fee calculations in the codebase
ast-grep --pattern 'amount.sub($_, $_)'
Length of output: 215
Script:
#!/bin/bash
# Search for fee-related calculations using ripgrep without file type specifications
rg "networkFee|providerFee" -A 2
# Look for similar fee calculations with optional chaining
rg "fiatValue\?\.sub\(" -A 2
# Search for amount subtraction patterns
rg "amount\.sub\(" -A 2
Length of output: 48907
packages/web/server/api/routers/bridge-transfer.ts (1)
738-738: LGTM: Added user address tracking to deposits
Adding userOsmoAddress to the pending deposits response is a good addition that enables proper filtering of transactions per user in the frontend.
packages/web/components/complex/portfolio/assets-overview.tsx (1)
160-160: LGTM: Default portfolio view successfully changed to 7 days
The change correctly implements the PR objective by updating the default portfolio view from 1 day to 7 days. The implementation is type-safe and maintains all component functionality.
packages/web/hooks/one-click-trading/use-one-click-trading-swap-review.ts (3)
164-166: LGTM! Clear and descriptive variable extraction.
The introduction of enabledFetchingSessionAuthenticator improves code readability by consolidating the conditions for session authenticator fetching into a well-named variable.
170-170: LGTM! Enhanced error handling.
The addition of sessionAuthenticatorError improves error handling and prevents the UI from getting stuck in loading states when session authentication fails.
177-177: LGTM! Simplified query configuration.
Using the extracted enabledFetchingSessionAuthenticator variable improves the readability of the query configuration.
What is the purpose of the change:
Linear Task
https://linear.app/osmosis/issue/FE-1262/fix-incorrect-deposit-status-display-for-different-wallets
https://linear.app/osmosis/issue/FE-1256/fix-misleading-countdown-phrasing-for-withdrawal-progress
https://linear.app/osmosis/issue/FE-1263/confirm-swap-button-unclickable-when-1ct-is-enabled
https://linear.app/osmosis/issue/FE-1264/fix-transfers-disappearing-after-1-day
https://linear.app/osmosis/issue/FE-1255/change-default-osmosis-portfolio-view-from-1d-to-7d