-
Notifications
You must be signed in to change notification settings - Fork 21
add: ui functionality for bulk add + deployed dev env contracts #315
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: master
Are you sure you want to change the base?
Conversation
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.
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.
packages/app/src/utils/validation.ts
Outdated
| if (directPaymentSettings.validEvents.length === 0) { | ||
| errors.validEvents = 'At least one valid event type is required'; | ||
| } | ||
| if (directPaymentSettings.rewardPerEvent.length !== settings.validEvents.length) { |
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.
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.
| </HStack> | ||
| <Text fontWeight="600">Wallet Addresses</Text> | ||
| <TextArea | ||
| placeholder="0xabc...123, 0xdef...456 0xghi...789" |
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.
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").
6c2b17c to
cceb0e2
Compare
Description
#314
Summary by Sourcery
Enable bulk member management in the app and update development Celo deployment configuration and contracts.
New Features:
Enhancements:
Build: