fix: a bunch of bug fixes #135
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces streaming pause detection in chat, adds an approval confirmation step to the swap flow (EVM-specific), removes nonce support across EVM/swap transaction code, refactors swap UI with new helper components, and slightly adjusts asset scoring for native tokens. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/agentic-chat/src/hooks/useSwapExecution.tsx (1)
326-336: Fix incorrect completed steps for non-EVM approval flows.The final state unconditionally marks APPROVAL_CONFIRMATION as complete whenever
needsApprovalis true, but APPROVAL_CONFIRMATION is only actually performed for EVM chains. Non-EVM chains skip this step (lines 269-273), so it should not be marked as complete in the final state.Apply this diff to fix:
const finalState: SwapState = { currentStep: SwapStep.COMPLETE, completedSteps: new Set([ SwapStep.QUOTE, SwapStep.NETWORK_SWITCH, - ...(needsApproval ? [SwapStep.APPROVAL, SwapStep.APPROVAL_CONFIRMATION] : []), + ...(needsApproval ? [SwapStep.APPROVAL] : []), + ...(needsApproval && chainNamespace === CHAIN_NAMESPACE.Evm ? [SwapStep.APPROVAL_CONFIRMATION] : []), SwapStep.SWAP, ]), ...(approvalTxHash && { approvalTxHash }), ...(swapTxHash && { swapTxHash }), }
🧹 Nitpick comments (2)
packages/utils/src/AssetService.ts (1)
73-76: Consider adding a comment to document the native token detection heuristic.The
/slip44:substring check is a reasonable heuristic for detecting native tokens based on the CAIP-19 format, where slip44 namespace indicates BIP-44 coin types (typically native chain assets). The small tiebreaker bonus (1001 vs 1000) is appropriate for subtle ranking preference.Consider adding a brief comment to clarify the intent:
+ // Native tokens use slip44 namespace in CAIP-19 format (e.g., eip155:1/slip44:60 for ETH) const isNative = asset.assetId.includes('/slip44:')apps/agentic-chat/src/hooks/useSwapExecution.tsx (1)
250-279: Consider simplifying the conditional structure.The approval confirmation logic correctly handles EVM-only receipt waiting, but the else-if branches (lines 269-278) perform identical operations and can be consolidated.
Apply this diff to simplify:
// Step 3: Approval Confirmation (EVM only) if (needsApproval && approvalTxHash && chainNamespace === CHAIN_NAMESPACE.Evm) { setState(draft => { draft.currentStep = SwapStep.APPROVAL_CONFIRMATION }) const publicClient = getPublicClient(wagmiConfig, { chainId: Number(chainReference) }) if (publicClient) { await publicClient.waitForTransactionReceipt({ hash: approvalTxHash as `0x${string}`, confirmations: 1, }) } setState(draft => { draft.completedSteps.add(draft.currentStep) draft.currentStep = (draft.currentStep + 1) as SwapStep draft.error = undefined }) - } else if (needsApproval) { - // Non-EVM: skip confirmation step - setState(draft => { - draft.currentStep = (draft.currentStep + 1) as SwapStep - }) } else { - // No approval needed: skip confirmation step + // Non-EVM or no approval: skip confirmation step setState(draft => { draft.currentStep = (draft.currentStep + 1) as SwapStep }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/agentic-chat/src/components/Chat.tsx(3 hunks)apps/agentic-chat/src/components/tools/InitiateSwapUI.tsx(6 hunks)apps/agentic-chat/src/hooks/useStreamPauseDetector.ts(1 hunks)apps/agentic-chat/src/hooks/useSwapExecution.tsx(6 hunks)packages/utils/src/AssetService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/utils/src/AssetService.ts (1)
packages/types/src/asset.ts (1)
asset(3-26)
apps/agentic-chat/src/hooks/useSwapExecution.tsx (2)
packages/caip/src/constants.ts (1)
CHAIN_NAMESPACE(46-54)apps/agentic-chat/src/lib/wagmi-config.ts (1)
wagmiConfig(19-19)
apps/agentic-chat/src/components/Chat.tsx (3)
apps/agentic-chat/src/providers/ChatProvider.tsx (1)
useChatContext(33-39)apps/agentic-chat/src/hooks/useStreamPauseDetector.ts (1)
useStreamPauseDetector(3-45)apps/agentic-chat/src/components/LoadingIndicator.tsx (1)
LoadingIndicator(1-11)
⏰ 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). (1)
- GitHub Check: main
🔇 Additional comments (11)
apps/agentic-chat/src/hooks/useSwapExecution.tsx (3)
25-32: LGTM!The SwapStep enum correctly adds APPROVAL_CONFIRMATION as step 3, maintaining sequential numbering and shifting subsequent steps accordingly.
82-84: LGTM!The persisted state tracking correctly handles the new APPROVAL_CONFIRMATION step with symmetric serialization and deserialization.
Also applies to: 124-126
397-403: LGTM!The steps array correctly includes the new APPROVAL_CONFIRMATION step with proper status tracking.
apps/agentic-chat/src/components/Chat.tsx (3)
17-17: LGTM!The updated suggestion text is clearer and consistent with the other suggestions.
29-40: LGTM!The memoization correctly extracts text content from the last assistant message, and the pause detection integration properly derives streaming state from the context status. The type assertion on line 35 is safe given the preceding filter.
103-103: LGTM!The loading indicator now correctly shows only when the stream is paused rather than during the entire loading period, providing better UX feedback.
apps/agentic-chat/src/hooks/useStreamPauseDetector.ts (1)
3-45: LGTM!The pause detection logic correctly manages the timer lifecycle with proper cleanup. The effect resets the pause state when content changes, sets a new timeout, and cleans up timers on unmount or dependency changes, preventing memory leaks and timer races.
apps/agentic-chat/src/components/tools/InitiateSwapUI.tsx (4)
35-44: LGTM!The steps destructuring and validation correctly handles the new 5-step swap flow including the approval confirmation step.
46-52: LGTM!The completed count calculation correctly includes all 5 steps and properly counts both COMPLETE and SKIPPED statuses.
65-97: LGTM!The helper components (UsdValue, SwapPair, BuyAmount) provide clean loading states and placeholders, improving code readability and UX consistency.
157-169: LGTM!The stepper correctly updates to 5 total steps and adds the "Confirming approval" step with proper connector positioning between approval and swap.
premiumjibles
force-pushed
the
bug-fix-bonanza
branch
from
2eded98 to
578b7ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/agentic-chat/src/components/Chat.tsx (1)
29-37: Consider using a type guard instead of type assertion.The cast on line 35 assumes
part.type === 'text'parts have atextproperty. While the filter ensures this, a safer approach would use a type predicate.- const lastMessageContent = useMemo(() => { - const assistantMessages = messages.filter(m => m.role === 'assistant') - const lastAssistantMessage = assistantMessages[assistantMessages.length - 1] - if (!lastAssistantMessage) return undefined - return lastAssistantMessage.parts - .filter(part => part.type === 'text') - .map(part => (part as { type: 'text'; text: string }).text) - .join('') - }, [messages]) + const lastMessageContent = useMemo(() => { + const assistantMessages = messages.filter(m => m.role === 'assistant') + const lastAssistantMessage = assistantMessages[assistantMessages.length - 1] + if (!lastAssistantMessage) return undefined + const isTextPart = (part: unknown): part is { type: 'text'; text: string } => + typeof part === 'object' && part !== null && 'type' in part && part.type === 'text' && 'text' in part + return lastAssistantMessage.parts.filter(isTextPart).map(part => part.text).join('') + }, [messages])apps/agentic-chat/src/components/tools/InitiateSwapUI.tsx (1)
68-97: Inline components recreated on every render—consider extracting or memoizing.
UsdValue,SwapPair, andBuyAmountare defined inside the render function, meaning they get new identities on each render. This can cause unnecessary DOM reconciliation and breaks React's component identity optimization.Either extract these as separate components outside
InitiateSwapUIor memoize them:+const UsdValue = ({ swap, isLoading }: { swap: InitiateSwapOutput['swapData'] | undefined; isLoading: boolean }) => { + if (swap?.buyAmountUsd) return <Amount.Fiat value={swap.buyAmountUsd} /> + if (isLoading) return <Skeleton className="h-5 w-16" /> + return <>—</> +} + +const SwapPair = ({ swap, isLoading }: { swap: InitiateSwapOutput['swapData'] | undefined; isLoading: boolean }) => { + if (swap) { + return ( + <TxStepCard.SwapPair + fromSymbol={swap.sellAsset.symbol.toUpperCase()} + toSymbol={swap.buyAsset.symbol.toUpperCase()} + /> + ) + } + if (isLoading) return <Skeleton className="h-7 w-40" /> + return <div className="text-lg font-semibold text-muted-foreground">—</div> +} + +const BuyAmount = ({ swap, isLoading }: { swap: InitiateSwapOutput['swapData'] | undefined; isLoading: boolean }) => { + if (swap) { + return ( + <TxStepCard.Amount> + <Amount.Crypto value={swap.buyAmountCryptoPrecision} symbol={swap.buyAsset.symbol.toUpperCase()} /> + </TxStepCard.Amount> + ) + } + if (isLoading) return <Skeleton className="h-7 w-32" /> + return <div className="text-lg font-semibold text-muted-foreground">—</div> +} + export function InitiateSwapUI({ toolPart }: ToolUIComponentProps) {Then use them with props:
<UsdValue swap={swap} isLoading={isLoading} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/agentic-chat/src/components/Chat.tsx(3 hunks)apps/agentic-chat/src/components/tools/InitiateSwapUI.tsx(6 hunks)apps/agentic-chat/src/hooks/useStreamPauseDetector.ts(1 hunks)apps/agentic-chat/src/hooks/useSwapExecution.tsx(6 hunks)apps/agentic-chat/src/hooks/useToolExecutionEffect.ts(0 hunks)apps/agentic-chat/src/utils/chains/evm/transaction.ts(0 hunks)apps/agentic-chat/src/utils/chains/types.ts(0 hunks)apps/agentic-chat/src/utils/swapExecutor.ts(0 hunks)packages/utils/src/AssetService.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/agentic-chat/src/utils/chains/evm/transaction.ts
- apps/agentic-chat/src/utils/chains/types.ts
- apps/agentic-chat/src/utils/swapExecutor.ts
- apps/agentic-chat/src/hooks/useToolExecutionEffect.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/agentic-chat/src/hooks/useStreamPauseDetector.ts
- packages/utils/src/AssetService.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/agentic-chat/src/components/Chat.tsx (3)
apps/agentic-chat/src/providers/ChatProvider.tsx (1)
useChatContext(33-39)apps/agentic-chat/src/hooks/useStreamPauseDetector.ts (1)
useStreamPauseDetector(3-45)apps/agentic-chat/src/components/LoadingIndicator.tsx (1)
LoadingIndicator(1-11)
apps/agentic-chat/src/hooks/useSwapExecution.tsx (1)
packages/caip/src/constants.ts (1)
CHAIN_NAMESPACE(46-54)
apps/agentic-chat/src/components/tools/InitiateSwapUI.tsx (3)
apps/agentic-chat/src/components/ui/Amount.tsx (1)
Amount(88-92)apps/agentic-chat/src/components/ui/skeleton.tsx (1)
Skeleton(7-7)apps/agentic-chat/src/components/ui/TxStepCard.tsx (1)
TxStepCard(149-162)
⏰ 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). (1)
- GitHub Check: main
🔇 Additional comments (10)
apps/agentic-chat/src/hooks/useSwapExecution.tsx (4)
24-31: LGTM! Enum values correctly reflect the new step sequence.The addition of
APPROVAL_CONFIRMATIONat step 3 and reordering of subsequent steps is consistent with the new workflow.
81-86: Verify ordering of persisted phases for approval flow.The
approval_confirmedphase is pushed afterapproval_complete, but theapproval_skippedcheck on line 84 usesSwapStep.APPROVALwhich is correct. However, if approval was completed but confirmation failed,approval_completewould be present withoutapproval_confirmed, which seems intentional for resumability.
307-315: LGTM! Final state correctly includes the new confirmation step.The spread operator conditionally adds both
SwapStep.APPROVALandSwapStep.APPROVAL_CONFIRMATIONwhen approval was needed, maintaining consistency with the updated workflow.
376-382: Steps array correctly exposes all 5 steps for UI consumption.The ordering matches the enum definition and the execution flow.
apps/agentic-chat/src/components/Chat.tsx (3)
17-17: Good catch on the typo fix."Gib" → "Give" improves user-facing text quality.
39-40: LGTM! Streaming state derivation is clear and correct.The
isStreamingboolean correctly captures bothsubmittedandstreamingstates, which aligns with theuseStreamPauseDetectorhook's expectation of an "active loading" boolean.
103-103: Verify intended UX: LoadingIndicator only shows during pauses.Previously, a loading indicator likely showed continuously during streaming. Now it only appears after
pauseThresholdMs(500ms default) of no content updates. Confirm this is the intended behavior—users won't see immediate feedback when streaming starts.apps/agentic-chat/src/components/tools/InitiateSwapUI.tsx (3)
35-44: LGTM! Step destructuring and guard check correctly updated.The addition of
confirmationStepand its inclusion in the null check ensures the UI handles the new 5-step workflow properly.
46-52: LGTM! Completed count calculation includes all steps.The filter correctly counts both
COMPLETEandSKIPPEDstatuses for progress tracking.
157-172: LGTM! Stepper correctly reflects the 5-step workflow.The
totalCount={5}and new "Confirming approval" step integrate properly with the updateduseSwapExecutionhook.
| // Step 3: Approval Confirmation (EVM only) | ||
| if (needsApproval && approvalTxHash && chainNamespace === CHAIN_NAMESPACE.Evm) { | ||
| setState(draft => { | ||
| draft.currentStep = SwapStep.APPROVAL_CONFIRMATION | ||
| }) | ||
|
|
||
| const publicClient = getPublicClient(wagmiConfig, { chainId: Number(chainReference) }) | ||
| if (publicClient) { | ||
| await publicClient.waitForTransactionReceipt({ | ||
| hash: approvalTxHash as `0x${string}`, | ||
| confirmations: 1, | ||
| }) | ||
| } | ||
|
|
||
| setState(draft => { | ||
| draft.completedSteps.add(draft.currentStep) | ||
| draft.currentStep = (draft.currentStep + 1) as SwapStep | ||
| draft.error = undefined | ||
| }) | ||
| } else { | ||
| // Non-EVM or no approval: skip confirmation step | ||
| setState(draft => { | ||
| draft.currentStep = (draft.currentStep + 1) as SwapStep | ||
| }) | ||
| } |
There was a problem hiding this comment.
Handle publicClient being undefined more explicitly.
When getPublicClient returns undefined, the code silently skips waiting for the receipt and proceeds. This could lead to the swap transaction being submitted before the approval is actually confirmed on-chain, potentially causing the swap to fail.
Consider throwing an error or retrying when the public client is unavailable:
const publicClient = getPublicClient(wagmiConfig, { chainId: Number(chainReference) })
- if (publicClient) {
- await publicClient.waitForTransactionReceipt({
- hash: approvalTxHash as `0x${string}`,
- confirmations: 1,
- })
+ if (!publicClient) {
+ throw new Error(`Unable to get public client for chain ${chainReference}`)
}
+ await publicClient.waitForTransactionReceipt({
+ hash: approvalTxHash as `0x${string}`,
+ confirmations: 1,
+ })📝 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.
| // Step 3: Approval Confirmation (EVM only) | |
| if (needsApproval && approvalTxHash && chainNamespace === CHAIN_NAMESPACE.Evm) { | |
| setState(draft => { | |
| draft.currentStep = SwapStep.APPROVAL_CONFIRMATION | |
| }) | |
| const publicClient = getPublicClient(wagmiConfig, { chainId: Number(chainReference) }) | |
| if (publicClient) { | |
| await publicClient.waitForTransactionReceipt({ | |
| hash: approvalTxHash as `0x${string}`, | |
| confirmations: 1, | |
| }) | |
| } | |
| setState(draft => { | |
| draft.completedSteps.add(draft.currentStep) | |
| draft.currentStep = (draft.currentStep + 1) as SwapStep | |
| draft.error = undefined | |
| }) | |
| } else { | |
| // Non-EVM or no approval: skip confirmation step | |
| setState(draft => { | |
| draft.currentStep = (draft.currentStep + 1) as SwapStep | |
| }) | |
| } | |
| // Step 3: Approval Confirmation (EVM only) | |
| if (needsApproval && approvalTxHash && chainNamespace === CHAIN_NAMESPACE.Evm) { | |
| setState(draft => { | |
| draft.currentStep = SwapStep.APPROVAL_CONFIRMATION | |
| }) | |
| const publicClient = getPublicClient(wagmiConfig, { chainId: Number(chainReference) }) | |
| if (!publicClient) { | |
| throw new Error(`Unable to get public client for chain ${chainReference}`) | |
| } | |
| await publicClient.waitForTransactionReceipt({ | |
| hash: approvalTxHash as `0x${string}`, | |
| confirmations: 1, | |
| }) | |
| setState(draft => { | |
| draft.completedSteps.add(draft.currentStep) | |
| draft.currentStep = (draft.currentStep + 1) as SwapStep | |
| draft.error = undefined | |
| }) | |
| } else { | |
| // Non-EVM or no approval: skip confirmation step | |
| setState(draft => { | |
| draft.currentStep = (draft.currentStep + 1) as SwapStep | |
| }) | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.