Transaction Status for Buy flow on feat/PRO-3681/transaction-status-pulse#411
Transaction Status for Buy flow on feat/PRO-3681/transaction-status-pulse#411
Conversation
WalkthroughAdds a unified client-side transaction status system: HomeScreen now manages polling for Buy (bid + resource lock) and Sell (user-op) flows, maps external statuses to a shared TransactionStatus UI, and introduces new transaction components, hooks, types, utils, tests, and a CloseButton. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Home as HomeScreen
participant UI as TransactionStatus (UI)
participant API as Backend
rect rgba(240,248,255,1)
note right of User: Buy flow (shortlist -> bidHash)
User->>Home: Confirm Buy (shortlist)
Home-->>User: bidHash
Home->>UI: showTransactionStatus(bidHash)
loop Poll Bid & Resource Lock
Home->>API: getBidStatus(bidHash)
API-->>Home: BidStatus
Home->>API: getResourceLockInfo(bidHash)
API-->>Home: ResourceLockStatus
Home->>Home: map statuses -> TransactionStatusState
Home->>UI: update props (status, hashes, timestamps)
end
Home-->>UI: Final status (Complete/Failed)
UI->>UI: auto-open details or user opens
end
sequenceDiagram
autonumber
actor User
participant Home as HomeScreen
participant UI as TransactionStatus (UI)
participant API as Backend
rect rgba(240,248,255,1)
note right of User: Sell flow (confirm -> userOpHash)
User->>Home: Confirm Sell
Home-->>User: userOpHash
Home->>UI: showTransactionStatus(userOpHash)
loop Poll UserOp
Home->>API: getUserOperationStatus(userOpHash)
API-->>Home: UserOpStatus (+txHash?)
Home->>Home: map status -> TransactionStatusState
Home->>UI: update props (status, txHash, timestamps)
end
Home-->>UI: Final status (Complete/Failed)
UI->>UI: auto-open details or user opens
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
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 |
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
Deploying x with
|
| Latest commit: |
9a07b8b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a7308e00.x-e62.pages.dev |
| Branch Preview URL: | https://feat-pro-3681-transaction-st.x-e62.pages.dev |
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/apps/pulse/components/Buy/PreviewBuy.tsx (1)
171-179: Crash risk: unsafe access to bids[0].bidHash and optional intentSdk.If bids is empty or intentSdk is undefined, this throws or silently skips shortlist but still calls showTransactionStatus. Guard and derive bidHash first.
try { - await intentSdk?.shortlistBid( - expressIntentResponse?.intentHash!, - expressIntentResponse?.bids[0].bidHash! - ); - showTransactionStatus(expressIntentResponse?.bids[0].bidHash!); + const bidHash = expressIntentResponse.bids?.[0]?.bidHash; + const intentHash = expressIntentResponse.intentHash; + if (!intentSdk || !bidHash || !intentHash) { + // Abort gracefully + setIsWaitingForSignature(false); + setIsLoading(false); + return; + } + await intentSdk.shortlistBid(intentHash, bidHash); + showTransactionStatus(bidHash);src/apps/pulse/components/Transaction/TransactionErrorBox.tsx (1)
2-2: Use named import for CopyToClipboard throughout
Align every import toimport { CopyToClipboard } from 'react-copy-to-clipboard', and update any test mocks (replacedefault:withCopyToClipboard:) to match. You can locate remaining occurrences with:rg -n "react-copy-to-clipboard" -g '*.ts' -g '*.tsx'src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx (1)
193-221: Contradictory assertions in shortlist-failure test; wait for removal instead of immediate absenceYou first await the tracker text to appear, then immediately assert it’s not present. This will be flaky at best. Wait for the element to be removed and also assert that
showTransactionStatuswas not called.Apply this diff:
- it('handles shortlist bid failure', async () => { + it('handles shortlist bid failure', async () => { (useIntentSdk as any).mockReturnValue({ intentSdk: { shortlistBid: vi .fn() .mockRejectedValue(new Error('Shortlist failed')), }, }); render(<PreviewBuy {...mockProps} />); const confirmButton = screen.getByText('Confirm'); fireEvent.click(confirmButton); - await waitFor(() => { - expect( - screen.getByText( - 'Please open your wallet and confirm the transaction.' - ) - ).toBeInTheDocument(); - }); - - // Should not show tracker on error - expect( - screen.queryByText( - 'Please open your wallet and confirm the transaction.' - ) - ).not.toBeInTheDocument(); + // Should hide the tracker once the shortlist fails + // and must not trigger transaction status + await waitForElementToBeRemoved(() => + screen.queryByText('Please open your wallet and confirm the transaction.') + ); + expect(mockProps.showTransactionStatus).not.toHaveBeenCalled(); });Don’t forget to import the helper:
- import { fireEvent, render, screen, waitFor } from '@testing-library/react'; + import { fireEvent, render, screen, waitFor, waitForElementToBeRemoved } from '@testing-library/react';
🧹 Nitpick comments (39)
src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx (1)
119-119: Add expectations for the transaction status callback.
showTransactionStatusis newly wired into the component but the tests never assert that it fires after a successful sell. If PreviewSell stops invoking this callback, the test suite would still pass, so we lose coverage on the key behaviour introduced in this PR. Please extend the “executes sell transaction” spec to verify the callback is called with the expected payload.await waitFor(() => { expect( screen.getByText( 'Please open your wallet and confirm the transaction.' ) ).toBeInTheDocument(); }); expect(mockExecuteSell).toHaveBeenCalledWith( mockToken, '10.5', undefined ); + + expect(mockProps.showTransactionStatus).toHaveBeenCalledWith( + '0xTransactionHash123456789' + );src/apps/pulse/components/Transaction/TransactionInfo.tsx (3)
50-61: Show “copied” per row instead of globallyA single isCopied flag will flip both copy icons in Buy mode. Track the copied hash to scope feedback per-row.
- const [isCopied, setIsCopied] = useState(false); + const [copiedHash, setCopiedHash] = useState<string | null>(null); ... - useEffect(() => { - if (isCopied) { - const timer = setTimeout(() => setIsCopied(false), 2000); + useEffect(() => { + if (copiedHash) { + const timer = setTimeout(() => setCopiedHash(null), 2000); return () => clearTimeout(timer); } return undefined; - }, [isCopied]); + }, [copiedHash]);- {showCopy && isValidHash && ( + {showCopy && isValidHash && ( <CopyToClipboard - text={transactionHash} - onCopy={() => setIsCopied(true)} + text={transactionHash!} + onCopy={() => setCopiedHash(transactionHash!)} > <div className="flex items-center ml-1 cursor-pointer"> - {isCopied ? ( + {copiedHash === transactionHash ? ( <MdCheck className="w-[10px] h-3 text-white" /> ) : ( <img src={CopyIcon} alt="copy-address-icon" className="w-[10px] h-3" /> )} </div> </CopyToClipboard> )}Also applies to: 140-157, 171-181, 182-192, 195-205
171-181: Optional: add aria-labels for copy buttonsImprove a11y by labeling the copy actions.
- {detailsEntry( + {detailsEntry( 'Resource Lock', ... )} ... - {detailsEntry( + {detailsEntry( 'Completed', ... )}Within detailsEntry’s CopyToClipboard wrapper:
- <div className="flex items-center ml-1 cursor-pointer"> + <div className="flex items-center ml-1 cursor-pointer" aria-label={`Copy ${lhs} hash`}>Also applies to: 182-192
15-15: Heads-up: Arbitrum explorer URL uses http in getBlockScanThe referenced helper returns 'http://arbiscan.io' for chainId 42161. Prefer https. This is in src/utils/blockchain.ts, not this file, but affects these links. Recommend fixing there.
src/apps/pulse/components/App/HomeScreen.tsx (3)
228-235: Clear pending failure timeout when stopping pollingPrevent stray state updates after closing by clearing the scheduled timeout.
- const stopTransactionPolling = useCallback(() => { + const stopTransactionPolling = useCallback(() => { setIsPollingActive(false); setIsBackgroundPolling(false); setShouldAutoReopen(false); + if (failureTimeoutId) { + clearTimeout(failureTimeoutId); + setFailureTimeoutId(null); + } setTransactionData(null); - }, []); + }, [failureTimeoutId]);
656-664: Route errors through delayed failure pathDirectly setting “Transaction Failed” bypasses the 10s confirmation delay in updateStatusWithDelay. Use the same path for consistency and to avoid flapping on transient errors.
- } catch (error) { + } catch (error) { console.error('Failed to get transaction status:', error); - setCurrentTransactionStatus('Transaction Failed'); - setErrorDetails( + updateStatusWithDelay('Transaction Failed'); + setErrorDetails( error instanceof Error ? error.message : 'Failed to get transaction status' ); }
351-355: Optional: track and clear all setTimeoutsupdateStatusWithDelay schedules timeouts that aren’t cleared on unmount. Consider tracking them in a ref and clearing in a cleanup/useEffect to avoid stray updates.
Also applies to: 444-455
src/apps/pulse/components/Transaction/ProgressStep.tsx (2)
139-143: Verify Tailwind breakpoint “xs”Tailwind’s default breakpoints start at sm. If “xs” isn’t configured, these classes won’t work. Validate tailwind.config.js or switch to sm.
33-46: Minor: consolidate pending step logicThe two pending cases return identical classes/icons. You can merge to reduce branches.
- if ( - status === 'pending' && - (step === 'Pending' || step === 'ResourceLock') - ) { - return `${baseClasses} bg-[#8A77FF]`; - } - if (status === 'pending' && step === 'Completed') { - return `${baseClasses} bg-[#8A77FF]`; - } + if (status === 'pending') { + return `${baseClasses} bg-[#8A77FF]`; + }- if ( - status === 'pending' && - (step === 'Pending' || step === 'ResourceLock') - ) { - return ( - <TailSpin color="#FFFFFF" height={10} width={10} strokeWidth={8} /> - ); - } - if (status === 'pending' && step === 'Completed') { + if (status === 'pending') { return ( <TailSpin color="#FFFFFF" height={10} width={10} strokeWidth={8} /> ); }Also applies to: 108-120
src/apps/pulse/components/Transaction/TransactionErrorBox.tsx (1)
43-55: Add aria semantics to the “Technical Details” toggle and panel.Improve accessibility and testability by wiring aria-expanded/aria-controls on the button and an id/role on the panel.
- <button + <button type="button" onClick={() => setIsExpanded(!isExpanded)} - className="px-2 py-1.5 rounded-[6px] bg-[#FF366C]/30 text-white text-[13px] font-normal flex items-center gap-2" + className="px-2 py-1.5 rounded-[6px] bg-[#FF366C]/30 text-white text-[13px] font-normal flex items-center gap-2" + aria-expanded={isExpanded} + aria-controls="technical-details" > @@ - {isExpanded && ( - <div className="mt-2"> + {isExpanded && ( + <div id="technical-details" className="mt-2" role="region" aria-label="Technical details"> <p className="text-white/70 text-[12px] font-mono whitespace-pre-wrap break-all"> {technicalDetails} </p> </div> )}Also applies to: 71-76
src/apps/pulse/components/Misc/CloseButton.tsx (1)
23-45: Minor a11y polish: declare keyboard shortcut and visible focus styles.Add aria-keyshortcuts and a focus-visible ring to aid keyboard users; behavior unchanged.
<button - className="flex items-center justify-center w-[36px] h-[34px] bg-[#1E1D24] rounded-[8px]" + className="flex items-center justify-center w-[36px] h-[34px] bg-[#1E1D24] rounded-[8px] focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white/40" onClick={onClose} type="button" aria-label="Close" + aria-keyshortcuts="Esc" data-testid="close-button" >src/apps/pulse/components/Buy/PreviewBuy.tsx (4)
1-1: Prefer removing the global eslint-disable by fixing non-null assertions.With the guard above, the non-null assertion rule can stay enabled.
-/* eslint-disable @typescript-eslint/no-non-null-asserted-optional-chain */
497-504: Surface actual gas fee if available (optional).You already support gasFee in showTransactionStatus; consider plumbed-estimate here instead of a hardcoded “≈ $0.00”.
266-296: Auto-refresh considerations.15s polling is fine, but add jitter/backoff on repeated failures to avoid synchronized bursts and rate limits.
42-51: Unify showTransactionStatus parameter naming
The first argument is declared asuserOperationHashin Buy (but passed a bidHash) andtxHashin Sell—rename it to a generichash(or split into separate callbacks) for clarity.src/apps/pulse/components/Transaction/tests/TransactionErrorBox.test.tsx (2)
65-71: Add assertions for aria-expanded and the details region (after a11y update).Once aria attributes are added, assert toggle semantics to prevent regressions.
- it('expands when technical details button is clicked', () => { + it('expands when technical details button is clicked', () => { render(<TransactionErrorBox {...baseProps} />); - const expandButton = screen.getByText('Technical Details'); + const expandButton = screen.getByText('Technical Details'); fireEvent.click(expandButton); + expect(expandButton).toHaveAttribute('aria-expanded', 'true'); + const region = screen.getByRole('region', { name: /technical details/i }); + expect(region).toHaveAttribute('id', 'technical-details'); expect(screen.getByText(baseProps.technicalDetails)).toBeInTheDocument(); });Also applies to: 73-90
10-22: Mock both default and named exports for react-copy-to-clipboard (import consistency).If you normalize imports, keep the mock compatible with both.
-vi.mock('react-copy-to-clipboard', () => ({ - default: ({ children, onCopy, text }: any) => ( +vi.mock('react-copy-to-clipboard', () => ({ + __esModule: true, + default: ({ children, onCopy, text }: any) => ( <button type="button" onClick={() => onCopy(text)} data-testid="copy-button"> {children} </button> ), + CopyToClipboard: ({ children, onCopy, text }: any) => ( + <button type="button" onClick={() => onCopy(text)} data-testid="copy-button"> + {children} + </button> + ), }));src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (1)
636-651: Good redaction test; consider covering buy-mode redaction too.You validate API key redaction for sell; add the same assertion for buy flow to guard both paths.
src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx (3)
3-3: Use a type‑only import (or local type) to avoid runtime deep import couplingThe current import targets a deep CJS path and may leak into runtime if TS config changes. Make it explicit type‑only (safe), or inline a minimal local type.
Option A — keep path but mark type-only:
-import { ExpressIntentResponse } from '@etherspot/intent-sdk/dist/cjs/sdk/types/user-intent-types'; +import type { ExpressIntentResponse } from '@etherspot/intent-sdk/dist/cjs/sdk/types/user-intent-types';And drop the redundant cast:
-} as any; +};Option B — avoid the import entirely:
type ExpressIntentResponse = { intentHash: string; bids: { bidHash: string }[] };Also applies to: 48-51
169-189: Strengthen assertion around showTransactionStatus callAdd a precondition to ensure it isn’t called before user action, keeping the test intent crisp.
Apply this diff:
it('shows transaction status after successful shortlist', async () => { render(<PreviewBuy {...mockProps} />); + expect(mockProps.showTransactionStatus).not.toHaveBeenCalled(); const confirmButton = screen.getByText('Confirm'); fireEvent.click(confirmButton);
78-81: Consider replacing the broad snapshot with targeted assertionsSnapshots for complex, highly‑styled components tend to churn. Prefer focused assertions on visible text/roles.
Based on learnings
src/apps/pulse/components/Transaction/tests/ProgressStep.test.tsx (2)
114-126: Line visibility isn’t actually assertedBoth tests only check the label text; they don’t verify the line’s presence/absence. Add a stable test target and assert it.
Test changes (assuming the component exposes
data-testid="progress-line"whenshowLineis true and!isLast):it('shows line when not last step', () => { render(<ProgressStep {...baseProps} isLast={false} />); - - expect(screen.getByText('Transaction Submitted')).toBeInTheDocument(); + expect(screen.getByText('Transaction Submitted')).toBeInTheDocument(); + expect(screen.getByTestId('progress-line')).toBeInTheDocument(); }); it('does not show line when is last step', () => { render(<ProgressStep {...baseProps} isLast />); - - expect(screen.getByText('Transaction Submitted')).toBeInTheDocument(); + expect(screen.getByText('Transaction Submitted')).toBeInTheDocument(); + expect(screen.queryByTestId('progress-line')).not.toBeInTheDocument(); });If the component lacks such a hook, add one in
ProgressStep.tsxwhere the vertical line is rendered:<div data-testid="progress-line" /* existing props/styles */ />
29-34: Avoid snapshot for purely presentational componentSnapshots tend to be noisy here; rely on the behavior/state assertions you already have.
Based on learnings
src/apps/pulse/components/Transaction/tests/TransactionInfo.test.tsx (3)
94-126: Reduce brittleness: assert semantics, not Tailwind class namesAsserting specific classes like
text-white/50,text-[#FFAB36], etc., couples tests to styling. Prefer role/text plus a semantic hook (e.g.,data-status="pending|success|failed"), then assert on that.Example:
expect(screen.getByTestId('status')).toHaveAttribute('data-status', 'pending');Add
data-testid="status"anddata-statuson the status element inTransactionInfo.tsx.
350-360: Short‐hash truncation expectation looks offExpecting
0x123...x123for a 5‑char hash is unusual. Either we shouldn’t truncate very short inputs or should display them verbatim.If the intended behavior is “no truncation below threshold,” update the expectation:
- expect(screen.getByText('0x123...x123')).toBeInTheDocument(); + expect(screen.getByText('0x123')).toBeInTheDocument();
88-92: Snapshot can be dropped in favor of explicit assertionsGiven the breadth of behavior checks below, the snapshot adds little value and can churn on layout-only changes.
Based on learnings
src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx (5)
167-171: Prefer userEvent over fireEvent for realistic interactionsSwitch to @testing-library/user-event (click/keyboard) for closer-to-user behavior and fewer act()/timing pitfalls.
-import { fireEvent, render, screen } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; -const viewStatusButton = screen.getByText('View Status'); -fireEvent.click(viewStatusButton); +await userEvent.click(screen.getByText('View Status')); -const successButton = screen.getByText('Success'); -fireEvent.click(successButton); +await userEvent.click(screen.getByText('Success'));Also applies to: 181-185, 192-196
217-229: Strengthen outside-click test to avoid false positivesAlso assert that clicking inside the container does NOT close, to guard regressions in outside-click logic.
-// Click outside the modal -fireEvent.mouseDown(document.body); -expect(baseProps.closeTransactionStatus).toHaveBeenCalled(); +// Click outside the modal +await userEvent.click(document.body); +expect(baseProps.closeTransactionStatus).toHaveBeenCalled(); + +// Negative case: clicking inside should NOT close +const container = screen.getByText('Starting Transaction').closest('div'); +await userEvent.click(container as HTMLElement); +expect(baseProps.closeTransactionStatus).toHaveBeenCalledTimes(1);
108-111: Snapshot test is fine; consider adding a lightweight a11y assertionAdd a small check for dialog semantics (role/aria) once implemented to make tests more resilient than snapshot-only.
-const tree = renderer.create(<TransactionStatus {...baseProps} />).toJSON(); -expect(tree).toMatchSnapshot(); +const tree = renderer.create(<TransactionStatus {...baseProps} />).toJSON(); +expect(tree).toMatchSnapshot(); +// Assert basic a11y hooks (after component adds them) +render(<TransactionStatus {...baseProps} />); +expect(screen.getByRole('dialog', { name: /transaction status/i })).toBeInTheDocument();
300-337: ClassName equality assertions are brittleAsserting the full Tailwind class list will cause frequent churn. Prefer subset checks or role-based queries.
- expect(iconContainer).toHaveClass( - 'w-[90px]','h-[90px]','rounded-full','border-[3px]','border-white/10','bg-[#8A77FF]' - ); + expect(iconContainer).toHaveClass('rounded-full'); + expect(iconContainer?.className).toMatch(/bg-\[#8A77FF\]/);Also applies to: 339-375
26-35: Mock heavy spinners to speed up and stabilize testsMock react-loader-spinner to a simple stub to avoid SVG/animation overhead in JSDOM.
+vi.mock('react-loader-spinner', () => ({ + TailSpin: (props: any) => <div data-testid="tail-spin" {...props} />, +}));src/apps/pulse/components/Transaction/TransactionStatus.tsx (1)
96-111: Duplicate global listeners (here and in TransactionDetails/CloseButton/Esc)Both this component and its child add Escape/outside-click listeners. Centralize to one place to avoid multiple handlers firing.
- Option A: Keep listeners here; pass a simple onClose to children; remove listeners from TransactionDetails.
- Option B: Remove listeners here and rely on CloseButton/Esc + a wrapper Modal that owns outside-click.
If keeping here, consider pointerdown to better catch drag cases and add a guard to ignore clicks originating from within portals.
Also applies to: 113-126
src/apps/pulse/components/Transaction/TransactionDetails.tsx (7)
119-131: Triple Escape handling; dedupe to a single source of truthThis component adds its own ESC listener while also rendering Esc/CloseButton (each adds ESC listeners) and the parent adds one too. This can trigger multiple onDone calls.
- Remove this component’s ESC listener and rely on Esc/CloseButton (or vice‑versa). If you keep a listener here, make Esc/CloseButton dumb (no listeners).
- useEffect(() => { - const handleKeyDown = (event: KeyboardEvent) => { - if (event.key === 'Escape') { - onDone(); - } - }; - document.addEventListener('keydown', handleKeyDown); - return () => { - document.removeEventListener('keydown', handleKeyDown); - }; - }, [onDone]);Also applies to: 383-392
382-392: Add dialog semantics to the details panelMark details as a nested dialog/region and focus the container on mount for better keyboard navigation.
- return ( - <div ref={detailsRef} className="flex flex-col gap-4 h-full w-full"> + return ( + <div + ref={detailsRef} + role="dialog" + aria-modal="true" + aria-labelledby="tx-details-title" + tabIndex={-1} + className="flex flex-col gap-4 h-full w-full" + > <div className="flex justify-between items-center"> - <p className="text-xl text-white font-normal">Transaction Details</p> + <p id="tx-details-title" className="text-xl text-white font-normal"> + Transaction Details + </p>
491-504: completedAt likely wrong for BuyYou pass pendingCompletedAt to TransactionInfo as completedAt. For Buy, the “final” step completion may be resourceLockCompletedAt. Consider conditional wiring.
- completedAt={pendingCompletedAt} + completedAt={isBuy ? (resourceLockCompletedAt || pendingCompletedAt) : pendingCompletedAt}
264-271: Broaden secret redactionThe regex only catches api-key=...; consider covering apiKey, x-api-key, and Authorization headers to avoid accidental leakage.
- return errorThrown.replace( - /api-key=[A-Za-z0-9+/=]+/g, - 'api-key=***REDACTED***' - ); + return errorThrown + .replace(/(?:api[-_]?key|x-api-key)=([A-Za-z0-9._\-+/=]+)/gi, '$1=***REDACTED***') + .replace(/Authorization:\s*Bearer\s+[A-Za-z0-9._\-+/=]+/gi, 'Authorization: Bearer ***REDACTED***');
394-422: Guard against missing token logo/source errorsWhen logo is missing, empty src attributes can cause 404s/noise. Provide a placeholder or hide the img when no logo is available.
- <img - className="w-4 h-4 rounded" - src={isBuy ? buyToken?.logo || '' : sellToken?.logo || ''} - alt={isBuy ? buyToken?.symbol || 'Token' : sellToken?.symbol || 'Token'} - /> + {(isBuy ? buyToken?.logo : sellToken?.logo) ? ( + <img + className="w-4 h-4 rounded" + src={isBuy ? (buyToken!.logo as string) : (sellToken!.logo as string)} + alt={isBuy ? buyToken?.symbol || 'Token' : sellToken?.symbol || 'Token'} + /> + ) : null}
321-339: Minor: transactionHash assignment comment is confusingtransactionHash sets to userOpHash for both branches; the comment implies bidHash vs userOpHash. Consider using distinct field names to avoid ambiguity.
- transactionHash: isBuy ? userOpHash : userOpHash, // For Buy: bidHash, For Sell: userOpHash - hashType: isBuy ? 'bidHash' : 'userOpHash', + transactionHash: userOpHash, + hashType: isBuy ? 'bidHash' : 'userOpHash', // clarify that userOpHash carries bidHash in buy mode
102-117: Outside-click handler: consider pointerdown and portal-aware checkIf details/status ever render via portals, contains() can misfire. Use pointerdown and a composedPath() check when available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/apps/pulse/components/Transaction/tests/__snapshots__/ProgressStep.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Transaction/tests/__snapshots__/TransactionDetails.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Transaction/tests/__snapshots__/TransactionErrorBox.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Transaction/tests/__snapshots__/TransactionInfo.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Transaction/tests/__snapshots__/TransactionStatus.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
src/apps/pulse/components/App/HomeScreen.tsx(7 hunks)src/apps/pulse/components/Buy/PreviewBuy.tsx(3 hunks)src/apps/pulse/components/Buy/tests/PreviewBuy.test.tsx(3 hunks)src/apps/pulse/components/Misc/CloseButton.tsx(1 hunks)src/apps/pulse/components/Sell/TransactionDetails.tsx(0 hunks)src/apps/pulse/components/Sell/TransactionInfo.tsx(0 hunks)src/apps/pulse/components/Sell/TransactionStatus.tsx(0 hunks)src/apps/pulse/components/Sell/tests/PreviewSell.test.tsx(1 hunks)src/apps/pulse/components/Sell/tests/SellButton.test.tsx(1 hunks)src/apps/pulse/components/Transaction/ProgressStep.tsx(5 hunks)src/apps/pulse/components/Transaction/TransactionDetails.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionErrorBox.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionInfo.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionStatus.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/ProgressStep.test.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionErrorBox.test.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionInfo.test.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- src/apps/pulse/components/Sell/TransactionDetails.tsx
- src/apps/pulse/components/Sell/TransactionStatus.tsx
- src/apps/pulse/components/Sell/TransactionInfo.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (1)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)
src/apps/pulse/components/Transaction/TransactionDetails.tsx (4)
src/apps/pulse/components/Misc/Esc.tsx (1)
Esc(7-36)src/apps/pulse/components/Misc/CloseButton.tsx (1)
CloseButton(7-46)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)src/apps/pulse/components/Sell/TransactionDetails.tsx (1)
TransactionDetailsProps(37-211)
src/apps/pulse/components/Transaction/TransactionInfo.tsx (2)
src/utils/blockchain.ts (1)
getBlockScan(225-246)src/apps/pulse/components/Sell/TransactionInfo.tsx (1)
TransactionInfoProps(29-167)
src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx (1)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)
src/apps/pulse/components/App/HomeScreen.tsx (4)
src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(9-53)src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(37-43)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)
⏰ 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 (6)
src/apps/pulse/components/Sell/tests/SellButton.test.tsx (1)
27-30: Mock update aligns with SellOffer changes. AddingminimumReceiveandslippageTolerancekeeps the test fixtures in sync with the expandedSellOfferinterface, so everything compiles cleanly. Nicely done.src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (1)
151-186: Solid coverage of status/interaction paths.Snapshot + behavioral checks across statuses and close actions look good.
Also applies to: 187-235
src/apps/pulse/components/Transaction/TransactionErrorBox.tsx (1)
12-17: Verify upstream sanitization in generateTechnicalDetails
Only usage of TransactionErrorBox is in TransactionDetails.tsx (<TransactionErrorBox technicalDetails={generateTechnicalDetails()} />). Ensure generateTechnicalDetails redacts PII, API keys or tokens before passing its output here—tests cover redaction in TransactionDetails but double-check this upstream sanitization.src/apps/pulse/components/Transaction/tests/TransactionInfo.test.tsx (1)
11-21: No change needed: mock aligns with default importTransactionInfo imports CopyToClipboard as the default export; the test’s vi.mock correctly mocks
default. No update required.Likely an incorrect or invalid review comment.
src/apps/pulse/components/Transaction/TransactionStatus.tsx (1)
261-270: Confirm txHash plumbing for Buy vs SellFor Buy you pass completedTxHash and omit txHash; for Sell you pass blockchainTxHash as txHash. Ensure TransactionInfo expects completedAt aligned with the final step (see comment in TransactionDetails).
src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx (1)
1-6: jest-dom matchers are already set up globally
The global test setup in src/test-utils/setupTests.ts imports @testing-library/jest-dom and is loaded via setupFiles in vite.config.js, so matchers like toBeInTheDocument and toHaveClass are available.
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
If you don't mind checking the one CodeRabbit issue but other than that - no showstoppers!
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/apps/pulse/components/Transaction/TransactionStatus.tsx (1)
265-319: Add dialog role and ARIA attributes for accessibility.The past review comment correctly identifies that this modal-like component should have
role="dialog",aria-modal="true", and an associated label for screen reader users. Implementing these attributes will improve accessibility.return ( <div ref={transactionStatusRef} + role="dialog" + aria-modal="true" + aria-labelledby="transaction-status-title" className={`flex flex-col w-full max-w-[446px] ${!showDetails ? 'h-[calc(100vh-94px)] max-h-[600px]' : 'h-min'} bg-[#1E1D24] border border-white/5 rounded-[10px] p-6`} > {showDetails ? ( <TransactionDetails onDone={ currentStatus === 'Transaction Pending' ? () => setShowDetails(false) : closeTransactionStatus } {.../* other props */} /> ) : ( <div className="flex flex-col items-center justify-center h-full"> {statusIcon && ( <div className={statusIcon.containerClasses}> <img src={statusIcon.icon} alt={currentStatus} className={statusIcon.iconClasses} /> </div> )} <div + id="transaction-status-title" className={`text-white font-normal text-[20px] ${statusIcon ? 'mt-3' : ''}`} > {currentStatus} </div> {getThirdElement()} </div> )} </div> );
🧹 Nitpick comments (2)
src/apps/pulse/components/App/HomeScreen.tsx (2)
249-292: Verify dependency array completeness.The
showTransactionStatuscallback includesbuyToken,payingTokens, andusdAmountin its closure but not in the dependency array (Line 291). While the ESLint rule is disabled, this could lead to stale closures where old values are captured.Consider whether these should be added to the dependency array:
[sellToken, tokenAmount, sellOffer, isBuy] + [sellToken, buyToken, tokenAmount, sellOffer, payingTokens, usdAmount, isBuy]Or, if the current behavior is intentional (capturing values at callback creation time), add a comment explaining why these dependencies are omitted.
559-561: Verify chainId type from API.Line 559 converts
lock.chainIdto a number. If the API consistently returns a number, this conversion is unnecessary. However, if the API might return a string, the conversion is appropriate.Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pillarx-app/components/PillarXLogo/PillarXLogo.tsx(1 hunks)src/apps/pulse/components/App/HomeScreen.tsx(7 hunks)src/apps/pulse/components/Transaction/TransactionDetails.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionStatus.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/apps/pillarx-app/components/PillarXLogo/PillarXLogo.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/apps/pulse/components/App/HomeScreen.tsx (5)
src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(9-53)src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(37-43)src/apps/pulse/components/Buy/PayingToken.tsx (1)
PayingToken(18-93)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)
src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (1)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)
src/apps/pulse/components/Transaction/TransactionDetails.tsx (4)
src/apps/pulse/components/Misc/Esc.tsx (1)
Esc(7-36)src/apps/pulse/components/Misc/CloseButton.tsx (1)
CloseButton(7-46)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)src/apps/pulse/components/Sell/TransactionDetails.tsx (1)
TransactionDetailsProps(37-211)
src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx (1)
src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)
⏰ 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 (19)
src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (4)
14-71: LGTM! Mock setup follows testing best practices.The mock implementations are appropriate for unit testing and use data-testid attributes effectively for querying elements.
73-137: LGTM! Mock data is comprehensive and realistic.The mock tokens, paying tokens, and base props provide good coverage for testing various scenarios.
649-663: Excellent security test for API key sanitization.This test ensures that sensitive API keys are redacted from error details, which is critical for preventing credential leaks in logs and error reports.
1-694: Comprehensive test coverage with good practices.This test suite demonstrates strong coverage across multiple dimensions: status variations, user interactions, buy/sell modes, error handling, edge cases, and security (API key sanitization). The use of mocks is appropriate for unit testing, and the tests effectively validate component behavior without over-reliance on implementation details.
src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx (3)
14-101: LGTM! Mock setup is clean and appropriate.The simplified TransactionDetails mock is acceptable for unit testing TransactionStatus in isolation.
198-214: Good test coverage of context-dependent done button behavior.This test correctly verifies that clicking done in the pending status returns to the main view rather than closing the transaction status entirely, which is the expected UX flow.
1-530: Comprehensive and well-organized test suite.The tests provide strong coverage of the TransactionStatus component, including status variations, conditional interactions (ESC/outside-click behavior based on status), buy/sell modes, styling, and edge cases. The interaction tests particularly shine in verifying context-dependent behavior.
src/apps/pulse/components/Transaction/TransactionStatus.tsx (1)
12-66: LGTM! Well-typed interface with clear documentation.The TransactionStatusProps interface is comprehensive and the comment on Line 14 usefully clarifies that userOpHash serves dual purposes (UserOp hash for Sell, bidHash for Buy).
src/apps/pulse/components/App/HomeScreen.tsx (2)
294-307: LGTM! Smart background polling logic.The closeTransactionStatus function correctly enables background polling when the transaction is still in progress, allowing the user to close the modal while preserving transaction state awareness. The auto-reopen flag ensures the modal reappears when the transaction completes.
524-689: LGTM! Comprehensive polling logic for Buy and Sell flows.The polling effect correctly handles both Buy (bid + resource lock) and Sell (UserOp) transaction flows with appropriate status mapping, error handling, and hash extraction. The 2-second polling interval is reasonable, and the chainId resolution now correctly uses the buyToken chain for Buy transactions (past issue resolved).
src/apps/pulse/components/Transaction/TransactionDetails.tsx (9)
1-16: LGTM!All imports are used appropriately throughout the component. The conditional usage of
EscandCloseButtonbased on transaction status is intentional.
18-73: LGTM!The interface is comprehensive and correctly models both Buy and Sell transaction flows with appropriate optional fields.
141-199: Past review concern remains: Pending step cannot indicate failure.The previous review correctly identified that
getStepStatusnever returns'failed'for the'Pending'step (line 186 always returns'completed'when status is'Transaction Failed'). This preventsdetermineFailureStepfrom detecting a pending-step failure via lines 258-260.Verify whether this is intentional behavior (i.e., pending always completes before failure) or if the mapping should distinguish between "pending completed then failed" vs "failed during pending."
If failures can occur during pending without a transaction hash, consider applying the diff suggested in the past review to mark Pending as
'failed'when the expected hash is missing.Based on learnings
201-242: LGTM!The timestamp formatting correctly returns JSX elements for completed steps (excluding the final step) with appropriate date/time formats and styling.
244-269: determineFailureStep depends on getStepStatus fix.This function's ability to detect pending/submission failures depends on
getStepStatusreturning'failed'for those steps, which currently doesn't happen (lines 255-260 will never execute as written). See the earlier comment ongetStepStatus.
282-288: Clarify bidHash vs userOpHash usage.Line 286 uses
userOpHashfor both Buy and Sell, but the comment on line 286 states "For Buy: bidHash, For Sell: userOpHash". If Buy transactions should use a different hash identifier (bidHash), ensure the correct value is passed or update the comment.Verify whether
userOpHashis the correct identifier for Buy transactions, or if a separatebidHashprop should be added to the interface.
271-279: LGTM!The API key sanitization regex correctly redacts sensitive data from error messages.
390-430: LGTM!The header and summary card correctly render transaction details with proper conditional logic for Buy vs Sell flows, and the Esc/CloseButton distinction matches the expected behavior per status.
432-529: LGTM!The rendering logic correctly handles Buy vs Sell flows with appropriate progress steps, conditional displays, and proper prop passing to child components.
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/apps/pulse/components/Transaction/TransactionInfo.tsx (2)
47-49: Guard explorer link when getBlockScan returns empty URL.If
getBlockScan(cId)returns an empty string,window.openmay navigate to a broken relative path. This issue was previously flagged and remains unresolved.Apply this diff to guard the handler:
const handleExternalLink = (hash: string, cId: number) => { + const base = getBlockScan(cId); + if (!base) return; - window.open(`${getBlockScan(cId)}${hash}`, '_blank', 'noopener,noreferrer'); + window.open(`${base}${hash}`, '_blank', 'noopener,noreferrer'); };Additionally, only render the external link button when a valid explorer URL exists:
- {showExternalLink && isHashValid && transactionHash && ( + {showExternalLink && isHashValid && transactionHash && getBlockScan(effectiveChainId) && ( <button type="button" + aria-label="Open in block explorer" onClick={() => handleExternalLink(transactionHash, effectiveChainId) }
107-109: Avoid boolean-to-string in className.Using a boolean inside a template literal yields
"false"as a class whenlhs !== 'Status'. This issue was previously flagged and remains unresolved.Apply this diff:
- <span className={`${lhs === 'Status' && statusInfo.color}`}> + <span className={lhs === 'Status' ? statusInfo.color : ''}> {rhs} </span>
🧹 Nitpick comments (17)
src/apps/pulse/hooks/tests/useClickOutside.test.ts (1)
24-36: Ineffective event listener cleanup in afterEach.Lines 25, 33, and 35 call
removeEventListenerwithvi.fn(), which creates new function instances each time. Event listeners are identified by reference, so these calls won't remove the actual listeners registered by the hook. ThequerySelectorAllloop (lines 31-34) adds unnecessary overhead and also doesn't remove listeners.The hook itself handles cleanup in its
useEffectreturn, so explicit afterEach cleanup is redundant here. If you need to verify cleanup, the test at lines 127-146 already validates it.Apply this diff to remove the ineffective cleanup:
afterEach(() => { vi.clearAllMocks(); - // Clean up event listeners - remove all mousedown listeners - const listeners = document.querySelectorAll('*'); - listeners.forEach((el) => { - el.removeEventListener('mousedown', vi.fn()); - }); - document.removeEventListener('mousedown', vi.fn()); });src/apps/pulse/utils/utils.tsx (1)
57-64: Consider broader regex pattern for API key redaction.The current pattern
/api-key=[A-Za-z0-9+/=]+/gmatches base64-like characters but may miss URL-encoded keys or keys with dashes/underscores. If your API keys can contain-,_, or%-encoded characters, consider a more inclusive pattern:return errorThrown.replace( - /api-key=[A-Za-z0-9+/=]+/g, + /api-key=[A-Za-z0-9+/=_-]+/g, 'api-key=***REDACTED***' );Alternatively, use a non-whitespace pattern if keys are delimited by whitespace:
/api-key=\S+/g.src/apps/pulse/hooks/useClickOutside.ts (1)
7-29: Stabilize the callback passed to useClickOutside
Wrap thecloseTransactionStatusprop and inline() => setShowDetails(false)callback inuseCallback(or adopt a ref-based handler inside the hook) so the effect’s dependency oncallbackdoesn’t re-register the listener on every render.src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
34-34: Consider documenting the need for stable callbacks.The effect depends on
onEscapeandonEnter, which will cause the listener to be re-registered on every render if these callbacks are not stable (e.g., inline functions).Consumers should wrap
onEscapeandonEnterwithuseCallbackto avoid unnecessary re-registrations. Consider adding a JSDoc comment to document this requirement, or alternatively, wrap the callbacks internally withuseCallback(though that would require additional dependencies).src/apps/pulse/hooks/useTransactionStatus.ts (1)
27-43: Consider simplifying the memoization pattern.The
useMemowraps a function that delegates togetStepStatus. However, since the consumer will callgetStepStatusForStep(step)on each render anyway, memoizing the wrapper function provides limited benefit unless the function is passed as a prop to a memoized child component.Consider one of the following:
- If the function is passed to memoized children, keep the current pattern and document the reason.
- If not, simplify by computing all step statuses upfront and returning an object instead of a function:
const stepStatuses = useMemo(() => ({ Submitted: getStepStatus('Submitted', status, isBuy, resourceLockTxHash, completedTxHash, isResourceLockFailed), Pending: getStepStatus('Pending', status, isBuy, resourceLockTxHash, completedTxHash, isResourceLockFailed), ResourceLock: getStepStatus('ResourceLock', status, isBuy, resourceLockTxHash, completedTxHash, isResourceLockFailed), Completed: getStepStatus('Completed', status, isBuy, resourceLockTxHash, completedTxHash, isResourceLockFailed), }), [status, isBuy, resourceLockTxHash, completedTxHash, isResourceLockFailed]);src/apps/pulse/components/Transaction/TransactionStatusContainer.tsx (1)
15-26: Consider accessibility and localization.The component directly renders the status string without any accessibility attributes or localization. Consider the following improvements:
- Accessibility: Add an
aria-live="polite"attribute to the status text container so screen readers announce status changes.- Localization: If the app supports multiple languages, the status text should be localized. If not, consider documenting that status strings are intentionally user-facing.
Example:
- <div className="text-white font-normal text-[20px] mt-3">{status}</div> + <div + className="text-white font-normal text-[20px] mt-3" + aria-live="polite" + role="status" + > + {status} + </div>src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (1)
384-436: Consider completing the commented-out timestamp tests.Two timestamp tests are partially commented out (lines 418 and 432-434) with notes about "complex conditional logic". While this is acceptable for the current PR, these tests should ideally be completed to ensure full coverage of timestamp rendering.
Consider adding these to a follow-up task to improve test completeness.
src/apps/pulse/constants/constants.ts (2)
22-36: Duplication: STARTING and PENDING configs are identical.Lines 23-29 and 30-36 define identical configurations for
STARTINGandPENDINGstatuses (same icon, containerClasses, iconClasses, and color). Consider consolidating these to reduce maintenance burden.export const STATUS_CONFIG = { - [TRANSACTION_STATUSES.STARTING]: { + [TRANSACTION_STATUSES.STARTING]: (() => { + const sharedPendingConfig = { + icon: 'pending', + containerClasses: + 'w-[90px] h-[90px] rounded-full border-[3px] border-white/10 bg-[#8A77FF] flex items-center justify-center flex-shrink-0', + iconClasses: 'w-[60px] h-[60px]', + color: '#8A77FF', + }; + return sharedPendingConfig; + })(), - [TRANSACTION_STATUSES.PENDING]: { + [TRANSACTION_STATUSES.PENDING]: STATUS_CONFIG[TRANSACTION_STATUSES.STARTING], - icon: 'pending', - containerClasses: - 'w-[90px] h-[90px] rounded-full border-[3px] border-white/10 bg-[#8A77FF] flex items-center justify-center flex-shrink-0', - iconClasses: 'w-[60px] h-[60px]', - color: '#8A77FF', - },Alternatively, extract the shared config to a constant and reference it in both keys.
15-20: Consider extracting color palette constants.The hex colors (
#8A77FF,#FFAB36,#5CFF93,#FF366C) are repeated across STATUS_COLORS, STATUS_CONFIG, and BUTTON_CONFIG. Extracting these to named palette constants would improve maintainability and ensure consistency.const COLORS = { PURPLE: '#8A77FF', ORANGE: '#FFAB36', GREEN: '#5CFF93', RED: '#FF366C', } as const; export const STATUS_COLORS = { [TRANSACTION_STATUSES.STARTING]: COLORS.PURPLE, [TRANSACTION_STATUSES.PENDING]: COLORS.ORANGE, [TRANSACTION_STATUSES.COMPLETE]: COLORS.GREEN, [TRANSACTION_STATUSES.FAILED]: COLORS.RED, };src/apps/pulse/components/Transaction/TransactionInfo.tsx (1)
53-132: Consider reducing parameter count in detailsEntry.The
detailsEntryhelper accepts 7 parameters, which can be hard to track. Consider using an options object for optional parameters.const detailsEntry = ( lhs: string, rhs: string, options?: { showExternalLink?: boolean; showCopy?: boolean; tooltipContent?: string; transactionHash?: string; txChainId?: number; } ) => { const { showExternalLink = false, showCopy = false, tooltipContent = '', transactionHash, txChainId, } = options || {}; // ... rest of implementation };src/apps/pulse/hooks/useTechnicalDetails.ts (1)
98-121: Nested ternary is hard to parse.The triple-nested ternary for the
tokenfield is difficult to read and maintain, despite the eslint-disable comment. Consider extracting this logic into a helper function.const getTokenDetails = (): TokenDetails | null => { if (isBuy && buyToken) { return { symbol: buyToken.symbol, name: buyToken.name, address: buyToken.address || 'N/A', chainId, amount: tokenAmount || 'N/A', logo: buyToken.logo || 'N/A', type: 'BUY_TOKEN' as const, } satisfies TokenDetails; } if (!isBuy && sellToken) { return { symbol: sellToken.symbol, name: sellToken.name, address: sellToken.address || 'N/A', chainId, amount: tokenAmount || 'N/A', logo: sellToken.logo || 'N/A', type: 'SELL_TOKEN' as const, } satisfies TokenDetails; } return null; }; const details: TechnicalDetails = { // ... token: getTokenDetails(), // ... };src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx (4)
1-2: Reconsider broad ESLint disables.Disabling
@typescript-eslint/no-explicit-anyandreact/jsx-props-no-spreadingfor the entire file weakens type safety. Limit these to specific lines where necessary, or refactor to avoidanytypes and object spreading.
18-22: Mock return value lacks type safety.The mock returns an object without type constraints. Consider typing the return value to match
useTransactionKit's actual return type.-vi.mock('../../../../hooks/useTransactionKit', () => ({ - default: () => ({ - walletAddress: '0x1234567890123456789012345678901234567890', - }), -})); +vi.mock('../../../../hooks/useTransactionKit', () => ({ + default: (): { walletAddress: string } => ({ + walletAddress: '0x1234567890123456789012345678901234567890', + }), +}));
309-333: Handle undefined callback more gracefully.The test uses
if (onEscapeCallback)but the callback should always be defined when the hook is called. Consider asserting its presence instead.// Simulate ESC key press - if (onEscapeCallback) { - act(() => { - onEscapeCallback!(); - }); - } + expect(onEscapeCallback).toBeDefined(); + act(() => { + onEscapeCallback!(); + });
392-416: Same pattern for click-outside callback.Similar to the ESC handler, assert that the callback is defined rather than conditionally checking it.
// Simulate click outside - if (callback) { - act(() => { - callback!(); - }); - } + expect(callback).toBeDefined(); + act(() => { + callback!(); + });src/apps/pulse/types/types.ts (1)
167-200: TechnicalDetails interface is comprehensive for debugging.The nested structure with dedicated sections (transactionHashes, chains, timestamps, error, gas, stepStatus) supports detailed error diagnostics. The
[key: string]: stringpattern intransactionHashes,timestamps, andstepStatusprovides flexibility for dynamic keys.Minor suggestion: Consider using more specific key types instead of
[key: string]for better type safety if the keys are known at compile time.src/apps/pulse/components/Transaction/TransactionDetails.tsx (1)
264-272: Consider adding keyboard Enter support for the Done button.The button correctly triggers
onDoneon click, but keyboard users navigating with Tab+Enter would benefit from the Enter key callingonDonewhen focused. TheuseKeyboardNavigationhook supportsonEnterbut it's not currently configured.Optional enhancement to improve keyboard accessibility:
useKeyboardNavigation({ onEscape: () => { if ( status === 'Transaction Complete' || status === 'Transaction Failed' ) { onDone(); } else if (status === 'Transaction Pending') { onDone(); } }, + onEnter: onDone, });Note: This would make Enter trigger Done from anywhere in the modal, not just when the button is focused. If you want Enter only on button focus, the native button behavior already handles that, so this change is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/apps/pulse/components/Transaction/tests/__snapshots__/TransactionDetails.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Transaction/tests/__snapshots__/TransactionStatus.test.tsx.snapis excluded by!**/*.snapsrc/apps/pulse/components/Transaction/tests/__snapshots__/TransactionStatusContainer.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
src/apps/pulse/components/Transaction/ProgressStep.tsx(5 hunks)src/apps/pulse/components/Transaction/TransactionDetails.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionInfo.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionStatus.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionStatusButton.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionStatusContainer.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx(1 hunks)src/apps/pulse/components/Transaction/tests/TransactionStatusContainer.test.tsx(1 hunks)src/apps/pulse/constants/constants.ts(1 hunks)src/apps/pulse/hooks/tests/useClickOutside.test.ts(1 hunks)src/apps/pulse/hooks/tests/useKeyboardNavigation.test.ts(1 hunks)src/apps/pulse/hooks/useClickOutside.ts(1 hunks)src/apps/pulse/hooks/useKeyboardNavigation.ts(1 hunks)src/apps/pulse/hooks/useTechnicalDetails.ts(1 hunks)src/apps/pulse/hooks/useTransactionStatus.ts(1 hunks)src/apps/pulse/types/types.ts(1 hunks)src/apps/pulse/utils/tests/utils.test.tsx(1 hunks)src/apps/pulse/utils/utils.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
src/apps/pulse/components/Transaction/TransactionStatusButton.tsx (2)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)src/apps/pulse/utils/utils.tsx (1)
getButtonConfig(43-45)
src/apps/pulse/components/Transaction/TransactionStatusContainer.tsx (1)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)
src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
src/apps/pulse/types/types.ts (1)
UseKeyboardNavigationOptions(161-165)
src/apps/pulse/hooks/useClickOutside.ts (1)
src/apps/pulse/types/types.ts (1)
UseClickOutsideOptions(155-159)
src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx (2)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)src/apps/pulse/utils/utils.tsx (1)
getStatusConfig(36-38)
src/apps/pulse/components/Transaction/TransactionDetails.tsx (9)
src/apps/pulse/types/types.ts (1)
TransactionDetailsProps(100-124)src/apps/pulse/hooks/useTransactionStatus.ts (1)
useTransactionStatus(20-53)src/apps/pulse/hooks/useTechnicalDetails.ts (1)
useTechnicalDetails(49-219)src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)src/apps/pulse/utils/utils.tsx (1)
formatStepTimestamp(79-94)src/apps/pulse/components/Misc/Esc.tsx (1)
Esc(7-36)src/apps/pulse/components/Misc/CloseButton.tsx (1)
CloseButton(7-46)src/utils/number.tsx (2)
formatExponentialSmallNumber(119-149)limitDigitsNumber(42-69)
src/apps/pulse/components/Transaction/TransactionStatus.tsx (3)
src/apps/pulse/types/types.ts (1)
TransactionStatusProps(73-98)src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)
src/apps/pulse/utils/tests/utils.test.tsx (1)
src/apps/pulse/utils/utils.tsx (12)
truncateHash(21-24)getStatusColor(29-31)getStatusConfig(36-38)getButtonConfig(43-45)isValidHash(50-52)sanitizeErrorDetails(57-64)formatTimestamp(69-74)formatStepTimestamp(79-94)getStatusInfo(99-110)getStepStatus(115-171)determineFailureStep(176-195)canCloseTransaction(200-204)
src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (5)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)src/apps/pulse/hooks/useTransactionStatus.ts (1)
useTransactionStatus(20-53)src/apps/pulse/hooks/useTechnicalDetails.ts (1)
useTechnicalDetails(49-219)
src/apps/pulse/utils/utils.tsx (2)
src/apps/pulse/types/types.ts (3)
TransactionStatusState(1-5)TransactionStep(7-11)StepStatus(13-13)src/apps/pulse/constants/constants.ts (3)
STATUS_COLORS(15-20)STATUS_CONFIG(22-51)BUTTON_CONFIG(53-78)
src/apps/pulse/hooks/tests/useKeyboardNavigation.test.ts (1)
src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)
src/apps/pulse/hooks/useTechnicalDetails.ts (2)
src/apps/pulse/types/types.ts (7)
TransactionStatusState(1-5)TokenInfo(15-20)SellOffer(22-25)PayingToken(27-36)TechnicalDetails(167-200)TokenDetails(38-46)BuyModeDetails(48-52)src/apps/pulse/utils/utils.tsx (3)
getStepStatus(115-171)sanitizeErrorDetails(57-64)determineFailureStep(176-195)
src/apps/pulse/components/Transaction/TransactionInfo.tsx (4)
src/apps/pulse/types/types.ts (1)
TransactionInfoProps(126-139)src/utils/blockchain.ts (1)
getBlockScan(225-246)src/apps/pulse/utils/utils.tsx (3)
getStatusInfo(99-110)isValidHash(50-52)truncateHash(21-24)src/apps/pulse/components/Sell/TransactionInfo.tsx (1)
TransactionInfoProps(29-167)
src/apps/pulse/hooks/tests/useClickOutside.test.ts (1)
src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)
src/apps/pulse/hooks/useTransactionStatus.ts (2)
src/apps/pulse/types/types.ts (2)
TransactionStatusState(1-5)TransactionStep(7-11)src/apps/pulse/utils/utils.tsx (1)
getStepStatus(115-171)
src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx (3)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)
⏰ 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 (81)
src/apps/pulse/hooks/tests/useClickOutside.test.ts (5)
38-65: LGTM!The test correctly validates that the callback is invoked when clicking outside the referenced element with condition true. DOM setup, event dispatch, and cleanup are properly handled.
67-94: LGTM!Correctly validates that the callback is suppressed when condition is false.
96-125: LGTM!Properly tests the null ref guard to ensure the callback is not invoked when ref.current is null.
127-146: LGTM!Validates that the hook cleans up the event listener on unmount.
148-191: LGTM!Thoroughly tests dynamic condition changes by using rerender and verifying that the callback respects the updated condition.
src/apps/pulse/utils/utils.tsx (6)
29-52: LGTM!The configuration lookup functions have appropriate fallbacks, and
isValidHashcorrectly validates hash presence.
69-94: LGTM!Timestamp formatting functions correctly use date-fns and provide appropriate UI-specific formatting for different step contexts.
99-110: LGTM!Status-to-UI mapping is clear and provides appropriate text and color for each state.
115-171: LGTM! Complex but well-structured.This function correctly handles the nuanced per-step status logic for Buy vs. Sell flows, including resource lock and completion hash checks. The logic is covered by tests (per the PR context), which is essential given the complexity.
176-195: LGTM!Correctly determines the failure step by checking resource lock failure and step statuses in order.
200-204: LGTM!Correctly restricts closing to terminal states (Complete or Failed).
src/apps/pulse/components/Transaction/TransactionStatusButton.tsx (3)
22-52: LGTM!The
renderIconhelper correctly maps each status to its corresponding icon or spinner.
54-60: LGTM!Correctly renders a non-interactive message for the Starting Transaction state.
62-81: LGTM!Button structure is correct with proper type attribute, dynamic styling, and memoization.
src/apps/pulse/components/Transaction/tests/TransactionStatusContainer.test.tsx (4)
9-28: LGTM!Mock implementations are appropriate for isolating the container's behavior.
37-129: LGTM!Rendering tests provide good coverage across all status states with appropriate assertions.
131-160: LGTM!Interaction tests correctly verify callback invocation and prop passing.
162-182: LGTM!Layout test appropriately validates the container's CSS classes.
src/apps/pulse/hooks/useKeyboardNavigation.ts (4)
7-11: LGTM!The hook signature is clean and follows React conventions. The default value for
enabled=trueis appropriate for the common case.
12-15: LGTM!The early return pattern for the disabled state is correct and prevents unnecessary listener registration.
17-27: Verify that Enter preventDefault doesn't interfere with form inputs.The handler unconditionally calls
preventDefault()for both Escape and Enter keys. In contexts where form inputs might be focused (e.g., if transaction UI ever includes text fields), this would prevent form submissions or other native Enter behaviors.For the current transaction status UI, this is likely acceptable, but please verify that no inputs exist in the same document scope where Enter should retain its default behavior.
29-33: LGTM!The event listener registration and cleanup pattern is correct. The cleanup function ensures no memory leaks on unmount or dependency changes.
src/apps/pulse/hooks/useTransactionStatus.ts (4)
9-15: LGTM!The interface definition is clear and complete. Optional properties are appropriately marked.
20-26: LGTM!The hook signature is clean and follows React conventions. The default value for
isResourceLockFailed = falseis appropriate.
45-47: LGTM!The
canClosememoization is appropriate and the logic correctly identifies terminal states.
49-52: LGTM!The return object is clear and provides a clean API for consumers.
src/apps/pulse/components/Transaction/TransactionStatusContainer.tsx (2)
10-13: LGTM!The interface definition is clear and complete.
28-28: LGTM! Ensure parent provides stableonViewDetailscallback.The component is appropriately memoized. However, the memoization will only be effective if the parent provides a stable
onViewDetailscallback (e.g., wrapped withuseCallback). Without a stable callback, the component will re-render on every parent render.src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (10)
1-6: LGTM!The imports are clean and appropriate for the test suite.
19-106: LGTM!The mocking strategy is comprehensive and appropriate for isolating the component under test. Subcomponents and hooks are properly mocked.
108-146: LGTM!The
basePropsobject is well-structured and provides comprehensive default data for tests. This is a good practice for maintainability.
148-214: LGTM!The
beforeEachsetup is thorough and ensures clean state for each test. Default mock implementations are reasonable and provide a solid baseline.
216-279: LGTM!The rendering tests provide good coverage of different transaction statuses and verify the expected UI elements are present or hidden accordingly.
281-302: LGTM!The buy mode test correctly verifies that the ResourceLock step is rendered for buy transactions.
438-568: LGTM!The user interaction tests are comprehensive and well-structured. They correctly simulate hook behavior and verify that callbacks are invoked appropriately based on transaction status.
570-590: LGTM!The error state tests correctly verify that the error box is displayed for failed transactions and that resource lock failures are handled appropriately in buy mode.
592-658: LGTM!The hook integration tests are thorough and verify that the component correctly passes props to its dependencies. This is valuable for catching integration issues.
660-684: LGTM!The props passing test correctly verifies that the component forwards props to the TransactionInfo subcomponent.
src/apps/pulse/hooks/tests/useKeyboardNavigation.test.ts (9)
25-43: LGTM!The test correctly verifies that
onEscapeis called when the Escape key is pressed.
45-63: LGTM!The test correctly verifies that
onEnteris called when the Enter key is pressed and the callback is provided.
65-82: LGTM!The test correctly verifies that
onEnteris not called when the callback is not provided.
84-102: LGTM!The test correctly verifies that key handlers are not invoked when the hook is disabled.
123-146: LGTM!The test correctly verifies that non-target keys are ignored. Good coverage of edge cases.
148-169: LGTM!The test correctly verifies that
preventDefaultis called for handled keys.
171-190: LGTM!The test correctly verifies that the event listener is cleaned up on unmount. This is important for preventing memory leaks.
192-223: LGTM!The test correctly verifies that the hook responds to changes in the
enabledprop across rerenders. Good coverage of dynamic state changes.
225-260: LGTM!The test correctly verifies that the hook responds to callback changes across rerenders. Good coverage of dynamic callback updates.
src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx (1)
23-33: Clean implementation with good memoization.The icon mapping logic is clear and the use of React.memo is appropriate for this presentational component.
src/apps/pulse/hooks/useTechnicalDetails.ts (1)
73-192: Well-structured hook with comprehensive memoization.The hook correctly assembles the technical details payload and includes all relevant dependencies in the useMemo array. The use of
satisfiesfor type safety and helper functions for step status logic is good practice.src/apps/pulse/utils/tests/utils.test.tsx (3)
263-357: Comprehensive step status test coverage.The test suite covers all transaction states, Buy/Sell modes, resource lock scenarios, and failure cases. This provides strong confidence in the
getStepStatusutility logic.
359-402: Good use of mocking for failure step determination.The tests properly mock the
getStepStatusfunction to verify each failure path. The sequential mock return values clearly demonstrate the branching logic.
169-175: sanitizeErrorDetails test expectation is correct
The regex/api-key=[A-Za-z0-9+/=]+/gmatches onlyapi-key=test, so the test’s output (Error with api-key=***REDACTED***-api-key-123 and other data) aligns with actual behavior.src/apps/pulse/components/Transaction/tests/TransactionStatus.test.tsx (3)
67-105: LGTM! Comprehensive base props.The
basePropsobject provides thorough test data covering all required and optional fields. This enables robust testing across various scenarios.
442-482: LGTM! Hook integration tests are thorough.These tests validate that hooks are called with correct parameters for different transaction states, ensuring proper integration between the component and its hooks.
183-213: Timer cleanup verified. All three tests that callvi.useFakeTimers()also invokevi.useRealTimers(), so no further action is needed.src/apps/pulse/components/Transaction/TransactionStatus.tsx (6)
44-58: Effect cleanup correctly handles timer.The effect properly cleans up the timeout and returns
undefinedwhen conditions aren't met, preventing memory leaks and ensuring correct behavior across re-renders.
60-66: Click-outside only active for terminal states.Good defensive design—click-outside is only enabled for completed/failed states, preventing accidental dismissal during pending transactions.
68-79: Keyboard navigation logic is status-aware.The ESC key behavior correctly differentiates between terminal states (close entirely) and pending state with details open (collapse to container). This provides intuitive navigation.
81-122: Component structure is clean and well-organized.The component logic is straightforward: it manages detail visibility, integrates hooks for interactions, and conditionally renders the appropriate child component with comprehensive props. The structure supports maintainability.
109-109: Confirm sell flow handles missing completedTxHash
EnsureTransactionDetailsgracefully handles an undefinedcompletedTxHash(whenisBuyis false) without breaking the UI or displaying empty data.
105-105: Conditional txHash assignment is correct
txHash is intentionally undefined for buy transactions; TransactionInfo’sisBuybranch renderscompletedTxHashinstead, so no changes are needed.src/apps/pulse/components/Transaction/ProgressStep.tsx (7)
2-2: LGTM! Type externalization improves maintainability.Moving
ProgressStepPropsto a shared types file promotes reusability and consistency across components.
38-40: Line status logic covers all states.The line rendering correctly handles active states (completed, pending, failed) with purple and inactive with dark background. This provides clear visual progression.
67-70: Icon sizing increased for better visibility.Changing from
w-2tow-3and addingstrokeWidth="2"improves visual clarity of the checkmark icon.
98-116: Pending spinner logic extended for ResourceLock and Completed.The component now shows spinners for both ResourceLock and Completed steps when pending, and correctly renders nothing for inactive status. This aligns with the multi-step buy flow.
126-133: Responsive timestamp display is user-friendly.The implementation shows full timestamps on larger screens and time-only on small screens using Tailwind's responsive utilities, improving mobile UX.
53-61: Timestamp parsing assumes specific format. This helper extracts the text after a bullet (“•”). Confirm that every timestamp string consistently includes exactly one bullet prefix; otherwise, add validation or a fallback branch to handle missing or multiple bullets.
23-35: No change required. The ResourceLock step correctly renders as pending (with spinner) for buy transactions, per thegetStepStatusimplementation and existingProgressStep.test.tsxcoverage.src/apps/pulse/types/types.ts (6)
1-13: LGTM! Core type unions are well-defined.The union types for
TransactionStatusState,TransactionStep, andStepStatusprovide clear state machines for the transaction flow. String literal unions are appropriate here for type safety and enable exhaustive pattern matching.
15-46: LGTM! Token-related interfaces are comprehensive.The token data structures (
TokenInfo,SellOffer,PayingToken,TokenDetails) capture all necessary fields for both Buy and Sell flows. Thetypediscriminator inTokenDetailsenables proper type narrowing.
48-71: LGTM! Configuration interfaces support UI composition.
BuyModeDetails,TransactionStatusConfig,ButtonConfig, andProgressStepConfigappropriately separate UI configuration from business logic.
100-124: LGTM! TransactionDetailsProps mirrors TransactionStatusProps appropriately.The props interface includes all necessary fields for the details view. The optional field pattern is consistent with
TransactionStatusProps.
126-165: LGTM! Component and hook props are well-typed.
TransactionInfoProps,ProgressStepProps,TransactionErrorBoxProps,UseClickOutsideOptions, andUseKeyboardNavigationOptionsprovide clear contracts for their respective consumers.
73-98: No unsafe optional accesses found
All optional props in TransactionStatusProps are accessed with optional chaining or fallback defaults throughout consuming components.src/apps/pulse/components/Transaction/TransactionDetails.tsx (7)
1-26: LGTM! Imports are well-organized.Dependencies are grouped logically (components, hooks, types, utils) and all necessary imports are present.
27-60: LGTM! Hook initialization follows best practices.The component properly extracts account address, initializes transaction status logic, and configures technical details for debugging. Hook dependency patterns are correct.
87-104: LGTM! Event handlers are correctly implemented.The
useClickOutsidehook properly guards with thecanClosecondition, anduseKeyboardNavigationcorrectly differentiates behavior for Pending vs Complete/Failed states. The past review concern about duplicate ESC listeners has been addressed (no component-level useEffect duplicating the behavior).
177-242: LGTM! Progress step rendering correctly handles Buy vs Sell flows.The conditional rendering (lines 194-238) appropriately shows different step sequences:
- Buy: Submitted → ResourceLock → Completed
- Sell: Submitted → Pending → Completed
Dynamic labels for failed states (lines 208-210, 230-232) provide clear user feedback.
244-258: LGTM! TransactionInfo receives all required props.The component is passed comprehensive transaction data including optional Buy-specific fields for multi-chain transactions.
260-262: LGTM! Error handling is appropriately conditional.
TransactionErrorBoxonly renders for failed transactions and receives sanitized technical details.
106-133: Verify and cover step‐timestamp logic
- Confirm with UX/product whether the final “Completed” step should ever render a timestamp (the code currently returns
undefinedwhenstep === 'Completed').- Ensure falling back to
pendingCompletedAtfor the ‘ResourceLock’ step in buy flows matches business requirements.- Add unit tests for
getStepTimestampcovering both the “Completed” step and the ‘ResourceLock’ fallback scenario.
src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/apps/pulse/hooks/tests/useKeyboardNavigation.test.ts (1)
143-164: Consider verifying preventDefault more reliably.While this test works, assigning
preventDefaultafter event construction can be fragile. Consider usingObject.definePropertyfor better cross-environment reliability:- escapeEvent.preventDefault = mockPreventDefault; + Object.defineProperty(escapeEvent, 'preventDefault', { + value: mockPreventDefault, + writable: true, + });However, since the current approach works in the test environment, this is an optional improvement.
src/apps/pulse/utils/tests/utils.test.tsx (1)
209-227: LGTM!The
formatStepTimestamptests validate that JSX is returned for different step types. The current assertions (toBeDefinedandtypeof 'object') provide basic coverage. For more robust validation, consider using@testing-library/reactto render and assert on the JSX structure.src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (2)
4-4: Migrate away from deprecatedreact-test-renderer.The
react-test-rendererpackage is deprecated by the React team and will emit warnings on React 19+. Since this project already uses@testing-library/react, you can replace snapshot testing withasFragment()orcontainer.innerHTMLserialization.Replace the snapshot test on lines 217-222 with:
-import renderer from 'react-test-renderer'; it('renders correctly and matches snapshot', () => { - const tree = renderer - .create(<TransactionDetails {...baseProps} />) - .toJSON(); - expect(tree).toMatchSnapshot(); + const { asFragment } = render(<TransactionDetails {...baseProps} />); + expect(asFragment()).toMatchSnapshot(); });Based on learnings.
281-302: Consider expanding Buy mode test coverage.The current Buy mode tests only verify that the resource lock step is present. Consider adding tests for:
- Buy-specific props (
payingTokens,usdAmount)- Buy mode with completed resource lock timestamp
- Buy mode failure scenarios (resource lock failed)
Example additional test:
it('renders buy mode with paying tokens and USD amount', () => { const payingTokens = [ { symbol: 'USDC', amount: 50, totalUsd: 50 }, { symbol: 'DAI', amount: 50, totalUsd: 50 }, ]; render( <TransactionDetails {...baseProps} isBuy buyToken={{ name: 'ETH', symbol: 'ETH', logo: 'eth.png', address: '0x...', }} payingTokens={payingTokens} usdAmount="100" sellToken={null} /> ); // Assert TransactionInfo receives correct Buy mode props // Assert technical details include payingTokens data });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx(1 hunks)src/apps/pulse/hooks/tests/useKeyboardNavigation.test.ts(1 hunks)src/apps/pulse/utils/tests/utils.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/apps/pulse/hooks/tests/useKeyboardNavigation.test.ts (1)
src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)
src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (5)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)src/apps/pulse/hooks/useTransactionStatus.ts (1)
useTransactionStatus(20-53)src/apps/pulse/hooks/useTechnicalDetails.ts (1)
useTechnicalDetails(49-219)
src/apps/pulse/utils/tests/utils.test.tsx (1)
src/apps/pulse/utils/utils.tsx (12)
truncateHash(21-24)getStatusColor(29-31)getStatusConfig(36-38)getButtonConfig(43-45)isValidHash(50-52)sanitizeErrorDetails(57-64)formatTimestamp(69-74)formatStepTimestamp(79-94)getStatusInfo(99-110)getStepStatus(115-171)determineFailureStep(176-195)canCloseTransaction(200-204)
⏰ 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 (22)
src/apps/pulse/hooks/tests/useKeyboardNavigation.test.ts (10)
1-18: LGTM! Clean test setup.The imports are appropriate, and the setup/teardown correctly initializes mocks without the previously flagged ineffective cleanup calls.
20-38: LGTM!Test correctly verifies that the Escape key triggers the onEscape callback when enabled.
40-58: LGTM!Test correctly verifies that the Enter key triggers the onEnter callback when provided and enabled.
60-77: LGTM!Test correctly verifies that Enter key does nothing when the onEnter callback is not provided.
79-97: LGTM!Test correctly verifies that key handlers are inactive when enabled is false.
99-116: LGTM! Default enabled behavior verified.Test correctly verifies that the hook handles keys when the enabled prop is omitted (defaulting to true). Previous test description issue has been resolved.
118-141: LGTM! Good coverage of ignored keys.Test correctly verifies that the hook ignores keys other than Escape and Enter, with good coverage of different key types.
166-185: LGTM! Proper cleanup verification.Test correctly verifies that the hook removes the event listener on unmount and properly restores the spy.
187-218: LGTM! Excellent dynamic behavior test.Test correctly verifies that the hook properly responds to changes in the enabled prop during rerenders.
220-254: LGTM! Excellent callback update test.Test correctly verifies that the hook properly responds to callback reference changes during rerenders, ensuring the latest callback is always invoked.
src/apps/pulse/utils/tests/utils.test.tsx (11)
20-43: LGTM!The
truncateHashtests correctly validate both typical hash truncation and edge cases (empty string, dash).
45-61: LGTM!The
getStatusColortests correctly validate all status-to-color mappings.
63-107: LGTM!The
getStatusConfigtests comprehensively validate icon, styling, and color configurations for all transaction statuses.
109-149: LGTM!The
getButtonConfigtests correctly verify button styling and labels for all transaction statuses.
151-166: LGTM!The
isValidHashtests correctly validate the function's logic (non-empty and not'-'). The test description accurately reflects the behavior following the previous fix.
168-193: LGTM!The
sanitizeErrorDetailstests correctly validate API key redaction, including multiple keys, absence of keys, and empty input.
195-207: LGTM!The
formatTimestamptests correctly validate both successful formatting and invalid date handling.
229-261: LGTM!The
getStatusInfotests correctly validate status text and color mappings for all transaction statuses.
263-357: LGTM!The
getStepStatustests comprehensively validate step status determination across transaction states, buy/sell modes, and resource lock scenarios. The critical paths are well covered.
359-402: LGTM!The
determineFailureSteptests correctly validate failure step identification using mocked step status functions, covering all major failure scenarios.
404-420: LGTM!The
canCloseTransactiontests correctly validate closability logic for all transaction statuses.src/apps/pulse/components/Transaction/tests/TransactionDetails.test.tsx (1)
148-684: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of the TransactionDetails component:
✅ Status variations (Starting, Pending, Complete, Failed)
✅ Buy vs Sell modes
✅ User interactions (keyboard, mouse, buttons)
✅ Hook integration with parameter validation
✅ Error states and resource lock failures
✅ Props passing to child componentsThe test structure is well-organized with clear describe blocks and meaningful test names.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx (2)
33-36: Initial spring config includes unusedrotateproperty.The initial spring configuration defines
rotate: '0deg'but this property is only used in the seconduseEffect(lines 97-113) for completion/failure states. The heartbeat animation (lines 38-94) only usesscaleandfilter.Consider splitting into two separate springs for clarity:
- const [springs, api] = useSpring(() => ({ - from: { scale: 1, filter: 'blur(0px)', rotate: '0deg' }, - to: { scale: 1, filter: 'blur(0px)', rotate: '0deg' }, - })); + const [heartbeatSprings, heartbeatApi] = useSpring(() => ({ + from: { scale: 1, filter: 'blur(0px)' }, + to: { scale: 1, filter: 'blur(0px)' }, + })); + + const [rotationSprings, rotationApi] = useSpring(() => ({ + from: { rotate: '0deg', scale: 1 }, + to: { rotate: '0deg', scale: 1 }, + }));Then combine both in the animated.div style prop.
38-94: Deeply nestedonRestcallbacks create complex animation chain.The heartbeat animation uses four levels of nested
onRestcallbacks (lines 46-82), making the control flow difficult to follow and maintain. Each level also checksshouldAnimateRef.currentbefore proceeding.Consider using
async/awaitwith react-spring's promise-based API or extract the sequence into a separate function:+ const runHeartbeatSequence = async () => { + if (!shouldAnimateRef.current) return; + + // Step 1: bigger + blurry + await api.start({ to: { scale: 1.1, filter: 'blur(1px)' }, config: { duration: 100 } }); + if (!shouldAnimateRef.current) return; + + // Step 2: back to normal + await api.start({ to: { scale: 1, filter: 'blur(0px)' }, config: { duration: 100 } }); + if (!shouldAnimateRef.current) return; + + // Step 3: bigger + blurry + await api.start({ to: { scale: 1.1, filter: 'blur(1px)' }, config: { duration: 100 } }); + if (!shouldAnimateRef.current) return; + + // Step 4: back to normal with longer duration + await api.start({ to: { scale: 1, filter: 'blur(0px)' }, config: { duration: 700 } }); + + // Restart if still animating + if (shouldAnimateRef.current) { + runHeartbeatSequence(); + } + }; + useEffect(() => { if (shouldAnimate) { - const startHeartbeat = () => { /* nested callbacks */ }; - startHeartbeat(); + runHeartbeatSequence(); } else { api.stop(); api.start({ to: { scale: 1, filter: 'blur(0px)' }, config: { duration: 200 } }); } }, [shouldAnimate, api]);src/apps/pulse/components/Transaction/TransactionStatusButton.tsx (2)
62-92: Extract and deduplicate icon container styling.The
renderIconfunction contains three nearly identical circular container divs (lines 65-68, 73-80, 85-88) with only the colors differing. This duplication makes maintenance harder and increases bundle size.Extract a shared component or helper function:
+const IconContainer: React.FC<{ + borderColor: string; + bgColor: string; + children: React.ReactNode; +}> = ({ borderColor, bgColor, children }) => ( + <div + className={`w-[14px] h-[14px] rounded-full border flex items-center justify-center flex-shrink-0`} + style={{ + borderColor, + backgroundColor: bgColor + }} + > + {children} + </div> +); const renderIcon = () => { if (status === 'Transaction Pending') { return ( - <div className="w-[14px] h-[14px] rounded-full border-[2px] border-[#FFAB36]/[.23] bg-[#FFAB36]/30 flex items-center justify-center flex-shrink-0"> + <IconContainer borderColor="rgba(255, 171, 54, 0.23)" bgColor="rgba(255, 171, 54, 0.3)"> <TailSpin color="#FFAB36" height={14} width={14} strokeWidth={6} /> - </div> + </IconContainer> ); } // Similar for Complete and Failed cases };
30-60: Document linear status transition flow above zoom animation effect.Add a brief comment before the
useEffectin TransactionStatusButton.tsx stating that thestatusprop is guaranteed to follow the finite state machine
“Starting Transaction → Transaction Pending → Transaction Complete or Transaction Failed,”
so the zoom animation only fires on the two intended transitions.src/apps/pulse/components/Transaction/TransactionStatus.tsx (4)
92-105: Cleanup return is correct butundefinedis unnecessary.The effect properly cleans up the timeout (line 102), and the explicit
return undefinedat line 104 is unnecessary since implicit return is equivalent in this context.Simplify the effect:
useEffect(() => { if ( (currentStatus === 'Transaction Complete' || currentStatus === 'Transaction Failed') && !showDetails ) { const timer = setTimeout(() => { setShowDetails(true); }, 1000); return () => clearTimeout(timer); } - return undefined; }, [currentStatus, showDetails]);
60-89: Bounce animation triggers on everyshowDetailstoggle.The bounce animation effect (lines 60-89) triggers whenever
showDetailschanges, including both opening and closing the details panel. This may cause unnecessary visual feedback when collapsing details (e.g., pressing Escape while in pending state).Consider animating only when expanding details:
useEffect(() => { + if (!showDetails) return; + // Quick bounce effect when size changes api.start({ to: { scale: 1.04, // ... }, // ... }); }, [showDetails, api]);Or use a previous value ref to detect the direction of the toggle:
+ const prevShowDetails = useRef(showDetails); + useEffect(() => { + if (showDetails && !prevShowDetails.current) { // Bounce animation only when expanding api.start({ /* ... */ }); + } + prevShowDetails.current = showDetails; }, [showDetails, api]);
153-153: Clarify conditional hash forwarding logic.The component forwards
txHashandcompletedTxHashconditionally based onisBuy(lines 153, 157):
txHash={isBuy ? undefined : blockchainTxHash}(sell flow only)completedTxHash={isBuy ? completedTxHash : undefined}(buy flow only)This logic implies that buy transactions use
completedTxHashwhile sell transactions usetxHash(fromblockchainTxHash). However, the reasoning isn't immediately clear from the code.Add a comment explaining the distinction:
+ // Buy flow: resource lock + completed tx (two-phase) + // Sell flow: single blockchain tx (user-op) <TransactionDetails onDone={/* ... */} userOpHash={userOpHash} chainId={chainId} status={currentStatus} isBuy={isBuy} sellToken={sellToken} buyToken={buyToken} tokenAmount={tokenAmount} sellOffer={sellOffer} payingTokens={payingTokens} usdAmount={usdAmount} submittedAt={submittedAt} pendingCompletedAt={pendingCompletedAt} - txHash={isBuy ? undefined : blockchainTxHash} + txHash={isBuy ? undefined : blockchainTxHash} // Sell: single tx gasFee={gasFee} errorDetails={errorDetails} resourceLockTxHash={resourceLockTxHash} - completedTxHash={isBuy ? completedTxHash : undefined} + completedTxHash={isBuy ? completedTxHash : undefined} // Buy: second phase resourceLockChainId={resourceLockChainId} completedChainId={completedChainId} resourceLockCompletedAt={resourceLockCompletedAt} isResourceLockFailed={isResourceLockFailed} />Also applies to: 157-157
115-126: Keyboard navigation might not handle all edge cases.The
useKeyboardNavigationhook (lines 115-126) handles Escape key behavior, but doesn't account for Enter key (which the hook supports via the optionalonEnterprop). Additionally, focus management isn't explicitly handled when the modal opens or closes.Consider:
- Adding Enter key support to confirm/proceed actions when appropriate
- Managing focus trap within the dialog when it's open
- Restoring focus to the trigger element when closing
Example enhancement:
useKeyboardNavigation({ onEscape: () => { if ( currentStatus === 'Transaction Complete' || currentStatus === 'Transaction Failed' ) { closeTransactionStatus(); } else if (currentStatus === 'Transaction Pending' && showDetails) { setShowDetails(false); } }, + onEnter: () => { + // Allow Enter to expand details when in compact view + if (!showDetails) { + setShowDetails(true); + } + }, });For focus management, consider using a library like
react-focus-lockor implementing manual focus trap logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/pulse/components/Transaction/tests/__snapshots__/TransactionStatus.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/apps/pulse/components/Transaction/TransactionStatus.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionStatusButton.tsx(1 hunks)src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/apps/pulse/components/Transaction/TransactionStatusButton.tsx (2)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)src/apps/pulse/utils/utils.tsx (1)
getButtonConfig(43-45)
src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx (2)
src/apps/pulse/types/types.ts (1)
TransactionStatusState(1-5)src/apps/pulse/utils/utils.tsx (1)
getStatusConfig(36-38)
src/apps/pulse/components/Transaction/TransactionStatus.tsx (3)
src/apps/pulse/types/types.ts (1)
TransactionStatusProps(73-98)src/apps/pulse/hooks/useClickOutside.ts (1)
useClickOutside(7-29)src/apps/pulse/hooks/useKeyboardNavigation.ts (1)
useKeyboardNavigation(7-35)
⏰ 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/Transaction/TransactionStatusButton.tsx (1)
65-67: Verify TailSpin dimensions match container.The TailSpin spinner is configured with
height={14} width={14}(line 66) to match the 14×14px container, butstrokeWidth={6}seems quite thick for such a small spinner and may cause clipping or rendering issues.Test the spinner visually at this size, or check the react-loader-spinner documentation for recommended strokeWidth values at 14px dimensions. Typical strokeWidth for small spinners is 1-3.
src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx (1)
94-94: Verify react-springapistability in useEffect deps
Confirm that theapiobject returned from react-spring is referentially stable before including it in youruseEffectdependency arrays (lines 94 & 113). If stability is guaranteed, disable the ESLint warning with an inline comment explaining why; otherwise memoize the API or remove it from the deps.
| const getIcon = () => { | ||
| switch (config.icon) { | ||
| case 'confirmed': | ||
| return ConfirmedIcon; | ||
| case 'failed': | ||
| return FailedIcon; | ||
| case 'pending': | ||
| default: | ||
| return PendingIcon; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate icon selection logic with utility function.
The getIcon function (lines 115-125) duplicates icon mapping logic. According to the relevant code snippets, src/apps/pulse/utils/utils.tsx exports a getIcon utility that's used elsewhere.
Import and use the shared getIcon utility from utils instead:
// utils
-import { getStatusConfig } from '../../utils/utils';
+import { getStatusConfig, getIcon } from '../../utils/utils';
const TransactionStatusIcon: React.FC<TransactionStatusIconProps> = ({
status,
}) => {
const config = getStatusConfig(status);
+ const iconSrc = getIcon(config.icon);
// ...
-
- const getIcon = () => {
- switch (config.icon) {
- case 'confirmed':
- return ConfirmedIcon;
- case 'failed':
- return FailedIcon;
- case 'pending':
- default:
- return PendingIcon;
- }
- };
return (
<animated.div className={config.containerClasses} style={springs}>
- <img src={getIcon()} alt={status} className={config.iconClasses} />
+ <img src={iconSrc} alt={status} className={config.iconClasses} />
</animated.div>
);📝 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.
| const getIcon = () => { | |
| switch (config.icon) { | |
| case 'confirmed': | |
| return ConfirmedIcon; | |
| case 'failed': | |
| return FailedIcon; | |
| case 'pending': | |
| default: | |
| return PendingIcon; | |
| } | |
| }; | |
| // src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx | |
| import React from 'react'; | |
| import ConfirmedIcon from '../../assets/confirm.svg'; | |
| import FailedIcon from '../../assets/fail.svg'; | |
| import PendingIcon from '../../assets/pending.svg'; | |
| // utils | |
| import { getStatusConfig, getIcon } from '../../utils/utils'; | |
| const TransactionStatusIcon: React.FC<TransactionStatusIconProps> = ({ | |
| status, | |
| }) => { | |
| const config = getStatusConfig(status); | |
| const iconSrc = getIcon(config.icon); | |
| - const getIcon = () => { | |
| - switch (config.icon) { | |
| - case 'confirmed': | |
| - return ConfirmedIcon; | |
| - case 'failed': | |
| - return FailedIcon; | |
| - case 'pending': | |
| - default: | |
| - return PendingIcon; | |
| - } | |
| - }; | |
| return ( | |
| <animated.div className={config.containerClasses} style={springs}> | |
| <img src={iconSrc} alt={status} className={config.iconClasses} /> | |
| </animated.div> | |
| ); | |
| }; | |
| export default TransactionStatusIcon; |
🤖 Prompt for AI Agents
In src/apps/pulse/components/Transaction/TransactionStatusIcon.tsx around lines
115 to 125, the local getIcon function duplicates logic that already exists in
src/apps/pulse/utils/utils.tsx; remove the local getIcon implementation, add an
import for the shared getIcon utility from src/apps/pulse/utils/utils.tsx, and
replace all uses of the local function with the imported utility (adjust any
naming or typing if there's a conflict). Ensure the imported function signature
matches how this component calls it and update any associated types/props
imports if needed.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/apps/pulse/components/App/HomeScreen.tsx (5)
39-68: Consider extracting types to shared types file.These transaction status types (TransactionStatusState, BidStatus, ResourceLockStatus, UserOpStatus) appear to be domain types that could be reused across components. According to the AI summary, these types are used via shared utilities and the TransactionStatus component.
Consider moving these to
src/apps/pulse/types/types.tsfor better reusability and maintainability:// In src/apps/pulse/types/types.ts export type TransactionStatusState = | 'Starting Transaction' | 'Transaction Pending' | 'Transaction Complete' | 'Transaction Failed'; export type BidStatus = | 'PENDING' | 'SHORTLISTING_INITIATED' | 'SHORTLISTED' | 'EXECUTED' | 'CLAIMED' | 'RESOURCE_LOCK_RELEASED' | 'FAILED_EXECUTION' | 'SHORTLISTING_FAILED'; export type ResourceLockStatus = | 'PENDING_USER_OPS_CREATION' | 'USER_OPS_CREATION_INITIATED' | 'USER_OPS_EXECUTION_SUCCESSFUL' | 'USER_OPS_EXECUTION_FAILED'; export type UserOpStatus = | 'New' | 'Pending' | 'Submitted' | 'OnChain' | 'Finalized' | 'Cancelled' | 'Reverted';Then import them in this file:
+import { + TransactionStatusState, + BidStatus, + ResourceLockStatus, + UserOpStatus, +} from '../../types/types'; - -// types -type TransactionStatusState = - | 'Starting Transaction' - | 'Transaction Pending' - | 'Transaction Complete' - | 'Transaction Failed'; - -type BidStatus = - | 'PENDING' - | 'SHORTLISTING_INITIATED' - | 'SHORTLISTED' - | 'EXECUTED' - | 'CLAIMED' - | 'RESOURCE_LOCK_RELEASED' - | 'FAILED_EXECUTION' - | 'SHORTLISTING_FAILED'; - -type ResourceLockStatus = - | 'PENDING_USER_OPS_CREATION' - | 'USER_OPS_CREATION_INITIATED' - | 'USER_OPS_EXECUTION_SUCCESSFUL' - | 'USER_OPS_EXECUTION_FAILED'; - -type UserOpStatus = - | 'New' - | 'Pending' - | 'Submitted' - | 'OnChain' - | 'Finalized' - | 'Cancelled' - | 'Reverted';
124-161: Consider consolidating transaction status state.This component manages 17+ transaction status-related state variables. For better maintainability, consider consolidating related state into a single object managed by
useReduceror extracting the polling logic into a custom hook (e.g.,useTransactionStatusPolling).Example with
useReducer:type TransactionPollingState = { currentStatus: TransactionStatusState; errorDetails: string; submittedAt?: Date; pendingCompletedAt?: Date; blockchainTxHash?: string; resourceLockTxHash?: string; completedTxHash?: string; completedChainId?: number; resourceLockChainId?: number; resourceLockCompletedAt?: Date; isResourceLockFailed: boolean; isPollingActive: boolean; statusStartTime: number; hasSeenSuccess: boolean; failureTimeoutId: ReturnType<typeof setTimeout> | null; isBackgroundPolling: boolean; shouldAutoReopen: boolean; }; const [pollingState, dispatch] = useReducer(pollingReducer, initialState);Or extract to a custom hook:
const { currentStatus, errorDetails, // ... other values stopPolling, } = useTransactionStatusPolling({ userOpHash, transactionData, intentSdk, });
317-338: Consider extracting status mapping to shared utility.The mapping logic is correct, but according to the AI summary,
mapBidStatusToTransactionStatusis used via shared utilities (src/apps/pulse/utils/utils.tsx). Consider moving this function there for reusability across components.Move to
src/apps/pulse/utils/utils.tsx:export const mapBidStatusToTransactionStatus = ( status: BidStatus ): TransactionStatusState => { // ... same implementation };
341-357: Consider extracting status mapping to shared utility.Similar to
mapBidStatusToTransactionStatus, this function should be moved to a shared utility file for reusability, as indicated by the AI summary.Move to
src/apps/pulse/utils/utils.tsx:export const mapUserOpStatusToTransactionStatus = ( status: UserOpStatus ): TransactionStatusState => { // ... same implementation };
548-627: Add validation for Intent SDK API responses.The Buy flow accesses nested properties from Intent SDK responses without validating the structure beyond optional chaining. The type cast at line 570 (
as ResourceLockStatus) assumes the API returns valid status strings without validation.Add validation before type casting:
const resourceLockStatus: ResourceLockStatus | undefined = - resourceLock?.resourceLockInfo?.status as ResourceLockStatus; + resourceLock?.resourceLockInfo?.status && + ['PENDING_USER_OPS_CREATION', 'USER_OPS_CREATION_INITIATED', + 'USER_OPS_EXECUTION_SUCCESSFUL', 'USER_OPS_EXECUTION_FAILED'].includes( + resourceLock.resourceLockInfo.status + ) + ? (resourceLock.resourceLockInfo.status as ResourceLockStatus) + : undefined;Similarly for
bidStatus:const bidStatus: BidStatus | undefined = bid?.bidStatus; +// Add validation if bidStatus doesn't already match the BidStatus type +const validBidStatuses = ['PENDING', 'SHORTLISTING_INITIATED', 'SHORTLISTED', + 'EXECUTED', 'CLAIMED', 'RESOURCE_LOCK_RELEASED', + 'FAILED_EXECUTION', 'SHORTLISTING_FAILED']; +if (bidStatus && !validBidStatuses.includes(bidStatus)) { + console.warn('Unexpected bid status:', bidStatus); + return; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/pulse/components/App/HomeScreen.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/apps/pulse/components/App/HomeScreen.tsx (4)
src/apps/pulse/hooks/useIntentSdk.ts (1)
useIntentSdk(9-53)src/apps/pulse/types/tokens.ts (1)
SelectedToken(1-10)src/apps/pulse/hooks/useRelaySell.ts (1)
SellOffer(37-43)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)
⏰ 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 (5)
src/apps/pulse/components/App/HomeScreen.tsx (5)
165-179: Token amount calculation looks correct.The calculation logic correctly mirrors PreviewBuy's approach (totalPay / tokenUsdValue), and the dependencies are appropriate since
payingTokensreference changes trigger the effect when needed.
244-254: LGTM! Preview close handlers correctly stop polling.Both handlers appropriately call
stopTransactionPolling()to clean up polling state when previews are closed.
301-314: Background polling logic looks good.The function correctly handles both final and intermediate states: stopping polling for final states and enabling background polling with auto-reopen for intermediate states.
496-529: Step completion tracking logic looks correct.The effect properly tracks completion timestamps for each transaction step, setting them only once using the
|| operatorpattern. Dependencies are appropriate.
763-797: TransactionStatus props wiring looks comprehensive.All required props are correctly passed, including the fixed
chainIdlogic that usesbuyToken.chainIdfor Buy transactions andsellToken.chainIdfor Sell transactions.
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3681/transaction-status-pulse)
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Changes
Removed
Style
Tests