Recurring donation setup fix on project creation #4903
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a new interface Changes
Possibly Related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (11)
src/components/views/create/AlloProtocol/AlloProtocolModal.tsx (6)
35-36: Optional anchor contract fields are clear but consider defensive defaults.
Having these properties as optional is a good approach. However, you might consider assigning default empty objects or verifying them at runtime to avoid potential undefined reference errors in the UI.
94-108: Consider adding more explicit error handling.
When the transaction succeeds, you do a backend update, but if the mutation fails, the user may receive no feedback. You might handle the error scenario to surface issues more clearly (e.g., show escalation or rollback messaging).
149-160: Base anchor contract logic is functional but consider parallel execution.
If you anticipate multiple anchor registrations in quick succession, you might allow parallel calls or additional error handling. As it stands, the sequential approach is acceptable.
162-174: Optimism anchor contract logic mirrors Base logic; verify duplication.
You replicate the same code structure for Optimism. Factor out repeated logic if this pattern becomes more complex or if additional networks are introduced in the future.
186-188: Properties opAnchorReady and baseAnchorReady are straightforward.
No immediate issues, but consider clarifying naming or combining them if usage consistently checks both.
211-215: Conditional text for multi-network readies is clear.
This user-facing string logic is functional. Consider localizing both network names to ensure consistent i18n coverage.src/components/views/create/AddressInterface.tsx (1)
64-65: Use of DO_RECURRING_SETUP_ON_ENABLE is a clear approach to toggling logic.
This constant name reads well, but more descriptive naming might further clarify.src/apollo/types/types.ts (1)
38-43: New IAnchorContractBasicData interface is well-structured.
Defining optional properties with clarity is a good approach. Consider clarifying if enabled defaults to true or false, or ensuring it’s always provided by the consumer if needed.src/components/views/create/CreateProject.tsx (3)
97-97: Consider using a more descriptive constant name.The constant
DO_RECURRING_SETUP_ON_CREATIONis negatively defined (opposite ofisEditMode), which can be confusing. A more descriptive name likeSHOW_RECURRING_SETUP_ON_NEW_PROJECTwould better convey its purpose.-const DO_RECURRING_SETUP_ON_CREATION = !isEditMode; +const SHOW_RECURRING_SETUP_ON_NEW_PROJECT = !isEditMode;
390-392: Add explicit validation for anchor contracts.The
doAlloProtocolRegistrycheck might allow edge cases where both contracts are falsy. Consider adding explicit validation to ensure at least one contract is properly configured.-const doAlloProtocolRegistry = - baseAnchorContract || opAnchorContract; +const doAlloProtocolRegistry = Boolean(baseAnchorContract || opAnchorContract); + +// Add validation +if (doAlloProtocolRegistry && !baseAnchorContract && !opAnchorContract) { + showToastError(formatMessage({ id: 'label.anchor_contract_required' })); + setIsLoading(false); + return; +}Also applies to: 412-413
694-696: Add error handling for modal display.Consider adding error handling in case the modal fails to display, especially since it's crucial for the recurring donation setup.
-{showAlloProtocolModal && addedProjectState && ( +{showAlloProtocolModal && addedProjectState ? ( <AlloProtocolModal setShowModal={setShowAlloProtocolModal} addedProjectState={addedProjectState} project={project} baseAnchorContract={baseAnchorContract} opAnchorContract={opAnchorContract} /> +) : showAlloProtocolModal ? ( + <> + {showToastError(formatMessage({ id: 'error.modal_display_failed' }))} + {setShowAlloProtocolModal(false)} + </> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/apollo/types/types.ts(1 hunks)src/components/views/create/AddressInterface.tsx(6 hunks)src/components/views/create/AlloProtocol/AlloProtocolModal.tsx(8 hunks)src/components/views/create/CreateProject.tsx(7 hunks)src/components/views/create/types.tsx(3 hunks)
🔇 Additional comments (22)
src/components/views/create/AlloProtocol/AlloProtocolModal.tsx (10)
9-9: No issues found with new import.
The import statement for useSwitchChain is valid and follows convention.
18-22: Imports for IAnchorContractBasicData and related types are correctly added.
These imports align with the newly introduced anchor contract interface, ensuring type coherence with the rest of the application.
43-44: New optional parameters look good.
Providing optional parameters for drafts and anchor contracts increases flexibility in handling different states.
Also applies to: 49-50
119-120: Prop additions approved.
Including baseAnchorContract and opAnchorContract in the modal props captures multi-network usage effectively.
122-122: Switching chains on user action is appropriate.
This import and usage help in seamlessly changing networks.
146-148: Early return for undefined project state is fine, but consider user feedback.
You do return if addedProjectState is not available, which is correct. Confirm that the user receives a clear message indicating the missing project or incomplete data scenario.
222-222: Button labeling 'Confirm' is consistent.
No concerns; labeling is straightforward and resonates with typical user flows.
Line range hint 227-227: Conditional rendering is logically sound.
The code is easy to follow, straightforward, and aligns with your design.
Line range hint 236-239: Disabling conditions are appropriate.
Disabling the button when recurring donations are not ready or an anchor contract is already present prevents erroneous user submissions.
53-64: Verify partial anchorContract data before mutation.
If anchorContract is only partially populated, this could cause incomplete or unexpected backend updates. Ensure that mandatory fields (e.g., contractAddress) are present or validated prior to making this mutation.
src/components/views/create/AddressInterface.tsx (7)
28-32: Importing IAnchorContractBasicData is correct.
Integrating the new interface ensures consistent type usage for anchor contract data.
81-87: Reading alloProtocolRegistry from watch is correct.
Ensures the current state is always retrieved from the form’s context.
227-227: IconCheck for already enabled recurring donations is helpful.
This aids in visually confirming that the user’s configuration is intact.
236-239: Disable condition ensures user can’t re-enable anchor contract inadvertently.
Preventing multiple anchor contract setups for the same network is good UX.
260-267: saveAnchorContract usage is correct in edit mode.
No issues found. Good to see an immediate setHasAnchorContract call after a successful contract save.
268-288: Form-based approach for storing anchor objects is coherent.
You store the anchor contract data in the relevant form fields. This ensures consistency once you finalize the project.
295-303: Toggle switch logic for enabled is straightforward.
Appropriately toggling the .enabled property based on user action. Confirm that you handle partial anchor states if the user toggles on/off in quick succession.
src/components/views/create/types.tsx (3)
1-5: Importing IAnchorContractBasicData is appropriate.
Bringing in the new interface ensures the rest of your types align with anchor contract usage.
28-29: baseAnchorContract and opAnchorContract additions completed.
These new entries in EInputs effectively support multi-network anchor contracts.
64-65: Extended TInputs to handle anchor contracts.
Defines typed fields for anchor contracts, ensuring form data is validated.
src/components/views/create/CreateProject.tsx (2)
250-251: Verify anchor contract field usage across the codebase.
Let's ensure these new anchor contract fields are properly handled in related components.
Also applies to: 694-695
✅ Verification successful
Let me check the actual implementation and usage of these fields to ensure proper integration.
The anchor contract fields are properly integrated across the codebase
The verification shows that:
- The fields are properly typed in the interface definitions
- They are correctly handled in AlloProtocolModal for both Base and Optimism networks
- AddressInterface component properly manages the contract inputs
- The implementation includes proper type safety and network-specific handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage of anchor contract fields in other components
echo "Checking anchor contract field usage..."
rg -l "baseAnchorContract|opAnchorContract" src/
# Check AlloProtocolModal component for proper prop types
echo "Checking AlloProtocolModal prop types..."
ast-grep --pattern 'interface $modalProps {
$$$
baseAnchorContract?: $_;
opAnchorContract?: $_;
$$$
}'
Length of output: 539
Script:
#!/bin/bash
# Check the implementation details in the files
echo "Checking AlloProtocolModal implementation..."
rg -A 5 "baseAnchorContract|opAnchorContract" src/components/views/create/AlloProtocol/AlloProtocolModal.tsx
echo "\nChecking AddressInterface implementation..."
rg -A 5 "baseAnchorContract|opAnchorContract" src/components/views/create/AddressInterface.tsx
echo "\nChecking types definition..."
rg -A 5 "baseAnchorContract|opAnchorContract" src/components/views/create/types.tsx
Length of output: 2708
250-251: 🛠️ Refactor suggestion
Add type safety for anchor contract fields.
The baseAnchorContract and opAnchorContract fields lack type definitions. Consider adding them to the TInputs type to ensure type safety.
Let's verify the type definitions:
| if (project) { | ||
| if ( | ||
| isRecurringOnOptimismReady && | ||
| !isOnOptimism | ||
| ) { | ||
| switchChain?.({ | ||
| chainId: | ||
| config.OPTIMISM_NETWORK_NUMBER, | ||
| }); | ||
| } else if ( | ||
| isRecurringOnBaseReady && | ||
| !isOnBase | ||
| ) { | ||
| switchChain?.({ | ||
| chainId: | ||
| config.BASE_NETWORK_NUMBER, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Chain switching logic is correct, but watch out for UI feedback while switching.
Users might experience a slight delay. Providing a loading or progress indicator can improve clarity.
| false, | ||
| ); | ||
| } | ||
| setValue(alloContract, false); |
There was a problem hiding this comment.
💡 Codebase verification
Type mismatch confirmed when setting alloContract to false
The verification reveals a type mismatch issue. The IAnchorContractBasicData interface requires an object with recipientAddress and optional fields, but the code sets false as the value. This conflicts with the type definition. The correct usage is demonstrated elsewhere in the file where setValue(alloContract, { recipientAddress: ... }) is used.
- In
AddressInterface.tsx:181:setValue(alloContract, false)should be updated to match theIAnchorContractBasicDatatype - Consider setting it to
{ recipientAddress: walletAddress || value, enabled: false }to maintain type consistency
🔗 Analysis chain
setValue(alloContract, false) could conflict with the expected type.
Because alloContract is typed as IAnchorContractBasicData? or boolean, confirm that the form or downstream usage handles a boolean gracefully.
Let me gather more information about the setValue usage and the form context.
Let me check one more thing to understand the form field's usage pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking usage of the `alloContract` field across the codebase to confirm no type mismatch
rg -A 5 $'EInputs.opAnchorContract|EInputs.baseAnchorContract'
Length of output: 1776
Script:
#!/bin/bash
# Check the setValue usage and form context
ast-grep --pattern 'const { setValue } = useForm<$_>($_)'
# Also check the IAnchorContractBasicData type definition
rg "interface IAnchorContractBasicData" -A 10
Length of output: 609
Script:
#!/bin/bash
# Check the context around where alloContract is used in the form
rg -B 5 -A 10 "alloContract.*=.*watch\(" src/components/views/create/AddressInterface.tsx
# Also check where setValue is used with alloContract
rg -B 5 -A 5 "setValue\(.*alloContract" src/components/views/create/AddressInterface.tsx
Length of output: 781
There was a problem hiding this comment.
This LGTM ;) @mateodaza, but you decide will you do new changes about enabling recurring donation on this PR or on new ;)
|
@mateodaza looks like this PR is still set to use the toggles, I thought we discussed coming back with the buttons..
|
After discussing w @divine-comedian we gonna do a tiny change and bring back the enable for creation Thank you lots |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/views/create/AddressInterface.tsx (1)
183-183:⚠️ Potential issueType safety issue with anchor contract value.
Setting
alloContracttofalseconflicts with theIAnchorContractBasicDatatype.Update the code to maintain type safety:
-setValue(alloContract, false); +setValue(alloContract, { + recipientAddress: walletAddress || value, + enabled: false +});
🧹 Nitpick comments (2)
src/components/views/create/AlloProtocol/AlloProtocolModal.tsx (1)
42-46: Typo in parameter name.The parameter
ownerAddreshas a typo (missing 's'). This should be corrected for consistency.- ownerAddres, + ownerAddress,Also applies to: 51-54
src/components/views/create/AddressInterface.tsx (1)
238-241: Consider adding a tooltip for disabled state.The button can be disabled in two scenarios (no address or existing anchor contract), but there's no indication to the user why it's disabled.
+import { IconWithTooltip } from '@/components/IconWithToolTip'; -<Button - buttonType={isRecurringDonationsReady ? 'secondary' : 'texty-secondary'} - label={'Enable'} - disabled={!isRecurringDonationsReady || hasAnchorContract} - onClick={async () => { +<IconWithTooltip + icon={ + <Button + buttonType={isRecurringDonationsReady ? 'secondary' : 'texty-secondary'} + label={'Enable'} + disabled={!isRecurringDonationsReady || hasAnchorContract} + onClick={async () => { + /> + } + direction='top' +> + {!isRecurringDonationsReady + ? 'Add a recipient address to enable recurring donations' + : hasAnchorContract + ? 'Recurring donations are already enabled' + : ''} +</IconWithTooltip>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/views/create/AddressInterface.tsx(7 hunks)src/components/views/create/AlloProtocol/AlloProtocolModal.tsx(9 hunks)src/components/views/create/CreateProject.tsx(10 hunks)
🔇 Additional comments (1)
src/components/views/create/AlloProtocol/AlloProtocolModal.tsx (1)
Line range hint 261-297: Add loading state management for chain switching.
The chain switching operation could take time, but there's no loading state management. This could lead to a poor user experience.
Consider adding a loading state during chain switching:
+const [isSwitchingChain, setIsSwitchingChain] = useState(false);
const handleButtonClick = async () => {
try {
if (!project) return;
setIsLoading(true);
// Handle Base anchor contract
if (baseAnchorContract?.recipientAddress) {
+ setIsSwitchingChain(true);
switchChain?.({
chainId: config.BASE_NETWORK_NUMBER,
});
+ setIsSwitchingChain(false);
await saveAnchorContract({/*...*/});
}
} catch (error) {
console.error('Error:', error);
+ setIsSwitchingChain(false);
}
};
|
@divine-comedian @kkatusic this is now ready to check again :) - backend is updated |
Must point to PRODUCTION backend to work properly
Summary by CodeRabbit
New Features
Bug Fixes
Documentation