[MNY-231] SDK: Fix TransactionWidget not updating on currency prop change after initial render #8192
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 968913d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughRemoves currency/USD handling from the useTransactionDetails hook and its types. Moves fiat price lookup and formatted fiat display into TransactionPayment. Updates PaymentOverview and TransactionOverViewCompact to stop passing currency. Adds a patch changeset noting a bug fix for currency prop updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant TW as TransactionWidget
participant TP as TransactionPayment
participant Hook as useTransactionDetails
participant Price as tokenInfo.prices
U->>TW: Change fiat currency prop
TW->>TP: Re-render with new currency
TP->>Hook: useTransactionDetails({ client, transaction, wallet })
Hook-->>TP: { totalCost, txCostDisplay, tokenInfo }
TP->>Price: Lookup prices[currency]
Price-->>TP: fiatPricePerToken
TP->>TP: totalFiatCost = fiatPricePerToken * totalCost
TP->>TP: costToDisplay = formatCurrencyAmount(totalFiatCost) || txCostDisplay
TP-->>TW: Render updated costToDisplay
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 2
🧹 Nitpick comments (1)
packages/thirdweb/src/react/core/hooks/useTransactionDetails.ts (1)
45-48: Add required TSDoc @example and stability tag.As per coding guidelines: "Every public symbol must have comprehensive TSDoc with at least one compiling @example and a custom tag (@beta, @internal, @experimental, etc.)."
The current documentation lacks both an
@exampleand a stability tag. Consider enhancing it:/** * Hook to fetch comprehensive transaction details including contract metadata, * function information, cost calculations, and gas estimates. + * + * @example + * ```tsx + * const transactionQuery = useTransactionDetails({ + * client, + * transaction: preparedTx, + * wallet: activeWallet, + * }); + * + * if (transactionQuery.data) { + * console.log(transactionQuery.data.functionInfo.functionName); + * console.log(transactionQuery.data.txCostDisplay); + * } + * ``` + * + * @beta */Based on coding guidelines.
📜 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 (4)
.changeset/rich-pens-hug.md(1 hunks)packages/thirdweb/src/react/core/hooks/useTransactionDetails.ts(1 hunks)packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx(3 hunks)packages/thirdweb/src/react/web/ui/Bridge/payment-details/PaymentOverview.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- packages/thirdweb/src/react/web/ui/Bridge/payment-details/PaymentOverview.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/react/core/hooks/useTransactionDetails.tspackages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/react/core/hooks/useTransactionDetails.tspackages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/react/core/hooks/useTransactionDetails.tspackages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
.changeset/*.md: Each change inpackages/*must include a changeset for the appropriate package
Version bump rules: patch for non‑API changes; minor for new/modified public API
Files:
.changeset/rich-pens-hug.md
🧬 Code graph analysis (1)
packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx (1)
packages/thirdweb/src/react/web/ui/ConnectWallet/screens/formatTokenBalance.ts (1)
formatCurrencyAmount(37-42)
⏰ 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). (8)
- GitHub Check: Unit Tests
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Build Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx (2)
27-27: LGTM!The import of
formatCurrencyAmountis correctly added to support the new fiat cost formatting logic.
145-148: LGTM! This fix correctly addresses the reactivity issue.Moving the currency-dependent calculation into the component body ensures that
costToDisplayupdates whenever thecurrencyprop changes. The component will re-render and recalculate the fiat cost, solving the bug where "changing currency prop on TransactionWidget does not immediately update UI".The fallback to
txCostDisplaywhen fiat cost is unavailable is appropriate.Also applies to: 223-223
packages/thirdweb/src/react/core/hooks/useTransactionDetails.ts (2)
20-20: LGTM!The import correctly reflects the simplified hook scope, removing
formatCurrencyAmountsince currency formatting is now handled in the consuming component.
39-43: LGTM! This API simplification addresses the root cause.Removing currency handling from the hook is the correct fix. The previous implementation had two issues:
- Even if currency was passed to the hook, the
queryKeydidn't include it, so changing currency wouldn't trigger a refetch- Currency-specific formatting is a presentation concern, not a data-fetching concern
The new approach provides better separation: the hook fetches transaction data, and consuming components handle currency-specific display formatting. This makes the display reactive to currency changes without unnecessary refetches.
Also applies to: 49-53
size-limit report 📦
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8192 +/- ##
=======================================
Coverage 55.03% 55.04%
=======================================
Files 919 919
Lines 60562 60563 +1
Branches 4122 4125 +3
=======================================
+ Hits 33332 33338 +6
+ Misses 27126 27121 -5
Partials 104 104
🚀 New features to boost your workflow:
|
Merge activity
|
…ange after initial render (#8192) <!-- ## title your PR with this format: "[SDK/Dashboard/Portal] Feature/Fix: Concise title for the changes" If you did not copy the branch name from Linear, paste the issue tag here (format is TEAM-0000): ## Notes for the reviewer Anything important to call out? Be sure to also clarify these in your comments. ## How to test Unit tests, playground, etc. --> <!-- start pr-codex --> --- ## PR-Codex overview This PR focuses on fixing the `TransactionWidget` to ensure it updates correctly when the `currency` prop changes after the initial render. It also refines how the total cost and display values are calculated based on the selected currency. ### Detailed summary - Updated `PaymentOverview` to remove the `currency` prop from `TransactionOverViewCompact`. - Enhanced `TransactionPayment` to calculate `totalFiatCost` and `costToDisplay` based on the selected currency. - Adjusted the display of USD value to use `costToDisplay`. - Removed the `currency` type from `UseTransactionDetailsOptions`. - Simplified `useTransactionDetails` by removing currency-related calculations. > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}` <!-- end pr-codex --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Transaction widget now updates correctly when the selected currency changes after initial render. * Payment and transaction details display accurate fiat totals, with graceful fallback when pricing data is unavailable. * **Improvements** * Streamlined pricing display to focus on the selected currency, improving clarity and consistency across payment views. * **Chores** * Added a patch-level changeset entry documenting the bug fix. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
bot
force-pushed
the
mny-231
branch
from
93e1166 to
968913d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/thirdweb/src/react/core/hooks/useTransactionDetails.ts (1)
23-43: Breaking changes verified. No calls touseTransactionDetailspasscurrencyand no references tousdValueDisplayremain. Consider adding TSDoc with@exampleto the public interfacesTransactionDetailsandUseTransactionDetailsOptions.
📜 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 (4)
.changeset/rich-pens-hug.md(1 hunks)packages/thirdweb/src/react/core/hooks/useTransactionDetails.ts(1 hunks)packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx(3 hunks)packages/thirdweb/src/react/web/ui/Bridge/payment-details/PaymentOverview.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- packages/thirdweb/src/react/web/ui/Bridge/payment-details/PaymentOverview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/rich-pens-hug.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/react/core/hooks/useTransactionDetails.tspackages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/react/core/hooks/useTransactionDetails.tspackages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/react/core/hooks/useTransactionDetails.tspackages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx
🧬 Code graph analysis (1)
packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx (1)
packages/thirdweb/src/react/web/ui/ConnectWallet/screens/formatTokenBalance.ts (1)
formatCurrencyAmount(37-42)
🪛 GitHub Check: codecov/patch
packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx
[warning] 137-138: packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx#L137-L138
Added lines #L137 - L138 were not covered by tests
[warning] 140-143: packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx#L140-L143
Added lines #L140 - L143 were not covered by tests
[warning] 145-148: packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx#L145-L148
Added lines #L145 - L148 were not covered by tests
[warning] 223-223: packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx#L223
Added line #L223 was not covered by tests
⏰ 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). (4)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Size
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/thirdweb/src/react/web/ui/Bridge/TransactionPayment.tsx (4)
27-27: LGTM: Import added for currency formatting.The import is correctly sourced and used later in the component for fiat cost display.
100-104: LGTM: Hook call simplified.Removing the
currencyparameter aligns with the refactor to handle fiat calculations at the component level, enabling reactive updates when the currency prop changes.
137-148: LGTM: Fiat cost calculation logic is correct.The implementation correctly:
- Retrieves the fiat price for the selected currency from
tokenInfo.prices[currency]- Computes
totalFiatCostwhen both price and transaction data are available- Falls back to
txCostDisplaywhen fiat cost cannot be computedThis addresses the PR objective of making the display reactive to currency prop changes.
Note: Static analysis indicates these lines lack test coverage. Consider adding tests to verify:
- Correct fiat cost calculation when currency data is available
- Fallback behavior when
tokenFiatPricePerTokenis undefined- Currency switching updates the display correctly
223-223: LGTM: Display updated to show computed cost.The UI now displays
costToDisplay, which reactively updates based on thecurrencyprop, resolving the issue described in the PR objectives.packages/thirdweb/src/react/core/hooks/useTransactionDetails.ts (2)
20-20: LGTM: Import simplified.Removed
formatCurrencyAmountas currency formatting is no longer performed in this hook.
49-167: LGTM: Hook simplified by removing currency logic.The refactor successfully:
- Removes
currencyfrom the hook parameters- Eliminates USD-specific calculations and formatting
- Simplifies the hook's responsibility to transaction details only
- Maintains all other functionality intact
This enables components to handle currency-specific formatting independently, allowing reactive updates when the currency prop changes.
PR-Codex overview
This PR focuses on fixing the
TransactionWidgetto ensure it updates correctly when thecurrencyprop changes after the initial render.Detailed summary
PaymentOverviewto removecurrencyprop fromTransactionOverViewCompact.TransactionPaymentto calculate and display the total fiat cost based on the selectedcurrency.useTransactionDetailsto removecurrencyfrom options and related calculations.Summary by CodeRabbit
Bug Fixes
Improvements
Chores