-
Notifications
You must be signed in to change notification settings - Fork 0
Feature implementation from commits 643773c..7e049ee #4
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
base: feature-base-branch-2
Are you sure you want to change the base?
Conversation
…dit_dApp_pt1 UX Audit dapp fix
… useResolve to handle optional details
…ull handling; update useInvoiceCreate to support optional details and network configuration
… fix useFetchTokens to handle network configuration correctly
@@ -152,7 +156,7 @@ export const useEscrowZap = ({ | |||
status, | |||
} = useSimulateContract({ | |||
chainId, | |||
address: networkConfig[chainId].ZAP_ADDRESS ?? '0x0', | |||
address: networkConfig?.ZAP_ADDRESS ?? '0x0', |
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.
🐛 Correctness Issue
Chain-Specific Address Resolution Removed.
This change removes chainId-specific address lookup which will cause the contract to use incorrect addresses or fallback to '0x0'
Current Code (Diff):
- address: networkConfig?.ZAP_ADDRESS ?? '0x0',
+ address: networkConfig[chainId]?.ZAP_ADDRESS ?? '0x0',
📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
address: networkConfig?.ZAP_ADDRESS ?? '0x0', | |
address: networkConfig[chainId]?.ZAP_ADDRESS ?? '0x0', |
prepareLoading || | ||
writeLoading || | ||
waitingForTx || | ||
!(details || !detailsLoading), |
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.
🐛 Correctness Issue
Complex Boolean Logic Creates Potential Loading State Bug.
The expression !(details || !detailsLoading) is unnecessarily complex and could lead to incorrect loading state determination which might cause UI inconsistencies.
Current Code (Diff):
- !(details || !detailsLoading),
+ !details && detailsLoading,
📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
!(details || !detailsLoading), | |
!details && detailsLoading, |
onError: error => { | ||
if (toast) errorToastHandler('useResolve', error, toast); | ||
}, |
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.
🐛 Correctness Issue
Missing error logging for null toast case.
When toast is null, errors are silently ignored without any logging, potentially hiding critical failures in production.
Current Code (Diff):
- onError: error => {
- if (toast) errorToastHandler('useResolve', error, toast);
+ onError: error => {
+ if (toast) errorToastHandler('useResolve', error, toast);
+ else logError('useResolve', error);
},
📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
onError: error => { | |
if (toast) errorToastHandler('useResolve', error, toast); | |
}, | |
onError: error => { | |
if (toast) errorToastHandler('useResolve', error, toast); | |
else logError('useResolve', error); | |
}, |
prepareLoading || | ||
writeLoading || | ||
waitingForTx || | ||
!(details || !detailsLoading), |
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.
🐛 Correctness Issue
Incorrect Loading State Logic.
The changed loading state logic using !(details || !detailsLoading) modifies the component's behavior and could cause UI inconsistencies.
Current Code (Diff):
- !(details || !detailsLoading),
+ detailsLoading,
📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
!(details || !detailsLoading), | |
detailsLoading, |
PR Summary
Enhance Hooks with Optional Parameters and Optimize Data Flow
Overview
This PR updates several hooks to support optional parameters, improves data flow optimization, and fixes potential null pointer exceptions. It also updates documentation URLs and implements a new useDebounce hook.
Change Types
Affected Modules
constants/src/config.ts
hooks/src/useDebounce.ts
hooks/src/useDetailsPin.ts
hooks/src/useEscrowZap.ts
hooks/src/useFetchTokens.ts
hooks/src/useInvoiceCreate.ts
hooks/src/useLock.ts
hooks/src/useResolve.ts
utils/src/resolvers.ts
Notes for Reviewers