-
Notifications
You must be signed in to change notification settings - Fork 9
Fix/transaction modal #554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughRefactors TransactionExamples layout into a flat stacked UI; switches action buttons to in-component onClick handlers that open TransactionToast/TransactionModal; wraps TransactionModal with VechainKitThemeProvider; changes modal/toast button variants and adds a refresh icon for error state. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
Files:
🧠 Learnings (1)📚 Learning: 2025-12-01T13:01:33.771ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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 |
packages/vechain-kit/src/components/TransactionModal/TransactionModal.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/vechain-kit/src/components/TransactionToast/TransactionToastContent.tsx (1)
192-198: Consider variant consistency across toast and modal close buttons.The close IconButton in the toast no longer has a variant specified, while the close Button in TransactionModalContent uses
variant="ghost"(line 210). If both should have consistent styling, consider addingvariant="ghost"here as well.<IconButton onClick={onClose} size="sm" borderRadius={'full'} aria-label="Close" icon={<Icon as={LuX} boxSize={4} />} + variant="ghost" />examples/homepage/src/app/components/features/TransactionExamples/TransactionExamples.tsx (1)
73-101: Consider simplifying the single-column SimpleGrid.The layout now uses
SimpleGridwithcolumns={{ base: 1, md: 1 }}, which always renders a single column. Since there's no responsive column variation, aVStackwould be simpler and more semantically appropriate.- <SimpleGrid columns={{ base: 1, md: 1 }} spacing={6}> + <VStack spacing={6} w="full"> <VStack spacing={4} p={6} borderRadius="md" bg="whiteAlpha.50"> <Text fontWeight="bold">Test Transactions</Text> ... </VStack> ... - </SimpleGrid> + </VStack>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/homepage/src/app/components/features/TransactionExamples/TransactionExamples.tsx(1 hunks)packages/vechain-kit/src/components/TransactionModal/TransactionModal.tsx(2 hunks)packages/vechain-kit/src/components/TransactionModal/TransactionModalContent.tsx(1 hunks)packages/vechain-kit/src/components/TransactionToast/TransactionToastContent.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
**/*.{ts,tsx}: In VeChain Kit Version 2, useuseThorinstead ofuseConnexfor contract interactions
For single contract read operations, use theuseCallClausehook with the pattern: import dependencies, define ABI and method as const, create query key function usinggetCallClauseQueryKeyWithArgs, and wrap with useCallClause including data transformation in the select option
For multiple parallel contract calls, use theexecuteMultipleClausesCallutility wrapped in auseQueryhook with the pattern: define query key function, useexecuteMultipleClausesCallin queryFn mapping items to clause objects, and transform results
For transaction building and sending, use theuseBuildTransactionhook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Always provide an arguments array for contract calls, even when no parameters are required - use an empty array for parameter-less functions to enable TypeScript type checking
Always conditionally enable queries using theenabledproperty to prevent unnecessary contract calls, checking for all required parameters:enabled: !!requiredParam && !!otherRequiredParam
Use theselectoption in useCallClause or transform data in queryFn to handle data transformation, particularly for converting BigInt values to strings and normalizing contract return data
Maintain consistent query key patterns: usegetCallClauseQueryKeyWithArgsfor contract calls with arguments andgetCallClauseQueryKeyfor calls without arguments to ensure proper caching and invalidation
Use TypeScriptas constassertions for method names andas0x${string}`` assertions for Ethereum addresses to ensure type safety in contract interactions
Files:
packages/vechain-kit/src/components/TransactionModal/TransactionModalContent.tsxpackages/vechain-kit/src/components/TransactionToast/TransactionToastContent.tsxpackages/vechain-kit/src/components/TransactionModal/TransactionModal.tsxexamples/homepage/src/app/components/features/TransactionExamples/TransactionExamples.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-01T13:01:33.771Z
Learnt from: CR
Repo: vechain/vechain-kit PR: 0
File: .cursor/rules/migration-guide-to-v2.mdc:0-0
Timestamp: 2025-12-01T13:01:33.771Z
Learning: Applies to **/*.{ts,tsx} : In VeChain Kit Version 2, use `useThor` instead of `useConnex` for contract interactions
Applied to files:
packages/vechain-kit/src/components/TransactionToast/TransactionToastContent.tsxpackages/vechain-kit/src/components/TransactionModal/TransactionModal.tsx
📚 Learning: 2025-12-01T13:01:33.771Z
Learnt from: CR
Repo: vechain/vechain-kit PR: 0
File: .cursor/rules/migration-guide-to-v2.mdc:0-0
Timestamp: 2025-12-01T13:01:33.771Z
Learning: Applies to **/*.{ts,tsx} : For transaction building and sending, use the `useBuildTransaction` hook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Applied to files:
examples/homepage/src/app/components/features/TransactionExamples/TransactionExamples.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint & Build
- GitHub Check: build-size-check
- GitHub Check: e2e-test
- GitHub Check: deploy
🔇 Additional comments (4)
packages/vechain-kit/src/components/TransactionModal/TransactionModalContent.tsx (1)
210-210: LGTM! Button variant change aligns with theme provider fix.The change from "vechainKitSecondary" to "ghost" variant for the Close button is a minor styling adjustment that likely improves consistency now that the modal is properly wrapped in the theme provider.
packages/vechain-kit/src/components/TransactionToast/TransactionToastContent.tsx (1)
14-20: LGTM! Icon enhancement improves error state UX.Adding the refresh icon for the error "Try again" action provides clear visual feedback that distinguishes retry from normal confirmation. Good UX improvement.
Also applies to: 166-173
packages/vechain-kit/src/components/TransactionModal/TransactionModal.tsx (1)
6-6: LGTM! Theme provider wrapper correctly addresses the PR objective.The TransactionModal is now properly wrapped with
VechainKitThemeProvider, ensuring it uses the kit's theme configuration. The implementation:
- Correctly extracts
darkModeandthemefromuseVeChainKitConfig- Preserves all existing props and behavior
- Maintains the
closeOnOverlayClicklogicThis fix ensures the modal displays consistently with the kit's theming system.
Also applies to: 36-57
examples/homepage/src/app/components/features/TransactionExamples/TransactionExamples.tsx (1)
130-154: LGTM! Modal and toast components correctly positioned.The
TransactionModalandTransactionToastare properly placed as siblings to the main content VStack, ensuring correct z-index layering. All required props are correctly passed, and the modal'suiConfigenables all features for demonstration purposes.The modal now benefits from the theme provider wrapper implemented in
TransactionModal.tsx.
examples/homepage/src/app/components/features/TransactionExamples/TransactionExamples.tsx
Show resolved
Hide resolved
|
Size Change: +1.69 kB (+0.03%) Total Size: 5.43 MB
ℹ️ View Unchanged
|
mikeredmond
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, lgtm
The issue was that the TransactionModal was not wrapped inside the kit's theme provider.
Registrazione.schermo.2025-12-16.alle.09.38.32.mov
Planning to create a separate pr with more enhancement to this, but for now I prefer to just fix it immediately
Closes #259
Summary by CodeRabbit
Enhancements
UI
✏️ Tip: You can customize this high-level summary in your review settings.