updated create-pool and implemented bulk addMember#318
updated create-pool and implemented bulk addMember#318iYukay wants to merge 9 commits intoGoodDollar:masterfrom
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
Welcome.tsx, the terms of use are rendered as two separate clickableTextelements with identicalonPresshandlers; consider consolidating this into a single clearly-labeled touch target to reduce duplication and improve accessibility/UX. - The
projectIdgeneration inCreatePoolProviderusesreplace(' ', '/'), which only replaces the first space; if multiple spaces are expected, consider using a global regex orsplit/jointo normalize the name consistently. - In
usePoolConfigurationValidation, the comparisonmembers.length > maximumMemberswill silently do nothing whenmaximumMembersis undefined; if this check should only run when a max is explicitly set, consider guarding it with an explicitmaximumMembers != nullto make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Welcome.tsx`, the terms of use are rendered as two separate clickable `Text` elements with identical `onPress` handlers; consider consolidating this into a single clearly-labeled touch target to reduce duplication and improve accessibility/UX.
- The `projectId` generation in `CreatePoolProvider` uses `replace(' ', '/')`, which only replaces the first space; if multiple spaces are expected, consider using a global regex or `split/join` to normalize the name consistently.
- In `usePoolConfigurationValidation`, the comparison `members.length > maximumMembers` will silently do nothing when `maximumMembers` is undefined; if this check should only run when a max is explicitly set, consider guarding it with an explicit `maximumMembers != null` to make the intent clearer.
## Individual Comments
### Comment 1
<location path="packages/app/src/hooks/useCreatePool/CreatePoolContext.tsx" line_range="105-113" />
<code_context>
return false;
}
+ const rawRecipients = form.poolRecipients?.trim() ?? '';
+ const memberAddresses = rawRecipients ? parseMemberAddresses(rawRecipients) : [];
+ if (rawRecipients) {
+ const memberError = validateMemberAddresses(memberAddresses);
+ if (memberError) {
+ console.error(memberError);
+ return false;
+ }
+ if (form.maximumMembers && memberAddresses.length > form.maximumMembers) {
+ console.error(`Members count (${memberAddresses.length}) exceeds maximum (${form.maximumMembers}).`);
+ return false;
</code_context>
<issue_to_address>
**suggestion:** Recipient validation logic is duplicated here and in `usePoolConfigurationValidation`, which risks drift.
Parsing and validating recipient addresses (including the `maximumMembers` length check) now happens both in this context and in `usePoolConfigurationValidation`. This duplication makes it easy for rules (regex, formatting, limits) to diverge. Consider centralizing this in a shared utility—building on `parseMemberAddresses`/`validateMemberAddresses`—and having both the hook and context call the same higher-level validator.
Suggested implementation:
```typescript
const sdk = new GoodCollectiveSDK(chainIdString, signer.provider as ethers.providers.Provider, { network });
const validatePoolRecipients = (
rawRecipients: string | undefined,
maximumMembers?: number,
): {
isValid: boolean;
memberAddresses: string[];
error?: string;
} => {
const trimmedRecipients = rawRecipients?.trim() ?? '';
if (!trimmedRecipients) {
return { isValid: true, memberAddresses: [] };
}
const memberAddresses = parseMemberAddresses(trimmedRecipients);
const memberError = validateMemberAddresses(memberAddresses);
if (memberError) {
return { isValid: false, memberAddresses, error: memberError };
}
if (maximumMembers && memberAddresses.length > maximumMembers) {
return {
isValid: false,
memberAddresses,
error: `Members count (${memberAddresses.length}) exceeds maximum (${maximumMembers}).`,
};
}
return { isValid: true, memberAddresses };
};
// Final form validation
```
```typescript
const recipientsValidation = validatePoolRecipients(form.poolRecipients, form.maximumMembers);
if (!recipientsValidation.isValid) {
if (recipientsValidation.error) {
console.error(recipientsValidation.error);
}
return false;
}
const { memberAddresses } = recipientsValidation;
```
To fully address the duplication with `usePoolConfigurationValidation`, you should:
1. Move `validatePoolRecipients` into a shared utility module (for example, `packages/app/src/utils/poolRecipients.ts`) so it is not tied to this context.
2. Update this file to import `validatePoolRecipients` from that shared module instead of defining it inline.
3. Update `usePoolConfigurationValidation` to call the shared `validatePoolRecipients` helper (and remove its local recipient parsing/validation logic), ensuring both the hook and this context share the same rules and error messages.
</issue_to_address>
### Comment 2
<location path="packages/app/src/hooks/useCreatePool/CreatePoolContext.tsx" line_range="150-156" />
<code_context>
// Calculate cycle length based on claim frequency
const cycleLengthDays = form.claimFrequency && form.claimFrequency <= 7 ? 7 : form.claimFrequency || 1;
+ const onlyMembers = form.joinStatus === 'closed';
const ubiSettings: UBISettings = {
claimPeriodDays: ethers.BigNumber.from(form.claimFrequency || 1),
minActiveUsers: ethers.BigNumber.from(1),
maxClaimAmount: ethers.utils.parseEther(String(form.claimAmountPerWeek || 0)),
maxMembers: form.maximumMembers || form.expectedMembers || 100,
- onlyMembers: form.poolRecipients ? form.canNewMembersJoin ?? false : false,
+ onlyMembers,
cycleLengthDays: ethers.BigNumber.from(cycleLengthDays),
claimForEnabled: false,
</code_context>
<issue_to_address>
**question:** Revisiting the `onlyMembers` logic now that it’s based solely on `joinStatus`.
Previously, `onlyMembers` also depended on `form.poolRecipients` being set, but now it’s purely `joinStatus === 'closed'`. With this change, a pool that is `closed` and has no initial members will still have `onlyMembers = true`, which may prevent anyone from ever joining. If that’s not the intended behavior, consider conditioning `onlyMembers` on both `joinStatus` and the presence of initial members, or clearly define and enforce the expected behavior for empty-member pools.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hi @L03TJ3 , I just made a video walkthrough showing the functional fixes |
L03TJ3
left a comment
L03TJ3
left a comment
There was a problem hiding this comment.
Homepage
-
homepage styles are not aligned with figma (no emphasising with bold text). also, test on mobile screens. the spacing of the words is weird.
-
the confirm terms of use should be a clickable link, not include the link (figma was just showing where the link should lead to. So: terms of use itself should be clickable, redirecting to > url
-
the checkbox should be confirmed before able to click on get started
create a pool screen
- the link is not clickable
|
Hi @L03TJ3 ,,,, |
|
Hey, sorry that I see this so late. but you produced a demo-video but not actually launched the pool? |
|
Hi @L03TJ3. I attempted to launch with multiple initial members, but the flow fails at bulk‑add with |
|
Okay. Lets do it like this. So we just have to test the bulk-add. you can pull in the changes of this pull-request (your other assigned item): #315 and include the changes here. |
|
Alright @L03TJ3 Makes sense |
|
I tested both bulk-add flows related to
Important Note on UBI Pools Results
Demo Video |
2 participants
Title: Create‑pool copy refresh + bulk add members
Summary:
Testing:
Summary by Sourcery
Refresh the create-pool flow copy and add support for specifying and bulk-adding initial pool members.
New Features:
Enhancements: