ui fixes after QA review - PRO-3679/pulse-preview-screen#403
ui fixes after QA review - PRO-3679/pulse-preview-screen#403
Conversation
WalkthroughAdds a small-number formatter and native-token address normalizer, applies them across Buy/Sell/Search UIs and lists, updates tooltip fade-in gating, and adjusts related tests to match revised formatting and labels. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant HoverTarget as Hover Target
participant Tooltip as Tooltip Component
participant Positioner as calculatePosition()
User->>HoverTarget: mouseenter
HoverTarget->>Tooltip: set isVisible = true
Tooltip->>Positioner: calculatePosition()
Positioner-->>Tooltip: {x,y}
Tooltip->>Tooltip: set isPositioned = true (opacity 0 -> 1)
User->>HoverTarget: mouseleave
HoverTarget->>Tooltip: set isVisible = false
Tooltip->>Tooltip: set isPositioned = false (opacity 1 -> 0)
sequenceDiagram
autonumber
participant UI as PreviewBuy/PreviewSell/Sell/Search
participant BC as blockchain.utils
participant NUM as number.utils
UI->>BC: formatNativeTokenAddress(address)
BC-->>UI: normalizedAddress
UI->>UI: truncate & render normalizedAddress
UI->>NUM: formatExponentialSmallNumber(limitDigitsNumber(value))
NUM-->>UI: formattedString
UI->>UI: render price / balance / usdValue / propagated amounts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
Deploying x with
|
| Latest commit: |
bdacf08
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://93419b86.x-e62.pages.dev |
| Branch Preview URL: | https://pro-3679-pulse-preview-scree.x-e62.pages.dev |
![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
🧹 Nitpick comments (1)
src/utils/number.tsx (1)
1-1: Fix global property shadowing.The import is shadowing global properties
isNaNandparseInt. Consider using aliases to avoid confusion.Apply this diff to resolve the shadowing:
-import { isNaN, parseInt, parseInt as parseIntLodash } from 'lodash'; +import { isNaN as isNaNLodash, parseInt as parseIntLodash } from 'lodash';Also update line 126 to use the alias:
- if (isNaN(numValue)) return '0'; + if (isNaNLodash(numValue)) return '0';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/apps/pulse/components/Buy/PreviewBuy.tsx(2 hunks)src/apps/pulse/components/Misc/Tooltip.tsx(4 hunks)src/apps/pulse/components/Search/PortfolioTokenList.tsx(2 hunks)src/apps/pulse/components/Search/Search.tsx(4 hunks)src/apps/pulse/components/Sell/PreviewSell.tsx(2 hunks)src/apps/pulse/components/Sell/Sell.tsx(4 hunks)src/apps/pulse/utils/blockchain.ts(1 hunks)src/utils/number.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Search/Search.tsxsrc/apps/pulse/components/Search/PortfolioTokenList.tsx
📚 Learning: 2025-09-09T12:40:15.629Z
Learnt from: RanaBug
PR: pillarwallet/x#391
File: src/apps/pulse/components/Sell/Sell.tsx:113-130
Timestamp: 2025-09-09T12:40:15.629Z
Learning: In the Pulse app Sell component, when a user changes/switches tokens, the input amount automatically resets to 0, which means liquidity validation state doesn't become stale when tokens change.
Applied to files:
src/apps/pulse/components/Sell/Sell.tsxsrc/apps/pulse/components/Sell/PreviewSell.tsxsrc/apps/pulse/components/Buy/PreviewBuy.tsx
🧬 Code graph analysis (5)
src/apps/pulse/components/Sell/Sell.tsx (1)
src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/apps/pulse/components/Sell/PreviewSell.tsx (1)
src/apps/pulse/utils/blockchain.ts (1)
formatNativeTokenAddress(1-9)
src/apps/pulse/components/Search/Search.tsx (1)
src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/apps/pulse/components/Search/PortfolioTokenList.tsx (1)
src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
src/apps/pulse/utils/blockchain.ts (1)
formatNativeTokenAddress(1-9)
🪛 Biome (2.1.2)
src/utils/number.tsx
[error] 1-1: Do not shadow the global "isNaN" 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)
[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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (15)
src/apps/pulse/utils/blockchain.ts (1)
1-9: LGTM!The address formatting utility is well-implemented and serves a clear purpose - normalizing the ETH placeholder address to the zero address for consistent display.
src/apps/pulse/components/Sell/PreviewSell.tsx (2)
8-8: Consistent address formatting implementation.The import of
formatNativeTokenAddressaligns with the PR's goal of standardizing address display across components.
483-490: Enhanced token display with consistent formatting.The changes improve the UI by:
- Displaying token name and symbol inline with proper styling
- Using
formatNativeTokenAddressto normalize addresses before truncationThis ensures consistent address formatting across buy/sell previews.
src/apps/pulse/components/Buy/PreviewBuy.tsx (2)
16-16: Consistent with the PR's address formatting pattern.The import follows the same pattern as PreviewSell.tsx, ensuring consistent address handling across components.
421-427: Dynamic token label with proper address formatting.The changes improve the UI by:
- Using dynamic
buyToken.nameinstead of hard-coded "USD Coin"- Applying
formatNativeTokenAddressbefore truncation for consistent displayThis matches the pattern used in PreviewSell.tsx for consistent UX.
src/apps/pulse/components/Misc/Tooltip.tsx (3)
10-10: Improved tooltip fade-in behavior.The addition of
isPositionedstate creates a smooth fade-in effect by ensuring the tooltip is positioned before becoming visible. This prevents the flash of incorrectly positioned content.Also applies to: 67-67, 76-76
80-83: Simplified positioning logic.The removal of the
setTimeoutdelay in favor of immediate position calculation whenisVisible && tooltipRef.currentis present improves responsiveness while maintaining correct positioning.
104-106: Clean fade transition implementation.The inline opacity and transition styles create a polished fade effect that enhances the user experience.
src/apps/pulse/components/Search/Search.tsx (2)
18-21: Updated import for consistent numeric formatting.The import change to include
formatExponentialSmallNumbersupports the PR's goal of standardizing numeric displays across the application.
213-215: Consistent numeric formatting across buy/sell flows.The replacement of direct
limitDigitsNumberusage withformatExponentialSmallNumber(limitDigitsNumber(...))ensures consistent formatting of USD values, preventing scientific notation display for small numbers.Also applies to: 227-229, 241-243
src/apps/pulse/components/Search/PortfolioTokenList.tsx (2)
13-16: Import aligns with numeric formatting standardization.The addition of
formatExponentialSmallNumberto the imports supports consistent number display formatting.
201-206: Enhanced numeric formatting for better readability.The implementation improves the UI by:
- Using
formatExponentialSmallNumberto prevent scientific notation in price/balance displays- Rendering the dollar sign as a separate element for cleaner formatting
- Maintaining the '0.00' fallback for missing prices
Also applies to: 214-214, 217-219
src/utils/number.tsx (1)
119-149: Well-implemented small number formatter.The
formatExponentialSmallNumberfunction effectively handles the common issue of scientific notation in JavaScript number displays. The implementation:Based on the search results, I can see that the `formatExponentialSmallNumber` implementation follows established best practices for handling JavaScript's scientific notation issues. The approach of using `toFixed()` and manual reconstruction is a well-documented solution for preventing scientific notation display, and the solution handles both very large and very small numbers appropriately.
- Properly handles edge cases (zero, NaN, falsy values)
- Uses appropriate threshold (0.0001) for switching to custom formatting
- Correctly reconstructs decimal strings from scientific notation
- Provides reasonable default for
maxDecimalssrc/apps/pulse/components/Sell/Sell.tsx (2)
16-19: Updated import for consistent numeric formatting.The import change supports the PR's standardization of numeric displays across the application, replacing direct
limitDigitsNumberusage with enhanced formatting.
212-212: Comprehensive numeric formatting improvements.The changes enhance user experience by:
- Using
formatExponentialSmallNumberfor token USD values and balances to prevent scientific notation- Applying consistent formatting to percentage/MAX button calculations before state updates
- Ensuring formatted strings are properly propagated to parent components
This provides a consistent, user-friendly numeric display throughout the sell interface.
Also applies to: 300-306, 342-345, 351-354
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (1)
305-313: Make the assertion resilient to markup changesCounting exactly 6 nodes for this composite text is brittle. Prefer a semantic or threshold-based check, or scope via a stable container/testid to avoid incidental duplicates.
Apply this minimal, more resilient change:
- expect( - screen.getAllByText((content, element) => { - return !!( - element?.textContent?.includes('10.500000') && - element?.textContent?.includes('TEST') - ); - }) - ).toHaveLength(6); + const matches = screen.getAllByText((_, element) => + Boolean( + element?.textContent?.includes('10.500000') && + element?.textContent?.includes('TEST') + ) + ); + expect(matches.length).toBeGreaterThanOrEqual(5);If possible, add stable testids to the key, user-visible instances (e.g., sell amount row, summary tile) and assert those explicitly to avoid count-based assertions altogether.
src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx (2)
87-87: Make the assertion resilient to copy changes.Prefer referencing the fixture to avoid hard‑coded strings.
- expect(screen.getAllByText('USD Coin')).toHaveLength(1); + expect(screen.getAllByText(mockPayingToken.name)).toHaveLength(1);
83-93: Also assert the presence of the dynamic buy token name.This locks in the intended switch from hard‑coded to dynamic labeling.
render(<PreviewBuy {...mockProps} />); expect(screen.getByText('Confirm Transaction')).toBeInTheDocument(); expect(screen.getAllByText('USD Coin')).toHaveLength(1); + expect(screen.getByText(mockToken.name)).toBeInTheDocument(); expect(screen.getAllByText('TEST')).toHaveLength(2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/apps/pulse/components/Buy/tests/__snapshots__/PreviewBuy.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Sell/tests/__snapshots__/PreviewSell.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Sell/tests/__snapshots__/Sell.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx(1 hunks)src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx(1 hunks)src/apps/pulse/components/Sell/tests/Sell.test.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/apps/pulse/components/Sell/tests/Sell.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (2)
src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx (2)
87-87: LGTM: count reduced to 1 matches the new dynamic buyToken label.Now that the bought token label uses
buyToken.name, "USD Coin" should only appear for the paying token.
22-24: Add a mock for formatNativeTokenAddress (keep the existing getLogoForChainId mock)PreviewBuy imports getLogoForChainId from '../../../../utils/blockchain' (already mocked) and formatNativeTokenAddress from '../../utils/blockchain'. The test mock only covers getLogoForChainId — add a mock for '../../utils/blockchain' in src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx, e.g.
vi.mock('../../utils/blockchain', () => ({ formatNativeTokenAddress: (addr: string) => addr }));
Likely an incorrect or invalid review comment.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Style
Tests