add: ui functionality for bulk add + deployed dev env contracts#315
Open
add: ui functionality for bulk add + deployed dev env contracts#315
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
validatePoolSettings,UBIPoolSettingsis referenced but never imported/defined andsettings.validEventsshould likely bedirectPaymentSettings.validEvents, which will currently cause type or runtime errors. - The
poolTypediscriminator invalidatePoolSettingsuses the string'DirectPayments', which is inconsistent with the'DIRECT'/'UBI'values used elsewhere (e.g., inuseMemberManagement), so this branch may never execute as intended. - In
ManageCollectivePage, theTextAreaplaceholder uses the HTML entity for a newline; in this JSX/NativeBase context you likely want an actual newline ("\n"or{"\n"}) to avoid rendering the literal entity text.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validatePoolSettings`, `UBIPoolSettings` is referenced but never imported/defined and `settings.validEvents` should likely be `directPaymentSettings.validEvents`, which will currently cause type or runtime errors.
- The `poolType` discriminator in `validatePoolSettings` uses the string `'DirectPayments'`, which is inconsistent with the `'DIRECT'`/`'UBI'` values used elsewhere (e.g., in `useMemberManagement`), so this branch may never execute as intended.
- In `ManageCollectivePage`, the `TextArea` placeholder uses the HTML entity ` ` for a newline; in this JSX/NativeBase context you likely want an actual newline (`"\n"` or `{"\n"}`) to avoid rendering the literal entity text.
## Individual Comments
### Comment 1
<location> `packages/app/src/utils/validation.ts:37` </location>
<code_context>
+ if (directPaymentSettings.validEvents.length === 0) {
+ errors.validEvents = 'At least one valid event type is required';
+ }
+ if (directPaymentSettings.rewardPerEvent.length !== settings.validEvents.length) {
+ errors.rewardPerEvent = 'Must specify reward amount for each event type';
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The reference to `settings.validEvents` is likely a bug and should use `directPaymentSettings.validEvents` instead.
In `validatePoolSettings`, `settings` is not in scope, so this line will fail at type-check or runtime. This should compare `rewardPerEvent.length` to `directPaymentSettings.validEvents.length` instead.
</issue_to_address>
### Comment 2
<location> `packages/app/src/pages/ManageCollectivePage.tsx:426` </location>
<code_context>
- </HStack>
+ <Text fontWeight="600">Wallet Addresses</Text>
+ <TextArea
+ placeholder="0xabc...123, 0xdef...456 0xghi...789"
+ value={memberManagement.memberInput}
+ onChangeText={memberManagement.setMemberInput}
</code_context>
<issue_to_address>
**issue (bug_risk):** The use of ` ` in the placeholder likely won’t render as a newline in React Native and may show the raw entity.
Because React Native doesn’t parse HTML entities in JSX strings, this will likely render the literal ` ` instead of a newline. To get a line break in the placeholder, use an actual newline in a template string or `{"\n"}` between the examples (e.g., `"0xabc...123, 0xdef...456\n0xghi...789"`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sirpy
force-pushed
the
feat--add-bulk-members
branch
from
6c2b17c to
cceb0e2
Compare
13 tasks
Collaborator
|
@edehvictor Hey what is the status of this? |
|
Gm @L03TJ3, I've fixed it. |
Collaborator
|
@edehvictor did you test the PR? any comments or confirmation about the flow working? its a code-review + qa type request. |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The memoized
GoodCollectiveSDKis created even whenproviderorchainIdmight be undefined/invalid; consider guarding theuseMemo(or returningnulland checking before use) so that SDK construction doesn’t rely onas anyand fail at runtime during intermediate loading states. memberSuccessmessages are only cleared on add/remove actions and not when the input changes, which can leave stale success messages visible while the user is preparing a new operation; consider resettingmemberSuccesswhenmemberInputorparsedMemberAddresseschanges.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The memoized `GoodCollectiveSDK` is created even when `provider` or `chainId` might be undefined/invalid; consider guarding the `useMemo` (or returning `null` and checking before use) so that SDK construction doesn’t rely on `as any` and fail at runtime during intermediate loading states.
- `memberSuccess` messages are only cleared on add/remove actions and not when the input changes, which can leave stale success messages visible while the user is preparing a new operation; consider resetting `memberSuccess` when `memberInput` or `parsedMemberAddresses` changes.
## Individual Comments
### Comment 1
<location path="packages/app/src/hooks/managePool/useMemberManagement.ts" line_range="91-94" />
<code_context>
- const chainIdString = chainId.toString() as `${SupportedNetwork}`;
- const network = SupportedNetworkNames[chainId as SupportedNetwork];
+ const extraData = addressesToAdd.map(() => '0x'); // Empty bytes for extraData
- const sdk = new GoodCollectiveSDK(chainIdString, provider, { network });
-
- // Use SDK method to add members
- for (const addr of parsedMemberAddresses) {
- const tx = await sdk.addUBIPoolMember(signer, poolAddress, addr);
- await tx.wait();
- }
+ // Use the SDK's addPoolMembers function for bulk addition
+ const tx = await sdk.addPoolMembers(signer, poolAddress, addressesToAdd, extraData);
+ await tx.wait();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider filtering out already-managed members before calling addPoolMembers.
Currently all `addressesToAdd` are sent to `sdk.addPoolMembers`, and only after success are they merged into `managedMembers`. If the contract rejects duplicates or treats them as no-ops, this could cause a revert or unnecessary gas use. Consider pre-filtering `addressesToAdd` against `managedMembers` (using the same lowercasing) so only truly new addresses are passed to the contract.
</issue_to_address>
### Comment 2
<location path="packages/app/src/pages/ManageCollectivePage.tsx" line_range="424-434" />
<code_context>
- />
- </HStack>
+ <Text fontWeight="600">Wallet Addresses</Text>
+ <TextArea
+ autoCompleteType={''}
+ placeholder="0xabc...123, 0xdef...456 0xghi...789"
+ value={memberManagement.memberInput}
+ onChangeText={memberManagement.setMemberInput}
+ autoCapitalize="none"
+ borderRadius={8}
+ h={120} // Set a fixed height for the textarea
+ />
+ <Text fontSize="xs" color="gray.500">
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid using an empty string for autoCompleteType and adjust multiline placeholder formatting.
1) Instead of `autoCompleteType={''}`, omit the prop when you don’t want autocomplete; an empty string can conflict with the prop’s union type and cause platform warnings.
2) ` ` in the placeholder will render as literal text in React/React Native. If you want a visible line break, use a template literal or `{'
'}` in the string instead.
```suggestion
<Text fontWeight="600">Wallet Addresses</Text>
<TextArea
placeholder={`0xabc...123, 0xdef...456
0xghi...789`}
value={memberManagement.memberInput}
onChangeText={memberManagement.setMemberInput}
autoCapitalize="none"
borderRadius={8}
h={120} // Set a fixed height for the textarea
/>
<Text fontSize="xs" color="gray.500">
```
</issue_to_address>
### Comment 3
<location path="packages/app/src/hooks/managePool/useMemberManagement.ts" line_range="62" />
<code_context>
return null;
};
- const handleAddMembers = async () => {
+ const handleAddMembers = async (addressesToAdd: string[]) => {
setMemberError(null);
</code_context>
<issue_to_address>
**issue (complexity):** Consider keeping parsing and validation internal to the hook, centralizing status and SDK readiness handling to shrink the hook’s API and simplify its control flow.
You can reduce the hook’s surface area and control flow without changing behavior by tightening the boundary around parsing/validation and simplifying status + SDK handling.
### 1. Keep parsing/validation internal to the hook
`handleAddMembers` is always used with `parsedMemberAddresses`. You don’t gain real flexibility from the `addressesToAdd` argument + `validateAddresses(addresses: string[])`, but you do couple the caller to internal parsing.
You can:
- Make `validateAddresses` parameterless and work on `parsedMemberAddresses`.
- Make `handleAddMembers` parameterless and call `validateAddresses` + use `parsedMemberAddresses` internally.
- Optionally keep `parsedMemberAddresses` in the return only for display.
```ts
const validateAddresses = (): string | null => {
if (!parsedMemberAddresses.length) {
return 'Please enter at least one wallet address.';
}
const invalid = parsedMemberAddresses.find((addr) => !/^0x[a-fA-F0-9]{40}$/.test(addr));
if (invalid) {
return `Invalid wallet address: ${invalid}`;
}
return null;
};
const handleAddMembers = async () => {
clearStatus();
const error = validateAddresses();
if (error) {
setMemberError(error);
return;
}
if (!signer || !poolAddress || !pooltype || !provider) {
setMemberError('Pool management is not fully initialized.');
return;
}
if (pooltype !== 'UBI' && pooltype !== 'DIRECT') {
setMemberError('Member management is currently supported for UBI and Direct Payments pools only.');
return;
}
const addressesToAdd = parsedMemberAddresses;
try {
setIsAddingMembers(true);
const extraData = addressesToAdd.map(() => '0x');
const tx = await sdk.addPoolMembers(signer, poolAddress, addressesToAdd, extraData);
await tx.wait();
setTotalMemberCount((prev) => (prev ?? 0) + addressesToAdd.length);
setManagedMembers((prev) => {
const next = new Set(prev.map((a) => a.toLowerCase()));
addressesToAdd.forEach((a) => next.add(a));
return Array.from(next);
});
setMemberInput('');
setMemberSuccess(`Successfully added ${addressesToAdd.length} members.`);
} catch (e: any) {
setMemberError(e?.reason || e?.message || 'Failed to add members.');
setMemberSuccess(null);
} finally {
setIsAddingMembers(false);
}
};
```
And in the return value, if you don’t actually need to pass addresses into `handleAddMembers` from outside, you can avoid exporting `parsedMemberAddresses` or keep it clearly read-only:
```ts
return {
memberInput,
setMemberInput,
memberError,
memberSuccess,
isAddingMembers,
isRemovingMember,
managedMembers,
totalMemberCount,
handleAddMembers, // no args
handleRemoveMember,
parsedMemberAddresses, // optional, for display only
};
```
### 2. Factor out status clearing
You clear `memberError` and `memberSuccess` in multiple places. A tiny helper removes that noise and makes the intent explicit:
```ts
const clearStatus = () => {
setMemberError(null);
setMemberSuccess(null);
};
const handleAddMembers = async () => {
clearStatus();
// ...
};
const handleRemoveMember = async (member: string) => {
clearStatus();
// ...
};
```
### 3. Optional: centralize SDK readiness check
Right now you have:
- `sdk` memoized on `chainId` and `provider`
- Repeated guards on `!signer || !poolAddress || !pooltype || !provider`
You can keep memoization but centralize the readiness check to avoid duplicating the “not initialized” logic:
```ts
const getSdkOrFail = () => {
if (!signer || !poolAddress || !pooltype || !provider) {
throw new Error('Pool management is not fully initialized.');
}
return sdk;
};
const handleAddMembers = async () => {
clearStatus();
const error = validateAddresses();
if (error) {
setMemberError(error);
return;
}
if (pooltype !== 'UBI' && pooltype !== 'DIRECT') {
setMemberError('Member management is currently supported for UBI and Direct Payments pools only.');
return;
}
try {
setIsAddingMembers(true);
const sdkInstance = getSdkOrFail();
// use sdkInstance here
} catch (e: any) {
if (e.message === 'Pool management is not fully initialized.') {
setMemberError(e.message);
return;
}
// existing error handling
} finally {
setIsAddingMembers(false);
}
};
```
This keeps the new functionality (bulk add, success messages, DIRECT pools, memoized SDK) intact while reducing the hook’s API surface and internal branching.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
|
@edehvictor You request a review but what is tested or potentially fixed? |
|
@L03TJ3, sorry for the delay. I'm sending the fix, report and UI flow today |
|
Sorry for the delay! I've addressed the latest AI review comments and got the QA video recorded. Updates: ManageCollectivePage.tsx: Removed the empty autoCompleteType prop and fixed the \n literal rendering bug in the TextArea placeholder. Also fixed a UI bug so that only the specific member button spins when removing an address from the list. useMemberManagement.ts: Guarded the SDK useMemo against undefined providers, cleared stale success messages on input change, filtered out existing members before calling the contract to save gas, and refactored the hook logic as requested by the AI. I've attached a QA video demonstrating the UI flow (newline placeholder working, addresses parsing correctly, and the UI success states updating properly). Let me know if everything looks good to merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
#314
Summary by Sourcery
Add bulk member management support in the app and deploy updated development Celo contracts.
New Features:
Enhancements:
Build:
Deployment: